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

Refactor: gdrive tools auth #2544

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Nov 21, 2024

PR Checklist

  • PR title descriptive (can be used in release notes)

Description

Noted on mattermost thread, the current system used to authorize access to google drive uses a credential that is encrypted within the scripts config folder. There is also a second set of credentials unencrypted in the gdrive-tools workspace.

This PR removes the encrypted credentials from scripts, in favour of the unencrypted credentials that already exist in the gdrive-tools workspace. In turn this removes the need to decrypt the scripts config workspace (code removed and main readme updated accordingly)

Author Notes

This should mean that any new author should be able to run the platform without the config private.key (still required deployment private.key if using encrypted deployments)

Existing users will likely be prompted to re-authenticate to google drive using the alternate unencrypted credential (no special requirements, just follow prompts when next syncing)

The old credentials private.key and token.json file in the packages/scripts/config are no longer needed, however I've kept the folder gitignored to prevent anyone accidentally committing the files (can safely be deleted and won't generate in future)

Review Notes

If checking out repo for the first time should be able to just import a deployment (with deployment private key if required) and then run sync (should auto-prompt authorization). Hopefully this is a smoother workflow than previous

Dev Notes

I've still kept the option to override credential paths from within the gdrive-tools workspace (in case we ever publish to npm and want to still keep local when calling from scripts), however for now I've removed all options from deployment config and scripts code for working with custom token and credential paths as I fail to see any use-case for them and it over-complicates the code somewhat.

I'm not sure exactly what the implications might be of using the unencrypted dev credentials, however I can't imagine there being much difference between platform developers using these and platform developers using the encrypted version (possibly google starts to limit credentials if a large number of people use them). In any case that issue would be better-served in the long term by registering as a verified google drive application and not rely on local app credentials

As an additional bit of tidying, I noticed the gdrive-tools workspace was originally setup to be called from cli, however also exposed direct bindings to methods too (lib). The scripts have been using lib bindings for a while now (refactored in #1552) as these support passing filter functions as a function and not stringified base64, so I've opted to remove the cli bindings as I don't think they serve much purpose.

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

@github-actions github-actions bot added documentation Improvements or additions to documentation scripts Work on backend scripts and devops maintenance Core updates, refactoring and code quality improvements and removed documentation Improvements or additions to documentation labels Nov 21, 2024
@istride
Copy link
Contributor

istride commented Nov 25, 2024

I strongly recommend that all credentials (encrypted or not) be removed from this repository as soon as possible - see advice from Google. The secrets for each client ID should be rotated.

The recommended way to authenticate is via Application Default Credentials, but this may need to be considered carefully and done as a separate PR.

I'd be happy to assist with all of the above.

@chrismclarke
Copy link
Member Author

I strongly recommend that all credentials (encrypted or not) be removed from this repository as soon as possible - see advice from Google. The secrets for each client ID should be rotated.

The recommended way to authenticate is via Application Default Credentials, but this may need to be considered carefully and done as a separate PR.

I'd be happy to assist with all of the above.

The problem with application default credentials is that every user would have to log onto the google apis dashboard and generate personal credentials, which is quite a technical barrier for users who just want to clone the repo and start syncing their drive content into the app builder.

I'm not really sure what the best option would be (seems like what we need is discouraged by google generally), but I'm guessing maybe one of the following might work:

  1. Get the scripts used to sync drive content for the app registered and approved as an app by google, so that users can just use their gmail to sign in without the need for app credentials.
  2. Setup a server-server workflow where we have a dedicated service account interacting with the drive sync scripts and users invite that service account access to their specific folder.
  3. Create an api/endpoint where users can either download locally the dev credentials (still shared, but not on git), or perhaps act as some sort of intermediary between the users and requests

There might be other workarounds, I'm really not up-to-date on google auth methods. Would be keen to hear what you discover and if you have any recommendations for how we can still have a running service without the credentials available

@chrismclarke
Copy link
Member Author

chrismclarke commented Nov 25, 2024

Looking more at the examples in https://developers.google.com/identity/protocols/oauth2/javascript-implicit-flow, I think it might still be possible to just use the client_id and omit the rest of the credentials. Given the examples directly embed this value in their HTML pages (so would be public facing) I assume this is considered non-sensitive and reasonable to do

Although at that point I'm guessing we would still want it deployed as a lightweight app/endpoint so that we can restrict domain access to it

@istride
Copy link
Contributor

istride commented Nov 25, 2024

Whatever happens regarding authentication, the existing credentials should be removed from this repo by this PR. Then, new credentials should be created and securely sent to those that need it. I'm assuming those that need credentials, at the moment, are in the IDEMS team or can be contacted directly.

I suggest a new issue be created to tackle the auth issue more generally, including whether Application Default Credentials could be part of the solution.

@chrismclarke
Copy link
Member Author

chrismclarke commented Nov 26, 2024

Whatever happens regarding authentication, the existing credentials should be removed from this repo by this PR. Then, new credentials should be created and securely sent to those that need it. I'm assuming those that need credentials, at the moment, are in the IDEMS team or can be contacted directly.

I suggest a new issue be created to tackle the auth issue more generally, including whether Application Default Credentials could be part of the solution.

Removing the credentials as part of this PR doesn't make sense given that this PR removes the alternative workflow that makes use of encrypted credentials. So it essentially would be breaking both methods and not providing any alternatives (no benefit to manually sharing credentials with developers when they can already access the same credentials using the private key to decrypt).

So I'll mark this PR as draft for now pending decisions and raise a separate PR to remove the dev credentials so any knock-ons can be better established. #2556

I'm still pretty sure application default credentials are no use here as they authenticate on behalf of the cloud project, and don't provide access to individual user's google drives which is what is required for syncing content. So most likely best option will be a standalone server app to handle authentication where we can embed the credentials directly, but can open up a separate issue to discuss #2557

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation maintenance Core updates, refactoring and code quality improvements scripts Work on backend scripts and devops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants