NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

maidmaid wants to merge9 commits intosymfony:masterfrommaidmaid:vote

Closed

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

maidmaid wants to merge9 commits intosymfony:masterfrommaidmaid:vote

Conversation

Copy link

Contributor

maidmaid commentedFeb 4, 2020

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets	#27995,#26343
License	MIT
Doc PR	/

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

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

Before :

class PostVoterextends Voter{// ...protectedfunctionvoteOnAttribute($attribute,$subject,TokenInterface$token)    {if ($subject->getAuthor() !==$token->getUser()) {returnfalse;        }if ($subject->getStatus() !=='draft') {returnfalse;        }returntrue;    }}

After :

class PostVoterextends Voter{// ...protectedfunctionvoteOnAttribute($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 constantACCESS_{GRANTED,ABSTAIN,DENIED} but aVote object which contains the access result and the reason. Thus, the security panel of the profiler can display the reason of each vote.

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

It's in a PoC state. Todo : BC layer, migration of the others voters, update current tests, add new tests...

carsonbot added Status: Needs Review Security Feature Deprecation labels

Feb 4, 2020

maidmaid force-pushed thevote branch from949cf7d tod905dadCompare

February 4, 2020 15:37

Pierstoval reviewed

Feb 4, 2020

View reviewed changes

Copy link

Contributor

Pierstoval left a comment

Choose a reason for hiding this comment

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

I like this PR very much 😄 I hope it's gonna be appreciated by the core team!
In the meantime I have a few comments :)

src/Symfony/Component/Security/Core/Authorization/AccessDecision.phpShow resolvedHide resolved

src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Core/Authorization/Voter/TraceableVoter.phpShow resolvedHide resolved

ro0NL reviewed

Feb 4, 2020

View reviewed changes

Copy link

Contributor

ro0NL left a comment

Choose a reason for hiding this comment

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

we should re-evaluate#21094 (comment) first

AccessDecision vsVote is confusing, there should be only 1 IMHO. AFAIK there's no real reason to merge multiple votes now, which is a new behavior

src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php

		/**
		* Sets an access decision and appends the denied reasons to the exception message.
		*/
		public function setAccessDecision(AccessDecision $accessDecision)

Copy link

Contributor

ro0NLFeb 4, 2020

Choose a reason for hiding this comment

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

this cannot be stateful IMHO

Copy link

ContributorAuthor

maidmaidFeb 5, 2020

Choose a reason for hiding this comment

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

What do you suggest ?

Copy link

ContributorAuthor

maidmaidFeb 18, 2020

Choose a reason for hiding this comment

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

We could make it stateless and have a specific exception render for AccessDeniedException. WDYT ?

Copy link

ContributorAuthor

maidmaid commentedFeb 5, 2020

AccessDecision vs Vote is confusing, there should be only 1

We need a distinction between 1 vote amongst others and the final decision. E.g. 3 votes in + 4 votes against = decision to deny (in a consensus strategy). So, a voter votes (it returns aVote object), whereas the access decision manager decides the final access (it returns anAccessDecision object). Maybe,AccessDecision could be renamed to something likeVerdict for clarity ?

chalasr added this to thenext milestone

Feb 5, 2020

Copy link

Member

chalasr commentedFeb 5, 2020•
edited
Loading

AccessDecision could be renamed to something like Verdict for clarity ?

Not sure it would bring much:AccessDecisionManager takes access decisions,verdict is just a synonym, might be more confusing than helpful.

noniagriconomie reviewed

Feb 5, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twigShow resolvedHide resolved

src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig Outdated

		{% endif %}
		</td>
		<td class="font-normal text-small">{{ voter_detail['vote'].reason }}</td>

Copy link

Contributor

noniagriconomieFeb 5, 2020

Choose a reason for hiding this comment

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

maybe only display if not empty reason?

Copy link

ContributorAuthor

maidmaidFeb 18, 2020

Choose a reason for hiding this comment

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

Thetd html node must always be present.

Copy link

Member

chalasrFeb 18, 2020

Choose a reason for hiding this comment

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

I believe we are used to display~ or(unknown) to not let it blank. Could you check and apply what looks better?

Copy link

ContributorAuthor

maidmaidMar 3, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Much better imo ;)

Copy link

ContributorAuthor

maidmaidMar 3, 2020

Choose a reason for hiding this comment

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

@javiereguiluz would you have a suggestion here maybe ?

src/Symfony/Component/Security/Core/Authorization/AccessDecision.phpShow resolvedHide resolved

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManagerInterface.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Core/Authorization/Voter/Vote.phpShow resolvedHide resolved

poc

26a8381

maidmaid force-pushed thevote branch 2 times, most recently from2261746 to69e8d07Compare

February 18, 2020 10:31

round 2

868b9ff

maidmaid force-pushed thevote branch from69e8d07 to868b9ffCompare

February 18, 2020 10:31

This comment has been minimized.

noniagriconomie reviewed

Feb 25, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php OutdatedShow resolvedHide resolved

noniagriconomie reviewed

Feb 25, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php OutdatedShow resolvedHide resolved

noniagriconomie reviewed

Feb 25, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php OutdatedShow resolvedHide resolved

noniagriconomie reviewed

Feb 25, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/Vote.phpShow resolvedHide resolved

noniagriconomie reviewed

Feb 25, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php OutdatedShow resolvedHide resolved

noniagriconomie reviewed

Feb 25, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php OutdatedShow resolvedHide resolved

chalasr reviewed

Feb 26, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/AccessDecision.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php

		trait AccessTrait
		{
		/** @var int */
		protected $access;

Copy link

Member

chalasrFeb 18, 2020

Choose a reason for hiding this comment

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

Should be private.

Copy link

ContributorAuthor

maidmaidMar 3, 2020

Choose a reason for hiding this comment

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

Already discussed here#35592 (comment). Ok for you ?

Copy link

Member

chalasrApr 24, 2020

Choose a reason for hiding this comment

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

Not sure to understand the rationale there.
If a class uses this trait, it does have write access to the private properties defined by the trait. Usingprotected would only serve for inheritance purpose, which is not needed here AFAIK.
Am I missing something?

maidmaidand others added6 commits

March 3, 2020 09:42

Add a "~" default filter in profiler twig view

edfc392

Fix the message deprecated

ec41df2

Co-Authored-By: Antoine Makdessi <antoine.makdessi@agriconomie.com>

Fix a typo

842e59e

Co-Authored-By: Antoine Makdessi <antoine.makdessi@agriconomie.com>

Fix a typo

7339108

Co-Authored-By: Antoine Makdessi <antoine.makdessi@agriconomie.com>

Make final the Vote and the AccessDecision classes

8ef8d19

Fix minor

c8a360c

Add phpdoc

c4cfb5f

Copy link

ContributorAuthor

maidmaid commentedMar 3, 2020•
edited
Loading

Changes committed from your feedbacks, thanks !

This PR looks appreciated and its design seems validated, so I'm gonna complete it by writting the tests soon.

noniagriconomie reviewed

Mar 3, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php

		@@ -11,13 +11,22 @@

		namespace Symfony\Component\Security\Core\Authorization\Voter;

		/**
		* A Vote is returned by a Voter and contains the access (granted, abstain or denied). It can also contains a reason

Copy link

Contributor

noniagriconomieMar 3, 2020

Choose a reason for hiding this comment

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

Suggested change

	* A Vote is returned by a Voterandcontains the access (granted, abstainor denied). It can also contains a reason
	* A Vote is returned by a Voterandcontains the access (granted, abstainor denied).
	* It can also contains a reason explaining the vote decision.

Copy link

Contributor

stephanvierkant commentedApr 24, 2020

Thanks for create this PR!

What about creating multiple reasons to deny access, like you can with workflows?

wouterj reviewed

Apr 24, 2020

View reviewed changes

Copy link

Member

wouterj left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I like this idea. I think I also agree on the difference betweenVote andAccessDecision. An access decision is a collection of votes.

As commented, there is a need for BC layers in the usage part. I guess the only possible method to change this would be to create a new method/new interface.In that case, I think it makes sense to also think a little bit about how we're going to solve#28868 (comment) in the future, as it might also require a different authorization API. If we need to create a new authorization API for this change, I think it's best to take that change into account as well (not fix it in this PR, but designing an API that allows it to happen without change).On second thought, let's not combine these things and only focus on this feature. The other issue can be implemented using the current interface as well.

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

Member

wouterjApr 24, 2020

Choose a reason for hiding this comment

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

The same thing applies here: I don't think we can change the method return signature fromint toVote, as it'll impact how you need to use this method.

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManagerInterface.php

		@@ -26,7 +26,7 @@ interface AccessDecisionManagerInterface
		* @param array $attributes An array of attributes associated with the method being invoked
		* @param object $object The object to secure
		*
		* @return bool true if the accessisgranted, false otherwise
		* @return bool\|AccessDecision Returning a booleanisdeprecated since Symfony 5.1. Return an AccessDecision object instead.

Copy link

Member

wouterjApr 24, 2020

Choose a reason for hiding this comment

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

This interface is not internal, so we should also be BC with the usage of the method AFAIK. I don't think we currently preserve backwards compatibility in e.g. this example:

if (!$accessDecisionManager->decide(...)) {thrownewAccessDeniedException();}

If anAccessDecision::createDenied() is returned, the condition!(AccessDecision::createDenied()) isfalse, causing the access denied exception not to be throw (while it was thrown using the oldbool signature).

src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php

		$this->lastAccessDecision = $this->accessDecisionManager->decide($token, [$attribute], $subject);

		if (\is_bool($this->lastAccessDecision)) {
		trigger_deprecation('symfony/security', 5.1, 'Returning a boolean from the "%s::decide()" method is deprecated. Return an "%s" object instead', \get_class($this->accessDecisionManager), AccessDecision::class);

Copy link

Member

wouterjApr 24, 2020

Choose a reason for hiding this comment

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

Suggested change

	trigger_deprecation('symfony/security',5.1,'Returning a boolean from the "%s::decide()" method is deprecated. Return an "%s" object instead',\get_class($this->accessDecisionManager), AccessDecision::class);
	trigger_deprecation('symfony/security-core',5.1,'Returning a boolean from the "%s::decide()" method is deprecated, return an "%s" object instead',\get_class($this->accessDecisionManager), AccessDecision::class);

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php

		private function vote(VoterInterface $voter, TokenInterface $token, $subject, array $attributes): Vote
		{
		if (\is_int($vote = $voter->vote($token, $subject, $attributes))) {
		trigger_deprecation('symfony/security', 5.1, 'Returning an int from the "%s::vote()" method is deprecated. Return a "%s" object instead.', \get_class($this->voter), Vote::class);

Copy link

Member

wouterjApr 24, 2020

Choose a reason for hiding this comment

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

Suggested change

	trigger_deprecation('symfony/security',5.1,'Returning an int from the "%s::vote()" method is deprecated. Return a "%s" object instead.',\get_class($this->voter), Vote::class);
	trigger_deprecation('symfony/security-core',5.1,'Returning an int from the "%s::vote()" method is deprecated, return a "%s" object instead.',\get_class($this->voter), Vote::class);

src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php

		return new self(VoterInterface::ACCESS_DENIED, $reason, $parameters);
		}

		public function merge(self $vote): void

Copy link

Member

wouterjApr 24, 2020•
edited
Loading

Choose a reason for hiding this comment

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

I don't really see the value of this method. It doesn't fullfill what I would call "merge" (i.e. it only merges the reason, what happens if we pass denied into the merge of a granted vote?) and it's used only in one specific case. I would rather add asetReason() method and doing this directly in the voter.

Copy link

Member

fabpot commentedAug 12, 2020

@wouterj @maidmaid What would be the next steps to move this PR forward?

Copy link

Member

wouterj commentedAug 13, 2020

I thinkthis comment from my review is the most important: How can we make this change BC?

Copy link

Contributor

deguif commentedSep 16, 2020

Will we be able to get in in next release before feature freeze?

Copy link

Member

fabpot commentedSep 16, 2020

@deguif Depends on when it's ready to be merged. We still have 2 weeks for 5.2.

Copy link

Contributor

noniagriconomie commentedSep 16, 2020

@maidmaid @wouterj @fabpot

Cant we use the same logic as thehttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/EventDispatcher/LegacyEventDispatcherProxy.php?

a proxy class that handles previous/new voting feature? (I am unsure if it can be applied here)

concerning the feature itself, i think it is usefull :) (like the workflow transition blocker)

(i can help test it if needed :))

Copy link

Contributor

dkarlovi commentedSep 16, 2020

This seems like very close to what was done for Workflow, guards got the same ability there from setting a boolean value, likely the same approach can be taken.

Copy link

Member

fabpot commentedSep 30, 2020

@maidmaid Still interested in working on this? This can still be part of 5.2 if approved before tomorrow.

Copy link

ContributorAuthor

maidmaid commentedOct 2, 2020

Sorry for the delay. I missed the 5.2 train I guess. I'm gonna resume this work soon. Let's wait 5.3 :)

fabpot closed this

Oct 7, 2020

Copy link

Member

fabpot commentedOct 7, 2020

We've just moved away frommaster as the main branch to use5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

nicolas-grekas added the for 5.x label

Oct 7, 2020

noniagriconomie mentioned this pull request

Feb 19, 2021

[Security][DX] Be able to know why exactly SecurityVoter returns false#27995

Closed

Copy link

Contributor

noniagriconomie commentedFeb 22, 2021

@maidmaid friendly ping,
are you able to complete this code for the next version 5.3? If not, can help be provided?
(related to#27995 (comment) and following comments)

Copy link

ContributorAuthor

maidmaid commentedMar 5, 2021

@noniagriconomie I don't have a lot of time to finish this work. I would be happy if someone finished it :)

Copy link

Contributor

azjezz commentedApr 4, 2021

Hey@maidmaid, i would like to pick up where you left here and continue working on this PR, is that okay with you?

Copy link

Contributor

noniagriconomie commentedApr 6, 2021

@azjezz i would be pleased to help you, review or testing in real project if you need :)

azjezz mentioned this pull request

Apr 6, 2021

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

Closed

4 tasks

Copy link

Contributor

azjezz commentedApr 6, 2021

@ noniagriconomie see#40711

yellow1912 mentioned this pull request

Sep 23, 2021

[Security] Add the ability for voter to return decision reason#43147

Closed

alamirault added a commit to alamirault/symfony that referenced this pull request

May 26, 2022

Begin merge 6.2symfony#43147symfony#35592

0d1e7e5

alamirault mentioned this pull request

May 29, 2022

[Security] Add the ability for voter to return decision reason#46493

Closed

3 tasks

eltharin mentioned this pull request

Aug 27, 2024

[Security][SecurityBundle] Add messages and score on votes#58107

Closed

eltharin mentioned this pull request

Jan 21, 2025

[Security] Continuation : Add messages on votes#59573

Closed

nicolas-grekas mentioned this pull request

Feb 13, 2025

[Security] Add ability for voters to explain their vote#59771

Merged

fabpot added a commit that referenced this pull request

Feb 17, 2025

feature#59771[Security] Add ability for voters to explain their vot…

2962567

…e (nicolas-grekas)This PR was merged into the 7.3 branch.Discussion----------[Security] Add ability for voters to explain their vote| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | no| New feature?  | no| Deprecations? | no| Issues        |Fix#27995,#26343,#35592,#43147| License       | MITThis PR takes over#58107, which itself took over from#46493, etc.It takes the only approach I could think of that would preserve BC.The visible tip of what this achieves is:![image](https://github.com/user-attachments/assets/67bd0678-868f-40ed-b2c6-3b1f10ffe101)![image](https://github.com/user-attachments/assets/715814d8-e4de-47f0-aa0e-c412092389ff)Internally, this provides a new access audit log infrastructure that relies on new `AccessDecision` and `Vote` objects.Note that I didn't add (nor fix) tests for now. I'll borrow (and credit) from#58107 /#46493 as much as possible.Commits-------c6eb9ae [Security] Add ability for voters to explain their vote