Expose INVALID_LOGIN_CREDENTIALS as auth/invalid-credential error. #7772
Conversation
🦋 Changeset detectedLatest commit: 89f01c0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
prameshj
force-pushed
the
email-enum-docs2
branch
from
54201a8
to
b160e44
Compare
prameshj
force-pushed
the
email-enum-docs2
branch
2 times, most recently
from
c045b6a
to
3ec9b5e
Compare
Update the doc snippets for various SDK methods to explain the behavior when Email Enumeration Protection is enabled. Mark fetchSignInMethodsForEmail and updateEmail as deprecated. Update the demo app to use the error code. Fix error message for the error code and update tests.
prameshj
force-pushed
the
email-enum-docs2
branch
from
3ec9b5e
to
9291d5c
Compare
prameshj
requested review from
a team,
egilmorez,
markarndt,
kevinthecheung,
lisajian,
renkelvin and
sam-gc
as code owners
Co-authored-by: Kevin Cheung <kevinthecheung@users.noreply.github.com>
| @@ -528,6 +528,7 @@ export const AUTH_ERROR_CODES_MAP_DO_NOT_USE_INTERNALLY = { | |||
| INVALID_EMAIL: 'auth/invalid-email', | |||
| INVALID_EMULATOR_SCHEME: 'auth/invalid-emulator-scheme', | |||
| INVALID_IDP_RESPONSE: 'auth/invalid-credential', | |||
| INVALID_LOGIN_CREDENTIALS: 'auth/invalid-credential', | |||
There was a problem hiding this comment.
Should this be "auth/invalid-login-credentials", the missing code reported in #7661?
There was a problem hiding this comment.
We are reusing the existing error code 'auth/invalid-credential' since it is very similar to the backend error and is already public on the SDK.
There was a problem hiding this comment.
Is there a reason not to create a new error type?
There was a problem hiding this comment.
I am using firebase 10.6.0 and it is returning auth/invalid-login-credentials when there is an incorrect username or password. Will this change to start returning auth/invalid-credential in later updates?
There was a problem hiding this comment.
I am using firebase 10.6.0 and it is returning auth/invalid-login-credentials when there is an incorrect username or password. Will this change to start returning auth/invalid-credential in later updates?
That's correct. It is currently converting the server error "INVALID_LOGIN_CREDENTIALS" to lower-case and sending that error code -
We will now populate the error map so the server error maps to "auth/invalid-credential"
Is there a reason not to create a new error type?
This is to avoid creating another error code that is very similar to the existing invalid-credential code and to be consistent across iOS, Android and Web. On Android, we are reusing the https://firebase.google.com/docs/reference/android/com/google/firebase/auth/FirebaseAuthInvalidCredentialsException rather than create a new one, so following a similar approach on Web. Do you see any issue with this approach?
Changing behavior from auth/invalid-login-credentials (a non-exposed error code) to auth/invalid-credential (a public error code) can be a breaking change, but i am not sure, since the previous error code was not public. WDYT @DellaBitta ?
There was a problem hiding this comment.
Sounds like a good reason to me. Thanks for the clarification!
docs-devsite/auth.md
Outdated
| @@ -412,7 +413,7 @@ export declare function fetchSignInMethodsForEmail(auth: Auth, email: string): P | |||
| | Parameter | Type | Description | | |||
| | --- | --- | --- | | |||
| | auth | [Auth](./auth.auth.md#auth_interface) | The [Auth](./auth.auth.md#auth_interface) instance. | | |||
| | email | string | The user's email address. | | |||
| | email | string | The user's email address.<!-- -->Deprecated Migrating off of this method is recommended as a security best-practice. | | |||
There was a problem hiding this comment.
Missing a period here. Based on the other Deprecated things in this document, I think it should be 'Deprecated.'
| @@ -528,6 +528,7 @@ export const AUTH_ERROR_CODES_MAP_DO_NOT_USE_INTERNALLY = { | |||
| INVALID_EMAIL: 'auth/invalid-email', | |||
| INVALID_EMULATOR_SCHEME: 'auth/invalid-emulator-scheme', | |||
| INVALID_IDP_RESPONSE: 'auth/invalid-credential', | |||
| INVALID_LOGIN_CREDENTIALS: 'auth/invalid-credential', | |||
There was a problem hiding this comment.
Is there a reason not to create a new error type?
|
Is this not a breaking change? Should it have gone out in a minor release? Also the docs here state that this is an SDK error code. |
The rationale here was that the error code change is applicable when Email Enumeration Protection is enabled and the error code for "auth/invalid-login-credentials" wasn't publicly exposed previously. So it was less of a breaking change, and more of a "publicly exposing the error code for the first time" change. However, i do see that the string for the error code changed (from "auth/invalid-login-credentials" to "auth/invalid-credential" ), so apologies for this oversight. The docs you linked are the admin SDK error codes - https://firebase.google.com/docs/auth/admin/errors |
|
Thanks for the clarification on the documentation. My mistake. The error code change caught us out. We're automating updates and releasing daily so it wasn't until the Sentry alert fired that we noticed. No real harm done but the initial thought based on the error code was that somehow we were accidentally trying to call admin SDK code client side 😅. |
This PR has the following changes:
Expose INVALID_LOGIN_CREDENTIALS as auth/invalid-credential error.
Update the doc snippets for various SDK methods to explain the behavior when Email Enumeration Protection is enabled.
Mark fetchSignInMethodsForEmail and updateEmail as deprecated.
Fixes - #7661