feat(recovery): fetch recovery key status with passwordForgotToken #17636

chenba · 2024-09-18T13:41:01Z

Because:

we want to allow the user to get their recovery key status with the 'passwordForgotToken' they received during the reset password flow

This commit:

adds passwordForgotToken to the allowed auth strategies for the /recoveryKey/exists auth-server endpoint
- updates the auth client to use the passwordForgotToken to access that endpoint
returns the recovery key hint along with the exists status
passes the hint in Setting when navigating post-reset

Fixes FXA-9398, FXA-7400

chenba · 2024-09-18T13:41:41Z

@vpomerleau I believe this is relevant to your interests.

vpomerleau · 2024-09-18T16:05:48Z

packages/fxa-auth-server/lib/routes/recovery-key.js

@@ -297,7 +297,7 @@ module.exports = (
        ...RECOVERY_KEY_DOCS.RECOVERYKEY_EXISTS_POST,
        auth: {
          mode: 'optional',


Since this now returns the recovery key hint which could contain sensitive data, we'd want to make sure this request always uses either a session token or a passwordForgotToken (= not optional)

Good catch. I think I'll keep it optional for backwards compat but only return a hint if the request's authenticated.

packages/fxa-auth-server/lib/routes/recovery-key.js

chenba · 2024-09-18T21:15:27Z

packages/fxa-auth-server/lib/routes/recovery-key.js

@@ -296,8 +296,10 @@ module.exports = (
      options: {
        ...RECOVERY_KEY_DOCS.RECOVERYKEY_EXISTS_POST,
        auth: {
-          mode: 'optional',


auth is no longer optional for this endpoint.

Should also update the swagger docs to reflect the change?

chenba · 2024-09-19T16:17:13Z

packages/fxa-auth-server/lib/routes/auth-schemes/hawk-fxa-token.js

+            token = await getCredentialsFunc(parsedHeader.id);
+          } catch (_) {
+            // we'll handle the empty token case below
+          }


Previously if getCredentialsFunc throws an error, that's the resulting error of the auth. This makes the error a bit more consistent, but it is a change in the API.

vpomerleau · 2024-09-19T17:05:34Z

packages/fxa-auth-server/lib/routes/recovery-key.js

+          strategies: [
+            'multiStrategySessionToken',
+            'multiStrategyPasswordForgotToken',
+          ],
        },
        validate: {
          payload: isA.object({


We could remove this email here since we're no longer allowing unauthenticated use of this endpoint?

We could, yes. I don't know how much clean up it would require. So, I could either file a follow-up jira or I can clean it up as part of this patch. Since this PR is blocking you, what do you think?

Maybe it is scope creep since it doesn't technically cause problems to keep email as an optional payload... Could perhaps be noted in the backbone reset password cleanup ticket (FXA-8267) since that's the only remaining place (that I can see) that attempts to check for recovery key with an email.

Follow up is probably fine. I don't see any risks with removing the email value since we should be the only ones using it on our frontends. I don't believe others are using the email value in the api, or this api at all lol.

I don't see any risks with removing the email value ...

Oh yeah, I'm not worried about breaking anything. It's just a matter of how much delay we want to tolerate to get the code as clean as we'd like.

Could perhaps be noted in the backbone reset password cleanup ticket (FXA-8267) since that's the only remaining place ...

I was more concerned with the auth-client still accepting a payload for the email (and whatever might be using it). I assume the Backbone stuff will be deleted wholesale at some point anyway.

vbudhram

@chenba Thanks! I don't see anything blocking or at least could not be updated when frontend bits land.

vbudhram · 2024-09-19T17:42:59Z

packages/fxa-auth-server/lib/routes/recovery-key.js

-        }
-        const exists = await db.recoveryKeyExists(uid);
+        const uid = request.auth.credentials.uid;
+        const recoveryKeyRecordWithHint = await db.getRecoveryKeyHint(uid);


nit: The variable name is pretty clear, but database function name is a little confusing. Maybe they could be named similar.

vbudhram · 2024-09-19T17:45:12Z

packages/fxa-auth-server/lib/server.js

@@ -401,6 +401,19 @@ async function create(log, error, config, routes, db, statsd, glean) {
    hawkFxAToken.strategy(makeCredentialFn(db.passwordChangeToken.bind(db)))
  );

+  server.auth.scheme(


Cool, I like this. I think a comment above might be helpful to explain how these are used.

vbudhram · 2024-09-19T17:48:17Z

packages/fxa-auth-server/lib/routes/recovery-key.js

+          strategies: [
+            'multiStrategySessionToken',
+            'multiStrategyPasswordForgotToken',
+          ],
        },
        validate: {
          payload: isA.object({


Follow up is probably fine. I don't see any risks with removing the email value since we should be the only ones using it on our frontends. I don't believe others are using the email value in the api, or this api at all lol.

vpomerleau

Thanks for your work on this! Just a few comments related to the email + docs

vpomerleau · 2024-09-19T17:29:46Z

packages/fxa-auth-server/lib/routes/recovery-key.js

@@ -296,8 +296,10 @@ module.exports = (
      options: {
        ...RECOVERY_KEY_DOCS.RECOVERYKEY_EXISTS_POST,
        auth: {
-          mode: 'optional',


Should also update the swagger docs to reflect the change?

vpomerleau · 2024-09-19T17:32:32Z

packages/fxa-auth-server/lib/routes/recovery-key.js

+          strategies: [
+            'multiStrategySessionToken',
+            'multiStrategyPasswordForgotToken',
+          ],
        },
        validate: {
          payload: isA.object({


Maybe it is scope creep since it doesn't technically cause problems to keep email as an optional payload... Could perhaps be noted in the backbone reset password cleanup ticket (FXA-8267) since that's the only remaining place (that I can see) that attempts to check for recovery key with an email.

vpomerleau · 2024-09-19T17:42:01Z

packages/fxa-settings/src/pages/ResetPassword/ConfirmResetPassword/container.tsx

+      const result: RecoveryKeyCheckResult =
+        await authClient.passwordForgotRecoveryKeyStatus(
+          passwordForgotToken,
+          email


I don't think we need to pass in the email since it's optional and won't be used by the endpoint

Because: - we want to allow the user to get their recovery key status with the 'passwordForgotToken' they received during the reset password flow This commit: - adds passwordForgotToken to the allowed auth strategies for the /recoveryKey/exists auth-server endpoint - removes the optional auth config on the endpoint - updates the auth client to use the passwordForgotToken to access that endpoint - returns the recovery key hint along with the exists status - passes the hint in Setting when navigating post-reset

chenba · 2024-09-20T14:08:18Z

Filed FXA-10441 as a follow-up.

chenba requested a review from a team as a code owner September 18, 2024 13:41

vpomerleau reviewed Sep 18, 2024

View reviewed changes

chenba marked this pull request as draft September 18, 2024 16:34

vpomerleau reviewed Sep 18, 2024

View reviewed changes

packages/fxa-auth-server/lib/routes/recovery-key.js Show resolved Hide resolved

chenba force-pushed the FXA-9398-forgot-token-auth branch from 9f54acc to 550b0cb Compare September 18, 2024 21:06

chenba commented Sep 18, 2024

View reviewed changes

chenba force-pushed the FXA-9398-forgot-token-auth branch 2 times, most recently from b2bfdf0 to b4916b7 Compare September 19, 2024 15:35

chenba marked this pull request as ready for review September 19, 2024 16:10

chenba commented Sep 19, 2024

View reviewed changes

vpomerleau reviewed Sep 19, 2024

View reviewed changes

vbudhram approved these changes Sep 19, 2024

View reviewed changes

vpomerleau reviewed Sep 19, 2024

View reviewed changes

chenba force-pushed the FXA-9398-forgot-token-auth branch from b4916b7 to 78963c1 Compare September 19, 2024 19:40

vpomerleau approved these changes Sep 19, 2024

View reviewed changes

chenba force-pushed the FXA-9398-forgot-token-auth branch from 78963c1 to 85e5b49 Compare September 19, 2024 20:24

chenba force-pushed the FXA-9398-forgot-token-auth branch from 85e5b49 to 4958413 Compare September 19, 2024 21:01

chenba merged commit 794bf57 into main Sep 19, 2024
25 checks passed

chenba deleted the FXA-9398-forgot-token-auth branch September 19, 2024 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(recovery): fetch recovery key status with passwordForgotToken #17636

feat(recovery): fetch recovery key status with passwordForgotToken #17636

chenba commented Sep 18, 2024 •

edited

Loading

chenba commented Sep 18, 2024

vpomerleau Sep 18, 2024

chenba Sep 18, 2024

chenba Sep 18, 2024

vpomerleau Sep 19, 2024

chenba Sep 19, 2024

vpomerleau Sep 19, 2024

chenba Sep 19, 2024

vpomerleau Sep 19, 2024

vbudhram Sep 19, 2024

chenba Sep 19, 2024 •

edited

Loading

chenba Sep 19, 2024

vbudhram left a comment

vbudhram Sep 19, 2024

vbudhram Sep 19, 2024

vbudhram Sep 19, 2024

vpomerleau left a comment

vpomerleau Sep 19, 2024

vpomerleau Sep 19, 2024

vpomerleau Sep 19, 2024

chenba commented Sep 20, 2024

feat(recovery): fetch recovery key status with passwordForgotToken #17636

feat(recovery): fetch recovery key status with passwordForgotToken #17636

Conversation

chenba commented Sep 18, 2024 • edited Loading

chenba commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenba Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpomerleau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenba commented Sep 20, 2024

chenba commented Sep 18, 2024 •

edited

Loading

chenba Sep 19, 2024 •

edited

Loading