mirror of
https://github.com/kevinpapst/kimai2.git
synced 2025-03-16 14:03:30 +00:00
utilize UserService for SAML (#4748)
This commit is contained in:
parent
b6c98f871d
commit
dd51c8dfba
16 changed files with 320 additions and 352 deletions
composer.lock
config
src
Command
ActivateUserCommand.phpCreateUserCommand.phpDeactivateUserCommand.phpDemoteUserCommand.phpPromoteUserCommand.php
Controller
Saml
User
tests
510
composer.lock
generated
510
composer.lock
generated
File diff suppressed because it is too large
Load diff
|
@ -100,7 +100,9 @@ services:
|
|||
# SAML Services
|
||||
# ================================================================================
|
||||
App\Saml\SamlProvider:
|
||||
arguments: ['@App\Repository\UserRepository', '@security.user.provider.concrete.kimai_internal', '@App\Configuration\SamlConfigurationInterface']
|
||||
arguments:
|
||||
$userProvider: '@security.user.provider.concrete.kimai_internal'
|
||||
$configuration: '@App\Configuration\SamlConfigurationInterface'
|
||||
|
||||
# ================================================================================
|
||||
# REPOSITORIES
|
||||
|
|
|
@ -50,7 +50,7 @@ final class ActivateUserCommand extends Command
|
|||
|
||||
if (!$user->isEnabled()) {
|
||||
$user->setEnabled(true);
|
||||
$this->userService->updateUser($user);
|
||||
$this->userService->saveUser($user);
|
||||
$io->success(sprintf('User "%s" has been activated.', $username));
|
||||
} else {
|
||||
$io->warning(sprintf('User "%s" is already active.', $username));
|
||||
|
|
|
@ -76,7 +76,7 @@ final class CreateUserCommand extends AbstractUserCommand
|
|||
}
|
||||
|
||||
try {
|
||||
$this->userService->saveNewUser($user);
|
||||
$this->userService->saveUser($user);
|
||||
$io->success(sprintf('Success! Created user: %s', $username));
|
||||
} catch (ValidationFailedException $ex) {
|
||||
$this->validationError($ex, $io);
|
||||
|
|
|
@ -50,7 +50,7 @@ final class DeactivateUserCommand extends Command
|
|||
|
||||
if ($user->isEnabled()) {
|
||||
$user->setEnabled(false);
|
||||
$this->userService->updateUser($user);
|
||||
$this->userService->saveUser($user);
|
||||
$io->success(sprintf('User "%s" has been deactivated.', $username));
|
||||
} else {
|
||||
$io->warning(sprintf('User "%s" is already deactivated.', $username));
|
||||
|
|
|
@ -39,7 +39,7 @@ final class DemoteUserCommand extends AbstractRoleCommand
|
|||
if ($super) {
|
||||
if ($user->isSuperAdmin()) {
|
||||
$user->setSuperAdmin(false);
|
||||
$manipulator->updateUser($user);
|
||||
$manipulator->saveUser($user);
|
||||
$output->success(sprintf('Super administrator role has been removed from the user "%s".', $username));
|
||||
} else {
|
||||
$output->warning(sprintf('User "%s" doesn\'t have the super administrator role.', $username));
|
||||
|
@ -47,7 +47,7 @@ final class DemoteUserCommand extends AbstractRoleCommand
|
|||
} else {
|
||||
if ($user->hasRole($role)) {
|
||||
$user->removeRole($role);
|
||||
$manipulator->updateUser($user);
|
||||
$manipulator->saveUser($user);
|
||||
$output->success(sprintf('Role "%s" has been removed from user "%s".', $role, $username));
|
||||
} else {
|
||||
$output->warning(sprintf('User "%s" didn\'t have "%s" role.', $username, $role));
|
||||
|
|
|
@ -39,7 +39,7 @@ final class PromoteUserCommand extends AbstractRoleCommand
|
|||
if ($super) {
|
||||
if (!$user->isSuperAdmin()) {
|
||||
$user->setSuperAdmin(true);
|
||||
$manipulator->updateUser($user);
|
||||
$manipulator->saveUser($user);
|
||||
$output->success(sprintf('User "%s" has been promoted as a super administrator.', $username));
|
||||
} else {
|
||||
$output->warning(sprintf('User "%s" does already have the super administrator role.', $username));
|
||||
|
@ -47,7 +47,7 @@ final class PromoteUserCommand extends AbstractRoleCommand
|
|||
} else {
|
||||
if (!$user->hasRole($role)) {
|
||||
$user->addRole($role);
|
||||
$manipulator->updateUser($user);
|
||||
$manipulator->saveUser($user);
|
||||
$output->success(sprintf('Role "%s" has been added to user "%s".', $role, $username));
|
||||
} else {
|
||||
$output->warning(sprintf('User "%s" did already have "%s" role.', $username, $role));
|
||||
|
|
|
@ -136,7 +136,7 @@ final class ProfileController extends AbstractController
|
|||
$form->handleRequest($request);
|
||||
|
||||
if ($form->isSubmitted() && $form->isValid()) {
|
||||
$userService->updateUser($profile);
|
||||
$userService->saveUser($profile);
|
||||
|
||||
$this->flashSuccess('action.update.success');
|
||||
|
||||
|
@ -409,7 +409,7 @@ final class ProfileController extends AbstractController
|
|||
{
|
||||
if (!$profile->hasTotpSecret()) {
|
||||
$profile->setTotpSecret($totpAuthenticator->generateSecret());
|
||||
$userService->updateUser($profile);
|
||||
$userService->saveUser($profile);
|
||||
}
|
||||
|
||||
$data = new TotpActivation($profile);
|
||||
|
@ -423,7 +423,7 @@ final class ProfileController extends AbstractController
|
|||
|
||||
if ($form->isSubmitted() && $form->isValid()) {
|
||||
$profile->enableTotpAuthentication();
|
||||
$userService->updateUser($profile);
|
||||
$userService->saveUser($profile);
|
||||
|
||||
$this->flashSuccess('action.update.success');
|
||||
|
||||
|
@ -473,7 +473,7 @@ final class ProfileController extends AbstractController
|
|||
|
||||
if ($form->isSubmitted() && $form->isValid()) {
|
||||
$profile->disableTotpAuthentication();
|
||||
$userService->updateUser($profile);
|
||||
$userService->saveUser($profile);
|
||||
|
||||
$this->flashSuccess('action.update.success');
|
||||
}
|
||||
|
|
|
@ -87,7 +87,7 @@ final class PasswordResetController extends AbstractController
|
|||
$this->eventDispatcher->dispatch(new EmailEvent($event->getEmail()));
|
||||
|
||||
$user->markPasswordRequested();
|
||||
$this->userService->updateUser($user);
|
||||
$this->userService->saveUser($user);
|
||||
}
|
||||
|
||||
return $this->redirectToRoute('resetting_check_email', ['username' => $username]);
|
||||
|
@ -146,7 +146,7 @@ final class PasswordResetController extends AbstractController
|
|||
$user->markPasswordResetted();
|
||||
$user->setEnabled(true);
|
||||
|
||||
$this->userService->updateUser($user);
|
||||
$this->userService->saveUser($user);
|
||||
|
||||
$response = $this->redirectToRoute('my_profile');
|
||||
$loginManager->logInUser($user, $response);
|
||||
|
|
|
@ -69,7 +69,7 @@ final class SelfRegistrationController extends AbstractController
|
|||
|
||||
$request->getSession()->set('confirmation_email_address', $user->getEmail());
|
||||
|
||||
$this->userService->saveNewUser($user);
|
||||
$this->userService->saveUser($user);
|
||||
|
||||
return $this->redirectToRoute('user_registration_check_email');
|
||||
}
|
||||
|
@ -126,7 +126,7 @@ final class SelfRegistrationController extends AbstractController
|
|||
$user->setConfirmationToken(null);
|
||||
$user->setEnabled(true);
|
||||
|
||||
$this->userService->updateUser($user);
|
||||
$this->userService->saveUser($user);
|
||||
|
||||
$response = $this->redirectToRoute('registration_confirmed');
|
||||
$loginManager->logInUser($user, $response);
|
||||
|
|
|
@ -35,7 +35,7 @@ final class WizardController extends AbstractController
|
|||
|
||||
if ($wizard === 'intro') {
|
||||
$user->setWizardAsSeen('intro');
|
||||
$userService->updateUser($user);
|
||||
$userService->saveUser($user);
|
||||
|
||||
return $this->render('wizard/intro.html.twig', [
|
||||
'percent' => 0,
|
||||
|
@ -77,7 +77,7 @@ final class WizardController extends AbstractController
|
|||
$user->setTimezone($data[UserPreference::TIMEZONE]);
|
||||
$user->setPreferenceValue(UserPreference::SKIN, $data[UserPreference::SKIN]);
|
||||
$user->setWizardAsSeen('profile');
|
||||
$userService->updateUser($user);
|
||||
$userService->saveUser($user);
|
||||
|
||||
if ($data['reload'] === '1') {
|
||||
return $this->redirectToRoute('wizard', ['wizard' => 'profile', '_locale' => $user->getLanguage()]);
|
||||
|
@ -104,7 +104,7 @@ final class WizardController extends AbstractController
|
|||
|
||||
if ($form->isSubmitted() && $form->isValid()) {
|
||||
$user->setRequiresPasswordReset(false);
|
||||
$userService->updateUser($user);
|
||||
$userService->saveUser($user);
|
||||
|
||||
return $this->redirectToRoute('wizard', ['wizard' => 'done']);
|
||||
}
|
||||
|
|
|
@ -11,7 +11,7 @@ namespace App\Saml;
|
|||
|
||||
use App\Configuration\SamlConfigurationInterface;
|
||||
use App\Entity\User;
|
||||
use App\Repository\UserRepository;
|
||||
use App\User\UserService;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Symfony\Component\Security\Core\Exception\AuthenticationException;
|
||||
use Symfony\Component\Security\Core\Exception\UserNotFoundException;
|
||||
|
@ -23,7 +23,7 @@ final class SamlProvider
|
|||
* @param UserProviderInterface<User> $userProvider
|
||||
*/
|
||||
public function __construct(
|
||||
private readonly UserRepository $repository,
|
||||
private readonly UserService $userService,
|
||||
private readonly UserProviderInterface $userProvider,
|
||||
private readonly SamlConfigurationInterface $configuration,
|
||||
private readonly LoggerInterface $logger
|
||||
|
@ -40,17 +40,17 @@ final class SamlProvider
|
|||
$user = $this->userProvider->loadUserByIdentifier($token->getUserIdentifier());
|
||||
}
|
||||
} catch (UserNotFoundException $ex) {
|
||||
$this->logger->error($ex->getMessage());
|
||||
// this is expected for new users
|
||||
$this->logger->debug('User is not existing: ' . $token->getUserIdentifier());
|
||||
}
|
||||
|
||||
try {
|
||||
if (null === $user) {
|
||||
$user = $this->createUser($token);
|
||||
} else {
|
||||
$this->hydrateUser($user, $token);
|
||||
$user = $this->userService->createNewUser();
|
||||
$user->setUserIdentifier($token->getUserIdentifier());
|
||||
}
|
||||
|
||||
$this->repository->saveUser($user);
|
||||
$this->hydrateUser($user, $token);
|
||||
$this->userService->saveUser($user);
|
||||
} catch (\Exception $ex) {
|
||||
$this->logger->error($ex->getMessage());
|
||||
throw new AuthenticationException(
|
||||
|
@ -61,19 +61,6 @@ final class SamlProvider
|
|||
return $user;
|
||||
}
|
||||
|
||||
private function createUser(SamlLoginAttributes $token): User
|
||||
{
|
||||
// Not using UserService: user settings should be set via SAML attributes
|
||||
$user = new User();
|
||||
$user->setEnabled(true);
|
||||
$user->setUserIdentifier($token->getUserIdentifier());
|
||||
$user->setPassword('');
|
||||
|
||||
$this->hydrateUser($user, $token);
|
||||
|
||||
return $user;
|
||||
}
|
||||
|
||||
private function hydrateUser(User $user, SamlLoginAttributes $token): void
|
||||
{
|
||||
$groupAttribute = $this->configuration->getRolesAttribute();
|
||||
|
@ -119,11 +106,13 @@ final class SamlProvider
|
|||
}
|
||||
}
|
||||
|
||||
// fill them after hydrating account, so they can't be overwritten
|
||||
// by the mapping attributes
|
||||
// change after hydrating account, so it can't be overwritten by mapping attributes
|
||||
if ($user->getId() === null) {
|
||||
// set a plain password to satisfy the validator
|
||||
$user->setPlainPassword(substr(bin2hex(random_bytes(100)), 0, 50));
|
||||
$user->setPassword('');
|
||||
}
|
||||
|
||||
$user->setUserIdentifier($token->getUserIdentifier());
|
||||
$user->setAuth(User::AUTH_SAML);
|
||||
}
|
||||
|
|
|
@ -37,11 +37,11 @@ class UserService
|
|||
private array $cache = [];
|
||||
|
||||
public function __construct(
|
||||
private UserRepository $repository,
|
||||
private EventDispatcherInterface $dispatcher,
|
||||
private ValidatorInterface $validator,
|
||||
private SystemConfiguration $configuration,
|
||||
private UserPasswordHasherInterface $passwordHasher
|
||||
private readonly UserRepository $repository,
|
||||
private readonly EventDispatcherInterface $dispatcher,
|
||||
private readonly ValidatorInterface $validator,
|
||||
private readonly SystemConfiguration $configuration,
|
||||
private readonly UserPasswordHasherInterface $passwordHasher
|
||||
) {
|
||||
}
|
||||
|
||||
|
@ -70,6 +70,18 @@ class UserService
|
|||
return $user;
|
||||
}
|
||||
|
||||
public function saveUser(User $user): User
|
||||
{
|
||||
if ($user->getId() === null) {
|
||||
return $this->saveNewUser($user);
|
||||
} else {
|
||||
return $this->updateUser($user);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @internal will be made private soon
|
||||
*/
|
||||
public function saveNewUser(User $user): User
|
||||
{
|
||||
if (null !== $user->getId()) {
|
||||
|
|
|
@ -9,30 +9,18 @@
|
|||
|
||||
namespace App\Tests\Form\Type;
|
||||
|
||||
class TypeTestModel
|
||||
/**
|
||||
* @extends \ArrayObject<string, mixed>
|
||||
*/
|
||||
class TypeTestModel extends \ArrayObject
|
||||
{
|
||||
private $fields = [];
|
||||
|
||||
public function __construct(array $fields = [])
|
||||
public function __set(string $name, string|int|null $value)
|
||||
{
|
||||
$this->fields = $fields;
|
||||
$this->offsetSet($name, $value);
|
||||
}
|
||||
|
||||
public function __set($name, $value)
|
||||
public function __get(string $name): mixed
|
||||
{
|
||||
if (!isset($this->fields[$name])) {
|
||||
throw new \InvalidArgumentException('Unknown field: ' . $name);
|
||||
}
|
||||
|
||||
$this->fields[$name] = $value;
|
||||
}
|
||||
|
||||
public function __get($name)
|
||||
{
|
||||
if (!isset($this->fields[$name])) {
|
||||
throw new \InvalidArgumentException('Unknown field: ' . $name);
|
||||
}
|
||||
|
||||
return $this->fields[$name];
|
||||
return $this->offsetGet($name);
|
||||
}
|
||||
}
|
||||
|
|
|
@ -11,11 +11,11 @@ namespace App\Tests\Saml;
|
|||
|
||||
use App\Configuration\SamlConfiguration;
|
||||
use App\Entity\User;
|
||||
use App\Repository\UserRepository;
|
||||
use App\Saml\SamlLoginAttributes;
|
||||
use App\Saml\SamlProvider;
|
||||
use App\Tests\Configuration\TestConfigLoader;
|
||||
use App\Tests\Mocks\SystemConfigurationFactory;
|
||||
use App\User\UserService;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Symfony\Component\Security\Core\Exception\AuthenticationException;
|
||||
|
@ -50,15 +50,14 @@ class SamlProviderTest extends TestCase
|
|||
$userProvider = $this->getMockBuilder(UserProviderInterface::class)->disableOriginalConstructor();
|
||||
$userProvider->onlyMethods(['refreshUser', 'supportsClass', 'loadUserByIdentifier']);
|
||||
$userProvider = $userProvider->getMock();
|
||||
|
||||
$repository = $this->getMockBuilder(UserRepository::class)->disableOriginalConstructor()->getMock();
|
||||
$userService = $this->getMockBuilder(UserService::class)->disableOriginalConstructor()->getMock();
|
||||
if ($user !== null) {
|
||||
$userProvider->method('loadUserByIdentifier')->willReturn($user);
|
||||
} else {
|
||||
$userProvider->method('loadUserByIdentifier')->willReturn(new User());
|
||||
}
|
||||
|
||||
$provider = new SamlProvider($repository, $userProvider, $samlConfig, $this->createMock(LoggerInterface::class));
|
||||
$provider = new SamlProvider($userService, $userProvider, $samlConfig, $this->createMock(LoggerInterface::class));
|
||||
|
||||
return $provider;
|
||||
}
|
||||
|
|
|
@ -2237,36 +2237,6 @@ parameters:
|
|||
count: 1
|
||||
path: Form/Type/QuickEntryTimesheetTypeTest.php
|
||||
|
||||
-
|
||||
message: "#^Method App\\\\Tests\\\\Form\\\\Type\\\\TypeTestModel\\:\\:__construct\\(\\) has parameter \\$fields with no value type specified in iterable type array\\.$#"
|
||||
count: 1
|
||||
path: Form/Type/TypeTestModel.php
|
||||
|
||||
-
|
||||
message: "#^Method App\\\\Tests\\\\Form\\\\Type\\\\TypeTestModel\\:\\:__get\\(\\) has no return type specified\\.$#"
|
||||
count: 1
|
||||
path: Form/Type/TypeTestModel.php
|
||||
|
||||
-
|
||||
message: "#^Method App\\\\Tests\\\\Form\\\\Type\\\\TypeTestModel\\:\\:__get\\(\\) has parameter \\$name with no type specified\\.$#"
|
||||
count: 1
|
||||
path: Form/Type/TypeTestModel.php
|
||||
|
||||
-
|
||||
message: "#^Method App\\\\Tests\\\\Form\\\\Type\\\\TypeTestModel\\:\\:__set\\(\\) has parameter \\$name with no type specified\\.$#"
|
||||
count: 1
|
||||
path: Form/Type/TypeTestModel.php
|
||||
|
||||
-
|
||||
message: "#^Method App\\\\Tests\\\\Form\\\\Type\\\\TypeTestModel\\:\\:__set\\(\\) has parameter \\$value with no type specified\\.$#"
|
||||
count: 1
|
||||
path: Form/Type/TypeTestModel.php
|
||||
|
||||
-
|
||||
message: "#^Property App\\\\Tests\\\\Form\\\\Type\\\\TypeTestModel\\:\\:\\$fields type has no value type specified in iterable type array\\.$#"
|
||||
count: 1
|
||||
path: Form/Type/TypeTestModel.php
|
||||
|
||||
-
|
||||
message: "#^Method App\\\\Tests\\\\Invoice\\\\Calculator\\\\AbstractCalculatorTest\\:\\:assertDescription\\(\\) has parameter \\$addActivity with no type specified\\.$#"
|
||||
count: 1
|
||||
|
|
Loading…
Reference in a new issue