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

Allow scope names to be different from their value #641

Merged
merged 5 commits into from
Nov 7, 2022

Conversation

derek-globus
Copy link
Contributor

@derek-globus derek-globus commented Nov 3, 2022

  • Scope Names can be set explicitly in a ScopeBuilder
    • Idea behind this is that scope values (whether they should or shouldn't be) can have a different human-understandable name than their scope_value.
    • With this change, a ScopeBuilder instantiation can attach a more human understandable scope_name to a less human understandable scope_value to be formatted into either a urn or url
  • Introduced ScopeBuilder.scope_names property
    • Moved _sphinxext.py to use this instead of ScopeBuilder internal properties
  • Fixed SpecificFlowClient scope string
    • Proof that this is accurate:
      • The scope string has it's operation string replaced from - to _ here
      • An example get flow truncated call
      % globus-automate flow get f73633d2-9445-4558-8228-977f79d71971
      {
        ...trunc...
        "globus_auth_scope": "https://auth.globus.org/scopes/f73633d2-9445-4558-8228-977f79d71971/flow_f73633d2_9445_4558_8228_977f79d71971_user",
        ...trunc...
      }
      

Testing

  • Added unit test for re-named scope strings
  • Tested out the new SpecificFlowClient usage manually:
SpecificFlowClient("7edaaa8a-72cd-4491-b7bd-b1f6160f44db").scopes.user
'https://auth.globus.org/scopes/7edaaa8a-72cd-4491-b7bd-b1f6160f44db/flow_7edaaa8a_72cd_4491_b7bd_b1f6160f44db_user'

^ this scope string is verified accurate above

@derek-globus derek-globus marked this pull request as ready for review November 3, 2022 19:55
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.

The core of the change is good, but there are some minor tweaks I want here. Specifically, after #643, I think we can fixup the sphinx extension to avoid using some of the internal names which you've preserved.
I'm testing a fix to the sphinx extension now:

diff --git a/src/globus_sdk/_sphinxext.py b/src/globus_sdk/_sphinxext.py
index c92d894..243bb51 100644
--- a/src/globus_sdk/_sphinxext.py
+++ b/src/globus_sdk/_sphinxext.py
@@ -13,7 +13,7 @@ from sphinx.util.nodes import nested_parse_with_titles

 def _extract_known_scopes(scope_builder_name):
     sb = locate(scope_builder_name)
-    return sb._known_scopes
+    return sb._known_scope_names

It looks to me like this works.

(NB: I'm aware that there are no unit tests which enforce that the sphinx extension code handles objects correctly; maybe we should add some in the future.)

src/globus_sdk/scopes.py Outdated Show resolved Hide resolved
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.

Everything looks great, but we have one final fix to apply, to the sphinx extension.

See the following screenshot of docs for AuthScopes:
doc build, scopes page
image

Where did the duplicates come from? (I knew they were there, which is why I went to the doc build. 😉 )

A failing of the ScopeBuilder has been fixed. ScopeBuilder.scope_names fetches the classattr scopes in addition to the instance-assigned scopes. This was missing in the past mechanism.
The scopes were being explicitly listed with add_scopes=.... (I think the classattr list was added after the extension was written.)

The fast fix is to just do this:

 .. listknownscopes:: globus_sdk.scopes.AuthScopes
-    add_scopes=openid,email,profile
     example_scope=view_identity_set

The optional thing to add on here is to remove the add_scopes=... feature from the extension, but that can be done in a follow-up. I leave it to your discretion as the PR author whether you'd like us to do that in a separate PR or if you want to include it here.

src/globus_sdk/scopes.py Show resolved Hide resolved
src/globus_sdk/scopes.py Outdated Show resolved Hide resolved
src/globus_sdk/scopes.py Show resolved Hide resolved
@derek-globus
Copy link
Contributor Author

Good catch on AuthScopes Stephen, I generated the docs locally after adding scope_names but must've missed that one in my inspection.

I'll update that in a new rev.

@sirosen sirosen merged commit ee8a6d3 into globus:main Nov 7, 2022
@derek-globus derek-globus deleted the fix-specific-flow-scope-string branch April 22, 2024 16:17
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.

5 participants