fix: Fixed the forced password-change flow after first login - #4893
Open
Galane-dev wants to merge 5 commits into
Open
fix: Fixed the forced password-change flow after first login#4893Galane-dev wants to merge 5 commits into
Galane-dev wants to merge 5 commits into
Conversation
Redirected users with RequireChangePassword enabled back to change password form during login and protected-route auth checks That prevents them from bypassing the flow by editing the URL Also cleared RequireChangePassword after a successful token based password reset
Overrode the UserAppService class permission for ChangePasswordAsync to allow any authenticated user This ensures that authenticated users can complete the forced password change flow, even when they don't have user management rights required by the class
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shesha-reactjs/src/providers/auth/authenticator.ts (1)
231-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
loginUserAsyncdoesn't apply therequireChangePasswordguard — forced-password-change can be bypassed by editing the URL after fresh login.
loginUserAsyncunconditionally sets#loginInfoand state'ready', even whenuserProfile.user?.requireChangePasswordistrue.checkAuthAsyncshort-circuits at line 314 (if (this.loginInfo && !this.#isTokenExpired()) return) before reaching therequireChangePasswordcheck. The result:
- Login with
requireChangePassword=true→#loginInfois set, state ='ready', caller navigates to change-password URL.- User edits the URL to
/home(or any authenticated route).checkAuthAsyncfires →this.loginInfois truthy + token valid → early return, no redirect → user accesses the app without changing their password.This bypass only fails after a page refresh (where
#loginInfois cleared), so the protection is inconsistent. The PR's stated goal — "users cannot bypass the change-password screen by manually editing the URL" — is not fully achieved via the initial login path.
checkAuthAsyncalready handles this correctly (lines 341-345): it doesn't set#loginInfowhenrequireChangePasswordistrue.loginUserAsyncshould be consistent. The token expiration timer doesn't need to be restarted here since#saveUserTokenalready started it during#loginUserHttp.🐛 Proposed fix — align
loginUserAsyncwithcheckAuthAsyncconst userProfile = await this.#fetchUserInfoHttp(); - this.#loginInfo = userProfile; - - this.#updateState('ready', null, null); - - const redirectUrl = this.#getRedirectUrl(this.#router.fullPath, userProfile.user); - - return { - userProfile: userProfile, - url: redirectUrl ?? this.#router.fullPath - }; + if (userProfile.user?.requireChangePassword) { + this.#updateState('waiting', 'Password change required', null); + return { + userProfile: userProfile, + url: REQUIRED_PASSWORD_CHANGE_URL + }; + } + + this.#loginInfo = userProfile; + this.#updateState('ready', null, null); + + const redirectUrl = this.#getRedirectUrl(this.#router.fullPath, userProfile.user); + return { + userProfile: userProfile, + url: redirectUrl ?? this.#router.fullPath + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shesha-reactjs/src/providers/auth/authenticator.ts` around lines 231 - 235, In loginUserAsync: don't unconditionally set this.#loginInfo or call this.#updateState('ready', ...) when userProfile.user?.requireChangePassword is true; instead detect requireChangePassword on the userProfile returned from `#loginUserHttp`, save the token as needed (`#saveUserToken` already started the timer), then perform the redirect to the change-password flow (use `#getRedirectUrl` or the same redirect logic you use in checkAuthAsync) and return early so the guarded state isn't set; ensure the behavior matches checkAuthAsync's branch that avoids assigning this.#loginInfo when requireChangePassword is true.shesha-core/src/Shesha.NHibernate/NHibernate/SheshaNHibernateModule.cs (1)
214-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
IsEnabledForAnonymousUsersnot restored ifInitializeDbPermissionsthrows.
Configuration.EntityHistory.IsEnabledForAnonymousUsers = previs only reached on the happy path. IfInitializeDbPermissions()(orSeedDatabaseAsync) throws, the setting is left asfalse. Wrapping both calls in atry/finallyis the idiomatic fix.🛡️ Proposed fix — restore config in all cases
var prev = Configuration.EntityHistory.IsEnabledForAnonymousUsers; Configuration.EntityHistory.IsEnabledForAnonymousUsers = false; - AsyncHelper.RunSync(async () => { - await SeedDatabaseAsync(); - }); - IocManager.Resolve<ShaPermissionManager>().InitializeDbPermissions(); - Configuration.EntityHistory.IsEnabledForAnonymousUsers = prev; + try + { + AsyncHelper.RunSync(async () => { + await SeedDatabaseAsync(); + }); + IocManager.Resolve<ShaPermissionManager>().InitializeDbPermissions(); + } + finally + { + Configuration.EntityHistory.IsEnabledForAnonymousUsers = prev; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shesha-core/src/Shesha.NHibernate/NHibernate/SheshaNHibernateModule.cs` around lines 214 - 223, The code sets Configuration.EntityHistory.IsEnabledForAnonymousUsers to false but only restores it on the happy path; wrap the calls that run seeding and permission initialization (AsyncHelper.RunSync(() => SeedDatabaseAsync()) and IocManager.Resolve<ShaPermissionManager>().InitializeDbPermissions()) in a try/finally so that the original value held in prev is always restored (assign back to Configuration.EntityHistory.IsEnabledForAnonymousUsers in the finally block), while keeping the existing SkipDbSeed guard and prev capture.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shesha-reactjs/src/providers/auth/authenticator.ts`:
- Around line 341-345: When checkAuthAsync detects
userProfile.user.requireChangePassword is true, it currently updates state and
returns before calling `#startTokenExpirationTimer`; move or add a call to
this.#startTokenExpirationTimer(...) immediately before the
this.#updateState(...) / this.#router.push(REQUIRED_PASSWORD_CHANGE_URL) branch
so the token expiration watcher is started even when redirecting to the
change-password flow; ensure you pass the same token/session values used
elsewhere by `#startTokenExpirationTimer` and keep the early return after the
redirect.
---
Outside diff comments:
In `@shesha-core/src/Shesha.NHibernate/NHibernate/SheshaNHibernateModule.cs`:
- Around line 214-223: The code sets
Configuration.EntityHistory.IsEnabledForAnonymousUsers to false but only
restores it on the happy path; wrap the calls that run seeding and permission
initialization (AsyncHelper.RunSync(() => SeedDatabaseAsync()) and
IocManager.Resolve<ShaPermissionManager>().InitializeDbPermissions()) in a
try/finally so that the original value held in prev is always restored (assign
back to Configuration.EntityHistory.IsEnabledForAnonymousUsers in the finally
block), while keeping the existing SkipDbSeed guard and prev capture.
In `@shesha-reactjs/src/providers/auth/authenticator.ts`:
- Around line 231-235: In loginUserAsync: don't unconditionally set
this.#loginInfo or call this.#updateState('ready', ...) when
userProfile.user?.requireChangePassword is true; instead detect
requireChangePassword on the userProfile returned from `#loginUserHttp`, save the
token as needed (`#saveUserToken` already started the timer), then perform the
redirect to the change-password flow (use `#getRedirectUrl` or the same redirect
logic you use in checkAuthAsync) and return early so the guarded state isn't
set; ensure the behavior matches checkAuthAsync's branch that avoids assigning
this.#loginInfo when requireChangePassword is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d423fc4d-d3ff-4b6f-b60a-4ad48485e1ec
📒 Files selected for processing (4)
shesha-core/src/Shesha.Application/Users/UserAppService.csshesha-core/src/Shesha.Framework/Authorization/ShaPermissionManager.csshesha-core/src/Shesha.NHibernate/NHibernate/SheshaNHibernateModule.csshesha-reactjs/src/providers/auth/authenticator.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shesha-core/src/Shesha.NHibernate/NHibernate/SheshaNHibernateModule.cs`:
- Around line 362-367: The catch block currently captures the original
initialization exception as "e" but calls
appStartup.StartupFailedAsync(startupDto.Id, e) which may itself throw and
overwrite the original; change the catch to preserve "e" by wrapping the call to
appStartup.StartupFailedAsync in its own try/catch (or swallow/log any
exceptions from StartupFailedAsync) and always rethrow the original exception
"e" (use throw; to preserve stack) so that failures in StartupFailedAsync do not
replace the root-cause exception—target the catch(Exception e) block and the
call to appStartup.StartupFailedAsync(startupDto.Id, e).
- Line 223: The call to
IocManager.Resolve<ShaPermissionManager>().InitializeDbPermissions() must be
executed inside a distributed lock to avoid race conditions across instances;
wrap this call with the distributed lock facility (use
lockFactory.DoExclusiveAsync() with a sensible lock key) after
SeedDatabaseAsync() completes, or alternatively modify
ShaPermissionManager.InitializeDbPermissions to perform idempotent,
concurrency-safe upserts (or rely on DB unique constraints) so concurrent
executions cannot create duplicates. Ensure the lock is awaited and released
correctly and that the lock key is unique to permission initialization.
In `@shesha-reactjs/src/providers/auth/authenticator.ts`:
- Line 21: When redirecting to the forced-password-change URL
(REQUIRED_PASSWORD_CHANGE_URL), preserve and forward any existing returnUrl so
the user is returned to their original destination after changing password:
update every redirect that navigates to REQUIRED_PASSWORD_CHANGE_URL to read an
existing returnUrl from the current location's query (or from the login query
when coming from the login flow / from the current protected route when
restoring a session) and append it as a returnUrl query parameter to
REQUIRED_PASSWORD_CHANGE_URL before performing the redirect; ensure this change
is applied consistently in all places that reference
REQUIRED_PASSWORD_CHANGE_URL.
- Around line 272-280: The catch block on the fire-and-forget logout call
(this.#httpClient.post to URLS.LOGOFF using currentToken?.accessToken) currently
logs the full error object which can leak the Authorization header; change the
catch to only log sanitized fields (e.g., error?.response?.status,
error?.status, and error?.message or a short string) and avoid including the
entire error or request config; update the anonymous catch handler to extract
those safe fields and log a concise message like "Logout API call failed" with
only status/message details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83f9ace4-3dc7-4e40-9edb-3bcfa186ce58
⛔ Files ignored due to path filters (1)
shesha-functional-tests/backend/src/Boxfusion.SheshaFunctionalTests.Web.Host/appsettings.jsonis excluded by!shesha-functional-tests/**and included by none
📒 Files selected for processing (2)
shesha-core/src/Shesha.NHibernate/NHibernate/SheshaNHibernateModule.csshesha-reactjs/src/providers/auth/authenticator.ts
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
2 participants
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.
Enforces RequireChangePassword in the frontend authenticator so users cannot bypass the change-password screen by manually editing the URL.
Clears RequireChangePassword after successful token-based password reset.
Allows authenticated users to call ChangePasswordAsync without requiring the admin Users permission.
Details
Previously, users with RequireChangePassword = true received a valid access token during login and were redirected to the change-password form only by UI logic. If they removed /no-auth/Shesha/change-password?mode=edit from the URL, the app restored the valid token and allowed normal authenticated navigation.
The authenticator now checks requireChangePassword during login redirect and session restore. If the flag is still true, the user is redirected back to the change-password form and the app is not marked as ready.
The backend password reset token flow now clears RequireChangePassword after a successful password update, matching the normal change-password behavior.
ChangePasswordAsync now overrides the class-level Users permission with AnyAuthenticated, so normal logged-in users can change their own password. The method still uses the current session user, validates the existing password, applies password policy validation, and clears RequireChangePassword.
Summary by CodeRabbit