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

feat: adds Globus GCS-sourced assets as a datasource #675

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jbottigliero
Copy link

@jbottigliero jbottigliero commented Dec 2, 2024

This pull request adds integration with Globus. Specifically, adding the ability to source data via HTTPS on a Globus Connect Server instance, authenticated using Globus Auth.

  • The credential_provider was created based off of the #src/util/google_oauth2.ts implementation and other OAuth-like credential providers (e.g. middleauth and ngauth).

The Globus credential provider uses PKCE for the authorization flow, which I've added a few basic utilities around 1.

@ravescovi has successfully deployed an instance of Neuroglancer configured with Globus using the proposed changes – much of this implementation is based on his initial work integrating with the Neuroglancer codebase.

We're looking forward to discussing the implementation and seeing what needs to be addressed to get this into the mainline!

Footnotes

  1. These utilities could be replaced with an external library (e.g. pkce-challenge) if that is preferred. It might be worth noting many of added methods were pulled from the Globus SDK for JavaScript directly.

* own Client ID from Globus and substitute it in.
* @see https://docs.globus.org/api/auth/developer-guide/#developing-apps
*/
GLOBUS_CLIENT_ID: JSON.stringify("f3c5dd86-8c8e-4393-8f46-3bfa32bfcd73"),
Copy link
Author

Choose a reason for hiding this comment

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

This default value is a Client ID that is managed by the Globus team.

It is something that could be distributed as part of the codebase or removed as a default and only referenced here as a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 from my side for at least not needing a fork to set a different value.

Copy link
Author

Choose a reason for hiding this comment

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

@joshmoore – that was my intent here, but I am a bit unsure this method works as intended...

I thought something like npm run build -- --define GLOBUS_CLIENT_ID=example was the intended use, but it looks like the NEUROGLANCER_CLI environment flag needs to be disabled in order for the incoming --define properties to be merged.

Running npm run build -- --env NEUROGLANCER_CLI=false ... encounters an error due to the .strict() usage in build_tools/cli.ts – I don't want to derail the addition of this functionality, but just wanted to make sure I had a better grasp on the change and how it is used.

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

Successfully merging this pull request may close these issues.

2 participants