Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughExtracts "my account" UI into a dedicated MyAccount controller and form, centralizes AJAX/postback handler-name validation in CMS and Backend controllers, and adds centralized user-management authorization via User::canBeManagedByUser(), plus corresponding tests and translation key updates. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MyAccount as MyAccount Controller
participant FormBehavior as FormController Behavior
participant User as User Model
participant Auth as BackendAuth
Client->>MyAccount: POST index_onSave (form submit)
MyAccount->>FormBehavior: delegate update_onSave($userId, 'myaccount')
FormBehavior->>User: beforeSave() (authorization)
User->>User: canBeManagedByUser(actor)
alt Authorization Denied
User-->>FormBehavior: throw AuthorizationException
FormBehavior-->>Client: error response
else Authorization Allowed
User-->>FormBehavior: save succeeded
FormBehavior-->>MyAccount: return result
MyAccount->>MyAccount: detect login/password change
alt Credentials changed
MyAccount->>Auth: BackendAuth::login($user->reload(), true)
Auth-->>MyAccount: session refreshed
end
MyAccount-->>Client: success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
modules/backend/tests/controllers/MyAccountTest.php (1)
12-28: Add coverage for My Account save re-authentication behavior.Current tests validate permission wiring only. Consider adding a test that exercises
index_onSave()when login/password changes to lock in the security-critical re-login flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/tests/controllers/MyAccountTest.php` around lines 12 - 28, Add a new test that simulates changing the current user's login/password and asserts the controller enforces re-authentication: create a UserFixture and actingAs($user), instantiate MyAccount, populate request data with a different login and/or new password, call MyAccount->index_onSave(), then assert the post-save behavior enforces re-login by checking auth()->guest() (or that the user was logged out) and that the response/session/flash contains the re-authentication indicator (e.g., a 'relogin' flag or redirect to the login route); reference MyAccount::index_onSave(), UserFixture and auth() in the test to locate relevant code.modules/backend/tests/models/UserAuthorizationTest.php (1)
20-32: Re-guard the model in afinallyblock.
Model::unguard()flips global state for the whole suite. IfUser::create()throws insetUp(),Model::reguard()never runs and later tests can start passing or failing for the wrong reason.♻️ Suggested change
- Model::unguard(); - $this->targetUser = User::create([ - 'first_name' => 'Target', - 'last_name' => 'User', - 'login' => 'targetuser', - 'email' => 'target@test.com', - 'password' => 'TestPassword1', - 'password_confirmation' => 'TestPassword1', - 'is_activated' => true, - 'is_superuser' => false, - ]); - Model::reguard(); + Model::unguard(); + try { + $this->targetUser = User::create([ + 'first_name' => 'Target', + 'last_name' => 'User', + 'login' => 'targetuser', + 'email' => 'target@test.com', + 'password' => 'TestPassword1', + 'password_confirmation' => 'TestPassword1', + 'is_activated' => true, + 'is_superuser' => false, + ]); + } finally { + Model::reguard(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/tests/models/UserAuthorizationTest.php` around lines 20 - 32, In setUp(), Model::unguard() is currently called without ensuring Model::reguard() always runs; wrap the unguard/create/related code in a try/finally so that Model::reguard() is executed in the finally block even if User::create() (or any subsequent setup code) throws—keep the existing calls to Model::unguard(), User::create(), and Model::reguard() but move the reguard into the finally section to guarantee global state is restored after setUp().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/backend/classes/Controller.php`:
- Around line 489-494: validateHandlerName currently assumes $handler is a
string and calls preg_match, which will raise a TypeError for arrays; update
validateHandlerName to explicitly check the type before calling preg_match (e.g.
if (!is_string($handler)) throw new SystemException(...)), then perform the
existing preg_match check and throw the same SystemException on failure;
reference validateHandlerName, preg_match, and SystemException (and keep the
Lang::get('backend::lang.ajax_handler.invalid_name', ['name' => $handler])
message) so array or non-string payloads (such as from post('_handler')) are
converted into the expected graceful SystemException.
In `@modules/backend/controllers/MyAccount.php`:
- Around line 73-74: $post('User[password]') can be non-string and calling
strlen(...) will throw a TypeError and the current boolean only checks non-empty
rather than an actual change; update the $passwordChanged logic to first ensure
the posted value is a string (use is_string on post('User[password]')), check
it's non-empty, and then compare it to the existing value appropriately (e.g.
compare raw values or verify against the stored hash via the user password
field) so $passwordChanged becomes true only when a valid string password was
supplied and it actually differs from $this->user->password.
In `@modules/backend/controllers/Users.php`:
- Around line 122-124: The impersonation redirect in
Users::update_onImpersonateUser() still points to "backend/users/myaccount" but
the self-edit route was changed to "backend/myaccount" (see the self-edit check
using $context != 'myaccount' and $recordId == $this->user->id); update the
redirect target in update_onImpersonateUser() to "backend/myaccount" so
impersonated regular users are sent to the new My Account URL and avoid the
backend.manage_users permission check.
In `@modules/backend/models/User.php`:
- Around line 131-155: canBeManagedByUser currently treats
BackendAuth::getUser() being null as fully authorized; change it to only allow
null/anonymous actor when running in trusted console/queue contexts. In the
canBeManagedByUser(?User $user = null) method, after resolving $user = $user ??
BackendAuth::getUser(), if $user is null return true only when the process is a
CLI/queue worker (e.g. App::runningInConsole() or other project-specific
queue/runtime check) and otherwise return false; preserve the existing
backend.manage_users and isSuperUser checks for real users; update the docblock
to reflect the new behavior.
In `@modules/backend/tests/classes/ControllerPostbackTest.php`:
- Around line 99-115: The test testAjaxPathAcceptsValidHandlerName currently
only uses a syntactically valid name ('onSave') and catches any SystemException,
so it doesn't prove a real handler is accepted; update the test to either target
an actual AJAX handler that exists on the Auth controller (replace
configAjaxRequestMock('onSave') with a real handler name implemented on Auth,
e.g., configAjaxRequestMock('onSignin') or add a simple public onXxx handler to
Auth) so run('signin') proceeds to the handler, or tighten the assertion to
expect the specific fallback exception message you know will be thrown when the
handler is missing (assertEquals or assertStringContainsString against 'Handler
not found') to prove name validation passed rather than swallowing all
SystemException types. Ensure you modify the test method
testAjaxPathAcceptsValidHandlerName and adjust the Request::swap call and
assertions accordingly.
In `@modules/cms/classes/Controller.php`:
- Around line 694-698: The validateHandlerName method currently has a strict
string signature and will raise a TypeError for non-string _handler inputs;
update validateHandlerName (and callers that pass post('_handler')) to first
check that the incoming value is a string (e.g., is_string($handler)) and if not
throw the same SystemException with
Lang::get('cms::lang.ajax_handler.invalid_name', ['name' => $handler]) and HTTP
400; keep the existing regex validation for string values so only actual
non-string inputs are converted into the intended 400 invalid-handler response
instead of a 500 TypeError.
In `@modules/cms/tests/classes/ControllerPostbackTest.php`:
- Around line 116-131: The test testAjaxPathAcceptsValidHandlerName currently
swallows all SystemException from Controller::run, which masks unrelated
failures; change it to let unexpected exceptions fail or explicitly assert the
handler's observable effect instead of blanket-catching. Specifically, remove
the try/catch around $controller->run('/ajax-test') (or if you must catch,
rethrow unless strpos($e->getMessage(), 'Invalid AJAX handler name') !== false)
and replace the final assertTrue(true) with an assertion that verifies the
onTest handler ran (e.g., expected response, header, or state change produced by
the ajax-test.htm onTest handler). Ensure you reference
testAjaxPathAcceptsValidHandlerName and Controller::run when updating the test.
---
Nitpick comments:
In `@modules/backend/tests/controllers/MyAccountTest.php`:
- Around line 12-28: Add a new test that simulates changing the current user's
login/password and asserts the controller enforces re-authentication: create a
UserFixture and actingAs($user), instantiate MyAccount, populate request data
with a different login and/or new password, call MyAccount->index_onSave(), then
assert the post-save behavior enforces re-login by checking auth()->guest() (or
that the user was logged out) and that the response/session/flash contains the
re-authentication indicator (e.g., a 'relogin' flag or redirect to the login
route); reference MyAccount::index_onSave(), UserFixture and auth() in the test
to locate relevant code.
In `@modules/backend/tests/models/UserAuthorizationTest.php`:
- Around line 20-32: In setUp(), Model::unguard() is currently called without
ensuring Model::reguard() always runs; wrap the unguard/create/related code in a
try/finally so that Model::reguard() is executed in the finally block even if
User::create() (or any subsequent setup code) throws—keep the existing calls to
Model::unguard(), User::create(), and Model::reguard() but move the reguard into
the finally section to guarantee global state is restored after setUp().
🪄 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: CHILL
Plan: Pro
Run ID: 987edcfe-aaa5-47e9-bcbb-1548db08ec9c
📒 Files selected for processing (13)
modules/backend/ServiceProvider.phpmodules/backend/classes/Controller.phpmodules/backend/controllers/MyAccount.phpmodules/backend/controllers/Users.phpmodules/backend/controllers/myaccount/config_form.yamlmodules/backend/controllers/myaccount/index.phpmodules/backend/lang/en/lang.phpmodules/backend/models/User.phpmodules/backend/tests/classes/ControllerPostbackTest.phpmodules/backend/tests/controllers/MyAccountTest.phpmodules/backend/tests/models/UserAuthorizationTest.phpmodules/cms/classes/Controller.phpmodules/cms/tests/classes/ControllerPostbackTest.php
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests