Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Auth] Fix getRedirectResult behavior #5617

Merged
merged 3 commits into from
Oct 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/dry-toes-hammer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@firebase/auth-compat": minor
"@firebase/auth": minor
---

Fix behavior on subsequent calls to `getRedirectResult()`
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
Expand Down
6 changes: 4 additions & 2 deletions packages/auth/src/core/strategies/redirect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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);
});

Expand Down
6 changes: 6 additions & 0 deletions packages/auth/src/core/strategies/redirect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down
4 changes: 4 additions & 0 deletions packages/auth/test/integration/webdriver/redirect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down