F parse acl

Summary

Fixes the parsing of EntityACLs #106

Focus

The parsing uses the grant and deny methods of the ACL class. For convenience reasons, these would remove any present contradicting rules. This makes a lot of sense when an ACL is being constructed from scratch, but for parsing the server's response it is necessary to keep contradicting rules.

Test Environment

No manual testing required. Just check the new unit test.

Check List for the Author

Please, prepare your MR for a review. Be sure to write a summary and a focus and create gitlab comments for the reviewer. They should guide the reviewer through the changes, explain your changes and also point out open questions. For further good practices have a look at our review guidelines

• All automated tests pass
• Reference related issues
• Up-to-date CHANGELOG.md (or not necessary)
• Annotations in code (Gitlab comments)
  • Intent of new code
  • Problems with old code
  • Why this implementation?

Check List for the Reviewer

• I understand the intent of this MR
• All automated tests pass
• Up-to-date CHANGELOG.md (or not necessary)
• The test environment setup works and the intended behavior is reproducible in the test environment
• In-code documentation and comments are up-to-date.
• Check: Are there specifications? Are they satisfied?

For further good practices have a look at our review guidelines.

src/caosdb/common/models.py

@@ -5,9 +5,9 @@
 #
 # Copyright (C) 2018 Research Group Biomedical Physics,
 # Max-Planck-Institute for Dynamics and Self-Organization Göttingen
-# Copyright (C) 2020 Indiscale GmbH <info@indiscale.com>
+# Copyright (C) 2020-2022 Indiscale GmbH <info@indiscale.com>
 # Copyright (C) 2020 Florian Spreckelsen <f.spreckelsen@indiscale.com>
-# Copyright (C) 2020 Timm Fitschen <t.fitschen@indiscale.com>
+# Copyright (C) 2020-2022 Timm Fitschen <t.fitschen@indiscale.com>
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU Affero General Public License as
@@ -25,7 +25,14 @@
 # ** end header
 #
 
-"""missing docstring."""
+"""
+Collection of the central classes of the CaosDB client, namely the Entity class
+and all of its subclasses and the Container class which is used to carry out
+transactions.
+
+All additional classes are either important for the entities or the
+transactions.
+"""
 from __future__ import print_function, unicode_literals
 
 import re
@@ -269,14 +276,74 @@ class Entity(object):
         self.__pickup = new_pickup
 
     def grant(self, realm=None, username=None, role=None,
-              permission=None, priority=False):
+              permission=None, priority=False, revoke_denial=True):
+        """Grant a permission to a user or role for this entity.
+
+        You must specify either only the username and the realm, or only the
+        role.
+
+        By default a previously existing denial rule would be revoked, because
+        otherwise this grant wouldn't have any effect. However, for keeping
+        contradicting rules pass revoke_denial=False.
+
+        Parameters
+        ----------
+        permission: str
+            The permission to be granted.
+        username : str, optional
+            The username. Exactly one is required, either the `username` or the
+            `role`.
+        realm: str, optional
+            The user's realm. Required when username is not None.
+        role: str, optional
+            The role (as in Role-Based Access Control). Exactly one is
+            required, either the `username` or the `role`.
+        priority: bool, default False
+            Whether this permission is granted with priority over non-priority
+            rules.
+        revoke_denial: bool, default True
+            Whether a contradicting denial (with same priority flag) in this
+            ACL will be revoked.
+        """
+        # @review Florian Spreckelsen 2022-03-17
         self.acl.grant(realm=realm, username=username, role=role,
-                       permission=permission, priority=priority)
+                       permission=permission, priority=priority,
+                       revoke_denial=revoke_denial)
 
     def deny(self, realm=None, username=None, role=None,
-             permission=None, priority=False):
+             permission=None, priority=False, revoke_grant=True):
+        """Deny a permission to a user or role for this entity.
+
+        You must specify either only the username and the realm, or only the
+        role.
+
+        By default a previously existing grant rule would be revoked, because
+        otherwise this denial would override the grant rules anyways. However,
+        for keeping contradicting rules pass revoke_grant=False.
+
+        Parameters
+        ----------
+        permission: str
+            The permission to be denied.
+        username : str, optional
+            The username. Exactly one is required, either the `username` or the
+            `role`.
+        realm: str, optional
+            The user's realm. Required when username is not None.
+        role: str, optional
+            The role (as in Role-Based Access Control). Exactly one is
+            required, either the `username` or the `role`.
+        priority: bool, default False
+            Whether this permission is denied with priority over non-priority
+            rules.
+        revoke_grant: bool, default True
+            Whether a contradicting grant (with same priority flag) in this
+            ACL will be revoked.
+        """
+        # @review Florian Spreckelsen 2022-03-17
         self.acl.deny(realm=realm, username=username, role=role,
-                      permission=permission, priority=priority)
+                      permission=permission, priority=priority,
+                      revoke_grant=revoke_grant)
 
     def revoke_denial(self, realm=None, username=None,
                       role=None, permission=None, priority=False):
@@ -3636,13 +3703,15 @@ class ACI():
         self.permission = permission
 
     def __hash__(self):
-        return hash(str(self.realm) + ":" + str(self.username) +
-                    ":" + str(self.role) + ":" + str(self.permission))
+        return hash(self.__repr__())
 
     def __eq__(self, other):
         return isinstance(other, ACI) and (self.role is None and self.username == other.username and self.realm ==
                                            other.realm) or self.role == other.role and self.permission == other.permission
 
+    def __repr__(self):
+        return str(self.realm) + ":" + str(self.username) + ":" + str(self.role) + ":" + str(self.permission)
+
     def add_to_element(self, e):
         if self.role is not None:
             e.set("role", self.role)
@@ -3667,10 +3736,35 @@ class ACL():
             self.clear()
 
     def parse_xml(self, xml):
+        """Clear this ACL and parse the xml.
+
+        Iterate over the rules in the xml and add each rule to this ACL.
+
+        Contradicting rules will both be kept.
+
+        Parameters
+        ----------
+        xml : lxml.etree.Element
+            The xml element containing the ACL rules, i.e. <Grant> and <Deny>
+            rules.
+        """
         self.clear()
         self._parse_xml(xml)
 
     def _parse_xml(self, xml):
+        """Parse the xml.
+
+        Iterate over the rules in the xml and add each rule to this ACL.
+
+        Contradicting rules will both be kept.
+
+        Parameters
+        ----------
+        xml : lxml.etree.Element
+            The xml element containing the ACL rules, i.e. <Grant> and <Deny>
+            rules.
+        """
+        # @review Florian Spreckelsen 2022-03-17
         for e in xml:
             role = e.get("role")
             username = e.get("username")
@@ -3683,10 +3777,12 @@ class ACL():
 
                     if e.tag == "Grant":
                         self.grant(username=username, realm=realm, role=role,
-                                   permission=permission, priority=priority)
+                                   permission=permission, priority=priority,
+                                   revoke_denial=False)
                     elif e.tag == "Deny":
                         self.deny(username=username, realm=realm, role=role,
-                                  permission=permission, priority=priority)
+                                  permission=permission, priority=priority,
+                                  revoke_grant=False)
 
     def combine(self, other):
         """ Combine and return new instance."""
@@ -3764,12 +3860,42 @@ class ACL():
         if item in self._denials:
             self._denials.remove(item)
 
-    def grant(self, username=None, realm=None, role=None,
-              permission=None, priority=False):
+    def grant(self, permission, username=None, realm=None, role=None,
+              priority=False, revoke_denial=True):
+        """Grant a permission to a user or role.
+
+        You must specify either only the username and the realm, or only the
+        role.
+
+        By default a previously existing denial rule would be revoked, because
+        otherwise this grant wouldn't have any effect. However, for keeping
+        contradicting rules pass revoke_denial=False.
+
+        Parameters
+        ----------
+        permission: str
+            The permission to be granted.
+        username : str, optional
+            The username. Exactly one is required, either the `username` or the
+            `role`.
+        realm: str, optional
+            The user's realm. Required when username is not None.
+        role: str, optional
+            The role (as in Role-Based Access Control). Exactly one is
+            required, either the `username` or the `role`.
+        priority: bool, default False
+            Whether this permission is granted with priority over non-priority
+            rules.
+        revoke_denial: bool, default True
+            Whether a contradicting denial (with same priority flag) in this
+            ACL will be revoked.
+        """
+        # @review Florian Spreckelsen 2022-03-17
         priority = self._get_boolean_priority(priority)
         item = ACI(role=role, username=username,
                    realm=realm, permission=permission)
-        self._remove_item(item, priority)
+        if revoke_denial:
+            self._remove_item(item, priority)
 
         if priority is True:
             self._priority_grants.add(item)
@@ -3777,11 +3903,41 @@ class ACL():
             self._grants.add(item)
 
     def deny(self, username=None, realm=None, role=None,
-             permission=None, priority=False):
+             permission=None, priority=False, revoke_grant=True):
+        """Deny a permission to a user or role for this entity.
+
+        You must specify either only the username and the realm, or only the
+        role.
+
+        By default a previously existing grant rule would be revoked, because
+        otherwise this denial would override the grant rules anyways. However,
+        for keeping contradicting rules pass revoke_grant=False.
+
+        Parameters
+        ----------
+        permission: str
+            The permission to be denied.
+        username : str, optional
+            The username. Exactly one is required, either the `username` or the
+            `role`.
+        realm: str, optional
+            The user's realm. Required when username is not None.
+        role: str, optional
+            The role (as in Role-Based Access Control). Exactly one is
+            required, either the `username` or the `role`.
+        priority: bool, default False
+            Whether this permission is denied with priority over non-priority
+            rules.
+        revoke_grant: bool, default True
+            Whether a contradicting grant (with same priority flag) in this
+            ACL will be revoked.
+        """
+        # @review Florian Spreckelsen 2022-03-17
         priority = self._get_boolean_priority(priority)
         item = ACI(role=role, username=username,
                    realm=realm, permission=permission)
-        self._remove_item(item, priority)
+        if revoke_grant:
+            self._remove_item(item, priority)
 
         if priority is True:
             self._priority_denials.add(item)