Skip to content

Commit

Permalink
Merge pull request #18154 from mozilla/FXA-10360
Browse files Browse the repository at this point in the history
task(auth): Update /totp/destroy to take an sms code
dschom authored Dec 21, 2024
2 parents 078339c + 1cc7d11 commit ddd64dc
Showing 9 changed files with 227 additions and 6 deletions.
2 changes: 1 addition & 1 deletion packages/fxa-auth-server/bin/key_server.js
Original file line number Diff line number Diff line change
@@ -241,7 +241,7 @@ async function run(config) {
otpCodeManager,
config.recoveryPhone
);
Container.set('RecoveryPhoneService', recoveryPhoneService);
Container.set(RecoveryPhoneService, recoveryPhoneService);

// The AccountDeleteManager is dependent on some of the object set into
// Container above.
5 changes: 4 additions & 1 deletion packages/fxa-auth-server/lib/metrics/glean/index.ts
Original file line number Diff line number Diff line change
@@ -307,7 +307,9 @@ export function gleanMetrics(config: ConfigType) {
sendError: createEventFn('two_step_auth_phone_code_send_error'),
complete: createEventFn('two_step_auth_phone_code_complete'),
},

twoStepAuthRemove: {
success: createEventFn('two_step_auth_remove_success'),
},
inactiveAccountDeletion: {
statusChecked: createEventFn('inactive_account_deletion_status_checked'),
firstEmailTaskRequest: createEventFn(
@@ -363,6 +365,7 @@ export const logErrorWithGlean = ({
| 'twoFactorAuthSetup'
| 'inactiveAccountDeletion'
| 'twoStepAuthPhoneRemove'
| 'twoStepAuthRemove'
>
];
funnelFns[event as keyof typeof funnelFns](request, {
82 changes: 82 additions & 0 deletions packages/fxa-auth-server/lib/metrics/glean/server_events.ts
Original file line number Diff line number Diff line change
@@ -3626,6 +3626,88 @@ class EventsServerEventLogger {
event,
});
}
/**
* Record and submit a two_step_auth_remove_success event:
* Event that indicates the user successfully remove two-step authentication
* Event is logged using internal mozlog logger.
*
* @param {string} user_agent - The user agent.
* @param {string} ip_address - The IP address. Will be used to decode Geo
* information and scrubbed at ingestion.
* @param {string} account_user_id - The firefox/mozilla account id.
* @param {string} account_user_id_sha256 - A hex string of a sha256 hash of the account's uid.
* @param {string} relying_party_oauth_client_id - The client id of the relying party.
* @param {string} relying_party_service - The service name of the relying party.
* @param {string} session_device_type - one of 'mobile', 'tablet', or ''.
* @param {string} session_entrypoint - Entrypoint to the service.
* @param {string} session_entrypoint_experiment - Identifier for the experiment the user is part of at the entrypoint.
* @param {string} session_entrypoint_variation - Identifier for the experiment variation the user is part of at the entrypoint.
* @param {string} session_flow_id - an ID generated by FxA for its flow metrics.
* @param {string} utm_campaign - A marketing campaign. For example, if a user signs into FxA from selecting a Mozilla VPN plan on Mozilla VPN's product site, then the value of this metric could be 'vpn-product-page'. The value has a max length of 128 characters with the alphanumeric characters, _ (underscore), forward slash (/), . (period), % (percentage sign), and - (hyphen) in the allowed set of characters. The special value of 'page+referral+-+not+part+of+a+campaign' is also allowed..
* @param {string} utm_content - The content on which the user acted. For example, if the user clicked on the (previously available) "Get started here" link in "Looking for Firefox Sync? Get started here", then the value for this metric would be 'fx-sync-get-started'. The value has a max length of 128 characters with the alphanumeric characters, _ (underscore), forward slash (/), . (period), % (percentage sign), and - (hyphen) in the allowed set of characters..
* @param {string} utm_medium - The "medium" on which the user acted. For example, if the user clicked on a link in an email, then the value of this metric would be 'email'. The value has a max length of 128 characters with the alphanumeric characters, _ (underscore), forward slash (/), . (period), % (percentage sign), and - (hyphen) in the allowed set of characters..
* @param {string} utm_source - The source from where the user started. For example, if the user clicked on a link on the Mozilla accounts web site, this value could be 'fx-website'. The value has a max length of 128 characters with the alphanumeric characters, _ (underscore), forward slash (/), . (period), % (percentage sign), and - (hyphen) in the allowed set of characters..
* @param {string} utm_term - This metric is similar to the `utm.source`; it is used in the Firefox browser. For example, if the user started from about:welcome, then the value could be 'aboutwelcome-default-screen'. The value has a max length of 128 characters with the alphanumeric characters, _ (underscore), forward slash (/), . (period), % (percentage sign), and - (hyphen) in the allowed set of characters..
*/
recordTwoStepAuthRemoveSuccess({
user_agent,
ip_address,
account_user_id,
account_user_id_sha256,
relying_party_oauth_client_id,
relying_party_service,
session_device_type,
session_entrypoint,
session_entrypoint_experiment,
session_entrypoint_variation,
session_flow_id,
utm_campaign,
utm_content,
utm_medium,
utm_source,
utm_term,
}: {
user_agent: string;
ip_address: string;
account_user_id: string;
account_user_id_sha256: string;
relying_party_oauth_client_id: string;
relying_party_service: string;
session_device_type: string;
session_entrypoint: string;
session_entrypoint_experiment: string;
session_entrypoint_variation: string;
session_flow_id: string;
utm_campaign: string;
utm_content: string;
utm_medium: string;
utm_source: string;
utm_term: string;
}) {
const event = {
category: 'two_step_auth_remove',
name: 'success',
};
this.#record({
user_agent,
ip_address,
account_user_id,
account_user_id_sha256,
relying_party_oauth_client_id,
relying_party_service,
session_device_type,
session_entrypoint,
session_entrypoint_experiment,
session_entrypoint_variation,
session_flow_id,
utm_campaign,
utm_content,
utm_medium,
utm_source,
utm_term,
event,
});
}
}

/**
2 changes: 1 addition & 1 deletion packages/fxa-auth-server/lib/routes/recovery-phone.ts
Original file line number Diff line number Diff line change
@@ -28,7 +28,7 @@ class RecoveryPhoneHandler {
private readonly log: AuthLogger,
private readonly glean: GleanMetricsType
) {
this.recoveryPhoneService = Container.get('RecoveryPhoneService');
this.recoveryPhoneService = Container.get(RecoveryPhoneService);
}

async sendCode(request: AuthRequest) {
37 changes: 37 additions & 0 deletions packages/fxa-auth-server/lib/routes/totp.js
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@ const TOTP_DOCS = require('../../docs/swagger/totp-api').default;
const DESCRIPTION = require('../../docs/swagger/shared/descriptions').default;
const { Container } = require('typedi');
const { AccountEventsManager } = require('../account-events');
const { RecoveryPhoneService } = require('@fxa/accounts/recovery-phone');

const RECOVERY_CODE_SANE_MAX_LENGTH = 20;

@@ -35,6 +36,7 @@ module.exports = (log, db, mailer, customs, config, glean, profileClient) => {
promisify(qrcode.toDataURL);

const accountEventsManager = Container.get(AccountEventsManager);
const recoveryPhoneService = Container.get(RecoveryPhoneService);

return [
{
@@ -130,6 +132,16 @@ module.exports = (log, db, mailer, customs, config, glean, profileClient) => {
auth: {
strategy: 'sessionToken',
},
validate: {
payload: isA.object({
code: isA
.string()
.max(32)
.regex(validators.DIGITS)
.optional()
.description(DESCRIPTION.codeTotp),
}),
},
response: {},
},
handler: async function (request) {
@@ -151,6 +163,31 @@ module.exports = (log, db, mailer, customs, config, glean, profileClient) => {
throw errors.unverifiedSession();
}

// Will also use the recovery phone to validate the code.
const code = request.payload.code;
if (code) {
// Client has posted a code, but we need make it was a code that was actually
// sent to the client
const success = await recoveryPhoneService.confirmCode(uid, code);
if (!success) {
throw errors.invalidOrExpiredOtpCode();
}

// Next make sure the code hasn't expired
const token = await db.totpToken(sessionToken.uid);
const isValidCode = otpUtils.verifyOtpCode(code, token.sharedSecret, {
encoding: 'hex',
step: config.step,
window: config.window,
});
if (!isValidCode) {
throw errors.invalidOrExpiredOtpCode();
}

// Code seems legit. Record metric and proceed with totp deletion
glean.twoStepAuthRemove.success();
}

await db.deleteTotpToken(uid);

// Downgrade the session to email-based verification when TOTP is
2 changes: 2 additions & 0 deletions packages/fxa-auth-server/test/local/metrics/glean.ts
Original file line number Diff line number Diff line change
@@ -46,6 +46,7 @@ const recordTwoStepAuthPhoneCodeSentStub = sinon.stub();
const recordTwoStepAuthPhoneCodeSendErrorStub = sinon.stub();
const recordTwoStepAuthPhoneCodeCompleteStub = sinon.stub();
const recordTwoStepAuthPhoneRemoveSuccessStub = sinon.stub();
const recordTwoStepAuthRemoveSuccessStub = sinon.stub();
const recordPasswordResetTwoFactorSuccessStub = sinon.stub();
const recordPasswordResetRecoveryCodeSuccessStub = sinon.stub();
const recordInactiveAccountDeletionStatusCheckedStub = sinon.stub();
@@ -104,6 +105,7 @@ const gleanProxy = proxyquire('../../../lib/metrics/glean', {
recordTwoStepAuthPhoneCodeCompleteStub,
recordTwoStepAuthPhoneRemoveSuccess:
recordTwoStepAuthPhoneRemoveSuccessStub,
recordTwoStepAuthRemoveSuccess: recordTwoStepAuthRemoveSuccessStub,
recordPasswordResetTwoFactorSuccess:
recordPasswordResetTwoFactorSuccessStub,
recordPasswordResetRecoveryCodeSuccess:
8 changes: 5 additions & 3 deletions packages/fxa-auth-server/test/local/routes/recovery-phone.js
Original file line number Diff line number Diff line change
@@ -8,7 +8,10 @@ const chaiAsPromised = require('chai-as-promised');
const sinon = require('sinon');
const assert = { ...sinon.assert, ...chai.assert };
const { recoveryPhoneRoutes } = require('../../../lib/routes/recovery-phone');
import { RecoveryNumberNotSupportedError } from '@fxa/accounts/recovery-phone';
import {
RecoveryNumberNotSupportedError,
RecoveryPhoneService,
} from '@fxa/accounts/recovery-phone';

const { getRoute } = require('../../routes_helpers');
const { mockRequest } = require('../../mocks');
@@ -40,7 +43,7 @@ describe('/recovery-phone', () => {
let routes = [];

before(() => {
Container.set('RecoveryPhoneService', mockRecoveryPhoneService);
Container.set(RecoveryPhoneService, mockRecoveryPhoneService);
routes = recoveryPhoneRoutes(mockLog, mockGlean);
});

@@ -310,7 +313,6 @@ describe('/recovery-phone', () => {
});
assert.equal(mockGlean.twoStepAuthPhoneRemove.success.callCount, 0);
});

});

describe('POST /recovery-phone/available', async () => {
76 changes: 76 additions & 0 deletions packages/fxa-auth-server/test/local/routes/totp.js
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ const otplib = require('otplib');
const { Container } = require('typedi');
const { AccountEventsManager } = require('../../../lib/account-events');
const authErrors = require('../../../lib/error');
const { RecoveryPhoneService } = require('@fxa/accounts/recovery-phone');

let log,
db,
@@ -25,6 +26,9 @@ let log,
accountEventsManager;

const glean = mocks.mockGlean();
const mockRecoveryPhoneService = {
confirmCode: sinon.fake(),
};

const TEST_EMAIL = '[email protected]';
const secret = 'KE3TGQTRNIYFO2KOPE4G6ULBOV2FQQTN';
@@ -52,6 +56,8 @@ describe('totp', () => {
accountEventsManager = {
recordSecurityEvent: sinon.fake.resolves({}),
};
Container.set(RecoveryPhoneService, mockRecoveryPhoneService);
glean.twoStepAuthRemove.success.reset();
});

after(() => {
@@ -194,6 +200,76 @@ describe('totp', () => {
);
});
});

it('should delete TOTP token in verified session with valid code', async () => {
const authenticator = new otplib.authenticator.Authenticator();
authenticator.options = Object.assign({}, otplib.authenticator.options, {
secret,
});
const code = authenticator.generate(secret);

mockRecoveryPhoneService.confirmCode = sinon.fake.resolves(true);
requestOptions.credentials.tokenVerified = true;
requestOptions.payload = {
code,
};

const response = await setup(
{ db: { email: TEST_EMAIL }, profile },
{},
'/totp/destroy',
requestOptions
);
assert.ok(response);

// TODO figure out what to about db.totp
assert.equal(db.totpToken.callCount, 2);
assert.equal(db.totpToken.getCall(0).args[0], 'uid');
assert.equal(mockRecoveryPhoneService.confirmCode.callCount, 1);
assert.equal(glean.twoStepAuthRemove.success.callCount, 1);
});

it('should not delete TOTP token if provided sms code is invalid', async () => {
mockRecoveryPhoneService.confirmCode = sinon.fake.resolves(false);
requestOptions.credentials.tokenVerified = true;
requestOptions.payload = {
code: '000000',
};

try {
await setup(
{ db: { email: TEST_EMAIL } },
{},
'/totp/destroy',
requestOptions
);
assert.fail();
} catch (err) {
assert.equal(err.errno, 183);
assert.equal(glean.twoStepAuthRemove.success.callCount, 0);
}
});

it('should not delete TOTP token if provided code is invalid totp code', async () => {
mockRecoveryPhoneService.confirmCode = sinon.fake.resolves(true);
requestOptions.credentials.tokenVerified = true;
requestOptions.payload = {
code: '000000',
};

try {
await setup(
{ db: { email: TEST_EMAIL } },
{},
'/totp/destroy',
requestOptions
);
assert.fail();
} catch (err) {
assert.deepEqual(err.errno, 183);
assert.equal(glean.twoStepAuthRemove.success.callCount, 0);
}
});
});

describe('/totp/exists', () => {
19 changes: 19 additions & 0 deletions packages/fxa-shared/metrics/glean/fxa-backend-metrics.yaml
Original file line number Diff line number Diff line change
@@ -1004,6 +1004,25 @@ two_step_auth_phone_code:
data_sensitivity:
- interaction

two_step_auth_remove:
success:
type: event
description: |
Event that indicates the user successfully remove two-step authentication
lifetime: ping
send_in_pings:
- events
notification_emails:
- [email protected]
- [email protected]
bugs:
- https://mozilla-hub.atlassian.net/browse/FXA-10360
data_reviews:
- https://bugzilla.mozilla.org/show_bug.cgi?id=1830504
expires: never
data_sensitivity:
- interaction

inactive_account_deletion:
status_checked:
type: event

0 comments on commit ddd64dc

Please sign in to comment.