Firebase client always exposes all available credentials

Previously, when passing on the results of an authentication-related event, the Firebase client always exposed an object conforming to the Firebase `UserCredential` type. This contains exactly one user and one credential. In many cases the object itself was synthesized using credentials that we stored ourselves, but the shape of the data was consistent with what Firebase returns when a user successfully logs in. However, this only works if the user has only linked a single provider identity. In the general case, we always want access to all credentials we have for the user—e.g. if the user logs in with Google, we still want access to the stored GitHub credential if there is one. Similarly if the user is initially logged in, logs in in another tab, etc. So, the client ditches the `UserCredential` interface altogether, instead always exposing an object whose properties are `user` and `credentials`. The latter is an array of credential objects. The various consumers of this data are updated to handle a collection of credentials. Note that this also removes the storage of `additionalUserInfo`, i.e. raw profile data from the identity providers. Previously it was necessary to go spelunking in there to find a suitable display name for a user in the case that their account did not have an associated real name. However, since all user accounts will soon be Google-linked, we should be able to guarantee a good display name.
popcodeorg · Jul 17, 2018 · 7f02c4f · 7f02c4f
1 parent fff1964
commit 7f02c4f
Show file tree

Hide file tree

Showing 11 changed files with 87 additions and 123 deletions.
diff --git a/src/actions/user.js b/src/actions/user.js
@@ -1,5 +1,4 @@
 import {createAction} from 'redux-actions';
-import identity from 'lodash-es/identity';
 
 export const logIn = createAction(
   'LOG_IN',
@@ -8,10 +7,16 @@ export const logIn = createAction(
 
 export const linkGithubIdentity = createAction('LINK_GITHUB_IDENTITY');
 
-export const identityLinked = createAction('IDENTITY_LINKED');
+export const identityLinked = createAction(
+  'IDENTITY_LINKED',
+  credential => ({credential}),
+);
 
 export const logOut = createAction('LOG_OUT');
 
-export const userAuthenticated = createAction('USER_AUTHENTICATED', identity);
+export const userAuthenticated = createAction(
+  'USER_AUTHENTICATED',
+  (user, credentials) => ({user, credentials}),
+);
 
 export const userLoggedOut = createAction('USER_LOGGED_OUT');
diff --git a/src/channels/loginState.js b/src/channels/loginState.js
@@ -3,5 +3,7 @@ import {eventChannel} from 'redux-saga';
 import {onAuthStateChanged} from '../clients/firebase';
 
 export default eventChannel(
-  emit => onAuthStateChanged(userCredential => emit({userCredential})),
+  emit => onAuthStateChanged(
+    ({user, credentials}) => emit({user, credentials}),
+  ),
 );
diff --git a/src/clients/firebase.js b/src/clients/firebase.js
@@ -1,7 +1,9 @@
 import Cookies from 'js-cookie';
 import get from 'lodash-es/get';
+import isEqual from 'lodash-es/isEqual';
 import isNil from 'lodash-es/isNil';
 import isNull from 'lodash-es/isNull';
+import map from 'lodash-es/map';
 import omit from 'lodash-es/omit';
 import values from 'lodash-es/values';
 import uuid from 'uuid/v4';
@@ -20,9 +22,9 @@ const SESSION_TTL_MS = 5 * 60 * 1000;
 export function onAuthStateChanged(listener) {
   const unsubscribe = auth.onAuthStateChanged(async(user) => {
     if (isNull(user)) {
-      listener(null);
+      listener({user: null});
     } else {
-      listener(await userCredentialForUserData(user));
+      listener(await decorateUserWithCredentials(user));
     }
   });
   return unsubscribe;
@@ -65,21 +67,22 @@ export async function saveProject(uid, project) {
     setWithPriority(project, -Date.now());
 }
 
-async function userCredentialForUserData(user) {
+async function decorateUserWithCredentials(user) {
   const database = await loadDatabase();
-  const path = providerPath(user.uid, user.providerData[0].providerId);
-  const [credentialEvent, providerInfoEvent] = await Promise.all([
-    database.ref(`authTokens/${path}`).once('value'),
-    database.ref(`providerInfo/${path}`).once('value'),
-  ]);
-  const credential = credentialEvent.val();
-  const additionalUserInfo = providerInfoEvent.val();
-  if (isNil(credential)) {
+  const credentialEvent =
+    await database.ref(`authTokens/${user.uid}`).once('value');
+  const credentials = values(credentialEvent.val() || {});
+  if (
+    !isEqual(
+      map(credentials, 'providerId').sort(),
+      map(user.providerData, 'providerId').sort(),
+    )
+  ) {
     await auth.signOut();
-    return null;
+    return {user: null};
   }
 
-  return {user, credential, additionalUserInfo};
+  return {user, credentials};
 }
 
 export async function signIn(provider) {
@@ -105,7 +108,7 @@ export async function linkGithub() {
   const userCredential =
     await auth.currentUser.linkWithPopup(githubAuthProvider);
   await saveUserCredential(userCredential);
-  return userCredential;
+  return userCredential.credential;
 }
 
 async function signInWithGithub() {
@@ -132,28 +135,13 @@ export async function signOut() {
 async function saveUserCredential({
   user: {uid},
   credential,
-  additionalUserInfo,
 }) {
-  await Promise.all([
-    saveProviderInfo(uid, additionalUserInfo),
-    saveCredentials(uid, credential),
-  ]);
-}
-
-async function saveCredentials(uid, credential) {
   const database = await loadDatabase();
   await database.
     ref(`authTokens/${providerPath(uid, credential.providerId)}`).
     set(credential);
 }
 
-async function saveProviderInfo(uid, providerInfo) {
-  const database = await loadDatabase();
-  await database.
-    ref(`providerInfo/${providerPath(uid, providerInfo.providerId)}`).
-    set(providerInfo);
-}
-
 function providerPath(uid, providerId) {
   return `${uid}/${providerId.replace('.', '_')}`;
 }

diff --git a/src/reducers/user.js b/src/reducers/user.js
@@ -1,4 +1,4 @@
-import get from 'lodash-es/get';
+import reduce from 'lodash-es/reduce';
 
 import {User, UserAccount} from '../records';
 import {LoginState} from '../enums';
@@ -27,23 +27,19 @@ function user(stateIn, action) {
 
   switch (action.type) {
     case 'USER_AUTHENTICATED': {
-      const {user: userData, credential, additionalUserInfo} = action.payload;
+      const {user: userData, credentials} = action.payload;
 
-      const profileData = get(userData, ['providerData', 0], userData);
-
-      return addCredential(
+      return reduce(
+        credentials,
+        addCredential,
         state.merge({
           loginState: LoginState.AUTHENTICATED,
           account: new UserAccount({
             id: userData.uid,
-            displayName: profileData.displayName || get(
-              additionalUserInfo,
-              'username',
-            ),
-            avatarUrl: profileData.photoURL,
+            displayName: userData.displayName,
+            avatarUrl: userData.photoURL,
           }),
         }),
-        credential,
       );
     }
 

diff --git a/src/sagas/user.js b/src/sagas/user.js
@@ -1,4 +1,5 @@
 import {bugsnagClient} from '../util/bugsnag';
+import isEmpty from 'lodash-es/isEmpty';
 import isError from 'lodash-es/isError';
 import isString from 'lodash-es/isString';
 import {all, call, put, take, takeEvery} from 'redux-saga/effects';
@@ -25,32 +26,33 @@ export function* applicationLoaded() {
 }
 
 function* handleInitialAuth() {
-  const {userCredential} = yield take(loginState);
-  if (isNil(userCredential)) {
+  const {user, credentials} = yield take(loginState);
+
+  if (isNil(user)) {
     yield put(userLoggedOut());
   } else {
-    if (isNil(userCredential.credential)) {
+    if (isEmpty(credentials)) {
       yield call(signOut);
       return;
     }
 
     const sessionUid = yield call(getSessionUid);
-    if (userCredential.user.uid !== sessionUid) {
+    if (user.uid !== sessionUid) {
       yield call(signOut);
       return;
     }
 
-    yield put(userAuthenticated(userCredential));
+    yield put(userAuthenticated(user, credentials));
   }
 }
 
 function* handleAuthChange() {
   while (true) {
-    const {userCredential} = yield take(loginState);
-    if (isNil(userCredential)) {
+    const {user, credentials} = yield take(loginState);
+    if (isNil(user)) {
       yield put(userLoggedOut());
     } else {
-      yield put(userAuthenticated(userCredential));
+      yield put(userAuthenticated(user, credentials));
     }
   }
 }
@@ -98,12 +100,8 @@ export function* logIn({payload: {provider}}) {
 }
 
 export function* linkGithubIdentity() {
-  try {
-    const userCredential = yield call(linkGithub);
-    yield put(identityLinked(userCredential));
-  } catch (e) {
-    debugger;
-  }
+  const credential = yield call(linkGithub);
+  yield put(identityLinked(credential));
 }
 
 export function* logOut() {

diff --git a/test/helpers/Scenario.js b/test/helpers/Scenario.js
@@ -1,3 +1,5 @@
+import partial from 'lodash-es/partial';
+
 import reduce from '../../src/reducers';
 import {
   projectCreated,
@@ -7,7 +9,7 @@ import {
 } from '../../src/actions/user';
 import Analyzer from '../../src/analyzers';
 
-import {userCredential} from './factory';
+import {userWithCredentials} from './factory';
 
 export default class Scenario {
   constructor() {
@@ -16,8 +18,9 @@ export default class Scenario {
   }
 
   logIn() {
-    this.userCredential = userCredential();
-    this._reduce(userAuthenticated, this.userCredential);
+    const {user, credentials} = this.userWithCredentials =
+      userWithCredentials();
+    this._reduce(partial(userAuthenticated, user, credentials));
     return this;
   }
 

diff --git a/test/helpers/factory.js b/test/helpers/factory.js
@@ -29,31 +29,20 @@ export function gistData({
   return {files};
 }
 
-export function userCredential({
+export function userWithCredentials({
   user: userIn,
   credential: credentialIn,
-  additionalUserInfo: additionalUserInfoIn,
 } = {}) {
   return {
     user: user(userIn),
-    credential: credential(credentialIn),
-    additionalUserInfo: additionalUserInfo(additionalUserInfoIn),
+    credentials: [credential(credentialIn)],
   };
 }
 
 export function user(userIn) {
   return defaultsDeep({}, userIn, {
-    displayName: null,
-    photoURL: null,
-    providerData: [
-      {
-        displayName: 'Popcode User',
-        email: 'popcodeuser@example.com',
-        photoURL: 'https://camo.github.com/popcodeuser.jpg',
-        providerId: 'github.com',
-        uid: '345',
-      },
-    ],
+    displayName: 'Popcode User',
+    photoURL: 'https://camo.github.com/popcodeuser.jpg',
     uid: 'abc123',
   });
 }
@@ -78,11 +67,3 @@ export function credential(credentialIn) {
     providerId: 'github.com',
   }, credentialIn);
 }
-
-export function additionalUserInfo(additionalUserInfoIn) {
-  return defaultsDeep({}, additionalUserInfoIn, {
-    profile: {},
-    providerId: 'github.com',
-    username: 'popcoder',
-  });
-}
diff --git a/test/unit/reducers/ui.js b/test/unit/reducers/ui.js
@@ -244,7 +244,7 @@ test('linkGithubIdentity', (t) => {
 test('identityLinked', reducerTest(
   reducer,
   initialState,
-  partial(identityLinked, {credential: {providerId: 'github.com'}}),
+  partial(identityLinked, {providerId: 'github.com'}),
   withNotification('identity-linked', 'notice', {provider: 'github.com'}),
 ));
 

diff --git a/test/unit/reducers/user.js b/test/unit/reducers/user.js
@@ -4,7 +4,7 @@ import {Map} from 'immutable';
 
 import reducerTest from '../../helpers/reducerTest';
 import {user as states} from '../../helpers/referenceStates';
-import {userCredential} from '../../helpers/factory';
+import {userWithCredentials} from '../../helpers/factory';
 import reducer from '../../../src/reducers/user';
 import {
   identityLinked,
@@ -14,7 +14,7 @@ import {
 import {LoginState} from '../../../src/enums';
 import {User, UserAccount} from '../../../src/records';
 
-const userCredentialIn = userCredential();
+const userWithCredentialsIn = userWithCredentials();
 
 const loggedOutState = new User({
   loginState: LoginState.ANONYMOUS,
@@ -23,43 +23,32 @@ const loggedOutState = new User({
 const loggedInState = new User({
   loginState: LoginState.AUTHENTICATED,
   account: new UserAccount({
-    id: userCredentialIn.user.uid,
-    displayName: userCredentialIn.user.providerData[0].displayName,
-    avatarUrl: userCredentialIn.user.providerData[0].photoURL,
+    id: userWithCredentialsIn.user.uid,
+    displayName: userWithCredentialsIn.user.displayName,
+    avatarUrl: userWithCredentialsIn.user.photoURL,
     accessTokens: new Map({
-      'github.com': userCredentialIn.credential.accessToken,
+      'github.com': userWithCredentialsIn.credentials[0].accessToken,
     }),
   }),
 });
 
-test('userAuthenticated', (t) => {
-  t.test('with displayName', reducerTest(
-    reducer,
-    states.initial,
-    partial(userAuthenticated, userCredentialIn),
-    loggedInState,
-  ));
-
-  t.test('with no displayName', reducerTest(
-    reducer,
-    states.initial,
-    partial(
-      userAuthenticated,
-      userCredential({user: {providerData: [{displayName: null}]}}),
-    ),
-    loggedInState.update(
-      'account',
-      account => account.set('displayName', 'popcoder'),
-    ),
-  ));
-});
+test('userAuthenticated', reducerTest(
+  reducer,
+  states.initial,
+  partial(
+    userAuthenticated,
+    userWithCredentialsIn.user,
+    userWithCredentialsIn.credentials,
+  ),
+  loggedInState,
+));
 
 test('identityLinked', reducerTest(
   reducer,
   loggedInState,
   partial(
     identityLinked,
-    {credential: {providerId: 'google.com', idToken: 'abc'}},
+    {providerId: 'google.com', idToken: 'abc'},
   ),
   loggedInState.setIn(['account', 'accessTokens', 'google.com'], 'abc'),
 ));