Skip to content

ENG-3370: Fix incorrect error message on login with bad credentials#7882

Merged
gilluminate merged 3 commits intomainfrom
gill/ENG-3370/fix-login-error-message
Apr 9, 2026
Merged

ENG-3370: Fix incorrect error message on login with bad credentials#7882
gilluminate merged 3 commits intomainfrom
gill/ENG-3370/fix-login-error-message

Conversation

@gilluminate
Copy link
Copy Markdown
Contributor

Ticket ENG-3370

Description Of Changes

On SSO-configured environments (like RC), entering incorrect credentials showed "Username and password login is disabled when SSO is configured. Please use SSO to login." instead of the expected generic login error.

Root cause: PR #6904 introduced getErrorMessage() to the login catch block, which extracts error.data.detail from backend responses. The fidesplus user_login_extended endpoint runs an SSO check before credential validation, and when credentials are wrong, the SSO guard can fire and return its error message. This backend detail was being surfaced to users instead of the generic login failure message.

Fix: Only use getErrorMessage() for the invite and reset-password flows where backend error details are meaningful to the user. The standard login flow always shows the generic "Login failed. Please check your credentials and try again." message, matching the original behavior and avoiding leaking backend configuration details.

Code Changes

  • clients/admin-ui/src/pages/login.tsx - Modified the handleSubmit catch block to use getErrorMessage() only for invite/reset-password flows, not for standard login

Steps to Confirm

  1. On an SSO-configured environment, enter incorrect credentials
  2. Verify the error message is "Login failed. Please check your credentials and try again." (not the SSO message)
  3. Enter correct credentials and verify login succeeds normally
  4. Test invite flow with bad password - should still show backend error detail
  5. Test reset password flow with expired token - should still show backend error detail

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • No migrations
  • Documentation:
    • No documentation updates required

gilluminate and others added 2 commits April 9, 2026 14:19
The login error handler was surfacing raw backend error details (added
in PR #6904), which on SSO-configured environments showed "Username and
password login is disabled when SSO is configured" for any credential
failure. Now only uses getErrorMessage for invite/reset-password flows
where backend details are meaningful; standard login always shows the
generic fallback.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Apr 9, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
fides-plus-nightly Ready Ready Preview, Comment Apr 9, 2026 8:33pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
fides-privacy-center Ignored Ignored Apr 9, 2026 8:33pm

Request Review

@gilluminate gilluminate marked this pull request as ready for review April 9, 2026 20:21
@gilluminate gilluminate requested a review from a team as a code owner April 9, 2026 20:21
@gilluminate gilluminate requested review from kruulik and removed request for a team April 9, 2026 20:21
Copy link
Copy Markdown
Contributor

@kruulik kruulik left a comment

Choose a reason for hiding this comment

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

Makes sense!

@gilluminate gilluminate enabled auto-merge April 9, 2026 20:24
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: Fix incorrect error message on login with bad credentials

The fix is correct and minimal. The logic inversion — from calling getErrorMessage() on all three branches with a fallback, to only calling it where backend error detail is actually useful — directly addresses the root cause without over-engineering. The security implication (preventing backend SSO config state from leaking through login error messages) is handled properly.

Suggestions

console.log(error) at line 172 (inline comment): This pre-existing line logs the raw RTK error object to the browser console, including status codes, response body, and potentially SSO configuration detail — exactly the class of information this PR is trying to prevent from leaking. Should be console.error at minimum, or removed for a production login page.

Comment asymmetry between branches (inline comment): The else branch gets a clear comment explaining why it avoids getErrorMessage(). The invite/reset-password branches have no corresponding comment explaining why they intentionally do call it. Adding a brief comment there would make the three-way asymmetry self-documenting and reduce the chance of a future contributor copying the wrong pattern.

Missing test coverage: There are no tests for login.tsx. The original regression (calling getErrorMessage() unconditionally) is exactly the kind of error that branch-level unit tests would catch. The three paths are now clearly defined, making them straightforward to test with mocked RTK mutations:

  • Standard login failure → always returns the hardcoded string, regardless of error.data.detail
  • Invite failure → passes error through getErrorMessage()
  • Reset-password failure → passes error through getErrorMessage()

Positive notes

  • The rename from defaultErrorMsg to errorMsg is correct — the old name was misleading since the variable held the final message, not a fallback.
  • The inline comment on the else branch is good documentation of intent.
  • The changelog entry is accurate and well-scoped.

🔬 Codegraph: connected (42425 nodes)


💡 Write /code-review in a comment to re-run this review.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

Title Lines Statements Branches Functions
admin-ui Coverage: 7%
5.88% (2547/43304) 4.89% (1199/24515) 3.95% (508/12829)
fides-js Coverage: 78%
78.98% (1962/2484) 65.55% (1214/1852) 72.57% (336/463)
privacy-center Coverage: 88%
85.93% (330/384) 81.1% (176/217) 78.87% (56/71)

@gilluminate gilluminate added this pull request to the merge queue Apr 9, 2026
Merged via the queue into main with commit 0f7d498 Apr 9, 2026
51 checks passed
@gilluminate gilluminate deleted the gill/ENG-3370/fix-login-error-message branch April 9, 2026 20:59
gilluminate added a commit that referenced this pull request Apr 9, 2026
…7882)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.

2 participants