lightSAML / SpBundle

SAML2 SP Symfony Bundle based on LightSAML
https://www.lightsaml.com/SP-Bundle/
MIT License
66 stars 70 forks source link

UserCreator->createUser() should never return null #19

Closed ChMat closed 7 years ago

ChMat commented 8 years ago

Hi,

I am working on the integration of lightSAMLSpBundle for some applications in our company. We have set things up so as to use only one identity provider.

If you want to filter out some users at the time of authentication, the UserCreator->createUser() function should never return null. Contrary to the UserCreatorInterface documentation, returning null in the context of a delegated authentication will create a loop redirect between the identity provider and your Symfony application.

Since UserCreator->createUser() is called in the context of a Symfony AuthenticationProviderInterface, returning null will throw an AuthenticationException and Symfony is going to redirect the user to the login form. But the login form is located at the identity provider and it just told Symfony that the guy was a legitimate user. From there, the loop is created.

Therefore, your application should always trust the SSO service and create the user even if you don't like him.

But, if you still want to deny access to your application, you should just not give him any roles. Here is a modified UserCreator class modified from the example in the Getting Started guide:

<?php
// src/AppBundle/Security/User/UserCreator.php
namespace AppBundle\Security\User;

use AppBundle\Entity\User;
use Doctrine\Common\Persistence\ObjectManager;
use LightSaml\Model\Protocol\Response;
use LightSaml\SpBundle\Security\User\UserCreatorInterface;
use LightSaml\SpBundle\Security\User\UsernameMapperInterface;
use Symfony\Component\Security\Core\User\UserInterface;

class UserCreator implements UserCreatorInterface
{
    /** @var ObjectManager */
    private $objectManager;

    /** @var UsernameMapperInterface */
    private $usernameMapper;

    /**
     * @param ObjectManager           $objectManager
     * @param UsernameMapperInterface $usernameMapper
     */
    public function __construct($objectManager, $usernameMapper)
    {
        $this->objectManager = $objectManager;
        $this->usernameMapper = $usernameMapper;
    }

    /**
     * @param Response $response
     *
     * @return UserInterface|null
     */
    public function createUser(Response $response)
    {
        $username = $this->usernameMapper->getUsername($response);

        $user = new User();

        $user->setUsername($username);

        if ($iWantUserIn)
        {
            $user->setRoles(['ROLE_USER']);
        }
        else
        {
            // I know User but I don't want him in right now. 
            // But one day, may be I can grant him some roles.
        }

        $this->objectManager->persist($user);
        $this->objectManager->flush();

        return $user;
    }
}

Not sure if this is an issue. Perhaps this is more a subject for a cookbook?

I hope this will help.

tmilos commented 8 years ago

That's interesting idea, and I like it, but I'm not sure if all users of the bundle will take it that way. Anyway, you have user_creator configuration option in the security firewall to provide you own logic. https://github.com/lightSAML/SpBundle/blob/master/src/LightSaml/SpBundle/DependencyInjection/Security/Factory/LightSamlSpFactory.php#L29

Don't have that documented in http://www.lightsaml.com/SP-Bundle/Configuration/ Should be added.

Thanks

INSEAD-asim commented 8 years ago

Hi @ChMat, I am having similar issue. Let me try your solution.

@tmilos , as I mentioned in other issue, setting user_creator as null keep looping the system in SP and IDP.

tmilos commented 7 years ago

Think that this is overcome with #39, where default user will be created w/out roles. Released in v1.1.0