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

GlobusApp login, logout, and login_required #1041

Merged
merged 6 commits into from
Sep 6, 2024

Conversation

derek-globus
Copy link
Contributor

@derek-globus derek-globus commented Sep 4, 2024

Closes #1036

Added

  • Added login(...), logout(...), and login_required(...) to the
    experimental GlobusApp construct.

    • login(...) initiates a login flow if:

      • the current entity requires a login to satisfy local scope requirements or
      • 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 believes
      a login is required to satisfy local scope requirements.

Removed

  • Made run_login_flow private in the experimental GlobusApp construct.
    Usage sites should be replaced with either app.login() or
    app.login(force=True).
    • Old Usage

      ```python
      app = UserApp("my-app", client_id="<my-client-id>")
      app.run_login_flow()
      ```
      
    • New Usage

      ```python
      app = UserApp("my-app", client_id="<my-client-id>")
      app.login(force=True)
      ```
      

📚 Documentation preview 📚: https://globus-sdk-python--1041.org.readthedocs.build/en/1041/

@@ -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
Copy link
Contributor Author

@derek-globus derek-globus Sep 4, 2024

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

Comment on lines 150 to 152
- ``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.
Copy link
Contributor Author

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:

docs/experimental/examples/oauth2/globus_app.rst Outdated Show resolved Hide resolved
docs/experimental/examples/oauth2/globus_app.rst Outdated Show resolved Hide resolved
docs/experimental/examples/oauth2/globus_app.rst Outdated Show resolved Hide resolved
docs/experimental/examples/oauth2/globus_app.rst Outdated Show resolved Hide resolved
Comment on lines 237 to 239
* `auth_params` are provided
* `force` is set to True
* `self.login_required()` evaluates to True
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest rev.

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

Rebased from main to resolve these changes against #1040

Copy link
Contributor

@ada-globus ada-globus left a comment

Choose a reason for hiding this comment

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

🙌

src/globus_sdk/experimental/globus_app/app.py Outdated Show resolved Hide resolved
src/globus_sdk/experimental/globus_app/app.py Outdated Show resolved Hide resolved
src/globus_sdk/experimental/globus_app/app.py Outdated Show resolved Hide resolved
src/globus_sdk/experimental/globus_app/app.py Outdated Show resolved Hide resolved
@derek-globus derek-globus merged commit ca3bb36 into globus:main Sep 6, 2024
15 checks passed
@derek-globus derek-globus deleted the login-logout branch September 6, 2024 15:45
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.

Add GlobusApp method to check login status
4 participants