Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntegrates Laravel Fortify across auth flows: replaces legacy controllers/traits with Fortify controllers/pipelines and form requests, adds Fortify provider/config, introduces a Login event listener and MissingAttributeException render, updates routes/rate-limiters, adjusts frontend throttle handling, tests, and composer deps. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2902 +/- ##
==========================================
Coverage 96.73% 96.74%
- Complexity 1924 1929 +5
==========================================
Files 457 461 +4
Lines 12988 13008 +20
Branches 2079 2081 +2
==========================================
+ Hits 12564 12584 +20
Misses 424 424 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PILOS
|
||||||||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
refactor-auth
|
| Run status |
|
| Run duration | 07m 09s |
| Commit |
|
| Committer | Samuel Weirich |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
621
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
e2e/RoomsJoinWithLobby.cy.js • 1 failed test • System tests
| Test | Artifacts | |
|---|---|---|
| Room Join with lobby settings > Lobby enabled for guests only |
Test Replay
Screenshots
|
|
samuelwei
force-pushed
the
refactor-auth
branch
2 times, most recently
from
1a67e4b to
a403205
Compare
samuelwei
force-pushed
the
refactor-auth
branch
3 times, most recently
from
c1761b5 to
6484373
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app/Auth/MissingAttributeException.php (1)
16-19: Return an explicit JSON response fromrender()instead of callingabort().In Laravel's exception rendering layer,
abort()should not be called. Therender()method is responsible for converting the exception into an HTTP response, and it should return that response directly rather than throwing another exception. This keeps exception rendering deterministic and avoids unnecessary exception nesting.♻️ Proposed refactor
public function render(Request $request): JsonResponse { - abort(500, __('auth.error.missing_attributes')); + return response()->json([ + 'message' => __('auth.error.missing_attributes'), + ], 500); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Auth/MissingAttributeException.php` around lines 16 - 19, The render method in MissingAttributeException should return an explicit JSON HTTP response instead of calling abort(); update MissingAttributeException::render(Request $request): JsonResponse to construct and return a JsonResponse (or use response()->json(..., 500)) containing the translated error message __('auth.error.missing_attributes') and an appropriate HTTP 500 status, so the method returns the response directly rather than raising another exception.resources/js/views/Login.vue (1)
178-185: Add a fallback whenRetry-Afteris missing.If the header is absent, Line 181/Line 184 may render a message with undefined seconds.
Proposed hardening
- const retryAfter = error.response.headers["retry-after"]; - if (data.username) { - errors[id] = { - username: [t("auth.throttle", { seconds: retryAfter })], - }; - } else { - errors[id] = { email: [t("auth.throttle", { seconds: retryAfter })] }; - } + const retryAfter = error.response.headers?.["retry-after"] ?? "60"; + const field = data.username ? "username" : "email"; + errors[id] = { + [field]: [t("auth.throttle", { seconds: retryAfter })], + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/views/Login.vue` around lines 178 - 185, The code reads retryAfter from error.response.headers and passes it directly to t("auth.throttle", { seconds: retryAfter }), which can be undefined; add a safe fallback (e.g., defaultSeconds = 60 or similar) when reading retryAfter so the translation always receives a number/string. Update the retrieval of retryAfter and the calls that set errors[id] (the branches using data.username and the email branch) to use the fallback value rather than raw retryAfter.app/Auth/LDAP/LDAPController.php (1)
14-20: Consider moving Fortify config scoping from constructor to login method.While your project currently lacks long-lived worker frameworks (Octane, Swoole), setting
fortify.guardandfortify.usernamein the constructor mutates shared config state. This pattern can cause issues if workers are introduced later. As a defensive measure, scope the config changes to thelogin()method and restore previous values in afinallyblock to ensure isolation per request.♻️ Safer scoping pattern
public function __construct(StatefulGuard $guard) { - config(['fortify.guard' => 'ldap']); - config(['fortify.username' => 'username']); - parent::__construct($guard); } public function login(LoginRequest $request) { + $previousGuard = config('fortify.guard'); + $previousUsername = config('fortify.username'); + config(['fortify.guard' => 'ldap', 'fortify.username' => 'username']); + + try { - $response = $this->loginPipeline($request)->then(function ($request) { - return app(LoginResponse::class); - }); + $response = $this->loginPipeline($request)->then(function ($request) { + return app(LoginResponse::class); + }); + } finally { + config(['fortify.guard' => $previousGuard, 'fortify.username' => $previousUsername]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Auth/LDAP/LDAPController.php` around lines 14 - 20, The Fortify config mutations in LDAPController->__construct (config(['fortify.guard'=>'ldap'], config(['fortify.username'=>'username']))) should be moved into the login() method: capture the current values of 'fortify.guard' and 'fortify.username' at the start of LDAPController::login(), set them to 'ldap' and 'username' for the duration of the login attempt, and then restore the original values inside a finally block so config changes are request-scoped; keep the StatefulGuard injection and parent::__construct($guard) in the constructor but remove the direct config() calls from __construct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Listeners/SuccessfulLogin.php`:
- Around line 27-38: Wrap all non-essential side-effects in the SuccessfulLogin
listener (the block modifying $event->user->last_login and calling
$event->user->save(), the Counter::get(...)->inc(...) calls, and the
Log::info(...) calls) in a try/catch so any exception from DB, metrics, or
logging is caught and does not bubble up to the login flow; on catch, emit a
safe warning-level Log::warning with contextual info (include
$event->user->getLogLabel() and the exception message) and return (or continue)
so authentication always succeeds even if side-effects fail.
In `@app/Providers/RouteServiceProvider.php`:
- Around line 54-57: Rate limiter key currently uses
$request->input(Fortify::username()) which may be null for LDAP logins
(Fortify::username() is 'email'), causing throttling to collapse to IP-only;
update the RateLimiter::for('login') throttle key logic to prefer the Fortify
username but fall back to the LDAP input (e.g. $request->input('username')) and
coerce to string before lowering so the key becomes
Str::transliterate(Str::lower((string) $username).'|'.$request->ip()); adjust
the variable referenced ($throttleKey) in that closure to use this fallback and
string casting to restore per-user throttling.
In `@composer.json`:
- Line 19: In composer.json replace the unbounded "laravel/fortify": "*" entry
with a bounded semver constraint (for example a tested range like "^1.0" or a
specific patch series such as "^1.14") to prevent accidental breaking upgrades;
after changing the "laravel/fortify" constraint, run composer update to
regenerate composer.lock and run the authentication-related tests (and any
Fortify integration code) to confirm nothing broke.
In `@routes/api.php`:
- Around line 48-49: Both login routes need the guest middleware scoped to both
guards so authenticated users in either guard are prevented from hitting the
login endpoints: update the Route definitions for login.local
(LoginController::login) and login.ldap (LDAPController::login) to replace
unscoped 'guest' and add the guest middleware parameter 'guest:users,ldap'
(apply the same scoped guest to both routes) while keeping the other middlewares
(enable_if_config and throttle) unchanged.
---
Nitpick comments:
In `@app/Auth/LDAP/LDAPController.php`:
- Around line 14-20: The Fortify config mutations in LDAPController->__construct
(config(['fortify.guard'=>'ldap'], config(['fortify.username'=>'username'])))
should be moved into the login() method: capture the current values of
'fortify.guard' and 'fortify.username' at the start of LDAPController::login(),
set them to 'ldap' and 'username' for the duration of the login attempt, and
then restore the original values inside a finally block so config changes are
request-scoped; keep the StatefulGuard injection and parent::__construct($guard)
in the constructor but remove the direct config() calls from __construct.
In `@app/Auth/MissingAttributeException.php`:
- Around line 16-19: The render method in MissingAttributeException should
return an explicit JSON HTTP response instead of calling abort(); update
MissingAttributeException::render(Request $request): JsonResponse to construct
and return a JsonResponse (or use response()->json(..., 500)) containing the
translated error message __('auth.error.missing_attributes') and an appropriate
HTTP 500 status, so the method returns the response directly rather than raising
another exception.
In `@resources/js/views/Login.vue`:
- Around line 178-185: The code reads retryAfter from error.response.headers and
passes it directly to t("auth.throttle", { seconds: retryAfter }), which can be
undefined; add a safe fallback (e.g., defaultSeconds = 60 or similar) when
reading retryAfter so the translation always receives a number/string. Update
the retrieval of retryAfter and the calls that set errors[id] (the branches
using data.username and the email branch) to use the fallback value rather than
raw retryAfter.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
app/Auth/ExternalUser.phpapp/Auth/LDAP/LDAPController.phpapp/Auth/MissingAttributeException.phpapp/Http/Controllers/api/v1/auth/ForgotPasswordController.phpapp/Http/Controllers/api/v1/auth/LoginController.phpapp/Http/Controllers/api/v1/auth/ResetPasswordController.phpapp/Http/Requests/ForgotPasswordRequest.phpapp/Http/Requests/ResetPasswordRequest.phpapp/Listeners/SuccessfulLogin.phpapp/Providers/EventServiceProvider.phpapp/Providers/FortifyServiceProvider.phpapp/Providers/RouteServiceProvider.phpcomposer.jsonconfig/app.phpconfig/fortify.phpdatabase/migrations/2026_03_02_200000_add_two_factor_columns_to_users_table.phpresources/js/views/Login.vueroutes/api.phptests/Backend/Feature/api/v1/LoginTest.phptests/Backend/Unit/MeetingTest.phptests/Frontend/e2e/Login.cy.js
| $event->user->last_login = now(); | ||
| $event->user->save(); | ||
|
|
||
| // Log successful authentication | ||
| if ($guard == 'users') { | ||
| Counter::get('login_total')->inc('local'); | ||
| Log::info('Local user {user} has been successfully authenticated.', ['user' => $event->user->getLogLabel()]); | ||
| } | ||
| if ($guard == 'ldap') { | ||
| Counter::get('login_total')->inc('ldap'); | ||
| Log::info('External user {user} has been successfully authenticated.', ['user' => $event->user->getLogLabel(), 'type' => 'ldap']); | ||
| } |
There was a problem hiding this comment.
Do not let listener side-effects fail the login path.
If save()/metrics/logging throws (DB/metrics backend issues), login can fail despite successful authentication. Wrap listener side-effects defensively.
🛡️ Proposed hardening
public function handle(Login $event): void
{
- $guard = $event->guard;
-
- // Update the last login timestamp
- $event->user->last_login = now();
- $event->user->save();
-
- // Log successful authentication
- if ($guard == 'users') {
- Counter::get('login_total')->inc('local');
- Log::info('Local user {user} has been successfully authenticated.', ['user' => $event->user->getLogLabel()]);
- }
- if ($guard == 'ldap') {
- Counter::get('login_total')->inc('ldap');
- Log::info('External user {user} has been successfully authenticated.', ['user' => $event->user->getLogLabel(), 'type' => 'ldap']);
- }
+ try {
+ $guard = $event->guard;
+ $event->user->last_login = now();
+ $event->user->save();
+
+ if ($guard === 'users') {
+ Counter::get('login_total')->inc('local');
+ Log::info('Local user {user} has been successfully authenticated.', ['user' => $event->user->getLogLabel()]);
+ } elseif ($guard === 'ldap') {
+ Counter::get('login_total')->inc('ldap');
+ Log::info('External user {user} has been successfully authenticated.', ['user' => $event->user->getLogLabel(), 'type' => 'ldap']);
+ }
+ } catch (\Throwable $e) {
+ Log::warning('Post-login side effects failed.', ['error' => $e->getMessage()]);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Listeners/SuccessfulLogin.php` around lines 27 - 38, Wrap all
non-essential side-effects in the SuccessfulLogin listener (the block modifying
$event->user->last_login and calling $event->user->save(), the
Counter::get(...)->inc(...) calls, and the Log::info(...) calls) in a try/catch
so any exception from DB, metrics, or logging is caught and does not bubble up
to the login flow; on catch, emit a safe warning-level Log::warning with
contextual info (include $event->user->getLogLabel() and the exception message)
and return (or continue) so authentication always succeeds even if side-effects
fail.
| RateLimiter::for('login', function (Request $request) { | ||
| $throttleKey = Str::transliterate(Str::lower($request->input(Fortify::username())).'|'.$request->ip()); | ||
|
|
||
| return Limit::perMinute(5)->by($throttleKey); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Login routes + throttle middleware =="
rg -nP 'login/(local|ldap)|api\.v1\.login\.(local|ldap)|throttle:login' --type php . -C2 || true
echo
echo "== Login payload fields used in app/tests =="
rg -nP 'login/local|login/ldap|username|email|Fortify::username' --type php --type js --type vue . -C1 || true
echo
echo "== Current limiter block =="
if [ -f app/Providers/RouteServiceProvider.php ]; then
nl -ba app/Providers/RouteServiceProvider.php | sed -n '48,66p'
fiRepository: THM-Health/PILOS
Length of output: 15481
🏁 Script executed:
#!/bin/bash
# Locate Fortify configuration
echo "== Looking for Fortify config =="
fd -e php . config | head -20
echo
rg -l 'Fortify|username.*email|FORTIFY' config --type php || trueRepository: THM-Health/PILOS
Length of output: 511
🏁 Script executed:
#!/bin/bash
# Find LDAP login controller/implementation
echo "== LDAP login implementation =="
rg -nP 'ldap|LDAP' --type php app/Http/Controllers -C3 || true
echo
rg -nP 'class.*Login|function.*login' --type php app/Http/Controllers -C2 || trueRepository: THM-Health/PILOS
Length of output: 725
🏁 Script executed:
#!/bin/bash
# Check form submission structure
echo "== Login form frontend (vue/js) =="
find resources/js -name '*login*' -o -name '*auth*' 2>/dev/null | head -10
echo
rg -nP 'username|email' resources/js --type vue --type js -C2 || trueRepository: THM-Health/PILOS
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Read Fortify configuration
echo "== Fortify config (username field) =="
cat config/fortify.php | grep -i username -A2 -B2 || trueRepository: THM-Health/PILOS
Length of output: 582
🏁 Script executed:
#!/bin/bash
# Find and read LDAP login controller
echo "== LDAP controller login method =="
find app/Http/Controllers -name '*LDAP*' -type fRepository: THM-Health/PILOS
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Read RouteServiceProvider around the limiter
echo "== RouteServiceProvider limiter block =="
cat app/Providers/RouteServiceProvider.php | sed -n '45,70p'Repository: THM-Health/PILOS
Length of output: 1160
🏁 Script executed:
#!/bin/bash
# Check LoginRequest validation to see username field requirements
echo "== LoginRequest validation =="
find app/Http/Requests -name '*Login*' -type f | xargs cat 2>/dev/null || trueRepository: THM-Health/PILOS
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Check auth store to see what fields are submitted
echo "== Auth store (frontend) =="
cat resources/js/stores/auth.js | head -100 || trueRepository: THM-Health/PILOS
Length of output: 2342
Rate limiter key derivation bypasses per-user throttling for LDAP logins.
LDAP login submissions use the username field, but the rate limiter reads Fortify::username() (configured as email). When LDAP requests lack an email field, $request->input('email') returns null. After Str::lower(null) and string concatenation, the throttle key becomes '|'.$request->ip(), collapsing all LDAP attempts from the same IP into a single rate limit bucket instead of per-user buckets.
Proposed fix
RateLimiter::for('login', function (Request $request) {
- $throttleKey = Str::transliterate(Str::lower($request->input(Fortify::username())).'|'.$request->ip());
+ $loginIdentifier = $request->input(Fortify::username())
+ ?? $request->input('username')
+ ?? $request->input('email')
+ ?? '';
+
+ $throttleKey = Str::transliterate(
+ Str::lower((string) $loginIdentifier).'|'.$request->ip()
+ );
return Limit::perMinute(5)->by($throttleKey);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RateLimiter::for('login', function (Request $request) { | |
| $throttleKey = Str::transliterate(Str::lower($request->input(Fortify::username())).'|'.$request->ip()); | |
| return Limit::perMinute(5)->by($throttleKey); | |
| RateLimiter::for('login', function (Request $request) { | |
| $loginIdentifier = $request->input(Fortify::username()) | |
| ?? $request->input('username') | |
| ?? $request->input('email') | |
| ?? ''; | |
| $throttleKey = Str::transliterate( | |
| Str::lower((string) $loginIdentifier).'|'.$request->ip() | |
| ); | |
| return Limit::perMinute(5)->by($throttleKey); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Providers/RouteServiceProvider.php` around lines 54 - 57, Rate limiter
key currently uses $request->input(Fortify::username()) which may be null for
LDAP logins (Fortify::username() is 'email'), causing throttling to collapse to
IP-only; update the RateLimiter::for('login') throttle key logic to prefer the
Fortify username but fall back to the LDAP input (e.g.
$request->input('username')) and coerce to string before lowering so the key
becomes Str::transliterate(Str::lower((string) $username).'|'.$request->ip());
adjust the variable referenced ($throttleKey) in that closure to use this
fallback and string casting to restore per-user throttling.
There was a problem hiding this comment.
Inside app/Auth/LDAP/LDAPController.php the fortify.username is changed to username for the request
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the LDAPController constructor and login method to see fortify.username config
echo "== LDAPController constructor and login method =="
cat app/Auth/LDAP/LDAPController.php
Length of output: 1137
@samuelwei, thank you for that clarification! I can see that the LDAPController constructor does indeed set config(['fortify.username' => 'username']) at line 17.
However, I have a concern about the timing of when this config change takes effect. In Laravel's request lifecycle, the middleware pipeline (including throttle:login) typically executes before the controller is instantiated. This means:
- Request hits the LDAP login route
throttle:loginmiddleware runs → rate limiter closure evaluatesFortify::username()- At this point,
LDAPControllerhasn't been instantiated yet, sofortify.usernameis still'email' - Controller is instantiated → constructor sets
fortify.usernameto'username'(too late for rate limiting)
Result: The rate limiter might still be reading $request->input('email') (which returns null for LDAP requests), causing the throttle key to collapse to just '|'.$ip.
Could you verify whether per-user LDAP throttling is actually working as expected? You could test by:
- Making multiple failed LDAP login attempts with different usernames from the same IP
- Checking if they hit the rate limit after 5 attempts per username (correct) or 5 attempts total from that IP (incorrect)
If the timing issue is real, the config change would need to happen earlier (e.g., in a middleware or service provider boot) or the rate limiter would need the fallback I suggested. 🤔
There was a problem hiding this comment.
Both tests and debugging shows that Fortify::username() is correctly set to username. Requesting 5x with one username results in further requests being rate-limited, trying again with an other username allows 5x requests for this username
There was a problem hiding this comment.
@samuelwei, thank you for testing and verifying this! Your findings confirm that the per-user throttling is working correctly, and my concern about the timing of the config change was unfounded.
It appears that the config(['fortify.username' => 'username']) call in the LDAPController constructor does take effect before the rate limiter evaluates Fortify::username(), despite my expectation about the typical middleware execution order. The throttle key is correctly using the username field for LDAP requests, ensuring that each username+IP combination gets its own 5-attempt limit as intended.
I apologize for the false alarm, and appreciate you taking the time to debug and test this thoroughly! 🙏
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Auth/LDAP/LDAPController.php`:
- Around line 14-19: The constructor in LDAPController (public function
__construct(StatefulGuard $guard)) sets config(['fortify.guard' => 'ldap']) too
late for dependency injection, so the injected StatefulGuard is resolved with
the default 'users' guard; remove the config mutation from
LDAPController::__construct and instead register a contextual binding in a
service provider using
$this->app->when(LDAPController::class)->needs(StatefulGuard::class)->give(fn()
=> Auth::guard('ldap')), ensuring the controller receives the correct 'ldap'
guard instance at resolution time.
- Around line 29-34: The code is accessing non-guaranteed internals of the
LoginResponse contract (using $response->exception and
$response->getStatusCode()) — instead, catch exceptions thrown by
loginPipeline() and handle counting there: wrap the call to
loginPipeline($request)->then(...) in a try/catch (or attach a ->catch(...) to
the pipeline) and in the catch block inspect the caught Throwable (e.g., check
for an HttpException or $e->getStatusCode() ===
ResponseAlias::HTTP_TOO_MANY_REQUESTS) to decide whether to increment
Counter::get('login_failed_total')->inc('ldap') and then rethrow or return the
appropriate response; remove any direct property access on the LoginResponse
instance and rely only on its toResponse()/Responsable contract.
In `@app/Auth/MissingAttributeException.php`:
- Around line 16-19: The render method in MissingAttributeException currently
calls abort(500, ...) which throws instead of returning a JsonResponse; update
MissingAttributeException::render(Request $request): JsonResponse to construct
and return a JsonResponse directly (e.g., use response()->json(...) or new
JsonResponse(...)) with the error payload and 500 status, using
__('auth.error.missing_attributes') as the message so the method's declared
return type is honored.
In `@app/Http/Controllers/api/v1/auth/ResetPasswordController.php`:
- Line 29: The ResetPasswordRequest FormRequest is missing an authorize()
implementation causing 403s when ResetPasswordController::reset is called; open
the ResetPasswordRequest class and add a public function authorize() that
returns true so the request is allowed, keeping validation rules untouched;
ensure the method signature matches Laravel FormRequest (public function
authorize(): bool) and save the file so ResetPasswordController::reset no longer
receives a forbidden response.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
app/Auth/ExternalUser.phpapp/Auth/LDAP/LDAPController.phpapp/Auth/MissingAttributeException.phpapp/Http/Controllers/api/v1/auth/ForgotPasswordController.phpapp/Http/Controllers/api/v1/auth/LoginController.phpapp/Http/Controllers/api/v1/auth/ResetPasswordController.phpapp/Http/Requests/ForgotPasswordRequest.phpapp/Http/Requests/ResetPasswordRequest.phpapp/Listeners/SuccessfulLogin.phpapp/Providers/EventServiceProvider.phpapp/Providers/FortifyServiceProvider.phpapp/Providers/RouteServiceProvider.phpcomposer.jsonconfig/app.phpconfig/fortify.phpresources/js/views/Login.vueroutes/api.phptests/Backend/Feature/api/v1/LoginTest.phptests/Backend/Unit/MeetingTest.phptests/Frontend/e2e/Login.cy.js
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/Backend/Feature/api/v1/LoginTest.php
- tests/Frontend/e2e/Login.cy.js
- app/Listeners/SuccessfulLogin.php
- composer.json
- config/app.php
- tests/Backend/Unit/MeetingTest.php
- app/Http/Requests/ResetPasswordRequest.php
- resources/js/views/Login.vue
- config/fortify.php
- app/Providers/RouteServiceProvider.php
- routes/api.php
- app/Http/Requests/ForgotPasswordRequest.php
| $response = $this->loginPipeline($request)->then(function ($request) { | ||
| return app(LoginResponse::class); | ||
| }); | ||
|
|
||
| return abort(500, __('auth.error.missing_attributes')); | ||
| if ($response->exception != null && $response->getStatusCode() !== ResponseAlias::HTTP_TOO_MANY_REQUESTS) { | ||
| Counter::get('login_failed_total')->inc('ldap'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify bound LoginResponse implementation and whether it exposes getStatusCode/exception.
rg -n -C4 "Laravel\\\\Fortify\\\\Contracts\\\\LoginResponse|implements\\s+LoginResponse|class\\s+.*LoginResponse" app
rg -n -C3 "function\\s+toResponse\\s*\\(" appRepository: THM-Health/PILOS
Length of output: 1570
🏁 Script executed:
#!/bin/bash
# Check the actual code at lines 29-34
cat -n app/Auth/LDAP/LDAPController.php | sed -n '20,40p'
# Search for service provider bindings
rg -n "LoginResponse|bind.*Response" app/Providers/ config/Repository: THM-Health/PILOS
Length of output: 697
🏁 Script executed:
#!/bin/bash
# Search for any custom LoginResponse implementation in app
fd -t f "LoginResponse" app
# Check if there's any explicit binding or custom implementation
rg -n "class.*Response.*implements|LoginResponse" app --type php
# Let's also check the actual response chain and what loginPipeline returns
rg -n "loginPipeline\|toResponse" app/Auth/LDAP/LDAPController.php -A5 -B5Repository: THM-Health/PILOS
Length of output: 423
🌐 Web query:
Laravel Fortify LoginResponse contract interface methods
💡 Result:
Laravel\Fortify\Contracts\LoginResponse does not define any additional methods of its own; it’s a response “marker” contract that extends Laravel’s Illuminate\Contracts\Support\Responsable, so the only required method is:
toResponse(\Illuminate\Http\Request $request): \Symfony\Component\HttpFoundation\Response(return an HTTP response for a successful login) [1]
Fortify resolves this contract from the container after a successful login so you can override the post-login response/redirect by binding your own implementation. [2]
Sources: [1] (api.laravel.com) [2] (laravel.com)
Citations:
- 1: https://api.laravel.com/docs/11.x/Illuminate/Contracts/Support/Responsable.html
- 2: https://laravel.com/docs/11.x/fortify?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Search for loginPipeline definition
rg -n "loginPipeline|Pipeline" app/Auth/LDAP/LDAPController.php -B10 -A10
# Search in parent class (AuthenticatedSessionController)
rg -n "class AuthenticatedSessionController|loginPipeline" app --type php -A5 -B5
# Check what the pipeline returns - look for parent class definition
rg -n "extends AuthenticatedSessionController" app --type phpRepository: THM-Health/PILOS
Length of output: 2593
🌐 Web query:
Laravel Fortify default LoginResponse implementation exception getStatusCode
💡 Result:
Fortify’s built-in Laravel\Fortify\Http\Responses\LoginResponse returns a real Symfony/Laravel Response:
- if
$request->wantsJson()→response()->json(['two_factor' => false]) - else →
redirect()->intended(config('fortify.home'))[1]
So an exception like “Call to a member function getStatusCode() on …” typically happens when you override LoginResponse (or another response in the pipeline) and accidentally return something that is not an HTTP response (e.g., an array, string, User model, etc.). Laravel (or a middleware/exception handler) later assumes it’s a Response and calls getStatusCode().
Fix: make sure your custom response returns \Symfony\Component\HttpFoundation\Response, e.g.:
use Symfony\Component\HttpFoundation\Response;
use Laravel\Fortify\Contracts\LoginResponse as LoginResponseContract;
class LoginResponse implements LoginResponseContract
{
public function toResponse($request): Response
{
return $request->wantsJson()
? response()->json(['ok' => true], 200)
: redirect()->intended(config('fortify.home'));
}
}And ensure you’ve actually bound it:
$this->app->singleton(LoginResponseContract::class, \App\Http\Responses\LoginResponse::class);[2]
If you expect JSON but are getting redirects/HTML, ensure the client sends headers that make Laravel treat it as JSON (since Fortify uses wantsJson() for the JSON branch). [3]
Sources:
[1] Laravel News snippet showing Fortify default LoginResponse logic
[2] Example of binding a custom Fortify LoginResponse (Stack Overflow)
[3] Fortify docs: XHR/JSON vs redirect behavior and customizing responses
🌐 Web query:
Laravel Fortify loginPipeline exception property ValidationException pipeline
💡 Result:
In Fortify, the login flow is a Laravel Pipeline. If any action in the pipeline throws a ValidationException, the pipeline immediately stops and Laravel’s exception handler turns it into the normal “invalid credentials” response (redirect with errors for web requests, or 422 JSON for API requests).
Why your “custom AttemptToAuthenticate” doesn’t see the exception
Even when you’re not using 2FA, RedirectIfTwoFactorAuthenticatable can run before AttemptToAuthenticate and can be the one that throws the failed-login ValidationException, so your customized AttemptToAuthenticate::throwFailedAuthenticationException() never executes. (github.com)
Fix options
-
If you want to change the error bag / messages, you must catch and rethrow the exception in the stage that actually throws it (often
RedirectIfTwoFactorAuthenticatable, notAttemptToAuthenticate). The GitHub issue shows this exact pitfall and why the pipeline stops where it does. (github.com) -
Or adjust your login pipeline so that your customized action runs where you expect (or remove/replace
RedirectIfTwoFactorAuthenticatablewhen you truly don’t need it).
Reminder: where the default “auth failed” exception comes from
Fortify’s default failed-auth behavior is to throw:
throw ValidationException::withMessages([
Fortify::username() => [trans('auth.failed')],
]);which is what ends the pipeline. (stackoverflow.com)
Citations:
- 1: AttemptToAuthenticate is not throwing validation exceptions by default laravel/fortify#120
- 2: AttemptToAuthenticate is not throwing validation exceptions by default laravel/fortify#120
- 3: https://stackoverflow.com/questions/65097400/laravel-8-class-laravel-fortify-actions-auth-not-found?utm_source=openai
🌐 Web query:
Laravel Response exception property pipeline handler wrapper
💡 Result:
In Laravel’s HTTP middleware stack, the “pipeline handler wrapper” you’re seeing is Illuminate\Routing\Pipeline, a subclass of Illuminate\Pipeline\Pipeline that catches exceptions thrown by middleware/pipes and converts them into an HTTP Response via Laravel’s registered ExceptionHandler. (fossies.org)
What happens in that wrapper (why it looks like a “Response exception property” thing)
When an exception occurs while the request is going through middleware:
Illuminate\Routing\Pipeline::handleException()resolvesIlluminate\Contracts\Debug\ExceptionHandlerfrom the container.- It calls:
$handler->report($e)$response = $handler->render($request, $e)
- If the produced
$responsehas awithException()method, Laravel calls it to attach the exception to the response object (so later layers/debug tooling can read it). - The response is then normalized (e.g., if it’s
Responsable, it’s converted viatoResponse). (fossies.org)
Where to customize the behavior
You generally don’t change the pipeline—you customize exception-to-response rendering in Laravel’s exception handling configuration (Laravel 12: bootstrap/app.php via withExceptions()->render(...), or equivalent project setup). (laravel.com)
If you tell me the exact error / stack trace line
If you paste the exact exception message you got (e.g., “Undefined property … response” or “Call to a member function … on Response”), I can point to the precise object involved (the thrown exception vs. the rendered response vs. a Responsable) and the correct fix.
Citations:
- 1: https://fossies.org/linux/laravel-framework/src/Illuminate/Routing/Pipeline.php?utm_source=openai
- 2: https://fossies.org/linux/laravel-framework/src/Illuminate/Routing/Pipeline.php?utm_source=openai
- 3: https://laravel.com/docs/12.x/errors?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Search for LoginResponse binding in service providers
rg -n "LoginResponse|bind.*Response" app/Providers/ -A3 -B3
# Also check config files
rg -n "LoginResponse" config/ -A3 -B3
# Let's look at the complete LDAPController to understand context
cat -n app/Auth/LDAP/LDAPController.phpRepository: THM-Health/PILOS
Length of output: 1420
🏁 Script executed:
#!/bin/bash
# Search all service providers
find app/Providers -name "*.php" -exec basename {} \; | head -20
# Check for any binding of LoginResponse
rg -n "singleton\|bind" app/Providers/ -A2 -B2 | grep -i response
# Also check if there's a custom response class anywhere
fd -t f "Response\.php|LoginResponse\.php" appRepository: THM-Health/PILOS
Length of output: 351
🏁 Script executed:
#!/bin/bash
# Check FortifyServiceProvider for LoginResponse binding
cat -n app/Providers/FortifyServiceProvider.php
# Also check AppServiceProvider
echo "=== AppServiceProvider ==="
cat -n app/Providers/AppServiceProvider.phpRepository: THM-Health/PILOS
Length of output: 2404
🌐 Web query:
Laravel Pipeline then() method what does it return Responsable
💡 Result:
Illuminate\Pipeline\Pipeline::then(Closure $destination) returns whatever your $destination closure returns (type: mixed). Internally, Laravel builds a callable “onion” of pipes and then executes it with the original passable, finally returning the result of that execution (return $pipeline($this->passable);). So if your destination returns a Responsable (or a Response), then() will return that object as-is—Pipeline itself does not convert it to an HTTP response. [1]
Related: thenReturn() is just a convenience that runs the pipeline and returns the final $passable value (it calls then(fn ($passable) => $passable)). [1]
Sources:
[1] Laravel framework source (Pipeline::then, Pipeline::thenReturn) https://raw.githubusercontent.com/laravel/framework/12.x/src/Illuminate/Pipeline/Pipeline.php
Accessing undeclared response properties breaks contract abstraction.
Lines 33–34 access $response->exception and $response->getStatusCode() on a LoginResponse contract instance. The Laravel\Fortify\Contracts\LoginResponse contract only requires toResponse() (from Responsable); it does not guarantee exception or getStatusCode() properties. If a custom LoginResponse implementation is bound later, this code will fail with "undefined property/method" errors.
Exceptions thrown during the pipeline should be caught where they occur, not inspected from response internals:
Suggested pattern
- $response = $this->loginPipeline($request)->then(function ($request) {
- return app(LoginResponse::class);
- });
-
- if ($response->exception != null && $response->getStatusCode() !== ResponseAlias::HTTP_TOO_MANY_REQUESTS) {
- Counter::get('login_failed_total')->inc('ldap');
- }
-
- return $response;
+ try {
+ return $this->loginPipeline($request)->then(function ($request) {
+ return app(LoginResponse::class);
+ });
+ } catch (\Illuminate\Validation\ValidationException $e) {
+ if (($e->status ?? null) !== ResponseAlias::HTTP_TOO_MANY_REQUESTS) {
+ Counter::get('login_failed_total')->inc('ldap');
+ }
+ throw $e;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Auth/LDAP/LDAPController.php` around lines 29 - 34, The code is accessing
non-guaranteed internals of the LoginResponse contract (using
$response->exception and $response->getStatusCode()) — instead, catch exceptions
thrown by loginPipeline() and handle counting there: wrap the call to
loginPipeline($request)->then(...) in a try/catch (or attach a ->catch(...) to
the pipeline) and in the catch block inspect the caught Throwable (e.g., check
for an HttpException or $e->getStatusCode() ===
ResponseAlias::HTTP_TOO_MANY_REQUESTS) to decide whether to increment
Counter::get('login_failed_total')->inc('ldap') and then rethrow or return the
appropriate response; remove any direct property access on the LoginResponse
instance and rely only on its toResponse()/Responsable contract.
| public function render(Request $request): JsonResponse | ||
| { | ||
| abort(500, __('auth.error.missing_attributes')); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "MissingAttributeException.php" | head -20Repository: THM-Health/PILOS
Length of output: 100
🏁 Script executed:
cat -n app/Auth/MissingAttributeException.phpRepository: THM-Health/PILOS
Length of output: 629
🏁 Script executed:
# Find other exception classes with render methods
find . -type f -name "*Exception.php" | head -20Repository: THM-Health/PILOS
Length of output: 419
🏁 Script executed:
# Check other exception render implementations
for file in ./app/Auth/OIDC/OpenIDConnectClientException.php ./app/Auth/OIDC/OpenIDConnectCodeMissingException.php ./app/Auth/Shibboleth/ShibbolethSessionDuplicateException.php; do
echo "=== $file ==="
cat -n "$file"
doneRepository: THM-Health/PILOS
Length of output: 946
🏁 Script executed:
# Search for other render methods in exceptions
rg "public function render" --type php -A 5 app/Repository: THM-Health/PILOS
Length of output: 384
🏁 Script executed:
# Check exception handler
find . -type f -name "Handler.php" | grep -i exceptionRepository: THM-Health/PILOS
Length of output: 88
🏁 Script executed:
cat -n app/Exceptions/Handler.phpRepository: THM-Health/PILOS
Length of output: 2504
🏁 Script executed:
# Search for how abort is used in authentication context
rg "abort\(" app/Auth/ -B 2 -A 2Repository: THM-Health/PILOS
Length of output: 384
🏁 Script executed:
# Check if there are any other patterns where exceptions define render() methods
rg "function render\(" app/ -B 2 -A 5 --type phpRepository: THM-Health/PILOS
Length of output: 469
🏁 Script executed:
# Check how abort() is used elsewhere in API/JSON contexts
rg "abort\(" app/ -B 3 -A 1 --type php | grep -E "(JsonResponse|json|API|api)" -B 5 -A 2 | head -50Repository: THM-Health/PILOS
Length of output: 4022
Return an explicit JsonResponse from render() instead of calling abort().
The method signature declares JsonResponse as the return type, but abort() throws an exception instead of returning. This violates the explicit type contract. In a render() method that declares a specific return type, returning the response directly is clearer and ensures the method behaves as its signature promises.
Proposed fix
public function render(Request $request): JsonResponse
{
- abort(500, __('auth.error.missing_attributes'));
+ return response()->json([
+ 'message' => __('auth.error.missing_attributes'),
+ ], 500);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function render(Request $request): JsonResponse | |
| { | |
| abort(500, __('auth.error.missing_attributes')); | |
| } | |
| public function render(Request $request): JsonResponse | |
| { | |
| return response()->json([ | |
| 'message' => __('auth.error.missing_attributes'), | |
| ], 500); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 16-16: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Auth/MissingAttributeException.php` around lines 16 - 19, The render
method in MissingAttributeException currently calls abort(500, ...) which throws
instead of returning a JsonResponse; update
MissingAttributeException::render(Request $request): JsonResponse to construct
and return a JsonResponse directly (e.g., use response()->json(...) or new
JsonResponse(...)) with the error payload and 500 status, using
__('auth.error.missing_attributes') as the message so the method's declared
return type is honored.
| * @throws ValidationException | ||
| */ | ||
| public function reset(Request $request) | ||
| public function reset(ResetPasswordRequest $request) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether ResetPasswordRequest explicitly allows authorization.
sed -n '1,220p' app/Http/Requests/ResetPasswordRequest.php
rg -nP --type=php 'function\s+authorize\s*\(' app/Http/Requests/ResetPasswordRequest.phpRepository: THM-Health/PILOS
Length of output: 633
Implement authorize() method in ResetPasswordRequest to prevent 403 responses.
At Line 29, the endpoint depends on ResetPasswordRequest, which does not implement authorize(). In Laravel 8+, FormRequest defaults to returning false for authorize(), causing the request to fail with a 403 Forbidden response. Add an explicit authorize() method returning true to app/Http/Requests/ResetPasswordRequest.php.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/api/v1/auth/ResetPasswordController.php` at line 29, The
ResetPasswordRequest FormRequest is missing an authorize() implementation
causing 403s when ResetPasswordController::reset is called; open the
ResetPasswordRequest class and add a public function authorize() that returns
true so the request is allowed, keeping validation rules untouched; ensure the
method signature matches Laravel FormRequest (public function authorize(): bool)
and save the file so ResetPasswordController::reset no longer receives a
forbidden response.
93895ea to
b598a62
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Providers/FortifyServiceProvider.php (1)
26-31: Consider removing unnecessaryarray_filter()call.The array contains only class name strings, which are always truthy. The
array_filter()serves no purpose here unless you plan to conditionally add elements that could benullorfalsein the future.Proposed simplification
Fortify::authenticateThrough(function (Request $request) { - return array_filter([ + return [ AttemptToAuthenticate::class, PrepareAuthenticatedSession::class, - ]); + ]; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Providers/FortifyServiceProvider.php` around lines 26 - 31, The authenticateThrough callback wraps the middleware list in an unnecessary array_filter call; update the Fortify::authenticateThrough closure to return a plain array of middleware instead of array_filter([...]) — replace array_filter([AttemptToAuthenticate::class, PrepareAuthenticatedSession::class]) with [AttemptToAuthenticate::class, PrepareAuthenticatedSession::class] inside the Fortify::authenticateThrough(function (Request $request) { ... }) block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Providers/FortifyServiceProvider.php`:
- Around line 26-31: The authenticateThrough callback wraps the middleware list
in an unnecessary array_filter call; update the Fortify::authenticateThrough
closure to return a plain array of middleware instead of array_filter([...]) —
replace array_filter([AttemptToAuthenticate::class,
PrepareAuthenticatedSession::class]) with [AttemptToAuthenticate::class,
PrepareAuthenticatedSession::class] inside the
Fortify::authenticateThrough(function (Request $request) { ... }) block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ddd901d4-cc72-467a-8f07-5ed7c88bfc56
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
app/Auth/ExternalUser.phpapp/Auth/LDAP/LDAPController.phpapp/Auth/MissingAttributeException.phpapp/Http/Controllers/api/v1/auth/ForgotPasswordController.phpapp/Http/Controllers/api/v1/auth/LoginController.phpapp/Http/Controllers/api/v1/auth/ResetPasswordController.phpapp/Http/Requests/ForgotPasswordRequest.phpapp/Http/Requests/ResetPasswordRequest.phpapp/Listeners/SuccessfulLogin.phpapp/Providers/EventServiceProvider.phpapp/Providers/FortifyServiceProvider.phpapp/Providers/RouteServiceProvider.phpcomposer.jsonconfig/app.phpconfig/fortify.phpresources/js/views/Login.vueroutes/api.phptests/Backend/Feature/api/v1/LoginTest.phptests/Backend/Unit/MeetingTest.phptests/Frontend/e2e/Login.cy.js
✅ Files skipped from review due to trivial changes (3)
- app/Auth/ExternalUser.php
- config/fortify.php
- app/Http/Requests/ResetPasswordRequest.php
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/Backend/Feature/api/v1/LoginTest.php
- config/app.php
- composer.json
- app/Http/Requests/ForgotPasswordRequest.php
- app/Providers/EventServiceProvider.php
- tests/Backend/Unit/MeetingTest.php
- app/Http/Controllers/api/v1/auth/ForgotPasswordController.php
- tests/Frontend/e2e/Login.cy.js
- app/Listeners/SuccessfulLogin.php
- resources/js/views/Login.vue
# Conflicts: # composer.json # composer.lock
samuelwei
force-pushed
the
refactor-auth
branch
from
b598a62 to
9d9ecdf
Compare
|
@samuelwei Check coderabbit comments |
1 participant
Type
Checklist
Changes
Other information
This change will make implementing features like 2FA easier
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests