Skip to content

Conversation

weaverryan
Copy link
Member

Hi guys!

A long time coming, here is the refactored security documentation:

  • There is now one main chapter - much was moved into cookbook articles
  • Several "stub" cookbook articles were created - I think these are topics that should be covered, but it's more important to merge in this refactoring first

Also, one of the most important areas to attack - across the entire framework - is the configuration references. In the coming weeks, I'll try to organize people to attack that in a consistent way. Also, after this is merged, we should refactor the cookbook page - it's utterly unreadable now.

Comments warmly appreciated - thanks!

Timo Haberkern and others added 30 commits April 13, 2011 16:52
…rkern/symfony-docs into thaberkern-cookbook_pdo_session_storage
[security] fixed wrong fully qualified class name in voter chapter.
Fixed typo in routing examples
…ymfony-docs into thaberkern-doctrine-repositories
[security] added a note to change the voter strategy in the AccessDecisio
Per comments from @stof, using the reference link collapses the class's name, which we don't want here since we want the reader to see the difference between the two potential base Command classes.
this login form visually is your job. First, create two routes: one that
will display the login form (i.e. ``/login``) and one that will handle the
login form submission (i.e. ``/login_check``):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not create a route for /login_check as it will never be used and can cause confusion to the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now that you are using the login_check route in the templates. So, it is indeed useful... but confusing ;) As there is not controller, and because the /login_check is configured as a string directly into the firewall, why not use an hardcoded URL?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought. I can use a hardcoded URL, but what should that look like? Of course, the router also guarantees that I hit the same controller and stay under the right web root for my project.

Also, since the firewall effectively acts like a route+controller for /login_check, I instinctively want to ask the security system to generate the login check URL for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the explanation you have for logout is perfect: https://github.com/symfony/symfony-docs/pull/288/files#L3R1376

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, here's what I've done - I do think it's clearer, though I still make the user create both routes up front, because ultimately they'll "need" both: c4b0e78

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's better. thanks.

@fabpot
Copy link
Member

fabpot commented May 14, 2011

Very well done @weaverryan! This is an excellent chapter about a very tough topic.

@weaverryan
Copy link
Member Author

Thanks! I've made the changes you've suggested - good catch on both of them.

@fabpot
Copy link
Member

fabpot commented May 16, 2011

What about merging this soon? I think it's much better than the current doc.


If a user requests ``/admin/foo``, however, the process behaves differently.
This is because of the ``access_control`` configuration section that says
that any URL matching the regular expression pattern ``^admin`` (i.e. ``/admin``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the regex should be ^/admin

@stof
Copy link
Member

stof commented May 16, 2011

I'm +1 about merging it.

@weaverryan weaverryan merged commit c4b0e78 into master May 16, 2011
@weaverryan
Copy link
Member Author

Thanks guys - last two changes made per @stof and this has now been merged in!

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

Successfully merging this pull request may close these issues.

6 participants