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

introduce CacheableVoterInterface to reduce amounts of calls #228

Merged
merged 5 commits into from
Apr 20, 2024

Conversation

kevinpapst
Copy link
Contributor

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
Copy link
Owner

scheb commented Apr 19, 2024

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

@kevinpapst
Copy link
Contributor Author

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
Copy link
Owner

scheb commented Apr 19, 2024

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
Copy link
Contributor Author

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

@kevinpapst
Copy link
Contributor Author

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
Copy link
Owner

scheb commented Apr 19, 2024

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
Copy link
Contributor Author

Agree 👍 changed. Thanks for you input.

@scheb
Copy link
Owner

scheb commented Apr 19, 2024

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
Copy link
Contributor Author

done

@scheb scheb merged commit 0649ea5 into scheb:7.x Apr 20, 2024
6 of 7 checks passed
@scheb
Copy link
Owner

scheb commented Apr 20, 2024

Thank you for contributing! 🙇

Released as v7.3.0.

@kevinpapst kevinpapst deleted the cacheable-voter branch April 20, 2024 17:11
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.

Increase performance by using CacheableVoterInterface
2 participants