Skip to content

Commit f2a0fbc

Browse files
turegjorupclaude
andcommitted
fix: correct inverted user-type guard in UserService::activateExternalUser()
The guard "Make sure user is an external user" compared the enum against a boolean (`UserTypeEnum::OIDC_EXTERNAL === !$user->getUserType()`), which is always false, so the check never threw. Use !== against the enum value, matching the equivalent guard in removeUserFromCurrentTenant(). The activate endpoint is still gated by ExternalUserAuthenticator at the firewall, so this was dead defense-in-depth rather than an exploitable hole; this restores the intended service-layer check. Removes the corresponding identical.alwaysFalse / booleanNot.alwaysFalse entries from the PHPStan baseline and adds unit tests covering both the non-external rejection and the unknown-code path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 634925a commit f2a0fbc

4 files changed

Lines changed: 69 additions & 13 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
44

55
## [Unreleased]
66

7+
- Fixed inverted user-type guard in `UserService::activateExternalUser()`: the "user is not of
8+
external type" check could never trigger due to a stray negation (`=== !$user->getUserType()`).
9+
The endpoint was still protected by `ExternalUserAuthenticator`, so this restores the intended
10+
defense-in-depth at the service layer. Added unit tests.
711
- Enabled PHPStan's `reportIgnoresWithoutComments`: inline `@phpstan-ignore` annotations must
812
carry a comment explaining the suppression.
913

phpstan-baseline.neon

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2826,18 +2826,6 @@ parameters:
28262826
count: 1
28272827
path: src/Service/TenantFactory.php
28282828

2829-
-
2830-
message: '#^Negated boolean expression is always false\.$#'
2831-
identifier: booleanNot.alwaysFalse
2832-
count: 1
2833-
path: src/Service/UserService.php
2834-
2835-
-
2836-
message: '#^Strict comparison using \=\=\= between App\\Enum\\UserTypeEnum\:\:OIDC_EXTERNAL and false will always evaluate to false\.$#'
2837-
identifier: identical.alwaysFalse
2838-
count: 1
2839-
path: src/Service/UserService.php
2840-
28412829
-
28422830
message: '#^Class App\\State\\AbstractProcessor implements generic interface ApiPlatform\\State\\ProcessorInterface but does not specify its types\: T1, T2$#'
28432831
identifier: missingType.generics

src/Service/UserService.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public function activateExternalUser(string $code): void
7171
$user = $this->security->getUser();
7272

7373
// Make sure user is an external user.
74-
if (UserTypeEnum::OIDC_EXTERNAL === !$user->getUserType()) {
74+
if (UserTypeEnum::OIDC_EXTERNAL !== $user->getUserType()) {
7575
throw new BadRequestException('User is not of external type.');
7676
}
7777

tests/Service/UserServiceTest.php

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace App\Tests\Service;
6+
7+
use App\Entity\User;
8+
use App\Enum\UserTypeEnum;
9+
use App\Exceptions\BadRequestException;
10+
use App\Exceptions\NotFoundException;
11+
use App\Repository\UserActivationCodeRepository;
12+
use App\Repository\UserRepository;
13+
use App\Repository\UserRoleTenantRepository;
14+
use App\Service\UserService;
15+
use Doctrine\ORM\EntityManagerInterface;
16+
use PHPUnit\Framework\TestCase;
17+
use Symfony\Component\Security\Core\Security;
18+
19+
class UserServiceTest extends TestCase
20+
{
21+
public function testActivateExternalUserThrowsForNonExternalUser(): void
22+
{
23+
$user = new User();
24+
$user->setUserType(UserTypeEnum::OIDC_INTERNAL);
25+
26+
$userService = $this->createUserService($user);
27+
28+
$this->expectException(BadRequestException::class);
29+
$this->expectExceptionMessage('User is not of external type.');
30+
31+
$userService->activateExternalUser('SOMECODE');
32+
}
33+
34+
public function testActivateExternalUserThrowsForUnknownCode(): void
35+
{
36+
$user = new User();
37+
$user->setUserType(UserTypeEnum::OIDC_EXTERNAL);
38+
39+
$userService = $this->createUserService($user);
40+
41+
$this->expectException(NotFoundException::class);
42+
43+
$userService->activateExternalUser('UNKNOWNCODE');
44+
}
45+
46+
private function createUserService(User $user): UserService
47+
{
48+
$security = $this->createMock(Security::class);
49+
$security->method('getUser')->willReturn($user);
50+
51+
$activationCodeRepository = $this->createMock(UserActivationCodeRepository::class);
52+
$activationCodeRepository->method('findOneBy')->willReturn(null);
53+
54+
return new UserService(
55+
$activationCodeRepository,
56+
$this->createMock(EntityManagerInterface::class),
57+
$security,
58+
'hash-salt',
59+
$this->createMock(UserRoleTenantRepository::class),
60+
$this->createMock(UserRepository::class),
61+
'P2D',
62+
);
63+
}
64+
}

0 commit comments

Comments
 (0)