Merge branch 'ci-runners-master-or-owner' into 'master'

Show specific runners from projects where user is master or owner

This fix for permission escalation when handling specific runners.

The users were allowed to assign runners from projects where they were guests.


See merge request !1809

3 files changed, 22 insertions, 6 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 98066668335..45ef22e7e86 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -35,6 +35,7 @@ v 8.2.0 (unreleased)
   - New design for project graphs page
   - Remove deprecated dumped yaml file generated from previous job definitions
   - Fix incoming email config defaults
+  - Show specific runners from projects where user is master or owner
   - MR target branch is now visible on a list view when it is different from project's default one
   - Improve Continuous Integration graphs page
   - Make color of "Accept Merge Request" button consistent with current build status
diff --git a/app/models/user.rb b/app/models/user.rb
index 9ffadcf4468..61abea1f6ea 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -405,6 +405,15 @@ class User < ActiveRecord::Base
     end
   end
 
+  def master_or_owner_projects_id
+    @master_or_owner_projects_id ||= begin
+      scope = { access_level: [ Gitlab::Access::MASTER, Gitlab::Access::OWNER ] }
+      project_ids = personal_projects.pluck(:id)
+      project_ids.push(*groups_projects.where(members: scope).pluck(:id))
+      project_ids.push(*projects.where(members: scope).pluck(:id).uniq)
+    end
+  end
+
   # Projects user has access to
   def authorized_projects
     @authorized_projects ||= Project.where(id: authorized_projects_id)
@@ -765,14 +774,10 @@ class User < ActiveRecord::Base
     !solo_owned_groups.present?
   end
 
-  def ci_authorized_projects
-    @ci_authorized_projects ||= Ci::Project.where(gitlab_id: authorized_projects_id)
-  end
-
   def ci_authorized_runners
     @ci_authorized_runners ||= begin
       runner_ids = Ci::RunnerProject.joins(:project).
-        where(ci_projects: { gitlab_id: authorized_projects_id }).select(:runner_id)
+        where(ci_projects: { gitlab_id: master_or_owner_projects_id }).select(:runner_id)
       Ci::Runner.specific.where(id: runner_ids)
     end
   end
diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb
index 06adb7633b2..b0259026630 100644
--- a/spec/features/runners_spec.rb
+++ b/spec/features/runners_spec.rb
@@ -14,15 +14,25 @@ describe "Runners" do
       @project2 = FactoryGirl.create :ci_project
       @project2.gl_project.team << [user, :master]
 
+      @project3 = FactoryGirl.create :ci_project
+      @project3.gl_project.team << [user, :developer]
+
       @shared_runner = FactoryGirl.create :ci_shared_runner
       @specific_runner = FactoryGirl.create :ci_specific_runner
       @specific_runner2 = FactoryGirl.create :ci_specific_runner
+      @specific_runner3 = FactoryGirl.create :ci_specific_runner
       @project.runners << @specific_runner
       @project2.runners << @specific_runner2
+      @project3.runners << @specific_runner3
 
       visit runners_path(@project.gl_project)
     end
 
+    before do
+      expect(page).to_not have_content(@specific_runner3.display_name)
+      expect(page).to_not have_content(@specific_runner3.display_name)
+    end
+
     it "places runners in right places" do
       expect(page.find(".available-specific-runners")).to have_content(@specific_runner2.display_name)
       expect(page.find(".activated-specific-runners")).to have_content(@specific_runner.display_name)
@@ -76,10 +86,10 @@ describe "Runners" do
       @project.gl_project.team << [user, :master]
       @specific_runner = FactoryGirl.create :ci_specific_runner
       @project.runners << @specific_runner
-      visit runners_path(@project.gl_project)
     end
 
     it "shows runner information" do
+      visit runners_path(@project.gl_project)
       click_on @specific_runner.short_sha
       expect(page).to have_content(@specific_runner.platform)
     end