Skip to content

Commit 823bfd0

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

2 files changed

Lines changed: 52 additions & 14 deletions

File tree

app/libs/Auth/Models/User.php

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2473,7 +2473,13 @@ public function setTwoFactorEnforcedAt(?\DateTime $at): void
24732473
*/
24742474
public function shouldRequire2FA(): bool
24752475
{
2476-
return ($this->isAdmin() || (bool) $this->two_factor_enabled);
2476+
$enforcedGroups = config('two_factor.enforced_groups', []);
2477+
foreach ($enforcedGroups as $slug) {
2478+
if ($this->belongToGroup($slug)) {
2479+
return true;
2480+
}
2481+
}
2482+
return (bool) $this->two_factor_enabled;
24772483
}
24782484

24792485
/**

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)