Skip to content

[Security] Be able to know the reasons of the denied access #40711

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
Closed
wants to merge 2 commits into from
Closed

[Security] Be able to know the reasons of the denied access #40711

wants to merge 2 commits into from

Conversation

azjezz
Copy link
Contributor

@azjezz azjezz commented Apr 6, 2021

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Fix #27995, #26343, #35592
License MIT
Doc PR n/a

This is a PR continuing the work started by @maidmaid and @noniagriconomie in #35592

About the voters, the gif of the the original issue sums up the situation well :

no

Currently, the voters can only say yes or no. This PR allows to give a reason from the voters.

Before :

class PostVoter extends Voter
{
    // ...

    protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
    {
        if ($subject->getAuthor() !== $token->getUser()) {
            return false;
        }

        if ($subject->getStatus() !== 'draft') {
            return false;
        }

        return true;
    }
}

After:

class PostVoter extends Voter
{
    // ...

    protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
    {
        if ($subject->getAuthor() !== $token->getUser()) {
            return $this->deny('You are not the author.')
        }

        if ($subject->getStatus() !== 'draft') {
            return $this->deny('The post is no more a draft.')
        }

        return $this->grant();
    }
}

A voter returns no more an int based on the constant ACCESS_{GRANTED,ABSTAIN,DENIED} but a Vote object which contains the access result and the reason. Thus, the security panel of the profiler can display the reason of each vote.

security

The access decision manager returns no more a bool for the final verdict but an AccessDecision object which contains the final verdict and all the votes. Thus, the 403 response can display all the denied raisons.

security-ex

TODO:

  • add BC Layer
  • migrate existing voters
  • update old tests
  • add new tests

@azjezz @maidmaid @noniagriconomie
[Security] be able to know the reasons of the denied access
e9e653d 
Co-Authored-By: Dany Maillard <danymaillard93b@gmail.com>
Co-Authored-By: Antoine Makdessi <antoine.makdessi@agriconomie.com>
@carsonbot

This comment has been minimized.

Sign in to view
@azjezz azjezz changed the title Vote [Security] Be able to know the reasons of the denied access Apr 6, 2021
@azjezz azjezz marked this pull request as ready for review April 6, 2021 09:36
@azjezz azjezz requested review from chalasr and wouterj as code owners April 6, 2021 09:36
@azjezz azjezz mentioned this pull request Apr 6, 2021
Closed
@azjezz azjezz force-pushed the vote branch 2 times, most recently from b56fa30 to 93e9c87 Compare April 6, 2021 09:40
@chalasr chalasr added this to the 5.x milestone Apr 6, 2021
@noniagriconomie
Copy link
Contributor

Tank you for continuing this @azjezz I'll review very soon
Also to be mentioned, all the initial work has been done by @maidmaid, I've just done some reviews like others ppl :)

noniagriconomie
noniagriconomie reviewed Apr 6, 2021
View reviewed changes
Copy link
Contributor

@noniagriconomie noniagriconomie left a comment

Choose a reason for hiding this comment

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

some early reviews

src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php
@@ -33,7 +33,7 @@ interface VoterInterface
* @param mixed $subject The subject to secure
* @param array $attributes An array of attributes associated with the method being invoked
*
* @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
* @return int|Vote either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
Copy link
Contributor

@noniagriconomie noniagriconomie Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

related to comment on Voter/Voter.php

src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php
use AccessTrait;

private $reason;
private $parameters;
Copy link
Contributor

Choose a reason for hiding this comment

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

i would call this $context like in workflow, wdyt?

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php Outdated
@@ -206,4 +216,22 @@ private function decidePriority(TokenInterface $token, array $attributes, $objec

return $this->allowIfAllAbstainDecisions;
}

private function decideIfAllAbstainDecisions(): AccessDecision
Copy link
Contributor

Choose a reason for hiding this comment

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

i would directly write this code, the function seem not mandatory

src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php
@@ -75,7 +108,7 @@ abstract protected function supports(string $attribute, $subject);
*
* @param mixed $subject
*
* @return bool
* @return Vote Returning a boolean is deprecated since Symfony 5.1. Return a Vote object instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo, 5.3
also, shouldnt we create someting like #35592 (comment) to make it BC?

@azjezz azjezz force-pushed the vote branch 13 times, most recently from bd948f7 to 5489fe2 Compare April 6, 2021 16:35
@azjezz azjezz force-pushed the vote branch 8 times, most recently from b5f1e4c to 5b89b7e Compare April 9, 2021 13:28
@94noni 94noni mentioned this pull request Aug 20, 2021
Closed
@noniagriconomie
Copy link
Contributor

👋 ping @azjezz will you be able to rebase and finish for v5.4? thx

@fabpot
Copy link
Member

fabpot commented Sep 21, 2021

Let's close for now as there is no more work here. @azjezz Feel free to reopen if you'd like to finish it.

@fabpot fabpot closed this Sep 21, 2021
@yellow1912
Copy link

I would really like to see this feature merged. Is it possible for me to pick up the work? @fabpot @azjezz I can use the current 5.4 base, merge the changes made.

@azjezz
Copy link
Contributor Author

azjezz commented Sep 23, 2021

@yellow1912 feel free to do so, I'm currently unable to do so due to lack of time :)

@yellow1912 yellow1912 mentioned this pull request Sep 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

@noniagriconomie noniagriconomie noniagriconomie left review comments

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@wouterj wouterj Awaiting requested review from wouterj

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

Successfully merging this pull request may close these issues.

[Security][DX] Be able to know why exactly SecurityVoter returns false
7 participants
@azjezz @carsonbot @noniagriconomie @fabpot @yellow1912 @OskarStark @chalasr