Add the ability to start a new OAuth flow while authenticated, so you can authorize additional scopes [ci full]#7306
Conversation
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.
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? |
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 |
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.
ad06176 to
80b7e80
Compare
|
I decided to stay with the same entry-points, and by adding a little extra data to a couple of the states I also avoided needing any more states. It also seems to correctly implement the merging of scopes and managing the device record. @bendk do you mind having another look? |
… can authorize additional scopes. This supports desktop-like flows, where a user is already signed in to (say) Sync, but not wants to authorize "vpn" or "smartwindow". Only the new scopes needed are requested rather than a union of the scopes. Therefore the new refresh token only has those new scopes, so we need to then calculate the full set and re-create a new refresh token. This does not attempt to handle reauth correctly, where somehow we will need to know the union of scopes we held before we entered the needs-reauth state and re-request them all. Also taking this opportunity to introduce the concept of "service" to the oauth flows.
80b7e80 to
0241cae
Compare
|
also has the quite cool ability to test most of this from the commandline - first log in with vpn (ie, not sync), then request access tokens for vpn and sync now upgrade to sync: |
|
this looks like it's not a breaking change for iOS. But probably will be after @skhamis's patch lands (or if this lands first, it will break that patch) |
bendk
left a comment
There was a problem hiding this comment.
This makes sense to me. I had a couple suggestions, but nothing needs re-review.
|
|
||
| if let Some(ref old_refresh_token) = old_refresh_token { | ||
| // We now have a new refresh token, but only with new scopes we explicitly requested. | ||
| // We possibly an old refresh token with only the scopes we had before. |
There was a problem hiding this comment.
| // We possibly an old refresh token with only the scopes we had before. | |
| // We possibly had an old refresh token with only the scopes we had before. |
| self.state.config(), | ||
| session_token, | ||
| &scopes_slice, | ||
| )?; |
There was a problem hiding this comment.
IIUC, this is a workaround to the fact that we don't want to re-request all the scopes when fetching the refresh token. In particular, re-requesting the sync scope creates a UI issue.
Are there downsides to this approach or is this okay to keep for the long term? The one downside I can see is that this maybe prevents us from revoking access to a scope, since the client can always get it back via the session token. Is that right?
I think the code is fine, but can you document what the current status and plan is?
|
|
||
| pub struct AuthenticatingStateMachine; | ||
| pub struct AuthenticatingStateMachine { | ||
| pub is_connected: bool, |
There was a problem hiding this comment.
How about using an enum to allow for more possibilities?
In fact, maybe we should add a variant for coming from the AuthIssues state so that we can go back to that state on cancellation. This would be a UX change though so that doesn't need to be in this PR, but I do like an enum in any case.
| BeginOAuthFlow(sequence<string> scopes, string entrypoint); | ||
| BeginPairingFlow(string pairing_url, sequence<string> scopes, string entrypoint); | ||
| BeginOAuthFlow(string service, sequence<string> scopes, string entrypoint); | ||
| BeginPairingFlow(string pairing_url, string service, sequence<string> scopes, string entrypoint); |
There was a problem hiding this comment.
This is a breaking change since the clients are going to need to send us the new field. I'm fine with that, but let's call it out in the changelog.
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.