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

globus api --scope-string (for clients) #906

Merged
merged 5 commits into from
Dec 8, 2023

Conversation

derek-globus
Copy link
Contributor

@derek-globus derek-globus commented Dec 7, 2023

What?

  • The globus api <service> command now supports a --scope-string parameter.

    • If supplied, the CLI will enforce that any specified scope strings are included
      in consent requirements in addition to standard service scope requirements.

    • This parameter may be supplied multiple times to specify multiple scope strings.

    • This parameter is only supported in the context of Client Credentials-based authentication.
      (Client Credentials with GLOBUS_CLI_CLIENT_ID)

Why?

Provide users a path to interact with APIs not currently supported in the CLI which require specific scope consents (ie. auth's project management apis)

Testing

> export GLOBUS_CLI_CLIENT_ID="...redacted..."
> export GLOBUS_CLI_CLIENT_SECRET="...redacted..."
> globus api auth GET /v2/api/projects --scope-string 'urn:globus:auth:scope:auth.globus.org:manage_projects'

Comment on lines 262 to 264
"A scope string to consent to for this command (only supported for "
"confidential client usage). "
"Pass this option multiple times to specify multiple scopes."
Copy link
Member

Choose a reason for hiding this comment

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

Recommending removal of "for this command". I've rewrapped the lines as well.

Suggested change
"A scope string to consent to for this command (only supported for "
"confidential client usage). "
"Pass this option multiple times to specify multiple scopes."
"A scope string to consent to "
"(only supported for confidential client usage). "
"Pass this option multiple times to specify multiple scopes."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this based on the standup conversation yesterday around "we'll keep using this cached token which now has expanded consents"? Or is this more of just trying to make it read cleaner?

Copy link
Member

Choose a reason for hiding this comment

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

Only about readability -- "to consent to for" caught my eye.

Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

One minor question/refactoring option, but I like how tightly this is scoped.
Because it passes through the existing login manager mechanisms, I have fewer concerns about impact, upon review, than I voiced in earlier discussions.

tests/functional/test_api.py Outdated Show resolved Hide resolved
src/globus_cli/login_manager/manager.py Show resolved Hide resolved
@derek-globus derek-globus merged commit 6bbb1ed into globus:main Dec 8, 2023
19 checks passed
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.

3 participants