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

* Handle anonymous auth re-login edge case

* Formatting

* Initial fixes

* Fix redirect

* clean up additional user info

* Formatting

* PR feedback

* Fix some tests

* Fix tests & write some new ones

* Fix broken build

* Formatting

* Formatting

* PR feedback

* Formatting

* PR feedback

Co-authored-by: avolkovi <[email protected]>

Author: sam-gc

packages-exp/auth-compat-exp/src/user_credential.ts

@@ -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))
   };
 }

packages-exp/auth-exp/src/api/account_management/profile.test.ts

@@ -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;

packages-exp/auth-exp/src/api/account_management/profile.ts

@@ -23,6 +23,7 @@ export interface UpdateProfileRequest {
   idToken: string;
   displayName?: string | null;
   photoUrl?: string | null;
+  returnSecureToken: boolean;
 }
 
 export interface UpdateProfileResponse extends IdTokenResponse {

packages-exp/auth-exp/src/api/authentication/custom_token.test.ts

@@ -32,7 +32,8 @@ use(chaiAsPromised);
 
 describe('api/authentication/signInWithCustomToken', () => {
   const request = {
-    token: 'my-token'
+    token: 'my-token',
+    returnSecureToken: true
   };
 
   let auth: TestAuth;

packages-exp/auth-exp/src/api/authentication/custom_token.ts

@@ -21,6 +21,7 @@ import { Auth } from '@firebase/auth-types-exp';
 
 export interface SignInWithCustomTokenRequest {
   token: string;
+  returnSecureToken: boolean;
 }
 
 export interface SignInWithCustomTokenResponse extends IdTokenResponse {}

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

@@ -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
@@ -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(() => {
+      if (this.redirectInitializerError) {
+        throw this.redirectInitializerError;
+      }
+    });
   }
 
   /**
@@ -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();
+      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);
     }
 
@@ -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);
@@ -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();
@@ -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;

packages-exp/auth-exp/src/core/auth/firebase_internal.test.ts

@@ -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'

packages-exp/auth-exp/src/core/auth/initialize.test.ts

@@ -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;

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

@@ -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({

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

@@ -73,6 +73,7 @@ export class GoogleAuthProvider extends OAuthProvider {
 
   constructor() {
     super(externs.ProviderId.GOOGLE);
+    this.addScope('profile');
   }
 
   /**

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

@@ -42,7 +42,8 @@ import { AUTH_ERROR_FACTORY, AuthErrorCode } from '../errors';
 import {
   linkWithCredential,
   reauthenticateWithCredential,
-  signInWithCredential
+  signInWithCredential,
+  _signInWithCredential
 } from './credential';
 
 use(chaiAsPromised);
@@ -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',

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

@@ -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(
@@ -43,7 +44,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/custom_token.test.ts

@@ -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]
     });
@@ -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);

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

@@ -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(