LoginFlowManager and CommandLineLoginFlowManager

[aaschaer]

Shortcut: https://app.shortcut.com/globus/story/30763/sdk-loginmanagers-authrequirements

Two places of potential issue/improvement:

1. I'm using GlobusAuthorizationParameters as the input to run_login_flow instead of implementing AuthRequirements as originally spec'd. This mostly makes sense because that object has all the parameters needed for an authorization flow. It might be a bit fishy with types though. Making prompt a Literal["login] everywhere wasn't too painful, but currently any Scope objects will have to be serialized into strings despite that fact that they're just being passed through to oauth2_start_flow which accepts any ScopeCollectionType (which should have Scope added to it at some point). Making GlobusAuthorizationParameters accept ScopeCollectionType seemed to not line up well with the validation logic.

2. I've added type: ignore to my call to self.login_client.oauth2_start_flow as AuthLoginClient doesn't have a signature for oauth2_start_flow - presumably because it has different signatures across NativeAppAuthClient and ConfidentialAppAuthClient? Not sure if there is some abstract method magic that could be done here? Or maybe I should just make login_client's type NativeAppAuthClient | ConfidentialAppAuthClient, but that also feels wrong.

[kurtmckee]

This looks fine to me. It's wholly self-contained in this method and looks like it works just fine.

[sirosen]

I agree that this seems fine -- for this code, as a user of the client classes.

It strikes me as a "code/design smell in the clients themselves". I'd love to resolve it, but I think it's safely out of scope for this PR to even try to fix that issue. I'm not sure what the fix really is. (A new method on the base accepting *args and raising notimplemented? 🤔 )

[sirosen]

Some high-level review comments:

I'm using GlobusAuthorizationParameters as the input to run_login_flow instead of implementing AuthRequirements as originally spec'd.

This seems like a good "optimization" to me. Maintaining yet-another type which has borderline identical semantics is wasted effort if we can avoid it. And we have time before we promote this out of experimental to decide if it's causing us other harms.

Making GlobusAuthorizationParameters accept ScopeCollectionType seemed to not line up well with the validation logic.

👍 I would be against that, if avoidable. IMO those GARE-internal objects should be pretty close to the wire-format for the data after init. That keeps the mode of interaction consistent, in that you're expected to put JSON-serializable types in there. Allowing init to convert other types to JSON-serializable ones -- like ScopeCollection -> str -- is fair game.

I've added type: ignore to my call to self.login_client.oauth2_start_flow as AuthLoginClient doesn't have a signature for oauth2_start_flow - presumably because it has different signatures across NativeAppAuthClient and ConfidentialAppAuthClient? Not sure if there is some abstract method magic that could be done here?

Let's consider this noted but non-blocking for now. I agree, some 🪄 magic ✨ can probably fix this, but let's treat it separately. I'm not sure what kind of fix might be best, and it will probably involve some level of gamesmanship around backwards compatibility, typing, and abstract classes. Probably we'll do something that falls into that classic category of "backwards incompatible, but only if you're really strict about it".