fix(core): clear notAllowedError so a valid user can log in after a rejected one - #946
Merged
Merged
Conversation
`useValidateAuthenticator` only ever set `notAllowedError` to `true` or to the thrown error, and never back to `false`. Since `canAccessMainView` is derived from `!notAllowedError`, a rejection was sticky for the whole page lifetime: once the `Authenticator` denied a user, any other user logging in afterwards was validated successfully but still got `canAccessMainView === false`, leaving them stuck on the login screen until a full page reload. The error is now reset when a new authentication attempt starts, right where the authenticator is about to re-run for a user whose uid has not been checked yet. That is guarded by `checkedUserRef`, so it runs at most once per user change and cannot loop, and it batches with the existing `setAuthLoading(true)` so no extra render or authenticator call is introduced. The reset is deliberately not placed at the top of `checkAuthentication`: `authController` is rebuilt on every render, so the effect re-runs constantly and an unconditional reset would wipe the error before the login view had a chance to display it. Adds regression tests covering the rejected-then-allowed flow, for both a denied and a throwing authenticator, while asserting that the error is still surfaced for the rejected user. Fixes #708 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes an authentication-state bug in @firecms/core where notAllowedError could persist across login attempts, preventing a subsequently valid user from accessing the main CMS view until a full reload.
Changes:
- Clear
notAllowedErrorat the start of a new authenticator validation attempt (scoped to the “new uid” guarded block). - Add a focused hook test suite to prevent regressions: accepted user, denied user error persistence on login screen, and error-clearing when switching to an allowed user.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/firecms_core/src/hooks/useValidateAuthenticator.tsx |
Resets notAllowedError when validating a newly logged-in user so prior rejections don’t block valid users. |
packages/firecms_core/test/useValidateAuthenticator.test.tsx |
Adds regression tests covering denial behavior and verifying the error is cleared on a subsequent successful login. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #708
The bug
Once
notAllowedErrorwas set inuseValidateAuthenticator, it was never cleared. It is only ever assignedtrueor a caught error, andcanAccessMainViewincludes!notAllowedError. So after one rejected login, a legitimate user logging in afterwards stayed stuck on the login screen until a full page reload.The fix
One
setNotAllowedError(false), placed inside thecheckedUserRef-guarded block before the authenticator runs.Why not at the top of
checkAuthentication(the obvious spot):useFirebaseAuthControllerreturns a fresh object literal every render (nouseMemo), soauthControlleris an unstableuseCallbackdep and the effect re-runs on every render. An unconditional reset there fires right aftersignOut()and wipes the error before the login view can display it — regressing the one behaviour the reporter said already worked. This was tested, not assumed: with the reset at the top, 3 of the 5 new tests fail.Placing it in the guarded block ties the error's lifetime to a validation attempt, runs at most once per uid change, and batches with the existing
setAuthLoading(true).Tests
New suite, 5 tests. Reverting only the source change makes exactly the two "clears the error" tests fail; the other 3 pass either way as guardrails.
@firecms/core: 242/245 passing (was 237/240) — 3 pre-existing failures unchanged.Follow-ups found, deliberately not included
authenticator instanceof Functionis realm-sensitive — ajest.fn()fails that check under jsdom, so a mocked authenticator is silently skipped.typeof authenticator === "function"would be robust.checkedUserRef.currentis assigned only after theawait, so a re-render during the async call re-enters the block. Pre-existing, not worsened, but it matters if an authenticator performs writes.Not verified
Unit-level only — not exercised in a browser against a real Firebase project.
🤖 Generated with Claude Code