[Globus] Remove the need for globus_sdk as a python dependency #337
Conversation
| self.log.info( | ||
| 'Logout: Revoked tokens for user "{}" services: {}'.format( | ||
| user.name, ','.join(state['tokens'].keys()) | ||
| ) | ||
| ) | ||
| state['tokens'] = '' | ||
| state['tokens'] = {} |
There was a problem hiding this comment.
This is a minor change, unrelated to removing the Globus SDK. Unit testing caught that it's possible to get tripped up by the type changing from dict to str, although in practice I think it would be difficult to get into a state where this caused an error. This change keeps types consistent, just in case.
| @@ -79,7 +96,7 @@ def _authorize_url_default(self): | |||
| ).tag(config=True) | |||
|
|
|||
| def _identity_provider_default(self): | |||
| return os.getenv('IDENTITY_PROVIDER', 'globusid.org') | |||
| return os.getenv('IDENTITY_PROVIDER', '') | |||
There was a problem hiding this comment.
This is another change unrelated to streamlining dependencies, but I think it's preferable. The previous value of globusid.org essentially locks down the default installation to an identity almost nobody uses. Setting this to the empty string allows the user to login with any IdP initially.
|
|
||
| def _allow_refresh_tokens_default(self): | ||
| return True | ||
|
|
There was a problem hiding this comment.
I think the last refactor pulled out the code that could request refresh tokens, so this feature is no longer usable. In order to request refresh tokens, an additional parameter needs to be sent through the first leg of the OAuth flow.
| for tok, v in tokens.by_resource_server.items() | ||
| if tok not in self.exclude_tokens | ||
| }, | ||
| 'tokens': by_resource_server, |
There was a problem hiding this comment.
The whole authenticate() method is very similar to the GenericOAuthenticator, where the only functional difference is the shape of tokens it returns. When the admin sets custom scopes, including scopes for third party resource servers secured with Globus Auth, the access/refresh tokens will come through the other_tokens key. If the admin didn't enable auth_state, this method behaves much the same as the GenericOAuthenticator
| auth_state = await mock_globus_user.get_auth_state() | ||
| assert auth_state == {'tokens': ''} | ||
| assert auth_state == {'tokens': {}} |
There was a problem hiding this comment.
Globus Test coverage is now 100%!
NickolausDS
force-pushed
the
streamline-globus-dependencies
branch
from
0435442
to
28b5dac
Compare
|
Rebased to include fixes for Generic tests #339. All tests pass! |
The Globus OAuthenticator currently requires a custom dependency in order to authenticate users. The Globus SDK was originally chosen for its convenience functions in generating URLs and automatically structuring tokens. However, it's not necessary and can be replaced with generic components included in this repo. With these changes, the globus_sdk can also be removed from the Zero to Jupyterhub 'hub' dependencies. While the globus_sdk is extremely useful for transferring data within the Single User Server, in the hub it is only used for the OAuthenticator.
NickolausDS
force-pushed
the
streamline-globus-dependencies
branch
from
28b5dac
to
b180ccc
Compare
|
Rebased on #343, which better organized the globus_sdk dependency install for Globus. These changes remove that dependency entirely to bring the Globus OAuthenticator more inline with how the other OAuthenticators work, instead of relying on oauth2 shortcuts within the globus_sdk. The implementation here is very similar to the Generic OAuthenticator, with some extensions to token handling to allow Globus Transfer tokens and any other third party tokens for user-created services. |
|
@NickolausDS I ran out of time for the moment to review the full PR, but I read through 50% and think everything has made sense so far. I also concluded that you are in this PR changing files in a way that will only influence globus. |
|
@consideRatio Correct, this only modifies Globus. #338 does affect this PR (it brings back refresh tokens), but I didn't want to add major changes outside of Thanks for reviewing! |
| # Default scopes are below if unspecified. Add a custom transfer server if you have one. | ||
| c.LocalGlobusOAuthenticator.scope = ['openid', 'profile', 'urn:globus:auth:scope:transfer.api.globus.org:all'] | ||
| c.GlobusOAuthenticator.scope = ['openid', 'profile', 'urn:globus:auth:scope:transfer.api.globus.org:all'] |
| @@ -113,7 +113,6 @@ def run(self): | |||
|
|
|||
| setup_args['extras_require'] = { | |||
| 'googlegroups': ['google-api-python-client==1.7.11', 'google-auth-oauthlib==0.4.1'], | |||
| 'globus': ['globus_sdk[jwt]>=1.0.0,<2.0.0'] | |||
consideRatio
changed the title
consideRatio
changed the title
The Globus OAuthenticator currently requires a custom dependency
in order to authenticate users. The Globus SDK was originally
chosen for its convenience functions in generating URLs and
automatically structuring tokens. However, it's not necessary
and can be replaced with generic components included in this
repo.
With these changes, the globus_sdk can also be removed from the
Zero to Jupyterhub 'hub' dependencies. While the globus_sdk is
extremely useful for transferring data within the Single User
Server, in the hub it is only used for the OAuthenticator.