Skip to content

Commit bd6e93f

Browse files
matiasperrone-exoCopilot
matiasperrone-exo
and
Copilot
committed
chore: Add PR's requested changes and additional AI comments
Co-authored-by: Copilot <copilot@github.com>
1 parent 6a2bd34 commit bd6e93f

5 files changed

Lines changed: 259 additions & 14 deletions

File tree

‎app/Repositories/DoctrineUserTrustedDeviceRepository.php‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use App\libs\Auth\Models\UserTrustedDevice;
1515
use Auth\Repositories\IUserTrustedDeviceRepository;
1616
use Auth\User;
17+
use Doctrine\Common\Collections\Criteria;
1718

1819
final class DoctrineUserTrustedDeviceRepository
1920
extends ModelDoctrineRepository implements IUserTrustedDeviceRepository
@@ -23,20 +24,35 @@ protected function getBaseEntity()
2324
return UserTrustedDevice::class;
2425
}
2526

27+
private function buildActiveExpiryExpr(): \Doctrine\Common\Collections\Expr\CompositeExpression
28+
{
29+
$now = new \DateTime('now', new \DateTimeZone('UTC'));
30+
return Criteria::expr()->orX(
31+
Criteria::expr()->gt('expires_at', $now),
32+
Criteria::expr()->isNull('expires_at')
33+
);
34+
}
35+
2636
public function getActiveByUserAndIdentifier(User $user, string $deviceIdentifier): ?UserTrustedDevice
2737
{
28-
return $this->findOneBy([
29-
'user' => $user,
30-
'device_identifier' => $deviceIdentifier,
31-
'is_revoked' => false,
32-
]);
38+
$criteria = Criteria::create()
39+
->where(Criteria::expr()->eq('user', $user))
40+
->andWhere(Criteria::expr()->eq('device_identifier', $deviceIdentifier))
41+
->andWhere(Criteria::expr()->eq('is_revoked', false))
42+
->andWhere($this->buildActiveExpiryExpr())
43+
->setMaxResults(1);
44+
45+
$result = $this->matching($criteria)->first();
46+
return $result instanceof UserTrustedDevice ? $result : null;
3347
}
3448

3549
public function getActiveByUser(User $user): array
3650
{
37-
return $this->findBy([
38-
'user' => $user,
39-
'is_revoked' => false,
40-
]);
51+
$criteria = Criteria::create()
52+
->where(Criteria::expr()->eq('user', $user))
53+
->andWhere(Criteria::expr()->eq('is_revoked', false))
54+
->andWhere($this->buildActiveExpiryExpr());
55+
56+
return $this->matching($criteria)->toArray();
4157
}
4258
}

‎app/libs/Auth/Models/UserRecoveryCode.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,10 @@
1414

1515
use Auth\User;
1616
use Doctrine\ORM\Mapping AS ORM;
17+
use App\Repositories\DoctrineUserRecoveryCodeRepository;
1718

1819
#[ORM\Table(name: 'user_recovery_codes')]
19-
#[ORM\Entity(repositoryClass: \App\Repositories\DoctrineUserRecoveryCodeRepository::class)]
20+
#[ORM\Entity(repositoryClass: DoctrineUserRecoveryCodeRepository::class)]
2021
class UserRecoveryCode
2122
{
2223
#[ORM\Id]
@@ -25,7 +26,7 @@ class UserRecoveryCode
2526
protected $id;
2627

2728
#[ORM\JoinColumn(name: 'user_id', referencedColumnName: 'id', onDelete: 'CASCADE')]
28-
#[ORM\ManyToOne(targetEntity: \Auth\User::class)]
29+
#[ORM\ManyToOne(targetEntity: User::class)]
2930
private $user;
3031

3132
#[ORM\Column(name: 'code_hash', type: 'string', length: 255)]
@@ -63,5 +64,4 @@ public function markUsed(): void
6364
$this->used_at = new \DateTime('now', new \DateTimeZone('UTC'));
6465
}
6566

66-
public function __get($name) { return $this->{$name}; }
6767
}

‎app/libs/Auth/Models/UserTrustedDevice.php‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,4 @@ public function setLastSeenAt(\DateTime $value): void { $this->last_seen_at = $v
8181

8282
public function isRevoked(): bool { return (bool) $this->is_revoked; }
8383
public function setIsRevoked(bool $value): void { $this->is_revoked = $value; }
84-
85-
public function __get($name) { return $this->{$name}; }
8684
}

‎database/migrations/Version20260424120000.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<?php namespace Database\Migrations;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
use Doctrine\Migrations\AbstractMigration;
15+
use Doctrine\DBAL\Schema\Schema as Schema;
16+
17+
/**
18+
* Class Version20260424120000
19+
* @package Database\Migrations
20+
*
21+
* Enforce uniqueness of (user_id, device_identifier) on user_trusted_devices.
22+
* Replaces the plain index utd_user_device_idx with a unique one so that a
23+
* given user can never accumulate duplicate device rows.
24+
*/
25+
final class Version20260424120000 extends AbstractMigration
26+
{
27+
public function up(Schema $schema): void
28+
{
29+
$this->addSql(
30+
'ALTER TABLE user_trusted_devices
31+
DROP INDEX utd_user_device_idx,
32+
ADD UNIQUE INDEX utd_user_device_uniq (user_id, device_identifier)'
33+
);
34+
}
35+
36+
public function down(Schema $schema): void
37+
{
38+
$this->addSql(
39+
'ALTER TABLE user_trusted_devices
40+
DROP INDEX utd_user_device_uniq,
41+
ADD INDEX utd_user_device_idx (user_id, device_identifier)'
42+
);
43+
}
44+
}

‎tests/TwoFactorRepositoriesTest.php‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,4 +138,191 @@ public function testRecoveryCodeRoundTrip(): void
138138
$deleted = $repo->deleteAllForUser($this->user);
139139
$this->assertGreaterThanOrEqual(1, $deleted);
140140
}
141+
142+
// -------------------------------------------------------------------------
143+
// Targeted behaviour tests
144+
// -------------------------------------------------------------------------
145+
146+
public function testExpiredTrustedDeviceIsExcluded(): void
147+
{
148+
$repo = App::make(IUserTrustedDeviceRepository::class);
149+
$now = new \DateTime('now', new \DateTimeZone('UTC'));
150+
$expired = (clone $now)->modify('-1 minute');
151+
$deviceId = hash('sha256', 'expired-device-' . uniqid());
152+
153+
$device = $this->buildDevice($deviceId, $now, $expired);
154+
EntityManager::persist($device);
155+
EntityManager::flush();
156+
$id = $device->getId();
157+
EntityManager::clear();
158+
159+
$this->assertNull(
160+
$repo->getActiveByUserAndIdentifier($this->user, $deviceId),
161+
'getActiveByUserAndIdentifier must return null for an expired device.'
162+
);
163+
164+
$ids = array_map(
165+
fn(UserTrustedDevice $d) => $d->getDeviceIdentifier(),
166+
$repo->getActiveByUser($this->user)
167+
);
168+
$this->assertNotContains($deviceId, $ids, 'getActiveByUser must not include expired devices.');
169+
170+
$stale = EntityManager::find(UserTrustedDevice::class, $id);
171+
if ($stale) { EntityManager::remove($stale); EntityManager::flush(); }
172+
}
173+
174+
public function testRevokedTrustedDeviceIsExcluded(): void
175+
{
176+
$repo = App::make(IUserTrustedDeviceRepository::class);
177+
$now = new \DateTime('now', new \DateTimeZone('UTC'));
178+
$expires = (clone $now)->modify('+30 days');
179+
$deviceId = hash('sha256', 'revoked-device-' . uniqid());
180+
181+
$device = $this->buildDevice($deviceId, $now, $expires);
182+
$device->setIsRevoked(true);
183+
EntityManager::persist($device);
184+
EntityManager::flush();
185+
$id = $device->getId();
186+
EntityManager::clear();
187+
188+
$this->assertNull(
189+
$repo->getActiveByUserAndIdentifier($this->user, $deviceId),
190+
'getActiveByUserAndIdentifier must return null for a revoked device.'
191+
);
192+
193+
$ids = array_map(
194+
fn(UserTrustedDevice $d) => $d->getDeviceIdentifier(),
195+
$repo->getActiveByUser($this->user)
196+
);
197+
$this->assertNotContains($deviceId, $ids, 'getActiveByUser must not include revoked devices.');
198+
199+
$stale = EntityManager::find(UserTrustedDevice::class, $id);
200+
if ($stale) { EntityManager::remove($stale); EntityManager::flush(); }
201+
}
202+
203+
public function testDuplicateDeviceIdentifierCannotOccur(): void
204+
{
205+
$connection = EntityManager::getConnection();
206+
$indexes = $connection->createSchemaManager()->listTableIndexes('user_trusted_devices');
207+
208+
$hasUnique = false;
209+
foreach ($indexes as $index) {
210+
if ($index->isUnique()) {
211+
$cols = $index->getColumns();
212+
if (in_array('user_id', $cols) && in_array('device_identifier', $cols)) {
213+
$hasUnique = true;
214+
break;
215+
}
216+
}
217+
}
218+
219+
$this->assertTrue(
220+
$hasUnique,
221+
'user_trusted_devices must have a UNIQUE index on (user_id, device_identifier).'
222+
);
223+
}
224+
225+
public function testRecoveryCodeDeletionRemovesUsedAndUnusedCodes(): void
226+
{
227+
$repo = App::make(IUserRecoveryCodeRepository::class);
228+
229+
$unused = new UserRecoveryCode();
230+
$unused->setUser($this->user);
231+
$unused->setCodeHash(password_hash('UNUSED_' . uniqid(), PASSWORD_BCRYPT));
232+
233+
$used = new UserRecoveryCode();
234+
$used->setUser($this->user);
235+
$used->setCodeHash(password_hash('USED_' . uniqid(), PASSWORD_BCRYPT));
236+
$used->markUsed();
237+
238+
EntityManager::persist($unused);
239+
EntityManager::persist($used);
240+
EntityManager::flush();
241+
$unusedId = $unused->getId();
242+
$usedId = $used->getId();
243+
244+
$deleted = $repo->deleteAllForUser($this->user);
245+
$this->assertGreaterThanOrEqual(2, $deleted, 'deleteAllForUser must remove both used and unused codes.');
246+
247+
EntityManager::clear();
248+
$this->assertNull(
249+
EntityManager::find(UserRecoveryCode::class, $unusedId),
250+
'Unused recovery code must be deleted.'
251+
);
252+
$this->assertNull(
253+
EntityManager::find(UserRecoveryCode::class, $usedId),
254+
'Used recovery code must also be deleted.'
255+
);
256+
}
257+
258+
public function testAuditLogsReturnedMostRecentFirst(): void
259+
{
260+
$repo = App::make(ITwoFactorAuditLogRepository::class);
261+
$createdIds = [];
262+
263+
$timestamps = [
264+
new \DateTime('2020-01-01 01:00:00', new \DateTimeZone('UTC')),
265+
new \DateTime('2020-01-01 02:00:00', new \DateTimeZone('UTC')),
266+
new \DateTime('2020-01-01 03:00:00', new \DateTimeZone('UTC')),
267+
];
268+
269+
$setCreatedAt = static function (TwoFactorAuditLog $log, \DateTime $dt): void {
270+
$prop = new \ReflectionProperty(TwoFactorAuditLog::class, 'created_at');
271+
$prop->setAccessible(true);
272+
$prop->setValue($log, $dt);
273+
};
274+
275+
foreach ($timestamps as $ts) {
276+
$entry = new TwoFactorAuditLog();
277+
$entry->setUser($this->user);
278+
$entry->setEventType(TwoFactorAuditLog::EventChallengeIssued);
279+
$entry->setMethod(TwoFactorAuditLog::MethodEmailOtp);
280+
$entry->setIpAddress('127.0.0.1');
281+
$entry->setUserAgent('Mozilla/5.0 (test)');
282+
$setCreatedAt($entry, $ts);
283+
EntityManager::persist($entry);
284+
EntityManager::flush();
285+
$createdIds[] = $entry->getId();
286+
}
287+
288+
EntityManager::clear();
289+
290+
$all = $repo->getRecentByUser($this->user, 200);
291+
$ours = array_values(array_filter($all, fn(TwoFactorAuditLog $e) => in_array($e->getId(), $createdIds)));
292+
293+
$this->assertCount(3, $ours, 'All three seeded audit entries must be returned.');
294+
295+
for ($i = 0; $i < count($ours) - 1; $i++) {
296+
$this->assertGreaterThanOrEqual(
297+
$ours[$i + 1]->getCreatedAt()->getTimestamp(),
298+
$ours[$i]->getCreatedAt()->getTimestamp(),
299+
'Audit logs must be ordered most-recent first.'
300+
);
301+
}
302+
303+
// cleanup
304+
foreach ($createdIds as $logId) {
305+
$log = EntityManager::find(TwoFactorAuditLog::class, $logId);
306+
if ($log) { EntityManager::remove($log); }
307+
}
308+
EntityManager::flush();
309+
}
310+
311+
// -------------------------------------------------------------------------
312+
// Helpers
313+
// -------------------------------------------------------------------------
314+
315+
private function buildDevice(string $deviceId, \DateTime $now, \DateTime $expires): UserTrustedDevice
316+
{
317+
$device = new UserTrustedDevice();
318+
$device->setUser($this->user);
319+
$device->setDeviceIdentifier($deviceId);
320+
$device->setDeviceName('Test Browser');
321+
$device->setIpAddress('127.0.0.1');
322+
$device->setUserAgent('Mozilla/5.0 (test)');
323+
$device->setTrustedAt($now);
324+
$device->setExpiresAt($expires);
325+
$device->setLastSeenAt($now);
326+
return $device;
327+
}
141328
}

0 commit comments

Comments
 (0)