Notify push and email on code exchanges #2985
Conversation
ghost
added
the
| @@ -114,7 +114,7 @@ module.exports = (log, db, push) => { | |||
| deviceName = synthesizeName(deviceInfo) | |||
| } | |||
| if (credentials.tokenVerified) { | |||
| request.app.devices.then(devices => { | |||
| db.devices(credentials.uid).then(devices => { | |||
There was a problem hiding this comment.
Why was this needed? I wonder if we can find a way to fix request.app.devices so that we maintain the nice caching behaviour.
There was a problem hiding this comment.
it's problematic, https://github.com/mozilla/fxa-auth-server/issues/2992 was filed.
lib/routes/utils/oauth.js
Outdated
|
|
||
| const tokenVerify = await oauthdb.checkAccessToken({ | ||
| token: grant.access_token | ||
| }) |
There was a problem hiding this comment.
It seems like a bit of a code smell that you have to check the access_token here immediately after generating it; is this so that you can find out the uid in the grant_type=authorization_code case? I wonder if we could arrange for the oauth-server to return that information directly from /token rather than having to verify it again here.
There was a problem hiding this comment.
Yeah that would be nice :)
|
|
||
| module.exports = (config) => { | ||
| return { | ||
| path: '/v1/verify', |
There was a problem hiding this comment.
get rid of this, /token endpoint can provide the uid
lib/routes/utils/oauth.js
Outdated
| } | ||
|
|
||
| const account = await db.account(uid) | ||
| await mailer.sendNewDeviceLoginNotification(account.emails, account, emailOptions) |
There was a problem hiding this comment.
might have emails on sessionToken
There was a problem hiding this comment.
nope, no emails on sessionToken, just one email.
vladikoff
force-pushed
the
fenix-token-exchanges
branch
2 times, most recently
from
dc9bfa8
to
4bb82c1
Compare
vladikoff
force-pushed
the
fenix-token-exchanges
branch
from
4bb82c1
to
4087322
Compare
vladikoff
changed the title
vladikoff
force-pushed
the
fenix-token-exchanges
branch
from
4087322
to
9cf5bbe
Compare
There was a problem hiding this comment.
Thanks @vladikoff, this looks good except for the return of the refreshToken-vs-refreshTokenId issue.
lib/routes/utils/oauth.js
Outdated
|
|
||
| if (! credentials.refreshTokenId) { | ||
| // provide a refreshToken for the device creation below | ||
| credentials.refreshTokenId = grant.refresh_token; |
There was a problem hiding this comment.
This is mixing up refresh_token and refresh_token_id (that is, the unhashed and the hashed variants). You'll need to either hash refresh_token yourself here, or return an additional fxa-refreshTokenId field in the /token response.
There was a problem hiding this comment.
oh classic ... , updated , also updated the test
|
|
||
| // we set tokenVerified because the granted scope is part of NOTIFICATION_SCOPES | ||
| credentials.tokenVerified = true; | ||
| credentials.client = await oauthdb.getClientInfo(clientId); |
There was a problem hiding this comment.
We ought to be able to do a cached lookup here, but not for this PR; I filed https://github.com/mozilla/fxa-auth-server/issues/3006 as a followup.
vladikoff
force-pushed
the
fenix-token-exchanges
branch
from
9cf5bbe
to
2e25c45
Compare
|
|
||
| 'use strict'; | ||
|
|
||
| const encrypt = require('../../../fxa-oauth-server/lib/encrypt'); |
There was a problem hiding this comment.
Is this going to be OK at runtime in the docker images we build for production? I wonder if it would be less risky to just copy the code over here, it's only 3 lines worth.
There was a problem hiding this comment.
Yeah! we have the technology!
Fixes #2880
Fixes #2955