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

Apply DenyAccessListener before deserialization #2710

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@antograssiot
Copy link
Member

antograssiot commented Apr 7, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #1646
License MIT
Doc PR

I agree with @vincentchalamon that the priority is not the expected one.
The access control must be checked against the data read from the DB in the first place.
We have been discussing this during the EU-FOSSA Hackathon and couldn't really imagine a scenario where this would not be the expected behavior

ping @dunglas

@vincentchalamon
Copy link
Contributor

vincentchalamon left a comment

Please update PR description as changing the DenyAccessListener priority is a BC break.

Show resolved Hide resolved features/authorization/deny.feature Outdated
Show resolved Hide resolved features/authorization/deny.feature Outdated

@antograssiot antograssiot force-pushed the antograssiot:deny-access-listener branch from d2c237e to 05a8754 Apr 7, 2019

@antograssiot antograssiot changed the base branch from 2.4 to master Apr 7, 2019

@antograssiot antograssiot force-pushed the antograssiot:deny-access-listener branch from 05a8754 to 4b6c142 Apr 7, 2019

@soyuka

This comment has been minimized.

Copy link
Member

soyuka commented Apr 7, 2019

Changing a priority is not a BC break.

@vincentchalamon

This comment has been minimized.

Copy link
Contributor

vincentchalamon commented Apr 7, 2019

But as the behavior is changed by changing this priority, I think it's a BC break, as users will get a 403 instead of a 400 in some cases now.

@soyuka

This comment has been minimized.

Copy link
Member

soyuka commented Apr 7, 2019

But as the behavior is changed by changing this priority, I think it's a BC break, as users will get a 403 instead of a 400 in some cases now.

But because the previous behavior was wrong it's a bug fix :p.

@norkunas

This comment has been minimized.

Copy link
Contributor

norkunas commented Apr 8, 2019

This will break my security voters 😑
I mean i won't be able to allow/deny based on deserialized relation state

@antograssiot

This comment has been minimized.

Copy link
Member Author

antograssiot commented Apr 8, 2019

@norkunas Can you please give us more insight on your use case ?

@norkunas

This comment has been minimized.

Copy link
Contributor

norkunas commented Apr 8, 2019

I am checking in security voters if a user can make an action based if he is a member of a company and has permissions for it.

class CompanyUpgradeRequest {
  public $company;
}
protected function voteOnAttribute($attribute, $subject, TokenInterface $token): bool {
    switch ($attribute) {
        case self::CREATE:
            return $this->companyPermissionChecker->hasPermission($token->getUser(), $subject->company, 'upgrade');
    }
}

But probably I'll rewrite these later with a validator constraint. Maybe I'm not only who depends on this now 🤔

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Apr 8, 2019

Indeed using a validation constraint for that would be cleaner. It’s not a code BC break, but it’s still a BC break. It needs at least to be documented in the changelog.

@soyuka

This comment has been minimized.

Copy link
Member

soyuka commented Apr 8, 2019

Indeed using a validation constraint for that would be cleaner. It’s not a code BC break, but it’s still a BC break. It needs at least to be documented in the changelog.

Definitely! We have 3 behavior changes in recent PRs on master which are in the same case. I'm going to add these in the changelog soon!

@teohhanhui

This comment has been minimized.

Copy link
Member

teohhanhui commented Apr 8, 2019

I think this is just shifting the problem instead of solving it. The application likely needs both before AND after states to be able to make an authorization decision, i.e. we need to know what the user is trying to do before we can say whether it should be allowed or not.

@vincentchalamon

This comment has been minimized.

Copy link
Contributor

vincentchalamon commented Apr 8, 2019

i.e. we need to know what the user is trying to do before we can say whether it should be allowed or not.

If you base on the request, it should be validation instead of security. Otherwise, could you please provide an example to explain your point?

@teohhanhui

This comment has been minimized.

Copy link
Member

teohhanhui commented Apr 8, 2019

Well, I can only speak from my personal experience. I've been trying to disprove my point that validation cannot be separated from authorization. So far, I still don't see how. The dilemma is:

  1. Is this change valid? It depends on whether the user is authorized to perform this change.
  2. Is the user authorized to perform this change? It depends on what change the user is trying to perform. (Is it a valid change given the roles / permissions the user have?)

So it seems to me that validation and authorization are interdependent (in some / many cases?).

@vincentchalamon

This comment has been minimized.

Copy link
Contributor

vincentchalamon commented Apr 8, 2019

It depends on whether the user is authorized to perform this change.

There are serialization groups for that. For example, if you want to restrict a field to specific users roles, you can decorate the ContextBuilder and dynamically change the serialization context (as explained here).

@teohhanhui

This comment has been minimized.

Copy link
Member

teohhanhui commented Apr 8, 2019

No, it's not something straightforward like that. Authorization can be as complex as the business logic requires. Same for validation. It's the interdependence between the two that's troublesome, and requires access to both "before" and "after" states.

@teohhanhui

This comment has been minimized.

Copy link
Member

teohhanhui commented Apr 8, 2019

Here is an example from my old API Platform-based (probably 2.1?) project: https://gist.github.com/teohhanhui/994ab0d0bab439302ef96ac080568a8f

@vincentchalamon

This comment has been minimized.

Copy link
Contributor

vincentchalamon commented Apr 8, 2019

So this is a specific use case that can be handled by a customer listener/subscriber?

@teohhanhui

This comment has been minimized.

Copy link
Member

teohhanhui commented Apr 8, 2019

If DenyAccessListener is only meant to cover the most basic use cases, then sure. Anything non trivial will require custom listeners.

@lukasluecke

This comment has been minimized.

Copy link
Contributor

lukasluecke commented Apr 10, 2019

Maybe relevant: #2454 (I wanted to check permissions to create an item, based on its passed properties - e.g. only allow some role to create "root" items in a hierarchy of related objects)

@soyuka

This comment has been minimized.

Copy link
Member

soyuka commented Apr 11, 2019

Might be that we should have 2 listeners? One pre-normalization for authentification guards, one after for object related guards? But wait isn't the second one part of the item validation process?
I don't think that there is a perfect solution for this use case, to move forward should we just document the solution with a custom listener?

@antograssiot

This comment has been minimized.

Copy link
Member Author

antograssiot commented Apr 11, 2019

Another listener is indeed a solution. Deprecating this listener and replacing it by an event subscriber is also an option. WDYT?

@teohhanhui

This comment has been minimized.

Copy link
Member

teohhanhui commented Apr 11, 2019

But wait isn't the second one part of the item validation process?

So, a validator that uses the AuthrizationChecker? It works for some cases, but not if you need both before / after states. At least, that's what I remember as the lesson I've learnt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.