Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip #17998

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

wip #17998

wants to merge 1 commit into from

Conversation

vpomerleau
Copy link
Contributor

Because:

  • We want to hook up the BackupCodeManager to the front end

This commit:

  • Add new /recoveryCodes/exists route
  • Add new handler method that uses the BackupCodeManager to retrieve the count of remaining codes
  • Add backup codes to account resolver
  • Update fxa-settings account model to include backup codes count

Closes #FXA-10231

Because

This pull request

Issue that this pull request solves

Closes: # (issue number)

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

Copy link
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vpomerleau I think this might be the first time we have used libs in the auth-server that uses the database. Below is a patch that loads the settings page with the additional details

diff --git a/packages/fxa-auth-server/lib/routes/recovery-codes.js b/packages/fxa-auth-server/lib/routes/recovery-codes.js
index b06dd9d515..4ba74c5d5f 100644
--- a/packages/fxa-auth-server/lib/routes/recovery-codes.js
+++ b/packages/fxa-auth-server/lib/routes/recovery-codes.js
@@ -4,6 +4,8 @@
 
 'use strict';
 
+import { AppConfig } from '../types';
+
 const errors = require('../error');
 const isA = require('joi');
 const validators = require('./validators');
@@ -12,7 +14,8 @@ const RECOVERY_CODES_DOCS =
   require('../../docs/swagger/recovery-codes-api').default;
 const {
   BackupCodeManager,
-} = require('../../../../libs/accounts/two-factor/src/');
+} = require('@fxa/accounts/two-factor');
+import { setupAccountDatabase } from '@fxa/shared/db/mysql/account';
 
 const RECOVERY_CODE_SANE_MAX_LENGTH = 20;
 
@@ -20,7 +23,15 @@ module.exports = (log, db, config, customs, mailer, glean) => {
   const codeConfig = config.recoveryCodes;
   const RECOVERY_CODE_COUNT = (codeConfig && codeConfig.count) || 8;
 
-  const backupCodeManager = Container.get(BackupCodeManager);
+  let backupCodeManager;
+  (async () => {
+    const appConfig = Container.get(AppConfig);
+    const kyselyDb = await setupAccountDatabase(appConfig.database.mysql.auth);
+    if (!Container.has(BackupCodeManager)) {
+      Container.set(BackupCodeManager, new BackupCodeManager(kyselyDb));
+    }
+    backupCodeManager = Container.get(BackupCodeManager);
+  })();
 
   // Validate backup authentication codes
   const recoveryCodesSchema = validators.recoveryCodes(
@@ -146,11 +157,11 @@ module.exports = (log, db, config, customs, mailer, glean) => {
       async handler(request) {
         log.begin('checkRecoveryCodesExist', request);
 
-        const { sessionToken, uid } = request.auth.credentials;
+        const { email, uid } = request.auth.credentials;
 
         await customs.check(
           request,
-          sessionToken.email,
+          email,
           'checkRecoveryCodesExist'
         );
 

We could probably improve this code a bit.

@@ -187,6 +192,10 @@ export const GET_ACCOUNT = gql`
exists
verified
}
backupCodes {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be hesistant to introduce a new term. These are technically recoveryCodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't certain here since in the libs and in the ux we call them backup codes...

async handler(request) {
log.begin('checkRecoveryCodesExist', request);

const { sessionToken, uid } = request.auth.credentials;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The credentials object is the sessionToken. You could do something like this

const { email, uid } = request.auth.credentials;

@vpomerleau vpomerleau force-pushed the FXA-10231 branch 3 times, most recently from f052cf0 to ca68a19 Compare November 13, 2024 18:14
import { BackupCodeManager } from '@fxa/accounts/two-factor';
import { setupAccountDatabase } from '@fxa/shared/db/mysql/account';

export async function getBackupCodeManager(): Promise<BackupCodeManager> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vbudhram This is proving difficult to mock for tests, wondering if there might be a better way to set this up 🤔

…g backup codes

Because:

* We want to hook up the BackupCodeManager to the front end

This commit:

* Add new /recoveryCodes/exists route
* Add new handler method that uses the BackupCodeManager to retrieve the count of remaining codes
* Add backup codes to account resolver
* Update fxa-settings account model to include backup codes count

Closes #FXA-10231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants