Skip to content

Commit 2646aec

Browse files
LZoogdschom
authored andcommitted
feat(third-party-auth): Conditionally render login buttons on signin
Because: * Users that have an account via third-party auth and have not yet set a password should be shown a login screen that shows the login button(s) relevant to them instead of the 'enter a password' view This commit: * Adds necessary conditional logic to sign_in_password * Updates the accoun/status endpoint to return third party auth status data when requested closes FXA-4776
1 parent 218b3a6 commit 2646aec

File tree

19 files changed

+376
-67
lines changed

19 files changed

+376
-67
lines changed

packages/functional-tests/tests/oauth/signinTokenCode.spec.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,9 @@ test.describe('OAuth signin token code', () => {
6262
// This will cause the token become 'invalid' and ultimately cause an
6363
// INVALID_TOKEN error to be thrown.
6464
await login.destroySession(email);
65-
await page.waitForNavigation({ waitUntil: 'networkidle' }),
66-
// Destroying the session should direct user back to sign in page
67-
expect(await login.passwordHeader.isVisible()).toBeTruthy();
65+
await page.waitForURL(/oauth\/signin/);
66+
// Destroying the session should direct user back to sign in page
67+
await login.passwordHeader.waitFor({ state: 'visible' });
6868
});
6969

7070
test('verified - valid code', async ({

packages/fxa-auth-client/lib/client.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -710,8 +710,11 @@ export default class AuthClient {
710710
return this.request('GET', `/account/status?uid=${uid}`);
711711
}
712712

713-
async accountStatusByEmail(email: string) {
714-
return this.request('POST', '/account/status', { email });
713+
async accountStatusByEmail(
714+
email: string,
715+
options: { thirdPartyAuthStatus?: boolean } = {}
716+
) {
717+
return this.request('POST', '/account/status', { email, ...options });
715718
}
716719

717720
async accountProfile(sessionToken: hexstring) {

packages/fxa-auth-server/lib/db/index.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -265,9 +265,9 @@ module.exports = (config, log, Token, UnblockCode = null) => {
265265
return PasswordChangeToken.fromHex(data.tokenData, data);
266266
};
267267

268-
DB.prototype.accountRecord = async function (email) {
268+
DB.prototype.accountRecord = async function (email, options) {
269269
log.trace('DB.accountRecord', { email });
270-
const account = await Account.findByPrimaryEmail(email);
270+
const account = await Account.findByPrimaryEmail(email, options);
271271
if (!account) {
272272
throw error.unknownAccount(email);
273273
}

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

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,6 +1148,8 @@ export class AccountHandler {
11481148
async accountStatusCheck(request: AuthRequest) {
11491149
const email = (request.payload as any).email;
11501150
const checkDomain = !!(request.payload as any).checkDomain;
1151+
const thirdPartyAuthStatus = !!(request.payload as any)
1152+
.thirdPartyAuthStatus;
11511153
let invalidDomain = false;
11521154

11531155
if (checkDomain) {
@@ -1156,14 +1158,31 @@ export class AccountHandler {
11561158

11571159
await this.customs.check(request, email, 'accountStatusCheck');
11581160

1159-
const result: { exists: boolean; invalidDomain?: boolean } = {
1161+
const result: {
1162+
exists: boolean;
1163+
invalidDomain?: boolean;
1164+
hasLinkedAccount?: boolean;
1165+
hasPassword?: boolean;
1166+
} = {
11601167
exists: false,
11611168
invalidDomain: undefined,
1169+
hasLinkedAccount: undefined,
1170+
hasPassword: undefined,
11621171
};
11631172

11641173
try {
1165-
const exist = await this.db.accountExists(email);
1166-
result.exists = exist;
1174+
if (thirdPartyAuthStatus) {
1175+
const account = await this.db.accountRecord(email, {
1176+
linkedAccounts: true,
1177+
});
1178+
// account must exist or unknown account error is thrown
1179+
result.exists = true;
1180+
result.hasLinkedAccount = account.linkedAccounts.length > 0;
1181+
result.hasPassword = account.verifierSetAt > 0;
1182+
} else {
1183+
const exist = await this.db.accountExists(email);
1184+
result.exists = exist;
1185+
}
11671186

11681187
if (checkDomain) {
11691188
result.invalidDomain = invalidDomain;
@@ -1895,12 +1914,15 @@ export const accountRoutes = (
18951914
validate: {
18961915
payload: isA.object({
18971916
email: validators.email().required(),
1917+
thirdPartyAuthStatus: isA.boolean().optional().default(false),
18981918
checkDomain: isA.optional(),
18991919
}),
19001920
},
19011921
response: {
19021922
schema: isA.object({
19031923
exists: isA.boolean().required(),
1924+
hasLinkedAccount: isA.boolean().optional(),
1925+
hasPassword: isA.boolean().optional(),
19041926
invalidDomain: isA.boolean().optional(),
19051927
}),
19061928
},

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

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1348,7 +1348,11 @@ describe('/account/stub', () => {
13481348
});
13491349

13501350
describe('/account/status', () => {
1351-
function setup(extraConfig) {
1351+
function setup({
1352+
extraConfig = {},
1353+
dbOptions = {},
1354+
shouldError = true,
1355+
} = {}) {
13521356
const config = {
13531357
securityHistory: {
13541358
enabled: true,
@@ -1397,15 +1401,14 @@ describe('/account/status', () => {
13971401
uaOSVersion: '10.10',
13981402
uid,
13991403
wrapWrapKb: 'wibble',
1404+
...dbOptions,
14001405
},
14011406
{
1402-
emailRecord: new error.unknownAccount(),
1407+
...(shouldError && {
1408+
emailRecord: new error.unknownAccount(),
1409+
}),
14031410
}
14041411
);
1405-
mockDB.accountExists = (arg) => {
1406-
return false;
1407-
};
1408-
14091412
const mockMailer = mocks.mockMailer();
14101413
const mockPush = mocks.mockPush();
14111414
const verificationReminders = mocks.mockVerificationReminders();
@@ -1473,6 +1476,41 @@ describe('/account/status', () => {
14731476
assert.equal(response.invalidDomain, undefined);
14741477
});
14751478
});
1479+
1480+
it('calls accountRecord and returns expected values when thirdPartyAuthStatus is requested', async () => {
1481+
const { route, mockRequest, mockDB } = setup({
1482+
dbOptions: {
1483+
linkedAccounts: [{}],
1484+
verifierSetAt: 0,
1485+
},
1486+
shouldError: false,
1487+
});
1488+
mockRequest.payload.thirdPartyAuthStatus = true;
1489+
1490+
return runTest(route, mockRequest, (response) => {
1491+
assert.equal(mockDB.accountRecord.callCount, 1);
1492+
assert.equal(mockDB.accountExists.callCount, 0);
1493+
1494+
assert.equal(response.exists, true);
1495+
assert.equal(response.hasLinkedAccount, true);
1496+
assert.equal(response.hasPassword, false);
1497+
});
1498+
});
1499+
1500+
it('calls accountExists when thirdPartyAuthStatus is not requested', async () => {
1501+
const { route, mockRequest, mockDB } = setup({
1502+
dbOptions: { exists: false },
1503+
});
1504+
1505+
return runTest(route, mockRequest, (response) => {
1506+
assert.equal(mockDB.accountRecord.callCount, 0);
1507+
assert.equal(mockDB.accountExists.callCount, 1);
1508+
1509+
assert.equal(response.exists, false);
1510+
assert.equal(response.linkedAccounts, undefined);
1511+
assert.equal(response.hasPassword, undefined);
1512+
});
1513+
});
14761514
});
14771515

14781516
describe('/account/finish_setup', () => {

packages/fxa-auth-server/test/mocks.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ const DB_METHOD_NAMES = [
111111
'getLinkedAccount',
112112
'createLinkedAccount',
113113
'deleteLinkedAccount',
114+
'accountExists',
114115
];
115116

116117
const LOG_METHOD_NAMES = [
@@ -308,6 +309,9 @@ function mockDB(data, errors) {
308309
},
309310
]);
310311
}),
312+
accountExists: sinon.spy(() => {
313+
return Promise.resolve(data.exists ?? true);
314+
}),
311315
accountRecord: sinon.spy(() => {
312316
if (errors.emailRecord) {
313317
return Promise.reject(errors.emailRecord);
@@ -339,6 +343,7 @@ function mockDB(data, errors) {
339343
uid: data.uid,
340344
wrapWrapKb: crypto.randomBytes(32),
341345
verifierSetAt: data.verifierSetAt ?? Date.now(),
346+
linkedAccounts: data.linkedAccounts,
342347
});
343348
}),
344349
consumeSigninCode: sinon.spy(() => {

packages/fxa-auth-server/test/remote/db_tests.js

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1571,6 +1571,39 @@ describe('#integration - remote db', function () {
15711571
});
15721572
});
15731573

1574+
it('can retrieve linked account', async () => {
1575+
const linkedAccount = await db.createLinkedAccount(
1576+
account.uid,
1577+
'googleid',
1578+
'google'
1579+
);
1580+
// linkedAccount UID comes back as a buffer but we want a hex string
1581+
// comparison to see what accountRecord retrieves
1582+
if (linkedAccount.uid instanceof Buffer) {
1583+
linkedAccount.uid = linkedAccount.uid.toString('hex');
1584+
}
1585+
1586+
const accountRecord = await db.accountRecord(account.email, {
1587+
linkedAccounts: true,
1588+
});
1589+
1590+
assert.deepEqual(
1591+
linkedAccount,
1592+
accountRecord.linkedAccounts[0],
1593+
'should contain an array of linked accounts'
1594+
);
1595+
});
1596+
1597+
it('does not retrieve linked account without option specified', async () => {
1598+
await db.createLinkedAccount(account.uid, 'googleid', 'google');
1599+
const accountRecord = await db.accountRecord(account.email);
1600+
assert.strictEqual(
1601+
accountRecord.linkedAccounts,
1602+
undefined,
1603+
'linkedAccounts should be undefined'
1604+
);
1605+
});
1606+
15741607
it('returns unknown account', () => {
15751608
return db
15761609
.accountRecord('[email protected]')

packages/fxa-content-server/app/scripts/lib/fxa-client.js

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,27 @@ FxaClientWrapper.prototype = {
207207
});
208208
}),
209209

210+
/**
211+
* Check if the account's email is registered and retrieve third-party auth related values.
212+
* @param {String} email
213+
* @returns {Promise<{
214+
* exists: boolean,
215+
* hasLinkedAccount: boolean,
216+
* hasPassword: boolean
217+
* }>}
218+
*/
219+
checkAccountStatus: withClient((client, email) => {
220+
return client
221+
.accountStatusByEmail(email, { thirdPartyAuthStatus: true })
222+
.then(function ({ exists, hasLinkedAccount, hasPassword }) {
223+
return {
224+
exists,
225+
hasLinkedAccount,
226+
hasPassword,
227+
};
228+
});
229+
}),
230+
210231
/**
211232
* Authenticate a user.
212233
*

packages/fxa-content-server/app/scripts/models/account.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ const DEFAULTS = _.extend(
7474
verificationReason: undefined,
7575
totpVerified: undefined,
7676
providerUid: undefined,
77+
hasLinkedAccount: undefined,
78+
hasPassword: undefined,
7779
},
7880
PERSISTENT
7981
);
@@ -818,6 +820,27 @@ const Account = Backbone.Model.extend(
818820
return this._fxaClient.checkAccountExistsByEmail(this.get('email'));
819821
},
820822

823+
/**
824+
* Check if the account's email is registered and retrieve third-party auth related values.
825+
* Sets the third-party auth values onto the model.
826+
* @returns {Promise<{
827+
* exists: boolean,
828+
* hasLinkedAccount: boolean,
829+
* hasPassword: boolean
830+
* }>}
831+
*/
832+
async checkAccountStatus() {
833+
const { hasLinkedAccount, hasPassword, exists } =
834+
await this._fxaClient.checkAccountStatus(this.get('email'));
835+
this.set('hasLinkedAccount', hasLinkedAccount);
836+
this.set('hasPassword', hasPassword);
837+
return {
838+
hasLinkedAccount,
839+
hasPassword,
840+
exists,
841+
};
842+
},
843+
821844
/**
822845
* Check whether the account's UID is registered.
823846
*

packages/fxa-content-server/app/scripts/models/user.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -713,6 +713,26 @@ var User = Backbone.Model.extend({
713713
});
714714
},
715715

716+
/**
717+
* Check whether an Account's `email` is registered. Removes the account
718+
* from storage if account no longer exists on the server.
719+
* Additionally, retrieves third-party auth related values.
720+
* @param {Object} account - account to check
721+
* @returns {Promise<{
722+
* exists: boolean,
723+
* hasLinkedAccount: boolean,
724+
* hasPassword: boolean
725+
* }>}
726+
*/
727+
checkAccountStatus(account) {
728+
return account.checkAccountStatus().then((result) => {
729+
if (!result.exists) {
730+
this.removeAccount(account);
731+
}
732+
return result;
733+
});
734+
},
735+
716736
/**
717737
* Reject the unblockCode for the given account. This invalidates
718738
* the unblock code and logs the signin attempt as suspicious.

0 commit comments

Comments
 (0)