Skip to content

Align recaptcha error codes with android #7064

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

Align recaptcha error codes with android #7064

wants to merge 9 commits into from

Conversation

renkelvin
Copy link

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Feb 28, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 99662da

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@renkelvin renkelvin changed the base branch from recaptcha to master February 28, 2023 19:11
@renkelvin renkelvin changed the base branch from master to recaptcha February 28, 2023 19:11
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Feb 28, 2023 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/auth

    TypeBase (f7cac56)Merge (241e4c8)Diff
    browser164 kB165 kB+1.14 kB (+0.7%)
    cordova195 kB196 kB+1.21 kB (+0.6%)
    esm5215 kB216 kB+1.21 kB (+0.6%)
    main159 kB161 kB+1.21 kB (+0.8%)
    module164 kB165 kB+1.14 kB (+0.7%)
    react-native179 kB180 kB+1.21 kB (+0.7%)

  • @firebase/auth/cordova

    TypeBase (f7cac56)Merge (241e4c8)Diff
    browser195 kB196 kB+1.21 kB (+0.6%)
    module195 kB196 kB+1.21 kB (+0.6%)

  • @firebase/auth/internal

    TypeBase (f7cac56)Merge (241e4c8)Diff
    browser175 kB176 kB+1.14 kB (+0.7%)
    esm5229 kB230 kB+1.21 kB (+0.5%)
    main195 kB196 kB+1.21 kB (+0.6%)
    module175 kB176 kB+1.14 kB (+0.7%)

  • @firebase/auth/react-native

    TypeBase (f7cac56)Merge (241e4c8)Diff
    browser179 kB180 kB+1.21 kB (+0.7%)
    module179 kB180 kB+1.21 kB (+0.7%)

  • bundle

    TypeBase (f7cac56)Merge (241e4c8)Diff
    auth (Anonymous)70.3 kB70.5 kB+148 B (+0.2%)
    auth (EmailAndPassword)75.3 kB75.5 kB+177 B (+0.2%)
    auth (GoogleFBTwitterGitHubPopup)96.4 kB96.5 kB+148 B (+0.2%)
    auth (GooglePopup)93.7 kB93.8 kB+148 B (+0.2%)
    auth (GoogleRedirect)93.9 kB94.0 kB+148 B (+0.2%)
    auth (Phone)80.2 kB80.4 kB+148 B (+0.2%)

  • firebase

    TypeBase (f7cac56)Merge (241e4c8)Diff
    firebase-auth-compat.js131 kB131 kB+877 B (+0.7%)
    firebase-auth-cordova.js145 kB146 kB+1.23 kB (+0.8%)
    firebase-auth-react-native.js157 kB158 kB+1.23 kB (+0.8%)
    firebase-auth.js122 kB124 kB+1.19 kB (+1.0%)
    firebase-compat.js752 kB753 kB+877 B (+0.1%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/V4IMiAZzXA.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Feb 28, 2023 •
edited
Loading

Size Analysis Report 1

This report is too large (133,893 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/0Gp3ocXGEU.html

prameshj
prameshj reviewed Feb 28, 2023
View reviewed changes
prameshj
prameshj reviewed Feb 28, 2023
View reviewed changes
packages/auth/src/api/errors.ts Outdated
@@ -216,7 +216,7 @@ export const SERVER_ERROR_MAP: Partial<ServerErrorMap<ServerError>> = {
[ServerError.BLOCKING_FUNCTION_ERROR_RESPONSE]: AuthErrorCode.INTERNAL_ERROR,

// Recaptcha related errors.
[ServerError.INVALID_RECAPTCHA_SCORE]: AuthErrorCode.RECAPTCHA_CHECK_FAILED,
[ServerError.INVALID_RECAPTCHA_SCORE]: AuthErrorCode.CAPTCHA_CHECK_FAILED,
[ServerError.RECAPTCHA_NOT_ENABLED]: AuthErrorCode.RECAPTCHA_NOT_ENABLED,
[ServerError.MISSING_RECAPTCHA_TOKEN]: AuthErrorCode.INTERNAL_ERROR,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we expose these errors too, instead of INTERNAL_ERROR?

Also, there is no INVALID_RECAPTCHA_ENFORCEMENT_STATE anymore/

Copy link
Contributor

Choose a reason for hiding this comment

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

Reopening to check if we can expose these error codes directly instead of INTERNAL_ERROR.
[ServerError.INVALID_RECAPTCHA_VERSION]: AuthErrorCode.INVALID_RECAPTCHA_VERSION,

Copy link
Author

@renkelvin renkelvin Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You mean the recaptcha errors right? Yeah, they're exposed. See the auth.api.md

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, why are we mapping - [ServerError.MISSING_RECAPTCHA_TOKEN]: AuthErrorCode.INTERNAL_ERROR, to INTERNAL_ERROR instead of AuthErrorCode.MISSING_RECAPTCHA_TOKEN

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I completely missed the right part. Just updated. Thanks for pointing this out.

prameshj
prameshj reviewed Mar 1, 2023
View reviewed changes
packages/auth/src/api/errors.ts Outdated
@@ -216,7 +216,7 @@ export const SERVER_ERROR_MAP: Partial<ServerErrorMap<ServerError>> = {
[ServerError.BLOCKING_FUNCTION_ERROR_RESPONSE]: AuthErrorCode.INTERNAL_ERROR,

// Recaptcha related errors.
[ServerError.INVALID_RECAPTCHA_SCORE]: AuthErrorCode.RECAPTCHA_CHECK_FAILED,
[ServerError.INVALID_RECAPTCHA_SCORE]: AuthErrorCode.CAPTCHA_CHECK_FAILED,
[ServerError.RECAPTCHA_NOT_ENABLED]: AuthErrorCode.RECAPTCHA_NOT_ENABLED,
[ServerError.MISSING_RECAPTCHA_TOKEN]: AuthErrorCode.INTERNAL_ERROR,
Copy link
Contributor

Choose a reason for hiding this comment

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

Reopening to check if we can expose these error codes directly instead of INTERNAL_ERROR.
[ServerError.INVALID_RECAPTCHA_VERSION]: AuthErrorCode.INVALID_RECAPTCHA_VERSION,

@renkelvin renkelvin deleted the branch recaptcha-dev March 7, 2023 19:30
@renkelvin renkelvin closed this Mar 7, 2023
@renkelvin renkelvin reopened this Mar 7, 2023
@renkelvin renkelvin changed the base branch from recaptcha to recaptcha-dev March 7, 2023 21:22
@renkelvin renkelvin changed the base branch from recaptcha-dev to recaptcha March 7, 2023 21:23
@renkelvin renkelvin changed the base branch from recaptcha to recaptcha-dev March 7, 2023 21:32
@renkelvin renkelvin requested a review from markarndt as a code owner March 7, 2023 21:58
@renkelvin renkelvin changed the base branch from recaptcha-dev to recaptcha-public-preview March 21, 2023 22:17
@renkelvin renkelvin changed the base branch from recaptcha-public-preview to recaptcha-dev March 21, 2023 22:17
renkelvin pushed a commit that referenced this pull request Mar 21, 2023
Apply diff of #7064
5f3d484
@renkelvin renkelvin closed this Mar 24, 2023
@renkelvin renkelvin deleted the error-codes branch March 24, 2023 20:12
renkelvin pushed a commit that referenced this pull request Apr 3, 2023
Apply diff of #7064
f01bd53
@firebase firebase locked and limited conversation to collaborators Apr 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@prameshj prameshj prameshj left review comments

@lisajian lisajian Awaiting requested review from lisajian

@sam-gc sam-gc Awaiting requested review from sam-gc

@dwyfrequency dwyfrequency Awaiting requested review from dwyfrequency

@hsubox76 hsubox76 Awaiting requested review from hsubox76

@egilmorez egilmorez Awaiting requested review from egilmorez

@maneesht maneesht Awaiting requested review from maneesht

@jsdt jsdt Awaiting requested review from jsdt

@jmwski jmwski Awaiting requested review from jmwski

@markarndt markarndt Awaiting requested review from markarndt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@renkelvin @google-oss-bot @prameshj