Skip to content

Commit 64c32d0

Browse files
nshirleynshirley
authored
Merge pull request #20288 from mozilla/FXA-13360
chore(auth): Remove skipConfirmationForEmailAddresses config
2 parents e3b44e9 + b2155ec commit 64c32d0

4 files changed

Lines changed: 30 additions & 198 deletions

File tree

‎packages/fxa-auth-server/config/index.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1693,14 +1693,8 @@ const convictConf = convict({
16931693
default: /.+@mozilla\.com$/,
16941694
env: 'SIGNIN_CONFIRMATION_FORCE_EMAIL_REGEX',
16951695
},
1696-
skipForEmailAddresses: {
1697-
doc: 'Comma separated list of email addresses that will always skip any non TOTP sign-in confirmation',
1698-
format: Array,
1699-
default: [],
1700-
env: 'SIGNIN_CONFIRMATION_SKIP_FOR_EMAIL_ADDRESS',
1701-
},
17021696
skipForEmailRegex: {
1703-
doc: 'Regex pattern for email addresses that will always skip any non-TOTP sign-in confirmation. Checked in addition to skipForEmailAddresses.',
1697+
doc: 'Regex pattern for email addresses that will always skip any non-TOTP sign-in confirmation.',
17041698
format: RegExp,
17051699
default: /^$/,
17061700
env: 'SIGNIN_CONFIRMATION_SKIP_FOR_EMAIL_REGEX',

‎packages/fxa-auth-server/lib/routes/account.spec.ts‎

Lines changed: 25 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -3080,86 +3080,14 @@ describe('/account/login', () => {
30803080
);
30813081
});
30823082
});
3083-
});
3084-
3085-
describe('skip for emails', () => {
3086-
function setupSkipForEmails(email: string) {
3087-
config.securityHistory.ipProfiling.allowedRecency = 0;
3088-
config.signinConfirmation.skipForNewAccounts = { enabled: false };
3089-
config.signinConfirmation.skipForEmailAddresses = [
3090-
'[email protected]',
3091-
'[email protected]',
3092-
];
3093-
3094-
// Reset the spy to avoid leaking state between tests
3095-
mockDB.verifiedLoginSecurityEvents = sinon.spy(() =>
3096-
Promise.resolve([])
3097-
);
3098-
3099-
mockRequest.payload.email = email;
3100-
3101-
mockDB.accountRecord = () => {
3102-
return Promise.resolve({
3103-
authSalt: hexString(32),
3104-
data: hexString(32),
3105-
email,
3106-
emailVerified: true,
3107-
primaryEmail: {
3108-
normalizedEmail: normalizeEmail(email),
3109-
email,
3110-
isVerified: true,
3111-
isPrimary: true,
3112-
},
3113-
kA: hexString(32),
3114-
lastAuthAt: () => Date.now(),
3115-
uid,
3116-
wrapWrapKb: hexString(32),
3117-
});
3118-
};
3119-
3120-
const innerAccountRoutes = makeRoutes({
3121-
checkPassword: () => Promise.resolve(true),
3122-
config,
3123-
customs: mockCustoms,
3124-
db: mockDB,
3125-
log: mockLog,
3126-
mailer: mockMailer,
3127-
push: mockPush,
3128-
});
3129-
3130-
route = getRoute(innerAccountRoutes, '/account/login');
3131-
}
31323083

31333084
afterEach(() => {
3134-
// Restore config to default to avoid leaking into subsequent tests
3085+
config.signinConfirmation.skipForNewAccounts = undefined;
31353086
config.securityHistory.ipProfiling.allowedRecency =
31363087
defaultConfig.securityHistory.ipProfiling.allowedRecency;
3137-
});
3138-
3139-
it('should not skip sign-in confirmation for specified email', () => {
3140-
setupSkipForEmails('[email protected]');
3141-
3142-
return runTest(route, mockRequest, (response: any) => {
3143-
expect(mockDB.createSessionToken.callCount).toBe(1);
3144-
const tokenData = mockDB.createSessionToken.getCall(0).args[0];
3145-
expect(tokenData.tokenVerificationId).toBeTruthy();
3146-
expect(mockFxaMailer.sendVerifyLoginEmail.callCount).toBe(1);
3147-
expect(mockFxaMailer.sendNewDeviceLoginEmail.callCount).toBe(0);
3148-
expect(response.verified).toBeFalsy();
3149-
});
3150-
});
3151-
3152-
it('should skip sign-in confirmation for specified email', () => {
3153-
setupSkipForEmails('[email protected]');
3154-
3155-
return runTest(route, mockRequest, (response: any) => {
3156-
expect(mockDB.createSessionToken.callCount).toBe(1);
3157-
const tokenData = mockDB.createSessionToken.getCall(0).args[0];
3158-
expect(tokenData.tokenVerificationId).toBeFalsy();
3159-
expect(mockMailer.sendVerifyLoginEmail.callCount).toBe(0);
3160-
expect(mockFxaMailer.sendNewDeviceLoginEmail.callCount).toBe(1);
3161-
expect(response.emailVerified).toBeTruthy();
3162-
});
3088+
mockDB.verifiedLoginSecurityEvents = sinon.spy(() =>
3089+
Promise.resolve([])
3090+
);
31633091
});
31643092
});
31653093

@@ -3184,7 +3112,6 @@ describe('/account/login', () => {
31843112
function setupSkipForEmailRegex(email: string, regex: RegExp) {
31853113
config.securityHistory.ipProfiling.allowedRecency = 0;
31863114
config.signinConfirmation.skipForNewAccounts = { enabled: false };
3187-
config.signinConfirmation.skipForEmailAddresses = [];
31883115
config.signinConfirmation.skipForEmailRegex = regex;
31893116

31903117
mockDB.verifiedLoginSecurityEvents = sinon.spy(() =>
@@ -3224,11 +3151,18 @@ describe('/account/login', () => {
32243151

32253152
route = getRoute(innerAccountRoutes, '/account/login');
32263153
}
3227-
3154+
beforeEach(() => {
3155+
// one test is checking the statsd, and this is included in it.
3156+
// We set it here, and reset in the afterEach to avoid having the
3157+
// config state leak to other tests if these fail
3158+
mockRequest.app.clientIdTag = 'test-client-id';
3159+
statsd.increment.resetHistory();
3160+
});
32283161
afterEach(() => {
32293162
config.securityHistory.ipProfiling.allowedRecency =
32303163
defaultConfig.securityHistory.ipProfiling.allowedRecency;
32313164
config.signinConfirmation.skipForEmailRegex = /^$/;
3165+
mockRequest.app.clientIdTag = undefined;
32323166
});
32333167

32343168
it('should skip sign-in confirmation for email matching regex', () => {
@@ -3252,6 +3186,19 @@ describe('/account/login', () => {
32523186
expect(response.verified).toBeFalsy();
32533187
});
32543188
});
3189+
3190+
it('should increment statsd metric for emailAlways', () => {
3191+
setupSkipForEmailRegex('[email protected]', /.+@example\.com$/);
3192+
3193+
return runTest(route, mockRequest, () => {
3194+
sinon.assert.calledWith(
3195+
statsd.increment,
3196+
'account.signin.confirm.bypass.emailAlways',
3197+
{ clientId: 'test-client-id' }
3198+
);
3199+
mockRequest.app.clientIdTag = undefined;
3200+
});
3201+
});
32553202
});
32563203

32573204
describe('skip for known device', () => {

‎packages/fxa-auth-server/lib/routes/account.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,6 @@ export class AccountHandler {
8787

8888
private otpUtils: OtpUtils;
8989
private otpOptions: ConfigType['otp'];
90-
private skipConfirmationForEmailAddresses: string[];
9190
private skipConfirmationForEmailRegex: RegExp;
9291
private capabilityService: CapabilityService;
9392
private accountEventsManager: AccountEventsManager;
@@ -117,8 +116,6 @@ export class AccountHandler {
117116
private authServerCacheRedis: Redis
118117
) {
119118
this.otpUtils = require('./utils/otp').default(db, statsd);
120-
this.skipConfirmationForEmailAddresses = config.signinConfirmation
121-
.skipForEmailAddresses as string[];
122119
this.skipConfirmationForEmailRegex =
123120
config.signinConfirmation.skipForEmailRegex;
124121

@@ -1264,9 +1261,6 @@ export class AccountHandler {
12641261
// to guarantee the login experience.
12651262
const lowerCaseEmail = account.primaryEmail.normalizedEmail.toLowerCase();
12661263
const alwaysSkip =
1267-
this.skipConfirmationForEmailAddresses?.includes(lowerCaseEmail) ||
1268-
// use both as a backward compatibility and eventually remove
1269-
// the array of emails in favor of just a regex which is more flexible
12701264
this.skipConfirmationForEmailRegex?.test(lowerCaseEmail);
12711265
if (alwaysSkip) {
12721266
this.log.info('account.signin.confirm.bypass.emailAlways', {

‎packages/fxa-auth-server/test/local/routes/account.js‎

Lines changed: 4 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -781,7 +781,6 @@ describe('deleteAccountIfUnverified', () => {
781781
const mockConfig = {};
782782
mockConfig.oauth = {};
783783
mockConfig.signinConfirmation = {};
784-
mockConfig.signinConfirmation.skipForEmailAddresses = [];
785784
const emailRecord = {
786785
isPrimary: true,
787786
isVerified: false,
@@ -3780,116 +3779,14 @@ describe('/account/login', () => {
37803779
);
37813780
});
37823781
});
3783-
});
3784-
3785-
describe('skip for emails', () => {
3786-
function setup(email) {
3787-
config.securityHistory.ipProfiling.allowedRecency = 0;
3788-
config.signinConfirmation.skipForNewAccounts = { enabled: false };
3789-
config.signinConfirmation.skipForEmailAddresses = [
3790-
'[email protected]',
3791-
'[email protected]',
3792-
];
3793-
3794-
// Reset the spy to avoid leaking state between tests
3795-
// Previously, "should not skip sign-in confirmation for specified email" test
3796-
// was failing intermittently because the spy was not reset between tests.
3797-
mockDB.verifiedLoginSecurityEvents = sinon.spy(() =>
3798-
Promise.resolve([])
3799-
);
3800-
3801-
mockRequest.payload.email = email;
3802-
3803-
mockDB.accountRecord = () => {
3804-
return Promise.resolve({
3805-
authSalt: hexString(32),
3806-
data: hexString(32),
3807-
email,
3808-
emailVerified: true,
3809-
primaryEmail: {
3810-
normalizedEmail: normalizeEmail(email),
3811-
email,
3812-
isVerified: true,
3813-
isPrimary: true,
3814-
},
3815-
kA: hexString(32),
3816-
lastAuthAt: () => Date.now(),
3817-
uid,
3818-
wrapWrapKb: hexString(32),
3819-
});
3820-
};
3821-
3822-
const accountRoutes = makeRoutes({
3823-
checkPassword: () => Promise.resolve(true),
3824-
config,
3825-
customs: mockCustoms,
3826-
db: mockDB,
3827-
log: mockLog,
3828-
mailer: mockMailer,
3829-
push: mockPush,
3830-
});
3831-
3832-
route = getRoute(accountRoutes, '/account/login');
3833-
}
38343782

38353783
afterEach(() => {
3836-
// Restore config to default to avoid leaking into subsequent tests
3784+
config.signinConfirmation.skipForNewAccounts = undefined;
38373785
config.securityHistory.ipProfiling.allowedRecency =
38383786
defaultConfig.securityHistory.ipProfiling.allowedRecency;
3839-
});
3840-
3841-
it('should not skip sign-in confirmation for specified email', () => {
3842-
setup('[email protected]');
3843-
3844-
return runTest(route, mockRequest, (response) => {
3845-
assert.equal(
3846-
mockDB.createSessionToken.callCount,
3847-
1,
3848-
'db.createSessionToken was called'
3849-
);
3850-
const tokenData = mockDB.createSessionToken.getCall(0).args[0];
3851-
assert.ok(
3852-
tokenData.tokenVerificationId,
3853-
'sessionToken was created unverified'
3854-
);
3855-
assert.equal(
3856-
mockFxaMailer.sendVerifyLoginEmail.callCount,
3857-
1,
3858-
'mailer.sendVerifyLoginEmail was called'
3859-
);
3860-
assert.equal(mockFxaMailer.sendNewDeviceLoginEmail.callCount, 0);
3861-
assert.ok(
3862-
!response.verified,
3863-
'response indicates account is unverified'
3864-
);
3865-
});
3866-
});
3867-
3868-
it('should skip sign-in confirmation for specified email', () => {
3869-
setup('[email protected]');
3870-
3871-
return runTest(route, mockRequest, (response) => {
3872-
assert.equal(
3873-
mockDB.createSessionToken.callCount,
3874-
1,
3875-
'db.createSessionToken was called'
3876-
);
3877-
const tokenData = mockDB.createSessionToken.getCall(0).args[0];
3878-
assert.ok(
3879-
!tokenData.tokenVerificationId,
3880-
'sessionToken was created verified'
3881-
);
3882-
assert.equal(
3883-
mockMailer.sendVerifyLoginEmail.callCount,
3884-
0,
3885-
'mailer.sendVerifyLoginEmail was not called'
3886-
);
3887-
assert.equal(mockFxaMailer.sendNewDeviceLoginEmail.callCount, 1);
3888-
assert.ok(
3889-
response.emailVerified,
3890-
'response indicates account is verified'
3891-
);
3892-
});
3787+
mockDB.verifiedLoginSecurityEvents = sinon.spy(() =>
3788+
Promise.resolve([])
3789+
);
38933790
});
38943791
});
38953792

0 commit comments

Comments
 (0)