Add new "Authorization" states and functions to authorize additional scopes. [ci full]#7306
Add new "Authorization" states and functions to authorize additional scopes. [ci full]#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
2 times, most recently
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? |
…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
735d3d7 to
ad06176
Compare
I agree, but not sure it should happen here? wdyt? Otherwise I think this seems ready to roll! |
|
/taskcluster ci full |
mhammond
changed the title
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
Agreed, this looks ready to roll to me. I had a couple questions/suggestions but I'm happy to merge as-is.
| this.inner.completeOauthScopeAuthorizationFlow(code, state) | ||
| this.tryPersistState() | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we add these or is it better to force Android to use the state machine? I don't have strong feelings either way, just wanted to ask the question.
| // On success, go directly to Connected — no InitializeDevice step needed since | ||
| // the device is already initialized. | ||
| (CompleteOAuthScopeAuthorizationFlow { .. }, CompleteOAuthFlowSuccess) => { | ||
| Complete(FxaState::Connected) |
There was a problem hiding this comment.
This is for another PR, but I think we should include the list of scopes as associated data for the Connect state. It's kind of weird that you went through a "state change" and ended up in the exact same state as before.
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.