-
Notifications
You must be signed in to change notification settings - Fork 38
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
GlobusApp login, logout, and login_required #1041
Conversation
@@ -72,7 +72,7 @@ def __init__( | |||
:param consent_client: An AuthClient to be used for consent polling. If omitted, | |||
dependent scope requirements are ignored during validation. | |||
""" | |||
self._token_storage = token_storage | |||
self.token_storage = token_storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially worth calling this out in the changelog as well. Wasn't sure whether it rose to that level
- ``auth_params`` a collection of additional auth parameters to customize the login. | ||
This allows for specifications such as, requiring that a user be logged in with an | ||
MFA token or rendering the authorization webpage with a specific message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to link out to another doc here expanding on what can be passed but the two candidates I looked at were:
- SDK Docs on GAREs which don't enumerate the list of valid options.
- Auth API Docs on Authorize Code Grant which includes additional items beyond those that go strictly into a GAREData object.
* `auth_params` are provided | ||
* `force` is set to True | ||
* `self.login_required()` evaluates to True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `auth_params` are provided | |
* `force` is set to True | |
* `self.login_required()` evaluates to True | |
* ``auth_params`` are provided, | |
* ``force`` is set to True, or | |
* ``self.login_required()`` evaluates to True. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like part of this change is missing? I can check others, but might be a little bit before I can swing back through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my mistake. I didn't see the list-form part of the suggestion here, just the "``" to replace "`" enveloping.
I can update to that style as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in latest rev.
e01a179
to
2c409e7
Compare
Rebased from main to resolve these changes against #1040 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
Closes #1036
Added
Added
login(...)
,logout(...)
, andlogin_required(...)
to theexperimental
GlobusApp
construct.login(...)
initiates a login flow if:auth_params
/force=True
is passed to the method.logout(...)
remove and revokes the current entity's app-associated tokens.login_required(...)
returns a boolean indicating whether the app believesa login is required to satisfy local scope requirements.
Removed
run_login_flow
private in the experimentalGlobusApp
construct.Usage sites should be replaced with either
app.login()
orapp.login(force=True)
.Old Usage
New Usage
📚 Documentation preview 📚: https://globus-sdk-python--1041.org.readthedocs.build/en/1041/