Fix redirect

Author: sam-gc

common/api-review/auth-exp.api.md

@@ -156,7 +156,7 @@ export function getIdTokenResult(externUser: externs.User, forceRefresh?: boolea
 export function getMultiFactorResolver(auth: externs.Auth, errorExtern: externs.MultiFactorError): externs.MultiFactorResolver;
 
 // @public (undocumented)
-export function getRedirectResult(authExtern: externs.Auth, resolverExtern?: externs.PopupRedirectResolver): Promise<externs.UserCredential | null>;
+export function getRedirectResult(authExtern: externs.Auth, resolverExtern?: externs.PopupRedirectResolver, bypassAuthState?: boolean): Promise<externs.UserCredential | null>;
 
 // @public (undocumented)
 export class GithubAuthProvider extends OAuthProvider {
@@ -176,6 +176,7 @@ export class GithubAuthProvider extends OAuthProvider {
 
 // @public (undocumented)
 export class GoogleAuthProvider extends OAuthProvider {
+    constructor();
     // (undocumented)
     static credential(idToken?: string | null, accessToken?: string | null): externs.OAuthCredential;
     // (undocumented)

packages-exp/auth-exp/src/core/auth/auth_impl.ts

@@ -58,6 +58,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
@@ -114,7 +115,13 @@ export class AuthImpl implements Auth, _FirebaseService {
       }
 
       this._isInitialized = true;
-      this.notifyAuthListeners();
+    });
+
+    // After initialization completes, throw any error caused by redirect flow
+    this._initializationPromise.then(() => {
+      if (this.redirectInitializerError) {
+        throw this.redirectInitializerError;
+      }
     });
 
     return this._initializationPromise;
@@ -147,27 +154,34 @@ 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(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();
-      const result = await this._popupRedirectResolver!._completeRedirectFn(this, popupRedirectResolver);
-      if (result) {
-        return;
-      };
+      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.
+      if ((!redirectUserEventId || (redirectUserEventId === storedUserEventId)) && result?.user) {
+        storedUser = result.user as User
+      }
     }
 
-    const storedUser = (await this.assertedPersistence.getCurrentUser()) as User | null;
+    // 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);
     }
 
@@ -189,6 +203,36 @@ 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);
@@ -236,10 +280,10 @@ export class AuthImpl implements Auth, _FirebaseService {
       }
     );
 
-    return this._updateCurrentUser(user && user._clone(), false);
+    return this._updateCurrentUser(user && user._clone());
   }
 
-  async _updateCurrentUser(user: externs.User | null, isInternal = true): Promise<void> {
+  async _updateCurrentUser(user: externs.User | null): Promise<void> {
     if (this._deleted) {
       return;
     }
@@ -251,20 +295,10 @@ export class AuthImpl implements Auth, _FirebaseService {
       );
     }
 
-    const action = async (): Promise<void> => {
+    return this.queue(async () => {
       await this.directlySetCurrentUser(user as User | null);
       this.notifyAuthListeners();
-    };
-    
-    // Bypass the queue if this is an internal invocation and initialization is
-    // incomplete. This state only occurs if there's a pending redirect
-    // operation. That redirect operation was invoked from within the queue,
-    // meaning that we're safe to directly set the user
-    if (isInternal && !this._isInitialized) {
-      return action();
-    }
-
-    return this.queue(action);
+    });
   }
 
   async signOut(): Promise<void> {
@@ -273,7 +307,7 @@ export class AuthImpl implements Auth, _FirebaseService {
       await this._setRedirectUser(null);
     }
 
-    return this._updateCurrentUser(null, false);
+    return this._updateCurrentUser(null);
   }
 
   setPersistence(persistence: externs.Persistence): Promise<void> {
@@ -372,11 +406,7 @@ export class AuthImpl implements Auth, _FirebaseService {
 
   async _persistUserIfCurrent(user: User): Promise<void> {
     if (user === this.currentUser) {
-      if (!this._isInitialized) {
-        return this.directlySetCurrentUser(user);
-      } else {
-        return this.queue(async () => this.directlySetCurrentUser(user));
-      }
+      return this.queue(async () => this.directlySetCurrentUser(user));
     }
   }
 

packages-exp/auth-exp/src/core/providers/google.ts

@@ -29,6 +29,11 @@ export class GoogleAuthProvider extends OAuthProvider {
   static readonly PROVIDER_ID = externs.ProviderId.GOOGLE;
   readonly providerId = GoogleAuthProvider.PROVIDER_ID;
 
+  constructor() {
+    super(externs.ProviderId.GOOGLE);
+    this.addScope('profile');
+  }
+
   static credential(
     idToken?: string | null,
     accessToken?: string | null

packages-exp/auth-exp/src/core/strategies/credential.ts

@@ -29,7 +29,8 @@ import { _castAuth } from '../auth/auth_impl';
 
 export async function _signInWithCredential(
   auth: Auth,
-  credential: AuthCredential
+  credential: AuthCredential,
+  bypassAuthState = false
 ): Promise<UserCredential> {
   const operationType = OperationType.SIGN_IN;
   const response = await _processCredentialSavingMfaContextIfNecessary(
@@ -42,7 +43,10 @@ export async function _signInWithCredential(
     operationType,
     response
   );
-  await auth._updateCurrentUser(userCredential.user);
+
+  if (!bypassAuthState) {
+    await auth._updateCurrentUser(userCredential.user);
+  }
   return userCredential;
 }
 

packages-exp/auth-exp/src/core/strategies/idp.ts

@@ -39,6 +39,7 @@ export interface IdpTaskParams {
   postBody?: string;
   pendingToken?: string;
   user?: User;
+  bypassAuthState?: boolean;
 }
 
 export type IdpTask = (params: IdpTaskParams) => Promise<UserCredential>;
@@ -84,18 +85,19 @@ class IdpCredential extends AuthCredential {
 export function _signIn(params: IdpTaskParams): Promise<UserCredential> {
   return _signInWithCredential(
     params.auth,
-    new IdpCredential(params)
+    new IdpCredential(params),
+    params.bypassAuthState
   ) as Promise<UserCredential>;
 }
 
 export function _reauth(params: IdpTaskParams): Promise<UserCredential> {
   const { auth, user } = params;
   assert(user, AuthErrorCode.INTERNAL_ERROR, { appName: auth.name });
-  return _reauthenticate(user, new IdpCredential(params));
+  return _reauthenticate(user, new IdpCredential(params), params.bypassAuthState);
 }
 
 export async function _link(params: IdpTaskParams): Promise<UserCredential> {
   const { auth, user } = params;
   assert(user, AuthErrorCode.INTERNAL_ERROR, { appName: auth.name });
-  return _linkUser(user, new IdpCredential(params));
+  return _linkUser(user, new IdpCredential(params), params.bypassAuthState);
 }

packages-exp/auth-exp/src/core/user/invalidation.ts

@@ -22,8 +22,12 @@ import { AuthErrorCode } from '../errors';
 
 export async function _logoutIfInvalidated<T>(
   user: User,
-  promise: Promise<T>
+  promise: Promise<T>,
+  bypassAuthState = false
 ): Promise<T> {
+  if (bypassAuthState) {
+    return promise;
+  }
   try {
     return await promise;
   } catch (e) {

packages-exp/auth-exp/src/core/user/link_unlink.ts

@@ -60,11 +60,13 @@ export async function unlink(
  */
 export async function _link(
   user: User,
-  credential: AuthCredential
+  credential: AuthCredential,
+  bypassAuthState = false
 ): Promise<UserCredential> {
   const response = await _logoutIfInvalidated(
     user,
-    credential._linkToIdToken(user.auth, await user.getIdToken())
+    credential._linkToIdToken(user.auth, await user.getIdToken()),
+    bypassAuthState
   );
   return UserCredentialImpl._forOperation(
     user,

packages-exp/auth-exp/src/core/user/reauthenticate.ts

@@ -28,7 +28,8 @@ import { UserCredentialImpl } from './user_credential_impl';
 
 export async function _reauthenticate(
   user: User,
-  credential: AuthCredential
+  credential: AuthCredential,
+  bypassAuthState = false,
 ): Promise<UserCredentialImpl> {
   const appName = user.auth.name;
   const operationType = OperationType.REAUTHENTICATE;
@@ -41,7 +42,8 @@ export async function _reauthenticate(
         operationType,
         credential,
         user
-      )
+      ),
+      bypassAuthState,
     );
     assert(response.idToken, AuthErrorCode.INTERNAL_ERROR, { appName });
     const parsed = _parseToken(response.idToken);

packages-exp/auth-exp/src/model/popup_redirect.ts

@@ -94,5 +94,5 @@ export interface PopupRedirectResolver extends externs.PopupRedirectResolver {
   _redirectPersistence: externs.Persistence;
 
   // This is needed so that auth does not have a hard dependency on redirect
-  _completeRedirectFn: (auth: externs.Auth, resolver: externs.PopupRedirectResolver) => Promise<externs.UserCredential | null>;
+  _completeRedirectFn: (auth: externs.Auth, resolver: externs.PopupRedirectResolver, bypassAuthState: boolean) => Promise<externs.UserCredential | null>;
 }

packages-exp/auth-exp/src/platform_browser/strategies/abstract_popup_redirect_operation.ts

@@ -50,14 +50,16 @@ export abstract class AbstractPopupRedirectOperation
   private pendingPromise: PendingPromise | null = null;
   private eventManager: EventManager | null = null;
   readonly filter: AuthEventType[];
+  // private readonly bypassAuthState: boolean;
 
   abstract eventId: string | null;
 
   constructor(
     protected readonly auth: Auth,
     filter: AuthEventType | AuthEventType[],
     protected readonly resolver: PopupRedirectResolver,
-    protected user?: User
+    protected user?: User,
+    private readonly bypassAuthState = false,
   ) {
     this.filter = Array.isArray(filter) ? filter : [filter];
   }
@@ -91,7 +93,8 @@ export abstract class AbstractPopupRedirectOperation
       sessionId: sessionId!,
       tenantId: tenantId || undefined,
       postBody: postBody || undefined,
-      user: this.user
+      user: this.user,
+      bypassAuthState: this.bypassAuthState
     };
 
     try {

packages-exp/auth-exp/src/platform_browser/strategies/redirect.ts

@@ -98,24 +98,20 @@ export async function linkWithRedirect(
 
 export async function getRedirectResult(
   authExtern: externs.Auth,
-  resolverExtern?: externs.PopupRedirectResolver
+  resolverExtern?: externs.PopupRedirectResolver,
+  bypassAuthState = false,
 ): Promise<externs.UserCredential | null> {
   const auth = _castAuth(authExtern);
   const resolver = _withDefaultResolver(auth, resolverExtern);
-  const action = new RedirectAction(auth, resolver);
-  console.log('Executing redirect action');
+  const action = new RedirectAction(auth, resolver, bypassAuthState);
   const result = await action.execute();
 
-  if (result) {
+  if (result && !bypassAuthState) {
     delete result.user._redirectEventId;
-    console.log('Persisting user');
     await auth._persistUserIfCurrent(result.user as User);
-    console.log('Updating redirect user');
     await auth._setRedirectUser(null, resolverExtern);
-    console.log('Finished');
   }
 
-  console.log('Finished with total');
   return result;
 }
 
@@ -137,7 +133,7 @@ const redirectOutcomeMap: Map<
 class RedirectAction extends AbstractPopupRedirectOperation {
   eventId = null;
 
-  constructor(auth: Auth, resolver: PopupRedirectResolver) {
+  constructor(auth: Auth, resolver: PopupRedirectResolver, bypassAuthState = false) {
     super(
       auth,
       [
@@ -146,7 +142,9 @@ class RedirectAction extends AbstractPopupRedirectOperation {
         AuthEventType.REAUTH_VIA_REDIRECT,
         AuthEventType.UNKNOWN
       ],
-      resolver
+      resolver,
+      undefined,
+      bypassAuthState
     );
   }