Skip to content

Commit

Permalink
AUT-1432: Fix secret key display in error state and tests
Browse files Browse the repository at this point in the history
This fixes a bug reported by QA where authenticator app secret
keys were not displayed following a validation error.

The fix ensures the secret will display both:

- where the code format is invalid and therefore fails
  the validation rules as set in setup-authenticator-app-validation.ts
- where the code is incorrect, as demonstrated by the verifyMfaCode
  service not returning a success

The related integration tests have also been updated to prevent
passing where the code is not included (the previous non-empty test
would always pass because the text included whitespace and line breaks)
  • Loading branch information
gtvj committed May 20, 2024
1 parent e972401 commit 71067bd
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ export function setupAuthenticatorAppPost(
);
return renderBadRequest(res, req, TEMPLATE, error, {
qrCode: req.session.user.authAppQrCodeUrl,
secretKey: req.session.user.authAppSecret,
secretKeyFragmentArray: splitSecretKeyIntoFragments(
req.session.user.authAppSecret
),
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import { body } from "express-validator";
import { validateBodyMiddleware } from "../../middleware/form-validation-middleware";
import { ValidationChainFunc } from "../../types";
import { Request } from "express";
import { containsNumbersOnly } from "../../utils/strings";
import {
containsNumbersOnly,
splitSecretKeyIntoFragments,
} from "../../utils/strings";

export function validateSetupAuthAppRequest(): ValidationChainFunc {
return [
Expand Down Expand Up @@ -47,6 +50,8 @@ const postValidationLocals = function locals(
): Record<string, unknown> {
return {
qrCode: req.session.user.authAppQrCodeUrl,
secretKey: req.session.user.authAppSecret,
secretKeyFragmentArray: splitSecretKeyIntoFragments(
req.session.user.authAppSecret
),
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ describe("Integration::setup-authenticator-app", () => {
let cookies: string;
let app: any;
let baseApi: string;
const AUTH_APP_SECRET: string = "MJRGA2KMETI7BEVNT33MOITMEQQUJMAQ";

before(async () => {
decache("../../../app");
Expand All @@ -33,7 +34,7 @@ describe("Integration::setup-authenticator-app", () => {
journey: {
nextPath: PATH_NAMES.CREATE_ACCOUNT_SETUP_AUTHENTICATOR_APP,
},
authAppSecret: "secret",
authAppSecret: AUTH_APP_SECRET,
};

next();
Expand Down Expand Up @@ -89,7 +90,7 @@ describe("Integration::setup-authenticator-app", () => {
expect($("#code-error").text()).to.contains(
"Enter the security code shown in your authenticator app"
);
expect($("#secret-key").text()).to.not.be.empty;
expect($("#secret-key").text()).to.contain(AUTH_APP_SECRET);
})
.expect(400, done);
});
Expand All @@ -108,7 +109,7 @@ describe("Integration::setup-authenticator-app", () => {
expect($("#code-error").text()).to.contains(
"Enter the security code using only 6 digits"
);
expect($("#secret-key").text()).to.not.be.empty;
expect($("#secret-key").text()).to.contain(AUTH_APP_SECRET);
})
.expect(400, done);
});
Expand All @@ -127,7 +128,7 @@ describe("Integration::setup-authenticator-app", () => {
expect($("#code-error").text()).to.contains(
"Enter the security code using only 6 digits"
);
expect($("#secret-key").text()).to.not.be.empty;
expect($("#secret-key").text()).to.contain(AUTH_APP_SECRET);
})
.expect(400, done);
});
Expand Down

0 comments on commit 71067bd

Please sign in to comment.