Improved documentation about access controls #11118
Conversation
HeahDude
force-pushed
the
fix-access-control
branch
from
8c8c224 to
d87d382
Compare
| @@ -114,9 +121,9 @@ will match any ``ip``, ``host`` or ``method``: | |||
| | ``/admin/user`` | 168.0.0.1 | example.com | POST | rule #3 (``ROLE_USER_METHOD``) | The ``ip`` and ``host`` don't match the first two entries, | | |||
| | | | | | | but the third - ``ROLE_USER_METHOD`` - matches and is used. | | |||
| +-----------------+-------------+-------------+------------+--------------------------------+-------------------------------------------------------------+ | |||
| | ``/admin/user`` | 168.0.0.1 | example.com | GET | rule #4 (``ROLE_USER``) | The ``ip``, ``host`` and ``method`` prevent the first | | |||
| | ``/admin/user`` | 168.0.0.1 | example.com | GET | rule #4 (``ROLE_MANAGER``) | The ``ip``, ``host`` and ``method`` prevent the first | | |||
There was a problem hiding this comment.
ROLE_MANAGER or ROLE_ADMIN
right?
There was a problem hiding this comment.
Given the amount of confusion that are provided by the array roles, should we even document them? I mean, the role hierarchy is complex enough to manage everything with single roles instead of array roles. Sometimes it's better to hide confusing things 😃
There was a problem hiding this comment.
This question will especially, if roles is used in plural everywhere. Let's just be clear about it to avoid any confusion anymore. I've added a comment here too.
| @@ -265,7 +280,8 @@ key: | |||
| access_control: | |||
| - | |||
| path: ^/_internal/secure | |||
| allow_if: "'127.0.0.1' == request.getClientIp() or has_role('ROLE_ADMIN')" | |||
| roles: 'ROLE_ADMIN' | |||
| allow_if: "'127.0.0.1' == request.getClientIp() or request.header.has('X-Secure-Access')" | |||
There was a problem hiding this comment.
you should document with a comment here that the logic is or. You explain it more below, but a comment is all most people will read ;)
security/access_control.rst
Outdated
|
|
||
| .. note:: | ||
|
|
||
| Internally ``allow_if`` enforce calling the built-in |
There was a problem hiding this comment.
Internally, allow_if calls the built-in
I know it doesn't call it directly, but it effectively does.
There was a problem hiding this comment.
My initial sentence was triggers instead of calling, that seems much more appropriate, wdyt?
|
Thank you for the review! |
| @@ -114,9 +121,9 @@ will match any ``ip``, ``host`` or ``method``: | |||
| | ``/admin/user`` | 168.0.0.1 | example.com | POST | rule #3 (``ROLE_USER_METHOD``) | The ``ip`` and ``host`` don't match the first two entries, | | |||
| | | | | | | but the third - ``ROLE_USER_METHOD`` - matches and is used. | | |||
| +-----------------+-------------+-------------+------------+--------------------------------+-------------------------------------------------------------+ | |||
| | ``/admin/user`` | 168.0.0.1 | example.com | GET | rule #4 (``ROLE_USER``) | The ``ip``, ``host`` and ``method`` prevent the first | | |||
| | ``/admin/user`` | 168.0.0.1 | example.com | GET | rule #4 (``ROLE_MANAGER``) | The ``ip``, ``host`` and ``method`` prevent the first | | |||
There was a problem hiding this comment.
Given the amount of confusion that are provided by the array roles, should we even document them? I mean, the role hierarchy is complex enough to manage everything with single roles instead of array roles. Sometimes it's better to hide confusing things 😃
HeahDude
force-pushed
the
fix-access-control
branch
from
d87d382 to
5deeb53
Compare
|
Jules, thanks a lot for this contribution and I'm sorry it took us so long to merge this. |
This PR was merged into the 3.4 branch. Discussion ---------- Improved documentation about access controls <!-- If your pull request fixes a BUG, use the oldest maintained branch that contains the bug (see https://symfony.com/roadmap for the list of maintained branches). If your pull request documents a NEW FEATURE, use the same Symfony branch where the feature was introduced (and `master` for features of unreleased versions). --> Commits ------- a6b0959 Improved documentation about access controls
No description provided.