hslavich / OneloginSamlBundle

OneLogin SAML Bundle for Symfony
MIT License
149 stars 94 forks source link

Persisting users to database not working properly #134

Closed pixelfantasy closed 3 years ago

pixelfantasy commented 4 years ago

Hello, i am using this bundle in my project and by now i am able to authenticate properly with my idp through saml. But i soon recognized, that my user is never persisted to database and started investigating. My actual application setup is:

The problem seems to be located in the SamlUserProvider.php

` public function __construct($userClass, array $defaultRoles) { $this->userClass = $userClass; $this->defaultRoles = $defaultRoles; }

public function loadUserByUsername($username)
{
    return new $this->userClass($username, $this->defaultRoles);
}

` The loadUserByUsername() function always generates an empty user-object instead of trying to load the user from database as expected. This leads to the following problem in the SamlAuthenticator.php in the createPassport() function, which is responsible for persisting the new users and checking if user object already exists in database.

` protected function createPassport(): PassportInterface { $attributes = $this->extractAttributes(); dump($attributes); $username = $this->extractUsername($attributes); dump($username);

    try {
       // At this point the SamlUserProvider.php is called and it always returns a valid user-entity
       // UsernameNotFoundException will never be thrown and generateUser() never be triggered
        $user = $this->userProvider->loadUserByUsername($username);

       // This my actual workaround for testing purposes which forces the exception
        if ($user->getUsername() === null) {
            throw new UsernameNotFoundException();
        }
    } catch (UsernameNotFoundException $exception) {
        // This exception will never be thrown because of the behaviour in SamlUserProvider.php
        if (!$this->userFactory instanceof SamlUserFactoryInterface) {
            throw $exception;
        }
        // This will persist the user to database
        $user = $this->generateUser($username, $attributes);
    } catch (\Throwable $exception) {
        throw new AuthenticationException('The authentication failed.', 0, $exception);
    }

    if ($user instanceof SamlUserInterface) {
        dump("set setSamlAttributes");
        $user->setSamlAttributes($attributes);
    }

    return new SamlPassport($user, $attributes);
}

` Am i missing something? Only by adding my workaround in the vendor-directory i could get it working. But maybe only the documentation is not up-to-date and some class-implementation is missing? I already recognized that the usage of SamlUserProvider.php is marked as deprecated in the service.php. Any help would be appreciated :-)

pixelfantasy commented 4 years ago

I think i figured something out. The problem lies in the new security feature of symfony 5.1 or maybe the implementation of it. I am using 2 different authenticators and because of this i added 2 user-providers.

    providers:
        app_db_provider:
            entity:
                class: 'App\Entity\AppUser'
                property: username
                manager_name: default

        saml_provider:
            saml:
                user_class: 'App\Entity\AppUser'
                default_roles: ['ROLE_MYLINKS_USER']

The second user-provider matches your documentation. But now I added app_db_provider instead of saml_provider to my saml-firewall as provider and now it works! User-data now is persisted and even the second error of trying to write duplicate entries to database of already existing user-entry is gone. So i am coming to the conclusing that the implementation of the new symfony security feature is buggy.

a-menshchikov commented 3 years ago

@pixelfantasy hello. SamlUserProvider was never deprecated and is using in case when you only need to instantiate a user object. If you need to persist a user, you can use saml.user_factory option (see https://github.com/hslavich/OneloginSamlBundle#just-in-time-user-provisioning-optional for details).

crbanman commented 3 years ago

I've come across the same issue as @pixelfantasy while on Symfony 5.1.6, and the same solution worked.

SamlUserProvider was never deprecated and is using in case when you only need to instantiate a user object. If you need to persist a user, you can use saml.user_factory option (see https://github.com/hslavich/OneloginSamlBundle#just-in-time-user-provisioning-optional for details).

@a-menshchikov even following the documentation I was still ending up with an empty user and nothing persisted to the database. Using an entity provider instead of a SAML provider, however, works and users are properly persisted and appear to be SAML authenticated.

a-menshchikov commented 3 years ago

@crbanman thank you for comment. Yes, you are right (I made mistake in the previous message). Usage of SamlUserProvider prevent user persistence and in case when you need it you should use any user provider that will throw UsernameNotFoundException (EntityUserProvider e.g.).

I will be grateful if you propose PR to readme that better explains this logic.