Skip to content

Symfony authenticator's no longer called with anonymous: lazy #34614

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

Closed
weaverryan opened this issue Nov 25, 2019 · 7 comments
Closed

Symfony authenticator's no longer called with anonymous: lazy #34614

weaverryan opened this issue Nov 25, 2019 · 7 comments
Milestone

Comments

@weaverryan
Copy link
Member

Symfony version(s) affected: 4.4.0

Description
Until now, if you have a Guard authenticator, its supports() method will be called on every request. However, with anonymous: lazy under your firewall, that no longer happens.

Maybe that's intended? If it is, I think anonymous: lazy can't be used as the default config in the security.yaml recipe, as it creates a very different experience.

How to reproduce
Reproducer: https://github.com/weaverryan/symfony-anonymous-lazy-reproducer

Basically, create an authenticator on a new project and access any URL. It will not be executed (because anonymous: lazy is the default setting in the recipe).

Possible Solution
I'm not sure if this is a bug with anonymous: lazy or if we should revert the recipe back to anonymous: true.

Additional context
Caught by MakerBundle tests :)

@nicolas-grekas
Copy link
Member

What's the issue in terms of real-world use case? Here is the answer about lazy: disabling laziness will make HTTP caches incompatible with firewalls again. Not sure this is what we want.

@weaverryan
Copy link
Member Author

weaverryan commented Nov 25, 2019

What's the issue in terms of real-world use case?

I think a normal login form won't work anymore. When the user does a POST /login_check, if I understand correctly, there is nothing that will cause the authenticator to be called. The request will continue to the controller, which will just render the login form again. This is effectively how MakerBundle tests noticed the change.

@nicolas-grekas
Copy link
Member

The request will continue to the controller, which will just render the login form again.

Interesting. Does the reproducer above showcase this? Could you update it to do so otherwise please?

@weaverryan
Copy link
Member Author

weaverryan commented Nov 25, 2019

It does:

git clone git@github.com:weaverryan/symfony-anonymous-lazy-reproducer.git
cd symfony-anonymous-lazy-reproducer
composer install
symfony serve -d
curl https://127.0.0.1:8000

When you try this, you'll hit DefaultController::index() (the homepage controller). But you should NOT, because AppCustomAuthenticator::supports() has a die() statement in it. Change to anonymous: true and try the curl request again and you'll hit the supports() method in the authenticator.

Let me know if it's not clear - I'm not very educated about the anonymous: lazy yet - so apologies.

@nicolas-grekas
Copy link
Member

I'd prefer a real guard: the die is just a symptom, I'd prefer not fixing the symptom without understanding the full lifecycle.

@dunglas
Copy link
Member

dunglas commented Nov 25, 2019

I've the same problem in an app I'm upgrading:

            json_login:
                check_path: login
                success_handler: Dunglas\UserBundle\Handler\JsonLoginSuccessHandler
    public function testBadLogin(): void
    {
        $client = static::createClient();
        $response = $client->request('POST',
            '/login',
            (new HttpOptions())->setJson([
                'username' => 'dunglas@gmail.com',
                'password' => 'badPassword',
            ])->toArray()
        );
        $this->assertSame(401, $response->getStatusCode()); // Now, it's a 404
    }

@weaverryan
Copy link
Member Author

Fair enough :). Let's try this again:

git clone git@github.com:weaverryan/symfony-anonymous-lazy-reproducer.git
cd symfony-anonymous-lazy-reproducer
composer install
symfony serve -d

This now has a standard login form (from make:auth). Go to /login and fill out the form. You will see the login form again with no errors. This is because the LoginFormAuthenticator is never called.

If you (as you know) even do one thing on your page to access auth (even an app.user reference in the template), it WILL trigger the system. So now I understand that this is the desired behavior of the anonymous: lazy system. I'm still worried the behavior is too strange. Here's another problematic example: OAuth. When an OAuth server redirects back to my site, I typically have an authenticator for that URL that fetches the access token, does work, then redirects. That authenticator will no longer be called.

@fabpot fabpot closed this as completed Nov 30, 2019
fabpot added a commit that referenced this issue Nov 30, 2019
…ey "support" the request (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Security/Http] call auth listeners/guards eagerly when they "support" the request

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34614, Fix #34679
| License       | MIT
| Doc PR        | -

This fixes the form authenticator linked to #34614.
Since laziness is here to provide compatibility with HTTP caching, it should be disabled when the request cannot be cached.

Tests don't pass yet, but I'm on the path to something here.

The PR now introduces a new `AbstractListener` that splits the handling logic in two:
- `supports(Request): ?bool` is always called eagerly and tells whether the listener matches the request for an earger call or a lazy call
- `authenticate(RequestEvent)` does the rest of the job when `supports()` allows so - lazily or not depending on the return value of `supports()`.

Of course, this remains compatible with non-lazy logics, see `AbstractListener::__invoke()`.

Commits
-------

b20ebe6 [Security/Http] call auth listeners/guards eagerly when they "support" the request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants