Merge branch 'allow-owner-to-run-ci-builds' into 'master'

Allow owners to fetch source code in CI builds

Due to different way of handling owners of a project, they were not allowed to fetch CI sources for project.

This adds a separate code path for handling owners, that are not admins.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/23437

See merge request !6943

Signed-off-by: Rémy Coutable <remy@rymai.me>

6 files changed, 66 insertions, 9 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 85fbf6baaff..0baf1d29ae0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 - Honour issue and merge request visibility in their respective finders.
 - Disable reference Markdown for unavailable features.
+- Allow owners to fetch source code in CI builds.
 
 ## 8.13.3 (2016-11-02)
 
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index fbb3d4507d6..1ee31023e26 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -2,11 +2,11 @@ class ProjectPolicy < BasePolicy
   def rules
     team_access!(user)
 
-    owner = user.admin? ||
-            project.owner == user ||
+    owner = project.owner == user ||
             (project.group && project.group.has_owner?(user))
 
-    owner_access! if owner
+    owner_access! if user.admin? || owner
+    team_member_owner_access! if owner
 
     if project.public? || (project.internal? && !user.external?)
       guest_access!
@@ -16,7 +16,7 @@ class ProjectPolicy < BasePolicy
       can! :read_build if project.public_builds?
 
       if project.request_access_enabled &&
-         !(owner || project.team.member?(user) || project_group_member?(user))
+         !(owner || user.admin? || project.team.member?(user) || project_group_member?(user))
         can! :request_access
       end
     end
@@ -135,6 +135,10 @@ class ProjectPolicy < BasePolicy
     can! :destroy_issue
   end
 
+  def team_member_owner_access!
+    team_member_reporter_access!
+  end
+
   # Push abilities on the users team role
   def team_access!(user)
     access = project.team.max_member_access(user.id)
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index a18e5b63ec6..f1d0a190002 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -147,6 +147,14 @@ describe Gitlab::GitAccess, lib: true do
     describe 'build authentication_abilities permissions' do
       let(:authentication_abilities) { build_authentication_abilities }
 
+      describe 'owner' do
+        let(:project) { create(:project, namespace: user.namespace) }
+
+        context 'pull code' do
+          it { expect(subject).to be_allowed }
+        end
+      end
+
       describe 'reporter user' do
         before { project.team << [user, :reporter] }
 
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 658e3c13a73..96249a7d8c3 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -6,6 +6,7 @@ describe ProjectPolicy, models: true do
   let(:dev) { create(:user) }
   let(:master) { create(:user) }
   let(:owner) { create(:user) }
+  let(:admin) { create(:admin) }
   let(:project) { create(:empty_project, :public, namespace: owner.namespace) }
 
   let(:guest_permissions) do
@@ -155,6 +156,19 @@ describe ProjectPolicy, models: true do
       it do
         is_expected.to include(*guest_permissions)
         is_expected.to include(*reporter_permissions)
+        is_expected.to include(*team_member_reporter_permissions)
+        is_expected.to include(*developer_permissions)
+        is_expected.to include(*master_permissions)
+        is_expected.to include(*owner_permissions)
+      end
+    end
+
+    context 'admin' do
+      let(:current_user) { admin }
+
+      it do
+        is_expected.to include(*guest_permissions)
+        is_expected.to include(*reporter_permissions)
         is_expected.not_to include(*team_member_reporter_permissions)
         is_expected.to include(*developer_permissions)
         is_expected.to include(*master_permissions)
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index dbdf83a0dff..9bfc84c7425 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -284,7 +284,17 @@ describe 'Git LFS API and storage' do
           let(:authorization) { authorize_ci_project }
 
           shared_examples 'can download LFS only from own projects' do
-            context 'for own project' do
+            context 'for owned project' do
+              let(:project) { create(:empty_project, namespace: user.namespace) }
+
+              let(:update_permissions) do
+                project.lfs_objects << lfs_object
+              end
+
+              it_behaves_like 'responds with a file'
+            end
+
+            context 'for member of project' do
               let(:pipeline) { create(:ci_empty_pipeline, project: project) }
 
               let(:update_permissions) do
diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb
index c64df4979b0..bb26513103d 100644
--- a/spec/services/auth/container_registry_authentication_service_spec.rb
+++ b/spec/services/auth/container_registry_authentication_service_spec.rb
@@ -245,6 +245,12 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do
 
             it_behaves_like 'a pullable'
           end
+
+          context 'when you are owner' do
+            let(:project) { create(:empty_project, namespace: current_user.namespace) }
+
+            it_behaves_like 'a pullable'
+          end
         end
 
         context 'for private' do
@@ -266,6 +272,12 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do
 
               it_behaves_like 'a pullable'
             end
+
+            context 'when you are owner' do
+              let(:project) { create(:empty_project, namespace: current_user.namespace) }
+
+              it_behaves_like 'a pullable'
+            end
           end
         end
       end
@@ -276,13 +288,21 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do
         end
 
         context 'disallow for all' do
-          let(:project) { create(:empty_project, :public) }
+          context 'when you are member' do
+            let(:project) { create(:empty_project, :public) }
 
-          before do
-            project.team << [current_user, :developer]
+            before do
+              project.team << [current_user, :developer]
+            end
+
+            it_behaves_like 'an inaccessible'
           end
 
-          it_behaves_like 'an inaccessible'
+          context 'when you are owner' do
+            let(:project) { create(:empty_project, :public, namespace: current_user.namespace) }
+
+            it_behaves_like 'an inaccessible'
+          end
         end
       end
     end