EP/myep-team
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: stop MyE&P from granting roles and matching the wrong account

Browse Source
This commit is contained in:
Björn Fromme
2026-08-12 17:55:26 +02:00
parent 450f5b0515
commit 3b116add75
2 changed files with 613 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Security/MyEpAuthenticator.php
+219 -44
View File
@@ -5,9 +5,11 @@ namespace App\Security;
use App\BusProNet\UserDataHandler;
use App\Entity\Teamer;
use App\Entity\User;
use App\RequiredTeamerCheck\RequiredTeamerCheckRegistry;
use App\Security\OAuth2\AuthorizationRequestException;
use App\Security\OAuth2\MyEpClient;
use Doctrine\ORM\EntityManagerInterface;
use Flagception\Manager\FeatureManagerInterface;
use League\OAuth2\Client\Provider\Exception\IdentityProviderException;
use Psr\Http\Client\ClientExceptionInterface;
use Psr\Log\LoggerInterface;
@@ -26,22 +28,35 @@ use Symfony\Component\Security\Http\Util\TargetPathTrait;
class MyEpAuthenticator extends AbstractAuthenticator
{
private const ELIGIBLE_ROLES = ['ROLE_ADMIN', 'ROLE_TEAMER', 'ROLE_MANAGER', 'ROLE_HOUSE_MANAGER'];
use TargetPathTrait;
/**
* The roles that entitle someone to log in here at all. Anything else MyE&P reports
* is dropped rather than stored, so that no role this application assigns a meaning
* to can be set from the outside.
*/
private const ELIGIBLE_ROLES = ['ROLE_ADMIN', 'ROLE_TEAMER', 'ROLE_MANAGER', 'ROLE_HOUSE_MANAGER'];
public function __construct(
private readonly MyEpClient $client,
private readonly UserDataHandler $userDataHandler,
private readonly EntityManagerInterface $entityManager,
private readonly UrlGeneratorInterface $urlGenerator,
private readonly LoggerInterface $logger,
private readonly FeatureManagerInterface $featureManager,
private readonly RequiredTeamerCheckRegistry $requiredTeamerCheckRegistry,
) {
}
public function supports(Request $request): ?bool
{
return 'app_myep_auth_check' === $request->attributes->get('_route');
if ('app_myep_auth_check' !== $request->attributes->get('_route')) {
return false;
}
// the controller guards the route as well, but the authenticator must not fire
// on a disabled feature either: it is what creates and updates accounts
return $this->featureManager->isActive('myep_login');
}
public function authenticate(Request $request): Passport
@@ -58,30 +73,44 @@ class MyEpAuthenticator extends AbstractAuthenticator
try {
$provider = $this->client->getProvider();
$url = $provider->getResourceOwnerDetailsUrl($accessToken);
$request = $provider->getAuthenticatedRequest('GET', $url, $accessToken, [
$userinfoRequest = $provider->getAuthenticatedRequest('GET', $url, $accessToken, [
'headers' => [
'Accept' => 'application/json',
'Content-Type' => 'application/json',
],
]);
$response = $provider->getHttpClient()->sendRequest($request);
$response = $provider->getHttpClient()->sendRequest($userinfoRequest);
} catch (ClientExceptionInterface $e) {
$this->logger->error('Login via MyE&P failed due to unexpected userinfo response');
throw new CustomUserMessageAuthenticationException('Login via MyE&P nicht möglich');
}
$userinfo = json_decode($response->getBody(), true);
$username = $userinfo['email'] ?? null;
if (null === $username) {
$userinfo = json_decode($response->getBody(), true);
if (false === is_array($userinfo)) {
$this->logger->error('Login via MyE&P failed due to unreadable userinfo payload');
throw new CustomUserMessageAuthenticationException('Login via MyE&P nicht möglich');
}
if (null === ($userinfo['email'] ?? null)) {
$this->logger->error('Login via MyE&P failed due to missing username claim');
throw new CustomUserMessageAuthenticationException('Login via MyE&P nicht möglich');
}
if (null !== $this->createOrUpdateUserFromUserinfo($userinfo)) {
return new SelfValidatingPassport(new UserBadge($username));
$user = $this->createOrUpdateUserFromUserinfo($userinfo);
if (null === $user) {
throw new CustomUserMessageAuthenticationException('Login via MyE&P nicht möglich');
}
throw new CustomUserMessageAuthenticationException('Login via MyE&P nicht möglich');
// The user is resolved here rather than by the user provider: the account is
// matched on the BusPro ids, which the provider cannot do, and handing it an
// email would risk loading a different row than the one just written. The
// UserChecker still runs and still refuses deleted, disabled and unapproved
// accounts.
return new SelfValidatingPassport(
new UserBadge($user->getUserIdentifier(), static fn (): User => $user),
);
}
public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response
@@ -89,13 +118,26 @@ class MyEpAuthenticator extends AbstractAuthenticator
/** @var User $user */
$user = $token->getUser();
// recorded here rather than while importing, so that only a login that actually
// passed the UserChecker is counted as one
$user->setLastLoginAt(new \DateTimeImmutable());
$this->entityManager->flush();
$this->logger->info('Logged in');
if ($targetPath = $this->getTargetPath($request->getSession(), $firewallName)) {
return new RedirectResponse($targetPath);
}
$url = $this->urlGenerator->generate($user->getDefaultRoute());
$route = $user->getDefaultRoute();
if ('app_teamer_index' === $route) {
$check = $this->requiredTeamerCheckRegistry->getFirstUnresolvedCheck($user);
if (null !== $check) {
$route = $check->getRouteName();
}
}
$url = $this->urlGenerator->generate($route);
return new RedirectResponse($url);
}
@@ -111,57 +153,185 @@ class MyEpAuthenticator extends AbstractAuthenticator
private function createOrUpdateUserFromUserinfo(array $userinfo): ?User
{
// User is expected to have at least one role
if (false === isset($userinfo['roles']) || 0 === count($userinfo['roles'])) {
// Claim keys only, the payload itself carries the full profile
$this->logger->warning('Login via MyE&P failed due to missing roles claim', [
'claims' => array_keys($userinfo),
]);
$claimedRoles = $this->collectClaimedRoles($userinfo);
if ([] === $claimedRoles) {
return null;
}
// User is expected to have at least one of the roles teamer, manager, house manager or admin
if ([] === array_intersect(self::ELIGIBLE_ROLES, $userinfo['roles'])) {
$this->logger->warning('Login via MyE&P failed due to lack of an eligible role', [
'roles' => $userinfo['roles'],
'eligible_roles' => self::ELIGIBLE_ROLES,
$isTeamer = in_array('ROLE_TEAMER', $claimedRoles, true);
$pendingRoles = $this->userDataHandler->toPendingRoles($claimedRoles);
$user = $this->findLocalUser($userinfo);
// A deleted account is excluded from every process, and MyE&P must not be able to
// undo that: no data is written back, no role is granted or revoked, not even
// lastLoginAt is bumped. It is returned untouched so the UserChecker can refuse
// the login and say why. Only an admin restores it.
if (true === $user?->isDeleted()) {
$this->logger->info('Skip MyE&P sync for deleted user', [
'user_id' => $user->getId(),
'user_email' => $userinfo['email'],
]);
return null;
return $user;
}
// Check if user is already present in local database
$user = $this
->entityManager
->getRepository(User::class)
->findOneBy(['email' => $userinfo['email']])
;
// Update existing user's roles and teamer data and return it
// Update existing user, leaving granted roles and hotel codes alone: they are
// imported once on creation and managed manually afterwards. Only the
// privilege-free markers and ROLE_TEAMER track MyE&P on every login.
if (null !== $user) {
$user
->setRoles($userinfo['roles'])
->setHotelCodes($userinfo['profile']['hotel_codes'])
->setLastLoginAt(new \DateTimeImmutable('now'))
;
$this->refreshBusProIds($user, $userinfo);
$this->userDataHandler->refreshPendingRoles($user, $pendingRoles);
if (true === $isTeamer) {
$this->userDataHandler->grantTeamerRole($user);
}
$this->entityManager->flush();
return $user;
}
return $this->createLocalUser($userinfo, $pendingRoles, $isTeamer);
}
/**
* The roles MyE&P grants this person in this application.
*
* The remote list is filtered down to the eligible roles rather than trusted as it
* stands: anything else the identity provider reports is meaningless here at best,
* and at worst a role this application assigns a meaning to - ROLE_ADMINISTRATIVE
* from the role hierarchy, say - which must never be settable from the outside.
*
* @return string[]
*/
private function collectClaimedRoles(array $userinfo): array
{
$roles = $userinfo['roles'] ?? [];
// User is expected to have at least one role
if (false === is_array($roles) || [] === $roles) {
// Claim keys only, the payload itself carries the full profile
$this->logger->warning('Login via MyE&P failed due to missing roles claim', [
'claims' => array_keys($userinfo),
]);
return [];
}
// User is expected to have at least one of the roles teamer, manager, house manager or admin
$claimedRoles = array_values(array_intersect(self::ELIGIBLE_ROLES, $roles));
if ([] === $claimedRoles) {
$this->logger->warning('Login via MyE&P failed due to lack of an eligible role', [
'roles' => $roles,
'eligible_roles' => self::ELIGIBLE_ROLES,
]);
}
return $claimedRoles;
}
/**
* Matches the local account on the BusPro ids first and falls back to the email, the
* same precedence UserDataHandler::findLocalUser() applies to a BusPro login, so that
* the two login paths cannot create two accounts for the same person.
*/
private function findLocalUser(array $userinfo): ?User
{
$repository = $this->entityManager->getRepository(User::class);
$addressId = $userinfo['address_id'] ?? null;
$personId = $userinfo['person_id'] ?? null;
if (null !== $addressId && null !== $personId) {
$user = $repository->findOneBy([
'busProAddressId' => $addressId,
'busProPersonId' => $personId,
]);
if (null !== $user) {
return $user;
}
}
$users = $repository->findBy(['email' => $userinfo['email']]);
if (1 !== count($users)) {
if (1 < count($users)) {
$this->logger->warning('Unable to match local user by email: multiple users found', [
'email' => $userinfo['email'],
'count' => count($users),
]);
}
return null;
}
return $users[0];
}
/**
* Backfills the BusPro ids on an account matched by email, so the next login matches
* on the ids instead. Never called for a deleted user, who must not be written to.
*/
private function refreshBusProIds(User $user, array $userinfo): void
{
$addressId = $userinfo['address_id'] ?? null;
$personId = $userinfo['person_id'] ?? null;
if (null === $addressId || null === $personId) {
$this->logger->warning('Skip BusPro ID refresh: userinfo missing IDs', [
'user_id' => $user->getId(),
'user_email' => $user->getEmail(),
'bus_pro_address_id' => $addressId,
'bus_pro_person_id' => $personId,
]);
return;
}
$user
->setBusProAddressId((int) $addressId)
->setBusProPersonId((int) $personId)
;
}
/**
* @param string[] $pendingRoles
*/
private function createLocalUser(array $userinfo, array $pendingRoles, bool $isTeamer): ?User
{
$addressId = $userinfo['address_id'] ?? null;
$personId = $userinfo['person_id'] ?? null;
// Both are required columns and there is nothing sensible to fall back to, so an
// account is not created at all rather than half-created from a partial payload
if (null === $addressId || null === $personId) {
$this->logger->warning('Login via MyE&P failed due to missing BusPro ID claims', [
'email' => $userinfo['email'],
'claims' => array_keys($userinfo),
]);
return null;
}
$profile = is_array($userinfo['profile'] ?? null) ? $userinfo['profile'] : [];
$hotelCodes = is_array($profile['hotel_codes'] ?? null) ? $profile['hotel_codes'] : [];
$user = new User();
$user
->setFirstName($userinfo['profile']['first_name'])
->setLastName($userinfo['profile']['last_name'])
->setEmail($userinfo['profile']['communication']['email'])
->setBusProPersonId($userinfo['id'])
->setHotelCodes($userinfo['profile']['hotel_codes'])
->setRoles($userinfo['roles'])
->setFirstName($profile['first_name'] ?? null)
->setLastName($profile['last_name'] ?? null)
->setEmail((string) $userinfo['email'])
->setBusProAddressId((int) $addressId)
->setBusProPersonId((int) $personId)
->setHotelCodes($hotelCodes)
->setRoles([...$pendingRoles, ...($isTeamer ? ['ROLE_TEAMER'] : [])])
;
if (true === in_array('ROLE_TEAMER', $user->getRoles(), true)) {
if (true === $isTeamer) {
$teamer = Teamer::fromUserinfo($userinfo);
$user->setTeamer($teamer);
@@ -176,6 +346,11 @@ class MyEpAuthenticator extends AbstractAuthenticator
$this->entityManager->persist($user);
$this->entityManager->flush();
$this->logger->info('Create user', [
'user_id' => $user->getId(),
'user_email' => $user->getEmail(),
]);
return $user;
}
}