Skip to content

Commit 05f7464

Browse files
matiasperrone-exoCopilot
andcommitted
chore: Add PR's requested changes
Co-authored-by: Copilot <copilot@github.com>
1 parent 392c560 commit 05f7464

3 files changed

Lines changed: 71 additions & 16 deletions

File tree

app/Http/Controllers/SocialLoginController.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ public function callback($provider)
157157
if(!$user->canLogin()) {
158158
throw new AuthenticationException
159159
(
160-
"We are sorry, your username does not match an existing record."
160+
"username does not match an existing record."
161161
);
162162
}
163163

app/libs/Auth/Models/User.php

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,12 @@ class User extends BaseEntity implements AuthenticatableContract, IOpenIdUser, I
8181
public const MFAMethod_TOTP = 'totp';
8282
public const MFAMethod_PASSKEY = 'passkey';
8383

84-
public const ValidMFAMethods = [self::MFAMethod_OTP];
84+
public const ValidMFAMethods = [
85+
self::MFAMethod_OTP,
86+
// self::MFAMethod_SMS,
87+
// self::MFAMethod_TOTP,
88+
// self::MFAMethod_PASSKEY
89+
];
8590

8691
/**
8792
* @var string
@@ -2452,6 +2457,18 @@ protected function setTwoFactorMethod(string $method): void
24522457
)
24532458
);
24542459
}
2460+
2461+
$availableMethods = $this->getAvailableTwoFactorMethods();
2462+
if (!in_array($method, $availableMethods, true)) {
2463+
throw new ValidationException(
2464+
sprintf(
2465+
"Disabled 2FA method '%s'. Enabled methods: %s",
2466+
$method,
2467+
implode(', ', $availableMethods)
2468+
)
2469+
);
2470+
}
2471+
24552472
$this->two_factor_method = $method;
24562473
}
24572474

@@ -2473,7 +2490,13 @@ public function setTwoFactorEnforcedAt(?\DateTime $at): void
24732490
*/
24742491
public function shouldRequire2FA(): bool
24752492
{
2476-
return ($this->isAdmin() || (bool) $this->two_factor_enabled);
2493+
$enforcedGroups = config('two_factor.enforced_groups', []);
2494+
foreach ($enforcedGroups as $slug) {
2495+
if ($this->belongToGroup($slug)) {
2496+
return true;
2497+
}
2498+
}
2499+
return (bool) $this->two_factor_enabled;
24772500
}
24782501

24792502
/**

tests/unit/UserTwoFactorTest.php

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -83,25 +83,34 @@ public function testShouldRequire2FA_adminUser(): void
8383
$this->assertTrue($user->shouldRequire2FA());
8484
}
8585

86-
public function testShouldRequire2FA_oauth2AdminUser(): void
86+
public function testShouldRequire2FA_BelongsToAnEnforcedGroup(): void
8787
{
88-
// Guard against the gotcha: shouldRequire2FA() must look at the full
89-
// config('two_factor.enforced_groups') list, not just call isAdmin().
90-
$user = new User();
91-
$this->assignGroups($user, [$this->buildGroup(IGroupSlugs::AdminGroup)]);
88+
config(['two_factor.enforced_groups' => []]);
89+
$this->assertSame([], config('two_factor.enforced_groups'), "Config value 'two_factor.enforced_groups' is set");
9290

93-
$this->assertTrue($user->isAdmin(), IGroupSlugs::AdminGroup.' is an admin group per isAdmin()');
94-
$this->assertTrue($user->shouldRequire2FA());
91+
$groups = [
92+
IGroupSlugs::SuperAdminGroup,
93+
IGroupSlugs::AdminGroup,
94+
IGroupSlugs::OAuth2ServerAdminGroup,
95+
];
96+
config(['two_factor.enforced_groups' => $groups]);
97+
$this->assertSame($groups, config('two_factor.enforced_groups'), "Config value 'two_factor.enforced_groups' is set");
9598

9699
$user = new User();
97-
$this->assignGroups($user, [$this->buildGroup(IGroupSlugs::SuperAdminGroup)]);
98-
$this->assertTrue($user->isAdmin(), IGroupSlugs::SuperAdminGroup.' is an admin group per isAdmin()');
99-
$this->assertTrue($user->shouldRequire2FA());
100+
$this->assertFalse($user->shouldRequire2FA(), "The user does not belong to any enforced group");
101+
100102

103+
$this->assignGroups($user, array_map([$this, 'buildGroup'], $groups));
104+
$this->assertTrue($user->belongToGroup(IGroupSlugs::SuperAdminGroup), "The user belongs to ".IGroupSlugs::SuperAdminGroup." group");
105+
$this->assertTrue($user->belongToGroup(IGroupSlugs::AdminGroup), "The user belongs to ".IGroupSlugs::AdminGroup." group");
106+
$this->assertTrue($user->belongToGroup(IGroupSlugs::OAuth2ServerAdminGroup), "The user belongs to ".IGroupSlugs::OAuth2ServerAdminGroup." group");
107+
$this->assertTrue($user->shouldRequire2FA(), "The user does belong to an enforced group");
108+
109+
$groups = [IGroupSlugs::RawUsersGroup];
101110
$user = new User();
102-
$this->assignGroups($user, [$this->buildGroup(IGroupSlugs::OAuth2ServerAdminGroup)]);
103-
$this->assertFalse($user->isAdmin(), IGroupSlugs::OAuth2ServerAdminGroup.' is NOT an admin group per isAdmin()');
104-
$this->assertFalse($user->shouldRequire2FA());
111+
$this->assertFalse($user->shouldRequire2FA(), "The user does not belong to any enforced group");
112+
$this->assignGroups($user, array_map([$this, 'buildGroup'], $groups));
113+
$this->assertFalse($user->shouldRequire2FA(), "The user does not belong to any enforced group");
105114
}
106115

107116
public function testShouldRequire2FA_regularUser_enabled(): void
@@ -200,6 +209,29 @@ public function testSetTwoFactorMethod_invalidMethod_throws(): void
200209
$setter->invoke($user, 'bogus');
201210
}
202211

212+
public function testShouldRequire2FA_configDrivenEnforcement(): void
213+
{
214+
// Users in enforced_groups must require 2FA even if two_factor_enabled=false
215+
// and even if isAdmin() returns false for their group (OAuth2/OpenId admins).
216+
$groupsUnderTest = [
217+
IGroupSlugs::OAuth2ServerAdminGroup,
218+
IGroupSlugs::OpenIdServerAdminsGroup,
219+
];
220+
221+
foreach ($groupsUnderTest as $groupSlug) {
222+
$user = new User();
223+
$this->assignGroups($user, [$this->buildGroup($groupSlug)]);
224+
$this->assertFalse($user->isTwoFactorEnabled());
225+
$this->assertFalse($user->isAdmin(), "$groupSlug must NOT be covered by isAdmin()");
226+
$this->assertTrue($user->shouldRequire2FA(), "$groupSlug is in enforced_groups — shouldRequire2FA() must return true regardless of the stored flag");
227+
}
228+
229+
// Sanity: a regular user with two_factor_enabled=false is NOT enforced
230+
$regular = new User();
231+
$this->assignGroups($regular, [$this->buildGroup(IGroupSlugs::RawUsersGroup)]);
232+
$this->assertFalse($regular->shouldRequire2FA());
233+
}
234+
203235
public function testShouldRequire2FA_emptyEnforcedGroups_fallsThroughToFlag(): void
204236
{
205237
// Locks in the config fall-through: when enforced_groups is empty,

0 commit comments

Comments
 (0)