Skip to content

Commit

Permalink
task(auth): Update /totp/destroy to take an sms code
Browse files Browse the repository at this point in the history
  • Loading branch information
dschom committed Dec 20, 2024
1 parent b07808b commit 1cc7d11
Show file tree
Hide file tree
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
Expand Up @@ -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.
Expand Down
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
Expand Up @@ -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(
Expand Down Expand Up @@ -363,6 +365,7 @@ export const logErrorWithGlean = ({
| 'twoFactorAuthSetup'
| 'inactiveAccountDeletion'
| 'twoStepAuthPhoneRemove'
| 'twoStepAuthRemove'
>
];
funnelFns[event as keyof typeof funnelFns](request, {
Expand Down
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
Expand Up @@ -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,
});
}
}

/**
Expand Down
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
Expand Up @@ -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) {
Expand Down
37 changes: 37 additions & 0 deletions packages/fxa-auth-server/lib/routes/totp.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 [
{
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand Down
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
Expand Up @@ -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();
Expand Down Expand Up @@ -104,6 +105,7 @@ const gleanProxy = proxyquire('../../../lib/metrics/glean', {
recordTwoStepAuthPhoneCodeCompleteStub,
recordTwoStepAuthPhoneRemoveSuccess:
recordTwoStepAuthPhoneRemoveSuccessStub,
recordTwoStepAuthRemoveSuccess: recordTwoStepAuthRemoveSuccessStub,
recordPasswordResetTwoFactorSuccess:
recordPasswordResetTwoFactorSuccessStub,
recordPasswordResetRecoveryCodeSuccess:
Expand Down
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
Expand Up @@ -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');
Expand Down Expand Up @@ -39,7 +42,7 @@ describe('/recovery-phone', () => {
let routes = [];

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

Expand Down Expand Up @@ -309,7 +312,6 @@ describe('/recovery-phone', () => {
});
assert.equal(mockGlean.twoStepAuthPhoneRemove.success.callCount, 0);
});

});

describe('POST /recovery-phone/available', async () => {
Expand Down
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
Expand Up @@ -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,
Expand All @@ -25,6 +26,9 @@ let log,
accountEventsManager;

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

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

after(() => {
Expand Down Expand Up @@ -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', () => {
Expand Down
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
Expand Up @@ -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
Expand Down

0 comments on commit 1cc7d11

Please sign in to comment.