Add route permissions guard and controller permissions guard #238

jmleroux · 2014-06-13T07:26:10Z

First commit to add permissions check on route, as discussed in #182

Discussin is open, tests to come

bakura10 · 2014-06-13T10:30:37Z

tests/ZfcRbacTest/Guard/RoutePermissionsGuard.php

@@ -0,0 +1,100 @@
+<?php


File seems to be in "tests" folder?

A mistake, wil move to src

bakura10 · 2014-06-13T10:34:20Z

Apart from CS problems, it looks good to me! @arekkas, @danizord, do you mind a review?

aeneasr · 2014-06-13T10:48:11Z

On first sight a very good idea and apart from the things you commented @bakura10 it looks fine to me too. However, I'd like to see some tests :)

jmleroux · 2014-06-13T11:18:59Z

Just a note on comments and dockblocks.
I recently read this excellent book : Clean Code and i try to comment as little as possible, because a good code don't need comments. If documentation is not generated, dockblock does not seems mandatory to me.
But anyway, if you really want dockblock, i will write it. 👍

bakura10 · 2014-06-13T11:25:17Z

I want it mostly because it helps IDE :)

Envoyé de mon iPhone

Le 13 juin 2014 à 13:19, jean-marie leroux [email protected] a écrit :

Just a note on comments and dockblocks.
I recently read this excellent book : Clean Code and i try to comment as little as possible, because a good code don't need comments. If documentation is not generated, dockblock does not seems mandatory to me.
But anyway, if you really want dockblock, i will write it.

—
Reply to this email directly or view it on GitHub.

jmleroux · 2014-06-13T11:29:02Z

I want it mostly because it helps IDE :)

PHPStorm don't need them : it can read the function prototype and the return type if fixed.
Maybe for Eclipse ?

coveralls · 2014-06-13T11:46:51Z

Coverage decreased (-9.26%) when pulling b3dda6d on jmleroux:route-permissions-guard into f17bf0f on ZF-Commons:master.

bakura10 · 2014-06-13T11:50:52Z

I LIKE THEM :D. That's my reason :p

Envoyé de mon iPhone

Le 13 juin 2014 à 13:29, jean-marie leroux [email protected] a écrit :

I want it mostly because it helps IDE :)

PHPStorm don't need them : it can read the function prototype and the return type if fixed.
Maybe for Eclipse ?

—
Reply to this email directly or view it on GitHub.

coveralls · 2014-06-13T12:02:46Z

Coverage decreased (-5.59%) when pulling 78f8e66 on jmleroux:route-permissions-guard into f17bf0f on ZF-Commons:master.

coveralls · 2014-06-13T12:10:06Z

Coverage decreased (-5.59%) when pulling 8e75f52 on jmleroux:route-permissions-guard into f17bf0f on ZF-Commons:master.

bakura10 · 2014-06-13T12:53:00Z

Looks perfect. Just waiting for tests for the new guard AND doc ;).

bakura10 · 2014-06-13T12:56:10Z

Additionally, I think we should change the default event priority to -10 (like ControllerGuard), so that RouteGuard is always executed first. This allows to clarify the case where you would have both a RouteGuard and RoutePermissionsGuard for the same route.

By default the RouteGuard is registered with -5, ControllerGuard with -10, and I suggest RoutePermissionsGuard to be -10 too (https://github.com/ZF-Commons/zfc-rbac/blob/master/src/ZfcRbac/Guard/ControllerGuard.php#L37)

aeneasr · 2014-06-13T13:00:23Z

👍 on the priority

jmleroux · 2014-06-13T13:12:48Z

It would be best to execute Guards in the order declared in the config.

But for the sake of simplicity, i will change the priority

coveralls · 2014-06-13T13:19:33Z

Coverage decreased (-5.78%) when pulling 9539831 on jmleroux:route-permissions-guard into f17bf0f on ZF-Commons:master.

danizord · 2014-06-13T13:29:44Z

What's the point of mapping multiple permissions for a single route? I think 1 to 1 is more reasonable.

bakura10 · 2014-06-14T21:13:19Z

src/ZfcRbac/Guard/AbstractGuard.php

@@ -18,6 +18,7 @@

 namespace ZfcRbac\Guard;

+use Doctrine\Common\Proxy\Exception\InvalidArgumentException;


What what? Doctrine exception in this part of the code? I don't like that. People can use ZfcRbac without Doctrine!

Oops ! PHPStorm autocompletion can be tricky...

bakura10 · 2014-06-14T21:16:08Z

I will be on holidays the next few days so I won't be able to merge this.

In overall I like it, I just don't like that you included the RULE_AND feature in this PR. Please revert that and only keeps what we need here. I'd prefer you to do that as another PR once this one is merged, so we can discuss it separately.

jmleroux · 2014-06-14T22:18:39Z

I'd prefer you to do that as another PR once this one is merged, so we can discuss it separately.

👍 OK for a next PR. I revert.

coveralls · 2014-06-14T22:27:56Z

Coverage increased (+0.23%) when pulling 6984b01 on jmleroux:route-permissions-guard into 397c676 on ZF-Commons:master.

coveralls · 2014-06-16T22:32:17Z

Coverage increased (+0.14%) when pulling 7926465 on jmleroux:route-permissions-guard into 397c676 on ZF-Commons:master.

bakura10 · 2014-06-18T16:49:40Z

src/ZfcRbac/Guard/ControllerPermissionsGuard.php

@@ -0,0 +1,141 @@
+<?php


Why is the whole file marked as changed? Very hard to know what has changed :/

'cause it's a new one 😉

ControllerPermissionsGuard

bakura10 · 2014-06-18T16:50:46Z

Sorry but it seems a lot lot of files have a complete different change set, and all the lines are marked as strange. I could merge this but I'm a bit worried because I can't know what has changed in all those files that does not directly belong to this PR :/.

jmleroux · 2014-06-18T19:55:55Z

I don't see wich file is worrying you.
Could you annotate ?

jmleroux · 2014-06-18T21:10:58Z

I have changed the title to reflect all the changes of this PR.

aeneasr · 2014-06-19T07:52:44Z

From what I can tell only the GuardPluginManager and docs have changed, all the other files are new additions?

bakura10 · 2014-06-19T09:20:49Z

Sorry, I didn't realized you added a fourth guard. Doc is missing for ControllerPermissionsGuard though.

bakura10 · 2014-06-19T09:22:13Z

src/ZfcRbac/Guard/ControllerPermissionsGuard.php

+
+        foreach ($rules as $rule) {
+            $controller = strtolower($rule['controller']);
+            $actions    = isset($rule['actions']) ? (array) $rule['actions'] : [];


Fix alignment of "="

bakura10 · 2014-06-19T09:27:46Z

Looks good. I'll merge once doc is added for the new guard.

coveralls · 2014-06-19T10:24:29Z

Coverage increased (+0.14%) when pulling f26b666 on jmleroux:route-permissions-guard into 397c676 on ZF-Commons:master.

Add route permissions guard and controller permissions guard

jmleroux · 2014-06-19T12:28:46Z

oops, i modified a test in the meanwhile.
Could you re merge, and i'll update to master

jmleroux · 2014-06-19T12:30:19Z

nevermind, i'll do it on master.
Keep this closed.

bakura10 · 2014-07-25T20:38:16Z

I received a mail notification @davidwindell. But can't find your comment?

davidwindell · 2014-07-25T21:13:19Z

@bakura10 moved it to an issue #242

davidwindell · 2014-07-25T21:13:57Z

Oops I mean #262

Add route permissions guard

5f5e1e7

bakura10 reviewed Jun 13, 2014
View reviewed changes

Moved to src + Docblock

b3dda6d

route permissions guard factory test

78f8e66

typo

8e75f52

change permissions guard priority to 10

9539831

bakura10 reviewed Jun 14, 2014
View reviewed changes

revert rule_type

6984b01

This was referenced Jun 14, 2014

Guards are not really aware of permissions #182

Closed

add permissions capacity to RouteGuard and ControllerGuard #243

Closed

ControllerPermissionsGuard

7926465

bakura10 reviewed Jun 18, 2014
View reviewed changes

jmleroux changed the title ~~Add route permissions guard~~ Add route permissions guard and controller permissions guard Jun 18, 2014

bakura10 reviewed Jun 19, 2014
View reviewed changes

Documentation for ControllerPermissionsGuard

f26b666

bakura10 added a commit that referenced this pull request Jun 19, 2014

Merge pull request #238 from jmleroux/route-permissions-guard

ad6f08a

Add route permissions guard and controller permissions guard

bakura10 merged commit ad6f08a into ZF-Commons:master Jun 19, 2014

davidwindell mentioned this pull request Jul 29, 2014

Conditions for Guards #262

Open

		@@ -18,6 +18,7 @@

		namespace ZfcRbac\Guard;

		use Doctrine\Common\Proxy\Exception\InvalidArgumentException;

Add route permissions guard and controller permissions guard #238

Add route permissions guard and controller permissions guard #238

Conversation

jmleroux commented Jun 13, 2014

bakura10 Jun 13, 2014

Choose a reason for hiding this comment

jmleroux Jun 13, 2014

Choose a reason for hiding this comment

bakura10 commented Jun 13, 2014

aeneasr commented Jun 13, 2014

jmleroux commented Jun 13, 2014

bakura10 commented Jun 13, 2014

jmleroux commented Jun 13, 2014

coveralls commented Jun 13, 2014

bakura10 commented Jun 13, 2014

coveralls commented Jun 13, 2014

coveralls commented Jun 13, 2014

bakura10 commented Jun 13, 2014

bakura10 commented Jun 13, 2014

aeneasr commented Jun 13, 2014

jmleroux commented Jun 13, 2014

coveralls commented Jun 13, 2014

danizord commented Jun 13, 2014

bakura10 Jun 14, 2014

Choose a reason for hiding this comment

jmleroux Jun 14, 2014

Choose a reason for hiding this comment

bakura10 commented Jun 14, 2014

jmleroux commented Jun 14, 2014

coveralls commented Jun 14, 2014

coveralls commented Jun 16, 2014

bakura10 Jun 18, 2014

Choose a reason for hiding this comment

jmleroux Jun 18, 2014

Choose a reason for hiding this comment

bakura10 commented Jun 18, 2014

jmleroux commented Jun 18, 2014

jmleroux commented Jun 18, 2014

aeneasr commented Jun 19, 2014

bakura10 commented Jun 19, 2014

bakura10 Jun 19, 2014

Choose a reason for hiding this comment

bakura10 commented Jun 19, 2014

coveralls commented Jun 19, 2014

jmleroux commented Jun 19, 2014

jmleroux commented Jun 19, 2014

bakura10 commented Jul 25, 2014

davidwindell commented Jul 25, 2014

davidwindell commented Jul 25, 2014