Feature/otp service refactor#130
Conversation
Split AuthService::verifyOTPChallenge into a strict primitive (no auto-register, no Auth::login) and refactor loginWithOTP to be a full implementation that wraps the same private helpers. Both public methods now take an OAuth2OTP $otpClaim — symmetric API. - Extract findAndValidateOTP private helper: TX-A find+logRedeemAttempt (committed independently to preserve brute-force counter), TX-B lifecycle/value/scope/audience checks. - Extract finalizeRedemption private helper: setAuthTime + setUserId + setUser + redeem + revoke siblings. Reads user_name from $otp->getUserName() — no extra parameter needed. - verifyOTPChallenge: strict primitive for MFA reuse. Throws AuthenticationException (not NPE) on missing user. Return type tightened to non-null OAuth2OTP. - loginWithOTP: signature byte-for-byte unchanged (?OAuth2OTP nullable). Body now does its own user resolution with the auto-register branch inline. - IAuthService updated to match impl signatures. Removes the boolean flag anti-pattern and the $top_required_scopes typo that was silently disabling the scope-escalation check. Plan: docs/plans/2026-05-09-remove-should-create-user-flag.md
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a transient ChangesOTP Challenge Verification
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AuthService
participant OTPRepo as IOAuth2OTPRepository
participant OTP as OAuth2OTP
participant UserRepo
participant Auth as AuthFacade
Caller->>AuthService: verifyOTPChallenge(claim, sessionUser, client?)
AuthService->>OTPRepo: findAndValidateOTP(claim)
OTPRepo-->>AuthService: OAuth2OTP (validated)
AuthService->>UserRepo: resolve OTP user (must exist)
alt user id matches sessionUser
AuthService->>OTPRepo: refreshExclusiveLock(OTP)
AuthService->>OTP: setAuthTime/setUserId/setUser
AuthService->>OTPRepo: redeem(OTP) and revoke siblings
OTPRepo-->>AuthService: confirmation
AuthService-->>Caller: returns OAuth2OTP
else mismatch
AuthService-->>Caller: throws AuthenticationException (no consume)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-130/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Models/OAuth2/OAuth2OTP.php (1)
122-127: 💤 Low valuePlace the transient
$userfield with the other non-DB fields.The other transient fields (
$auth_time,$user_id) are grouped under the// non db fieldsdivider at line 471 alongside their accessors at the bottom of the class. Placing$userin the middle of the Doctrine column-mapped declarations is inconsistent and obscures which fields are persisted. Functionally it is fine (no#[ORM\Column], so Doctrine ignores it), but moving it for consistency improves readability.♻️ Proposed move
@@ lines 122-127 - /** - * `@var` User - * this is a transient state - */ - private $user; - @@ near line 471 (// non db fields) // non db fields private $auth_time; private $user_id; + + /** + * `@var` User|null transient state, populated during OTP verification + */ + private $user;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/OAuth2/OAuth2OTP.php` around lines 122 - 127, The transient $user property is declared among Doctrine-mapped fields; move its declaration (private $user) into the existing "// non db fields" section with $auth_time and $user_id near the bottom of the OAuth2OTP class so all non-persisted fields are grouped together, and keep its docblock/accessor placement aligned with the other transient fields (ensure no ORM attributes are added and update ordering to match $auth_time/$user_id).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/libs/Auth/AuthService.php`:
- Around line 672-680: In AuthService::postLoginUserActions, when
$this->user_repository->getById($user_id) does not return an instance of User
you should log a warning before returning so the unexpected/missing post-login
user is observable; add a Log::warning that includes $user_id (and optionally
the repository/getById result or context) immediately prior to the early return
in the method, leaving the existing behavior and exception handling for inactive
users intact.
- Around line 416-420: invalidateSession currently encrypts Session::getId()
anew so it writes a different ciphertext than the JTI stored by generateJTI and
thus reloadSession never finds the "invalid" marker; change invalidateSession to
write the invalid marker under the exact same key used by generateJTI (i.e., use
the previously generated ciphertext/JTI value or derive a deterministic
identifier from the plain session id such as a stable hash) so reloadSession's
cache->exists(A . "invalid") check succeeds, and if you choose the deterministic
approach update generateJTI and reloadSession to use that same deterministic key
derivation consistently.
---
Nitpick comments:
In `@app/Models/OAuth2/OAuth2OTP.php`:
- Around line 122-127: The transient $user property is declared among
Doctrine-mapped fields; move its declaration (private $user) into the existing
"// non db fields" section with $auth_time and $user_id near the bottom of the
OAuth2OTP class so all non-persisted fields are grouped together, and keep its
docblock/accessor placement aligned with the other transient fields (ensure no
ORM attributes are added and update ordering to match $auth_time/$user_id).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02ca08e4-5a03-4380-9dcb-319e687c865a
📒 Files selected for processing (3)
app/Models/OAuth2/OAuth2OTP.phpapp/libs/Auth/AuthService.phpapp/libs/Utils/Services/IAuthService.php
| public function invalidateSession(): void | ||
| { | ||
| $session_id = Crypt::encrypt(Session::getId()); | ||
| $this->cache_service->addSingleValue($session_id . "invalid", $session_id); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is Laravel 12 Illuminate\Support\Facades\Crypt::encrypt deterministic, or does it use a random IV per call?
💡 Result:
Laravel 12’s Illuminate\Support\Facades\Crypt::encrypt is non-deterministic: it generates a fresh random IV for every encrypt call. In the encryption implementation, Encrypter::encrypt does $iv = random_bytes(openssl_cipher_iv_length(...)) and then includes that IV in the returned payload (it base64-encodes the IV and stores it in the JSON returned from encrypt()). [1] [2] So even if you encrypt the same plaintext multiple times with the same APP_KEY, the output will differ because the IV differs per call. [1]
Citations:
- 1: https://github.com/laravel/framework/blob/12.x/src/Illuminate/Encryption/Encrypter.php
- 2: https://github.com/illuminate/encryption/blob/master/Encrypter.php
Critical: invalidateSession() writes a marker key that reloadSession() will never find.
Laravel's Crypt::encrypt uses a random IV per invocation, so calling it twice on the same Session::getId() produces different ciphertexts. The contract on the cache is:
generateJTI()(line 655) stores$jti => Crypt::encrypt(Session::getId())— call this ciphertext A.reloadSession()(line 624/630) reads back A via$jtiand probescache->exists(A . "invalid").invalidateSession()here re-encryptsSession::getId()to produce a different ciphertext B and storesB . "invalid".
Result: the "invalid" marker is keyed under B, but reloadSession only ever looks for it under A, so a logged-out session is never observed as invalidated. The session-flush / principal-clear in logout() partially mitigates this, but the explicit invalidation marker is silently dead, weakening the defense for anyone reusing a captured JTI before the cache TTL expires.
Use the same JTI key (or another deterministic identifier derived from the plain session id) so the lookup actually matches.
🔒 One possible fix: key the invalidation off the plain session id (or off all active JTIs)
- public function invalidateSession(): void
- {
- $session_id = Crypt::encrypt(Session::getId());
- $this->cache_service->addSingleValue($session_id . "invalid", $session_id);
- }
+ public function invalidateSession(): void
+ {
+ // Use a deterministic key so reloadSession() can actually detect the marker.
+ // reloadSession looks up `$session_id . "invalid"` where $session_id is the
+ // ciphertext stored at JTI generation time; that ciphertext is non-deterministic,
+ // so we cannot reproduce it here. Hash the plain session id instead and have
+ // generateJTI()/reloadSession() probe the same hash-based key.
+ $marker = hash('sha256', Session::getId()) . ':invalid';
+ $this->cache_service->addSingleValue($marker, '1');
+ }(generateJTI and reloadSession would need the matching change.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/libs/Auth/AuthService.php` around lines 416 - 420, invalidateSession
currently encrypts Session::getId() anew so it writes a different ciphertext
than the JTI stored by generateJTI and thus reloadSession never finds the
"invalid" marker; change invalidateSession to write the invalid marker under the
exact same key used by generateJTI (i.e., use the previously generated
ciphertext/JTI value or derive a deterministic identifier from the plain session
id such as a stable hash) so reloadSession's cache->exists(A . "invalid") check
succeeds, and if you choose the deterministic approach update generateJTI and
reloadSession to use that same deterministic key derivation consistently.
| $this->tx_service->transaction(function () use ($user_id) { | ||
| $user = $this->user_repository->getById($user_id); | ||
| if(!$user instanceof User) return; | ||
| if (!$user instanceof User) return; | ||
|
|
||
| if (!$user->isActive()) { | ||
| Log::warning(sprintf("AuthService::postLoginUserActions user %s is not active.", $user_id)); | ||
| Log::warning(sprintf("AuthService::postLoginUserActions user %s is not active.", $user_id)); | ||
| throw new AuthenticationLockedUserLoginAttempt($user->getEmail(), | ||
| sprintf("User %s is locked.", $user->getEmail())); | ||
| } |
There was a problem hiding this comment.
Don't silently swallow a missing post-login user.
postLoginUserActions is invoked right after a successful authentication for $user_id, so getById($user_id) returning anything other than a User indicates a real inconsistency (deleted user, wrong repository, etc.). Returning silently leaves no trace in logs, while the surrounding code still treats the login as fully successful. At minimum log a warning so this is observable.
🪵 Add a warning log before returning
$this->tx_service->transaction(function () use ($user_id) {
$user = $this->user_repository->getById($user_id);
- if (!$user instanceof User) return;
+ if (!$user instanceof User) {
+ Log::warning(sprintf(
+ "AuthService::postLoginUserActions user %s not found or not a User instance; skipping.",
+ $user_id
+ ));
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/libs/Auth/AuthService.php` around lines 672 - 680, In
AuthService::postLoginUserActions, when
$this->user_repository->getById($user_id) does not return an instance of User
you should log a warning before returning so the unexpected/missing post-login
user is observable; add a Log::warning that includes $user_id (and optionally
the repository/getById result or context) immediately prior to the early return
in the method, leaving the existing behavior and exception handling for inactive
users intact.
Acquire a PESSIMISTIC_WRITE row lock and re-hydrate the OTP entity at the start of finalizeRedemption(). Concurrent submitters of the same OTP now serialize on the row; the loser sees redeemed_at populated and gets a clean AuthenticationException instead of silently double-redeeming or surfacing OAuth2OTP::redeem()'s ValidationException as a 500. The split TX-A/TX-B/TX-C structure already widened the pre-existing race window between isRedeemed() check and redeem() write. find() with PESSIMISTIC_WRITE alone is insufficient because tx_service reuses one EntityManager across all three transactions: the OTP loaded in TX-A stays in the UoW identity map, so find() returns the stale cached instance and only acquires the lock — no refresh. refresh() with PESSIMISTIC_WRITE does both in one round-trip (SELECT ... FOR UPDATE that also re-hydrates), which is what's needed here. Inline OTPs (OAuth2PasswordlessConnectionInline) are non-redeemable by design and are intentionally not locked. Also tighten the sibling-revoke comparison from getValue() (collision- prone if two OTPs ever share a value) to getId().
verifyOTPChallenge previously took the user identity from the OTP claim itself ($otpClaim->getUserName()) and authenticated whichever account that resolved to. That is unsafe as an MFA primitive: an attacker who obtained a valid OTP for account B could submit it during MFA for an in-session user A and the call would succeed — classic second-factor mismatch bypass. Require the caller to pass the in-session User explicitly. After the OTP resolves to its rightful owner, compare ids; on mismatch, throw AuthenticationException BEFORE finalizeRedemption runs so the OTP is NOT consumed. The redeem-attempt counter (committed in TX-A by findAndValidateOTP) still increments, so brute-force tracking is preserved across rejected attempts. The signature change is a breaking interface change on IAuthService, but the method had zero callers in the codebase prior to this commit, so no production code needs updating. Test (tests/VerifyOTPChallengeTest.php) asserts the two behaviours: - Cross-account: OTP for user B + session user A → throws, AND neither refreshExclusiveLock nor getByUserNameNotRedeemed is called (proves finalizeRedemption was not reached, so the OTP was not consumed). - Same-account: OTP for user A + session user A → succeeds, lock is acquired, sibling sweep runs, and the OTP entity is returned.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-130/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/libs/Auth/AuthService.php (1)
465-469:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAlready flagged:
invalidateSession()writes a keyreloadSession()will never read.
Crypt::encryptis non-deterministic (random IV per call), so the ciphertext stored here under<ciphertext>invalidcannot collide with the ciphertext stored bygenerateJTI(Line 704) thatreloadSession(Line 679) probes. The "invalid" marker is dead. Please see the prior detailed review for the suggested deterministic-key fix; the issue is still present in this revision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/libs/Auth/AuthService.php` around lines 465 - 469, invalidateSession() currently stores a non-deterministic ciphertext (Crypt::encrypt(Session::getId())) so the "<ciphertext>invalid" key will never match the ciphertext used by generateJTI()/reloadSession(); replace the non-deterministic encryption with a deterministic, collision-resistant key derivation (e.g. HMAC-SHA256 or SHA256) of the session id and use that same function in invalidateSession(), generateJTI() and reloadSession() so they compute the same cache key; specifically, stop using Crypt::encrypt(Session::getId()) in invalidateSession() and instead compute a deterministic token (e.g. hash_hmac('sha256', Session::getId(), config('app.key')) or hash('sha256', Session::getId())) and call $this->cache_service->addSingleValue($deterministicToken . "invalid", $deterministicToken) so reloadSession() and generateJTI() can probe the same key.
🧹 Nitpick comments (3)
app/libs/Auth/AuthService.php (2)
214-243: 💤 Low valueLGTM — lock+recheck correctly closes the validate→redeem race.
refreshExclusiveLock+isRedeemed()recheck is the right pattern: the second concurrent submitter blocks onSELECT … FOR UPDATE, then resumes and observes the already-redeemed state. Exempting the inline connection is consistent withOAuth2OTP::redeem()short-circuiting for non-redeemable OTPs. Sibling-revoke now compares bygetId()(rather than identity / value), which avoids both the self-revocation case and the false-negative when Doctrine hands back a different proxy for the same row.One small thought: at Line 240
Log::warning($ex)passes the Exception object to a%sconsumer downstream. PreferLog::warning($ex->getMessage(), ['exception' => $ex])so the message is structured and the stack trace lands in context rather than being string-cast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/libs/Auth/AuthService.php` around lines 214 - 243, In finalizeRedemption change the sibling-revoke catch so it doesn't pass the Exception object directly to Log::warning; replace Log::warning($ex) with Log::warning($ex->getMessage(), ['exception' => $ex]) (or equivalent structured logging) so the warning message is the exception message and the full exception is included in context; refer to the finalizeRedemption method and the catch block handling $ex around the sibling revoke loop.
142-199: 💤 Low valueLGTM — clean split of "find + count attempt" from "validate".
The TX-A/TX-B split is the right shape: committing
logRedeemAttempt()independently preserves the brute-force counter even if downstream validation throws. Lifecycle checks read well, and the audience-mismatch condition correctly handles both "OTP requires a client but caller passed none" and "client mismatch".One concurrency clarification for the docblock: TX-B reads
isAlive()/isValid()and the audience under no row lock. If a sibling redemption commits between TX-B and therefreshExclusiveLockinfinalizeRedemption, that's intentionally fine — the lock+isRedeemed()recheck infinalizeRedemptionis what actually closes the validate→redeem window. Worth a one-line note in the docblock so future readers don't try to "fix" TX-B by adding a lock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/libs/Auth/AuthService.php` around lines 142 - 199, Add a one-line clarification to the docblock for findAndValidateOTP explaining that TX-B deliberately performs isAlive()/isValid() and audience checks without taking a row lock; the subsequent finalizeRedemption call (which uses refreshExclusiveLock and rechecks isRedeemed()) is the mechanism that closes the validate→redeem race, so readers should not attempt to add locking to TX-B. Reference TX-A/TX-B, findAndValidateOTP, finalizeRedemption, refreshExclusiveLock, and isRedeemed() in that single-line note.tests/VerifyOTPChallengeTest.php (1)
99-138: 💤 Low valueOptional: assert the brute-force counter promise on the mismatch path.
The class docblock and
verifyOTPChallengePHPDoc both promise thatlogRedeemAttempt(the TX-A brute-force counter) still increments even when the session user mismatches. The test allows it viazeroOrMoreTimes()inbuildValidOTPMockbut doesn't actually assert it ran. Tightening this toonce()(or asserting it explicitly here) would lock down the contract:🧪 Suggested tightening
- $otp->shouldReceive('logRedeemAttempt')->zeroOrMoreTimes(); + $otp->shouldReceive('logRedeemAttempt')->once();…in
buildValidOTPMock, or override per-test in the mismatch case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/VerifyOTPChallengeTest.php` around lines 99 - 138, The test promises that the TX-A brute-force counter logRedeemAttempt still increments on a session-user/OTP-user mismatch but never asserts it; update this test to explicitly expect that call (e.g. add $this->mock_otp_repository->expects($this->once())->method('logRedeemAttempt');) before invoking $this->service->verifyOTPChallenge(...) or change buildValidOTPMock to use ->once() for logRedeemAttempt so the verifyOTPChallenge contract is enforced; reference verifyOTPChallenge, logRedeemAttempt, buildValidOTPMock and the current test method when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/libs/Auth/AuthService.php`:
- Around line 161-165: Replace all logs that emit raw OTP values or plaintext
user emails with redacted identifiers: in findAndValidateOTP,
finalizeRedemption, any revoke logs (revoking otp), loginWithOTP, and
verifyOTPChallenge stop passing $otp_value, $otp->getValue(),
$otp2Revoke->getValue(), $otpClaim->getValue(), and $otpClaim->getUserName()
directly to Log::warning/Log::debug; instead log a stable non-secret identifier
such as $otp->getId() or a short masked form (e.g. "****" + last 2 chars) or a
hash, and use User::getId() or a hashed/obfuscated username instead of the raw
$user_name; factor the masking/hashing into a single helper (e.g.
redactOtpValue()/redactUserId()) and call it from those sites so
Log::warning/Log::debug only receives the safe identifier.
---
Duplicate comments:
In `@app/libs/Auth/AuthService.php`:
- Around line 465-469: invalidateSession() currently stores a non-deterministic
ciphertext (Crypt::encrypt(Session::getId())) so the "<ciphertext>invalid" key
will never match the ciphertext used by generateJTI()/reloadSession(); replace
the non-deterministic encryption with a deterministic, collision-resistant key
derivation (e.g. HMAC-SHA256 or SHA256) of the session id and use that same
function in invalidateSession(), generateJTI() and reloadSession() so they
compute the same cache key; specifically, stop using
Crypt::encrypt(Session::getId()) in invalidateSession() and instead compute a
deterministic token (e.g. hash_hmac('sha256', Session::getId(),
config('app.key')) or hash('sha256', Session::getId())) and call
$this->cache_service->addSingleValue($deterministicToken . "invalid",
$deterministicToken) so reloadSession() and generateJTI() can probe the same
key.
---
Nitpick comments:
In `@app/libs/Auth/AuthService.php`:
- Around line 214-243: In finalizeRedemption change the sibling-revoke catch so
it doesn't pass the Exception object directly to Log::warning; replace
Log::warning($ex) with Log::warning($ex->getMessage(), ['exception' => $ex]) (or
equivalent structured logging) so the warning message is the exception message
and the full exception is included in context; refer to the finalizeRedemption
method and the catch block handling $ex around the sibling revoke loop.
- Around line 142-199: Add a one-line clarification to the docblock for
findAndValidateOTP explaining that TX-B deliberately performs
isAlive()/isValid() and audience checks without taking a row lock; the
subsequent finalizeRedemption call (which uses refreshExclusiveLock and rechecks
isRedeemed()) is the mechanism that closes the validate→redeem race, so readers
should not attempt to add locking to TX-B. Reference TX-A/TX-B,
findAndValidateOTP, finalizeRedemption, refreshExclusiveLock, and isRedeemed()
in that single-line note.
In `@tests/VerifyOTPChallengeTest.php`:
- Around line 99-138: The test promises that the TX-A brute-force counter
logRedeemAttempt still increments on a session-user/OTP-user mismatch but never
asserts it; update this test to explicitly expect that call (e.g. add
$this->mock_otp_repository->expects($this->once())->method('logRedeemAttempt');)
before invoking $this->service->verifyOTPChallenge(...) or change
buildValidOTPMock to use ->once() for logRedeemAttempt so the verifyOTPChallenge
contract is enforced; reference verifyOTPChallenge, logRedeemAttempt,
buildValidOTPMock and the current test method when making the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb171999-4143-47c6-96f3-1a5b9ca44391
📒 Files selected for processing (5)
app/Repositories/DoctrineOAuth2OTPRepository.phpapp/libs/Auth/AuthService.phpapp/libs/OAuth2/Repositories/IOAuth2OTPRepository.phpapp/libs/Utils/Services/IAuthService.phptests/VerifyOTPChallengeTest.php
| Log::warning(sprintf( | ||
| "AuthService::findAndValidateOTP otp %s user %s grant not found", | ||
| $otp_value, | ||
| $user_name | ||
| )); |
There was a problem hiding this comment.
Don't log raw OTP values (and avoid raw user emails at debug).
Several call sites log the cleartext OTP value:
- Line 161-165:
findAndValidateOTPwarning includes$otp_value. - Line 220-223:
finalizeRedemptionwarning includes$otp->getValue(). - Line 236:
Log::debug(... revoking otp %s ..., $otp2Revoke->getValue()). - Line 254:
loginWithOTPdebug includes$otpClaim->getValue()and$otpClaim->getUserName(). - Line 324-328:
verifyOTPChallengedebug includes$otpClaim->getValue().
OTPs are authentication factors and shouldn't end up in log aggregators / archives / SIEM exports. Even though the validity window is short, anyone with log read access during that window can complete an in-flight challenge. Emails additionally fall under PII (GDPR/CCPA) and should not be at debug in plaintext.
Prefer logging the OTP's persisted ID once available, a stable hash, or a masked form (**** + last 2 chars). Replace $user_name with a hashed identifier or the resolved User::getId().
🔒 Suggested redaction helper
+ private static function maskSecret(?string $s): string
+ {
+ if (empty($s)) return '∅';
+ $len = strlen($s);
+ return $len <= 2 ? str_repeat('*', $len) : str_repeat('*', $len - 2) . substr($s, -2);
+ }Then e.g.:
- Log::warning(sprintf(
- "AuthService::findAndValidateOTP otp %s user %s grant not found",
- $otp_value,
- $user_name
- ));
+ Log::warning(sprintf(
+ "AuthService::findAndValidateOTP otp %s grant not found",
+ self::maskSecret($otp_value)
+ ));Also applies to: 220-223, 236-236, 254-254, 324-328
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/libs/Auth/AuthService.php` around lines 161 - 165, Replace all logs that
emit raw OTP values or plaintext user emails with redacted identifiers: in
findAndValidateOTP, finalizeRedemption, any revoke logs (revoking otp),
loginWithOTP, and verifyOTPChallenge stop passing $otp_value, $otp->getValue(),
$otp2Revoke->getValue(), $otpClaim->getValue(), and $otpClaim->getUserName()
directly to Log::warning/Log::debug; instead log a stable non-secret identifier
such as $otp->getId() or a short masked form (e.g. "****" + last 2 chars) or a
hash, and use User::getId() or a hashed/obfuscated username instead of the raw
$user_name; factor the masking/hashing into a single helper (e.g.
redactOtpValue()/redactUserId()) and call it from those sites so
Log::warning/Log::debug only receives the safe identifier.
The $user property added in this PR's earlier commits had only one writer (AuthService::finalizeRedemption) and no readers anywhere in the codebase. Since the field is not ORM-mapped, a future caller that re-loaded the OTP from DB and called getUser() would silently receive null — a footgun for no current benefit. Remove the field, its setUser/getUser accessors, and the now-unused Auth\User import from OAuth2OTP. Drop the matching setUser call site in finalizeRedemption and the unused mock expectation in the unit test. YAGNI: re-add (and bind to a defined consumer) when an actual reader materialises.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-130/ This page is automatically updated on each push to this PR. |
Adds a third unit test that exercises the race-fix path in
finalizeRedemption: after refreshExclusiveLock acquires the lock and
re-hydrates the entity, isRedeemed() returns true (modelling a
concurrent winner having already committed the redemption between TX-B
and TX-C). The test asserts:
- AuthenticationException('Single-use code is already redeemed.') is
thrown.
- redeem() / setAuthTime() / setUserId() are never called on the OTP
(no double-write of the redemption state).
- getByUserNameNotRedeemed() is never queried (sibling-revoke sweep is
skipped, since this caller is not the redeemer).
The stale-then-fresh state is modelled by flipping a by-reference flag
inside refreshExclusiveLock's callback and reading it from isRedeemed,
which mirrors the production semantics (in-memory state was null until
the lock+refresh re-loaded the DB row the winner had updated).
Also pulls isRedeemed() out of the shared buildValidOTPMock helper so
each test states its assumption explicitly (false for the happy path,
true for the race-loser path).
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-130/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Pull request overview
This PR refactors OTP handling in AuthService to (1) split OTP validation into distinct transaction stages and (2) add a new MFA-binding API that verifies an OTP against an already-authenticated session user, ensuring the OTP is only consumed when it belongs to the same user and is still unredeemed after an exclusive row lock refresh.
Changes:
- Added
AuthService::verifyOTPChallenge(...)(andIAuthServicecontract) to support MFA binding without forcing a full re-login. - Refactored OTP processing into
findAndValidateOTP(...)+finalizeRedemption(...), including a pessimistic refresh/lock step to close the validate→redeem race window. - Added unit coverage for session-user vs OTP-user binding, OTP consumption behavior, and the concurrent-redeem loser path.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/VerifyOTPChallengeTest.php |
Adds unit tests validating MFA binding semantics, non-consumption on mismatch, and concurrency loser behavior. |
app/Repositories/DoctrineOAuth2OTPRepository.php |
Implements refreshExclusiveLock(...) using Doctrine refresh(..., PESSIMISTIC_WRITE) for lock+rehydrate. |
app/Models/OAuth2/OAuth2OTP.php |
Minor whitespace cleanup only. |
app/libs/Utils/Services/IAuthService.php |
Extends the auth service interface with verifyOTPChallenge(...) and documents its contract. |
app/libs/OAuth2/Repositories/IOAuth2OTPRepository.php |
Extends OTP repository interface with refreshExclusiveLock(...) to support safe finalization. |
app/libs/Auth/AuthService.php |
Introduces OTP validation/finalization helpers, adds MFA-binding method, and integrates lock+refresh into redemption finalization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ref https://app.clickup.com/t/86b9j5jup
Summary by CodeRabbit
New Features
Improvements
Tests