Skip to content

Commit f75c63d

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 f75c63d

5 files changed

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

0 commit comments

Comments
 (0)