Globus app #986
Globus app #986
Conversation
aaschaer
requested review from
ada-globus,
derek-globus,
kurtmckee and
sirosen
as code owners
aaschaer
force-pushed
the
globus_app
branch
2 times, most recently
from
79e3777 to
b383e12
Compare
| return os.path.join(datadir, "globus", app_name, "tokens.json") | ||
| else: | ||
| return os.path.expanduser(f"~/.globus/{app_name}/tokens.json") |
There was a problem hiding this comment.
We should namespace these 1 layer deeper to avoid collisions w/ other locally run globus products.
Even today, if a user creates a GlobusApp with the name cli or sharing they'll be colliding with existing names.
What about ~/.globus/globus-app/{app_name}/tokens.json?
There was a problem hiding this comment.
Good catch! I think I like ~/.globus/app/{app_name}/tokens.json slightly more since removing the extra globus makes it shorter
There was a problem hiding this comment.
I'm happy with that (and similar change for the windows variety)
| *, | ||
| login_flow_manager: LoginFlowManager | None = None, | ||
| token_storage: TokenStorage | None = None, | ||
| refresh_tokens: bool = False, |
There was a problem hiding this comment.
| refresh_tokens: bool = False, | |
| request_refresh_tokens: bool = False, |
I suggest this because refresh_tokens could be read as an imperative of "please refresh my tokens" if a user isn't familiar enough with Globus or OAuth.
| self, | ||
| app_name: str, | ||
| *, | ||
| login_client: AuthLoginClient | None = None, |
There was a problem hiding this comment.
| login_client: AuthLoginClient | None = None, | |
| login_client: ConfidentialAppAuthClient | None = None, |
Is there a reason we wouldn't want to encode this runtime requirement in the type info?
| client_id: UUIDLike | None = None, | ||
| client_secret: str | None = None, | ||
| scope_requirements: dict[str, list[Scope]] | None = None, | ||
| config: GlobusAppConfig | None = None, |
There was a problem hiding this comment.
With config being immutable now, we should be able to set the default in the constructor.
| config: GlobusAppConfig | None = None, | |
| config: GlobusAppConfig = GlobusAppConfig() |
| tokens for long lived access. | ||
| """ | ||
|
|
||
| login_flow_manager: LoginFlowManager | type[LoginFlowManager] | None = None |
There was a problem hiding this comment.
This is an elegant solution to my initial thread.
I'm very slightly hesitant about having the constructor interface be mandated like this but I can live with it. If a user is subclassing their own LoginFlowManager, they're 90% going to be pre-instantiating it notgiving it to us to instantiate it.
Shortcut: https://app.shortcut.com/globus/story/30768/sdk-globusapp-constructs
Change Summary (all in experimental:
GlobusAppwhich definesadd_scope_requirements,run_login_flow, andget_authorizeras an interface for getting tokens that meet scope requirementsUserAppwhich implementsGlobusAppand has aLoginFlowManagerfor running interactive login flows.ClientAppwhich implementsGlobusAppGlobusAppConfig, which has its own story https://app.shortcut.com/globus/story/30772/sdk-globusappconfig. I'm not sure to what extent we'll want to expand this so I'm leaving that story as is for now. I mostly added it now becauserefresh_tokenssignificantly changes behavior ofUserApp, and being able to configure aMemoryTokenStorageis useful for testing.Scope.merge_scopesmethod for scope de-duplication. This isn't as comprehensive as what was originally specc'd (https://app.shortcut.com/globus/story/33823/scope-de-duplication) but avoids the issue of how to hash non-static ScopesNotes:
type: ignorecomments in places I feel should be correct but are still raising mypy errors, but if anyone knows a better way to handle that lmkJSONTokenStoragesomewhat arbitrarily, happy to change it if desired📚 Documentation preview 📚: https://globus-sdk-python--986.org.readthedocs.build/en/986/