Skip to content

Commit 133b2a8

Browse files
matiasperrone-exomatiasperrone-exo
committed
chore: Add PR's requested changes
1 parent 1d8c6e4 commit 133b2a8

5 files changed

Lines changed: 91 additions & 18 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/AuthService.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ public function login(string $username, string $password, bool $remember_me): bo
153153
if (!Auth::attempt(['username' => $username, 'password' => $password], $remember_me)) {
154154
throw new AuthenticationException
155155
(
156-
"We are sorry, your username or password does not match an existing record."
156+
"username or password does not match an existing record."
157157
);
158158
}
159159
Log::debug("AuthService::login: clearing principal");
@@ -162,7 +162,7 @@ public function login(string $username, string $password, bool $remember_me): bo
162162
if(is_null($current_user) || !$current_user->canLogin())
163163
throw new AuthenticationException
164164
(
165-
"We are sorry, your username or password does not match an existing record."
165+
"username or password does not match an existing record."
166166
);
167167
$this->principal_service->register
168168
(
@@ -264,7 +264,7 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $
264264

265265
if(!$user->canLogin()){
266266
Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login ( is not active ).", $user->getId()));
267-
throw new AuthenticationException("We are sorry, your username or password does not match an existing record.");
267+
throw new AuthenticationException("username or password does not match an existing record.");
268268
}
269269

270270
$otp->setAuthTime(time());

‎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/CustomAuthProviderTest.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,24 @@
1212
* limitations under the License.
1313
**/
1414
use Auth\CustomAuthProvider;
15+
use Auth\User;
1516
use Utils\Services\UtilsServiceCatalog;
1617
use OpenId\Services\OpenIdServiceCatalog;
1718
use Auth\Repositories\IUserRepository;
1819
use Auth\IAuthenticationExtensionService;
1920
use Illuminate\Support\Facades\App;
21+
use Mockery;
2022
/**
2123
* Class CustomAuthProviderTest
2224
*/
2325
final class CustomAuthProviderTest extends TestCase {
2426

27+
public function tearDown(): void
28+
{
29+
parent::tearDown();
30+
Mockery::close();
31+
}
32+
2533
/**
2634
* @return CustomAuthProvider
2735
*/
@@ -39,4 +47,31 @@ public function testCreateProvider(){
3947

4048
return $provider;
4149
}
50+
51+
public function testValidateCredentialsReturnsFalseWhenUserCannotLoginDueToUnverifiedEmail(): void
52+
{
53+
// A freshly constructed User has active=true, email_verified=false → canLogin()=false.
54+
// This covers the canLogin()===false branch for a reason other than isActive().
55+
$user = new User();
56+
57+
$repo = Mockery::mock(IUserRepository::class);
58+
$repo->shouldReceive('getByEmailOrName')
59+
->with('unverified@example.com')
60+
->andReturn($user);
61+
62+
$provider = new CustomAuthProvider(
63+
$repo,
64+
App::make(IAuthenticationExtensionService::class),
65+
App::make(OpenIdServiceCatalog::UserService),
66+
App::make(UtilsServiceCatalog::CheckPointService),
67+
App::make(UtilsServiceCatalog::TransactionService)
68+
);
69+
70+
$result = $provider->validateCredentials(
71+
$user,
72+
['username' => 'unverified@example.com', 'password' => 'any-password']
73+
);
74+
75+
$this->assertFalse($result);
76+
}
4277
}

‎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)