launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-permission-type into lp:launchpad.

Commit message:
Add GitPermissionType and GitRuleGrant.permissions abstractions.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-permission-type/+merge/357426

At the moment, the new enumeration is mainly just a minor convenience for the GitAPI XML-RPC endpoint, which can now work with an enumeration internally rather than ad-hoc strings and only serialise them when producing a method response.  However, the UI will need to have a vocabulary of common combinations of permissions, and the enumeration will make that much easier.

Similarly, GitRuleGrant.permissions currently just makes it slightly easier for GitAPI to extract all the permissions in one go, but it will make it much easier for the UI to work with permissions.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-permission-type into lp:launchpad.

=== modified file 'lib/lp/code/enums.py'
--- lib/lp/code/enums.py	2018-09-11 16:39:57 +0000
+++ lib/lp/code/enums.py	2018-10-20 00:11:13 +0000
@@ -24,6 +24,7 @@
     'GitActivityType',
     'GitGranteeType',
     'GitObjectType',
+    'GitPermissionType',
     'GitRepositoryType',
     'NON_CVS_RCS_TYPES',
     'RevisionControlSystems',
@@ -218,6 +219,19 @@
     GRANT_REMOVED = DBItem(13, "Removed access grant")
 
 
+class GitPermissionType(EnumeratedType):
+    """Git Permission Type
+
+    A kind of permission that can be granted on part of a Git repository.
+    """
+
+    CAN_CREATE = Item("Can create")
+
+    CAN_PUSH = Item("Can push")
+
+    CAN_FORCE_PUSH = Item("Can force-push")
+
+
 class BranchLifecycleStatusFilter(EnumeratedType):
     """Branch Lifecycle Status Filter
 

=== modified file 'lib/lp/code/interfaces/gitrule.py'
--- lib/lp/code/interfaces/gitrule.py	2018-10-18 16:06:38 +0000
+++ lib/lp/code/interfaces/gitrule.py	2018-10-20 00:11:13 +0000
@@ -22,12 +22,16 @@
     Bool,
     Choice,
     Datetime,
+    FrozenSet,
     Int,
     TextLine,
     )
 
 from lp import _
-from lp.code.enums import GitGranteeType
+from lp.code.enums import (
+    GitGranteeType,
+    GitPermissionType,
+    )
 from lp.code.interfaces.gitrepository import IGitRepository
 from lp.services.fields import (
     PersonChoice,
@@ -173,6 +177,11 @@
         title=_("Can force-push"), required=True, readonly=False,
         description=_("Whether force-pushing references is allowed."))
 
+    permissions = FrozenSet(
+        title=_("Permissions"), required=True, readonly=False,
+        value_type=Choice(vocabulary=GitPermissionType),
+        description=_("The permissions granted by this grant."))
+
 
 class IGitRuleGrantEdit(Interface):
     """`IGitRuleGrant` attributes that require launchpad.Edit."""

=== modified file 'lib/lp/code/model/gitrule.py'
--- lib/lp/code/model/gitrule.py	2018-10-18 16:06:38 +0000
+++ lib/lp/code/model/gitrule.py	2018-10-20 00:11:13 +0000
@@ -42,7 +42,10 @@
     )
 from zope.security.proxy import removeSecurityProxy
 
-from lp.code.enums import GitGranteeType
+from lp.code.enums import (
+    GitGranteeType,
+    GitPermissionType,
+    )
 from lp.code.interfaces.gitactivity import IGitActivitySet
 from lp.code.interfaces.gitrule import (
     IGitNascentRuleGrant,
@@ -128,8 +131,17 @@
             GitRuleGrant, GitRuleGrant.rule_id == self.id)
 
     def addGrant(self, grantee, grantor, can_create=False, can_push=False,
-                 can_force_push=False):
+                 can_force_push=False, permissions=None):
         """See `IGitRule`."""
+        if permissions is not None:
+            if can_create or can_push or can_force_push:
+                raise AssertionError(
+                    "GitRule.addGrant takes either "
+                    "can_create/can_push/can_force_push or permissions, not "
+                    "both")
+            can_create = GitPermissionType.CAN_CREATE in permissions
+            can_push = GitPermissionType.CAN_PUSH in permissions
+            can_force_push = GitPermissionType.CAN_FORCE_PUSH in permissions
         grant = GitRuleGrant(
             rule=self, grantee=grantee, can_create=can_create,
             can_push=can_push, can_force_push=can_force_push, grantor=grantor,
@@ -286,6 +298,23 @@
             ", ".join(permissions), grantee_name, self.repository.unique_name,
             self.rule.ref_pattern)
 
+    @property
+    def permissions(self):
+        permissions = set()
+        if self.can_create:
+            permissions.add(GitPermissionType.CAN_CREATE)
+        if self.can_push:
+            permissions.add(GitPermissionType.CAN_PUSH)
+        if self.can_force_push:
+            permissions.add(GitPermissionType.CAN_FORCE_PUSH)
+        return frozenset(permissions)
+
+    @permissions.setter
+    def permissions(self, value):
+        self.can_create = GitPermissionType.CAN_CREATE in value
+        self.can_push = GitPermissionType.CAN_PUSH in value
+        self.can_force_push = GitPermissionType.CAN_FORCE_PUSH in value
+
     def toDataForJSON(self, media_type):
         """See `IJSONPublishable`."""
         if media_type != "application/json":

=== modified file 'lib/lp/code/model/tests/test_gitrule.py'
--- lib/lp/code/model/tests/test_gitrule.py	2018-10-16 15:29:37 +0000
+++ lib/lp/code/model/tests/test_gitrule.py	2018-10-20 00:11:13 +0000
@@ -26,6 +26,7 @@
 from lp.code.enums import (
     GitActivityType,
     GitGranteeType,
+    GitPermissionType,
     )
 from lp.code.interfaces.gitrule import (
     IGitNascentRuleGrant,
@@ -126,6 +127,47 @@
                 can_push=Is(False),
                 can_force_push=Is(True))))
 
+    def test_addGrant_refuses_inconsistent_permissions(self):
+        rule = self.factory.makeGitRule()
+        with person_logged_in(rule.repository.owner):
+            self.assertRaises(
+                AssertionError, rule.addGrant,
+                GitGranteeType.REPOSITORY_OWNER, rule.repository.owner,
+                can_create=True, can_push=True,
+                permissions={GitPermissionType.CAN_CREATE})
+
+    def test_addGrant_broken_down_permissions(self):
+        rule = self.factory.makeGitRule()
+        with person_logged_in(rule.repository.owner):
+            grant = rule.addGrant(
+                GitGranteeType.REPOSITORY_OWNER, rule.repository.owner,
+                can_create=True, can_push=True)
+        self.assertThat(grant, MatchesStructure(
+            rule=Equals(rule),
+            grantee_type=Equals(GitGranteeType.REPOSITORY_OWNER),
+            grantee=Is(None),
+            grantor=Equals(rule.repository.owner),
+            can_create=Is(True),
+            can_push=Is(True),
+            can_force_push=Is(False)))
+
+    def test_addGrant_combined_permissions(self):
+        rule = self.factory.makeGitRule()
+        with person_logged_in(rule.repository.owner):
+            grant = rule.addGrant(
+                GitGranteeType.REPOSITORY_OWNER, rule.repository.owner,
+                permissions={
+                    GitPermissionType.CAN_CREATE, GitPermissionType.CAN_PUSH,
+                    })
+        self.assertThat(grant, MatchesStructure(
+            rule=Equals(rule),
+            grantee_type=Equals(GitGranteeType.REPOSITORY_OWNER),
+            grantee=Is(None),
+            grantor=Equals(rule.repository.owner),
+            can_create=Is(True),
+            can_push=Is(True),
+            can_force_push=Is(False)))
+
     def test__validateGrants_ok(self):
         rule = self.factory.makeGitRule()
         grants = [
@@ -604,6 +646,19 @@
                 grantee.name, rule.repository.unique_name, rule.ref_pattern),
             repr(grant))
 
+    def test_permissions(self):
+        grant = self.factory.makeGitRuleGrant(can_push=True)
+        self.assertEqual(
+            frozenset({GitPermissionType.CAN_PUSH}), grant.permissions)
+        new_permissions = {
+            GitPermissionType.CAN_CREATE, GitPermissionType.CAN_FORCE_PUSH}
+        with person_logged_in(grant.repository.owner):
+            grant.permissions = new_permissions
+        self.assertEqual(frozenset(new_permissions), grant.permissions)
+        self.assertTrue(grant.can_create)
+        self.assertFalse(grant.can_push)
+        self.assertTrue(grant.can_force_push)
+
     def test_activity_grant_added(self):
         owner = self.factory.makeTeam()
         member = self.factory.makePerson(member_of=[owner])

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2018-10-19 08:50:55 +0000
+++ lib/lp/code/xmlrpc/git.py	2018-10-20 00:11:13 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementations of the XML-RPC APIs for Git."""
@@ -28,6 +28,7 @@
 from lp.app.validators import LaunchpadValidationError
 from lp.code.enums import (
     GitGranteeType,
+    GitPermissionType,
     GitRepositoryType,
     )
 from lp.code.errors import (
@@ -331,28 +332,23 @@
             # Only macaroons are supported for password authentication.
             return faults.Unauthorized()
 
-    def _sortPermissions(self, set_of_permissions):
-        """Sort an iterable of permission strings for consistency"""
+    def _renderPermissions(self, set_of_permissions):
+        """Render a set of permission strings for XML-RPC output."""
         permissions = []
-        if 'create' in set_of_permissions:
+        if GitPermissionType.CAN_CREATE in set_of_permissions:
             permissions.append('create')
-        if 'push' in set_of_permissions:
+        if GitPermissionType.CAN_PUSH in set_of_permissions:
             permissions.append('push')
-        if 'force_push' in set_of_permissions:
+        if GitPermissionType.CAN_FORCE_PUSH in set_of_permissions:
             permissions.append('force_push')
         return permissions
 
     def _buildPermissions(self, grant):
         """Build a set of the available permissions from a GitRuleGrant"""
-        permissions = set()
-        if grant.can_create:
-            permissions.add('create')
-        if grant.can_push:
-            permissions.add('push')
-        if grant.can_force_push:
-            # can_force_push implies can_push.
-            permissions.add('push')
-            permissions.add('force_push')
+        permissions = set(grant.permissions)
+        if GitPermissionType.CAN_FORCE_PUSH in permissions:
+            # Permission to force-push implies permission to push.
+            permissions.add(GitPermissionType.CAN_PUSH)
         return permissions
 
     def _findMatchingRules(self, repository, ref_path):
@@ -394,10 +390,10 @@
 
             owner_type = (None, GitGranteeType.REPOSITORY_OWNER)
             if is_owner and owner_type not in seen_grantees:
-                union_permissions.update(['create', 'push'])
+                union_permissions.update(
+                    [GitPermissionType.CAN_CREATE, GitPermissionType.CAN_PUSH])
 
-            sorted_permissions = self._sortPermissions(union_permissions)
-            result[ref] = sorted_permissions
+            result[ref] = self._renderPermissions(union_permissions)
         return result
 
     def checkRefPermissions(self, translated_path, ref_paths, auth_params):