Skip to content

Commit cad59b3

Browse files
smarcetsmarcet
committed
refactor(auth): drop $should_create_user flag from verifyOTPChallenge
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
1 parent a777e01 commit cad59b3

2 files changed

Lines changed: 125 additions & 122 deletions

File tree

‎app/libs/Auth/AuthService.php‎

Lines changed: 119 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -131,63 +131,24 @@ public function isUserLogged()
131131
}
132132

133133
/**
134-
* @param OAuth2OTP $otpClaim
135-
* @param Client|null $client
136-
* @param bool $remember
137-
* @return OAuth2OTP|null
138-
* @throws Exception
139-
*/
140-
public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP
141-
{
142-
143-
Log::debug(sprintf("AuthService::loginWithOTP otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName()));
144-
145-
$otp = $this->verifyOTPChallenge(
146-
$otpClaim->getValue(),
147-
$otpClaim->getUserName(),
148-
$otpClaim->getConnection(),
149-
true,
150-
$otpClaim->getScope(),
151-
$client
152-
);
153-
154-
$user = $otp->getUser();
155-
156-
if(is_null($user)){
157-
throw new AuthenticationException("Non existent user.");
158-
}
159-
160-
Auth::login($user, $remember);
161-
162-
Log::debug(sprintf("AuthService::verifyOTPChallenge user %s logged in.", $user->getId()));
163-
164-
return $otp;
165-
}
166-
167-
/**
168-
* @param string $otp_value
169-
* @param string $user_name
170-
* @param string $otp_conn
171-
* @param bool $should_create_user
172-
* @param string|null $otp_required_scopes
173-
* @param Client|null $client
174-
* @return OAuth2OTP|null
175-
* @throws Exception
134+
* Finds the OTP by value/connection/username, logs the redeem attempt (TX-A),
135+
* then validates lifecycle / value / scope / audience (TX-B).
136+
* TX-A is committed independently so the brute-force counter increments even when TX-B throws.
137+
*
138+
* @throws AuthenticationException
139+
* @throws InvalidOTPException
176140
*/
177-
public function verifyOTPChallenge
178-
(
141+
private function findAndValidateOTP(
179142
string $otp_value,
180143
string $user_name,
181144
string $otp_conn,
182-
bool $should_create_user = true,
183-
?string $otp_required_scopes = null,
184-
?Client $client = null
185-
): ?OAuth2OTP
145+
?string $otp_required_scopes,
146+
?Client $client
147+
): OAuth2OTP
186148
{
187-
Log::debug(sprintf("AuthService::verifyOTPChallenge otp_value %s user %s", $otp_value, $user_name));
149+
// TX-A: find + log attempt (committed before any validation can throw)
188150
$otp = $this->tx_service->transaction(function () use ($otp_value, $otp_conn, $user_name, $client) {
189151

190-
// find by value, connection and user name
191152
$otp = $this->otp_repository->getByValueConnectionAndUserName(
192153
$otp_value,
193154
$otp_conn,
@@ -196,25 +157,20 @@ public function verifyOTPChallenge
196157
);
197158

198159
if (is_null($otp)) {
199-
// otp no emitted
200-
Log::warning
201-
(
202-
sprintf
203-
(
204-
"AuthService::verifyOTPChallenge otp %s user %s grant not found",
205-
$otp_value,
206-
$user_name
207-
)
208-
);
209-
160+
Log::warning(sprintf(
161+
"AuthService::findAndValidateOTP otp %s user %s grant not found",
162+
$otp_value,
163+
$user_name
164+
));
210165
throw new AuthenticationException("Non existent single-use code.");
211166
}
212167

213168
$otp->logRedeemAttempt();
214169
return $otp;
215170
});
216171

217-
return $this->tx_service->transaction(function () use ($otp, $otp_value, $otp_required_scopes, $user_name, $should_create_user, $client) {
172+
// TX-B: lifecycle / value / scope / audience checks
173+
return $this->tx_service->transaction(function () use ($otp, $otp_value, $otp_required_scopes, $client) {
218174

219175
if (!$otp->isAlive()) {
220176
throw new AuthenticationException("Single-use code is expired.");
@@ -228,77 +184,135 @@ public function verifyOTPChallenge
228184
throw new AuthenticationException("Single-use code mismatch.");
229185
}
230186

231-
if (!empty($otp_required_scopes) && !$otp->allowScope($otp_required_scopes))
187+
if (!empty($otp_required_scopes) && !$otp->allowScope($otp_required_scopes)) {
232188
throw new InvalidOTPException("Single-use code requested scopes escalates former scopes.");
189+
}
233190

234191
if (($otp->hasClient() && is_null($client)) ||
235192
($otp->hasClient() && !is_null($client) && $client->getClientId() != $otp->getClient()->getClientId())) {
236193
throw new AuthenticationException("Single-use code audience mismatch.");
237194
}
238195

196+
return $otp;
197+
});
198+
}
199+
200+
/**
201+
* Marks the OTP redeemed, attaches the user (transient), revokes sibling pending OTPs.
202+
* Entity methods short-circuit for inline OTPs — no special-casing needed here.
203+
*/
204+
private function finalizeRedemption(OAuth2OTP $otp, User $user, ?Client $client): void
205+
{
206+
$otp->setAuthTime(time());
207+
$otp->setUserId($user->getId());
208+
$otp->setUser($user);
209+
$otp->redeem();
210+
211+
$grants2Revoke = $this->otp_repository->getByUserNameNotRedeemed($otp->getUserName(), $client);
212+
foreach ($grants2Revoke as $otp2Revoke) {
213+
try {
214+
Log::debug(sprintf("AuthService::finalizeRedemption revoking otp %s", $otp2Revoke->getValue()));
215+
if ($otp2Revoke->getValue() !== $otp->getValue())
216+
$otp2Revoke->redeem();
217+
} catch (Exception $ex) {
218+
Log::warning($ex);
219+
}
220+
}
221+
}
222+
223+
/**
224+
* @param OAuth2OTP $otpClaim
225+
* @param Client|null $client
226+
* @param bool $remember
227+
* @return OAuth2OTP|null
228+
* @throws Exception
229+
*/
230+
public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP
231+
{
232+
Log::debug(sprintf("AuthService::loginWithOTP otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName()));
233+
234+
$otp = $this->findAndValidateOTP(
235+
$otpClaim->getValue(),
236+
$otpClaim->getUserName(),
237+
$otpClaim->getConnection(),
238+
$otpClaim->getScope(),
239+
$client
240+
);
241+
242+
// TX-C: resolve or create user, finalize, login
243+
return $this->tx_service->transaction(function () use ($otp, $otpClaim, $client, $remember) {
244+
239245
$user = $this->getUserByUsername($otp->getUserName());
240-
$new_user = is_null($user);
241-
if ($new_user) {
242-
243-
if (!$should_create_user) {
244-
Log::warning(sprintf(
245-
"AuthService::verifyOTPChallenge user %s not found and auto-create disabled.",
246-
$otp->getUserName()
247-
));
248-
throw new AuthenticationException("We are sorry, your username or password does not match an existing record.");
249-
}
250-
// we need to create a new one ( auto register)
251-
252-
Log::debug(sprintf("AuthService::verifyOTPChallenge user %s does not exists ...", $otp->getUserName()));
253-
254-
$user = $this->auth_user_service->registerUser
255-
(
246+
247+
if (is_null($user)) {
248+
Log::debug(sprintf("AuthService::loginWithOTP user %s does not exist; auto-registering.", $otp->getUserName()));
249+
$user = $this->auth_user_service->registerUser(
256250
[
257251
'email' => $otp->getUserName(),
258252
'email_verified' => true,
259-
// dont send email
260253
'send_email_verified_notice' => false,
261254
'active' => true,
262255
],
263256
$otp
264257
);
265-
} else {
266-
if ($user->isActive()) {
267-
// verify email
268-
$user->verifyEmail(false);
269-
}
258+
} else if ($user->isActive()) {
259+
$user->verifyEmail(false);
270260
}
271261

272262
if (!$user->canLogin()) {
273-
Log::warning(sprintf("AuthService::verifyOTPChallenge user %s cannot login ( is not active ).", $user->getId()));
263+
Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login (not active).", $user->getId()));
274264
throw new AuthenticationException("We are sorry, your username or password does not match an existing record.");
275265
}
276266

277-
$otp->setAuthTime(time());
278-
$otp->setUserId($user->getId());
279-
$otp->setUser($user);
280-
$otp->redeem();
267+
$this->finalizeRedemption($otp, $user, $client);
281268

282-
// revoke former ones
283-
$grants2Revoke = $this->otp_repository->getByUserNameNotRedeemed
284-
(
285-
$user_name,
286-
$client
287-
);
269+
Auth::login($user, $remember);
270+
Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId()));
271+
return $otp;
272+
});
273+
}
274+
275+
/**
276+
* Verifies an OTP against an EXISTING user. Marks the OTP redeemed.
277+
* Does NOT auto-register users and does NOT call Auth::login.
278+
* Intended as the strict primitive for MFA challenge verification.
279+
*
280+
* @param OAuth2OTP $otpClaim
281+
* @param Client|null $client
282+
* @return OAuth2OTP
283+
* @throws AuthenticationException when the OTP is invalid or the user does not exist / cannot login.
284+
* @throws InvalidOTPException when the OTP requested scopes escalate.
285+
*/
286+
public function verifyOTPChallenge(OAuth2OTP $otpClaim, ?Client $client = null): OAuth2OTP
287+
{
288+
Log::debug(sprintf("AuthService::verifyOTPChallenge otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName()));
289+
290+
$otp = $this->findAndValidateOTP(
291+
$otpClaim->getValue(),
292+
$otpClaim->getUserName(),
293+
$otpClaim->getConnection(),
294+
$otpClaim->getScope(),
295+
$client
296+
);
297+
298+
// TX-C: resolve existing user, finalize
299+
return $this->tx_service->transaction(function () use ($otp, $client) {
288300

289-
foreach ($grants2Revoke as $otp2Revoke) {
290-
try {
291-
Log::debug(sprintf("AuthService::verifyOTPChallenge revoking otp %s ", $otp2Revoke->getValue()));
292-
if ($otp2Revoke->getValue() !== $otp_value)
293-
$otp2Revoke->redeem();
294-
} catch (Exception $ex) {
295-
Log::warning($ex);
296-
}
301+
$user = $this->getUserByUsername($otp->getUserName());
302+
303+
if (is_null($user) || !$user->canLogin()) {
304+
Log::warning(sprintf(
305+
"AuthService::verifyOTPChallenge user %s not found or cannot login.",
306+
$otp->getUserName()
307+
));
308+
throw new AuthenticationException("We are sorry, your username or password does not match an existing record.");
297309
}
298310

311+
$this->finalizeRedemption($otp, $user, $client);
312+
313+
Log::debug(sprintf("AuthService::verifyOTPChallenge user %s verified.", $user->getId()));
299314
return $otp;
300315
});
301-
302316
}
303317

304318
/**

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

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -156,23 +156,12 @@ public function invalidateSession();
156156
public function postLoginUserActions(int $user_id):void;
157157

158158
/**
159-
* @param string $otp_value
160-
* @param string $user_name
161-
* @param string $otp_conn
162-
* @param bool $should_create_user
163-
* @param string|null $otp_required_scopes
159+
* @param OAuth2OTP $otpClaim
164160
* @param Client|null $client
165-
* @return OAuth2OTP|null
166-
* @throws Exception
167-
*/
168-
public function verifyOTPChallenge
169-
(
170-
string $otp_value,
171-
string $user_name,
172-
string $otp_conn,
173-
bool $should_create_user = true,
174-
?string $otp_required_scopes = null,
175-
?Client $client = null
176-
): ?OAuth2OTP;
161+
* @return OAuth2OTP
162+
* @throws AuthenticationException
163+
* @throws \OAuth2\Exceptions\InvalidOTPException
164+
*/
165+
public function verifyOTPChallenge(OAuth2OTP $otpClaim, ?Client $client = null): OAuth2OTP;
177166

178167
}

0 commit comments

Comments
 (0)