Skip to content

Add persistence for redirect users #3410

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 13 commits 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
112 changes: 109 additions & 3 deletions packages-exp/auth-exp/src/core/auth/auth_impl.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

import { expect, use } from 'chai';
import * as chaiAsPromised from 'chai-as-promised';
import * as sinon from 'sinon';
import * as sinonChai from 'sinon-chai';

Expand All @@ -26,10 +27,16 @@ import { FirebaseError } from '@firebase/util';
import { testUser } from '../../../test/mock_auth';
import { Auth } from '../../model/auth';
import { User } from '../../model/user';
import { browserPopupRedirectResolver } from '../../platform_browser/popup_redirect';
import { AUTH_ERROR_FACTORY, AuthErrorCode } from '../errors';
import { Persistence } from '../persistence';
import { browserLocalPersistence } from '../persistence/browser';
import {
browserLocalPersistence,
browserSessionPersistence
} from '../persistence/browser';
import { inMemoryPersistence } from '../persistence/in_memory';
import { PersistenceUserManager } from '../persistence/persistence_user_manager';
import * as reload from '../user/reload';
import { _getInstance } from '../util/instantiator';
import * as navigator from '../util/navigator';
import { _getClientVersion, ClientPlatform } from '../util/version';
Expand All @@ -41,6 +48,7 @@ import {
} from './auth_impl';

use(sinonChai);
use(chaiAsPromised);

const FAKE_APP: FirebaseApp = {
name: 'test-app',
Expand Down Expand Up @@ -286,14 +294,23 @@ describe('core/auth/initializeAuth', () => {

describe('persistence manager creation', () => {
let createManagerStub: sinon.SinonSpy;
let reloadStub: sinon.SinonStub;

beforeEach(() => {
createManagerStub = sinon.spy(PersistenceUserManager, 'create');
reloadStub = sinon
.stub(reload, '_reloadWithoutSaving')
.returns(Promise.resolve());
});

async function initAndWait(
persistence: externs.Persistence | externs.Persistence[]
persistence: externs.Persistence | externs.Persistence[],
popupRedirectResolver?: externs.PopupRedirectResolver
): Promise<Auth> {
const auth = initializeAuth(FAKE_APP, { persistence });
const auth = initializeAuth(FAKE_APP, {
persistence,
popupRedirectResolver
});
// Auth initializes async. We can make sure the initialization is
// flushed by awaiting a method on the queue.
await auth.setPersistence(inMemoryPersistence);
Expand Down Expand Up @@ -326,6 +343,95 @@ describe('core/auth/initializeAuth', () => {
]);
});

it('does not reload redirect users', async () => {
const user = testUser({}, 'uid');
user._redirectEventId = 'event-id';
sinon
.stub(_getInstance<Persistence>(inMemoryPersistence), 'get')
.returns(Promise.resolve(user.toPlainObject()));
sinon
.stub(_getInstance<Persistence>(browserSessionPersistence), 'get')
.returns(Promise.resolve(user.toPlainObject()));
await initAndWait(inMemoryPersistence);
expect(reload._reloadWithoutSaving).not.to.have.been.called;
});

it('reloads non-redirect users', async () => {
sinon
.stub(_getInstance<Persistence>(inMemoryPersistence), 'get')
.returns(Promise.resolve(testUser({}, 'uid').toPlainObject()));
sinon
.stub(_getInstance<Persistence>(browserSessionPersistence), 'get')
.returns(Promise.resolve(null));

await initAndWait(inMemoryPersistence);
expect(reload._reloadWithoutSaving).to.have.been.called;
});

it('Does not reload if the event ids match', async () => {
const user = testUser({}, 'uid');
user._redirectEventId = 'event-id';

sinon
.stub(_getInstance<Persistence>(inMemoryPersistence), 'get')
.returns(Promise.resolve(user.toPlainObject()));
sinon
.stub(_getInstance<Persistence>(browserSessionPersistence), 'get')
.returns(Promise.resolve(user.toPlainObject()));

await initAndWait(inMemoryPersistence, browserPopupRedirectResolver);
expect(reload._reloadWithoutSaving).not.to.have.been.called;
});

it('Reloads if the event ids do not match', async () => {
const user = testUser({}, 'uid');
user._redirectEventId = 'event-id';

sinon
.stub(_getInstance<Persistence>(inMemoryPersistence), 'get')
.returns(Promise.resolve(user.toPlainObject()));

user._redirectEventId = 'some-other-id';
sinon
.stub(_getInstance<Persistence>(browserSessionPersistence), 'get')
.returns(Promise.resolve(user.toPlainObject()));

await initAndWait(inMemoryPersistence, browserPopupRedirectResolver);
expect(reload._reloadWithoutSaving).to.have.been.called;
});

it('Nulls out the current user if reload fails', async () => {
const stub = sinon.stub(_getInstance<Persistence>(inMemoryPersistence));
stub.get.returns(Promise.resolve(testUser({}, 'uid').toPlainObject()));
stub.remove.returns(Promise.resolve());
reloadStub.returns(
Promise.reject(
AUTH_ERROR_FACTORY.create(AuthErrorCode.TOKEN_EXPIRED, {
appName: 'app'
})
)
);

await initAndWait(inMemoryPersistence);
expect(stub.remove).to.have.been.called;
});

it('Keeps current user if reload fails with network error', async () => {
const stub = sinon.stub(_getInstance<Persistence>(inMemoryPersistence));
stub.get.returns(Promise.resolve(testUser({}, 'uid').toPlainObject()));
stub.remove.returns(Promise.resolve());
reloadStub.returns(
Promise.reject(
AUTH_ERROR_FACTORY.create(AuthErrorCode.NETWORK_REQUEST_FAILED, {
appName: 'app'
})
)
);

await initAndWait(inMemoryPersistence);
expect(stub.remove).not.to.have.been.called;
});

it('sets auth name and config', async () => {
const auth = await initAndWait(inMemoryPersistence);
expect(auth.name).to.eq(FAKE_APP.name);
Expand Down
87 changes: 79 additions & 8 deletions packages-exp/auth-exp/src/core/auth/auth_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,15 @@ import {
} from '@firebase/util';

import { Auth, Dependencies } from '../../model/auth';
import { PopupRedirectResolver } from '../../model/popup_redirect';
import { User } from '../../model/user';
import { AuthErrorCode } from '../errors';
import { Persistence } from '../persistence';
import { PersistenceUserManager } from '../persistence/persistence_user_manager';
import {
_REDIRECT_USER_KEY_NAME,
PersistenceUserManager
} from '../persistence/persistence_user_manager';
import { _reloadWithoutSaving } from '../user/reload';
import { assert } from '../util/assert';
import { _getInstance } from '../util/instantiator';
import { _getUserLanguage } from '../util/navigator';
Expand All @@ -50,8 +55,10 @@ export class AuthImpl implements Auth {
currentUser: User | null = null;
private operations = Promise.resolve();
private persistenceManager?: PersistenceUserManager;
private redirectPersistenceManager?: PersistenceUserManager;
private authStateSubscription = new Subscription<User>(this);
private idTokenSubscription = new Subscription<User>(this);
private redirectUser: User | null = null;
_isInitialized = false;

// Tracks the last notified UID for state change listeners to prevent
Expand All @@ -68,25 +75,73 @@ export class AuthImpl implements Auth {
) {}

_initializeWithPersistence(
persistenceHierarchy: Persistence[]
persistenceHierarchy: Persistence[],
popupRedirectResolver?: externs.PopupRedirectResolver
): Promise<void> {
return this.queue(async () => {
this.persistenceManager = await PersistenceUserManager.create(
this,
persistenceHierarchy
);

const storedUser = await this.persistenceManager.getCurrentUser();
// TODO: Check redirect user, if not redirect user, call refresh on stored user
if (storedUser) {
await this.directlySetCurrentUser(storedUser);
}
await this.initializeCurrentUser(popupRedirectResolver);

this._isInitialized = true;
this.notifyAuthListeners();
});
}

private async initializeCurrentUser(
popupRedirectResolver?: externs.PopupRedirectResolver
): Promise<void> {
const storedUser = await this.assertedPersistence.getCurrentUser();
if (!storedUser) {
return this.directlySetCurrentUser(storedUser);
}

if (!storedUser._redirectEventId) {
// This isn't a redirect user, we can reload and bail
return this.reloadAndSetCurrentUserOrClear(storedUser);
}

assert(popupRedirectResolver, this.name, AuthErrorCode.ARGUMENT_ERROR);
Copy link
Contributor

Choose a reason for hiding this comment

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

we may want a more user friendly error here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on the one hand: yes. on the other: this promise is not returned to the user so they'll never see it 😬

const resolver: PopupRedirectResolver = _getInstance(popupRedirectResolver);

this.redirectPersistenceManager = await PersistenceUserManager.create(
this,
[_getInstance(resolver._redirectPersistence)],
_REDIRECT_USER_KEY_NAME
);

this.redirectUser = await this.redirectPersistenceManager.getCurrentUser();

// If the redirect user's event ID matches the current user's event ID,
// DO NOT reload the current user, otherwise they'll be cleared from storage.
// This is important for the reauthenticateWithRedirect() flow.
if (
this.redirectUser &&
this.redirectUser._redirectEventId === storedUser._redirectEventId
) {
return this.directlySetCurrentUser(storedUser);
}

return this.reloadAndSetCurrentUserOrClear(storedUser);
}

private async reloadAndSetCurrentUserOrClear(user: User): Promise<void> {
try {
await _reloadWithoutSaving(user);
} catch (e) {
if (e.code !== `auth/${AuthErrorCode.NETWORK_REQUEST_FAILED}`) {
// Something's wrong with the user's token. Log them out and remove
// them from storage
return this.directlySetCurrentUser(null);
}
}

return this.directlySetCurrentUser(user);
}

useDeviceLanguage(): void {
this.languageCode = _getUserLanguage();
}
Expand Down Expand Up @@ -134,6 +189,22 @@ export class AuthImpl implements Auth {
);
}

async _setRedirectUser(user: User): Promise<void> {
return this.redirectPersistenceManager?.setCurrentUser(user);
}

_redirectUserForId(id: string): User | null {
if (this.currentUser?._redirectEventId === id) {
return this.currentUser;
}

if (this.redirectUser?._redirectEventId === id) {
return this.redirectUser;
}

return null;
}

async _persistUserIfCurrent(user: User): Promise<void> {
if (user === this.currentUser) {
return this.queue(async () => this.directlySetCurrentUser(user));
Expand Down Expand Up @@ -238,7 +309,7 @@ export function initializeAuth(
// This promise is intended to float; auth initialization happens in the
// background, meanwhile the auth object may be used by the app.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
auth._initializeWithPersistence(hierarchy);
auth._initializeWithPersistence(hierarchy, deps?.popupRedirectResolver);

return auth;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,16 @@ import { _getInstance } from '../util/instantiator';
import { inMemoryPersistence } from './in_memory';

export const _AUTH_USER_KEY_NAME = 'authUser';
export const _REDIRECT_USER_KEY_NAME = 'redirectUser';
export const _PERSISTENCE_KEY_NAME = 'persistence';
const _PERSISTENCE_NAMESPACE = 'firebase';
const PERSISTENCE_NAMESPACE = 'firebase';

function _persistenceKeyName(
key: string,
apiKey: ApiKey,
appName: AppName
): string {
return `${_PERSISTENCE_NAMESPACE}:${key}:${apiKey}:${appName}`;
return `${PERSISTENCE_NAMESPACE}:${key}:${apiKey}:${appName}`;
}

export class PersistenceUserManager {
Expand Down
16 changes: 13 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 @@ -69,6 +69,8 @@ export class UserImpl implements User {
photoURL: string | null;
isAnonymous: boolean = false;

_redirectEventId?: string;

constructor({ uid, auth, stsTokenManager, ...opt }: UserParameters) {
this.uid = uid;
this.auth = auth;
Expand Down Expand Up @@ -143,7 +145,8 @@ export class UserImpl implements User {
displayName: this.displayName || undefined,
email: this.email || undefined,
phoneNumber: this.phoneNumber || undefined,
photoURL: this.phoneNumber || undefined
photoURL: this.phoneNumber || undefined,
_redirectEventId: this._redirectEventId
};
}

Expand All @@ -158,7 +161,8 @@ export class UserImpl implements User {
displayName,
email,
phoneNumber,
photoURL
photoURL,
_redirectEventId
} = object;

assert(uid && plainObjectTokenManager, auth.name);
Expand All @@ -173,7 +177,8 @@ export class UserImpl implements User {
assertStringOrUndefined(email, auth.name);
assertStringOrUndefined(phoneNumber, auth.name);
assertStringOrUndefined(photoURL, auth.name);
return new UserImpl({
assertStringOrUndefined(_redirectEventId, auth.name);
const user = new UserImpl({
uid,
auth,
stsTokenManager,
Expand All @@ -182,6 +187,11 @@ export class UserImpl implements User {
phoneNumber,
photoURL
});
if (_redirectEventId) {
user._redirectEventId = _redirectEventId;
}

return user;
}

/**
Expand Down
5 changes: 4 additions & 1 deletion packages-exp/auth-exp/src/model/auth.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/

import * as externs from '@firebase/auth-types-exp';
import { ErrorFn, CompleteFn, Unsubscribe } from '@firebase/util';
import { CompleteFn, ErrorFn, Unsubscribe } from '@firebase/util';

import { User } from './user';

Expand All @@ -42,8 +42,11 @@ export interface Auth extends externs.Auth {
): Unsubscribe;
_notifyListenersIfCurrent(user: User): void;
_persistUserIfCurrent(user: User): Promise<void>;
_setRedirectUser(user: User): Promise<void>;
_redirectUserForId(id: string): User | null;
}

export interface Dependencies {
persistence?: externs.Persistence | externs.Persistence[];
popupRedirectResolver?: externs.PopupRedirectResolver;
}
1 change: 1 addition & 0 deletions packages-exp/auth-exp/src/model/popup_redirect.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,5 @@ export interface PopupRedirectResolver extends externs.PopupRedirectResolver {
authType: AuthEventType,
eventId?: string
): Promise<never>;
_redirectPersistence: externs.Persistence;
}
Loading