From bf41aedc9c2e37f5a03b33fc08396d600016f4b3 Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Tue, 2 May 2017 13:11:57 -0400
Subject: [PATCH] Have gitlab default to True on permissions if they are
 missing

This allows the repositories to be selected in the UI, if we are unsure whether the user has permission. Since gitlab will do the check anyway, this is safe, although not a great user experience if they chose an invalid repository, but we can't really do much about that.
---
 buildtrigger/gitlabhandler.py           | 15 ++++++++++++-
 buildtrigger/test/gitlabmock.py         | 28 +++++++++++++++----------
 buildtrigger/test/test_gitlabhandler.py |  9 +++++++-
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/buildtrigger/gitlabhandler.py b/buildtrigger/gitlabhandler.py
index 7abedc2d0..3decdca07 100644
--- a/buildtrigger/gitlabhandler.py
+++ b/buildtrigger/gitlabhandler.py
@@ -309,9 +309,22 @@ class GitLabBuildTrigger(BuildTriggerHandler):
     def repo_view(repo):
       # Because *anything* can be None in GitLab API!
       permissions = repo.get('permissions') or {}
+      group_access = permissions.get('group_access') or {}
       project_access = permissions.get('project_access') or {}
-      access_level = project_access.get('access_level') or 0
+
+      missing_group_access = permissions.get('group_access') is None
+      missing_project_access = permissions.get('project_access') is None
+
+      access_level = max(group_access.get('access_level') or 0,
+                         project_access.get('access_level') or 0)
+
       has_admin_permission = _ACCESS_LEVEL_MAP.get(access_level, ("", False))[1]
+      if missing_group_access or missing_project_access:
+        # Default to has permission if we cannot check the permissions. This will allow our users
+        # to select the repository and then GitLab's own checks will ensure that the webhook is
+        # added only if allowed.
+        # TODO: Do we want to display this differently in the UI?
+        has_admin_permission = True
 
       view = {
         'name': repo['path'],
diff --git a/buildtrigger/test/gitlabmock.py b/buildtrigger/test/gitlabmock.py
index c15f42a6b..9ac201b10 100644
--- a/buildtrigger/test/gitlabmock.py
+++ b/buildtrigger/test/gitlabmock.py
@@ -12,7 +12,7 @@ def get_gitlab_trigger(dockerfile_path=''):
     'username': 'knownuser'
   })
 
-  trigger._get_authorized_client = get_mock_gitlab(with_nullavatar=False)
+  trigger._get_authorized_client = get_mock_gitlab(with_nulls=False)
   return trigger
 
 def adddeploykey_mock(project_id, name, public_key):
@@ -29,10 +29,15 @@ def get_currentuser_mock():
 def project(namespace, name, is_org=False):
   project_access = None
 
-  if namespace == 'knownuser':
-    project_access = {
-      'access_level': 50,
-    }
+  if name != 'null':
+    if namespace == 'knownuser':
+      project_access = {
+        'access_level': 50,
+      }
+    else:
+      project_access = {
+        'access_level': 0,
+      }
 
   data = {
     'id': '%s/%s' % (namespace, name),
@@ -51,13 +56,14 @@ def project(namespace, name, is_org=False):
     'public': name != 'somerepo',
     'permissions': {
       'project_access': project_access,
+      'group_access': {'access_level': 0},
     },
     'owner': {
       'avatar_url': 'avatarurl',
     }
   }
 
-  if name == 'nullavatar':
+  if name == 'null':
     del data['owner']['avatar_url']
     data['namespace']['avatar'] = None
   elif is_org:
@@ -66,11 +72,11 @@ def project(namespace, name, is_org=False):
 
   return data
 
-def getprojects_mock(with_nullavatar=False):
-  if with_nullavatar:
+def getprojects_mock(with_nulls=False):
+  if with_nulls:
     def _getprojs(page=1, per_page=100):
       return [
-        project('someorg', 'nullavatar', is_org=True),
+        project('someorg', 'null', is_org=True),
       ]
     return _getprojs
 
@@ -183,7 +189,7 @@ def getrawfile_mock(repo_id, branch_name, path):
 
   return False
 
-def get_mock_gitlab(with_nullavatar=False):
+def get_mock_gitlab(with_nulls=False):
   def _get_mock():
     mock_gitlab = Mock()
     mock_gitlab.host = 'https://bitbucket.org'
@@ -191,7 +197,7 @@ def get_mock_gitlab(with_nullavatar=False):
     mock_gitlab.currentuser = Mock(side_effect=get_currentuser_mock)
     mock_gitlab.getusers = Mock(side_effect=getusers_mock)
 
-    mock_gitlab.getprojects = Mock(side_effect=getprojects_mock(with_nullavatar))
+    mock_gitlab.getprojects = Mock(side_effect=getprojects_mock(with_nulls))
     mock_gitlab.getproject = Mock(side_effect=getproject_mock)
     mock_gitlab.getbranches = Mock(side_effect=getbranches_mock)
 
diff --git a/buildtrigger/test/test_gitlabhandler.py b/buildtrigger/test/test_gitlabhandler.py
index f8a8931c6..fae6f4989 100644
--- a/buildtrigger/test/test_gitlabhandler.py
+++ b/buildtrigger/test/test_gitlabhandler.py
@@ -37,8 +37,15 @@ def test_lookup_user(email, expected_response, gitlab_trigger):
   assert gitlab_trigger.lookup_user(email) == expected_response
 
 
+def test_null_permissions(gitlab_trigger):
+  gitlab_trigger._get_authorized_client = get_mock_gitlab(with_nulls=True)
+  sources = gitlab_trigger.list_build_sources_for_namespace('someorg')
+  source = sources[0]
+  assert source['has_admin_permissions']
+
+
 def test_null_avatar(gitlab_trigger):
-  gitlab_trigger._get_authorized_client = get_mock_gitlab(with_nullavatar=True)
+  gitlab_trigger._get_authorized_client = get_mock_gitlab(with_nulls=True)
   namespace_data = gitlab_trigger.list_build_source_namespaces()
   expected = {
     'avatar_url': None,