Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.2k
review all Security code blocks#5486
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
<rule path="^/admin" roles="ROLE_USER_IP" ip="127.0.0.1" /> | ||
<rule path="^/admin" roles="ROLE_USER_HOST" host="symfony\.com$" /> | ||
<rule path="^/admin" roles="ROLE_USER_METHOD" methods="POST, PUT" /> | ||
<rule path="^/admin" roles="ROLE_USER" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
rule
andaccess_control
do have the same semantic meaning in XML (actuallyaccess_control
is the pluralised form ofrule
). In fact, nestingaccess-control
andrule
was a mistake we made in the documentation and only worked if we had only one rule.
The resulting array after processing the configuration would have been an array withrule
as the key. This only worked because the extension just loops the array and doesn't deal with its keys. Though obviously this will break as soon as you have more than one rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, I am not sure if we should really use therule
element at all. Would it be more clear if we simply used multipleaccess-control
elements?
3b08fb1
to49e589c
Compare@xabbuh first, thank you very much for this huge work! I have a question: do you still consider this PR as WIP? I'm asking because I've reviewed it and it looks ready to be merged. |
Yeah, there five or six files from the cookbook section I didn't look at yet. |
This is ready to be reviewed. |
<config> | ||
<!-- ... --> | ||
<firewall name="wsse_secured" pattern="/api/.*" stateless="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the examples above, we usually define the pattern as^/api
instead of/api/.*
Is there any reason for using this other format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know the reason for this (maybe just a matter of taste of the initial author of that file). I changed it for consistency.
@xabbuh I've reviewed this PR again and I love it. Thank you and congrats on this huge work. |
Thank you@xabbuh! 👍 |
<!-- ... --> | ||
<acl> | ||
<connection>default</connection> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
while this works, we always use attributes for non-array nodes (making this<acl connection="default"/>
This PR was merged into the 2.3 branch.Discussion----------review all Security code blocks| Q | A| ------------- | ---| Doc fix? | yes| New docs? | no| Applies to | all| Fixed tickets |#4606 (comment)As I promised@weaverryan I now found some time to review all the security-related code blocks. :)Commits-------9099cf2 review all Security code blocks
Wow! Merged it as "minor", but this PR really is major! 😃 Good job Christian! I did a very quick review of the code blocks in the cookbook and fixed my comments in this PR as well. See77555a6, let me know if anything is wrong. |
Awesome! |
👍 looks good@wouterj and I also like your reasoning about the position of the closing angle brackets (even if it looks not really pretty ;)) |
As I promised@weaverryan I now found some time to review all the security-related code blocks. :)