review all Security code blocks #5486
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.
rule and access_control do have the same semantic meaning in XML (actually access_control is the pluralised form of rule). In fact, nesting access-control and rule 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 with rule 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.
Actually, I am not sure if we should really use the rule element at all. Would it be more clear if we simply used multiple access-control elements?
xabbuh
force-pushed
the
security-code-blocks
branch
7 times, most recently
from
3b08fb1 to
49e589c
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. |
xabbuh
force-pushed
the
security-code-blocks
branch
from
49e589c to
04fc6a6
Compare
xabbuh
changed the title
|
This is ready to be reviewed. |
| <config> | ||
| <!-- ... --> | ||
|
|
||
| <firewall name="wsse_secured" pattern="/api/.*" stateless="true"> |
There was a problem hiding this comment.
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.
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. |
xabbuh
force-pushed
the
security-code-blocks
branch
from
04fc6a6 to
2159bda
Compare
|
Thank you @xabbuh! 👍 |
xabbuh
force-pushed
the
security-code-blocks
branch
from
2159bda to
34f7859
Compare
xabbuh
force-pushed
the
security-code-blocks
branch
from
34f7859 to
9099cf2
Compare
| <!-- ... --> | ||
|
|
||
| <acl> | ||
| <connection>default</connection> |
There was a problem hiding this comment.
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. See 77555a6, 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. :)