chore: Add PR's requested changes and additional AI comments

matiasperrone-exo · matiasperrone-exo · commit c668d639e10f · 2026-05-18T20:56:01.000Z
diff --git a/app/Repositories/DoctrineUserTrustedDeviceRepository.php b/app/Repositories/DoctrineUserTrustedDeviceRepository.php
@@ -14,6 +14,7 @@
 use App\libs\Auth\Models\UserTrustedDevice;
 use Auth\Repositories\IUserTrustedDeviceRepository;
 use Auth\User;
+use Doctrine\Common\Collections\Criteria;
 
 final class DoctrineUserTrustedDeviceRepository
     extends ModelDoctrineRepository implements IUserTrustedDeviceRepository
@@ -23,20 +24,35 @@ protected function getBaseEntity()
         return UserTrustedDevice::class;
     }
 
+    private function buildActiveExpiryExpr(): \Doctrine\Common\Collections\Expr\CompositeExpression
+    {
+        $now = new \DateTime('now', new \DateTimeZone('UTC'));
+        return Criteria::expr()->orX(
+            Criteria::expr()->gt('expires_at', $now),
+            Criteria::expr()->isNull('expires_at')
+        );
+    }
+
     public function getActiveByUserAndIdentifier(User $user, string $deviceIdentifier): ?UserTrustedDevice
     {
-        return $this->findOneBy([
-            'user'              => $user,
-            'device_identifier' => $deviceIdentifier,
-            'is_revoked'        => false,
-        ]);
+        $criteria = Criteria::create()
+            ->where(Criteria::expr()->eq('user', $user))
+            ->andWhere(Criteria::expr()->eq('device_identifier', $deviceIdentifier))
+            ->andWhere(Criteria::expr()->eq('is_revoked', false))
+            ->andWhere($this->buildActiveExpiryExpr())
+            ->setMaxResults(1);
+
+        $result = $this->matching($criteria)->first();
+        return $result instanceof UserTrustedDevice ? $result : null;
     }
 
     public function getActiveByUser(User $user): array
     {
-        return $this->findBy([
-            'user'       => $user,
-            'is_revoked' => false,
-        ]);
+        $criteria = Criteria::create()
+            ->where(Criteria::expr()->eq('user', $user))
+            ->andWhere(Criteria::expr()->eq('is_revoked', false))
+            ->andWhere($this->buildActiveExpiryExpr());
+
+        return $this->matching($criteria)->toArray();
     }
 }
diff --git a/app/libs/Auth/Models/UserRecoveryCode.php b/app/libs/Auth/Models/UserRecoveryCode.php
@@ -14,9 +14,10 @@
 
 use Auth\User;
 use Doctrine\ORM\Mapping AS ORM;
+use App\Repositories\DoctrineUserRecoveryCodeRepository;
 
 #[ORM\Table(name: 'user_recovery_codes')]
-#[ORM\Entity(repositoryClass: \App\Repositories\DoctrineUserRecoveryCodeRepository::class)]
+#[ORM\Entity(repositoryClass: DoctrineUserRecoveryCodeRepository::class)]
 class UserRecoveryCode
 {
     #[ORM\Id]
@@ -25,7 +26,7 @@ class UserRecoveryCode
     protected $id;
 
     #[ORM\JoinColumn(name: 'user_id', referencedColumnName: 'id', onDelete: 'CASCADE')]
-    #[ORM\ManyToOne(targetEntity: \Auth\User::class)]
+    #[ORM\ManyToOne(targetEntity: User::class)]
     private $user;
 
     #[ORM\Column(name: 'code_hash', type: 'string', length: 255)]
@@ -63,5 +64,4 @@ public function markUsed(): void
         $this->used_at = new \DateTime('now', new \DateTimeZone('UTC'));
     }
 
-    public function __get($name) { return $this->{$name}; }
 }
diff --git a/app/libs/Auth/Models/UserTrustedDevice.php b/app/libs/Auth/Models/UserTrustedDevice.php
@@ -81,6 +81,4 @@ public function setLastSeenAt(\DateTime $value): void { $this->last_seen_at = $v
 
     public function isRevoked(): bool { return (bool) $this->is_revoked; }
     public function setIsRevoked(bool $value): void { $this->is_revoked = $value; }
-
-    public function __get($name) { return $this->{$name}; }
 }
diff --git a/database/migrations/Version20260424120000.php b/database/migrations/Version20260424120000.php
@@ -0,0 +1,44 @@
+<?php namespace Database\Migrations;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+use Doctrine\Migrations\AbstractMigration;
+use Doctrine\DBAL\Schema\Schema as Schema;
+
+/**
+ * Class Version20260424120000
+ * @package Database\Migrations
+ *
+ * Enforce uniqueness of (user_id, device_identifier) on user_trusted_devices.
+ * Replaces the plain index utd_user_device_idx with a unique one so that a
+ * given user can never accumulate duplicate device rows.
+ */
+final class Version20260424120000 extends AbstractMigration
+{
+    public function up(Schema $schema): void
+    {
+        $this->addSql(
+            'ALTER TABLE user_trusted_devices
+             DROP INDEX utd_user_device_idx,
+             ADD  UNIQUE INDEX utd_user_device_uniq (user_id, device_identifier)'
+        );
+    }
+
+    public function down(Schema $schema): void
+    {
+        $this->addSql(
+            'ALTER TABLE user_trusted_devices
+             DROP INDEX utd_user_device_uniq,
+             ADD  INDEX utd_user_device_idx (user_id, device_identifier)'
+        );
+    }
+}
diff --git a/tests/TwoFactorRepositoriesTest.php b/tests/TwoFactorRepositoriesTest.php
@@ -17,7 +17,6 @@
 use Auth\Repositories\ITwoFactorAuditLogRepository;
 use Auth\Repositories\IUserRecoveryCodeRepository;
 use Auth\Repositories\IUserTrustedDeviceRepository;
-use Auth\Repositories\IUserRepository;
 use Auth\User;
 use Illuminate\Support\Facades\App;
 use LaravelDoctrine\ORM\Facades\EntityManager;
@@ -33,12 +32,32 @@ class TwoFactorRepositoriesTest extends TestCase
     public function setUp(): void
     {
         parent::setUp();
-        // Pull any persisted user; tests don't assert on user fields, only on FK linkage
-        $userRepo = App::make(IUserRepository::class);
-        $this->user = $userRepo->findOneBy([]);
-        if (is_null($this->user)) {
-            $this->markTestSkipped('No User exists; database must be seeded.');
+        $user = new User();
+        $user->setFirstName('Test');
+        $user->setLastName('TwoFactor');
+        $user->setEmail('test.twofactor.' . uniqid() . '@test.invalid');
+        $user->setAddress1('123 Test St');
+        $user->setState('CA');
+        $user->setCity('Testville');
+        $user->setPostCode('00000');
+        $user->setCountryIsoCode('US');
+        $user->setPic('');
+        $user->setLastLoginDate(new \DateTime('now', new \DateTimeZone('UTC')));
+        EntityManager::persist($user);
+        EntityManager::flush();
+        $this->user = $user;
+    }
+
+    public function tearDown(): void
+    {
+        if ($this->user !== null) {
+            $managed = EntityManager::find(User::class, $this->user->getId());
+            if ($managed !== null) {
+                EntityManager::remove($managed);
+                EntityManager::flush();
+            }
         }
+        parent::tearDown();
     }
 
     public function testTrustedDeviceRoundTrip(): void
@@ -136,6 +155,193 @@ public function testRecoveryCodeRoundTrip(): void
         $this->assertTrue($reload->isUsed());
 
         $deleted = $repo->deleteAllForUser($this->user);
-        $this->assertGreaterThanOrEqual(1, $deleted);
+        $this->assertEquals(1, $deleted);
+    }
+
+    // -------------------------------------------------------------------------
+    // Targeted behaviour tests
+    // -------------------------------------------------------------------------
+
+    public function testExpiredTrustedDeviceIsExcluded(): void
+    {
+        $repo     = App::make(IUserTrustedDeviceRepository::class);
+        $now      = new \DateTime('now', new \DateTimeZone('UTC'));
+        $expired  = (clone $now)->modify('-1 minute');
+        $deviceId = hash('sha256', 'expired-device-' . uniqid());
+
+        $device = $this->buildDevice($deviceId, $now, $expired);
+        EntityManager::persist($device);
+        EntityManager::flush();
+        $id = $device->getId();
+        EntityManager::clear();
+
+        $this->assertNull(
+            $repo->getActiveByUserAndIdentifier($this->user, $deviceId),
+            'getActiveByUserAndIdentifier must return null for an expired device.'
+        );
+
+        $ids = array_map(
+            fn(UserTrustedDevice $d) => $d->getDeviceIdentifier(),
+            $repo->getActiveByUser($this->user)
+        );
+        $this->assertNotContains($deviceId, $ids, 'getActiveByUser must not include expired devices.');
+
+        $stale = EntityManager::find(UserTrustedDevice::class, $id);
+        if ($stale) { EntityManager::remove($stale); EntityManager::flush(); }
+    }
+
+    public function testRevokedTrustedDeviceIsExcluded(): void
+    {
+        $repo     = App::make(IUserTrustedDeviceRepository::class);
+        $now      = new \DateTime('now', new \DateTimeZone('UTC'));
+        $expires  = (clone $now)->modify('+30 days');
+        $deviceId = hash('sha256', 'revoked-device-' . uniqid());
+
+        $device = $this->buildDevice($deviceId, $now, $expires);
+        $device->setIsRevoked(true);
+        EntityManager::persist($device);
+        EntityManager::flush();
+        $id = $device->getId();
+        EntityManager::clear();
+
+        $this->assertNull(
+            $repo->getActiveByUserAndIdentifier($this->user, $deviceId),
+            'getActiveByUserAndIdentifier must return null for a revoked device.'
+        );
+
+        $ids = array_map(
+            fn(UserTrustedDevice $d) => $d->getDeviceIdentifier(),
+            $repo->getActiveByUser($this->user)
+        );
+        $this->assertNotContains($deviceId, $ids, 'getActiveByUser must not include revoked devices.');
+
+        $stale = EntityManager::find(UserTrustedDevice::class, $id);
+        if ($stale) { EntityManager::remove($stale); EntityManager::flush(); }
+    }
+
+    public function testDuplicateDeviceIdentifierCannotOccur(): void
+    {
+        $connection = EntityManager::getConnection();
+        $indexes    = $connection->createSchemaManager()->listTableIndexes('user_trusted_devices');
+
+        $hasUnique = false;
+        foreach ($indexes as $index) {
+            if ($index->isUnique()) {
+                $cols = $index->getColumns();
+                if (in_array('user_id', $cols) && in_array('device_identifier', $cols)) {
+                    $hasUnique = true;
+                    break;
+                }
+            }
+        }
+
+        $this->assertTrue(
+            $hasUnique,
+            'user_trusted_devices must have a UNIQUE index on (user_id, device_identifier).'
+        );
+    }
+
+    public function testRecoveryCodeDeletionRemovesUsedAndUnusedCodes(): void
+    {
+        $repo = App::make(IUserRecoveryCodeRepository::class);
+
+        $unused = new UserRecoveryCode();
+        $unused->setUser($this->user);
+        $unused->setCodeHash(password_hash('UNUSED_' . uniqid(), PASSWORD_BCRYPT));
+
+        $used = new UserRecoveryCode();
+        $used->setUser($this->user);
+        $used->setCodeHash(password_hash('USED_' . uniqid(), PASSWORD_BCRYPT));
+        $used->markUsed();
+
+        EntityManager::persist($unused);
+        EntityManager::persist($used);
+        EntityManager::flush();
+        $unusedId = $unused->getId();
+        $usedId   = $used->getId();
+
+        $deleted = $repo->deleteAllForUser($this->user);
+        $this->assertGreaterThanOrEqual(2, $deleted, 'deleteAllForUser must remove both used and unused codes.');
+
+        EntityManager::clear();
+        $this->assertNull(
+            EntityManager::find(UserRecoveryCode::class, $unusedId),
+            'Unused recovery code must be deleted.'
+        );
+        $this->assertNull(
+            EntityManager::find(UserRecoveryCode::class, $usedId),
+            'Used recovery code must also be deleted.'
+        );
+    }
+
+    public function testAuditLogsReturnedMostRecentFirst(): void
+    {
+        $repo       = App::make(ITwoFactorAuditLogRepository::class);
+        $createdIds = [];
+
+        $timestamps = [
+            new \DateTime('2020-01-01 01:00:00', new \DateTimeZone('UTC')),
+            new \DateTime('2020-01-01 02:00:00', new \DateTimeZone('UTC')),
+            new \DateTime('2020-01-01 03:00:00', new \DateTimeZone('UTC')),
+        ];
+
+        $setCreatedAt = static function (TwoFactorAuditLog $log, \DateTime $dt): void {
+            $prop = new \ReflectionProperty(TwoFactorAuditLog::class, 'created_at');
+            $prop->setAccessible(true);
+            $prop->setValue($log, $dt);
+        };
+
+        foreach ($timestamps as $ts) {
+            $entry = new TwoFactorAuditLog();
+            $entry->setUser($this->user);
+            $entry->setEventType(TwoFactorAuditLog::EventChallengeIssued);
+            $entry->setMethod(TwoFactorAuditLog::MethodEmailOtp);
+            $entry->setIpAddress('127.0.0.1');
+            $entry->setUserAgent('Mozilla/5.0 (test)');
+            $setCreatedAt($entry, $ts);
+            EntityManager::persist($entry);
+            EntityManager::flush();
+            $createdIds[] = $entry->getId();
+        }
+
+        EntityManager::clear();
+
+        $all  = $repo->getRecentByUser($this->user, 200);
+        $ours = array_values(array_filter($all, fn(TwoFactorAuditLog $e) => in_array($e->getId(), $createdIds)));
+
+        $this->assertCount(3, $ours, 'All three seeded audit entries must be returned.');
+
+        for ($i = 0; $i < count($ours) - 1; $i++) {
+            $this->assertGreaterThanOrEqual(
+                $ours[$i + 1]->getCreatedAt()->getTimestamp(),
+                $ours[$i]->getCreatedAt()->getTimestamp(),
+                'Audit logs must be ordered most-recent first.'
+            );
+        }
+
+        // cleanup
+        foreach ($createdIds as $logId) {
+            $log = EntityManager::find(TwoFactorAuditLog::class, $logId);
+            if ($log) { EntityManager::remove($log); }
+        }
+        EntityManager::flush();
+    }
+
+    // -------------------------------------------------------------------------
+    // Helpers
+    // -------------------------------------------------------------------------
+
+    private function buildDevice(string $deviceId, \DateTime $now, \DateTime $expires): UserTrustedDevice
+    {
+        $device = new UserTrustedDevice();
+        $device->setUser($this->user);
+        $device->setDeviceIdentifier($deviceId);
+        $device->setDeviceName('Test Browser');
+        $device->setIpAddress('127.0.0.1');
+        $device->setUserAgent('Mozilla/5.0 (test)');
+        $device->setTrustedAt($now);
+        $device->setExpiresAt($expires);
+        $device->setLastSeenAt($now);
+        return $device;
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -81,6 +81,4 @@ public function setLastSeenAt(\DateTime $value): void { $this->last_seen_at = $v`
`81`	`81`
`82`	`82`	`public function isRevoked(): bool { return (bool) $this->is_revoked; }`
`83`	`83`	`public function setIsRevoked(bool $value): void { $this->is_revoked = $value; }`
`84`		`-`
`85`		`- public function __get($name) { return $this->{$name}; }`
`86`	`84`	`}`