Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass firewall ID to created tokens #25

Open
wants to merge 1 commit into
base: v2.x
Choose a base branch
from

Conversation

xelan
Copy link
Contributor

@xelan xelan commented Dec 28, 2017

The firewall ID must be passed to the UsernamePasswordToken to ensure that authentication is done correctly if there are multiple firewalls.
Having a hard-coded value prevents the AuthProvider to be used for more than one firewall.

In addition, a provider key which does not match the actual firewall ID prevents the Twig functions logout_path() and logout_url() to work correctly ("No LogoutListener found for firewall key").

Adding the firewall ID to the auth provider configuration, as it is not there by default, seemed the easiest option for now. Another way would be to pass the providerKey as an additional constructor argument of the AuthenticationProvider. However, this would change the method signature and require adaptations of the service definitions.

See Symfony\Component\Security\Core\Authentication\Provider\UserAuthenticationProvider for the implementation in the Symfony default authentication provider base class. The actual creation of the provider happens here (in the case of the DAO authentication provider).

The firewall ID must be passed to the UsernamePasswordToken (or its sub classes) to ensure that authentication is done correctly if there are multiple firewalls.
Having a hard-coded value prevents the AuthProvider to be used for more than one firewall.

In addition, a provider key which does not match the actual firewall ID prevents the Twig functions logout_path() and logout_url() to work correctly.
@xelan xelan force-pushed the bugfix/provider-key branch from 9444a06 to 811d71a Compare December 28, 2017 09:10
@ztec
Copy link
Member

ztec commented Feb 13, 2018

Your change seams straightforward. Did you test it against multiple firewall ?

@xelan
Copy link
Contributor Author

xelan commented Feb 19, 2018

Yes, I tested it with two different firewalls, where one uses the active_directory_provider and another one the in_memory provider. With the proposed PR, it works as expected.

My security.yml (with some values anonymized):

security:
    encoders:
        Symfony\Component\Security\Core\User\User: plaintext
        Riper\Security\ActiveDirectoryBundle\Security\User\AdUser: plaintext

    providers:
        in_memory:
            memory:
                users:
                    user:  { password: 'example', roles: [ 'ROLE_USER'] }
        active_directory_provider:
            id: riper.security.active.directory.user.provider

    firewalls:
        test:
            pattern: ^/test
            provider: in_memory
            form_login:
                check_path: custom_login_check_route
                login_path: custom_login_route
                require_previous_session: false
            logout:
                path:   custom_logout_route
                target: /
            anonymous: ~

        secured_area:
            pattern:    ^/
            provider: active_directory_provider
            active_directory:
                check_path: custom_login_check_route
                login_path: custom_login_route
                require_previous_session: false
            logout:
                path:   custom_logout_route
                target: /
            anonymous: ~

@xelan
Copy link
Contributor Author

xelan commented May 2, 2018

Hi @ztec!

Do you have any news on this PR? Would be great if you could review and integrate it in the next bugfix release 😄

Thank you very much, best regards
Andreas

@xelan
Copy link
Contributor Author

xelan commented Jul 25, 2018

Any news on the status of this PR?
Thanks 😄

@xelan
Copy link
Contributor Author

xelan commented Apr 23, 2019

Hi @ztec! Do you have any news concerning this PR?

Best regards
Andreas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants