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

Throw error message if user has no email from firebase #121

Conversation

MichaelDimitras
Copy link
Contributor

@MichaelDimitras MichaelDimitras commented Oct 1, 2024

If there are multiple users with the same email in Identity Platform for a particular tenant, an authentication attempt to firebase will return a user with an empty email.

On the frontend we display an error message when that happens:
https://github.com/p0-security/app/blob/5237e1a54597dc63258e56566353ad09ecd3e187/frontend/src/components/Login/hook.ts#L301-L306

But on the CLI we had no such check. This PR adds the same check as on the frontend. Now when a user logs in, if they have multiple identities in Identity Platform they will see the message Can not sign in: this user has previously signed in with a different identity provider.\nPlease contact support@p0.dev to enable this user.

login.bug.mov

@MichaelDimitras
Copy link
Contributor Author

The ticket has the additional goal:

Ideally, our system will automatically update the user profile in identity platform.

I will take a stab at that as a follow-up, now that I have some understanding of the system.

user: {},
});
await expect(login({ org: "test-org" })).rejects.toMatchInlineSnapshot(`
"Can not sign in: this user has previously signed in with a different identity provider.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is a little brittle, as any change to the message will cause the test to fail and require update. Is it not sufficient to verify that an exception is thrown?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to follow what we were already doing in this file.

await expect(login({ org: "test-org" })).rejects.toMatchInlineSnapshot(
    `"Could not find organization"`
);

await expect(login({ org: "test-org" })).rejects.toMatchInlineSnapshot(
    `"Unsupported login for your organization"`
    `"Unsupported login for your organization"`
);

Personally I like expecting on a particular message to ensure we are displaying the correct message to the user. Theoretically a logic change could expose an error with a different message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine. It's only a yarn jest -u to update snapshots.

@MichaelDimitras MichaelDimitras merged commit a043f43 into main Oct 1, 2024
3 checks passed
@MichaelDimitras MichaelDimitras deleted the michaeldimitras/eng-2676-p0-login-xxx-succeeds-without-user-update branch October 1, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants