From 683f61c9884d1621d2a54e7b918b10001f8243ad Mon Sep 17 00:00:00 2001 From: Malik <3097625+dermalikmann@users.noreply.github.com> Date: Mon, 13 May 2024 10:42:39 +0200 Subject: [PATCH 01/11] use guard check in createUserIfNotExists() Signed-off-by: Malik <3097625+dermalikmann@users.noreply.github.com> --- lib/UserBackend.php | 54 ++++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/lib/UserBackend.php b/lib/UserBackend.php index 98dd1e02f..84bd8b95b 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -103,41 +103,41 @@ protected function userExistsInDatabase(string $uid): bool { * @param array $attributes */ public function createUserIfNotExists(string $uid, array $attributes = []): void { - if (!$this->userExistsInDatabase($uid)) { - $values = [ - 'uid' => $uid, - ]; - - // Try to get the mapped home directory of the user - try { - $home = $this->getAttributeValue('saml-attribute-mapping-home_mapping', $attributes); - } catch (\InvalidArgumentException) { - $home = ''; - } + if ($this->userExistsInDatabase($uid)) return; + + $values = [ + 'uid' => $uid, + ]; + + // Try to get the mapped home directory of the user + try { + $home = $this->getAttributeValue('saml-attribute-mapping-home_mapping', $attributes); + } catch (\InvalidArgumentException) { + $home = ''; + } - if ($home !== '') { - //if attribute's value is an absolute path take this, otherwise append it to data dir - //check for / at the beginning or pattern c:\ resp. c:/ + if ($home !== '') { + //if attribute's value is an absolute path take this, otherwise append it to data dir + //check for / at the beginning or pattern c:\ resp. c:/ if ($home[0] !== '/' && !(strlen($home) > 3 && ctype_alpha($home[0]) && $home[1] === ':' && ($home[2] === '\\' || $home[2] === '/')) - ) { - $home = $this->config->getSystemValue('datadirectory', - \OC::$SERVERROOT.'/data') . '/' . $home; - } - - $values['home'] = $home; + ) { + $home = $this->config->getSystemValue('datadirectory', + \OC::$SERVERROOT.'/data') . '/' . $home; } - $qb = $this->db->getQueryBuilder(); - $qb->insert('user_saml_users'); - foreach ($values as $column => $value) { - $qb->setValue($column, $qb->createNamedParameter($value)); - } - $qb->execute(); + $values['home'] = $home; + } - $this->initializeHomeDir($uid); + $qb = $this->db->getQueryBuilder(); + $qb->insert('user_saml_users'); + foreach ($values as $column => $value) { + $qb->setValue($column, $qb->createNamedParameter($value)); } + $qb->execute(); + + $this->initializeHomeDir($uid); } /** From 4a466e984c02ad657e98efde90e322d0dbc6ddff Mon Sep 17 00:00:00 2001 From: Malik <3097625+dermalikmann@users.noreply.github.com> Date: Mon, 13 May 2024 14:10:02 +0200 Subject: [PATCH 02/11] Added UserLoggedOutEvent Signed-off-by: Malik <3097625+dermalikmann@users.noreply.github.com> --- lib/Controller/SAMLController.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index 984cc6603..d9e590655 100644 --- a/lib/Controller/SAMLController.php +++ b/lib/Controller/SAMLController.php @@ -425,6 +425,8 @@ public function assertionConsumerService(): Http\RedirectResponse { * @throws Error */ public function singleLogoutService(): Http\RedirectResponse { + $user = $this->userResolver->findExistingUser($this->userBackend->getCurrentUserId()); + $isFromGS = ($this->config->getSystemValue('gs.enabled', false) && $this->config->getSystemValue('gss.mode', '') === 'master'); @@ -486,10 +488,13 @@ public function singleLogoutService(): Http\RedirectResponse { } catch (Error $e) { $this->logger->warning($e->getMessage(), ['exception' => $e, 'app' => $this->appName]); $this->userSession->logout(); + $this->eventDispatcher->dispatchTyped(new UserLoggedOutEvent($user)); } } + if (!empty($targetUrl) && !$auth->getLastErrorReason()) { $this->userSession->logout(); + $this->eventDispatcher->dispatchTyped(new UserLoggedOutEvent($user)); } } if (empty($targetUrl)) { From f2eed53eb9941127d1b3090ffc88158cbb3907d7 Mon Sep 17 00:00:00 2001 From: Malik <3097625+dermalikmann@users.noreply.github.com> Date: Mon, 13 May 2024 14:10:24 +0200 Subject: [PATCH 03/11] Added UserLoggedInEvent Signed-off-by: Malik <3097625+dermalikmann@users.noreply.github.com> --- lib/Controller/SAMLController.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index d9e590655..b06b28080 100644 --- a/lib/Controller/SAMLController.php +++ b/lib/Controller/SAMLController.php @@ -413,6 +413,8 @@ public function assertionConsumerService(): Http\RedirectResponse { $response->addCookie('_shibsession_', 'authenticated'); } + $this->eventDispatcher->dispatchTyped(new UserLoggedInEvent($user, $user->getUID(), null, false)); + $response->invalidateCookie('saml_data'); return $response; } From 4e7dd201f984409686f8b39995a4e941d08b47c2 Mon Sep 17 00:00:00 2001 From: Malik <3097625+dermalikmann@users.noreply.github.com> Date: Mon, 13 May 2024 14:11:30 +0200 Subject: [PATCH 04/11] Forgot to commit includes&dep-injection Signed-off-by: Malik <3097625+dermalikmann@users.noreply.github.com> --- lib/Controller/SAMLController.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index b06b28080..16c020152 100644 --- a/lib/Controller/SAMLController.php +++ b/lib/Controller/SAMLController.php @@ -20,6 +20,7 @@ use OCA\User_SAML\UserResolver; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IL10N; use OCP\IRequest; @@ -29,6 +30,8 @@ use OCP\Security\ICrypto; use OCP\Security\ITrustedDomainHelper; use OCP\Server; +use OCP\User\Events\UserLoggedInEvent; +use OCP\User\Events\UserLoggedOutEvent; use OneLogin\Saml2\Auth; use OneLogin\Saml2\Error; use OneLogin\Saml2\Settings; @@ -63,6 +66,7 @@ class SAMLController extends Controller { */ private $crypto; private ITrustedDomainHelper $trustedDomainHelper; + private IEventDispatcher $eventDispatcher; public function __construct( string $appName, @@ -78,7 +82,8 @@ public function __construct( UserResolver $userResolver, UserData $userData, ICrypto $crypto, - ITrustedDomainHelper $trustedDomainHelper + ITrustedDomainHelper $trustedDomainHelper, + IEventDispatcher $eventDispatcher ) { parent::__construct($appName, $request); $this->session = $session; @@ -93,6 +98,7 @@ public function __construct( $this->userData = $userData; $this->crypto = $crypto; $this->trustedDomainHelper = $trustedDomainHelper; + $this->eventDispatcher = $eventDispatcher; } /** From ac763088dce1d5667c3d415c04b3aea2358a50ad Mon Sep 17 00:00:00 2001 From: Malik <3097625+dermalikmann@users.noreply.github.com> Date: Mon, 13 May 2024 14:13:03 +0200 Subject: [PATCH 05/11] Added UserDeletedEvent Signed-off-by: Malik <3097625+dermalikmann@users.noreply.github.com> --- lib/UserBackend.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/UserBackend.php b/lib/UserBackend.php index 84bd8b95b..34ce3251f 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -25,6 +25,8 @@ use OCP\User\Backend\IGetHomeBackend; use OCP\User\Events\UserChangedEvent; use OCP\User\Events\UserFirstTimeLoggedInEvent; +use OCP\User\Events\UserCreatedEvent; +use OCP\User\Events\UserDeletedEvent; use OCP\UserInterface; use Psr\Log\LoggerInterface; @@ -169,6 +171,10 @@ public function deleteUser($uid) { $affected = $qb->delete('user_saml_users') ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid))) ->executeStatement(); + + $user = $this->userManager->get($uid); + $this->eventDispatcher->dispatchTyped(new UserDeletedEvent($user)); + return $affected > 0; } From 88cfe38a1e40f10117509c74ccfa6f475954f7ad Mon Sep 17 00:00:00 2001 From: Malik Mann Date: Wed, 2 Oct 2024 15:23:27 +0200 Subject: [PATCH 06/11] Fixed braces formatting in lib/UserBackend.php Co-authored-by: Arthur Schiwon Signed-off-by: Malik Mann --- lib/UserBackend.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/UserBackend.php b/lib/UserBackend.php index 34ce3251f..e993a3776 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -105,7 +105,9 @@ protected function userExistsInDatabase(string $uid): bool { * @param array $attributes */ public function createUserIfNotExists(string $uid, array $attributes = []): void { - if ($this->userExistsInDatabase($uid)) return; + if ($this->userExistsInDatabase($uid)) { + return; + } $values = [ 'uid' => $uid, From d61bcd3d6753716bd03ca4acc2b19cee4ad1fc91 Mon Sep 17 00:00:00 2001 From: Malik <3097625+dermalikmann@users.noreply.github.com> Date: Wed, 2 Oct 2024 15:55:19 +0200 Subject: [PATCH 07/11] Edited login test to expect UserLoggedInEvent Signed-off-by: Malik <3097625+dermalikmann@users.noreply.github.com> --- tests/unit/Controller/SAMLControllerTest.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/unit/Controller/SAMLControllerTest.php b/tests/unit/Controller/SAMLControllerTest.php index 3987ac125..d59df9ba6 100644 --- a/tests/unit/Controller/SAMLControllerTest.php +++ b/tests/unit/Controller/SAMLControllerTest.php @@ -16,6 +16,7 @@ use OCA\User_SAML\UserResolver; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IL10N; use OCP\IRequest; @@ -57,6 +58,7 @@ class SAMLControllerTest extends TestCase { /** @var SAMLController */ private $samlController; private ITrustedDomainHelper|MockObject $trustedDomainController; + private IEventDispatcher $eventDispatcher; protected function setUp(): void { parent::setUp(); @@ -74,6 +76,7 @@ protected function setUp(): void { $this->userData = $this->createMock(UserData::class); $this->crypto = $this->createMock(ICrypto::class); $this->trustedDomainController = $this->createMock(ITrustedDomainHelper::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->l->expects($this->any())->method('t')->willReturnCallback( function ($param) { @@ -321,6 +324,11 @@ public function testLoginWithEnvVariable(array $samlUserData, string $redirect, ->method('createUserIfNotExists') ->with('MyUid'); + $this->eventDispatcher + ->expects($this->once()) + ->method('dispatchTyped') + ->with(new UserLoggedInEvent($user, 'MyUid', null, false)); + $expected = new RedirectResponse($redirect); $result = $this->samlController->login(1); $this->assertEquals($expected, $result); From f4588df619cc507f48394087f9517476160ff30c Mon Sep 17 00:00:00 2001 From: Malik <3097625+dermalikmann@users.noreply.github.com> Date: Wed, 2 Oct 2024 16:11:44 +0200 Subject: [PATCH 08/11] style(PHP): fix code style Signed-off-by: Malik <3097625+dermalikmann@users.noreply.github.com> --- lib/UserBackend.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/UserBackend.php b/lib/UserBackend.php index e993a3776..a7dfd1d0d 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -123,9 +123,9 @@ public function createUserIfNotExists(string $uid, array $attributes = []): void if ($home !== '') { //if attribute's value is an absolute path take this, otherwise append it to data dir //check for / at the beginning or pattern c:\ resp. c:/ - if ($home[0] !== '/' - && !(strlen($home) > 3 && ctype_alpha($home[0]) - && $home[1] === ':' && ($home[2] === '\\' || $home[2] === '/')) + if ($home[0] !== '/' + && !(strlen($home) > 3 && ctype_alpha($home[0]) + && $home[1] === ':' && ($home[2] === '\\' || $home[2] === '/')) ) { $home = $this->config->getSystemValue('datadirectory', \OC::$SERVERROOT.'/data') . '/' . $home; From de999387c27a2fc458025e510daa88313866af20 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 4 Oct 2024 20:19:44 +0200 Subject: [PATCH 09/11] style(PHP): remove unnecessary import Signed-off-by: Arthur Schiwon --- lib/UserBackend.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/UserBackend.php b/lib/UserBackend.php index a7dfd1d0d..2a0e921a0 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -24,9 +24,8 @@ use OCP\User\Backend\IGetDisplayNameBackend; use OCP\User\Backend\IGetHomeBackend; use OCP\User\Events\UserChangedEvent; -use OCP\User\Events\UserFirstTimeLoggedInEvent; -use OCP\User\Events\UserCreatedEvent; use OCP\User\Events\UserDeletedEvent; +use OCP\User\Events\UserFirstTimeLoggedInEvent; use OCP\UserInterface; use Psr\Log\LoggerInterface; From e1e993181205393fcbab05e57f7fd5e783f8f7d0 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 4 Oct 2024 20:32:12 +0200 Subject: [PATCH 10/11] tests(Unit): fix instantion of UserBackend - also add missing import Signed-off-by: Arthur Schiwon --- tests/unit/Controller/SAMLControllerTest.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/Controller/SAMLControllerTest.php b/tests/unit/Controller/SAMLControllerTest.php index d59df9ba6..773061db7 100644 --- a/tests/unit/Controller/SAMLControllerTest.php +++ b/tests/unit/Controller/SAMLControllerTest.php @@ -26,6 +26,7 @@ use OCP\IUserSession; use OCP\Security\ICrypto; use OCP\Security\ITrustedDomainHelper; +use OCP\User\Events\UserLoggedInEvent; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -103,7 +104,8 @@ function ($param) { $this->userResolver, $this->userData, $this->crypto, - $this->trustedDomainController + $this->trustedDomainController, + $this->eventDispatcher, ); } From e476537a7461d60873f3ce4c4ccfd7127847e320 Mon Sep 17 00:00:00 2001 From: Malik <3097625+dermalikmann@users.noreply.github.com> Date: Sun, 6 Oct 2024 13:16:49 +0200 Subject: [PATCH 11/11] added UserLoggedInEvent to env login Signed-off-by: Malik <3097625+dermalikmann@users.noreply.github.com> --- lib/Controller/SAMLController.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index 16c020152..6824b7218 100644 --- a/lib/Controller/SAMLController.php +++ b/lib/Controller/SAMLController.php @@ -228,12 +228,15 @@ public function login(int $idp = 1): Http\RedirectResponse { $response->addCookie('saml_data', $data, null, 'None'); break; + + case 'environment-variable': $ssoUrl = $originalUrl; if (empty($ssoUrl)) { $ssoUrl = $this->urlGenerator->getAbsoluteURL('/'); } $this->session->set('user_saml.samlUserData', $_SERVER); + try { $this->userData->setAttributes($this->session->get('user_saml.samlUserData')); $this->autoprovisionIfPossible(); @@ -241,12 +244,15 @@ public function login(int $idp = 1): Http\RedirectResponse { $firstLogin = $user->updateLastLoginTimestamp(); if ($firstLogin) { $this->userBackend->initializeHomeDir($user->getUID()); - } + } + $this->eventDispatcher->dispatchTyped(new UserLoggedInEvent($user, $user->getUID(), null, false)); + } catch (NoUserFoundException $e) { if ($e->getMessage()) { $this->logger->warning('Error while trying to login using sso environment variable: ' . $e->getMessage(), ['app' => 'user_saml']); } $ssoUrl = $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notProvisioned'); + } catch (UserFilterViolationException $e) { $this->logger->info( 'SAML filter constraints not met: {msg}', @@ -257,11 +263,14 @@ public function login(int $idp = 1): Http\RedirectResponse { ); $ssoUrl = $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notPermitted'); } + $response = new Http\RedirectResponse($ssoUrl); if (isset($e)) { $this->session->clear(); } break; + + default: throw new \Exception( sprintf( @@ -373,11 +382,13 @@ public function assertionConsumerService(): Http\RedirectResponse { try { $this->userData->setAttributes($auth->getAttributes()); $this->autoprovisionIfPossible(); + } catch (NoUserFoundException $e) { $this->logger->error($e->getMessage(), ['app' => $this->appName]); $response = new Http\RedirectResponse($this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notProvisioned')); $response->invalidateCookie('saml_data'); return $response; + } catch (UserFilterViolationException $e) { $this->logger->error($e->getMessage(), ['app' => $this->appName]); $response = new Http\RedirectResponse($this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notPermitted'));