Fix forced password change not redirecting on MVC pages#8101
Merged
Fix forced password change not redirecting on MVC pages#8101
Conversation
AuthMiddleware only returned a bool from validateUserSessionIsActive(), discarding the nextStepURL set when NeedPasswordChange is true. Legacy pages worked because ensureAuthentication() checked nextStepURL directly. Added ensureAuthentication() call for browser requests in AuthMiddleware to enforce the redirect on /v2 and /admin routes too. Also improved the forced-change UX: the page now uses the minimal login-style layout (no sidebar nav) with a clear warning heading, and the success screen shows a "Continue to Dashboard" button. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DawoudIO
requested review from
DAcodedBEAT,
MrClever,
bigtigerku,
grayeul and
respencer
and removed request for
a team
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes forced password-change enforcement for MVC pages (/v2/*, /admin/*) by ensuring authenticated browser sessions still honor “next step” redirects (e.g., password change), and updates the change-password UX to avoid showing full navigation during forced flows.
Changes:
- Enforce post-auth “next step” redirects for browser requests in
AuthMiddleware. - Pass
NeedPasswordChange(isForced) into the v2 change-password route/template rendering. - Conditionally switch change-password and success templates to a minimal login-style layout when forced.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/ChurchCRM/Slim/Middleware/AuthMiddleware.php | Adds enforcement of authentication “next step” redirects for browser MVC requests. |
| src/v2/routes/user-current.php | Passes isForced flag to templates based on NeedPasswordChange. |
| src/v2/templates/user/changepassword.php | Uses minimal “not logged in” layout when forced; keeps existing layout otherwise. |
| src/v2/templates/common/success-changepassword.php | Uses minimal layout when forced and adds a dashboard continuation CTA. |
- Remove value attributes from all password inputs (both layouts) to prevent passwords leaking into HTML/DOM on form re-display - Add $isForced ?? false default in success-changepassword.php to prevent undefined variable notices when $isForced is not passed - Personalize success message with $user->getFullName() so it is accurate when an admin changes another user's password - Replace ensureAuthentication() (header+exit) in AuthMiddleware with a PSR-15 302 Response using getAuthenticationProvider() - Exclude /background requests from nextStepURL check to avoid updating tLastOperation on background-only routes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- success-changepassword.php (non-forced branch): restore heading to
'Password Change Successful' so existing Cypress tests pass; the
user's full name is still shown in the body paragraph.
- AuthMiddleware::isPath(): explode('/','/.../path')[0] is always '' so
the background-path guard never fired. Switch to in_array() so the
check works correctly for any installation path depth.
- 01-setup-wizard.spec.js: fresh-install admin has NeedPasswordChange=true,
so after first login the new middleware now redirects to the forced
change-password page (minimal login-box layout). Extend the selector
to accept .login-box alongside the full dashboard layout elements.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After a fresh install the admin has NeedPasswordChange=true, so the new
AuthMiddleware redirects every browser request to /v2/user/current/changepassword.
The 01-setup-wizard spec was already patched, but 02-04 never completed
the forced change, causing every cy.visit('/admin/...') to bounce back
to the change-password page and leaving the DB in an unusable state for
all subsequent specs.
Fix:
- 02-demo-import / 03-backup-restore / 04-system-reset: rewrite the
loginAsAdmin()/manualLogin() helper to detect the /changepassword
redirect after login, fill the forced-change form (old=current,
new='Cypress@01!'), and store the new password in
Cypress.env('newSystemAdminPassword') so subsequent calls in the
same spec — and in later specs — pick it up lazily.
- 03-backup-restore: clear the env var before the SQL-restore test so
that Steps 8-9 (which run against the just-restored demo DB, where the
password reverts to 'changeme'/NeedPasswordChange=0) log in correctly.
- 04-system-reset: clear the env var before the API-reset request for the
same reason; relax the post-reset URL assertion to accept both the admin
dashboard and the change-password success page, either of which is a
valid outcome after the first post-reset login.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The login page is at /session/begin, not /login. The previous check
`cy.url().should('not.include', '/login')` was passing immediately even
when login FAILED (URL was /session/begin), causing the forced
password-change handler to be silently skipped. Subsequent cy.visit()
calls would hit AuthMiddleware and bounce back to /session/begin.
Changed to `should('not.include', '/session/begin')` in all four spec
files so a failed login correctly times out after 15-30 seconds instead
of silently proceeding as unauthenticated.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Problem
When a user had (first login after setup), the MVC (legacy) pages would not redirect to the password change form. The user could bypass the forced password change by directly accessing any MVC page, allowing them to continue with a password that must be changed.
Root Cause
The authentication check in
AuthMiddlewareonly calledensureAuthentication()(which checks and callsheader()redirects) when the user was not authenticated. For authenticated users with an active session, it did nothing, meaning any required redirect steps (like forced password change) were skipped.Solution
1. AuthMiddleware Enhancement (AuthMiddleware.php)
validateUserSessionIsActive(true)to check for required redirect stepsWhen a user had (first login after setup), the MVC (lenon
Root Cause
The authentication check in
AuthMiddlewareonly calledensureAuthentication()(which checks and callsheader()redirects) when the user was not authenticated. For authenticated users with an active session, it did nothing, meaning any required redirect sted