Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Googlesheets expose the client directly #976

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

PiusKariuki
Copy link
Collaborator

Summary

Expose googlesheets client for more flexible job writing

Fixes #575

Details

Override the common fn function that takes the state and the sheetsClient as arguments in the callback

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to
know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our
Responsible AI Policy

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • If this is a new adaptor, added the adaptor on marketing website ?
  • If this PR includes breaking changes, do we need to update any jobs in
    production? Is it safe to release?
  • Are there any unit tests?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.
  • Have you ticked a box under AI Usage?

Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking at this @PiusKariuki! I don't think the implementation looks quite right though

packages/googlesheets/src/Adaptor.js Outdated Show resolved Hide resolved
@@ -233,6 +233,30 @@ export function getValues(spreadsheetId, range, callback = s => s) {
};
}


/**
* Exposes the googlesheets client for more flexible job writing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't realised our docs for fn() were so bad 🙈

When we're done I'll do a pass on these docs (then raise an issue to fix it in common)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright sounds good

packages/googlesheets/test/index.js Show resolved Hide resolved
packages/googlesheets/src/Adaptor.js Show resolved Hide resolved
@josephjclark
Copy link
Collaborator

So I've tweaked the docs and, on reflection, made a small adjustment to the implementation (pass client.spreadsheets because that's the bit we want, and the docs for client itself are utterly useless!)

But I've hit a security problem - something I was worried about. If you console.log the client then the access token becomes visible:

image

This is a security issue. A malicious actor who can use but not see the access token could gain access to it. Maybe a bit of a long shot but I'm uncomfortable with this.

I'll have to sleep on it!

@PiusKariuki
Copy link
Collaborator Author

Oh yeah that's no good. Best we think on it see if we can find a workaround.

@josephjclark
Copy link
Collaborator

@PiusKariuki can you investigate this a bit for me? Please don't give it more than 3 hours. I was hoping this would be an easy thing really.

You'll need be able to run a simple googlesheets job locally with the CLI, for which you'll need an oauth access_token. There are some good docs for this on the gmail adaptor but I've never actually done it myself. Mtuchi may be able to help too.

Anyway you'll need to get a simple job working that can read from a google sheet of your creation.

Then you need to try and wrap the sheets client in a way that can't be accessed from job code. What I hope we can do is block off access externally, in job code, but still allow the client to read the credential to do its own thing.

You might be able to do this by wrapping the sheets client with a Proxy and denying access to context._options from outside. The wrapper would present context._options as undefined but hopefully inside the proxy the client can still get at the auth details.

@PiusKariuki
Copy link
Collaborator Author

@josephjclark these are some great ideas to start off of👏. I'll explore them and hopefully we get our solution.

@PiusKariuki
Copy link
Collaborator Author

@josephjclark I tried wrapping the client with a Proxy but that prevents it(the client) from accessing the auth details.

Here's another problem we'll have to think about too. The access_token is being passed as a Bearer token for the http requests. if the user logs the response of the http request they can see the Bearer token making all our masking useless

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

googlesheets: expose the client directly
2 participants