Skip to content

Populate enrolled factors on reload() #3406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages-exp/auth-exp/src/core/strategies/phone.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ describe('core/strategies/phone', () => {
});
});

describe("updatePhoneNumber", () => {
describe('updatePhoneNumber', () => {
let user: User;
let reloadMock: fetch.Route;
let signInMock: fetch.Route;
Expand Down
3 changes: 3 additions & 0 deletions packages-exp/auth-exp/src/core/user/reload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ export async function _reloadWithoutSaving(user: User): Promise<void> {
assert(response?.users.length, auth.name);

const coreAccount = response.users[0];

user._notifyReloadListener(coreAccount);

const newProviderData = coreAccount.providerUserInfo?.length
? extractProviderData(coreAccount.providerUserInfo)
: [];
Expand Down
32 changes: 29 additions & 3 deletions packages-exp/auth-exp/src/core/user/user_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,21 @@
*/

import * as externs from '@firebase/auth-types-exp';
import { NextFn } from '@firebase/util';

import { deleteAccount } from '../../api/account_management/account';
import {
APIUserInfo,
deleteAccount
} from '../../api/account_management/account';
import { FinalizeMfaResponse } from '../../api/authentication/mfa';
import { Auth } from '../../model/auth';
import { IdTokenResponse } from '../../model/id_token';
import { User } from '../../model/user';
import { PersistedBlob } from '../persistence';
import { assert } from '../util/assert';
import { getIdTokenResult } from './id_token_result';
import { _reloadWithoutSaving, reload } from './reload';
import { reload, _reloadWithoutSaving } from './reload';
import { StsTokenManager } from './token_manager';
import { FinalizeMfaResponse } from '../../api/authentication/mfa';

export interface UserParameters {
uid: string;
Expand Down Expand Up @@ -102,6 +106,28 @@ export class UserImpl implements User {
return reload(this);
}

private reloadUserInfo: APIUserInfo | null = null;
private reloadListener: NextFn<APIUserInfo> | null = null;

_onReload(callback: NextFn<APIUserInfo>): void {
// There should only ever be one listener, and that is a single instance of MultiFactorUser
assert(!this.reloadListener, this.auth.name);
this.reloadListener = callback;
if (this.reloadUserInfo) {
this._notifyReloadListener(this.reloadUserInfo);
this.reloadUserInfo = null;
}
}

_notifyReloadListener(userInfo: APIUserInfo): void {
if (this.reloadListener) {
this.reloadListener(userInfo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not set this.reloadUserInfo = null here instead of on line 118?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the check next to the use, that way the logic is check, use, null-out, we don't actually use reloadUserInfo in _notifyReloadListener so it seems a bit off to null it out without using it

} else {
// If no listener is subscribed yet, save the result so it's available when they do subscribe
this.reloadUserInfo = userInfo;
}
}

async _updateTokensIfNecessary(
response: IdTokenResponse | FinalizeMfaResponse,
reload = false
Expand Down
88 changes: 72 additions & 16 deletions packages-exp/auth-exp/src/mfa/mfa_user.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { MultiFactorInfo } from './mfa_info';
import { ServerError } from '../api/errors';
import { User } from '../model/user';
import { FinalizeMfaResponse } from '../api/authentication/mfa';
import { ProviderId } from '@firebase/auth-types-exp';

use(chaiAsPromised);

Expand Down Expand Up @@ -118,14 +119,14 @@ describe('core/mfa/mfa_user/MultiFactorUser', () => {
expect(await mfaUser.user.getIdToken()).to.eq('final-id-token');
});

// TODO: Uncomment once we integrate with reload()
// it('should update the enrolled Factors', async () => {
// await mfaUser.enroll(assertion);
it('should update the enrolled Factors', async () => {
await mfaUser.enroll(assertion);

// const enrolledFactor = mfaUser.enrolledFactors[1];
// expect(enrolledFactor.factorId).to.eq(ProviderId.PHONE);
// expect(enrolledFactor.uid).to.eq('mfa-id');
// });
expect(mfaUser.enrolledFactors.length).to.eq(1);
const enrolledFactor = mfaUser.enrolledFactors[0];
expect(enrolledFactor.factorId).to.eq(ProviderId.PHONE);
expect(enrolledFactor.uid).to.eq('mfa-id');
});
});

describe('unenroll', () => {
Expand All @@ -150,19 +151,11 @@ describe('core/mfa/mfa_user/MultiFactorUser', () => {

const serverUser: APIUserInfo = {
localId: 'local-id',
displayName: 'display-name',
photoUrl: 'photo-url',
email: 'email',
emailVerified: true,
phoneNumber: 'phone-number',
tenantId: 'tenant-id',
createdAt: 123,
lastLoginAt: 456,
mfaInfo: [
{
mfaEnrollmentId: 'other-mfa-id',
enrolledAt: Date.now(),
phoneInfo: 'other-phone-number'
phoneInfo: 'other-phone-info'
}
]
};
Expand Down Expand Up @@ -244,4 +237,67 @@ describe('core/mfa/mfa_user/multiFactor', () => {
const mfaUser = multiFactor(user);
expect((mfaUser as MultiFactorUser).user).to.eq(user);
});

it('should only create one instance of an MFA user per User', () => {
const mfaUser = multiFactor(user);
expect(multiFactor(user)).to.eq(mfaUser);
});

context('enrolledFactors', () => {
const serverUser: APIUserInfo = {
localId: 'local-id',
mfaInfo: [
{
mfaEnrollmentId: 'enrollment-id',
enrolledAt: Date.now(),
phoneInfo: 'masked-phone-number'
}
]
};

const updatedServerUser: APIUserInfo = {
localId: 'local-id',
mfaInfo: [
{
mfaEnrollmentId: 'enrollment-id',
enrolledAt: Date.now(),
phoneInfo: 'masked-phone-number'
},
{
mfaEnrollmentId: 'new-enrollment-id',
enrolledAt: Date.now(),
phoneInfo: 'other-masked-phone-number'
}
]
};

beforeEach(() => {
mockFetch.setUp();
mockEndpoint(Endpoint.GET_ACCOUNT_INFO, {
users: [serverUser]
});
});

afterEach(mockFetch.tearDown);

it('should initialize the enrolled factors from the last reload', async () => {
await user.reload();
const mfaUser = multiFactor(user);
expect(mfaUser.enrolledFactors.length).to.eq(1);
const mfaInfo = mfaUser.enrolledFactors[0];
expect(mfaInfo.uid).to.eq('enrollment-id');
expect(mfaInfo.factorId).to.eq(ProviderId.PHONE);
});

it('should update the enrolled factors if the user is reloaded', async () => {
await user.reload();
const mfaUser = multiFactor(user);
expect(mfaUser.enrolledFactors.length).to.eq(1);
mockEndpoint(Endpoint.GET_ACCOUNT_INFO, {
users: [updatedServerUser]
});
await user.reload();
expect(mfaUser.enrolledFactors.length).to.eq(2);
});
});
});
22 changes: 19 additions & 3 deletions packages-exp/auth-exp/src/mfa/mfa_user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,20 @@ import { MultiFactorSession } from './mfa_session';
import { MultiFactorAssertion } from './assertions';
import { withdrawMfa } from '../api/account_management/mfa';
import { AuthErrorCode } from '../core/errors';
import { MultiFactorInfo } from './mfa_info';

export class MultiFactorUser implements externs.MultiFactorUser {
// TODO(avolkovi): Set these correctly from getAccountInfo on reload
enrolledFactors: externs.MultiFactorInfo[] = [];

private constructor(readonly user: User) {}
private constructor(readonly user: User) {
user._onReload(userInfo => {
if (userInfo.mfaInfo) {
this.enrolledFactors = userInfo.mfaInfo.map(enrollment =>
MultiFactorInfo._fromServerResponse(user.auth, enrollment)
);
}
});
}

static _fromUser(user: User): MultiFactorUser {
return new MultiFactorUser(user);
Expand Down Expand Up @@ -79,6 +87,14 @@ export class MultiFactorUser implements externs.MultiFactorUser {
}
}

const multiFactorUserCache = new WeakMap<
externs.User,
externs.MultiFactorUser
>();

export function multiFactor(user: externs.User): externs.MultiFactorUser {
return MultiFactorUser._fromUser(user as User);
if (!multiFactorUserCache.has(user)) {
multiFactorUserCache.set(user, MultiFactorUser._fromUser(user as User));
}
return multiFactorUserCache.get(user)!;
}
7 changes: 6 additions & 1 deletion packages-exp/auth-exp/src/model/user.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@
*/

import * as externs from '@firebase/auth-types-exp';
import { NextFn } from '@firebase/util';

import { APIUserInfo } from '../api/account_management/account';
import { FinalizeMfaResponse } from '../api/authentication/mfa';
import { PersistedBlob } from '../core/persistence';
import { Auth } from './auth';
import { IdTokenResponse } from './id_token';
import { FinalizeMfaResponse } from '../api/authentication/mfa';

type MutableUserInfo = {
-readonly [K in keyof externs.UserInfo]: externs.UserInfo[K];
Expand All @@ -46,6 +48,9 @@ export interface User extends externs.User {
reload?: boolean
): Promise<void>;

_onReload: (cb: NextFn<APIUserInfo>) => void;
_notifyReloadListener: NextFn<APIUserInfo>;

getIdToken(forceRefresh?: boolean): Promise<string>;
getIdTokenResult(forceRefresh?: boolean): Promise<externs.IdTokenResult>;
reload(): Promise<void>;
Expand Down