Skip to content

Commit c44ed61

Browse files
committed
fix: remove nested transaction in enableTwoFactorAndGenerateCodes
enableTwoFactorAndGenerateCodes() wrapped enable2FA()/user persist in one transaction() call while also calling generateRecoveryCodes(), which opens its own. DoctrineTransactionService::transaction() closes the entity manager and connection on failure, so an inner failure could tear down the EM out from under the still-running outer transaction. Extracted the shared code-generation logic into a transaction-free regenerateCodesForUser(), so each public method now opens exactly one transaction.
1 parent 1e688cb commit c44ed61

1 file changed

Lines changed: 44 additions & 20 deletions

File tree

app/Services/Auth/RecoveryCodeService.php

Lines changed: 44 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -57,24 +57,7 @@ public function regenerateRecoveryCodes(User $user, string $currentPassword): ar
5757
*/
5858
public function generateRecoveryCodes(User $user): array
5959
{
60-
$count = (int)config('auth.recovery_codes.count', 10);
61-
$length = (int)config('auth.recovery_codes.length', 8);
62-
63-
$plaintext_codes = [];
64-
65-
$this->tx_service->transaction(function () use ($user, $count, $length, &$plaintext_codes) {
66-
$this->repository->deleteAllForUser($user);
67-
68-
for ($i = 0; $i < $count; $i++) {
69-
$plain = Rand::getString($length, self::CODE_CHARSET, true);
70-
$plaintext_codes[] = $plain;
71-
72-
$code = new UserRecoveryCode();
73-
$code->setUser($user);
74-
$code->setCodeHash(Hash::make($plain));
75-
$this->repository->add($code, false);
76-
}
77-
});
60+
$codes = $this->tx_service->transaction(fn() => $this->regenerateCodesForUser($user));
7861

7962
$this->audit_service->log(
8063
$user,
@@ -83,21 +66,34 @@ public function generateRecoveryCodes(User $user): array
8366
IPHelper::getUserIp()
8467
);
8568

86-
return array_map(static fn(string $code) => implode('-', str_split($code, 4)), $plaintext_codes);
69+
return $codes;
8770
}
8871

8972
/**
9073
* @inheritDoc
9174
*/
9275
public function enableTwoFactorAndGenerateCodes(User $user, string $method): array
9376
{
77+
// Everything must live in a single transaction() call: it opens/commits
78+
// its own connection-level transaction and closes the entity manager on
79+
// failure (see DoctrineTransactionService::transaction()), so nesting a
80+
// second call inside it (e.g. by calling generateRecoveryCodes() here)
81+
// would let an inner failure tear down the EM out from under this
82+
// still-running outer transaction.
9483
$codes = $this->tx_service->transaction(function () use ($user, $method) {
9584
$user->enable2FA($method);
9685
$this->user_repository->add($user, false);
9786

98-
return $this->generateRecoveryCodes($user);
87+
return $this->regenerateCodesForUser($user);
9988
});
10089

90+
$this->audit_service->log(
91+
$user,
92+
TwoFactorAuditLog::EventRecoveryCodesGenerated,
93+
TwoFactorAuditLog::MethodRecovery,
94+
IPHelper::getUserIp()
95+
);
96+
10197
$this->audit_service->log(
10298
$user,
10399
TwoFactorAuditLog::EventEnrollmentChanged,
@@ -108,6 +104,34 @@ public function enableTwoFactorAndGenerateCodes(User $user, string $method): arr
108104
return $codes;
109105
}
110106

107+
/**
108+
* Invalidates every existing recovery code for the user and generates a
109+
* fresh batch, within the caller's already-open transaction.
110+
*
111+
* @return string[] plaintext codes formatted as XXXX-XXXX
112+
*/
113+
private function regenerateCodesForUser(User $user): array
114+
{
115+
$count = (int)config('auth.recovery_codes.count', 10);
116+
$length = (int)config('auth.recovery_codes.length', 8);
117+
118+
$plaintext_codes = [];
119+
120+
$this->repository->deleteAllForUser($user);
121+
122+
for ($i = 0; $i < $count; $i++) {
123+
$plain = Rand::getString($length, self::CODE_CHARSET, true);
124+
$plaintext_codes[] = $plain;
125+
126+
$code = new UserRecoveryCode();
127+
$code->setUser($user);
128+
$code->setCodeHash(Hash::make($plain));
129+
$this->repository->add($code, false);
130+
}
131+
132+
return array_map(static fn(string $code) => implode('-', str_split($code, 4)), $plaintext_codes);
133+
}
134+
111135
/**
112136
* @inheritDoc
113137
*/

0 commit comments

Comments
 (0)