Skip to content

A whole bunch of things to bring auth (compat) to parity #3970

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 15 commits into from
Oct 29, 2020
6 changes: 3 additions & 3 deletions packages-exp/auth-compat-exp/src/user_credential.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,10 @@ export async function convertConfirmationResult(
auth: externs.Auth,
confirmationResultPromise: Promise<externs.ConfirmationResult>
): Promise<compat.ConfirmationResult> {
const { verificationId, confirm } = await confirmationResultPromise;
const confirmationResultExp = await confirmationResultPromise;
return {
verificationId,
verificationId: confirmationResultExp.verificationId,
confirm: (verificationCode: string) =>
convertCredential(auth, confirm(verificationCode))
convertCredential(auth, confirmationResultExp.confirm(verificationCode))
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ describe('api/account_management/updateProfile', () => {
const request = {
idToken: 'my-token',
email: '[email protected]',
password: 'my-password'
password: 'my-password',
returnSecureToken: true
};

let auth: TestAuth;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export interface UpdateProfileRequest {
idToken: string;
displayName?: string | null;
photoUrl?: string | null;
returnSecureToken: boolean;
}

export interface UpdateProfileResponse extends IdTokenResponse {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ use(chaiAsPromised);

describe('api/authentication/signInWithCustomToken', () => {
const request = {
token: 'my-token'
token: 'my-token',
returnSecureToken: true
};

let auth: TestAuth;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { Auth } from '@firebase/auth-types-exp';

export interface SignInWithCustomTokenRequest {
token: string;
returnSecureToken: boolean;
}

export interface SignInWithCustomTokenResponse extends IdTokenResponse {}
Expand Down
87 changes: 77 additions & 10 deletions packages-exp/auth-exp/src/core/auth/auth_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export class AuthImpl implements Auth, _FirebaseService {
private idTokenSubscription = new Subscription<externs.User>(this);
private redirectUser: User | null = null;
private isProactiveRefreshEnabled = false;
private redirectInitializerError: Error | null = null;

// Any network calls will set this to true and prevent subsequent emulator
// initialization
Expand Down Expand Up @@ -109,17 +110,21 @@ export class AuthImpl implements Auth, _FirebaseService {
return;
}

await this.initializeCurrentUser();
await this.initializeCurrentUser(popupRedirectResolver);

if (this._deleted) {
return;
}

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

return this._initializationPromise;
// After initialization completes, throw any error caused by redirect flow
return this._initializationPromise.then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why then instead of await?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we await it, the error becomes part of the control flow of initialization. If there's an error with redirect during initialization, we still want auth to finish initializing

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we just wrap in a try catch then?

if (this.redirectInitializerError) {
throw this.redirectInitializerError;
}
});
}

/**
Expand Down Expand Up @@ -149,18 +154,40 @@ export class AuthImpl implements Auth, _FirebaseService {

// Update current Auth state. Either a new login or logout.
await this._updateCurrentUser(user);
// Notify external Auth changes of Auth change event.
this.notifyAuthListeners();
}

private async initializeCurrentUser(): Promise<void> {
const storedUser = (await this.assertedPersistence.getCurrentUser()) as User | null;
private async initializeCurrentUser(
popupRedirectResolver?: externs.PopupRedirectResolver
): Promise<void> {
// First check to see if we have a pending redirect event.
let storedUser = (await this.assertedPersistence.getCurrentUser()) as User | null;
if (popupRedirectResolver) {
await this.getOrInitRedirectPersistenceManager();
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have this method return the redirectUser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no; the purpose of this method is to determine which user should be set as currentUser. The method first determines that, then updates it.

const redirectUserEventId = this.redirectUser?._redirectEventId;
const storedUserEventId = storedUser?._redirectEventId;
const result = await this.tryRedirectSignIn(popupRedirectResolver);

// If the stored user (i.e. the old "currentUser") has a redirectId that
// matches the redirect user, then we want to initially sign in with the
// new user object from result.
// TODO(samgho): More thoroughly test all of this
if (
(!redirectUserEventId || redirectUserEventId === storedUserEventId) &&
result?.user
) {
storedUser = result.user as User;
}
}

// If no user in persistence, there is no current user. Set to null.
if (!storedUser) {
return this.directlySetCurrentUser(storedUser);
return this.directlySetCurrentUser(null);
}

if (!storedUser._redirectEventId) {
// This isn't a redirect user, we can reload and bail
// This will also catch the redirected user, if available, as that method
// strips the _redirectEventId
return this.reloadAndSetCurrentUserOrClear(storedUser);
}

Expand All @@ -182,6 +209,42 @@ export class AuthImpl implements Auth, _FirebaseService {
return this.reloadAndSetCurrentUserOrClear(storedUser);
}

private async tryRedirectSignIn(
redirectResolver: externs.PopupRedirectResolver
): Promise<externs.UserCredential | null> {
// The redirect user needs to be checked (and signed in if available)
// during auth initialization. All of the normal sign in and link/reauth
// flows call back into auth and push things onto the promise queue. We
// need to await the result of the redirect sign in *inside the promise
// queue*. This presents a problem: we run into deadlock. See:
// ┌> [Initialization] ─────┐
// ┌> [<other queue tasks>] │
// └─ [getRedirectResult] <─┘
// where [] are tasks on the queue and arrows denote awaits
// Initialization will never complete because it's waiting on something
// that's waiting for initialization to complete!
//
// Instead, this method calls getRedirectResult() (stored in
// _completeRedirectFn) with an optional parameter that instructs all of
// the underlying auth operations to skip anything that mutates auth state.

let result: externs.UserCredential | null = null;
try {
// We know this._popupRedirectResolver is set since redirectResolver
// is passed in. The _completeRedirectFn expects the unwrapped extern.
result = await this._popupRedirectResolver!._completeRedirectFn(
this,
redirectResolver,
true
);
} catch (e) {
this.redirectInitializerError = e;
await this._setRedirectUser(null);
}

return result;
}

private async reloadAndSetCurrentUserOrClear(user: User): Promise<void> {
try {
await _reloadWithoutSaving(user);
Expand Down Expand Up @@ -243,6 +306,7 @@ export class AuthImpl implements Auth, _FirebaseService {
{ appName: this.name }
);
}

return this.queue(async () => {
await this.directlySetCurrentUser(user as User | null);
this.notifyAuthListeners();
Expand Down Expand Up @@ -335,8 +399,11 @@ export class AuthImpl implements Auth, _FirebaseService {
}

async _redirectUserForId(id: string): Promise<User | null> {
// Make sure we've cleared any pending ppersistence actions
await this.queue(async () => {});
// Make sure we've cleared any pending persistence actions if we're not in
// the initializer
if (this._isInitialized) {
await this.queue(async () => {});
}

if (this._currentUser?._redirectEventId === id) {
return this._currentUser;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ describe('core/auth/firebase_internal', () => {
const user = testUser(auth, 'uid');
await auth._updateCurrentUser(user);
user.stsTokenManager.accessToken = 'access-token';
user.stsTokenManager.refreshToken = 'refresh-token';
user.stsTokenManager.expirationTime = Date.now() + 1000 * 60 * 60 * 24;
expect(await authInternal.getToken()).to.eql({
accessToken: 'access-token'
Expand Down
7 changes: 7 additions & 0 deletions packages-exp/auth-exp/src/core/auth/initialize.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,13 @@ describe('core/auth/initialize', () => {
): void {
cb(true);
}
async _completeRedirectFn(
_auth: externs.Auth,
_resolver: externs.PopupRedirectResolver,
_bypassAuthState: boolean
): Promise<externs.UserCredential | null> {
return null;
}
}

const fakePopupRedirectResolver: externs.PopupRedirectResolver = FakePopupRedirectResolver;
Expand Down
6 changes: 6 additions & 0 deletions packages-exp/auth-exp/src/core/providers/google.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ describe('core/providers/google', () => {
expect(cred.signInMethod).to.eq(SignInMethod.GOOGLE);
});

it('adds the profile scope by default', () => {
const provider = new GoogleAuthProvider();
expect(provider.providerId).to.eq(ProviderId.GOOGLE);
expect(provider.getScopes()).to.eql(['profile']);
});

it('credentialFromResult creates the cred from a tagged result', async () => {
const auth = await testAuth();
const userCred = new UserCredentialImpl({
Expand Down
1 change: 1 addition & 0 deletions packages-exp/auth-exp/src/core/providers/google.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export class GoogleAuthProvider extends OAuthProvider {

constructor() {
super(externs.ProviderId.GOOGLE);
this.addScope('profile');
}

/**
Expand Down
12 changes: 11 additions & 1 deletion packages-exp/auth-exp/src/core/strategies/credential.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ import { AUTH_ERROR_FACTORY, AuthErrorCode } from '../errors';
import {
linkWithCredential,
reauthenticateWithCredential,
signInWithCredential
signInWithCredential,
_signInWithCredential
} from './credential';

use(chaiAsPromised);
Expand Down Expand Up @@ -111,6 +112,15 @@ describe('core/strategies/credential', () => {
expect(auth.currentUser).to.eq(user);
});

it('does not update the current user if bypass is true', async () => {
stub(authCredential, '_getIdTokenResponse').returns(
Promise.resolve(idTokenResponse)
);
const { user } = await _signInWithCredential(auth, authCredential, true);
expect(auth.currentUser).to.be.null;
expect(user).not.to.be.null;
});

it('should handle MFA', async () => {
const serverResponse: IdTokenMfaResponse = {
localId: 'uid',
Expand Down
8 changes: 6 additions & 2 deletions packages-exp/auth-exp/src/core/strategies/credential.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ import { _castAuth } from '../auth/auth_impl';
/** @internal */
export async function _signInWithCredential(
auth: Auth,
credential: AuthCredential
credential: AuthCredential,
bypassAuthState = false
): Promise<UserCredential> {
const operationType = OperationType.SIGN_IN;
const response = await _processCredentialSavingMfaContextIfNecessary(
Expand All @@ -43,7 +44,10 @@ export async function _signInWithCredential(
operationType,
response
);
await auth._updateCurrentUser(userCredential.user);

if (!bypassAuthState) {
await auth._updateCurrentUser(userCredential.user);
}
return userCredential;
}

Expand Down
14 changes: 13 additions & 1 deletion packages-exp/auth-exp/src/core/strategies/custom_token.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,15 @@ describe('core/strategies/signInWithCustomToken', () => {
};

let auth: TestAuth;
let signInRoute: mockFetch.Route;

beforeEach(async () => {
auth = await testAuth();
mockFetch.setUp();
mockEndpoint(Endpoint.SIGN_IN_WITH_CUSTOM_TOKEN, idTokenResponse);
signInRoute = mockEndpoint(
Endpoint.SIGN_IN_WITH_CUSTOM_TOKEN,
idTokenResponse
);
mockEndpoint(Endpoint.GET_ACCOUNT_INFO, {
users: [serverUser]
});
Expand All @@ -78,6 +82,14 @@ describe('core/strategies/signInWithCustomToken', () => {
expect(operationType).to.eq(OperationType.SIGN_IN);
});

it('should send with a valid request', async () => {
await signInWithCustomToken(auth, 'j.w.t');
expect(signInRoute.calls[0].request).to.eql({
token: 'j.w.t',
returnSecureToken: true
});
});

it('should update the current user', async () => {
const { user } = await signInWithCustomToken(auth, 'oh.no');
expect(auth.currentUser).to.eq(user);
Expand Down
3 changes: 2 additions & 1 deletion packages-exp/auth-exp/src/core/strategies/custom_token.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ export async function signInWithCustomToken(
customToken: string
): Promise<externs.UserCredential> {
const response: IdTokenResponse = await getIdTokenResponse(auth, {
token: customToken
token: customToken,
returnSecureToken: true
});
const authInternal = _castAuth(auth);
const cred = await UserCredentialImpl._fromIdTokenResponse(
Expand Down
Loading