Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
wouterj merged 1 commit intosymfony:2.3fromxabbuh:security-code-blocks
Jul 21, 2015

Conversation

xabbuh
Copy link
Member

QA
Doc fix?yes
New docs?no
Applies toall
Fixed tickets#4606 (comment)

As I promised@weaverryan I now found some time to review all the security-related code blocks. :)

<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" />
Copy link
MemberAuthor

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.

Copy link
MemberAuthor

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?

@xabbuhxabbuhforce-pushed thesecurity-code-blocks branch 7 times, most recently from3b08fb1 to49e589cCompareJuly 5, 2015 11:07
@javiereguiluz
Copy link
Member

@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.

@xabbuh
Copy link
MemberAuthor

Yeah, there five or six files from the cookbook section I didn't look at yet.

@xabbuhxabbuhforce-pushed thesecurity-code-blocks branch from49e589c to04fc6a6CompareJuly 7, 2015 19:22
@xabbuhxabbuh changed the title[WIP] review all Security code blocksreview all Security code blocksJul 7, 2015
@xabbuh
Copy link
MemberAuthor

This is ready to be reviewed.

<config>
<!-- ... -->

<firewall name="wsse_secured" pattern="/api/.*" stateless="true">
Copy link
Member

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?

Copy link
MemberAuthor

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.

@javiereguiluz
Copy link
Member

@xabbuh I've reviewed this PR again and I love it. Thank you and congrats on this huge work.

@xabbuhxabbuhforce-pushed thesecurity-code-blocks branch from04fc6a6 to2159bdaCompareJuly 8, 2015 06:57
@OskarStark
Copy link
Contributor

Thank you@xabbuh!

👍

@xabbuhxabbuhforce-pushed thesecurity-code-blocks branch from2159bda to34f7859CompareJuly 16, 2015 20:27
@xabbuhxabbuhforce-pushed thesecurity-code-blocks branch from34f7859 to9099cf2CompareJuly 16, 2015 20:29
<!-- ... -->

<acl>
<connection>default</connection>
Copy link
Member

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"/>

@wouterjwouterj merged commit9099cf2 intosymfony:2.3Jul 21, 2015
wouterj added a commit that referenced this pull requestJul 21, 2015
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
@xabbuhxabbuh deleted the security-code-blocks branchJuly 21, 2015 14:50
wouterj added a commit that referenced this pull requestJul 21, 2015
@wouterj
Copy link
Member

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.

@weaverryan
Copy link
Member

Awesome!

@xabbuh
Copy link
MemberAuthor

👍 looks good@wouterj and I also like your reasoning about the position of the closing angle brackets (even if it looks not really pretty ;))

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@xabbuh@javiereguiluz@OskarStark@wouterj@weaverryan

[8]ページ先頭

©2009-2025 Movatter.jp