From 9c3eac88bf0ece1804f95006ba9e38a504cda006 Mon Sep 17 00:00:00 2001
From: Patrick Uiterwijk <patrick@puiterwijk.org>
Date: Oct 06 2018 19:49:49 +0000
Subject: Make sure that if a user is direct committer, they can always commit


This fixes the case where the user is in an EXTERNAL_COMMITTER group that is
excluded from a specific repository, but they are actually a direct committer
(or in a group that is a direct committer) for that repository.

Example for Fedora Dist-Git: commit for "rpms/firefox" if one of the firefox
maintainers were provenpackager.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>

---

diff --git a/pagure/utils.py b/pagure/utils.py
index d2c3852..a875c1f 100644
--- a/pagure/utils.py
+++ b/pagure/utils.py
@@ -23,6 +23,7 @@ import six
 import werkzeug
 
 from pagure.config import config as pagure_config
+import pagure.lib
 
 
 _log = logging.getLogger(__name__)
@@ -122,41 +123,49 @@ def is_repo_admin(repo_obj, username=None):
 
 def is_repo_committer(repo_obj, username=None):
     """ Return whether the user is a committer of the provided repo. """
-    if not authenticated():
-        return False
-
-    if username:
-        user = username
+    if username is None:
+        if not authenticated():
+            return False
+        if is_admin():
+            return True
+        username = flask.g.fas_user.username
+        usergroups = set(flask.g.fas_user.groups)
     else:
-        user = flask.g.fas_user.username
+        user = pagure.lib.get_user(flask.g.session, username)
+        usergroups = set(user.groups)
 
-    if is_admin():
+    # If the user is main admin -> yep
+    if repo_obj.user.user == username:
         return True
 
-    grps = flask.g.fas_user.groups
+    # If they are in the list of committers -> yep
+    for user in repo_obj.committers:
+        if user.user == username:
+            return True
+
+    # If they are in a group that has commit access -> yep
+    for group in repo_obj.committer_groups:
+        if group in usergroups:
+            return True
+
+    # If no direct committer, check EXTERNAL_COMMITTER info
     ext_committer = pagure_config.get("EXTERNAL_COMMITTER", None)
     if ext_committer:
-        overlap = set(ext_committer).intersection(grps)
+        overlap = set(ext_committer) & usergroups
         if overlap:
             for grp in overlap:
                 restrict = ext_committer[grp].get("restrict", [])
                 exclude = ext_committer[grp].get("exclude", [])
                 if restrict and repo_obj.fullname not in restrict:
-                    return False
+                    continue
                 elif repo_obj.fullname in exclude:
-                    return False
+                    continue
                 else:
                     return True
 
-    usergrps = [
-        usr.user for grp in repo_obj.committer_groups for usr in grp.users
-    ]
-
-    return (
-        user == repo_obj.user.user
-        or (user in [usr.user for usr in repo_obj.committers])
-        or (user in usergrps)
-    )
+    # The user is not in an external_committer group that grants access, and
+    # not a direct committer -> You have no power here
+    return False
 
 
 def is_repo_user(repo_obj, username=None):
diff --git a/tests/test_pagure_flask.py b/tests/test_pagure_flask.py
index 4a5401e..50de84f 100644
--- a/tests/test_pagure_flask.py
+++ b/tests/test_pagure_flask.py
@@ -145,6 +145,22 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest):
             output = pagure.utils.is_repo_committer(repo)
             self.assertFalse(output)
 
+    @mock.patch.dict('pagure.config.config', {'EXTERNAL_COMMITTER': config})
+    def test_is_repo_committer_owner_external_committer_excluding_one(self):
+        """ Test is_repo_committer in pagure with EXTERNAL_COMMITTER
+        configured to give access to all the provenpackager but for this
+        one repo, but the user is still a direct committer
+        """
+        repo = pagure.lib._get_project(self.session, 'test')
+
+        g = munch.Munch()
+        g.fas_user = tests.FakeUser(username='pingou')
+        g.fas_user.groups.append('provenpackager')
+        g.authenticated = True
+        with mock.patch('pagure.flask_app.flask.g', g):
+            output = pagure.utils.is_repo_committer(repo)
+            self.assertTrue(output)
+
     config = {
         'provenpackager': {
             'restrict': ['test']
@@ -197,5 +213,6 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest):
             self.assertFalse(output)
 
 
+
 if __name__ == '__main__':
     unittest.main(verbosity=2)