[Auth] Fix getRedirectResult behavior (#5617)

* Fix getRedirectResult behavior * Fix compat test * Add changeset
firebase · Oct 14, 2021 · b6f30c2 · b6f30c2
1 parent 5a9a144
commit b6f30c2
Show file tree

Hide file tree

Showing 7 changed files with 32 additions and 8 deletions.
diff --git a/.changeset/dry-toes-hammer.md b/.changeset/dry-toes-hammer.md
@@ -0,0 +1,6 @@
+---
+"@firebase/auth-compat": minor
+"@firebase/auth": minor
+---
+
+Fix behavior on subsequent calls to `getRedirectResult()`
diff --git a/packages/auth-compat/test/integration/webdriver/static/redirect.js b/packages/auth-compat/test/integration/webdriver/static/redirect.js
@@ -39,8 +39,14 @@ export function idpLinkRedirect() {
     .currentUser.linkWithRedirect(new compat.auth.GoogleAuthProvider());
 }
 
-export function redirectResult() {
-  return compat.auth().getRedirectResult();
+export async function redirectResult() {
+  const result = await compat.auth().getRedirectResult();
+  if (result.user === null && result.credential === null) {
+    // In the new SDK (and consequently the tests), null is returned instead of
+    // a credential with a null user/cred
+    return null;
+  }
+  return result;
 }
 
 export async function generateCredentialFromRedirectResultAndStore() {

diff --git a/packages/auth/src/core/strategies/abstract_popup_redirect_operation.ts b/packages/auth/src/core/strategies/abstract_popup_redirect_operation.ts
@@ -58,7 +58,7 @@ export abstract class AbstractPopupRedirectOperation
     filter: AuthEventType | AuthEventType[],
     protected readonly resolver: PopupRedirectResolverInternal,
     protected user?: UserInternal,
-    private readonly bypassAuthState = false
+    protected readonly bypassAuthState = false
   ) {
     this.filter = Array.isArray(filter) ? filter : [filter];
   }

diff --git a/packages/auth/src/core/strategies/redirect.test.ts b/packages/auth/src/core/strategies/redirect.test.ts
@@ -116,7 +116,7 @@ describe('core/strategies/redirect', () => {
     expect(await promise).to.eq(cred);
   });
 
-  it('returns the same value if called multiple times', async () => {
+  it('returns null after the first call', async () => {
     const cred = new UserCredentialImpl({
       user: testUser(auth, 'uid'),
       providerId: ProviderId.GOOGLE,
@@ -128,7 +128,7 @@ describe('core/strategies/redirect', () => {
       type: AuthEventType.SIGN_IN_VIA_REDIRECT
     });
     expect(await promise).to.eq(cred);
-    expect(await redirectAction.execute()).to.eq(cred);
+    expect(await redirectAction.execute()).to.be.null;
   });
 
   it('interacts with redirectUser loading from auth object', async () => {
@@ -192,6 +192,8 @@ describe('core/strategies/redirect', () => {
       type: AuthEventType.REAUTH_VIA_REDIRECT
     });
     expect(await promise).to.eq(cred);
+
+    // In this case, bypassAuthState is true... The value won't be cleared
     expect(await redirectAction.execute()).to.eq(cred);
   });
 

diff --git a/packages/auth/src/core/strategies/redirect.ts b/packages/auth/src/core/strategies/redirect.ts
@@ -79,6 +79,12 @@ export class RedirectAction extends AbstractPopupRedirectOperation {
       redirectOutcomeMap.set(this.auth._key(), readyOutcome);
     }
 
+    // If we're not bypassing auth state, the ready outcome should be set to
+    // null.
+    if (!this.bypassAuthState) {
+      redirectOutcomeMap.set(this.auth._key(), () => Promise.resolve(null));
+    }
+
     return readyOutcome();
   }
 

diff --git a/packages/auth/src/platform_browser/strategies/redirect.test.ts b/packages/auth/src/platform_browser/strategies/redirect.test.ts
@@ -329,7 +329,7 @@ describe('platform_browser/strategies/redirect', () => {
       expect(await promise).to.eq(cred);
     });
 
-    it('returns the same value if called multiple times', async () => {
+    it('returns null after the first call', async () => {
       const cred = new UserCredentialImpl({
         user: testUser(auth, 'uid'),
         providerId: ProviderId.GOOGLE,
@@ -341,7 +341,7 @@ describe('platform_browser/strategies/redirect', () => {
         type: AuthEventType.SIGN_IN_VIA_REDIRECT
       });
       expect(await promise).to.eq(cred);
-      expect(await getRedirectResult(auth, resolver)).to.eq(cred);
+      expect(await getRedirectResult(auth, resolver)).to.be.null;
     });
 
     it('interacts with redirectUser loading from auth object', async () => {
@@ -405,7 +405,7 @@ describe('platform_browser/strategies/redirect', () => {
         type: AuthEventType.REAUTH_VIA_REDIRECT
       });
       expect(await promise).to.eq(cred);
-      expect(await getRedirectResult(auth, resolver)).to.eq(cred);
+      expect(await getRedirectResult(auth, resolver)).to.be.null;
     });
 
     it('removes the redirect user and clears eventId from currentuser', async () => {

diff --git a/packages/auth/test/integration/webdriver/redirect.test.ts b/packages/auth/test/integration/webdriver/redirect.test.ts
@@ -64,6 +64,10 @@ browserDescribe('WebDriver redirect IdP test', driver => {
     );
     expect(redirectResult.operationType).to.eq(OperationType.SIGN_IN);
     expect(redirectResult.user).to.eql(currentUser);
+
+    // After the first call to redirect result, redirect result should be
+    // null
+    expect(await driver.call(RedirectFunction.REDIRECT_RESULT)).to.be.null;
   });
 
   it('can link with another account account', async () => {