summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--Gemfile3
-rw-r--r--Gemfile.lock3
-rw-r--r--app/controllers/groups/variables_controller.rb2
-rw-r--r--changelogs/unreleased/security-bvl-update-thrift-gem.yml5
-rw-r--r--changelogs/unreleased/security-ci-api-variables-permissions.yml5
-rw-r--r--lib/api/group_variables.rb3
-rw-r--r--spec/controllers/groups/variables_controller_spec.rb39
-rw-r--r--spec/requests/api/group_variables_spec.rb51
8 files changed, 69 insertions, 42 deletions
diff --git a/Gemfile b/Gemfile
index c7ed1cd4d71..23b58d721f3 100644
--- a/Gemfile
+++ b/Gemfile
@@ -307,6 +307,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
diff --git a/Gemfile.lock b/Gemfile.lock
index b370024b95b..26a757a7683 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1176,7 +1176,7 @@ GEM
rack (>= 1, < 3)
thor (0.20.3)
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)
@@ -1522,6 +1522,7 @@ DEPENDENCIES
terser (= 1.0.2)
test-prof (~> 0.12.0)
thin (~> 1.7.0)
+ thrift (>= 0.14.0)
timecop (~> 0.9.1)
toml-rb (~> 1.0.0)
truncato (~> 0.7.11)
diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb
index 51670325ce3..a2289b540ec 100644
--- a/app/controllers/groups/variables_controller.rb
+++ b/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
diff --git a/changelogs/unreleased/security-bvl-update-thrift-gem.yml b/changelogs/unreleased/security-bvl-update-thrift-gem.yml
new file mode 100644
index 00000000000..afe1a0332e3
--- /dev/null
+++ b/changelogs/unreleased/security-bvl-update-thrift-gem.yml
@@ -0,0 +1,5 @@
+---
+title: Bump thrift gem to 0.14.0
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-ci-api-variables-permissions.yml b/changelogs/unreleased/security-ci-api-variables-permissions.yml
new file mode 100644
index 00000000000..05642a0ff57
--- /dev/null
+++ b/changelogs/unreleased/security-ci-api-variables-permissions.yml
@@ -0,0 +1,5 @@
+---
+title: Allow only owners to manage group variables
+merge_request:
+author:
+type: security
diff --git a/lib/api/group_variables.rb b/lib/api/group_variables.rb
index 0c40db02eb5..09744fbeda2 100644
--- a/lib/api/group_variables.rb
+++ b/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
diff --git a/spec/controllers/groups/variables_controller_spec.rb b/spec/controllers/groups/variables_controller_spec.rb
index e2a14165cb4..a450a4afb02 100644
--- a/spec/controllers/groups/variables_controller_spec.rb
+++ b/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: {
diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb
index 41b013f49ee..0b6bf65ca44 100644
--- a/spec/requests/api/group_variables_spec.rb
+++ b/spec/requests/api/group_variables_spec.rb
@@ -3,16 +3,19 @@
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) {}
+
+ before do
+ group.add_user(user, access_level) if access_level
+ end
+ describe 'GET /groups/:id/variables' do
context 'authorized user with proper permissions' do
- before do
- group.add_maintainer(user)
- end
+ 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)
View Lorry Controller Status