search

scheb / 2fa

Two-factor authentication for Symfony applications 🔐
MIT License
495 stars 72 forks source link

introduce CacheableVoterInterface to reduce amounts of calls #228

Closed kevinpapst closed 5 months ago

kevinpapst commented 5 months ago

Closes #227

Description

See #227 as well.

Adds the Symfony\Component\Security\Core\Authorization\Voter\CacheableVoterInterface which dramatically reduces the amount of calls to the 2FA specific Voter.

scheb commented 5 months ago

Looks like the integration tests really don't like it and seem to be stuck in an endless loop.

kevinpapst commented 5 months ago

I added code like that to more than a dozen Voters across multiple projects 🤷 I don't think that it has to do with my change, but who knows ....

Can you restart them and if that doesn't help: how can I run them locally?

scheb commented 5 months ago

Restarted, but it looks stuck again.

You can run the integration tests locally from the app folder. So basically cd into app, run composer install and then vendor/bin/phpunit.

See: https://github.com/scheb/2fa/tree/7.x/app

kevinpapst commented 5 months ago

Ups, missed the app directory, thanks. Will test and report back!

kevinpapst commented 5 months ago

Okay, the test showed thar the Voter receives a Request object during login as type and therefor it was stuck in a loop.

I am not familiar enough with the internals of the security system, so I don't know if any other type might be valid as well, such as the User or Token. Is it safe to only accept null and Request? We could also return true for every type and let the cache only work on the two attribute names.

Should make no big difference performance wise, as attributes are checked first anyway: https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php#L102

What do you say?

scheb commented 5 months ago

The $subject is completely irrelevant to the voter, so I believe it should just return true, because the subject can be anything. That's what Symfony does for its own cached voters, such as the https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Security/Core/Authorization/Voter/AuthenticatedVoter.php#L88-L103, which is also not doing anything with the subject.

kevinpapst commented 5 months ago

Agree 👍 changed. Thanks for you input.

scheb commented 5 months ago

Nice! Would you please rebase onto to target branch. I just solved these coding standards errors. So we should get a proper result for your PR then.

kevinpapst commented 5 months ago

done

scheb commented 5 months ago

Thank you for contributing! 🙇

Released as v7.3.0.