Skip to content

Commit db4677a

Browse files
committed
fix(sessions): Add env var for unverified session state + bandaid FE fix
Because: * Users in a non-Sync, non-2FA unverified session state are taken directly to Settings This commit: * Adds a new env var for testing to simulate an account/session in this state * Adds a bandaid fix for what auth-server is returning in this state around 'verified', which directly references the session verification state, until a better solution is implemented in a follow up * Adds our new auth strategy to the 'account/attached_client/destroy' endpoint closes FXA-12406
1 parent 7217918 commit db4677a

File tree

11 files changed

+103
-29
lines changed

11 files changed

+103
-29
lines changed

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1600,11 +1600,17 @@ const convictConf = convict({
16001600
},
16011601
},
16021602
forceGlobally: {
1603-
doc: 'Force sign-in confirmation for all accounts',
1603+
doc: 'Force sign-in confirmation for all accounts. Sets "mustVerify: 1" on created session tokens and creates an entry in unverifiedTokens, simulating a suspicious request or requesting scoped keys',
16041604
format: Boolean,
16051605
default: false,
16061606
env: 'SIGNIN_CONFIRMATION_FORCE_GLOBALLY',
16071607
},
1608+
tokenVerification: {
1609+
doc: 'If set to false, force sign-in confirmation for logins that do not request scoped keys. Sets "mustVerify: 0" on created session tokens but creates an entry in unverifiedTokens, simulating an unverified session state',
1610+
format: Boolean,
1611+
default: true,
1612+
env: 'SIGNIN_CONFIRMATION_TOKEN_VERIFICATION',
1613+
},
16081614
},
16091615
forcePasswordChange: {
16101616
forcedEmailAddresses: {

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1075,6 +1075,10 @@ export class AccountHandler {
10751075
};
10761076

10771077
const skipTokenVerification = (request: AuthRequest, account: any) => {
1078+
// Skip all checks to simulate an unverified session token state
1079+
if (this.config.signinConfirmation.tokenVerification === false) {
1080+
return false;
1081+
}
10781082
// If they're logging in from an IP address on which they recently did
10791083
// another, successfully-verified login, then we can consider this one
10801084
// verified as well without going through the loop again.

packages/fxa-auth-server/lib/routes/attached-clients.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,8 @@ module.exports = (log, db, devices, clientUtils) => {
122122
options: {
123123
...DEVICES_AND_SESSIONS_DOC.ACCOUNT_ATTACHED_CLIENT_DESTROY_POST,
124124
auth: {
125-
strategy: 'sessionToken',
126-
payload: 'required',
125+
strategy: 'verifiedSessionToken',
126+
payload: 'false',
127127
},
128128
validate: {
129129
payload: isA

packages/fxa-auth-server/test/local/routes/attached-clients.js

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,7 @@ describe('/account/attached_clients', () => {
419419
});
420420

421421
describe('/account/attached_client/destroy', () => {
422-
let config, uid, log, db, devices, request, route;
422+
let config, uid, log, db, devices, request, route, accountRoutes;
423423

424424
beforeEach(() => {
425425
config = {};
@@ -434,7 +434,7 @@ describe('/account/attached_client/destroy', () => {
434434
},
435435
payload: {},
436436
});
437-
const accountRoutes = makeRoutes({
437+
accountRoutes = makeRoutes({
438438
config,
439439
log,
440440
db,
@@ -443,6 +443,14 @@ describe('/account/attached_client/destroy', () => {
443443
route = getRoute(accountRoutes, '/account/attached_client/destroy').handler;
444444
});
445445

446+
it('requires verifiedSessionToken auth strategy', () => {
447+
const routeConfig = getRoute(
448+
accountRoutes,
449+
'/account/attached_client/destroy'
450+
);
451+
assert.equal(routeConfig.options.auth.strategy, 'verifiedSessionToken');
452+
});
453+
446454
it('can destroy by deviceId', async () => {
447455
const deviceId = newId();
448456
request.payload = {

packages/fxa-graphql-api/src/gql/session.resolver.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ export class SessionResolver {
5353
}
5454

5555
@Query((returns) => SessionType)
56-
@UseGuards(GqlAuthGuard)
56+
@UseGuards(UnverifiedSessionGuard)
5757
session(@GqlUserId() uid: string, @GqlUserState() state: string) {
5858
this.log.info('session', { uid });
5959
return {

packages/fxa-settings/src/pages/Authorization/container.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,9 @@ const AuthorizationContainer = ({
100100
const navigationOptions = {
101101
email: account?.email!,
102102
signinData: {
103-
verified: data.verified,
103+
// TODO, address signIn.verified vs session.verified discrepancy
104+
// we're currently using 'sessionVerified' from recovery_email/status
105+
verified: data.sessionVerified,
104106
verificationMethod: data.verificationMethod,
105107
verificationReason: data.verificationReason,
106108
uid: data.uid,

packages/fxa-settings/src/pages/Signin/container.test.tsx

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ import {
6262
OAuthQueryParams,
6363
SigninQueryParams,
6464
} from '../../models/pages/signin';
65+
import { storeAccountData } from '../../lib/storage-utils';
6566

6667
jest.mock('../../lib/channels/firefox', () => ({
6768
...jest.requireActual('../../lib/channels/firefox'),
@@ -70,6 +71,11 @@ jest.mock('../../lib/channels/firefox', () => ({
7071
},
7172
}));
7273

74+
jest.mock('../../lib/storage-utils', () => ({
75+
...jest.requireActual('../../lib/storage-utils'),
76+
storeAccountData: jest.fn(),
77+
}));
78+
7379
let integration: Integration;
7480

7581
function mockSyncDesktopV3Integration() {
@@ -181,6 +187,7 @@ const mockSensitiveDataClient = createMockSensitiveDataClient();
181187
mockSensitiveDataClient.setDataType = jest.fn();
182188

183189
const mockSession = {
190+
isSessionVerified: jest.fn().mockResolvedValue(true),
184191
sendVerificationCode: jest.fn().mockResolvedValue(undefined),
185192
verified: false,
186193
token: MOCK_SESSION_TOKEN,
@@ -215,6 +222,8 @@ function mockModelsModule() {
215222
},
216223
}));
217224
(ModelsModule.useSession as jest.Mock).mockImplementation(() => mockSession);
225+
mockSession.isSessionVerified = jest.fn().mockResolvedValue(true);
226+
mockSession.sendVerificationCode = jest.fn().mockResolvedValue(undefined);
218227
}
219228

220229
// Call this when testing local storage
@@ -569,6 +578,7 @@ describe('signin container', () => {
569578
expect(handlerResult?.data?.signIn?.verificationReason).toEqual(
570579
MOCK_VERIFICATION.verificationReason
571580
);
581+
expect(storeAccountData).toHaveBeenCalled();
572582
});
573583
});
574584

@@ -1035,6 +1045,10 @@ describe('signin container', () => {
10351045
// Note, we cannot automatically upgrade unverified accounts, because the
10361046
// reset password mechanism won't work in this case, so we want to fallback
10371047
// to using V1 credentials in this scenario.
1048+
(ModelsModule.useSession as jest.Mock).mockImplementation(() => ({
1049+
...mockSession,
1050+
isSessionVerified: jest.fn().mockResolvedValue(false),
1051+
}));
10381052
render([
10391053
mockGqlAvatarUseQuery(),
10401054
mockGqlCredentialStatusMutation({

packages/fxa-settings/src/pages/Signin/container.tsx

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,11 @@ import {
7575
isUnsupportedContext,
7676
} from '../../models/integrations/utils';
7777
import { GqlKeyStretchUpgrade } from '../../lib/gql-key-stretch-upgrade';
78-
import { setCurrentAccount, StoredAccountData } from '../../lib/storage-utils';
78+
import {
79+
setCurrentAccount,
80+
storeAccountData,
81+
StoredAccountData,
82+
} from '../../lib/storage-utils';
7983
import { cachedSignIn } from './utils';
8084
import OAuthDataError from '../../components/OAuthDataError';
8185

@@ -419,15 +423,39 @@ const SigninContainer = ({
419423
// In this case, we should stash the credentials so we can try at a later
420424
// point in then flow after verification is complete.
421425
//
426+
let isVerified = false;
427+
if ('data' in result && result.data && 'signIn' in result.data) {
428+
const accountData: StoredAccountData = {
429+
email,
430+
uid: result.data.signIn.uid,
431+
lastLogin: Date.now(),
432+
sessionToken: result.data.signIn.sessionToken,
433+
// TODO, address signIn.verified vs session.verified discrepancy
434+
verified: result.data.signIn.verified,
435+
metricsEnabled: result.data.signIn.metricsEnabled,
436+
};
437+
438+
storeAccountData(accountData);
439+
440+
// chicken and egg scenario where isSessionVerified GQL call needs
441+
// the updated session token in local storage that we set with storeAccountData
442+
// but we should update 'verified' once we know the real status
443+
// this will be taken care of in the clean up ticket FXA-12454
444+
isVerified = await session.isSessionVerified();
445+
console.log('HELLO isVerified', isVerified);
446+
storeAccountData({
447+
...accountData,
448+
verified: isVerified,
449+
});
450+
}
451+
422452
if (
423453
credentials.credentialStatus?.upgradeNeeded === true &&
424454
credentials.v2Credentials
425455
) {
426456
let upgraded = false;
427457
if ('data' in result) {
428-
const isVerified = result.data?.signIn.verified;
429458
const sessionToken = result.data?.signIn.sessionToken;
430-
431459
if (sessionToken && isVerified) {
432460
upgraded = await upgradeClient.upgrade(
433461
email,
@@ -449,7 +477,19 @@ const SigninContainer = ({
449477
}
450478
}
451479

452-
return result;
480+
// TODO, address signIn.verified vs session.verified discrepancy
481+
// This currently overrides data.signIn.verified with session.isSessionVerified
482+
return {
483+
...result,
484+
...('data' in result &&
485+
result.data && {
486+
data: {
487+
...result.data,
488+
// Temporarily override signIn data with session token verification state
489+
signIn: { ...result.data.signIn, verified: isVerified },
490+
},
491+
}),
492+
};
453493
},
454494
[
455495
beginSignin,
@@ -465,6 +505,7 @@ const SigninContainer = ({
465505
authClient,
466506
sensitiveDataClient,
467507
originFromEmailFirst,
508+
session,
468509
]
469510
);
470511

packages/fxa-settings/src/pages/Signin/index.test.tsx

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ import {
3636
} from '../mocks';
3737
import { MozServices } from '../../lib/types';
3838
import * as utils from 'fxa-react/lib/utils';
39-
import { storeAccountData } from '../../lib/storage-utils';
4039
import VerificationMethods from '../../constants/verification-methods';
4140
import VerificationReasons from '../../constants/verification-reasons';
4241
import { SigninProps } from './interfaces';
@@ -367,7 +366,6 @@ describe('Signin component', () => {
367366
});
368367
expect(GleanMetrics.login.submit).toHaveBeenCalledTimes(1);
369368
expect(GleanMetrics.login.success).toHaveBeenCalledTimes(1);
370-
expect(storeAccountData).toHaveBeenCalled();
371369
});
372370

373371
it('navigates to /signin_totp_code when TOTP verification requested', async () => {

packages/fxa-settings/src/pages/Signin/index.tsx

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import { REACT_ENTRYPOINT } from '../../constants';
1919
import { AuthUiErrors } from '../../lib/auth-errors/auth-errors';
2020
import GleanMetrics from '../../lib/glean';
2121
import { usePageViewEvent } from '../../lib/metrics';
22-
import { StoredAccountData, storeAccountData } from '../../lib/storage-utils';
2322
import {
2423
useSensitiveDataClient,
2524
useFtlMsgResolver,
@@ -146,7 +145,9 @@ const Signin = ({
146145
const navigationOptions = {
147146
email,
148147
signinData: {
149-
verified: data.verified,
148+
// TODO, address signIn.verified vs session.verified discrepancy
149+
// we're currently using 'sessionVerified' from recovery_email/status
150+
verified: data.sessionVerified,
150151
verificationMethod: data.verificationMethod,
151152
verificationReason: data.verificationReason,
152153
uid: data.uid,
@@ -201,17 +202,6 @@ const Signin = ({
201202
if (data) {
202203
GleanMetrics.login.success();
203204

204-
const accountData: StoredAccountData = {
205-
email,
206-
uid: data.signIn.uid,
207-
lastLogin: Date.now(),
208-
sessionToken: data.signIn.sessionToken,
209-
verified: data.signIn.verified,
210-
metricsEnabled: data.signIn.metricsEnabled,
211-
};
212-
213-
storeAccountData(accountData);
214-
215205
const navigationOptions = {
216206
email,
217207
signinData: data.signIn,
@@ -509,7 +499,9 @@ const Signin = ({
509499
{!hideThirdPartyAuth && (
510500
<ThirdPartyAuth
511501
showSeparator={true}
512-
separatorType={hasLinkedAccountAndNoPassword ? 'signInWith' : undefined}
502+
separatorType={
503+
hasLinkedAccountAndNoPassword ? 'signInWith' : undefined
504+
}
513505
{...{ viewName, flowQueryParams }}
514506
/>
515507
)}

0 commit comments

Comments
 (0)