PR feedback

Author: sam-gc

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

@@ -30,11 +30,7 @@ import { browserLocalPersistence } from '../persistence/browser';
 import { inMemoryPersistence } from '../persistence/in_memory';
 import { PersistenceUserManager } from '../persistence/persistence_user_manager';
 import { ClientPlatform, getClientVersion } from '../util/version';
-import {
-  DEFAULT_API_HOST,
-  DEFAULT_API_SCHEME,
-  initializeAuth
-} from './auth_impl';
+import { DEFAULT_API_HOST, DEFAULT_API_SCHEME, initializeAuth } from './auth_impl';
 
 use(sinonChai);
 
@@ -163,54 +159,90 @@ describe('AuthImpl', () => {
       });
 
       it('onAuthStateChange triggers on log in', async () => {
-        await auth.updateCurrentUser(null);
         auth.onAuthStateChanged(callback);
+        await auth.updateCurrentUser(null);
+        callback.resetHistory();
         await auth.updateCurrentUser(user);
-        expect(callback).to.have.been.calledTwice;
+        expect(callback).to.have.been.calledWith(user);
       });
 
       it('onAuthStateChange triggers on log out', async () => {
-        await auth.updateCurrentUser(user);
         auth.onAuthStateChanged(callback);
+        await auth.updateCurrentUser(user);
+        callback.resetHistory();
         await auth.updateCurrentUser(null);
-        expect(callback).to.have.been.calledTwice;
+        expect(callback).to.have.been.calledWith(null);
       });
 
       it('onIdTokenChange triggers on log in', async () => {
-        await auth.updateCurrentUser(null);
         auth.onIdTokenChange(callback);
+        await auth.updateCurrentUser(null);
+        callback.resetHistory();
         await auth.updateCurrentUser(user);
-        expect(callback).to.have.been.calledTwice;
+        expect(callback).to.have.been.calledWith(user);
       });
 
       it('onIdTokenChange triggers on log out', async () => {
-        await auth.updateCurrentUser(user);
         auth.onIdTokenChange(callback);
+        await auth.updateCurrentUser(user);
+        callback.resetHistory();
         await auth.updateCurrentUser(null);
-        expect(callback).to.have.been.calledTwice;
+        expect(callback).to.have.been.calledWith(null);
       });
 
       it('onAuthStateChange does not trigger for user props change', async () => {
-        await auth.updateCurrentUser(user);
         auth.onAuthStateChanged(callback);
+        await auth.updateCurrentUser(user);
+        callback.resetHistory();
         user.refreshToken = 'hey look I changed';
         await auth.updateCurrentUser(user);
-        expect(callback).to.have.been.calledOnce;
+        expect(callback).not.to.have.been.called;
       });
 
       it('onIdTokenChange triggers for user props change', async () => {
-        await auth.updateCurrentUser(user);
         auth.onIdTokenChange(callback);
+        await auth.updateCurrentUser(user);
+        callback.resetHistory();
         user.refreshToken = 'hey look I changed';
         await auth.updateCurrentUser(user);
-        expect(callback).to.have.been.calledTwice;
+        expect(callback).to.have.been.calledWith(user);
       });
 
       it('onAuthStateChange triggers if uid changes', async () => {
-        await auth.updateCurrentUser(user);
         auth.onAuthStateChanged(callback);
-        await auth.updateCurrentUser(testUser('different-uid'));
-        expect(callback).to.have.been.calledTwice;
+        await auth.updateCurrentUser(user);
+        callback.resetHistory();
+        const newUser = testUser('different-uid');
+        await auth.updateCurrentUser(newUser);
+        expect(callback).to.have.been.calledWith(newUser);
+      });
+
+      it('onAuthStateChange works for multiple listeners', async () => {
+        const cb1 = sinon.spy();
+        const cb2 = sinon.spy();
+        auth.onAuthStateChanged(cb1);
+        auth.onAuthStateChanged(cb2);
+        await auth.updateCurrentUser(null);
+        cb1.resetHistory();
+        cb2.resetHistory();
+
+        await auth.updateCurrentUser(user);
+        expect(cb1).to.have.been.calledWith(user);
+        expect(cb2).to.have.been.calledWith(user);
+      });
+
+      it('onIdTokenChange works for multiple listeners', async () => {
+        const cb1 = sinon.spy();
+        const cb2 = sinon.spy();
+        auth.onIdTokenChange(cb1);
+        auth.onIdTokenChange(cb2);
+        await auth.updateCurrentUser(null);
+        cb1.resetHistory();
+        cb2.resetHistory();
+
+        await auth.updateCurrentUser(user);
+        expect(cb1).to.have.been.calledWith(user);
+        expect(cb2).to.have.been.calledWith(user);
       });
     });
   });

packages-exp/auth-exp/src/core/user/reload.test.ts

@@ -20,14 +20,13 @@ import * as chaiAsPromised from 'chai-as-promised';
 import * as sinon from 'sinon';
 import * as sinonChai from 'sinon-chai';
 
+import { FirebaseError } from '@firebase/util';
+
 import { mockEndpoint } from '../../../test/api/helper';
 import { testUser } from '../../../test/mock_auth';
 import * as fetch from '../../../test/mock_fetch';
 import { Endpoint } from '../../api';
-import {
-  APIUserInfo,
-  ProviderUserInfo
-} from '../../api/account_management/account';
+import { APIUserInfo, ProviderUserInfo } from '../../api/account_management/account';
 import { UserInfo } from '../../model/user';
 import { ProviderId } from '../providers';
 import { _reloadWithoutSaving, reload } from './reload';
@@ -145,6 +144,27 @@ describe('reload()', () => {
     ]);
   });
 
+  it('throws an error if the providerData providerId is invalid', async () => {
+    const user = testUser('abc', '', true);
+    mockEndpoint(Endpoint.GET_ACCOUNT_INFO, {
+      users: [
+        {
+          providerUserInfo: [
+            {
+              ...BASIC_USER_INFO,
+              providerId: 'naaaah',
+            }
+          ]
+        }
+      ]
+    });
+
+    await expect(_reloadWithoutSaving(user)).to.be.rejectedWith(
+      FirebaseError,
+      'Firebase: An internal AuthError has occurred. (auth/internal-error).'
+    );
+  });
+
   it('reload calls auth.updateCurrentUser after completion', async () => {
     mockEndpoint(Endpoint.GET_ACCOUNT_INFO, {
       users: [{}]

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

@@ -15,10 +15,7 @@
  * limitations under the License.
  */
 
-import {
-  getAccountInfo,
-  ProviderUserInfo
-} from '../../api/account_management/account';
+import { getAccountInfo, ProviderUserInfo } from '../../api/account_management/account';
 import { User, UserInfo } from '../../model/user';
 import { ProviderId } from '../providers';
 import { assert } from '../util/assert';
@@ -32,7 +29,7 @@ export async function _reloadWithoutSaving(user: User): Promise<void> {
 
   const coreAccount = response.users[0];
   const newProviderData = coreAccount.providerUserInfo?.length
-    ? extractProviderData(coreAccount.providerUserInfo)
+    ? extractProviderData(coreAccount.providerUserInfo, auth.name)
     : [];
   const updates: Partial<User> = {
     uid: coreAccount.localId,
@@ -71,13 +68,16 @@ function mergeProviderData(
   return [...deduped, ...newData];
 }
 
-function extractProviderData(providers: ProviderUserInfo[]): UserInfo[] {
-  return providers.map(provider => ({
-    uid: provider.rawId || '',
-    displayName: provider.displayName || null,
-    email: provider.email || null,
-    phoneNumber: provider.phoneNumber || null,
-    providerId: provider.providerId as ProviderId,
-    photoURL: provider.photoUrl || null
-  }));
+function extractProviderData(providers: ProviderUserInfo[], appName: string): UserInfo[] {
+  return providers.map(({providerId, ...provider}) => {
+    assert(providerId && Object.values<string>(ProviderId).includes(providerId), appName);
+    return {
+      uid: provider.rawId || '',
+      displayName: provider.displayName || null,
+      email: provider.email || null,
+      phoneNumber: provider.phoneNumber || null,
+      providerId: providerId as ProviderId,
+      photoURL: provider.photoUrl || null
+    };
+  });
 }