Completely re-reading the security book #4606

weaverryan · 2014-12-07T21:51:14Z

Q	A
Doc fix?	no
New docs?	no
Applies to	all
Fixed tickets	n/a

Well, this should be interesting :). Several years ago, I bootstrapped the security chapter and it's been there ever since. That fact doesn't necessarily mean that it was good. I've just re-read and basically re-written the chapter from scratch. I thought it was too long, too theoretical in the beginning, and it also had some extra "baggage" just from being old and having things added to it.

My goal is to:

A) Not actually remove anything of importance - I've done my best with this
B) Actually get feedback that this is better. I feel that this is better, but rewrites aren't automatically better. It's like the second album of a band - even though they're older and wiser, maybe the original is still better :). I hope not!

Todo:

fill in config blocks - @xabbuh if you happen to have some time and can help, I would be even more in debt to you :)

As I merge to 2.5 and up, I'll need to check for the versionadded tags on each branch and re-add those things to the new chapter.

Thanks!

wouterj · 2014-12-07T22:20:40Z

cookbook/security/form_login_setup.rst

+    .. code-block:: php-annotations
+
+        // src/AppBundle/Controller/SecurityController.php
+        // ...


you should move the empty line below above this comment

wouterj · 2014-12-07T22:23:53Z

I left one comment (at a completely random location). I'll review the rest tomorrow

xabbuh · 2014-12-07T22:34:36Z

components/security/secure-tools.rst

+
+The
+:method:`Symfony\\Component\\Security\\Core\\Util\\SecureRandom::nextBytes`
+methods returns a random string composed of the number of characters passed as


xabbuh · 2014-12-08T09:41:31Z

cookbook/security/access_control.rst

@@ -0,0 +1,299 @@
+How does the Security access_control Work?


stof · 2014-12-08T10:53:42Z

meh, github does not show the diff for the security book (too big diff)

stof · 2014-12-08T10:55:39Z

components/security/secure-tools.rst

+    use Symfony\Component\Security\Core\Util\StringUtils;
+
+    // is password1 equals to password2?
+    $bool = StringUtils::equals($password1, $password2);


you should note that the order of arguments is important to avoid timing attack. The known string should be the first argument, and the user input should be the second one

stof · 2014-12-08T11:44:24Z

A few comments on the book itself, which cannot be inline because the diff is not visible in the PR because of its size:


.. tip::

    Supported algorithms for this method depend on your PHP version. A full list
    is available by calling the PHP function :phpfunction:`hash_algos`.

This sentence is incomplete. bcrypt and pbkf2 (and plaintext) are implemented using their own encoders in Symfony. They don't rely on the DigestPasswordEncoder (which is the one relying on algorithms know by hash_algos).


The process of authorization has two different sides:

#. The user receives a specific set of roles when logging in (e.g. ``ROLE_ADMIN``).
#. You add code so that a resource (e.g. URL, controller) requires a specific
   role in order to be accessed.

this description is again mixing roles and permission attributes (see #4158). A user receives roles, but a resource will require a specific permission attribute (which may be a role, but may be something else).
If you let people think resources are requiring roles, it will make things harder when explaining other voters (expressions, ACLs, authentication levels, custom voters)

.. caution::

    All roles **must** begin with the ``ROLE_`` prefix. Otherwise, they won't
    be handled by Symfony. If you define your own roles with a dedicated
    ``Role`` class (more advanced), don't use the ``ROLE_`` prefix.

I suggest removing this sentence. If you define something not starting with ROLE_, it will be a different kind of permission attributes, not of roles.
Thus, there is a discussion about stopping to represent roles as classes in 3.0, but only as strings starting with ROLE_

stof · 2014-12-08T13:54:00Z

The section about the @Security annotation must be removed btw. This feature of FrameworkExtraBundle is not available in Symfony 2.3. It requires 2.4+

``IS_AUTHENTICATED_FULLY`` isn't a role, but it kind of acts like one, and every
user that has successfull logged in will have this. In fact, there are thre
special attributes like this:

Typo here: three (missing e)

* ``IS_AUTHENTICATED_FULLY``: All "logged-in" users have this;
* ``IS_AUTHENTICATED_REMEMBERED``: Similar to ``IS_AUTHENTICATED_FULLY``
  but important if you're using :doc:`remember me functionality </cookbook/security/remember_me>`;

This is confusing, because users will not understand the difference. This should explain than IS_AUTHENTICATED_FULLY is not granted to people being logged-in by a remember me cookie (the one granted to everyone is IS_AUTHENTICATED_REMEMBERED)

weaverryan · 2014-12-17T23:25:24Z

Guys! Thanks so much for the review on this really long thing. What's cool is that we found several small tweaks where I had just copied and pasted old docs into a new location. In other words, we face-lifted some areas that I hadn't even intended to tweak.

The changes are at sha: 5d842e2. The big one is the new "ips" access_control restriction spot, where I had to change the example entirely.

I have a few small comments still pending, but if you have any big blocker concerns, please let me know. Also, as Stof mentioned, the book is not showing up as a diff, so please realize that it did change, and I'd appreciate notes on it. Here are my blockers:

Comments above from Stof (I ran out of time right now so could not process them yet)
XML+PHP code blocks
Waiting a couple of days to make sure there are no other big blockers, then merging and celebrating

... and then of course I'll merge up to the other branches and put the new version stuff back :D

xabbuh · 2014-12-17T23:31:08Z

@weaverryan Thanks so much for this great work. I'll try to put some time into updated/syncing the code blocks in the next days.

timglabisch · 2014-12-18T21:17:06Z

cookbook/security/form_login_setup.rst

+            <label for="password">Password:</label>
+            <input type="password" id="password" name="_password" />
+
+            <!--


use twig comments here

No. this is the PHP template. It should be a PHP comment, not a Twig one

In HTML+PHP templates, we always use HTML comments.

- more tutorial-styled - tried to move things into other entries - tried to keep as many anchor references as possible

@xabbuh

This PR was merged into the 2.3 branch. Discussion ---------- Completely re-reading the security book | Q | A | ------------- | --- | Doc fix? | no | New docs? | no | Applies to | all | Fixed tickets | n/a Well, this should be interesting :). Several years ago, I bootstrapped the security chapter and it's been there ever since. That fact doesn't necessarily mean that it was good. I've just re-read and basically re-written the chapter from scratch. I thought it was too long, too theoretical in the beginning, and it also had some extra "baggage" just from being old and having things added to it. My goal is to: A) Not actually remove anything of importance - I've done my best with this B) Actually get feedback that this is better. I feel that this is better, but rewrites aren't automatically better. It's like the second album of a band - even though they're older and wiser, maybe the original is still better :). I hope not! Todo: - [ ] fill in config blocks - @xabbuh if you happen to have some time and can help, I would be even more in debt to you :) As I merge to 2.5 and up, I'll need to check for the `versionadded` tags on each branch and re-add those things to the new chapter. Thanks! Commits ------- fe9fdac [#4606] Getting my XML (and PHP) on in the new security chapter aedfcd2 [#4606] Tweaks thanks entirely to stof 614da15 Changing to _ for consistency 95d6a7d [#4606] Updating thanks to comments from everyone! d9a9310 Completely re-reading the security book

* 2.3: [#4606] Getting my XML (and PHP) on in the new security chapter [#4606] Tweaks thanks entirely to stof Changing to _ for consistency [#4606] Updating thanks to comments from everyone! Completely re-reading the security book Misc changes [Cookbook] Fix XML example of RTE Conflicts: book/security.rst cookbook/map.rst.inc cookbook/security/index.rst

* 2.5: [#4606] Getting my XML (and PHP) on in the new security chapter [#4606] Tweaks thanks entirely to stof Changing to _ for consistency [#4606] Updating thanks to comments from everyone! Completely re-reading the security book Misc changes [Cookbook] Fix XML example of RTE Conflicts: book/security.rst

* 2.7: [#4606] Getting my XML (and PHP) on in the new security chapter [#4606] Tweaks thanks entirely to stof Changing to _ for consistency [#4606] Updating thanks to comments from everyone! Completely re-reading the security book Misc changes [Cookbook] Fix XML example of RTE

weaverryan · 2014-12-31T03:26:24Z

Ok guys :). I've just added a few last commits for the last comments, merged this in, the merged it up the branches. I'm fairly convinced that the merges were clean - I manually looked at the log difference between new branches to make sure changes/additions on more-recent branches were not "run over" with the merge. But if you spot anything, please let me know!

Thanks again - I will probably try not to completely rewrite chapters in the future, though I still think more need to be re-read (but many less than even 6 months ago thanks for a lot of people here).

@weaverryan

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

weaverryan force-pushed the security-re-read branch 2 times, most recently from 8e5a87b to 8657c6f Compare December 7, 2014 21:56

weaverryan mentioned this pull request Dec 7, 2014

Updated documentation regarding the SecurityContext split #4188

Closed

wouterj reviewed Dec 7, 2014
View reviewed changes

xabbuh reviewed Dec 7, 2014
View reviewed changes

xabbuh added the Status: Needs Work label Dec 8, 2014

xabbuh reviewed Dec 8, 2014
View reviewed changes

cookbook/security/access_control.rst

@@ -0,0 +1,299 @@

How does the Security access_control Work?

Copy link

Member

xabbuh Dec 8, 2014

Choose a reason for hiding this comment

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

Does

stof reviewed Dec 8, 2014
View reviewed changes

weaverryan added a commit that referenced this pull request Dec 17, 2014

[#4606] Updating thanks to comments from everyone!

5d842e2

timglabisch reviewed Dec 18, 2014
View reviewed changes

weaverryan added a commit that referenced this pull request Dec 31, 2014

[#4606] Tweaks thanks entirely to stof

0cddf68

weaverryan added a commit that referenced this pull request Dec 31, 2014

[#4606] Getting my XML (and PHP) on in the new security chapter

e6fca36

weaverryan added 5 commits December 30, 2014 21:14

Completely re-reading the security book

d9a9310

- more tutorial-styled - tried to move things into other entries - tried to keep as many anchor references as possible

[#4606] Updating thanks to comments from everyone!

95d6a7d

Changing to _ for consistency

614da15

[#4606] Tweaks thanks entirely to stof

aedfcd2

[#4606] Getting my XML (and PHP) on in the new security chapter

fe9fdac

weaverryan force-pushed the security-re-read branch from e6fca36 to fe9fdac Compare December 31, 2014 02:14

weaverryan merged commit fe9fdac into 2.3 Dec 31, 2014

weaverryan mentioned this pull request Dec 31, 2014

Clarify form_login versus simple_form #4718

Closed

weaverryan deleted the security-re-read branch December 31, 2014 03:24

wouterj mentioned this pull request Feb 19, 2015

[Security] Use the is_granted expression to keep consistency with php example above #4282

Closed

wouterj mentioned this pull request May 11, 2015

Tip for multiple authentication mechanisms #5004

Closed

xabbuh mentioned this pull request Jul 5, 2015

review all Security code blocks #5486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Completely re-reading the security book #4606

Completely re-reading the security book #4606

weaverryan commented Dec 7, 2014

wouterj Dec 7, 2014

wouterj commented Dec 7, 2014

xabbuh Dec 7, 2014

xabbuh Dec 8, 2014

stof commented Dec 8, 2014

stof Dec 8, 2014

stof commented Dec 8, 2014

stof commented Dec 8, 2014

weaverryan commented Dec 17, 2014

xabbuh commented Dec 17, 2014

timglabisch Dec 18, 2014

stof Dec 19, 2014

wouterj Dec 19, 2014

weaverryan commented Dec 31, 2014

Completely re-reading the security book #4606

Completely re-reading the security book #4606

Conversation

weaverryan commented Dec 7, 2014

wouterj Dec 7, 2014

Choose a reason for hiding this comment

wouterj commented Dec 7, 2014

xabbuh Dec 7, 2014

Choose a reason for hiding this comment

xabbuh Dec 8, 2014

Choose a reason for hiding this comment

stof commented Dec 8, 2014

stof Dec 8, 2014

Choose a reason for hiding this comment

stof commented Dec 8, 2014

stof commented Dec 8, 2014

weaverryan commented Dec 17, 2014

xabbuh commented Dec 17, 2014

timglabisch Dec 18, 2014

Choose a reason for hiding this comment

stof Dec 19, 2014

Choose a reason for hiding this comment

wouterj Dec 19, 2014

Choose a reason for hiding this comment

weaverryan commented Dec 31, 2014