Repository navigation
Conversation
Covers the areas listed in wintercms#271 against the real backend stack (BackendAuth, Backend\Models\User, UserThrottle): - login with valid credentials, and rejection of a wrong password / unknown login - failed attempts are recorded, and the account is suspended once the configured attempt limit is reached; a suspended account is rejected even with valid credentials - a successful login clears the recorded attempts - banned and soft deleted accounts cannot authenticate - logout clears the auth session and regenerates the session id - activation is enforced when the manager requires it, and the backend manager's own policy (activation not required) is pinned down - impersonation switches the session user, filters the impersonatee's permissions to those the impersonator also holds, and restores the impersonator afterwards; super users and users without `backend.impersonate_users` cannot be impersonated - role permissions are inherited and wildcard permissions match as documented, and super users bypass permission checks - registration creates an activated user and logs them in Two behaviours worth noting, both documented in the tests themselves: - `Manager::guest()` is hard-coded to return false, so "not logged in" is asserted through `check()`. - `impersonate()` re-loads the impersonator from the session, so the `$impersonator === $this` self-impersonation guard in `canBeImpersonated()` cannot match in the session flow. Not covered here; worth a separate look. Refs wintercms#271
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 38.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "Add coverage for the authentication system" matches the main change. The PR adds UserAuthenticationTest, a PluginTestCase suite that covers login, throttling, banning, logout, activation, i… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "Add coverage for the authentication system" matches the main change. The PR adds UserAuthenticationTest, a PluginTestCase suite that covers login, throttling, banning, logout, activation, impersonation, permissions, and registration. The title is somewhat broad, since it does not name the backend or the test file, but it names the real primary change and is not vague or misleading.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 38.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 18 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Summary
Adds test coverage for the authentication system, as listed in #271. The tests exercise the real backend stack (
BackendAuth,Backend\Models\User,Backend\Models\UserThrottle) throughPluginTestCase, so they run against the migrated schema and the actual manager configuration.Coverage
last_login; wrong passwords and unknown logins are rejectedauth.throttle.attemptLimitattempts; a suspended account is rejected even with valid credentials; a successful login clears the attemptsStrictActivationAuthManagerfixture); the backend manager's own policy ($requireActivation = false, so administrators can log in while inactive) is pinned downstopImpersonate(), and refuses super users / impersonators withoutbackend.impersonate_usersTwo things I found while writing these
Winter\Storm\Auth\Manager::guest()is hard-coded toreturn false;(aStatefulGuardmethod that never reports a guest). The tests therefore assert "not logged in" throughcheck(). Not changing it here, but it looks unintended.impersonate()re-loads the impersonator from the session (getPersistCodeFromSession()→findUserById()), so the$impersonator === $thisguard inBackend\Models\User::canBeImpersonated()can never match in the session flow — self-impersonation is therefore not blocked by that check. I dropped the test for it rather than assert the current behaviour; worth a separate issue/PR if you agree it should be blocked.Out of scope
register().getMergedPermissions()in the Storm base model (only role + individual permissions), so inheritance is covered for roles.Verification
php vendor/bin/phpunit modules/backend/tests/models/UserAuthenticationTest.php— 17 tests, 61 assertions, green.Tests: 1124, Assertions: 3749, Skipped: 29— green (previous baseline 1107 tests).phpcson the new file: clean.Refs #271
Summary by CodeRabbit