launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging lp:~twom/launchpad/branch-permissions-empty-default into lp:launchpad.

Commit message:
Add default no permissions rule to none repository owners

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/launchpad/branch-permissions-empty-default/+merge/356914

Users that have access to a repository should have a default no permissions grant, unless overridden by a specific grant to them.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~twom/launchpad/branch-permissions-empty-default into lp:launchpad.

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2018-10-15 13:36:07 +0000
+++ lib/lp/code/xmlrpc/git.py	2018-10-17 10:20:54 +0000
@@ -373,10 +373,6 @@
                      'permissions': ['create', 'push'],
                     })
                 continue
-            # If we otherwise don't have any matching grants,
-            # we can ignore this rule
-            if not matching_grants:
-                continue
 
             # Permissions are a union of all the applicable grants
             union_permissions = set()
@@ -397,14 +393,16 @@
                  'permissions': sorted_permissions,
                 })
 
-        # The last rule for a repository owner is a default permission
-        # for everything. This is overridden by any matching rules previously
-        # in the list
+        # The last rule is a default wildcard. The repository owner gets
+        # permissions to everything, whereas other people get no permissions.
         if is_owner:
-            result.append(
-                {'ref_pattern': '*',
-                 'permissions': ['create', 'push', 'force_push'],
-                })
+            default_permissions = ['create', 'push', 'force_push']
+        else:
+            default_permissions = []
+        result.append(
+            {'ref_pattern': '*',
+             'permissions': default_permissions,
+            })
 
         return result
 

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2018-10-15 13:36:07 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2018-10-17 10:20:54 +0000
@@ -284,6 +284,10 @@
                 'ref_pattern': Equals('refs/heads/*'),
                 'permissions': Equals(['create', 'push']),
                 }),
+            MatchesDict({
+                'ref_pattern': Equals('*'),
+                'permissions': Equals([]),
+                }),
             ]))
 
     def test_listRefRules_with_other_grants(self):
@@ -306,7 +310,11 @@
             MatchesDict({
                 'ref_pattern': Equals('refs/heads/*'),
                 'permissions': Equals(['push']),
-                })
+                }),
+            MatchesDict({
+                'ref_pattern': Equals('*'),
+                'permissions': Equals([]),
+                }),
             ]))
 
     def test_listRefRules_owner_has_default(self):
@@ -419,7 +427,16 @@
         results = self.git_api.listRefRules(
             repository.getInternalPath(),
             {'uid': requester.id})
-        self.assertEqual(0, len(results))
+        self.assertThat(results, MatchesListwise([
+            MatchesDict({
+                'ref_pattern': Equals('refs/heads/*'),
+                'permissions': Equals([]),
+                }),
+            MatchesDict({
+                'ref_pattern': Equals('*'),
+                'permissions': Equals([]),
+                }),
+            ]))
 
     def test_listRefRules_owner_has_default_with_other_grant(self):
         owner = self.factory.makePerson()
@@ -434,7 +451,6 @@
         results = self.git_api.listRefRules(
             repository.getInternalPath(),
             {'uid': owner.id})
-        self.assertEqual(len(results), 2)
         # Default grant should be last in pattern
         self.assertThat(results, MatchesListwise([
             MatchesDict({
@@ -770,6 +786,10 @@
 
         self.assertThat(results, MatchesListwise([
             MatchesDict(
+                {'ref_pattern': Equals('refs/heads/stable/next'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
                 {'ref_pattern': Equals('refs/heads/archived/*'),
                  'permissions': Equals(['create']),
                 }),
@@ -785,6 +805,10 @@
                 {'ref_pattern': Equals('refs/tags/*'),
                  'permissions': Equals(['create']),
                 }),
+            MatchesDict(
+                {'ref_pattern': Equals('*'),
+                 'permissions': Equals([]),
+                }),
             ]))
 
     def test_listRefRules_grantee_example_three(self):
@@ -832,9 +856,100 @@
 
         self.assertThat(results, MatchesListwise([
             MatchesDict(
+                {'ref_pattern': Equals('refs/heads/stable/next'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
+                {'ref_pattern': Equals('refs/heads/archived/*'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
+                {'ref_pattern': Equals('refs/heads/stable/*'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
                 {'ref_pattern': Equals('refs/heads/*/next'),
                  'permissions': Equals(['push', 'force_push']),
                 }),
+            MatchesDict(
+                {'ref_pattern': Equals('refs/tags/*'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
+                {'ref_pattern': Equals('*'),
+                 'permissions': Equals([]),
+                }),
+            ]))
+
+    def test_listRefRules_grantee_example_four(self):
+        user_a = self.factory.makePerson()
+        user_b = self.factory.makePerson()
+        user_c = self.factory.makePerson()
+        user_d = self.factory.makePerson()
+        stable_team = self.factory.makeTeam(members=[user_a, user_b])
+        next_team = self.factory.makeTeam(members=[user_b, user_c])
+
+        repository = removeSecurityProxy(
+            self.factory.makeGitRepository(owner=user_a))
+
+        rule = self.factory.makeGitRule(
+            repository, ref_pattern=u'refs/heads/stable/next')
+        self.factory.makeGitRuleGrant(
+            rule=rule, grantee=user_a, can_force_push=True)
+
+        rule = self.factory.makeGitRule(
+            repository, ref_pattern=u'refs/heads/archived/*')
+        self.factory.makeGitRuleGrant(
+            rule=rule, grantee=user_a)
+        self.factory.makeGitRuleGrant(
+            rule=rule, grantee=user_b, can_create=True)
+
+        rule = self.factory.makeGitRule(
+            repository, ref_pattern=u'refs/heads/stable/*')
+        self.factory.makeGitRuleGrant(
+            rule=rule, grantee=stable_team, can_push=True)
+
+        rule = self.factory.makeGitRule(
+            repository, ref_pattern=u'refs/heads/*/next')
+        self.factory.makeGitRuleGrant(
+            rule=rule, grantee=next_team, can_force_push=True)
+
+        rule = self.factory.makeGitRule(
+            repository, ref_pattern=u'refs/tags/*')
+        self.factory.makeGitRuleGrant(
+            rule=rule, grantee=user_a, can_create=True)
+        self.factory.makeGitRuleGrant(
+            rule=rule, grantee=stable_team, can_create=True)
+
+        results = self.git_api.listRefRules(
+            repository.getInternalPath(),
+            {'uid': user_d.id})
+
+        self.assertThat(results, MatchesListwise([
+            MatchesDict(
+                {'ref_pattern': Equals('refs/heads/stable/next'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
+                {'ref_pattern': Equals('refs/heads/archived/*'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
+                {'ref_pattern': Equals('refs/heads/stable/*'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
+                {'ref_pattern': Equals('refs/heads/*/next'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
+                {'ref_pattern': Equals('refs/tags/*'),
+                 'permissions': Equals([]),
+                }),
+            MatchesDict(
+                {'ref_pattern': Equals('*'),
+                 'permissions': Equals([]),
+                }),
             ]))