yiisoft / rbac

Role based access control
https://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
67 stars 24 forks source link

RBAC not compliant with SOLID principle #35

Closed agussuhartono closed 4 years ago

agussuhartono commented 4 years ago

RBAC Manager manage multiple object like Role, Permission, Rule but RBAC Manager use single method for manage them

add, remove, update

i think this method not compliant with solid principle on S = Single responsibility, and I = Interface segregation

Make fine grained interfaces that are client specific.

The principle points that an interface should not define more functionality that is actually used at the same time. It is like single responsibiltiy but for interfaces. In other words: if an interface does differnt things, break it into multiple more focused interfaces.

use dedicated metthod for each object i think more better

add() -> addRole(), addPermision, AddRule addChild() -> addRoleChild() remove() -> removeRole(), removePermision(), removeRule()

currenly for more readable code, i'm use meaning variable name as solution

$role = ...;
$manager->add($role); meaning add role object to Manager 

regards mr_a_ton

ref : https://github.com/samdark/yii2-cookbook/blob/master/book/solid.md

samdark commented 4 years ago

While proposal may make sense, in fact these methods comply with SRP and ISP well:

  1. SRP: there is a single reason to change about these methods - changing on how hierarchy is buillt.
  2. ISP: there is a single client (whatever calls RBAC manager methods) and usually when hierarchy is being built, all the methods are used together.
romkatsu commented 4 years ago

I'm also afraid of the number of occurrences in the code "instanceof"

I agree that it is worth dividing the methods into addRole, addPermision...

Also, in my opinion, a good solution would be to divide everything into storage and Manager.

samdark commented 4 years ago

@romkatsu agree.