From 8a46d54af4dad13a3ef934668bf99cc934bed321 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Kruli=C5=A1?= Date: Sat, 8 Feb 2025 19:46:53 +0100 Subject: [PATCH 1/7] Making the gravatar switch optional in edit-user endpoint. --- app/V1Module/presenters/UsersPresenter.php | 7 ++++-- tests/Presenters/UsersPresenter.phpt | 25 +++++++++++++--------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/app/V1Module/presenters/UsersPresenter.php b/app/V1Module/presenters/UsersPresenter.php index b15565040..14ac0d4c1 100644 --- a/app/V1Module/presenters/UsersPresenter.php +++ b/app/V1Module/presenters/UsersPresenter.php @@ -225,7 +225,7 @@ public function checkUpdateProfile(string $id) * description="New password of current user") * @Param(type="post", name="passwordConfirm", required=false, validation="string:1..", * description="Confirmation of new password of current user") - * @Param(type="post", name="gravatarUrlEnabled", validation="bool", + * @Param(type="post", name="gravatarUrlEnabled", validation="bool", required=false, * description="Enable or disable gravatar profile image") * @throws WrongCredentialsException * @throws NotFoundException @@ -254,7 +254,10 @@ public function actionUpdateProfile(string $id) $req->getPost("passwordConfirm") ); - $user->setGravatar(filter_var($req->getPost("gravatarUrlEnabled"), FILTER_VALIDATE_BOOLEAN)); + $gravatarUrlEnabled = $req->getPost("gravatarUrlEnabled"); + if ($gravatarUrlEnabled !== null) { // null or missing value -> no update + $user->setGravatar(filter_var($gravatarUrlEnabled, FILTER_VALIDATE_BOOLEAN)); + } // make changes permanent $this->users->flush(); diff --git a/tests/Presenters/UsersPresenter.phpt b/tests/Presenters/UsersPresenter.phpt index 74a75099f..8d8ab9853 100644 --- a/tests/Presenters/UsersPresenter.phpt +++ b/tests/Presenters/UsersPresenter.phpt @@ -244,7 +244,6 @@ class TestUsersPresenter extends Tester\TestCase 'lastName' => $lastName, 'titlesBeforeName' => $titlesBeforeName, 'titlesAfterName' => $titlesAfterName, - 'gravatarUrlEnabled' => false ] ); $response = $this->presenter->run($request); @@ -276,6 +275,9 @@ class TestUsersPresenter extends Tester\TestCase $emailVerificationHelper->shouldReceive("process")->with($user)->andReturn()->once(); $this->presenter->emailVerificationHelper = $emailVerificationHelper; + $user->setGravatar(true); + $this->presenter->users->persist($user); + $request = new Nette\Application\Request( $this->presenterPath, 'POST', @@ -286,7 +288,7 @@ class TestUsersPresenter extends Tester\TestCase 'titlesBeforeName' => $titlesBeforeName, 'titlesAfterName' => $titlesAfterName, 'email' => $email, - 'gravatarUrlEnabled' => false + 'gravatarUrlEnabled' => false // make sure gravatar gets reset ] ); $response = $this->presenter->run($request); @@ -313,6 +315,9 @@ class TestUsersPresenter extends Tester\TestCase $user = $this->users->getByEmail(PresenterTestHelper::ADMIN_LOGIN); $login = $this->presenter->logins->findByUsernameOrThrow($user->getEmail()); + $user->setGravatar(true); + $this->presenter->users->persist($user); + $firstName = "firstNameUpdated"; $lastName = "lastNameUpdated"; $titlesBeforeName = "titlesBeforeNameUpdated"; @@ -333,7 +338,6 @@ class TestUsersPresenter extends Tester\TestCase 'oldPassword' => $oldPassword, 'password' => $password, 'passwordConfirm' => $passwordConfirm, - 'gravatarUrlEnabled' => false ] ); $response = $this->presenter->run($request); @@ -345,7 +349,7 @@ class TestUsersPresenter extends Tester\TestCase $updatedUser = $result["payload"]["user"]; Assert::equal("$titlesBeforeName $firstName $lastName $titlesAfterName", $updatedUser["fullName"]); Assert::true($login->passwordsMatchOrEmpty($password, $this->presenter->passwordsService)); - Assert::null($updatedUser["avatarUrl"]); + Assert::true($updatedUser["avatarUrl"] !== null); // gravatar was not reset $storedUpdatedUser = $this->users->get($user->getId()); Assert::equal($updatedUser["id"], $storedUpdatedUser->getId()); @@ -459,7 +463,7 @@ class TestUsersPresenter extends Tester\TestCase [ 'titlesBeforeName' => '', 'titlesAfterName' => '', - 'gravatarUrlEnabled' => false, + 'gravatarUrlEnabled' => null, 'password' => $newPassword, 'passwordConfirm' => $newPassword, ] @@ -467,6 +471,7 @@ class TestUsersPresenter extends Tester\TestCase $updatedUser = $payload["user"]; Assert::equal($updatedUser["privateData"]["email"], PresenterTestHelper::GROUP_SUPERVISOR_LOGIN); + Assert::null($updatedUser["avatarUrl"]); $login = $this->logins->findByUsernameOrThrow($user->getEmail()); Assert::true($login->passwordsMatch($newPassword, $this->presenter->passwordsService)); @@ -579,13 +584,13 @@ class TestUsersPresenter extends Tester\TestCase ); Assert::equal($uiData, $payload["privateData"]["uiData"]); - $nested = [ 'pos1' => 0 ]; + $nested = ['pos1' => 0]; $payload = PresenterTestHelper::performPresenterRequest( $this->presenter, $this->presenterPath, 'POST', ['action' => 'updateUiData', 'id' => $user->getId()], - ['uiData' => [ 'stretcherSize' => 54, 'nestedStructure' => $nested ] ] + ['uiData' => ['stretcherSize' => 54, 'nestedStructure' => $nested]] ); $uiData['stretcherSize'] = 54; $uiData['nestedStructure'] = $nested; @@ -596,7 +601,7 @@ class TestUsersPresenter extends Tester\TestCase $this->presenterPath, 'POST', ['action' => 'updateUiData', 'id' => $user->getId()], - [ 'uiData' => $uiData2, 'overwrite' => true ] + ['uiData' => $uiData2, 'overwrite' => true] ); Assert::equal($uiData2, $payload["privateData"]["uiData"]); @@ -605,7 +610,7 @@ class TestUsersPresenter extends Tester\TestCase $this->presenterPath, 'POST', ['action' => 'updateUiData', 'id' => $user->getId()], - [ 'uiData' => null ] + ['uiData' => null] ); Assert::equal($uiData2, $payload["privateData"]["uiData"]); @@ -614,7 +619,7 @@ class TestUsersPresenter extends Tester\TestCase $this->presenterPath, 'POST', ['action' => 'updateUiData', 'id' => $user->getId()], - [ 'uiData' => null, 'overwrite' => true ] + ['uiData' => null, 'overwrite' => true] ); Assert::null($payload["privateData"]["uiData"]); } From bff5fada2d169a0dd9516ed5786fc9fb35abec48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Kruli=C5=A1?= Date: Sat, 8 Feb 2025 20:26:28 +0100 Subject: [PATCH 2/7] Fixing bug in last-auth-time update for deactivated users. --- app/V1Module/presenters/LoginPresenter.php | 19 ++++++++++++++++++- app/V1Module/security/AccessManager.php | 6 +++--- .../security/CredentialsAuthenticator.php | 9 +++++++++ .../ExternalServiceAuthenticator.php | 9 +++++++++ 4 files changed, 39 insertions(+), 4 deletions(-) diff --git a/app/V1Module/presenters/LoginPresenter.php b/app/V1Module/presenters/LoginPresenter.php index 8559d138d..b243102bf 100644 --- a/app/V1Module/presenters/LoginPresenter.php +++ b/app/V1Module/presenters/LoginPresenter.php @@ -22,6 +22,7 @@ use App\Security\Roles; use App\Security\TokenScope; use Nette\Security\AuthenticationException; +use Nette\Http\IResponse; /** * Endpoints used to log a user in @@ -201,6 +202,14 @@ public function actionRefresh() $token = $this->getAccessToken(); $user = $this->getCurrentUser(); + if (!$user->isAllowed()) { + throw new ForbiddenRequestException( + "Forbidden Request - User account was disabled", + IResponse::S403_Forbidden, + FrontendErrorMappings::E403_002__USER_NOT_ALLOWED + ); + } + $user->updateLastAuthenticationAt(); $this->users->flush(); @@ -247,6 +256,14 @@ public function actionIssueRestrictedToken() $this->validateEffectiveRole($effectiveRole); $user = $this->getCurrentUser(); + if (!$user->isAllowed()) { + throw new ForbiddenRequestException( + "Forbidden Request - User account was disabled", + IResponse::S403_Forbidden, + FrontendErrorMappings::E403_002__USER_NOT_ALLOWED + ); + } + $user->updateLastAuthenticationAt(); $this->users->flush(); @@ -265,7 +282,7 @@ private function validateScopeRoles(?array $scopes, $expiration) { $forbiddenScopes = [ TokenScope::CHANGE_PASSWORD => - "Password change tokens can only be issued through the password reset endpoint", + "Password change tokens can only be issued through the password reset endpoint", TokenScope::EMAIL_VERIFICATION => "E-mail verification tokens must be received via e-mail", ]; diff --git a/app/V1Module/security/AccessManager.php b/app/V1Module/security/AccessManager.php index 0ac504d53..7fcf77ed6 100644 --- a/app/V1Module/security/AccessManager.php +++ b/app/V1Module/security/AccessManager.php @@ -112,7 +112,7 @@ public function getUser(AccessToken $token): User if (!$user) { throw new ForbiddenRequestException( "Forbidden Request - User does not exist", - IResponse::S403_FORBIDDEN, + IResponse::S403_Forbidden, FrontendErrorMappings::E403_001__USER_NOT_EXIST ); } @@ -120,7 +120,7 @@ public function getUser(AccessToken $token): User if (!$user->isAllowed()) { throw new ForbiddenRequestException( "Forbidden Request - User account was disabled", - IResponse::S403_FORBIDDEN, + IResponse::S403_Forbidden, FrontendErrorMappings::E403_002__USER_NOT_ALLOWED ); } @@ -148,7 +148,7 @@ public function issueToken( if (!$user->isAllowed()) { throw new ForbiddenRequestException( "Forbidden Request - User account was disabled", - IResponse::S403_FORBIDDEN, + IResponse::S403_Forbidden, FrontendErrorMappings::E403_002__USER_NOT_ALLOWED ); } diff --git a/app/V1Module/security/CredentialsAuthenticator.php b/app/V1Module/security/CredentialsAuthenticator.php index 66620e4c6..c79939621 100644 --- a/app/V1Module/security/CredentialsAuthenticator.php +++ b/app/V1Module/security/CredentialsAuthenticator.php @@ -4,10 +4,12 @@ use App\Exceptions\FrontendErrorMappings; use App\Exceptions\WrongCredentialsException; +use App\Exceptions\ForbiddenRequestException; use App\Model\Entity\User; use App\Model\Repository\Logins; use Nette; use Nette\Security\Passwords; +use Nette\Http\IResponse; class CredentialsAuthenticator { @@ -40,8 +42,15 @@ public function authenticate(string $username, string $password) "The username or password is incorrect.", FrontendErrorMappings::E400_101__WRONG_CREDENTIALS_LOCAL ); + } elseif (!$user->isAllowed()) { + throw new ForbiddenRequestException( + "Forbidden Request - User account was disabled", + IResponse::S403_Forbidden, + FrontendErrorMappings::E403_002__USER_NOT_ALLOWED + ); } + return $user; } } diff --git a/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php b/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php index 752571e7b..0dcdf3b6e 100644 --- a/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php +++ b/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php @@ -7,6 +7,7 @@ use App\Exceptions\WrongCredentialsException; use App\Exceptions\InvalidExternalTokenException; use App\Exceptions\InvalidArgumentException; +use App\Exceptions\ForbiddenRequestException; use App\Model\Entity\Instance; use App\Model\Entity\User; use App\Model\Repository\ExternalLogins; @@ -15,6 +16,7 @@ use App\Model\Repository\Instances; use App\Helpers\EmailVerificationHelper; use Nette\Utils\Arrays; +use Nette\Http\IResponse; use Firebase\JWT\JWT; use Firebase\JWT\Key; use DomainException; @@ -148,8 +150,15 @@ public function authenticate(string $authName, string $token, string $instanceId FrontendErrorMappings::E400_104__EXTERNAL_AUTH_FAILED_USER_NOT_FOUND, ["service" => $authName] ); + } elseif (!$user->isAllowed()) { + throw new ForbiddenRequestException( + "Forbidden Request - User account was disabled", + IResponse::S403_Forbidden, + FrontendErrorMappings::E403_002__USER_NOT_ALLOWED + ); } + return $user; } From 29b64c09c121aae67e4119dba16e9da4f8c93ef9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Kruli=C5=A1?= Date: Sun, 9 Feb 2025 00:18:35 +0100 Subject: [PATCH 3/7] Processing extra IDs in external authentication tokens. --- app/config/config.local.neon.example | 3 +- .../ExternalServiceAuthenticator.php | 62 ++++++++++++++++++- app/model/entity/ExternalLogin.php | 5 ++ app/model/repository/ExternalLogins.php | 1 - .../ExternalServiceAuthenticator.phpt | 12 ++-- tests/Presenters/LoginPresenter.phpt | 21 ++++--- 6 files changed, 84 insertions(+), 20 deletions(-) diff --git a/app/config/config.local.neon.example b/app/config/config.local.neon.example index 3ffc62785..a6228d35e 100644 --- a/app/config/config.local.neon.example +++ b/app/config/config.local.neon.example @@ -51,8 +51,9 @@ parameters: externalAuthenticators: - name: "cas-auth-ext" jwtSecret: "secretStringSharedWithExternAuth" + jwtAlgorithm: HS256 # optional, HS256 is default expiration: 60 # seconds passed since iat - usedAlgorithm: HS256 # optional, HS256 is default + extraIds: [] # additional service types whose IDs may be provided as extra IDs in the auth token emails: footerUrl: "%webapp.address%" diff --git a/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php b/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php index 0dcdf3b6e..48d836855 100644 --- a/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php +++ b/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php @@ -15,6 +15,7 @@ use App\Model\Repository\Users; use App\Model\Repository\Instances; use App\Helpers\EmailVerificationHelper; +use App\Helpers\FailureHelper; use Nette\Utils\Arrays; use Nette\Http\IResponse; use Firebase\JWT\JWT; @@ -42,6 +43,9 @@ class ExternalServiceAuthenticator /** @var EmailVerificationHelper */ public $emailVerificationHelper; + /** @var FailureHelper */ + public $failureHelper; + /** * @var array [ name => { jwtSecret, expiration } ] */ @@ -61,13 +65,15 @@ public function __construct( Users $users, Logins $logins, Instances $instances, - EmailVerificationHelper $emailVerificationHelper + EmailVerificationHelper $emailVerificationHelper, + FailureHelper $failureHelper, ) { $this->externalLogins = $externalLogins; $this->users = $users; $this->logins = $logins; $this->instances = $instances; $this->emailVerificationHelper = $emailVerificationHelper; + $this->failureHelper = $failureHelper; foreach ($authenticators as $auth) { if (!empty($auth['name'] && !empty($auth['jwtSecret']))) { @@ -75,8 +81,9 @@ public function __construct( 'jwtSecret' => $auth['jwtSecret'], 'expiration' => Arrays::get($auth, 'expiration', 60), 'defaultRole' => Arrays::get($auth, 'defaultRole', null), - 'usedAlgorithm' => Arrays::get($auth, 'usedAlgorithm', 'HS256'), // if set, users may register even when extrnal authenticator does not provide role + 'usedAlgorithm' => Arrays::get($auth, 'jwtAlgorithm', 'HS256'), + 'extraIds' => Arrays::get($auth, 'extraIds', []), ]; } } @@ -144,6 +151,7 @@ public function authenticate(string $authName, string $token, string $instanceId } } + // failures throw exceptions... if ($user === null) { throw new WrongCredentialsException( "User authenticated through '$authName' has no corresponding account in ReCodEx.", @@ -158,6 +166,7 @@ public function authenticate(string $authName, string $token, string $instanceId ); } + $this->handleExtraIds($user, $decodedToken, $this->authenticators[$authName]->extraIds); return $user; } @@ -237,4 +246,53 @@ private function getInstance($decodedToken, string $instanceId = null): ?Instanc return null; } + + /** + * Process possible additional (extra) identifiers present in the token. + * @param User $user being authenticated + * @param object $decodedToken + * @param string[] $allowedServices whose extra IDs may be added from the token + */ + private function handleExtraIds(User $user, $decodedToken, array $allowedServices) + { + if (empty($decodedToken->extId)) { + return; + } + + foreach ($decodedToken->extId as $service => $eid) { + if (!in_array($service, $allowedServices)) { + continue; // skip services that are not allowed + } + + $extUser = $this->externalLogins->getUser($service, $eid); + if ($extUser) { + if ($extUser->getId() !== $user->getId()) { + // Identity crysis! ID belongs to another user... + $this->failureHelper->report( + FailureHelper::TYPE_API_ERROR, + sprintf( + "User '%s' was provided with extra ID '%s' (%s), " + . "but that is already associated with user '%s'.", + $user->getId(), + $eid, + $service, + $extUser->getId() + ) + ); + } + + continue; // either already exist or we cannot proceed anyway + } + + $login = $this->externalLogins->findByUser($user, $service); + if ($login->getExternalId() !== $eid) { + // extra ID has changed (strange, but possible) + $login->setExternalId($eid); + $this->externalLogins->persist($login); + continue; + } + + $this->externalLogins->connect($service, $user, $eid); + } + } } diff --git a/app/model/entity/ExternalLogin.php b/app/model/entity/ExternalLogin.php index d07af0c71..71bd3e39c 100644 --- a/app/model/entity/ExternalLogin.php +++ b/app/model/entity/ExternalLogin.php @@ -60,6 +60,11 @@ public function getExternalId(): string return $this->externalId; } + public function setExternalId(string $externalId): void + { + $this->externalId = $externalId; + } + public function getUser(): User { return $this->user; diff --git a/app/model/repository/ExternalLogins.php b/app/model/repository/ExternalLogins.php index b53487a1d..a1b379040 100644 --- a/app/model/repository/ExternalLogins.php +++ b/app/model/repository/ExternalLogins.php @@ -11,7 +11,6 @@ */ class ExternalLogins extends BaseRepository { - public function __construct(EntityManagerInterface $em) { parent::__construct($em, ExternalLogin::class); diff --git a/tests/ExternalLogin/ExternalServiceAuthenticator.phpt b/tests/ExternalLogin/ExternalServiceAuthenticator.phpt index 288de9ca1..74a0ed1fa 100644 --- a/tests/ExternalLogin/ExternalServiceAuthenticator.phpt +++ b/tests/ExternalLogin/ExternalServiceAuthenticator.phpt @@ -1,14 +1,9 @@ users = $container->getByType(Users::class); $this->logins = $container->getByType(Logins::class); $this->authenticator = new ExternalServiceAuthenticator( - [ [ + [[ 'name' => self::AUTH_NAME, 'jwtSecret' => self::AUTH_SECRET, 'expiration' => 60, - ] ], + ]], $this->externalLogins, $this->users, $this->logins, $container->getByType(Instances::class), - $container->getByType(EmailVerificationHelper::class) + $container->getByType(EmailVerificationHelper::class), + $container->getByType(App\Helpers\FailureHelper::class) ); } diff --git a/tests/Presenters/LoginPresenter.phpt b/tests/Presenters/LoginPresenter.phpt index 89e55d0bc..a622b4a89 100644 --- a/tests/Presenters/LoginPresenter.phpt +++ b/tests/Presenters/LoginPresenter.phpt @@ -52,6 +52,9 @@ class TestLoginPresenter extends Tester\TestCase /** @var \App\Helpers\EmailVerificationHelper */ private $emailVerificationHelper; + /** @var \App\Helpers\FailureHelper */ + private $failureHelper; + public function __construct($container) { $this->container = $container; @@ -62,6 +65,7 @@ class TestLoginPresenter extends Tester\TestCase $this->externalLogins = $container->getByType(\App\Model\Repository\ExternalLogins::class); $this->instances = $container->getByType(\App\Model\Repository\Instances::class); $this->emailVerificationHelper = $container->getByType(\App\Helpers\EmailVerificationHelper::class); + $this->failureHelper = $container->getByType(App\Helpers\FailureHelper::class); } protected function setUp() @@ -86,8 +90,8 @@ class TestLoginPresenter extends Tester\TestCase "POST", ["action" => "default"], [ - "username" => $this->userLogin, - "password" => $this->userPassword + "username" => $this->userLogin, + "password" => $this->userPassword ] ); @@ -114,8 +118,8 @@ class TestLoginPresenter extends Tester\TestCase "POST", ["action" => "default"], [ - "username" => $this->userLogin, - "password" => $this->userPassword . "42" + "username" => $this->userLogin, + "password" => $this->userPassword . "42" ] ); @@ -135,15 +139,16 @@ class TestLoginPresenter extends Tester\TestCase Assert::count(0, $events); $authenticator = new ExternalServiceAuthenticator( - [ [ + [[ 'name' => 'test-cas', 'jwtSecret' => 'tajnyRetezec', - ] ], + ]], $this->externalLogins, $this->users, $this->logins, $this->instances, - $this->emailVerificationHelper + $this->emailVerificationHelper, + $this->failureHelper ); $user = $this->presenter->users->getByEmail($this->userLogin); @@ -159,7 +164,7 @@ class TestLoginPresenter extends Tester\TestCase $this->presenter->externalServiceAuthenticator = $authenticator; - $request = new Request("V1:Login", "POST", ["action" => "external", "authenticatorName" => "test-cas"], [ 'token' => $token ]); + $request = new Request("V1:Login", "POST", ["action" => "external", "authenticatorName" => "test-cas"], ['token' => $token]); $response = $this->presenter->run($request); Assert::type(JsonResponse::class, $response); From 12e65d863cf2699bb8cbbc232a78ad1b5b3c944e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Kruli=C5=A1?= Date: Sun, 9 Feb 2025 12:29:30 +0100 Subject: [PATCH 4/7] Adding proper logging for external authentication. --- .../ExternalServiceAuthenticator.php | 68 +++++++++++++++++-- .../AssignmentSolversPresenter.phpt | 4 -- 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php b/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php index 48d836855..e332af03f 100644 --- a/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php +++ b/app/helpers/ExternalLogin/ExternalServiceAuthenticator.php @@ -18,6 +18,7 @@ use App\Helpers\FailureHelper; use Nette\Utils\Arrays; use Nette\Http\IResponse; +use Tracy\ILogger; use Firebase\JWT\JWT; use Firebase\JWT\Key; use DomainException; @@ -46,6 +47,9 @@ class ExternalServiceAuthenticator /** @var FailureHelper */ public $failureHelper; + /** @var ILogger|null */ + public $logger = null; + /** * @var array [ name => { jwtSecret, expiration } ] */ @@ -67,6 +71,7 @@ public function __construct( Instances $instances, EmailVerificationHelper $emailVerificationHelper, FailureHelper $failureHelper, + ?ILogger $logger = null, ) { $this->externalLogins = $externalLogins; $this->users = $users; @@ -74,6 +79,7 @@ public function __construct( $this->instances = $instances; $this->emailVerificationHelper = $emailVerificationHelper; $this->failureHelper = $failureHelper; + $this->logger = $logger; foreach ($authenticators as $auth) { if (!empty($auth['name'] && !empty($auth['jwtSecret']))) { @@ -89,6 +95,18 @@ public function __construct( } } + private function log($level, $msg, ...$args) + { + if (!$this->logger) { + return; + } + + if ($args) { + $msg = sprintf($msg, ...$args); + } + $this->logger->log($msg, $level); + } + /** * Verify that given authenticator exists. * @param string $name of the external authenticator @@ -129,6 +147,15 @@ public function authenticate(string $authName, string $token, string $instanceId // try to match existing local user by email address if ($user === null) { $user = $this->tryConnect($authName, $userData); + if ($user) { + $this->log( + ILogger::INFO, + "User '%s' was paired with external ID='%s' (%s)", + $user->getId(), + $userData->getId(), + $authName + ); + } } // try to register a new user @@ -148,6 +175,13 @@ public function authenticate(string $authName, string $token, string $instanceId // connect the account to the login method $this->externalLogins->connect($authName, $user, $userData->getId()); $this->emailVerificationHelper->process($user, true); // true = just created + $this->log( + ILogger::INFO, + "User '%s' just registered via external auth '%s' (ID='%s')", + $user->getId(), + $authName, + $userData->getId() + ); } } @@ -259,13 +293,23 @@ private function handleExtraIds(User $user, $decodedToken, array $allowedService return; } + $this->log(ILogger::DEBUG, "User '%s' got extra IDs: %s", $user->getId(), json_encode($decodedToken->extId)); + foreach ($decodedToken->extId as $service => $eid) { if (!in_array($service, $allowedServices)) { + $this->log( + ILogger::DEBUG, + "User '%s' got new [%s] ID='%s', but this auth service is not allowed", + $user->getId(), + $service, + $eid + ); + continue; // skip services that are not allowed } $extUser = $this->externalLogins->getUser($service, $eid); - if ($extUser) { + if ($extUser) { // a user with given ID exists if ($extUser->getId() !== $user->getId()) { // Identity crysis! ID belongs to another user... $this->failureHelper->report( @@ -285,14 +329,26 @@ private function handleExtraIds(User $user, $decodedToken, array $allowedService } $login = $this->externalLogins->findByUser($user, $service); - if ($login->getExternalId() !== $eid) { - // extra ID has changed (strange, but possible) - $login->setExternalId($eid); - $this->externalLogins->persist($login); - continue; + if ($login) { // a connected external login record for the auth service already exist + if ($login->getExternalId() !== $eid) { + // extra ID has changed (strange, but possible) + $this->log( + ILogger::INFO, + "User '%s' got new [%s] ID='%s', but already had different ID='%s'", + $user->getId(), + $service, + $eid, + $login->getExternalId() + ); + $login->setExternalId($eid); + $this->externalLogins->persist($login); + } + + continue; // either already exist or was duly updated } $this->externalLogins->connect($service, $user, $eid); + $this->log(ILogger::INFO, "User '%s' got new extra ID='%s' [%s]", $user->getId(), $eid, $service); } } } diff --git a/tests/Presenters/AssignmentSolversPresenter.phpt b/tests/Presenters/AssignmentSolversPresenter.phpt index 5f9253aea..adeb71c10 100644 --- a/tests/Presenters/AssignmentSolversPresenter.phpt +++ b/tests/Presenters/AssignmentSolversPresenter.phpt @@ -2,14 +2,10 @@ $container = require_once __DIR__ . "/../bootstrap.php"; -use App\Helpers\BrokerProxy; use App\V1Module\Presenters\AssignmentSolversPresenter; -use App\Model\Entity\AssignmentSolver; use App\Model\Repository\Assignments; -use App\Model\Repository\AssignmentSolvers; use Doctrine\ORM\EntityManagerInterface; use Tester\Assert; -use Tracy\ILogger; $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; From a3c954aadf22d4a5e8b7411cafc4f1f35d7ea151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Kruli=C5=A1?= Date: Sun, 9 Feb 2025 17:19:07 +0100 Subject: [PATCH 5/7] Adding endpoints for creating, updating, and removing external IDs. --- app/V1Module/presenters/UsersPresenter.php | 84 ++++++++++++++++++- app/V1Module/router/RouterFactory.php | 2 + .../security/ACL/IUserPermissions.php | 2 + app/config/permissions.neon | 1 + app/helpers/FailureHelper/FailureHelper.php | 1 - app/model/entity/ExternalLogin.php | 3 +- migrations/Version20250209140028.php | 31 +++++++ 7 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 migrations/Version20250209140028.php diff --git a/app/V1Module/presenters/UsersPresenter.php b/app/V1Module/presenters/UsersPresenter.php index 14ac0d4c1..ba70007db 100644 --- a/app/V1Module/presenters/UsersPresenter.php +++ b/app/V1Module/presenters/UsersPresenter.php @@ -7,11 +7,13 @@ use App\Exceptions\InvalidArgumentException; use App\Exceptions\NotFoundException; use App\Exceptions\WrongCredentialsException; +use App\Model\Entity\ExternalLogin; use App\Model\Entity\Group; use App\Model\Entity\Login; use App\Model\Entity\SecurityEvent; use App\Model\Entity\User; use App\Model\Entity\UserUiData; +use App\Model\Repository\ExternalLogins; use App\Model\Repository\Logins; use App\Model\Repository\SecurityEvents; use App\Exceptions\BadRequestException; @@ -36,6 +38,12 @@ class UsersPresenter extends BasePresenter */ public $logins; + /** + * @var ExternalLogins + * @inject + */ + public $externalLogins; + /** * @var SecurityEvents * @inject @@ -294,7 +302,7 @@ private function changeUserEmail(User $user, ?string $email) } if (filter_var($email, FILTER_VALIDATE_EMAIL) === false) { - throw new InvalidArgumentException("Provided email is not in correct format"); + throw new InvalidArgumentException('email', "Provided email is not in correct format"); } $oldEmail = $user->getEmail(); @@ -375,7 +383,7 @@ private function changeUserPassword( if (!$password || !$passwordConfirm) { // old password was provided but the new ones not, illegal state - throw new InvalidArgumentException("New password was not provided"); + throw new InvalidArgumentException('password|passwordConfirm', "New password was not provided"); } // passwords need to be handled differently @@ -764,4 +772,76 @@ public function actionSetAllowed(string $id) $this->users->flush(); $this->sendSuccessResponse($this->userViewFactory->getUser($user)); } + + public function checkUpdateExternalLogin(string $id, string $service) + { + $user = $this->users->findOrThrow($id); + if (!$this->userAcl->canSetExternalIds($user)) { + throw new ForbiddenRequestException(); + } + + // in the future, we might consider cross-checking the service ID + } + + /** + * Add or update existing external ID of given authentication service. + * @POST + * @param string $id identifier of the user + * @param string $service identifier of the authentication service (login type) + * @Param(type="post", name="externalId", validation="string:1..128") + * @throws InvalidArgumentException + */ + public function actionUpdateExternalLogin(string $id, string $service) + { + $user = $this->users->findOrThrow($id); + + // make sure the external ID is not used for another user + $externalId = $this->getRequest()->getPost("externalId"); + $anotherUser = $this->externalLogins->getUser($service, $externalId); + if ($anotherUser) { + if ($anotherUser->getId() !== $id) { + // oopsie, this external ID is alreay used for a different user + throw new InvalidArgumentException('externalId', "This ID is already used by another user."); + } + // otherwise the external ID is already set to this user, so there is nothing to change... + } else { + // create/update external login entry + $login = $this->externalLogins->findByUser($user, $service); + if ($login) { + $login->setExternalId($externalId); + } else { + $login = new ExternalLogin($user, $service, $externalId); + } + + $this->externalLogins->persist($login); + } + + $this->sendSuccessResponse($this->userViewFactory->getUser($user)); + } + + public function checkRemoveExternalLogin(string $id, string $service) + { + $user = $this->users->findOrThrow($id); + if (!$this->userAcl->canSetExternalIds($user)) { + throw new ForbiddenRequestException(); + } + + // in the future, we might consider cross-checking the service ID + } + + /** + * Remove external ID of given authentication service. + * @DELETE + * @param string $id identifier of the user + * @param string $service identifier of the authentication service (login type) + */ + public function actionRemoveExternalLogin(string $id, string $service) + { + $user = $this->users->findOrThrow($id); + $login = $this->externalLogins->findByUser($user, $service); + if ($login) { + $this->externalLogins->remove($login); + } + $this->sendSuccessResponse($this->userViewFactory->getUser($user)); + } } diff --git a/app/V1Module/router/RouterFactory.php b/app/V1Module/router/RouterFactory.php index c6357b495..b67a4b496 100644 --- a/app/V1Module/router/RouterFactory.php +++ b/app/V1Module/router/RouterFactory.php @@ -493,6 +493,8 @@ private static function createUsersRoutes(string $prefix): RouteList $router[] = new PostRoute("$prefix//create-local", "Users:createLocalAccount"); $router[] = new PostRoute("$prefix//role", "Users:setRole"); $router[] = new PostRoute("$prefix//allowed", "Users:setAllowed"); + $router[] = new PostRoute("$prefix//external-login/", "Users:updateExternalLogin"); + $router[] = new DeleteRoute("$prefix//external-login/", "Users:removeExternalLogin"); $router[] = new GetRoute("$prefix//calendar-tokens", "UserCalendars:userCalendars"); $router[] = new PostRoute("$prefix//calendar-tokens", "UserCalendars:createCalendar"); $router[] = new GetRoute("$prefix//pending-reviews", "AssignmentSolutionReviews:pending"); diff --git a/app/V1Module/security/ACL/IUserPermissions.php b/app/V1Module/security/ACL/IUserPermissions.php index 5d6c1dc11..dabef3e86 100644 --- a/app/V1Module/security/ACL/IUserPermissions.php +++ b/app/V1Module/security/ACL/IUserPermissions.php @@ -36,6 +36,8 @@ public function canSetRole(User $user): bool; public function canSetIsAllowed(User $user): bool; + public function canSetExternalIds(User $user): bool; + public function canInvalidateTokens(User $user): bool; public function canForceChangePassword(User $user): bool; diff --git a/app/config/permissions.neon b/app/config/permissions.neon index 5cd506242..41224d8ba 100644 --- a/app/config/permissions.neon +++ b/app/config/permissions.neon @@ -400,6 +400,7 @@ permissions: - updateProfile - updatePersonalData - setIsAllowed + - setExternalIds - createLocalAccount - invalidateTokens diff --git a/app/helpers/FailureHelper/FailureHelper.php b/app/helpers/FailureHelper/FailureHelper.php index 8f590f7d3..1fb3327b3 100644 --- a/app/helpers/FailureHelper/FailureHelper.php +++ b/app/helpers/FailureHelper/FailureHelper.php @@ -15,7 +15,6 @@ */ class FailureHelper { - public const TYPE_BACKEND_ERROR = "BACKEND ERROR"; public const TYPE_API_ERROR = "API ERROR"; diff --git a/app/model/entity/ExternalLogin.php b/app/model/entity/ExternalLogin.php index 71bd3e39c..4d6de1441 100644 --- a/app/model/entity/ExternalLogin.php +++ b/app/model/entity/ExternalLogin.php @@ -6,7 +6,8 @@ /** * @ORM\Entity - * @ORM\Table(uniqueConstraints={@ORM\UniqueConstraint(columns={"auth_service", "external_id"})}) + * @ORM\Table(uniqueConstraints={@ORM\UniqueConstraint(columns={"auth_service", "user_id"}), + * @ORM\UniqueConstraint(columns={"auth_service", "external_id"})}) */ class ExternalLogin { diff --git a/migrations/Version20250209140028.php b/migrations/Version20250209140028.php new file mode 100644 index 000000000..44a760220 --- /dev/null +++ b/migrations/Version20250209140028.php @@ -0,0 +1,31 @@ +addSql('CREATE UNIQUE INDEX UNIQ_9845B893AFB9BA21A76ED395 ON external_login (auth_service, user_id)'); + } + + public function down(Schema $schema): void + { + // this down() migration is auto-generated, please modify it to your needs + $this->addSql('DROP INDEX UNIQ_9845B893AFB9BA21A76ED395 ON external_login'); + } +} From 6f68eb8efecbf679fb2ce6f5904a9fccfa44b0e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Kruli=C5=A1?= Date: Sun, 9 Feb 2025 18:10:36 +0100 Subject: [PATCH 6/7] Adding tests for new users' external-IDs endpoints. --- app/V1Module/presenters/UsersPresenter.php | 2 + tests/Presenters/UsersPresenter.phpt | 89 ++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/app/V1Module/presenters/UsersPresenter.php b/app/V1Module/presenters/UsersPresenter.php index ba70007db..5fa7780f1 100644 --- a/app/V1Module/presenters/UsersPresenter.php +++ b/app/V1Module/presenters/UsersPresenter.php @@ -814,6 +814,7 @@ public function actionUpdateExternalLogin(string $id, string $service) } $this->externalLogins->persist($login); + $this->users->refresh($user); } $this->sendSuccessResponse($this->userViewFactory->getUser($user)); @@ -841,6 +842,7 @@ public function actionRemoveExternalLogin(string $id, string $service) $login = $this->externalLogins->findByUser($user, $service); if ($login) { $this->externalLogins->remove($login); + $this->users->refresh($user); } $this->sendSuccessResponse($this->userViewFactory->getUser($user)); } diff --git a/tests/Presenters/UsersPresenter.phpt b/tests/Presenters/UsersPresenter.phpt index 8d8ab9853..4f8c5d0aa 100644 --- a/tests/Presenters/UsersPresenter.phpt +++ b/tests/Presenters/UsersPresenter.phpt @@ -837,6 +837,95 @@ class TestUsersPresenter extends Tester\TestCase Assert::false($payload['privateData']['isAllowed']); Assert::false($this->users->getByEmail($victim)->isAllowed()); } + + public function testSetNewExternalId() + { + $victim = "user2@example.com"; + PresenterTestHelper::loginDefaultAdmin($this->container); + $user = $this->users->getByEmail($victim); + Assert::false($user->hasExternalAccounts()); + + $payload = PresenterTestHelper::performPresenterRequest( + $this->presenter, + $this->presenterPath, + 'POST', + ['action' => 'updateExternalLogin', 'id' => $user->getId(), 'service' => 'test-cas'], + ['externalId' => 'abc'] + ); + + Assert::equal($user->getId(), $payload['id']); + Assert::true($payload['privateData']['isExternal']); + + $els = $this->presenter->externalLogins->findAll(); + Assert::count(1, $els); + Assert::equal($user->getId(), $els[0]->getUser()->getId()); + Assert::equal('abc', $els[0]->getExternalId()); + Assert::equal('test-cas', $els[0]->getAuthService()); + + $this->users->refresh($user); + Assert::true($user->hasExternalAccounts()); + Assert::equal(['test-cas' => 'abc'], $user->getConsolidatedExternalLogins()); + } + + public function testUpdateExternalId() + { + $victim = "user2@example.com"; + PresenterTestHelper::loginDefaultAdmin($this->container); + $user = $this->users->getByEmail($victim); + Assert::false($user->hasExternalAccounts()); + $this->presenter->externalLogins->connect('test-cas', $user, 'old'); + $this->users->refresh($user); + Assert::equal(['test-cas' => 'old'], $user->getConsolidatedExternalLogins()); + + $payload = PresenterTestHelper::performPresenterRequest( + $this->presenter, + $this->presenterPath, + 'POST', + ['action' => 'updateExternalLogin', 'id' => $user->getId(), 'service' => 'test-cas'], + ['externalId' => 'abc'] + ); + + Assert::equal($user->getId(), $payload['id']); + Assert::true($payload['privateData']['isExternal']); + + $els = $this->presenter->externalLogins->findAll(); + Assert::count(1, $els); + Assert::equal($user->getId(), $els[0]->getUser()->getId()); + Assert::equal('abc', $els[0]->getExternalId()); + Assert::equal('test-cas', $els[0]->getAuthService()); + + $this->users->refresh($user); + Assert::true($user->hasExternalAccounts()); + Assert::equal(['test-cas' => 'abc'], $user->getConsolidatedExternalLogins()); + } + + public function testRemoveExternalId() + { + $victim = "user2@example.com"; + PresenterTestHelper::loginDefaultAdmin($this->container); + $user = $this->users->getByEmail($victim); + Assert::false($user->hasExternalAccounts()); + $this->presenter->externalLogins->connect('test-cas', $user, 'old'); + $this->users->refresh($user); + Assert::equal(['test-cas' => 'old'], $user->getConsolidatedExternalLogins()); + + $payload = PresenterTestHelper::performPresenterRequest( + $this->presenter, + $this->presenterPath, + 'DELETE', + ['action' => 'removeExternalLogin', 'id' => $user->getId(), 'service' => 'test-cas'] + ); + + Assert::equal($user->getId(), $payload['id']); + Assert::false($payload['privateData']['isExternal']); + + $els = $this->presenter->externalLogins->findAll(); + Assert::count(0, $els); + + $this->users->refresh($user); + Assert::false($user->hasExternalAccounts()); + Assert::equal([], $user->getConsolidatedExternalLogins()); + } } (new TestUsersPresenter())->run(); From 10f8c1a013dae47d2f0ea54bf43222b99969180f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Kruli=C5=A1?= Date: Sun, 9 Feb 2025 19:45:36 +0100 Subject: [PATCH 7/7] Allowing authorities that can edit external identifiers to see all external identifiers. --- app/model/view/UserViewFactory.php | 2 +- tests/Presenters/UsersPresenter.phpt | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/model/view/UserViewFactory.php b/app/model/view/UserViewFactory.php index f25d11c45..ebe1fd483 100644 --- a/app/model/view/UserViewFactory.php +++ b/app/model/view/UserViewFactory.php @@ -45,7 +45,7 @@ public function __construct(IUserPermissions $userAcl, Logins $logins, \Nette\Se */ private function getExternalIds(User $user, bool $canViewAllExternalIds = false) { - if (!$canViewAllExternalIds) { + if (!$canViewAllExternalIds && !$this->userAcl->canSetExternalIds($user)) { if (!$this->loggedInUser) { return []; } diff --git a/tests/Presenters/UsersPresenter.phpt b/tests/Presenters/UsersPresenter.phpt index 4f8c5d0aa..26f581228 100644 --- a/tests/Presenters/UsersPresenter.phpt +++ b/tests/Presenters/UsersPresenter.phpt @@ -855,6 +855,7 @@ class TestUsersPresenter extends Tester\TestCase Assert::equal($user->getId(), $payload['id']); Assert::true($payload['privateData']['isExternal']); + Assert::equal(['test-cas' => 'abc'], $payload['privateData']['externalIds']); $els = $this->presenter->externalLogins->findAll(); Assert::count(1, $els); @@ -887,6 +888,7 @@ class TestUsersPresenter extends Tester\TestCase Assert::equal($user->getId(), $payload['id']); Assert::true($payload['privateData']['isExternal']); + Assert::equal(['test-cas' => 'abc'], $payload['privateData']['externalIds']); $els = $this->presenter->externalLogins->findAll(); Assert::count(1, $els); @@ -918,6 +920,7 @@ class TestUsersPresenter extends Tester\TestCase Assert::equal($user->getId(), $payload['id']); Assert::false($payload['privateData']['isExternal']); + Assert::equal([], $payload['privateData']['externalIds']); $els = $this->presenter->externalLogins->findAll(); Assert::count(0, $els);