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

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

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

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