feat(keys): Update key stretching to optionally use session token on password change #18317
Conversation
vbudhram
force-pushed
the
fxa-11060
branch
3 times, most recently
from
d0c140f to
929802b
Compare
vbudhram
changed the title
vbudhram
changed the title
| const { oldAuthPW, email } = form; | ||
|
|
||
| await customs.check(request, email, 'passwordChange'); | ||
| const sessionToken = request.auth.credentials; |
There was a problem hiding this comment.
I'd be interested in recording a metric here to monitor when this endpoint is called without a sessionToken.
| const mockMailer = mocks.mockMailer(); | ||
| const mockLog = mocks.mockLog(); | ||
| const mockRequest = mocks.mockRequest({ | ||
| credentials: { |
There was a problem hiding this comment.
Shouldn't a sessionToken be passed in here? Is it getting added in some way that's not obvious? I see the customs check below, and apparently this passes?
There was a problem hiding this comment.
Yea it isn't obvious, the credentials object will exist if there is a valid session token. Normally, Hapi populates it from the authorization header, but that gets skipped when doing unit tests.
| } | ||
| } catch (error) { | ||
| console.error('Failed to retrieve keys:', error); | ||
| // Ignore errors |
There was a problem hiding this comment.
Do we really want to suppress this?
There was a problem hiding this comment.
In my testing it could fail to retrieve keys if not yet connected to redis. It would get called again on after the setTimeout. Figured this was fine since its only test code.
dschom
left a comment
dschom
left a comment
There was a problem hiding this comment.
LGTM. I like this better!
Because
This pull request
/password/change/startroute to optionally use a session token and callcustoms.checkAuthenticatedIssue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-11060
Checklist
Other information (Optional)
@dschom I ended up making the session token optional in the backend, but updated all the front end bits to use session token. This seems like a reasonable approach since it won't require the FxA Python client to ugrade yet. I will file some tickets with what I found while working on this.