Feature | Add AuthService validateCredentials method by matiasperrone-exo · Pull Request #127 · OpenStackweb/openstackid

matiasperrone-exo · 2026-04-27T21:58:01Z

Task:

Ref: https://app.clickup.com/t/86b9j51aa

Depends on:

https://app.clickup.com/t/86b9h8xt1 - Feature | Implement Two-Factor Authentication (2FA) support for users #126

Summary

Adds validateCredentials() and loginUser() to AuthService as the foundation for the MFA two-step login flow. The new methods split what was previously a single login() call into two discrete phases: verify the password (phase 1, no session) and establish the session (phase 2, after the second factor passes).

Changes

`app/libs/Auth/AuthService.php`

validateCredentials(string $username, string $password): User — validates credentials via Auth::getProvider()->retrieveByCredentials() without calling Auth::attempt(), so no session is established. Pre-checks the user's active state before touching the provider; if the account is locked it throws immediately with a distinct "is locked" message (the provider swallows AuthenticationLockedUserLoginAttempt and returns null, masking the lock state). Security checkpoints such as LockUserCounterMeasure still fire on provider-level failures.
loginUser(User $user, bool $remember): void — thin wrapper around Auth::login() for use after the second factor is verified.

`app/libs/Utils/Services/IAuthService.php`

Added validateCredentials() and loginUser() to the interface with full doc-blocks explaining the session-safety contract.

Tests

tests/unit/AuthServiceValidateCredentialsTest.php — isolated unit tests (Mockery alias mocks for Auth and Log facades, run in separate processes). Covers: valid credentials
return user without session, invalid credentials throw AuthenticationException, locked account short-circuits before calling the provider, active user does not take the locked path, loginUser() with remember = true/false.
tests/AuthServiceValidateCredentialsIntegrationTest.php — integration test against the real database and security-checkpoint stack. Verifies that a failed validateCredentials() call increments login_failed_attempt via LockUserCounterMeasure, and that a successful call returns the User entity without establishing a session (Auth::check() stays false).

Test plan

Run unit suite: docker compose exec app ./vendor/bin/phpunit tests/unit/AuthServiceValidateCredentialsTest.php
Run integration test: docker compose exec app ./vendor/bin/phpunit tests/AuthServiceValidateCredentialsIntegrationTest.php
Run full suite to check for regressions: docker compose exec app ./vendor/bin/phpunit

Requested Goal

Current state

AuthService::login() validates credentials AND establishes a session in a single call. There is no way to validate a user's password without logging them in, which is required to insert a 2FA gate between credential validation and session establishment.

Target state

AuthService exposes a new validateCredentials(string $username, string $password): User method that validates the password via the existing CustomAuthProvider::retrieveByCredentials() (which handles attempt tracking and account locking via security checkpoints) and returns the authenticated User object WITHOUT establishing a
session. Throws AuthenticationException on failure.

TASKS

Add validateCredentials() method to IAuthService interface
Implement validateCredentials() in AuthService: delegates to Auth::getProvider()->retrieveByCredentials() with username and password, throws AuthenticationException if user is null
Add loginUser(User $user, bool $remember) method to IAuthService interface (needed by verify2FA to establish session after 2FA succeeds) → it should call Auth::login($user, $remember);
Write unit test: valid credentials return User object without session being established
Write unit test: invalid credentials throw AuthenticationException
Write unit test: locked account throws AuthenticationException with locked-account message
Write integration test: validateCredentials() triggers security checkpoint tracking (failed attempt counter increments on failure)

ACCEPTANCE CRITERIA

validateCredentials() with correct username/password returns the User object
validateCredentials() with wrong password throws AuthenticationException
validateCredentials() does NOT call Auth::login() or establish any session
After validateCredentials() succeeds, Session::get('login_web') is null (no authenticated session)
Security checkpoints still fire: failed attempts increment the counter, account locking still triggers after threshold
loginUser() establishes a Laravel session for the given User object ( call Auth::login() )
All tests pass

DEVELOPMENT NOTES

Key files:

Modified: app/Services/Auth/IAuthService.php (interface)
Modified: app/Services/Auth/AuthService.php (implementation)
Modified: app/libs/Auth/CustomAuthProvider.php (reference only, do not modify)
New: tests/Unit/AuthServiceValidateCredentialsTest.php

Gotchas:

CustomAuthProvider::retrieveByCredentials() is the ONLY path for credential validation. Do NOT duplicate password checking logic. The provider handles password hashing, security checkpoints,
and lock countermeasures.
The existing login() method calls Auth::attempt() which both validates AND establishes a session. validateCredentials() must NOT use Auth::attempt().
LockUserCounterMeasure tracks failed attempts. Ensure validateCredentials() triggers this on failure by going through the same code path as the existing login flow.

Out of scope:

Controller changes

coderabbitai · 2026-04-27T21:58:09Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 67211df3-2ebb-4dab-880b-41676e45102d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/mfa-phase1-authservice-validatecredentials-method

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-04-27T21:59:29Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

github-actions · 2026-04-28T21:55:08Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

github-actions · 2026-04-28T22:08:49Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

github-actions · 2026-04-28T22:11:28Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

github-actions · 2026-04-28T22:14:34Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

github-actions · 2026-04-28T22:18:41Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

github-actions · 2026-04-29T14:31:51Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

github-actions · 2026-04-29T21:32:01Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

martinquiroga-exo

@matiasperrone-exo please see comments

smarcet

@matiasperrone-exo please review comments
@matiasperrone-exo @martinquiroga-exo changes on existent code are out of scope
anything like that should be discussed before any proposed changes

martinquiroga-exo

LGTM

github-actions · 2026-05-04T22:16:55Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-05T14:02:57Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

smarcet · 2026-05-05T14:34:49Z

@martinquiroga-exo the ticket ask for

Implement validateCredentials() in AuthService: delegates to Auth::getProvider()->retrieveByCredentials() with username and password, throws AuthenticationException if user is null

but the code is not following that please re review it

https://github.com/OpenStackweb/openstackid/pull/127/changes#diff-31b8c36cbff745e9a0410953fc435b4b933632a787b6815b2e861af3a5f6adb3R198

smarcet

@martinquiroga-exo @matiasperrone-exo please re review

github-actions · 2026-05-05T14:55:58Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

caseylocker

Two outstanding items before this can merge — both scoped to the new validateCredentials() method, no existing-code changes required:

The post-retrieveByCredentials() guard doesn't match what was requested in the prior review.
UnverifiedEmailMemberException escapes validateCredentials() uncaught, violating the IAuthService contract.

Inline comments below. Also worth a quick rewrite of the PR description before merge — it still describes the pre-check and getCurrentUser() instanceof User guard that were reverted in 70ba198, so the merged record won't match the diff.

matiasperrone-exo · 2026-05-07T20:50:21Z

Two outstanding items before this can merge — both scoped to the new validateCredentials() method, no existing-code changes required:

The post-retrieveByCredentials() guard doesn't match what was requested in the prior review.

UnverifiedEmailMemberException escapes validateCredentials() uncaught, violating the IAuthService contract.

Inline comments below. Also worth a quick rewrite of the PR description before merge — it still describes the pre-check and getCurrentUser() instanceof User guard that were reverted in 70ba198, so the merged record won't match the diff.

All the suggestion were introduced.

github-actions · 2026-05-07T21:31:10Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

caseylocker

LGTM

matiasperrone-exo · 2026-05-18T21:34:05Z

@smarcet please review again thanks!

github-actions · 2026-05-21T18:06:49Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-21T20:04:21Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

- test: cover canLogin()=false branch in validateCredentials() unit tests - docs: document known double-query cost in validateCredentials() - fix: use consistent error message in validateCredentials()

Add tests changes with suggestion

github-actions · 2026-05-21T21:26:55Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-127/

This page is automatically updated on each push to this PR.

matiasperrone-exo self-assigned this Apr 27, 2026

matiasperrone-exo force-pushed the feat/mfa-phase1-user-model branch 2 times, most recently from 133b2a8 to 1d8c6e4 Compare April 28, 2026 21:52

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from d560647 to f4ee340 Compare April 28, 2026 21:53

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from f4ee340 to a293dde Compare April 28, 2026 22:07

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from a293dde to 496a595 Compare April 28, 2026 22:10

matiasperrone-exo force-pushed the feat/mfa-phase1-user-model branch from 5f9b3a2 to 894cbe4 Compare April 28, 2026 22:10

matiasperrone-exo force-pushed the feat/mfa-phase1-user-model branch from 894cbe4 to fc6818d Compare April 28, 2026 22:12

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from 496a595 to 1bbe5ff Compare April 28, 2026 22:13

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from 1bbe5ff to e1bbb75 Compare April 28, 2026 22:17

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from e1bbb75 to ca405e6 Compare April 29, 2026 14:30

caseylocker mentioned this pull request Apr 29, 2026

Feature | Implement Two-Factor Authentication (2FA) support for users #126

Open

martinquiroga-exo suggested changes Apr 30, 2026

View reviewed changes

Comment thread app/libs/Auth/AuthService.php Outdated

Comment thread app/libs/Auth/AuthService.php Outdated

Comment thread app/libs/Auth/AuthService.php

smarcet reviewed Apr 30, 2026

View reviewed changes

Comment thread app/libs/Auth/AuthService.php Outdated

smarcet reviewed Apr 30, 2026

View reviewed changes

Comment thread app/libs/Auth/AuthService.php Outdated

smarcet reviewed Apr 30, 2026

View reviewed changes

Comment thread app/libs/Auth/AuthService.php Outdated

smarcet requested changes Apr 30, 2026

View reviewed changes

matiasperrone-exo force-pushed the feat/mfa-phase1-user-model branch 3 times, most recently from 4b760a8 to 9146757 Compare April 30, 2026 19:06

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from a810012 to cfd3bb7 Compare May 4, 2026 21:01

martinquiroga-exo approved these changes May 4, 2026

View reviewed changes

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from ecd7cd3 to 6b5f96e Compare May 5, 2026 14:01

smarcet requested changes May 5, 2026

View reviewed changes

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from 6b5f96e to 5f3c28e Compare May 5, 2026 14:54

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from 5f3c28e to 70ba198 Compare May 5, 2026 14:58

matiasperrone-exo requested review from caseylocker and smarcet May 5, 2026 15:06

caseylocker requested changes May 7, 2026

View reviewed changes

Comment thread app/libs/Auth/AuthService.php Outdated

Comment thread app/libs/Auth/AuthService.php Outdated

matiasperrone-exo requested a review from caseylocker May 7, 2026 20:47

caseylocker approved these changes May 7, 2026

View reviewed changes

matiasperrone-exo mentioned this pull request May 8, 2026

Feature | MFA Challenge Strategy Pattern (Interface, Abstract, Factory, EmailOTP) #129

Open

matiasperrone-exo force-pushed the feat/mfa-phase1-user-model branch from 9146757 to 45689ec Compare May 21, 2026 17:54

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from 28541f5 to f4fd63d Compare May 21, 2026 18:05

matiasperrone-exo force-pushed the feat/mfa-phase1-user-model branch from 45689ec to 410bbfa Compare May 21, 2026 21:25

matiasperrone-exo added 5 commits May 21, 2026 21:25

feat: Add AuthService validateCredentials method

4565337

- test: cover canLogin()=false branch in validateCredentials() unit tests - docs: document known double-query cost in validateCredentials() - fix: use consistent error message in validateCredentials()

chore: lint file app/libs/Auth/AuthService.php

63ac94a

chore: Add PR's requested changes

a339a47

chore: Add PR's requested changes

d4f4b25

Add tests changes with suggestion

chore: Fix issues created on rebase

a0c22ff

matiasperrone-exo force-pushed the feat/mfa-phase1-authservice-validatecredentials-method branch from 7e9fe41 to a0c22ff Compare May 21, 2026 21:25

Conversation

matiasperrone-exo commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Task:

Depends on:

Summary

Changes

app/libs/Auth/AuthService.php

app/libs/Utils/Services/IAuthService.php

Tests

Test plan

Requested Goal

Current state

Target state

TASKS

ACCEPTANCE CRITERIA

DEVELOPMENT NOTES

Gotchas:

Out of scope:

Uh oh!

coderabbitai Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Apr 27, 2026

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

github-actions Bot commented Apr 29, 2026

Uh oh!

github-actions Bot commented Apr 29, 2026

Uh oh!

martinquiroga-exo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smarcet left a comment

Choose a reason for hiding this comment

Uh oh!

martinquiroga-exo left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 4, 2026

Uh oh!

github-actions Bot commented May 5, 2026

Uh oh!

smarcet commented May 5, 2026

Uh oh!

smarcet left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 5, 2026

Uh oh!

caseylocker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matiasperrone-exo commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

caseylocker left a comment

Choose a reason for hiding this comment

Uh oh!

matiasperrone-exo commented May 18, 2026

Uh oh!

matiasperrone-exo commented Apr 27, 2026 •

edited

Loading

`app/libs/Auth/AuthService.php`

`app/libs/Utils/Services/IAuthService.php`

coderabbitai Bot commented Apr 27, 2026 •

edited

Loading