Removed Zfc/Rbac dependency and passed all the tests with some changes. #36

ALTAMASH80 · 2023-07-06T15:26:49Z

Only RbacCollecterTest remains to pass.

…s in tests. Signed-off-by: Mubashir <[email protected]>

visto9259 · 2023-07-11T13:27:54Z

Thanks @ALTAMASH80.
I will look into it in August.
In the meantime, what are your thoughts on applying these changes to LmcRbac as well? My thinking to have LmcRbac handle all RBAC and roles and have LmcRbacMvc only handle MVC related components like guards, strategies. And then have LmcRbacMvc based on LmcRbac.

ALTAMASH80 · 2023-07-11T14:10:13Z

Hi @visto9259,
It would be great to break LmcRbacMVC into two parts due to the fact that LmcRbac can be used in other custom projects. In that way, LmcRbacMVC has a dependency on LmcRbac which is fine by me. Also, others will agree on this. I hope. But I don't have the vision to break it into two parts. If you already have an idea then do let me know what to do and I'll separate the two. Thanks!

ALTAMASH80 · 2023-08-04T14:55:11Z

Hi @visto9259, just wanted did you get the time to go through the changes. You did say you'll look at it in August. Threfore, I thought I should give you a reminder. Thanks!

visto9259 · 2023-08-21T13:50:51Z

Hi @ALTAMASH80,

Thanks for the reminder. I've been busy with many non-work related issues.

I will take a look shortly

ALTAMASH80 · 2023-10-11T08:58:31Z

Hi @visto9259, I just wanted to know about the progress of removing the dependency of zfc-rbac. Are there any issues with the current commit that need to be resolved? Thanks!

dkmuir · 2024-02-12T00:34:50Z

Any movement on this?

ALTAMASH80 · 2024-02-12T08:03:52Z

@dkmuir, I've presented my solution and it is working. A review is required before merging. So, I'm still waiting for it to be merged into the official repo. Thanks!

visto9259 · 2024-02-12T15:19:50Z

This is long overdue on my end to look into. I will get to it.
I will most likely merge it as part of a v4 of the library.

ALTAMASH80 · 2024-02-12T17:37:57Z

I'm eagerly waiting for my first major contribution. I hope it will help someone.

visto9259

Please see my review comments.
It needs work before we can merge into branch 4x-dev.
I need to think about getting rid of anything that we are adding to the Laminas-permissions-rbac library, even if it means have backward compatibility issues. zf-fr Rbac was, as I understand it, a precursor to laminas-permissions-rbac. If we are not using these extensions within lmc-rbac-mvc, then I think we should get rid of them.
Otherwise, that was a lot a work on your part. Thanks for the input

visto9259 · 2024-02-12T18:40:56Z

tests/Factory/RoleServiceFactoryTest.php

On line 63, there is a mocking of Rbac\Rbac again that should be removed.

visto9259 · 2024-02-12T18:56:37Z

src/Factory/AuthorizationServiceFactory.php

-        /* @var Rbac $rbac */
-        $rbac = $container->get(Rbac::class);
+        /* @var \Laminas\Permissions\Rbac\Rbac $rbac */
+        $rbac = $container->get(\Rbac\Rbac::class);


There should not be any reference to Rbac\Rbac as a service.
It passed the tests because Rbac\Rbac is being mocked in the Factory test suite. See comments in the corresponsind Test case.

In addition, the service 'Rbac\Rbac' is defined in module.config.php and it should not since there is no longer a namespace called 'Rbac'

visto9259 · 2024-02-12T18:58:03Z

tests/Factory/AuthorizationServiceFactoryTest.php

@@ -31,7 +31,7 @@ public function testFactory()
    {
        $serviceManager = new ServiceManager();

-        $serviceManager->setService('Rbac\Rbac', $this->getMockBuilder('Rbac\Rbac')->disableOriginalConstructor()->getMock());
+        $serviceManager->setService('Rbac\Rbac', $this->getMockBuilder(\LmcRbacMvc\Rbac\Rbac::class)->disableOriginalConstructor()->getMock());


You should not be mocking the Rbac\Rbac service since it defeats the purpose of removing references to Rbac and make the test pass when it should not have.

@visto9259, the service call here is accessing LmcRbac\Rbac as defined here..

I know but this is misleading. Since we are removing references to Rbac\Rbac, we should not have a service that creates an object called Rbac\Rbac.

If you want the Rbac\Rbac to be removed it may lead to a backward compatibility break. I kept it because I saw something similar in Laminas MVC to keep backward compatibility. If the service name should also be changed, I'll change it. Thanks!

visto9259 · 2024-02-12T19:01:42Z

src/Factory/RbacFactory.php

@@ -19,7 +19,7 @@
 namespace LmcRbacMvc\Factory;

 use Psr\Container\ContainerInterface;
-use Rbac\Rbac;
+use LmcRbacMvc\Rbac\Rbac;
 use Rbac\Traversal\Strategy\GeneratorStrategy;


This reference to Rbac needs to be removed

visto9259 · 2024-02-12T19:07:44Z

src/Collector/RbacCollector.php

@@ -154,7 +153,7 @@ private function collectIdentityRolesAndPermissions(RoleService $roleService)
        foreach ($identityRoles as $role) {
            $roleName = $role->getName();

-            if (!$role instanceof HierarchicalRoleInterface) {
+            if (empty($role->hasChildren())) {


hasChildren() returns a boolean.
this should be if (!$role->hasChildren()) instead

visto9259 · 2024-02-12T21:11:44Z

tests/Asset/MockRoleWithPermissionTraversable.php

I did not review this.

visto9259 · 2024-02-12T21:12:02Z

tests/Asset/MockRoleWithPermissionProperty.php

I did not review this.

visto9259 · 2024-02-12T21:12:26Z

tests/Asset/MockRoleWithPermissionMethod.php

I did not review this

visto9259 · 2024-02-12T21:16:33Z

src/Role/RecursiveRoleIteratorStrategy.php

Do we still need this?
Since it comes from zf-fr Rbac, is it being used somewhere within LmcRbacMvc? If not, then I think we should not bring it in LmcRbacMvc

visto9259 · 2024-02-12T21:18:17Z

src/Role/RecursiveRoleIterator.php

Do we still need this?
Since it comes from zf-fr Rbac, is it being used somewhere within LmcRbacMvc? If not, then I think we should not bring it in LmcRbacMvc

visto9259 · 2024-02-12T21:26:46Z

@ALTAMASH80 Thanks for the effort.
As I mentioned in my review comments, we need to strive to get rid of any special extensions that zf-fr/Rbac library added on top of laminas-permissions-rbac if we are not using them withing lmc-rbac-mvc.

ALTAMASH80 · 2024-02-13T14:56:12Z

Hi @visto9259, I'll check it again and will get back to you. Thanks for the very precise comments.

visto9259 · 2024-02-13T15:04:01Z

Hi @ALTAMASH80. You're welcomed.
Let me take a look again at the old Rbac\Rbac class. I think that we should not try to carry over all its methods and properties if we are not using them in LmcRbacMvc. I'll give you an answer before the end of my day (GMT-5).

ALTAMASH80 · 2024-02-13T15:10:59Z

I'll check it again that how come an \Rbac\Rbac instance is compatible with the \LmcRbac\Rbac. The reference \Rbac\Rbac::class remained only as a service name accessibility. But, what you've just shown I can't deny that and it is indeed very helpful. To remove the zfr\Rbac extension traces completely, I may not be able to do that as it will require modification in the current tests as well. Because of the fact it also contains a non-hierarchal permission which will not be compatible with hierachal permission. That is why traces of some zfr-rbac remain.

visto9259 · 2024-02-13T20:12:44Z

@ALTAMASH80
I did a quick check. We can use Role and Rbac from Laminas and it requires changes in a few services and factories.
Laminas' Role is hierarchical and it can replace HierarchicalRole from zf-rbac. It also supports simple arrays of roles.
InMemoryRoleProvider uses hierachical roles and it can use Laminas Role instead.
I am not familiar enough with Doctrine and ORM to see what needs to be changed in ObjectRepositoryRoleProvider

There is an important difference between Laminas Rbac and zf-fr Rbac: the isGranted method in zf-fr Rbac accepts an array of RoleInterface but Laminas Rbac does not. In other words, the isGranted method only checks one role, not an array of roles.
This means that any calls to the isGranted method of the Rbac object must reviewed and modify to iterate through roles. This is only the case in the AuthorizationService.

Overall, Rbac is used internally and, to my knowledge, never supplied externally by the application using LmcRbacMvc. For Role, roles are created by role providers which could be supplied by the application. It those cases, it's a question of documentating the breaking change such that users modify their providers to provide roles compliant with Laminas RoleInterface.

Test cases will need to be adjusted to take into account these changes.

Let me know if this makes sense and if you need help.

ALTAMASH80 · 2024-02-15T07:46:59Z

Hi, @visto9259 the changes which you want I'm not capable of doing it. I can fix a test but can't create a test. I've not yet understood the philosophy of the LMCRbac and I don't even know how the first test was created and why. I'm just a tool user, not a creator. I can't write tests and I also don't understand the tests which are there in the LmcRbac. Therefore to change or replace a key class, I'm not well versed in that. I hope someone comes up and fix that. I can only make it usable for PHP8 and remove zfr-rbac. But to change their ideology and what they've implemented I can't do that. I hope I've explained myself. Thanks!

visto9259 · 2024-02-15T14:47:30Z

@ALTAMASH80 Thanks for your contribution and this PR.
I will merge it in the dev branch and continue from there.

ALTAMASH80 · 2024-02-22T10:16:48Z

Hi @visto9259, I've checked the code myself and I've not found any Rbac\Rbac instance getting created and all the tests passed. I think you're confused with the usage of 'Rbac\Rbac' and \Rbac\Rbac::class. Essentially both are the same thing. This is used because the service is created by the name 'Rbac\Rbac'; therefore, you assume it is creating a zfr-rbac instance. The instance which is getting returned is the Laminas\Permissions\Rbac. In my opinion this branch is ready to be merged. You can create another enhancement feature to remove 'Rbac\Rbac' service name and replace it with LmcRbacMVC\Rbac or anything you choose. Then we can work on separating doctrine from this repository. Thanks!

visto9259 · 2024-02-22T14:23:52Z

@ALTAMASH80
In our conversation, I was refering to the original code and not necessarily this PR.
I will merge your work into the 4.x-dev branch and finish the modifications where needed.
Thanks for the work

ALTAMASH80 · 2024-02-22T15:25:01Z

@visto9259,
Do let me know when it is merged in the 4.x-dev branch. Then I'll work on the other feature request.
To work on those feature requests, should I use the 4.x-dev branch to work on those issues? For a lazy person like me, that sounds like a good branch to work on those features. Thanks!

visto9259 · 2024-02-22T15:32:19Z

The 4.x-dev branch for all work related to V4 and new development.
For now, the master branch is to provide bug fixes on V3.

ALTAMASH80 · 2024-02-22T16:03:33Z

So, according to your liking, this work I've done is not up to mark for the 4.0? I thought it was. I'm crying! So, I should not expect a raise which I thought I was going to get. I bought a mansion on a big lease. Well, I've to call them off. Thanks!

ALTAMASH80 mentioned this pull request Jul 6, 2023

Remove dependency on Zfr/Rbac #3

Closed

Removed Zfc/Rbac dependency and passed all the tests with some change…

81e48b9

…s in tests. Signed-off-by: Mubashir <[email protected]>

ALTAMASH80 force-pushed the remove-dependency-on-zfr-rbac branch from cf8fe64 to 81e48b9 Compare July 8, 2023 08:56

Merge branch 'master' into remove-dependency-on-zfr-rbac

116ceb8

ALTAMASH80 changed the title ~~Removed Zfc/Rbac dependency but tests are failing.~~ Removed Zfc/Rbac dependency and passed all the tests with some changes. Jul 8, 2023

visto9259 requested changes Feb 12, 2024

View reviewed changes

visto9259 assigned ALTAMASH80 Feb 16, 2024

visto9259 added enhancement New feature or request v4 To be implemented in version 4 labels Feb 16, 2024

visto9259 linked an issue Feb 16, 2024 that may be closed by this pull request

Remove dependency on Zfr/Rbac #3

Closed

visto9259 changed the base branch from master to 4.x-dev February 22, 2024 15:36

visto9259 approved these changes Feb 23, 2024

View reviewed changes

visto9259 merged commit 960c07f into LM-Commons:4.x-dev Feb 23, 2024

Removed Zfc/Rbac dependency and passed all the tests with some changes. #36

Removed Zfc/Rbac dependency and passed all the tests with some changes. #36

Conversation

ALTAMASH80 commented Jul 6, 2023

visto9259 commented Jul 11, 2023

ALTAMASH80 commented Jul 11, 2023

ALTAMASH80 commented Aug 4, 2023

visto9259 commented Aug 21, 2023

ALTAMASH80 commented Oct 11, 2023

dkmuir commented Feb 12, 2024

ALTAMASH80 commented Feb 12, 2024

visto9259 commented Feb 12, 2024

ALTAMASH80 commented Feb 12, 2024

visto9259 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ALTAMASH80 Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

visto9259 commented Feb 12, 2024 • edited Loading

ALTAMASH80 commented Feb 13, 2024

visto9259 commented Feb 13, 2024

ALTAMASH80 commented Feb 13, 2024 • edited Loading

visto9259 commented Feb 13, 2024

ALTAMASH80 commented Feb 15, 2024

visto9259 commented Feb 15, 2024

ALTAMASH80 commented Feb 22, 2024

visto9259 commented Feb 22, 2024

ALTAMASH80 commented Feb 22, 2024

visto9259 commented Feb 22, 2024

ALTAMASH80 commented Feb 22, 2024 • edited Loading

ALTAMASH80 Feb 13, 2024 •

edited

Loading

visto9259 commented Feb 12, 2024 •

edited

Loading

ALTAMASH80 commented Feb 13, 2024 •

edited

Loading

ALTAMASH80 commented Feb 22, 2024 •

edited

Loading