Merge branch 'security-unauthorized-access-schedule-variables-milestone-13-6' into 'master'

Unauthorized user can access pipeline schedule variables

See merge request gitlab-org/security/gitlab!1028

app/policies/ci/pipeline_schedule_policy.rb

@@ -17,6 +17,7 @@ module Ci
     rule { can?(:admin_pipeline) | (can?(:update_build) & owner_of_schedule) }.policy do
       enable :update_pipeline_schedule
       enable :admin_pipeline_schedule
+      enable :read_pipeline_schedule_variables
     end
 
     rule { can?(:admin_pipeline_schedule) & ~owner_of_schedule }.policy do

changelogs/unreleased/security-unauthorized-access-schedule-variables-milestone-13-6.yml

+---
+title: Fix unauthorized user is able to access schedule pipeline variables and values
+merge_request:
+author:
+type: security

lib/api/ci/pipeline_schedules.rb

@@ -38,7 +38,7 @@ module API
           requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id'
         end
         get ':id/pipeline_schedules/:pipeline_schedule_id' do
-          present pipeline_schedule, with: Entities::Ci::PipelineScheduleDetails
+          present pipeline_schedule, with: Entities::Ci::PipelineScheduleDetails, user: current_user
         end
 
         desc 'Create a new pipeline schedule' do

lib/api/entities/ci/pipeline_schedule_details.rb

@@ -5,7 +5,9 @@ module API
     module Ci
       class PipelineScheduleDetails < PipelineSchedule
         expose :last_pipeline, using: ::API::Entities::Ci::PipelineBasic
-        expose :variables, using: ::API::Entities::Ci::Variable
+        expose :variables,
+          using: ::API::Entities::Ci::Variable,
+          if: ->(schedule, options) { Ability.allowed?(options[:user], :read_pipeline_schedule_variables, schedule) }
       end
     end
   end

spec/policies/project_policy_spec.rb

@@ -137,7 +137,7 @@ RSpec.describe ProjectPolicy do
       it 'disallows all permissions except pipeline when the feature is disabled' do
         builds_permissions = [
           :create_build, :read_build, :update_build, :admin_build, :destroy_build,
-          :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
+          :create_pipeline_schedule, :read_pipeline_schedule_variables, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
           :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment,
           :create_cluster, :read_cluster, :update_cluster, :admin_cluster, :destroy_cluster,
           :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment

spec/requests/api/ci/pipeline_schedules_spec.rb

@@ -97,46 +97,112 @@ RSpec.describe API::Ci::PipelineSchedules do
       pipeline_schedule.pipelines << build(:ci_pipeline, project: project)
     end
 
-    context 'authenticated user with valid permissions' do
-      it 'returns pipeline_schedule details' do
-        get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", developer)
-
+    matcher :return_pipeline_schedule_sucessfully do
+      match_unless_raises do |reponse|
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to match_response_schema('pipeline_schedule')
       end
+    end
 
-      it 'responds with 404 Not Found if requesting non-existing pipeline_schedule' do
-        get api("/projects/#{project.id}/pipeline_schedules/-5", developer)
+    shared_context 'request with project permissions' do
+      context 'authenticated user with project permisions' do
+        before do
+          project.add_maintainer(user)
+        end
 
-        expect(response).to have_gitlab_http_status(:not_found)
+        it 'returns pipeline_schedule details' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to return_pipeline_schedule_sucessfully
+          expect(json_response).to have_key('variables')
+        end
       end
     end
 
-    context 'authenticated user with invalid permissions' do
-      it 'does not return pipeline_schedules list' do
-        get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+    shared_examples 'request with schedule ownership' do
+      context 'authenticated user with pipeline schedule ownership' do
+        it 'returns pipeline_schedule details' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", developer)
 
-        expect(response).to have_gitlab_http_status(:not_found)
+          expect(response).to return_pipeline_schedule_sucessfully
+          expect(json_response).to have_key('variables')
+        end
       end
     end
 
-    context 'authenticated user with insufficient permissions' do
-      before do
-        project.add_guest(user)
+    shared_examples 'request with unauthenticated user' do
+      context 'with unauthenticated user' do
+        it 'does not return pipeline_schedule' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}")
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
       end
+    end
 
-      it 'does not return pipeline_schedules list' do
-        get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+    shared_examples 'request with non-existing pipeline_schedule' do
+      it 'responds with 404 Not Found if requesting non-existing pipeline_schedule' do
+        get api("/projects/#{project.id}/pipeline_schedules/-5", developer)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
-    context 'unauthenticated user' do
-      it 'does not return pipeline_schedules list' do
-        get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}")
+    context 'with private project' do
+      it_behaves_like 'request with schedule ownership'
+      it_behaves_like 'request with project permissions'
+      it_behaves_like 'request with unauthenticated user'
+      it_behaves_like 'request with non-existing pipeline_schedule'
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+      context 'authenticated user with no project permissions' do
+        it 'does not return pipeline_schedule' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+
+      context 'authenticated user with insufficient project permissions' do
+        before do
+          project.add_guest(user)
+        end
+
+        it 'does not return pipeline_schedule' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
+    context 'with public project' do
+      let_it_be(:project) { create(:project, :repository, :public, public_builds: false) }
+
+      it_behaves_like 'request with schedule ownership'
+      it_behaves_like 'request with project permissions'
+      it_behaves_like 'request with unauthenticated user'
+      it_behaves_like 'request with non-existing pipeline_schedule'
+
+      context 'authenticated user with no project permissions' do
+        it 'returns pipeline_schedule with no variables' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to return_pipeline_schedule_sucessfully
+          expect(json_response).not_to have_key('variables')
+        end
+      end
+
+      context 'authenticated user with insufficient project permissions' do
+        before do
+          project.add_guest(user)
+        end
+
+        it 'returns pipeline_schedule with no variables' do
+          get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to return_pipeline_schedule_sucessfully
+          expect(json_response).not_to have_key('variables')
+        end
       end
     end
   end