Can we get a Token Factory back?

[pluk77]

I am using the new SamlAuthenticator in Symfony 5.3 which works wonderful, but I see that the token factory has been removed from the new code.

In legacy mode, we could provide our custom token factory which allowed us to take more control over what went into the token. For example, I would like to use one of the token attributes to determine the users roles, which could be done if we can inject our own token factory.

Are you open to a PR for bringing a token factory back?

namespace Hslavich\OneloginSamlBundle\Security\Http\Authenticator\Token

interface SamlTokenFactoryInterface
{
    /**
     * Creates a new SAML Token object.
     *
     * @param SamlPassportInterface $passport
     * @param string $firewallName
     *
     * @return SamlTokenInterface
     */
    public function createToken(SamlPassportInterface $passport, string $firewallName): SamlTokenInterface;

}

class SamlTokenFactory implements SamlTokenFactoryInterface
{
    public function createToken(SamlPassportInterface $passport, string $firewallName): SamlTokenInterface
    {        
        return new SamlToken($passport->getUser(), $firewallName, $passport->getUser()->getRoles(), $passport->getAttributes());
    }

}

SamlAuthenticator to receive a tokenFactory as a setting via secutity.yaml with the SamlTokenFactory as the default:

public function createAuthenticatedToken(PassportInterface $passport, string $firewallName): TokenInterface
    {
        if (!$passport instanceof SamlPassportInterface) {
            throw new LogicException(sprintf('Passport should be an instance of "%s".', SamlPassportInterface::class));
        }

        return $this->tokenFactory->createToken($passport, $firewallName);
    }

[a-menshchikov]

Hello Marcel. You don't need TokenFactory for this purpose. You have at least 2 ways to achieve it:

• make custom UserFactory;
• if your User class implements our SamlUserInterface, you can set user roles during the setSamlAttributes() method call.

If these solutions don't work for you, please describe your case more.

[pluk77]

The problem I have with using a custom UserFactory, is that the factory does not get the token attributes when retrieving the user by identifier. Our users come from a database, where the roles are also stored.

$user = $this->userProvider->loadUserByIdentifier($identifier);

The problem with the setSamlAttributes() is that it is called directly on the user entity, which means I can not inject other services used when determining the additional roles. eg I need the entity manager registry, which I really do not want to inject into the user entity constructor and use the entity as a service.

$user->setSamlAttributes($attributes);

Perhaps I am overlooking something?

Here is the token factory I created. The roles are assigned to a user as well as a facility and when users log in, they have to select one of their assigned facilities (In the IDP, so it is passed to the application via the SML token attribute seeAlso).

When retrieving the roles for a user, we combine the roles assigned to the facility the user choose during login with those assigned to the user, so a user inherits permissions from the location they are working at (this is used to switch functionality on/off depending on where the user is working at).

The additional attributes (like facility name) set in the token are used in the UI so we can retrieve the location name, id and uuid directly from the token instead of having to retrieve the facility entity on every request.

If for whatever reason the attribute in the saml token is not set or can not be used to retrieve the facility, or the application does not have this facility in its database, we do not give the user any permissions as it means he is not allowed touse that facility in this specific application.

public function __construct(ManagerRegistry $registry, $facilityProperty = 'id')
    {
        $this->registry = $registry;
        $this->facilityProperty = $facilityProperty;
    }

    public function createToken(SamlPassportInterface $passport, string $firewallName): SamlTokenInterface
    {        
        $attributes = $passport->getAttributes();
        if(isset($attributes['seeAlso'][0])) {
            $repository = $this->registry->getManagerForClass(Facility::class)->getRepository(Facility::class);
            $facility = $repository->findOneBy(array($this->facilityProperty => $attributes['seeAlso'][0]));

            if($facility) {
                $attributes[self::ATTRIBUTE_FACILITY_NAME] = $facility->__toString();
                $attributes[self::ATTRIBUTE_FACILITY_ID] = $facility->getId();
                $attributes[self::ATTRIBUTE_FACILITY_UUID] = $facility->getUuid();

                return new SamlToken($passport->getUser(), $firewallName, $passport->getUser()->getRoles($facility->getId()), $attributes);
            } 
        } 

        return new SamlToken($passport->getUser(), $firewallName, [], $attributes);
    }

[a-menshchikov]

Maybe making of custom SamlAuthenticator (or decorating itself) could solve your issue?

[pluk77]

I did consider those options as well, but it does not seem to be that easy, especially when trying keep the ability to configure a custom Authenticator via options in security.yaml while maintaining the rest of your library and code.

A few lines of code in your library would solve my problem completely and could benefit others as well.

What is your biggest reservation with implementing a tokenFactory? It will add additional customization without much overhead. I will supply you the PR, so it should not take too much of your time.

[a-menshchikov]

I try to follow Occam's Razor principle in this case and don't to add any things without certain necessary. Let's try to figure out with existing ways. There is one else that you can use: make UserModifiedEvent listener and set roles by it according the attributes were set in setSamlAttributes method. This approach definitely easier that custom Authenticator.

[pluk77]

I agree with you that one should not add unnecessary code. Will give the listener a try tomorrow or later this evening and let you know. It might indeed work for setting the roles based on attributes after first adding them in the user and than using the listener to check for the facility. Although it seems like a long way around and not as nice as having direct control over the token creation itself. Also not 100% sure what will happen when the user gets refreshed?

One problem I do foresee is that the attributes of the token come from the passport, which come directly from the saml token, so I can not add any custom attributes to the token (like facility name), which I could do in a custom token factory.

Thanks for your assistance so far. It really is appreciated!

[pluk77]

Did some testing and had some success, but not enough :-(

I could set the attributes on the user by way of implementing the SamlUserInterface and use the attributes in the EventSubscriber to check and set the facility. Therefore the roles were extracted correctly when the token was generated.

However, as I am not persisting the samlAttributes on the user as the user should not be modified by the application, the attributes are no longer on the user entity after it has been re-loaded by the entity user provider.

This means I am still left with no way to modify / add to the attributes on the token itself and no way to keep the current facility or additional attributes on the user object in the token because it is reloaded from the database.

Only other option I can think of is creating my own userProvider which modifies the re-fresh method somehow to re-add the attributes on the user.

All and all a lot of work compared to injecting a custom TokenFactory. Can you please re-consider accepting a PR for a TokenFactory?

[2021-10-06T20:52:20.905341+02:00] security.DEBUG: User was reloaded from a user provider. {"provider":"Symfony\\Bridge\\Doctrine\\Security\\User\\EntityUserProvider","username":"admin"} []

User: (AbstractReadOnlyUser is my read-only entity)

class User extends AbstractReadOnlyUser implements SamlUserInterface
{
    private $currentFacility;
    private $samlAttributes;

    public function setSamlAttributes(array $attributes)
    {
        $this->samlAttributes = $attributes;
    }

    public function getSamlAttributes()
    {
        return $this->samlAttributes;
    }

    public function setCurrentFacility($facility)
    {
        $this->currentFacility = $facility;
    }

    public function getCurrentfacility()
    {
        return $this->currentFacility;
    }
    public function getRoles(): array
    {
        return parent::getRoles($this->currentFacility);
    }
}

eventSubscriber:

   public function setCurrentFacility(UserModifiedEvent $event)
    {
        $attributes = $event->getUser()->getSamlAttributes();
        $this->logger->debug('SamlSubscriber:UserModifiedEvent', [$event]);

        if(isset($attributes['seeAlso'][0])) {
            $repository = $this->emr->getManagerForClass(Facility::class)->getRepository(Facility::class);
            $facility = $repository->findOneBy(array('id' => $attributes['seeAlso'][0]));

            if($facility) {
                $attributes[self::ATTRIBUTE_FACILITY_NAME] = $facility->__toString();
                $attributes[self::ATTRIBUTE_FACILITY_ID] = $facility->getId();
                $attributes[self::ATTRIBUTE_FACILITY_UUID] = $facility->getUuid();

                $event->getUser()->setCurrentFacility($facility->getId());

                $this->logger->debug('SamlSubscriber:UserModifiedEvent:setSamlAttributes', [$attributes]);
                $event->getUser()->setSamlAttributes($attributes);
            } 
        } 
    }

[a-menshchikov]

Although it seems like a long way around and not as nice as having direct control over the token creation itself. I see here 2 things:

1. On the one hand you can make custom event listener for handle SAML attributes from user and set appropriate roles, and on the other hand we can create new factory class in the bundle, and nonetheless after that you'll still need to make your custom factory class to create token with appropriate roles. Yes, put attributes into user for subsequent handle looks some tricky, but it's not scary.
2. If you need to add some extra attributes to SamlToken, that looks like you need some custom token class (because as originally planned SamlToken contains only attributes from SAML response).

In combination with your case explanation, I'm still sure that custom Authenticator is better decision for you (than introducing of new SamlTokenFactory class). IIUC you need to pass some list of user roles without set them into user property (because in opposite way user will be deauthorized by ContextListener during refreshUser call). But it seems very specific for me and I don't see any advantages of custom TokenFactory over custom Authenticator.

Maybe I overlooked some important things. If you still sure that TokenFactory is better than custom Authenticator, you can open PR and we'll continue our discussion there. Anyway we will come to applicable solution that will satisfy all of us. It's not in my interest to make you suffer by using our bundle. =)

[pluk77]

I had another look at the low-level AuthenticatorManager code and thought I found a solution:

// get the passport from the Authenticator
$passport = $authenticator->authenticate($request);

// check the passport (e.g. password checking)
$event = new CheckPassportEvent($authenticator, $passport);
$this->eventDispatcher->dispatch($event);

Create a subscriber to the CheckPassportEvent and query the attributes and add attributes as required. I can also deal with the user as that is in the passport as well, and the SamlAuthenticator will use the passport to extract the roles and attributes to create the SamlToken.

public function checkPassport(CheckPassportEvent $event)
    {
        $authenticator = $event->getAuthenticator();
        if($authenticator instanceof SamlAuthenticator) {
            $passport = $event->getPassport();
            $seeAlso = $passport->getAttribute('seeAlso', null);

            if(isset($seeAlso[0])) {
                $repository = $this->emr->getManagerForClass(Facility::class)->getRepository(Facility::class);
                $facility = $repository->findOneBy(array('id' => $seeAlso[0]));
                if($facility) {
                    $passport->setAttribute(self::ATTRIBUTE_FACILITY_NAME, $facility->__toString());
                    $passport->setAttribute(self::ATTRIBUTE_FACILITY_ID, $facility->getId());
                    $passport->setAttribute(self::ATTRIBUTE_FACILITY_UUID, $facility->getUuid());
                }
            }
        }

Only problem now is scope. It seems that you have overloaded the $attributes property in your SamlToken because there is no other way to get all attributes out.

class SamlPassport extends SelfValidatingPassport implements SamlPassportInterface
{
    private $attributes;

    public function __construct(UserBadge $userBadge, array $attributes, array $badges = [])
    {
        parent::__construct($userBadge, $badges);

        $this->attributes = $attributes;
    }

    public function getAttributes(): array
    {
        return $this->attributes;
    }
}

namespace Symfony\Component\Security\Http\Authenticator\Passport;

class Passport implements UserPassportInterface
{
    use PassportTrait;

    protected $user;

    private $attributes = [];

    /**
     * @param CredentialsInterface $credentials the credentials to check for this authentication, use
     *                                          SelfValidatingPassport if no credentials should be checked
     * @param BadgeInterface[]     $badges
     */
    public function __construct(UserBadge $userBadge, CredentialsInterface $credentials, array $badges = [])
    {
        $this->addBadge($userBadge);
        $this->addBadge($credentials);
        foreach ($badges as $badge) {
            $this->addBadge($badge);
        }
    }

    /**
     * {@inheritdoc}
     */
    public function getUser(): UserInterface
    {
        if (null === $this->user) {
            if (!$this->hasBadge(UserBadge::class)) {
                throw new \LogicException('Cannot get the Security user, no username or UserBadge configured for this passport.');
            }

            $this->user = $this->getBadge(UserBadge::class)->getUser();
        }

        return $this->user;
    }

    /**
     * @param mixed $value
     */
    public function setAttribute(string $name, $value): void
    {
        $this->attributes[$name] = $value;
    }

    /**
     * @param mixed $default
     *
     * @return mixed
     */
    public function getAttribute(string $name, $default = null)
    {
        return $this->attributes[$name] ?? $default;
    }
}

I am not sure what the reasoning is for not having a getAttributes() and setAttributes() method in the underlying Passport class, but I think adding those 2 methods there would prevent you from having to declare it in your SamlToken and would allow me to modify the attributes in a subscriber.

Next is for me to see if I can get those added to the Passport class so you do not have to overload them, so I can use a subscriber to modify the attributes.

Do you have a direct line to Wouter that created the Passport class? ;-)

[a-menshchikov]

CheckPassportEvent — is really good solution, I've absolutely forgotten about it. But I don't understand what's wrong with Passport and why you need setAttributes method.

[pluk77]

If i use the setAttribute() of Passport to add my new attributes to the passport, (the scope is private) , they will not end up in the attributes property that is defined in your samlPassport. And as you use getAttributes() in your authenticator to get your private attributes my newly added attributes will not end up in the token.

Once getAttributes() and setAttributes() are added to the Passport, you do not need to redefine $attributes or getAttributes() in samlPassport and you can use setAttributes() in the construnctor to set them on Passport.

Without setAttributes() in Passport, you can not set the attributes in the samlPassport constructor as $attributes is private in Passport?

Hope that explains it?

[a-menshchikov]

Now it's clear, thank you. To solve our problem we need the only getAttributes method in base Passport class. Attributes passed into our SamlPassport constructor we can iterate as key:value and pass into setAttribute.

[pluk77]

I thought about that and tested it as well, but as we can achieve the same result it does not make sense to make our life's more complicated than necessary so rather have the setAttributes() as well. On top of that it would allow for the clearing of all attributes in a subscriber by passing an empty array, although I do not need that functionality (yet) . Let's see if my PR gets accepted and take it from there. One way or another we will find a solution...

[pluk77]

@a-menshchikov Thank you for expressing your support on my Symfony PR.