From 99f04d71afd9eec7eb1233137c75e2c4012992ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20B=C3=B6hmer?= Date: Fri, 24 Feb 2023 00:12:44 +0100 Subject: [PATCH] Revert "Moved all user info updating logic into SAMLUserFactory" This reverts commit 960ee342e410bcb1672d2cefae02eaeb10ff5059. --- config/packages/security.yaml | 2 +- config/services_test.yaml | 9 +++ psalm.xml | 14 ++-- src/Entity/UserSystem/User.php | 34 +++++++++- src/Security/SamlUserFactory.php | 88 +------------------------- tests/Entity/UserSystem/UserTest.php | 36 +++++++++++ tests/Security/SamlUserFactoryTest.php | 36 ----------- translations/messages.en.xlf | 2 +- 8 files changed, 88 insertions(+), 133 deletions(-) create mode 100644 config/services_test.yaml diff --git a/config/packages/security.yaml b/config/packages/security.yaml index f03e0feb..7e0ded3c 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -33,7 +33,7 @@ security: saml: use_referer: true user_factory: saml_user_factory - persist_user: false # False as we have our own persisting mechanism in SAMLUserFactory + persist_user: true check_path: saml_acs login_path: saml_login failure_path: login diff --git a/config/services_test.yaml b/config/services_test.yaml new file mode 100644 index 00000000..e29da6fc --- /dev/null +++ b/config/services_test.yaml @@ -0,0 +1,9 @@ +# Service overrides for the test environment + +services: + saml_user_factory: + class: App\Security\SamlUserFactory + public: true + + App\Security\SamlUserFactory: + public: true \ No newline at end of file diff --git a/psalm.xml b/psalm.xml index 71c720ab..872169ac 100644 --- a/psalm.xml +++ b/psalm.xml @@ -1,13 +1,11 @@ @@ -54,4 +52,4 @@ - + diff --git a/src/Entity/UserSystem/User.php b/src/Entity/UserSystem/User.php index 4289aa0d..eddf9179 100644 --- a/src/Entity/UserSystem/User.php +++ b/src/Entity/UserSystem/User.php @@ -62,7 +62,7 @@ use Jbtronics\TFAWebauthn\Model\TwoFactorInterface as WebauthnTwoFactorInterface * @UniqueEntity("name", message="validator.user.username_already_used") */ class User extends AttachmentContainingDBElement implements UserInterface, HasPermissionsInterface, TwoFactorInterface, - BackupCodeInterface, TrustedDeviceInterface, WebauthnTwoFactorInterface, PreferredProviderInterface, PasswordAuthenticatedUserInterface + BackupCodeInterface, TrustedDeviceInterface, WebauthnTwoFactorInterface, PreferredProviderInterface, PasswordAuthenticatedUserInterface, SamlUserInterface { //use MasterAttachmentTrait; @@ -892,4 +892,36 @@ class User extends AttachmentContainingDBElement implements UserInterface, HasPe $this->saml_user = $saml_user; return $this; } + + + + public function setSamlAttributes(array $attributes) + { + //When mail attribute exists, set it + if (isset($attributes['email'])) { + $this->setEmail($attributes['email'][0]); + } + //When first name attribute exists, set it + if (isset($attributes['firstName'])) { + $this->setFirstName($attributes['firstName'][0]); + } + //When last name attribute exists, set it + if (isset($attributes['lastName'])) { + $this->setLastName($attributes['lastName'][0]); + } + if (isset($attributes['department'])) { + $this->setDepartment($attributes['department'][0]); + } + + //Use X500 attributes as userinfo + if (isset($attributes['urn:oid:2.5.4.42'])) { + $this->setFirstName($attributes['urn:oid:2.5.4.42'][0]); + } + if (isset($attributes['urn:oid:2.5.4.4'])) { + $this->setLastName($attributes['urn:oid:2.5.4.4'][0]); + } + if (isset($attributes['urn:oid:1.2.840.113549.1.9.1'])) { + $this->setEmail($attributes['urn:oid:1.2.840.113549.1.9.1'][0]); + } + } } diff --git a/src/Security/SamlUserFactory.php b/src/Security/SamlUserFactory.php index 168c4269..fd181133 100644 --- a/src/Security/SamlUserFactory.php +++ b/src/Security/SamlUserFactory.php @@ -21,29 +21,13 @@ namespace App\Security; use App\Entity\UserSystem\User; -use Doctrine\ORM\EntityManagerInterface; -use Hslavich\OneloginSamlBundle\Event\AbstractUserEvent; -use Hslavich\OneloginSamlBundle\Event\UserCreatedEvent; -use Hslavich\OneloginSamlBundle\Event\UserModifiedEvent; -use Hslavich\OneloginSamlBundle\Security\Http\Authenticator\Token\SamlToken; use Hslavich\OneloginSamlBundle\Security\User\SamlUserFactoryInterface; -use Symfony\Component\EventDispatcher\EventSubscriberInterface; -use Symfony\Component\Security\Core\Security; use Symfony\Component\Security\Core\User\UserInterface; -class SamlUserFactory implements SamlUserFactoryInterface, EventSubscriberInterface +class SamlUserFactory implements SamlUserFactoryInterface { public const SAML_PASSWORD_PLACEHOLDER = '!!SAML!!'; - private Security $security; - private EntityManagerInterface $entityManager; - - public function __construct(Security $security, EntityManagerInterface $entityManager) - { - $this->entityManager = $entityManager; - $this->security = $security; - } - public function createUser($username, array $attributes = []): UserInterface { $user = new User(); @@ -53,76 +37,8 @@ class SamlUserFactory implements SamlUserFactoryInterface, EventSubscriberInterf //This is a SAML user now! $user->setSamlUser(true); - $this->updateUserInfoFromSAMLAttributes($user, $attributes); + $user->setSamlAttributes($attributes); return $user; } - - public function updateAndPersistUser(AbstractUserEvent $event): void - { - $user = $event->getUser(); - $token = $this->security->getToken(); - - if (!$user instanceof User) { - throw new \RuntimeException('User must be an instance of '.User::class); - } - if (!$token instanceof SamlToken) { - throw new \RuntimeException('Token must be an instance of '.SamlToken::class); - } - - $attributes = $token->getAttributes(); - - //Update the user info based on the SAML attributes - $this->updateUserInfoFromSAMLAttributes($user, $attributes); - - //Persist the user - $this->entityManager->persist($user); - - //Flush the entity manager - $this->entityManager->flush(); - } - - public static function getSubscribedEvents() - { - return [ - UserCreatedEvent::class => 'updateAndPersistUser', - UserModifiedEvent::class => 'updateAndPersistUser', - ]; - } - - /** - * Sets the SAML attributes to the user. - * @param User $user - * @param array $attributes - * @return void - */ - public function updateUserInfoFromSAMLAttributes(User $user, array $attributes): void - { - //When mail attribute exists, set it - if (isset($attributes['email'])) { - $user->setEmail($attributes['email'][0]); - } - //When first name attribute exists, set it - if (isset($attributes['firstName'])) { - $user->setFirstName($attributes['firstName'][0]); - } - //When last name attribute exists, set it - if (isset($attributes['lastName'])) { - $user->setLastName($attributes['lastName'][0]); - } - if (isset($attributes['department'])) { - $user->setDepartment($attributes['department'][0]); - } - - //Use X500 attributes as userinfo - if (isset($attributes['urn:oid:2.5.4.42'])) { - $user->setFirstName($attributes['urn:oid:2.5.4.42'][0]); - } - if (isset($attributes['urn:oid:2.5.4.4'])) { - $user->setLastName($attributes['urn:oid:2.5.4.4'][0]); - } - if (isset($attributes['urn:oid:1.2.840.113549.1.9.1'])) { - $user->setEmail($attributes['urn:oid:1.2.840.113549.1.9.1'][0]); - } - } } \ No newline at end of file diff --git a/tests/Entity/UserSystem/UserTest.php b/tests/Entity/UserSystem/UserTest.php index 89aab673..456457a4 100644 --- a/tests/Entity/UserSystem/UserTest.php +++ b/tests/Entity/UserSystem/UserTest.php @@ -148,4 +148,40 @@ class UserTest extends TestCase } $this->assertFalse($user->isWebAuthnAuthenticatorEnabled()); } + + public function testSetSAMLAttributes(): void + { + $data = [ + 'firstName' => ['John'], + 'lastName' => ['Doe'], + 'email' => ['j.doe@invalid.invalid'], + 'department' => ['Test Department'], + ]; + + $user = new User(); + $user->setSAMLAttributes($data); + + //Test if the data was set correctly + $this->assertSame('John', $user->getFirstName()); + $this->assertSame('Doe', $user->getLastName()); + $this->assertSame('j.doe@invalid.invalid', $user->getEmail()); + $this->assertSame('Test Department', $user->getDepartment()); + + //Test that it works for X500 attributes + $data = [ + 'urn:oid:2.5.4.42' => ['Jane'], + 'urn:oid:2.5.4.4' => ['Dane'], + 'urn:oid:1.2.840.113549.1.9.1' => ['mail@invalid.invalid'], + ]; + + $user->setSAMLAttributes($data); + + //Data must be changed + $this->assertSame('Jane', $user->getFirstName()); + $this->assertSame('Dane', $user->getLastName()); + $this->assertSame('mail@invalid.invalid', $user->getEmail()); + + //Department must not be changed + $this->assertSame('Test Department', $user->getDepartment()); + } } diff --git a/tests/Security/SamlUserFactoryTest.php b/tests/Security/SamlUserFactoryTest.php index 77fd6ab9..6127237a 100644 --- a/tests/Security/SamlUserFactoryTest.php +++ b/tests/Security/SamlUserFactoryTest.php @@ -62,40 +62,4 @@ class SamlUserFactoryTest extends WebTestCase $this->assertEquals('IT', $user->getDepartment()); $this->assertEquals('j.doe@invalid.invalid', $user->getEmail()); } - - public function testUpdateUserInfoFromSAMLAttributes(): void - { - $data = [ - 'firstName' => ['John'], - 'lastName' => ['Doe'], - 'email' => ['j.doe@invalid.invalid'], - 'department' => ['Test Department'], - ]; - - $user = new User(); - $this->service->updateUserInfoFromSAMLAttributes($user, $data); - - //Test if the data was set correctly - $this->assertSame('John', $user->getFirstName()); - $this->assertSame('Doe', $user->getLastName()); - $this->assertSame('j.doe@invalid.invalid', $user->getEmail()); - $this->assertSame('Test Department', $user->getDepartment()); - - //Test that it works for X500 attributes - $data = [ - 'urn:oid:2.5.4.42' => ['Jane'], - 'urn:oid:2.5.4.4' => ['Dane'], - 'urn:oid:1.2.840.113549.1.9.1' => ['mail@invalid.invalid'], - ]; - - $this->service->updateUserInfoFromSAMLAttributes($user, $data); - - //Data must be changed - $this->assertSame('Jane', $user->getFirstName()); - $this->assertSame('Dane', $user->getLastName()); - $this->assertSame('mail@invalid.invalid', $user->getEmail()); - - //Department must not be changed - $this->assertSame('Test Department', $user->getDepartment()); - } } diff --git a/translations/messages.en.xlf b/translations/messages.en.xlf index ae66d98c..34be1ce5 100644 --- a/translations/messages.en.xlf +++ b/translations/messages.en.xlf @@ -10990,7 +10990,7 @@ Element 3 login.local_login_hint - The form below is only for log in with a local user. If you want to log in via single sign-on, press the button above. + The form below is only for log in for a local user. If you want to log in via single sign-on, press the button above.