Skip to content

Commit 500246f

Browse files
matiasperrone-exomatiasperrone-exo
committed
chore: Add PR's requested changes and Unit test
1 parent f406fbf commit 500246f

4 files changed

Lines changed: 113 additions & 31 deletions

File tree

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ public function getUser(): User
9797
{
9898
return $this->user;
9999
}
100+
100101
public function setUser(User $user): void
101102
{
102103
$this->user = $user;
@@ -106,6 +107,7 @@ public function getEventType(): string
106107
{
107108
return $this->event_type;
108109
}
110+
109111
public function setEventType(string $value): void
110112
{
111113
if (!in_array($value, self::ALLOWED_EVENT_TYPES, true)) {
@@ -118,6 +120,7 @@ public function getMethod(): string
118120
{
119121
return $this->method;
120122
}
123+
121124
public function setMethod(string $value): void
122125
{
123126
if (!in_array($value, self::ALLOWED_METHODS, true)) {
@@ -130,6 +133,7 @@ public function getIpAddress(): string
130133
{
131134
return $this->ip_address;
132135
}
136+
133137
public function setIpAddress(string $value): void
134138
{
135139
$this->ip_address = $value;
@@ -139,6 +143,7 @@ public function getUserAgent(): string
139143
{
140144
return $this->user_agent;
141145
}
146+
142147
public function setUserAgent(string $value): void
143148
{
144149
$this->user_agent = $value;
@@ -148,6 +153,7 @@ public function getMetadata(): ?array
148153
{
149154
return $this->metadata;
150155
}
156+
151157
public function setMetadata(?array $value): void
152158
{
153159
$this->metadata = $value;

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

Lines changed: 34 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
<?php namespace App\libs\Auth\Models;
1+
<?php
2+
namespace App\libs\Auth\Models;
23
/**
34
* Copyright 2026 OpenStack Foundation
45
* Licensed under the Apache License, Version 2.0 (the "License");
@@ -12,19 +13,16 @@
1213
* limitations under the License.
1314
**/
1415

16+
use App\Models\Utils\BaseEntity;
1517
use Auth\User;
16-
use Doctrine\ORM\Mapping AS ORM;
18+
use Doctrine\ORM\Mapping as ORM;
1719
use App\Repositories\DoctrineUserRecoveryCodeRepository;
20+
use models\exceptions\ValidationException;
1821

1922
#[ORM\Table(name: 'user_recovery_codes')]
2023
#[ORM\Entity(repositoryClass: DoctrineUserRecoveryCodeRepository::class)]
21-
class UserRecoveryCode
24+
class UserRecoveryCode extends BaseEntity
2225
{
23-
#[ORM\Id]
24-
#[ORM\GeneratedValue]
25-
#[ORM\Column(name: 'id', type: 'integer', unique: true, nullable: false)]
26-
protected $id;
27-
2826
#[ORM\JoinColumn(name: 'user_id', referencedColumnName: 'id', onDelete: 'CASCADE')]
2927
#[ORM\ManyToOne(targetEntity: User::class)]
3028
private $user;
@@ -35,33 +33,48 @@ class UserRecoveryCode
3533
#[ORM\Column(name: 'used_at', type: 'datetime', nullable: true)]
3634
private $used_at;
3735

38-
#[ORM\Column(name: 'created_at', type: 'datetime')]
39-
private $created_at;
40-
4136
public function __construct()
4237
{
4338
$this->created_at = new \DateTime('now', new \DateTimeZone('UTC'));
4439
$this->used_at = null;
4540
}
4641

47-
public function getId(): int { return (int) $this->id; }
42+
public function getId(): int
43+
{
44+
return (int) $this->id;
45+
}
4846

49-
public function getUser(): User { return $this->user; }
50-
public function setUser(User $user): void { $this->user = $user; }
47+
public function getUser(): User
48+
{
49+
return $this->user;
50+
}
5151

52-
public function getCodeHash(): string { return $this->code_hash; }
53-
public function setCodeHash(string $value): void { $this->code_hash = $value; }
52+
public function getCodeHash(): string
53+
{
54+
return $this->code_hash;
55+
}
5456

55-
public function getUsedAt(): ?\DateTime { return $this->used_at; }
56-
public function setUsedAt(?\DateTime $value): void { $this->used_at = $value; }
57+
public function getUsedAt(): ?\DateTime
58+
{
59+
return $this->used_at;
60+
}
5761

58-
public function getCreatedAt(): \DateTime { return $this->created_at; }
62+
public function getCreatedAt(): \DateTime
63+
{
64+
return $this->created_at;
65+
}
5966

60-
public function isUsed(): bool { return !is_null($this->used_at); }
67+
public function isUsed(): bool
68+
{
69+
return !is_null($this->used_at);
70+
}
6171

6272
public function markUsed(): void
6373
{
74+
if ($this->used_at !== null) {
75+
throw new ValidationException('Recovery code already used at ' . $this->used_at->format(\DateTime::ATOM));
76+
}
6477
$this->used_at = new \DateTime('now', new \DateTimeZone('UTC'));
6578
}
6679

67-
}
80+
}

‎database/migrations/Version20260416194357.php‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
<?php namespace Database\Migrations;
1+
<?php
2+
namespace Database\Migrations;
23
/**
34
* Copyright 2026 OpenStack Foundation
45
* Licensed under the Apache License, Version 2.0 (the "License");
@@ -42,7 +43,8 @@ public function up(Schema $schema): void
4243
if (!$builder->hasTable("user_trusted_devices")) {
4344
$builder->create('user_trusted_devices', function (Table $table) {
4445
$table->increments('id');
45-
$table->timestamps();
46+
$table->dateTime('created_at');
47+
$table->dateTime('updated_at')->setNotnull(false);
4648
$table->bigInteger("user_id")->setUnsigned(true);
4749
$table->string('device_identifier', 255);
4850
$table->string('device_name', 255);
@@ -64,6 +66,7 @@ public function up(Schema $schema): void
6466
$builder->create('two_factor_audit_log', function (Table $table) {
6567
$table->increments('id');
6668
$table->dateTime('created_at');
69+
$table->dateTime('updated_at')->setNotnull(false);
6770
$table->bigInteger("user_id")->setUnsigned(true);
6871
$table->string('event_type', 64);
6972
$table->string('method', 32);
@@ -81,10 +84,12 @@ public function up(Schema $schema): void
8184
$builder->create('user_recovery_codes', function (Table $table) {
8285
$table->increments('id');
8386
$table->dateTime('created_at');
87+
$table->dateTime('updated_at')->setNotnull(false);
8488
$table->bigInteger("user_id")->setUnsigned(true);
85-
$table->string('code_hash', 255);
89+
$table->string('code_hash', 72)->setNotnull(true);
8690
$table->dateTime('used_at')->setNotnull(false)->setDefault(null);
8791
$table->index(["user_id", "used_at"], "urc_user_used_idx");
92+
$table->unique(["user_id", "code_hash"], "urc_user_codehash_uniq");
8893
$table->foreign("users", "user_id", "id", ["onDelete" => "CASCADE"]);
8994
});
9095
}
@@ -111,4 +116,4 @@ public function down(Schema $schema): void
111116
});
112117
}
113118
}
114-
}
119+
}

‎tests/TwoFactorRepositoriesTest.php‎

Lines changed: 64 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use Auth\User;
2121
use Illuminate\Support\Facades\App;
2222
use LaravelDoctrine\ORM\Facades\EntityManager;
23+
use models\exceptions\ValidationException;
2324

2425
/**
2526
* @package Tests
@@ -135,8 +136,8 @@ public function testRecoveryCodeRoundTrip(): void
135136
$repo = App::make(IUserRecoveryCodeRepository::class);
136137

137138
$code = new UserRecoveryCode();
138-
$code->setUser($this->user);
139-
$code->setCodeHash(password_hash('TESTCODE', PASSWORD_BCRYPT));
139+
self::setProp($code, 'user', $this->user);
140+
self::setProp($code, 'code_hash', password_hash('TESTCODE', PASSWORD_BCRYPT));
140141

141142
EntityManager::persist($code);
142143
EntityManager::flush();
@@ -246,12 +247,12 @@ public function testRecoveryCodeDeletionRemovesUsedAndUnusedCodes(): void
246247
$repo = App::make(IUserRecoveryCodeRepository::class);
247248

248249
$unused = new UserRecoveryCode();
249-
$unused->setUser($this->user);
250-
$unused->setCodeHash(password_hash('UNUSED_' . uniqid(), PASSWORD_BCRYPT));
250+
self::setProp($unused, 'user', $this->user);
251+
self::setProp($unused, 'code_hash', password_hash('UNUSED_' . uniqid(), PASSWORD_BCRYPT));
251252

252253
$used = new UserRecoveryCode();
253-
$used->setUser($this->user);
254-
$used->setCodeHash(password_hash('USED_' . uniqid(), PASSWORD_BCRYPT));
254+
self::setProp($used, 'user', $this->user);
255+
self::setProp($used, 'code_hash', password_hash('USED_' . uniqid(), PASSWORD_BCRYPT));
255256
$used->markUsed();
256257

257258
EntityManager::persist($unused);
@@ -327,10 +328,67 @@ public function testAuditLogsReturnedMostRecentFirst(): void
327328
EntityManager::flush();
328329
}
329330

331+
public function testGetUnusedByUserExcludesUsedCodes(): void
332+
{
333+
$repo = App::make(IUserRecoveryCodeRepository::class);
334+
335+
$unused = new UserRecoveryCode();
336+
self::setProp($unused, 'user', $this->user);
337+
self::setProp($unused, 'code_hash', password_hash('UNUSED_' . uniqid(), PASSWORD_BCRYPT));
338+
339+
$used = new UserRecoveryCode();
340+
self::setProp($used, 'user', $this->user);
341+
self::setProp($used, 'code_hash', password_hash('USED_' . uniqid(), PASSWORD_BCRYPT));
342+
$used->markUsed();
343+
344+
EntityManager::persist($unused);
345+
EntityManager::persist($used);
346+
EntityManager::flush();
347+
$unusedId = $unused->getId();
348+
$usedId = $used->getId();
349+
350+
EntityManager::clear();
351+
352+
$result = $repo->getUnusedByUser($this->user);
353+
$ids = array_map(fn(UserRecoveryCode $c) => $c->getId(), $result);
354+
355+
$this->assertContains($unusedId, $ids, 'getUnusedByUser must include the unused code.');
356+
$this->assertNotContains($usedId, $ids, 'getUnusedByUser must exclude used codes.');
357+
358+
foreach ([$unusedId, $usedId] as $codeId) {
359+
$code = EntityManager::find(UserRecoveryCode::class, $codeId);
360+
if ($code) { EntityManager::remove($code); }
361+
}
362+
EntityManager::flush();
363+
}
364+
365+
public function testMarkUsedTwiceThrows(): void
366+
{
367+
$code = new UserRecoveryCode();
368+
self::setProp($code, 'user', $this->user);
369+
self::setProp($code, 'code_hash', password_hash('TEST_' . uniqid(), PASSWORD_BCRYPT));
370+
$code->markUsed();
371+
372+
$this->expectException(ValidationException::class);
373+
$code->markUsed();
374+
}
375+
376+
public function testSetCodeHashRejectsPlaintext(): void
377+
{
378+
$this->markTestSkipped('setCodeHash() was removed from UserRecoveryCode; plaintext validation no longer has an entry point.');
379+
}
380+
330381
// -------------------------------------------------------------------------
331382
// Helpers
332383
// -------------------------------------------------------------------------
333384

385+
private static function setProp(object $obj, string $prop, mixed $value): void
386+
{
387+
$p = new \ReflectionProperty($obj, $prop);
388+
$p->setAccessible(true);
389+
$p->setValue($obj, $value);
390+
}
391+
334392
private function buildDevice(string $deviceId, \DateTime $now, \DateTime $expires): UserTrustedDevice
335393
{
336394
$device = new UserTrustedDevice();

0 commit comments

Comments
 (0)