Closed rfk closed 5 years ago
Expose the code/token exchange endpoint on the auth-server and require that sync clients do their code exchange there rather than on the oauth-server, and have that route manage notifications as appropriate.
Hm if we have key-data there already, I guess we can do this!
Expose the code/token exchange endpoint on the auth-server and require that sync clients do their code exchange there rather than on the oauth-server, and have that route manage notifications as appropriate.
I lean towards this option too, esp after our discussion yesterday of some APIs, e.g., devices, growing OAuth legs.
There is a 3rd option, have the OAuth server create its own senders or email sender objects and send mail itself, though putting that on the OAuth server could be considered a step back?
There is a 3rd option, have the OAuth server create its own senders or email sender objects and send mail itself, though putting that on the OAuth server could be considered a step back?
Unfortunately this won't help with the push notification that we send to other connected devices.
Also a quick note from IRL discussion with vlad: we need to avoid sending two emails when signing in to the reference-browser via a normal enter-your-email-and-password flow (a naive implementation of this might risk sending one email when creating the sessionToken, and a second email when claiming the oauth code).
I did a bit of digging to try to summarize the current state of affairs.
The logic for sending our "a new device was connected to sync" email is here, with a bunch of conditional flags to ensure that we only send it if we did not send some other email (such as sign-in confirmation):
https://github.com/mozilla/fxa-auth-server/blob/master/lib/routes/account.js#L673
I can confirm that this does currently send an email when signing in to the reference browser from an IP address that is allowlisted to bypass signin confirmation:
There's also an interesting hack to send the new sign-in notification when logging in with TOTP, because TOTP defeats the conditional logic from above:
https://github.com/mozilla/fxa-auth-server/blob/master/lib/routes/totp.js#L363
This does not work with reference-browser, because it checks for service === "sync"
. If you sign in to reference-browser with TOTP, you will not get an email alert about the new login.
AFAICT these are the only two places where we currently send the "new device" notification email.
We send a "new device connected" push notification by calling push.notifyDeviceConnected
when creating a new device record here:
https://github.com/mozilla/fxa-auth-server/blob/master/lib/devices.js#L112
This can happen either because the client explicitly registers as a device, or because we create a "placeholder" device record for legacy devices because they request a signed certificate with service === "sync"
:
https://github.com/mozilla/fxa-auth-server/blob/master/lib/routes/sign.js#L89
We also call it (I think) after a sessionToken becomes confirmed, but only if the sessionToken has an existing device record:
https://github.com/mozilla/fxa-auth-server/blob/master/lib/routes/emails.js
I don't recall why this is necessary and the code doesn't really help me understand, but it's definitely a thing we do.
AFAICT these are the only two places where we send the "new device connected" push notification.
We do not currently send a push notification when signing in to reference-browser, because it doesn't register a device record and we don't create a placeholder one for it.
I don't recall why this is necessary and the code doesn't really help me understand, but it's definitely a thing we do.
On reflection, I think we do this to ensure that the push notification is not a lie - it gets sent only when the device actually becomes capable of syncing, and it doesn't get sent if you sign in to a device but never confirm the signin.
I don't think this is written down anywhere for reference, but to go with the summary of what we actually do, here's my recollection of our intended behavior:
/account/login
).@shane-tomlinson does this match your recollection?
I'm actually not sure if (3) is still the right thing, given that AMO and IAM are potentially quite sensitive even if they don't require access to your sync keys.
Based on all that, what I think we want in an oauth-sync world is approximately:
The "unless" part there seems a bit iffy and potentially hard to implement reliably. I wonder if a simple freshness check would suffice to begin with, e.g. send an email if their sessionToken.lastAuthAt
is more than X minutes in the past.
I don't think this is written down anywhere for reference,
Hey, we are no longer 🤡 's ... For this case.
@shane-tomlinson does this match your recollection?
Yes, it does, @vbudhram worked on a lot of that and might have something to add.
Whenever we issue a new OAuth token with scope "oldsync", create a placeholder device record for that token. This will trigger the push notification in the same way it currently does for sync logins.
This seems reasonable and might help with https://github.com/mozilla/fxa-auth-server/issues/2547#issuecomment-458959636. A new OAuth device would not have to register itself, but it could update its device record if it so chooses.
Whenever we issue a new OAuth token with scope "oldsync" (or maybe with any key-bearing scope?) we send a "new login to XYZ service" email, unless we already sent an email for some other reason during that login flow.
The "unless" part there seems a bit iffy and potentially hard to implement reliably. I wonder if a simple freshness check would suffice to begin with, e.g. send an email if their sessionToken.lastAuthAt is more than X minutes in the past.
If it's not reliable, it seems this could be solved step by step. Allow sending more than one initially, for example this would be the case for signin codes where the user receives an email with the signin code, and then one once the device is connected. Once we have that piece solved, move on to sending only one email, if we decide that's really what we want. Maybe when a sessionToken/refreshToken is created we can store additional data that says whether any email was sent during the token creation process. Yes, I just suggested modifying the sessionToken table. Yikes.
Allow sending more than one initially, for example this would be the case for signin codes where the user receives an email with the signin code, and then one once the device is connected.
That works if we're sending two different emails, but it might be weird if we send the same email twice (e.g. if the user is signing in to reference-browser from a newly-created account that's within the skipSigninConfirmationForNewAccounts window). Maybe acceptably weird?
(e.g. if the user is signing in to reference-browser from a newly-created account that's within the skipSigninConfirmationForNewAccounts window)
I'm hazy on how the email sending logic works, how would signing in here send the same email twice?
I'm hazy on how the email sending logic works, how would signing in here send the same email twice?
Once for the new sessionToken when they sign in to the account, thanks to [1]. And once for the new OAuth token that we create when reference-brower completes its signin flow, thanks to whatever code we add for this issue.
[1] https://github.com/mozilla/fxa-auth-server/blob/master/lib/routes/account.js#L673
@vladikoff is this for train-133, and if so, how "big" is it?
From the above discussion, it sounds like we can split this into two separate implementation issues - one for sending the emails, and one for doing the device-registration bits. I've created https://github.com/mozilla/fxa-auth-server/issues/2955 for the later and will morph the descirption of this bug to focus only on the former.
Edited 2019-03-12: Based on discussion below, I'm splitting this up into separate issues. Let's use this bug for sending a notification email and a different bug for sending the push notification, since I think they can be approached independently.
New scope for this bug:
When a new client connects to sync, we need to send an email to the user for visibility. This currently does not happen for clients that get sync access by requesting the "oldsync" OAuth scope with an existing session token. The simplest way to do the right thing here is to send the email at the point where the new client exchanges its OAuth code for tokens bearing the "oldsync" scope.
Requires #2954 or similar functionality, so that the code->token exchanges happens in a place where we can send an email.
Original issue description:
This is going to be a little thorny, but it's one of the things that motivated our work to combine auth-server and oauth-server, so let's figure it out...
When a new device connects to sync, we currently alert the user via two mechanisms:
These notifications are currently triggered during the creation of a new sessionToken. With the pairing flow a new device gets connected without creating a new sessionToken, we will instead have to trigger them when the supplicant does the OAuth code exchange and obtains its OAuth tokens.
I see two options, but perhaps y'all can suggest others.
1) In the oauth-server, when it does the code/token exchange for something requesting the "oldsync" scope, have it sent some backchannel notification to the auth-server to trigger the notifications. 2) Expose the code/token exchange endpoint on the auth-server and require that sync clients do their code exchange there rather than on the oauth-server, and have that route manage notifications as appropriate.
I lean towards (2) because it furthers the auth/oauth merger story, but it could require some updates to existing clients that are obtaining "oldsync" scoped tokens directly the oauth-server.
@vladikoff thoughts?