comments and simplifications of the main effecitve permission method

Ouranosinc · Oct 21, 2020 · 9f6dda2 · 9f6dda2
1 parent 81b46a2
commit 9f6dda2
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 6 deletions.
diff --git a/docs/permissions.rst b/docs/permissions.rst
@@ -469,6 +469,7 @@ Therefore, it can be noted that all API responses that contain details about per
                 "access": "allow|deny",
                 "scope": "match|recursive",
                 "type": "access|allowed|applied|direct|inherited|effective|owned"
+            }
         ]
     }
 

diff --git a/magpie/services.py b/magpie/services.py
@@ -285,6 +285,12 @@ def effective_permissions(self, user, resource, permissions=None, allow_match=Tr
                     # if user is owner (directly or via groups), all permissions are set,
                     # but continue processing this resource until end in case user explicit deny reverts it
                     if perm_tup.perm_name == ALL_PERMISSIONS:
+                        # FIXME:
+                        #   This block needs to be validated if support of ownership rules are added.
+                        #   Conditions must be revised according to wanted behaviour...
+                        #   General idea for now is that explict user/group deny should be prioritized over resource
+                        #   ownership permissions since these can be attributed to *any user* while explicit deny are
+                        #   definitely set by an admin-level user.
                         for perm in requested_perms:
                             all_perm = PermissionSet(perm, perm_set.access, perm.scope, perm.type)
                             if perm_set.access == Access.DENY:
@@ -298,20 +304,25 @@ def effective_permissions(self, user, resource, permissions=None, allow_match=Tr
                     if not match and perm_set.scope == Scope.MATCH:
                         continue
 
+                    # less obvious use case for both of the following user/group blocks:
+                    #   no need to check explicitly for ALLOW since it was either already set during previous iteration
+                    #   (at that moment, perm=None) or a DENY was already set, but it takes precedence over it anyway
+
                     # user direct permissions have priority over inherited ones from groups
                     if perm_set.type == PermissionType.DIRECT:
                         perm = effective_perms.get(perm_name)
-                        # explicit user deny overrides user allow if any already found
+                        # explicit user DENY overrides user ALLOW if any already found
+                        # if permission name not already found, ALLOW/DENY is set regardless (first occurrence)
                         if perm is None or perm.type == PermissionType.INHERITED or perm_set.access == Access.DENY:
                             effective_perms[perm_name] = perm_set
                         continue  # final decision for this user, skip any group permissions
 
-                    # otherwise check if for another group permission and yet no user permission
+                    # otherwise check for group permission
+                    # like previously, explicit DENY overrides ALLOW if permission name was already found
+                    # if permission name not already found, ALLOW/DENY is set regardless (first occurrence)
                     perm = effective_perms.get(perm_name)
-                    if perm is None or perm.type == PermissionType.INHERITED:
-                        # explicit deny overrides allow if any already found
-                        if perm is None or perm_set.access == Access.DENY:
-                            effective_perms[perm_name] = perm_set
+                    if perm is None or (perm.type == PermissionType.INHERITED and perm_set.access == Access.DENY):
+                        effective_perms[perm_name] = perm_set
 
             # don't bother moving to parent if everything is resolved already
             if len(effective_perms) == len(requested_perms):