Skip to content

extra code example #12538

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

Merged
merged 2 commits into from
Nov 9, 2019
Merged

extra code example #12538

merged 2 commits into from
Nov 9, 2019

Conversation

webmasterMeyers
Copy link
Contributor

document the ability to pass in multiple roles.

@wouterj wouterj changed the base branch from 4.3 to 3.4 November 9, 2019 15:10
wouterj added a commit that referenced this pull request Nov 9, 2019
wouterj added a commit that referenced this pull request Nov 9, 2019
* 3.4:
  [#12538] Changed AND to OR
wouterj added a commit that referenced this pull request Nov 9, 2019
* 4.3:
  [#12538] Changed AND to OR
@wouterj wouterj closed this in e6f5f38 Nov 9, 2019
@wouterj wouterj merged commit 1531a2f into symfony:3.4 Nov 9, 2019
wouterj added a commit that referenced this pull request Nov 9, 2019
* 4.4:
  [#12538] Changed AND to OR
@wouterj
Copy link
Member

wouterj commented Nov 9, 2019

Thanks @webmasterMeyers!

Support for this was just readded in Symfony 5 (See symfony/symfony#34304), so it makes sense to document it more clearly and I like the way you did it (small change, huge impact).

However, if I look at the implementation of that PR and what I have written in symfony/symfony#33584 , I think the condition is OR instead of AND when you define multiple roles. I've changed this in b1c8312. If you think it's incorrect, please comment :)

@webmasterMeyers
Copy link
Contributor Author

Wow, now I think I have a security problem after reading the PR thread. Maybe the original PR is right, it should only allow 1 role passed in.

The implementation of OR makes sense when passing in "made up" roles, but for the built in role IS_AUTHENTICATED_FULLY it does not.

roles: [IS_AUTHENTICATED_FULLY, ROLE_ADMIN] as an OR will effectivly allow me to access this as ANY role, so long as I re-enter my password. I may not be ROLE_ADMIN but may be ROLE_USER and just entered my password, thus I am granted by IS_AUTHENTICATED_FULLY

confusing indeed.

@webmasterMeyers
Copy link
Contributor Author

a quick test on symfony 4.3.5 tells me it is AND. With a ROLE_USER (and other roles) I am still denied access. As well, if I add an "extra" role to the array, my ROLE_ADMIN user gets denied access. It is an AND condition in this instance.

@webmasterMeyers
Copy link
Contributor Author

Further, I'd like to say that passing multiple role makes the most sense as an AND condition. If I require a situation where I need OR I would use role hierarchy.

@xabbuh
Copy link
Member

xabbuh commented Nov 16, 2019

This one is a bit tricky. The point here is that the IS_AUTHENTICATED_* attributes are not roles and handled by a completely different voter (the AuthenticatedVoter). If you would pass more than one role (e.g. something like ROLE_USER and ROLE_ADMIN), the user will indeed only be required to have one of these roles.

@xabbuh
Copy link
Member

xabbuh commented Nov 16, 2019

Having said that I must admit that I am not sure how we should deal with that here. Stating that only one of the roles is required is true, but the example doesn't use them which makes this so confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants