[katello-devel] Permission Matrix discussion

[Lukas Zapletal]

 >>>
So I looked at the "Org" table in the latest version of the 
https://fedorahosted.org/katello/wiki/PermissionMatrix and see that 
there is an inconsistency between the way Lukas (1st proposal) and 
Justin (2nd) seem to define the permission. 	
 >>>

Well Justin's version is pretty same as my "rough version" and the only 
difference is instead of database rules

allowed_to?(create_env, organizations, org_id)
allowed_to?(read_env, organizations, org_id)
allowed_to?(update_env, organizations, org_id)
allowed_to?(destroy_env, organizations, org_id)

there are

allowed_to?(read, organizations, org_id)
allowed_to?(update, organizations, org_id)

The former version gives us possibility to assign particular verbs 
(CRUD) on (all) environments without additional checks (but there's need 
of creating tags), the latter integrates all CUD operations into one 
(update). I have no problem with that :-)

Now let me analyze this more deeply how to actually implement this (2nd) 
approach.

There is no need of modifying our RBAC codebase, and it is very easy to 
implement all cases from the wiki page, including the 2nd one. If you 
look here

https://engineering.redhat.com/trac/kalpana/wiki/Authorization

and open up authorization.rb method enforce_permission

<snip>

   # performs permission checks - do not override this method and
   # create check_permissions(operation) instead and return true if
   # permission is granted or call access_denied(operation) explicitely
   # for an intermediate denial

   def enforce_permissions operation
     # we get called again with the operation being set to create
     return true if operation == :update and new_record?

     # method A - developer defined permissions
     #            (can veto using SecurityViolation)
     if respond_to? :check_permissions
       return true if check_permissions operation
     end

     # method B - database (user) defined permissions
     return true if enforce_db_permissions operation

     # authorization unsuccessful
     access_denied operation
   end

   # enforce permissions from database for regular users

   def enforce_db_permissions(operation)
     User.allowed_to?(operation, type_name, self.id.to_s) or
       User.allowed_to?(operation, type_name)
   end

</snip>

you can see the RBAC stack is calling developer-defined checks first 
(check_permission method). In this case it will contain our 
"allowed_to?(update, organizations, org_id)" statement:

Now there are two more scenarios here:

A) If the permission is not found (allowed_to? returns false) we can 
raise SecurityViolation and access is denied. That's the simple case 
which does not allow finer granularity.

B) Or we can just return false here and the RBAC code will try to 
perform one additional check (see enforce_db_permissions method) - it 
tries to find this rule (assuming update operation) with (and also 
without) the record id. In this case (environments) it would be 
something like:

allowed_to?(:update, 'environments', 'id')

When a permission is found access is granted, denied otherwise. If we 
allow user to define permissions on environment resources in the UI (he 
currently can as all database tables are able to return tags (ids) and 
they are included there the granularity can be quite fine here.

Its upon us if we allow users to "post-define" (its not redefinition 
because developer-defined check goes first) rules or not. If we hide 
environment from the permission UI pane he wont be able to do that. The 
framework is ready.

Please note user would be also allowed to create permission for "all" 
environments providing "any" option in the UI screen. Because this 
permission whould be stored without an id and the RBAC code checks for 
this as well.

So at the end of the day it would be something like:

<snip>

   def check_permissions operation
     allowed_to?(:update, type_name, id) || raise SecurityViolation
   end

</snip>

for the first case (not allowing user to "post-define" additional 
permission), or

<snip>

   def check_permissions operation
     allowed_to?(:update, type_name, id)
   end

</snip>

allowing him to do that.

Wrapping up I am perfectly fine with the 2nd approach. It will just need 
one more line in the check_permission method - but that's the reason why 
did I created such a framework. The "S2 - fine appoach" would not need 
that giving the same result without explicit check_permission 
implementation, but also creating more tags in the RBAC tables (and some 
code for that).

Protecting REST resources is another story, but I would strongly suggest 
to start with protecting controllers and database resources first. The 
latter is being discussed now. Framework for the former is also 
in-place, the only issue is to unify protection of API and UI 
controllers (probably conventions over configuration, or with aliases).

LZ

-- 
Later,

  Lukas Zapletal | E32E400A
  RHN Satellite Engineering
  Red Hat Czech s.r.o. Brno