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

[2.3.22]AuthenticationEntryPoint does not work in Symfony 2.3.22 #12581

Closed
zacharyzh opened this issue Nov 26, 2014 · 8 comments
Closed

[2.3.22]AuthenticationEntryPoint does not work in Symfony 2.3.22 #12581

zacharyzh opened this issue Nov 26, 2014 · 8 comments

Comments

@zacharyzh
Copy link

I am not good at english and I will do my best to articulate..

Yestoday, I have upgrade my Symfony framework from 2.3.18 to 2.3.22. Everything is good, except my AuthenticationEntryPoint class.

Code here:

class AuthenticationEntryPoint implements AuthenticationEntryPointInterface
{
    private $router;

    public function __construct(Router $router)
    {
        $this->router = $router;
    }

    public function start(Request $request, AuthenticationException $exception)
    {
        if ($request->isXmlHttpRequest()) {
            return new JsonResponse(array('status' => 'login_required'));
        }
        return new RedirectResponse('/my/login');
    }
}
services:
    my_entry_point:
        class: My\Bundle\Security\AuthenticationEntryPoint
        arguments: [ "@router" ]

security:
    firewalls:
        secured:
            pattern: .*
            entry_point: my_entry_point
            form_login:
                check_path: /my/login/check

The question is, if remove form_login from security.yml, AuthenticationEntryPoint running and got this exception:

Unable to find the controller for path "/my/login/check". 
Maybe you forgot to add the matching route in your routing configuration?

if leave form_login in security.yml, AuthenticationEntryPoint does not work, and the default login_path will be used:

No route found for "GET /login"
404 Not Found - NotFoundHttpException

Thanks very much.

@zacharyzh
Copy link
Author

Changelog:

bug #12296 [SecurityBundle] Authentication entry point is only registered with 
firewall exception listener, not with authentication listeners.

This bug fixed and the code below has been removed:

# Symfony\Bundle\SecurityBundle\DependencyInjection\SecurityExtension.php @ Line 351
if (isset($firewall['entry_point'])) {
    $defaultEntryPoint = $firewall['entry_point'];
}

Does it means AuthenticationEntryPointInterface not working without access listeners ?

@zacharyzh zacharyzh changed the title AuthenticationEntryPoint does not work in Symfony 2.3.22 [2.3.22]AuthenticationEntryPoint does not work in Symfony 2.3.22 Nov 27, 2014
xabbuh referenced this issue Nov 28, 2014
…ered with firewall exception listener, not with authentication listeners (rjkip)

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #12296).

Discussion
----------

[SecurityBundle] Authentication entry point is only registered with firewall exception listener, not with authentication listeners

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | when relying on this configuration behaviour
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12261
| License       | MIT
| Doc PR        | —

See #12261.

I configured a different firewall entry point for one firewall. However, when authentication had to be performed, it still called BasicAuthenticationEntryPoint::start() instead of my service's start(). My service was instantiated, yet never used.

The issue appears to be that the entry point is registered with the firewall's exception listener, but not with the BasicAuthenticationListener. This means that when the BasicAuthenticationListener determines the user has  provided wrong credentials, BasicAuthenticationEntryPoint is still used. Only in case of an exception would my  entry point service be used.

In my opinion, this is not correct behaviour. Can someone confirm this? Are there currently tests that pertain to the `entry_point` configuration on which I can base a test?

---

Test setup:

```yaml
# security.yml
security:
    firewalls:
        api:
            pattern: ^/api/
            http_basic: ~
            entry_point: my.service
        default:
            anonymous: ~
```

Commits
-------

92c8dfb [SecurityBundle] Authentication entry point is only registered with firewall exception listener, not with authentication listeners
@imunhatep
Copy link

Same problem here on 2.5.7 (as expected). Probably if some authentication listener have it's own default entry point it will overwrite the one provided by the configuration.

@zacharyzh
Copy link
Author

I have a Kernel.EXCEPTION event listener instead of this before recovered.

@fabpot
Copy link
Member

fabpot commented Dec 2, 2014

@rjkip Can you have a look at this issue? We should probably add a test case and a fix to avoid any future regression. That's critical as it currently breaks all versions of Symfony.

@fabpot
Copy link
Member

fabpot commented Dec 2, 2014

As 2.6.1 should be released very soon, I propose to revert the PR to have more time to figure out a proper fix. What do you think? ping @symfony/deciders

@rjkip
Copy link

rjkip commented Dec 2, 2014

Hi @fabpot. Looking into this. This is not how I want my first contribution to Symfony to be remembered. 😢

@rjkip
Copy link

rjkip commented Dec 2, 2014

if leave form_login in security.yml, AuthenticationEntryPoint does not work, and the default login_path will be used:

No route found for "GET /login"
404 Not Found - NotFoundHttpException
  • @zacharyzh has configured the default entry point (that logic was moved, not removed).
  • The FormLoginFactory ignores the default entry point (unlike HttpBasicFactory and HttpDigestFactory) and registers its own. The custom entry point, thus, is never called.
  • The user, being unauthorised, is redirected to the default login route login by the FormAuthenticationEntryPoint.

This sounds logical to me thus far.

Previously, when no credentials were provided, an AuthenticationCredentialsNotFoundException was thrown by the AccessListener. The Firewall's exception listener then called the configured entry point's start() method. In this issue's case, it caused a redirect to /my/login.


[Fix]: I've added @zacharyzh's case to the functional test. Also, I've applied a fix that makes both test cases pass. It is closer to the original behaviour of SecurityExtension::createFirewall(), where the $defaultEntryPoint returned from createAuthenticationListeners() was used for the ExceptionListener, as seen here. However, the Security bundle extension being a beast, I need someone to confirm this fix.

@rjkip
Copy link

rjkip commented Dec 2, 2014

Ping @fabpot.

fabpot added a commit that referenced this issue Dec 2, 2014
…ured entry point or a default entry point (rjkip)

This PR was squashed before being merged into the 2.3 branch (closes #12811).

Discussion
----------

Configure firewall's kernel exception listener with configured entry point or a default entry point

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | when relying on buggy behaviour
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12581 #12801
| License       | MIT
| Doc PR        | —

The #12296 PR introduced a bug where the firewall's exception listener was sometimes configured with the default entry point returned by `SecurityExtension#createAuthenticationListeners()`. These changes add a regression test and make the configured entry point (if any) always take precedence over a default entry point.

If someone can confirm this fix, it would have my preference merging this over reverting #12296 for Symfony 2.6.1.

Commits
-------

b122262 Configure firewall's kernel exception listener with configured entry point or a default entry point
@fabpot fabpot closed this as completed Dec 2, 2014
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

No branches or pull requests

4 participants