Add new "Authorization" states and functions to authorize additional scopes.#7306
Add new "Authorization" states and functions to authorize additional scopes.#7306mhammond wants to merge 1 commit intomozilla:mainfrom
Conversation
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
This looks pretty much correct to me, except I think there's one more step to do.
My main takeaway is that these state machines feel overly complicated. It seems like you should be able to add an extra event, add code in a corresponding switch case, and that's it. When we were doing the Android migration and using the state machine tester to verify the code, we were getting some value in exchange. Now, more and more I'm feeling like we could rework this and simplify it.
| FxaEvent::CheckAuthorizationStatus => Ok(CheckAuthorizationStatus), | ||
| FxaEvent::CallGetProfile => Ok(GetProfile), | ||
| FxaEvent::BeginOAuthScopeAuthorizationFlow { scopes, entrypoint } => { | ||
| Ok(BeginOAuthFlow { scopes, entrypoint }) |
There was a problem hiding this comment.
I think you'll need a new internal state for this since it should call begin_oauth_scope_authorization_flow() instead of begin_oauth_flow(). Ditto for the complete flow event.
state_machine/internal_machines/mod.rs should get some extra cases to map the new states to the right calls.
mhammond
force-pushed
the
push-mkxvwknxqksu
branch
from
11b1351 to
f2e57d0
Compare
…scopes. This supports desktop-like flows, where a user is already signed in to (say) Sync, but not wants to authorize "vpn" or "smartwindow". I used the term "Authorization" to try and reflect we aren't expected to "authenticate" here - we are already authenticated - we typically expect just a confirmation button. I'm not entirely convinced the distinction between "authorize" and "authenticate" is enough, so the names might be confusing - I started with something like "UpgradeScopes" etc. Regardless, better names welcome here. I've tried to keep the names consistent, which made them verbose. Better names welcome here too.
mhammond
force-pushed
the
push-mkxvwknxqksu
branch
from
f2e57d0 to
735d3d7
Compare
|
https://phabricator.services.mozilla.com/D293447 is the fenix side of this. I think I got the semantics correct for the new flow, but it's hard to know if it doesn't work for that reason or for an fxa reason - I know Lauren mentioned some work needs to be done on the fxa side for this to work. This PR however doesn't seem like a breaking change and should be safe even if it need tweaks - wdyt? |
2 participants
As @jonalmeida and I were chatting about. @bendk does this look correct to you? @LZoog and @skhamis for some context for our slack messages.
This supports desktop-like flows, where a user is already signed in to (say) Sync, but not wants to authorize "vpn" or "smartwindow".
I used the term "Authorization" to try and reflect we aren't expected to "authenticate" here - we are already authenticated - we typically expect just a confirmation button. I'm not entirely convinced the distinction between "authorize" and "authenticate" is enough, so the names might be confusing - I started with something like "UpgradeScopes" etc. Regardless, better names welcome here.
I've tried to keep the names consistent, which made them verbose. Better names welcome here too.
Pull Request checklist
[ci full]to the PR title.