Add latest changes from gitlab-org/security/gitlab@13-9-stable-ee

Gemfile

@@ -310,6 +310,9 @@ gem 'premailer-rails', '~> 1.10.3'
 
 # LabKit: Tracing and Correlation
 gem 'gitlab-labkit', '0.14.0'
+# Thrift is a dependency of gitlab-labkit, we want a version higher than 0.14.0
+# because of https://gitlab.com/gitlab-org/gitlab/-/issues/321900
+gem 'thrift', '>= 0.14.0'
 
 # I18n
 gem 'ruby_parser', '~> 3.15', require: false

Gemfile.lock

@@ -1186,7 +1186,7 @@ GEM
       rack (>= 1, < 3)
     thor (1.1.0)
     thread_safe (0.3.6)
-    thrift (0.13.0)
+    thrift (0.14.0)
     tilt (2.0.10)
     timecop (0.9.1)
     timeliness (0.3.10)
@@ -1535,6 +1535,7 @@ DEPENDENCIES
   terser (= 1.0.2)
   test-prof (~> 0.12.0)
   thin (~> 1.8.0)
+  thrift (>= 0.14.0)
   timecop (~> 0.9.1)
   toml-rb (~> 1.0.0)
   truncato (~> 0.7.11)

app/controllers/groups/variables_controller.rb

@@ -2,7 +2,7 @@
 
 module Groups
   class VariablesController < Groups::ApplicationController
-    before_action :authorize_admin_build!
+    before_action :authorize_admin_group!
 
     skip_cross_project_access_check :show, :update
 

changelogs/unreleased/security-bvl-update-thrift-gem.yml

+---
+title: Bump thrift gem to 0.14.0
+merge_request:
+author:
+type: security

changelogs/unreleased/security-ci-api-variables-permissions.yml

+---
+title: Allow only owners to manage group variables
+merge_request:
+author:
+type: security

lib/api/group_variables.rb

@@ -5,8 +5,7 @@ module API
     include PaginationParams
 
     before { authenticate! }
-    before { authorize! :admin_build, user_group }
-
+    before { authorize! :admin_group, user_group }
     feature_category :continuous_integration
 
     params do

spec/controllers/groups/variables_controller_spec.rb

@@ -5,26 +5,35 @@ require 'spec_helper'
 RSpec.describe Groups::VariablesController do
   include ExternalAuthorizationServiceHelpers
 
-  let(:group) { create(:group) }
-  let(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:variable) { create(:ci_group_variable, group: group) }
+  let(:access_level) { :owner }
 
   before do
     sign_in(user)
-    group.add_maintainer(user)
+    group.add_user(user, access_level)
   end
 
   describe 'GET #show' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     subject do
       get :show, params: { group_id: group }, format: :json
     end
 
     include_examples 'GET #show lists all variables'
+
+    context 'when the user is a maintainer' do
+      let(:access_level) { :maintainer }
+
+      it 'returns not found response' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
   end
 
   describe 'PATCH #update' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
     let(:owner) { group }
 
     subject do
@@ -37,6 +46,19 @@ RSpec.describe Groups::VariablesController do
     end
 
     include_examples 'PATCH #update updates variables'
+
+    context 'when the user is a maintainer' do
+      let(:access_level) { :maintainer }
+      let(:variables_attributes) do
+        [{ id: variable.id, key: 'new_key' }]
+      end
+
+      it 'returns not found response' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
   end
 
   context 'with external authorization enabled' do
@@ -45,8 +67,6 @@ RSpec.describe Groups::VariablesController do
     end
 
     describe 'GET #show' do
-      let!(:variable) { create(:ci_group_variable, group: group) }
-
       it 'is successful' do
         get :show, params: { group_id: group }, format: :json
 
@@ -55,9 +75,6 @@ RSpec.describe Groups::VariablesController do
     end
 
     describe 'PATCH #update' do
-      let!(:variable) { create(:ci_group_variable, group: group) }
-      let(:owner) { group }
-
       it 'is successful' do
         patch :update,
               params: {

spec/requests/api/group_variables_spec.rb

@@ -3,17 +3,20 @@
 require 'spec_helper'
 
 RSpec.describe API::GroupVariables do
-  let(:group) { create(:group) }
-  let(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:variable) { create(:ci_group_variable, group: group) }
 
-  describe 'GET /groups/:id/variables' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
+  let(:access_level) {}
 
-    context 'authorized user with proper permissions' do
   before do
-        group.add_maintainer(user)
+    group.add_user(user, access_level) if access_level
   end
 
+  describe 'GET /groups/:id/variables' do
+    context 'authorized user with proper permissions' do
+      let(:access_level) { :owner }
+
       it 'returns group variables' do
         get api("/groups/#{group.id}/variables", user)
 
@@ -23,6 +26,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not return group variables' do
         get api("/groups/#{group.id}/variables", user)
 
@@ -40,12 +45,8 @@ RSpec.describe API::GroupVariables do
   end
 
   describe 'GET /groups/:id/variables/:key' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'returns group variable details' do
         get api("/groups/#{group.id}/variables/#{variable.key}", user)
@@ -64,6 +65,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not return group variable details' do
         get api("/groups/#{group.id}/variables/#{variable.key}", user)
 
@@ -82,11 +85,7 @@ RSpec.describe API::GroupVariables do
 
   describe 'POST /groups/:id/variables' do
     context 'authorized user with proper permissions' do
-      let!(:variable) { create(:ci_group_variable, group: group) }
-
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'creates variable' do
         expect do
@@ -124,6 +123,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not create variable' do
         post api("/groups/#{group.id}/variables", user)
 
@@ -141,12 +142,8 @@ RSpec.describe API::GroupVariables do
   end
 
   describe 'PUT /groups/:id/variables/:key' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'updates variable data' do
         initial_variable = group.variables.reload.first
@@ -180,6 +177,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not update variable' do
         put api("/groups/#{group.id}/variables/#{variable.key}", user)
 
@@ -197,12 +196,8 @@ RSpec.describe API::GroupVariables do
   end
 
   describe 'DELETE /groups/:id/variables/:key' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'deletes variable' do
         expect do
@@ -224,6 +219,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not delete variable' do
         delete api("/groups/#{group.id}/variables/#{variable.key}", user)