Skip to content

[Security][SecurityBundle] Implement ADM strategies as dedicated classes #42177

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 29, 2021
Merged

[Security][SecurityBundle] Implement ADM strategies as dedicated classes #42177

merged 1 commit into from
Oct 29, 2021

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Jul 18, 2021 •
edited
Loading

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Fix #42095
License MIT
Doc PR symfony/symfony-docs#16034

We currently allow to replace the access decision manager with a fully custom implementation. However, if the app makes use of that feature just because it needs a custom decision strategy, it also has to take care of wiring the voters into the custom implementation. This is a bit cumbersome.

This PR introduces a new interface AccessDecisionStrategyInterface that allows the app to implement a custom strategy that can be plugged into the default access decision manager. All built-in strategies have been migrated to that new interface.

Furthermore, a new option is added to SecurityBundle to leverage this feature:

security:
    access_decision_manager:
        strategy_service: app.custom_access_decision_strategy

The "old" way of configuring the strategy will continue to work:

security:
    access_decision_manager:
        strategy: unanimous
        allow_if_all_abstain: true
        allow_if_equal_granted_denied: false

In that case, SecurityBundle will wire the new strategy classes for us, so we don't have to change anything in our app when upgrading to Symfony 5.4.

Additionally, I decided to finalize AccessDecisionManager because once the strategies are pluggable, there shouldn't be a use-case left for extending the class that could not be covered with a decorator.

Implementing a custom strategy is straightforward. Your class implenting AccessDecisionStrategyInterface will receive a generator of voter results and is expected to return its decision as a boolean. This way, the interaction with the individual voters is abstracted away from the strategy and the strategy can stop the voter iteration whenever the decision is final.

/**
 * Always picks the third voter.
 */
class ThirdVoterStrategy implements AccessDecisionStrategyInterface
{
    public function decide(\Traversable $results): bool
    {
        $votes = 0;
        foreach ($results as $result) {
            if (++$votes === 3) {
                return $result === VoterInterface::ACCESS_GRANTED;
            }
        }

        return false;
    }
}

@derrabus derrabus requested review from chalasr and wouterj as code owners July 18, 2021 18:46
@carsonbot carsonbot changed the title [Security] Implement ADM strategies as dedicated classes [Security][SecurityBundle] Implement ADM strategies as dedicated classes Jul 18, 2021
@derrabus derrabus mentioned this pull request Jul 18, 2021
Closed
@derrabus derrabus force-pushed the feature/adm-strategies branch from 26b22a5 to abe1c15 Compare July 18, 2021 18:49
@derrabus derrabus added this to the 5.4 milestone Jul 18, 2021
@derrabus derrabus force-pushed the feature/adm-strategies branch 4 times, most recently from 1524d5b to 069864f Compare July 18, 2021 23:04
@carsonbot
Copy link

Hey!

I think @aschempp has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@aschempp
Copy link
Contributor

I wonder if you could simply reuse the strategy property for the service name instead of an additional strategy_service property? Since there are 4 known (old) options, if one of these is given then use the "old" one, otherwise expect it to be a service. Otherwise, should strategy be deprecated?

@derrabus
Copy link
Member Author

I wonder if you could simply reuse the strategy property for the service name instead of an additional strategy_service property? Since there are 4 known (old) options, if one of these is given then use the "old" one, otherwise expect it to be a service.

Yeah, I thought about that too. But that'll give you very cryptic error messages if want to select one of the core strategies and make a typo.

Otherwise, should strategy be deprecated?

I wouldn't do that. As long as you don't want to specify your own custom strategy, the DX is better with the old set of options.

@derrabus derrabus force-pushed the feature/adm-strategies branch from 069864f to 1e8af86 Compare July 27, 2021 17:04
@aschempp
Copy link
Contributor

I just looked into the issue with a custom access decision manager. As far as I see, using a custom access decision strategy seems like an edge case only relevant in an app context, would you agree to that? If you simply replace the security.access.decision_manager with your own class (instead of decorating or configuring the service), wouldn't that basically give you the same result, same constructor arguments etc. to implement a custom strategy?

@derrabus
Copy link
Member Author

@aschempp Yes, you can basically hack this with a small compiler pass. This PR creates an official extension point for strategies and provides you with tools to test them.

@derrabus derrabus force-pushed the feature/adm-strategies branch from 1e8af86 to 5733fa5 Compare September 20, 2021 11:56
chalasr
chalasr reviewed Oct 24, 2021
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Some comments, cosmetic only

@derrabus derrabus force-pushed the feature/adm-strategies branch 2 times, most recently from f70b38a to d2b8351 Compare October 29, 2021 08:39
@derrabus
Copy link
Member Author

I've addressed all comments from @chalasr and fixed conflict's coming from @jderusse's #43066. PR is ready again.

@derrabus
[Security] Implement ADM strategies as dedicated classes
d25956a 
Signed-off-by: Alexander M. Turek <me@derrabus.de>
@derrabus derrabus force-pushed the feature/adm-strategies branch from d2b8351 to d25956a Compare October 29, 2021 08:57
@fabpot
Copy link
Member

fabpot commented Oct 29, 2021

Thank you @derrabus.

@fabpot fabpot mentioned this pull request Oct 29, 2021
Closed
@fabpot fabpot merged commit f4d96a6 into symfony:5.4 Oct 29, 2021
@derrabus derrabus deleted the feature/adm-strategies branch October 29, 2021 12:30
@derrabus derrabus mentioned this pull request Oct 29, 2021
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 29, 2021
@javiereguiluz
minor #16034 [Security] Document custom access decision managers (der…
d6f676a 
…rabus)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Document custom access decision managers

Fixes #16032
Documents symfony/symfony#42177

Commits
-------

04ce795 Document custom access decision managers
This was referenced Nov 5, 2021
Merged
Merged
acrobat added a commit to acrobat/core-bundle that referenced this pull request Oct 4, 2023
@acrobat
Fix AccessDecisionManager deprecation
0dd1c8f 
Resolves deprecations added in symfony 5.4 (symfony/symfony#42177)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@aschempp aschempp aschempp left review comments

@fabpot fabpot fabpot approved these changes

@chalasr chalasr chalasr approved these changes

@wouterj wouterj Awaiting requested review from wouterj

Assignees
No one assigned
Labels
Deprecation Feature Security SecurityBundle Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

Custom AccessDecisionManager not getting list of voters as first argument in __construct
6 participants
@derrabus @carsonbot @aschempp @fabpot @stof @chalasr