From cee083333ece2b963708d5b10370c7b31ef24ff0 Mon Sep 17 00:00:00 2001 From: Pirate Praveen Date: Thu, 2 Jun 2022 21:05:25 +0530 Subject: [PATCH] New upstream version 14.9.5+ds1 --- .gitlab/ci/frontend.gitlab-ci.yml | 4 +- .gitlab/ci/rails.gitlab-ci.yml | 4 +- .gitlab/ci/reports.gitlab-ci.yml | 6 --- .gitlab/ci/review-apps/main.gitlab-ci.yml | 2 +- .gitlab/ci/rules.gitlab-ci.yml | 12 ----- CHANGELOG.md | 12 +++++ GITALY_SERVER_VERSION | 2 +- VERSION | 2 +- .../groups/application_controller.rb | 6 +++ .../groups/group_members_controller.rb | 1 + app/policies/ci/build_policy.rb | 2 +- app/policies/group_policy.rb | 9 ++++ app/policies/project_policy.rb | 4 ++ app/services/ci/pipeline_trigger_service.rb | 1 + .../members/import_project_team_service.rb | 2 +- doc/api/scim.md | 14 +++--- doc/user/group/subgroups/index.md | 3 ++ lib/api/helpers/members_helpers.rb | 4 ++ lib/api/members.rb | 8 ++++ .../projects/jobs_controller_spec.rb | 8 ++-- .../security/group/private_access_spec.rb | 2 +- spec/policies/ci/build_policy_spec.rb | 48 +++++++++++++++++++ spec/policies/project_policy_spec.rb | 30 ++++++++++++ spec/requests/api/members_spec.rb | 15 ++++++ .../ci/pipeline_trigger_service_spec.rb | 9 ++++ 25 files changed, 173 insertions(+), 37 deletions(-) diff --git a/.gitlab/ci/frontend.gitlab-ci.yml b/.gitlab/ci/frontend.gitlab-ci.yml index cb07384676..a0b693a454 100644 --- a/.gitlab/ci/frontend.gitlab-ci.yml +++ b/.gitlab/ci/frontend.gitlab-ci.yml @@ -280,7 +280,9 @@ coverage-frontend: paths: - coverage-frontend/ reports: - cobertura: coverage-frontend/cobertura-coverage.xml + coverage_report: + coverage_format: cobertura + path: coverage-frontend/cobertura-coverage.xml .qa-frontend-node: extends: diff --git a/.gitlab/ci/rails.gitlab-ci.yml b/.gitlab/ci/rails.gitlab-ci.yml index 1c745e6d98..c911c33795 100644 --- a/.gitlab/ci/rails.gitlab-ci.yml +++ b/.gitlab/ci/rails.gitlab-ci.yml @@ -602,7 +602,9 @@ rspec:coverage: - coverage/assets/ - coverage/lcov/ reports: - cobertura: coverage/coverage.xml + coverage_report: + coverage_format: cobertura + path: coverage/coverage.xml rspec:undercoverage: extends: diff --git a/.gitlab/ci/reports.gitlab-ci.yml b/.gitlab/ci/reports.gitlab-ci.yml index d040abfe90..3628013fc9 100644 --- a/.gitlab/ci/reports.gitlab-ci.yml +++ b/.gitlab/ci/reports.gitlab-ci.yml @@ -87,12 +87,6 @@ gemnasium-dependency_scanning: - apk add git-lfs rules: !reference [".reports:rules:gemnasium-dependency_scanning", rules] -bundler-audit-dependency_scanning: - rules: !reference [".reports:rules:bundler-audit-dependency_scanning", rules] - -retire-js-dependency_scanning: - rules: !reference [".reports:rules:retire-js-dependency_scanning", rules] - gemnasium-python-dependency_scanning: rules: !reference [".reports:rules:gemnasium-python-dependency_scanning", rules] diff --git a/.gitlab/ci/review-apps/main.gitlab-ci.yml b/.gitlab/ci/review-apps/main.gitlab-ci.yml index d083f876b0..b528a2c742 100644 --- a/.gitlab/ci/review-apps/main.gitlab-ci.yml +++ b/.gitlab/ci/review-apps/main.gitlab-ci.yml @@ -77,7 +77,7 @@ review-build-cng: variables: HOST_SUFFIX: "${CI_ENVIRONMENT_SLUG}" DOMAIN: "-${CI_ENVIRONMENT_SLUG}.${REVIEW_APPS_DOMAIN}" - GITLAB_HELM_CHART_REF: "v5.5.0" + GITLAB_HELM_CHART_REF: "41d7632d9eba84f5816d808b98ccf3f5623e9fb5" environment: name: review/${CI_COMMIT_REF_SLUG}${FREQUENCY} url: https://gitlab-${CI_ENVIRONMENT_SLUG}.${REVIEW_APPS_DOMAIN} diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index 70b532b97f..7c447ad3b3 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -1450,18 +1450,6 @@ when: never - changes: *dependency-patterns -.reports:rules:bundler-audit-dependency_scanning: - rules: - - if: '$DEPENDENCY_SCANNING_DISABLED || $GITLAB_FEATURES !~ /\bdependency_scanning\b/ || $DS_EXCLUDED_ANALYZERS =~ /bundler-audit/ || $DS_DEFAULT_ANALYZERS !~ /bundler-audit/' - when: never - - changes: *bundler-patterns - -.reports:rules:retire-js-dependency_scanning: - rules: - - if: '$DEPENDENCY_SCANNING_DISABLED || $GITLAB_FEATURES !~ /\bdependency_scanning\b/ || $DS_EXCLUDED_ANALYZERS =~ /retire.js/ || $DS_DEFAULT_ANALYZERS !~ /retire.js/' - when: never - - changes: *nodejs-patterns - .reports:rules:gemnasium-python-dependency_scanning: rules: - if: '$DEPENDENCY_SCANNING_DISABLED || $GITLAB_FEATURES !~ /\bdependency_scanning\b/ || $DS_EXCLUDED_ANALYZERS =~ /gemnasium-python/ || $DS_DEFAULT_ANALYZERS !~ /gemnasium-python/' diff --git a/CHANGELOG.md b/CHANGELOG.md index bbaa5af86f..b5775eb31c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,18 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 14.9.5 (2022-06-01) + +### Security (7 changes) + +- [Fix IP restrictions not applying to deploy tokens](gitlab-org/security/gitlab@b429997e253110ea5eb4d20a8b90e9879be97300) ([merge request](gitlab-org/security/gitlab!2472)) +- [Trigger token should respect group IP restrictions](gitlab-org/security/gitlab@326993794bba8659208cde212433a5a19fd3e5a9) ([merge request](gitlab-org/security/gitlab!2477)) +- [Fix content injection in Jira issue title](gitlab-org/security/gitlab@c968f7be35d829bfefbf089794343d2d20cdd078) ([merge request](gitlab-org/security/gitlab!2465)) +- [Subgroup member can list members of parent group](gitlab-org/security/gitlab@1606689819eeae574bd5d65c8c971c2d4eb19e41) ([merge request](gitlab-org/security/gitlab!2481)) +- [Do not allow project member import when membership is locked](gitlab-org/security/gitlab@e8324068c61c4c58d50646d4fa8dff77c4d147ae) ([merge request](gitlab-org/security/gitlab!2448)) +- [Disable changing user attributes when updating SCIM provisioned user](gitlab-org/security/gitlab@9d5aca002edb2e0ab3c7d5b6eb00ea781d3dde9f) ([merge request](gitlab-org/security/gitlab!2455)) +- [Allow only job owner to run interactive terminal](gitlab-org/security/gitlab@c9fe31e3a963c421db3d9c47c65dd98a2867a699) ([merge request](gitlab-org/security/gitlab!2434)) + ## 14.9.4 (2022-04-29) ### Security (15 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 4a99c04565..2cde6be732 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -14.9.4 \ No newline at end of file +14.9.5 \ No newline at end of file diff --git a/VERSION b/VERSION index 4a99c04565..2cde6be732 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -14.9.4 \ No newline at end of file +14.9.5 \ No newline at end of file diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index bf72ade32d..aec3247f4b 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -67,6 +67,12 @@ class Groups::ApplicationController < ApplicationController end end + def authorize_read_group_member! + unless can?(current_user, :read_group_member, group) + render_403 + end + end + def build_canonical_path(group) params[:group_id] = group.to_param diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index ece1083d4d..7731c541c9 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -14,6 +14,7 @@ class Groups::GroupMembersController < Groups::ApplicationController # Authorize before_action :authorize_admin_group_member!, except: admin_not_required_endpoints + before_action :authorize_read_group_member!, only: :index skip_before_action :check_two_factor_requirement, only: :leave skip_cross_project_access_check :index, :update, :destroy, :request_access, diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 6162a31c11..f377ff85b5 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -84,7 +84,7 @@ module Ci enable :update_commit_status end - rule { can?(:update_build) & terminal }.enable :create_build_terminal + rule { can?(:update_build) & terminal & owner_of_job }.enable :create_build_terminal rule { can?(:update_build) }.enable :play_job diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 7a49ad3d4a..bf70899ad7 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -22,6 +22,7 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy condition(:share_with_group_locked, scope: :subject) { @subject.share_with_group_lock? } condition(:parent_share_with_group_locked, scope: :subject) { @subject.parent&.share_with_group_lock? } condition(:can_change_parent_share_with_group_lock) { can?(:change_share_with_group_lock, @subject.parent) } + condition(:can_read_group_member) { can_read_group_member? } desc "User is a project bot" condition(:project_bot) { user.project_bot? && access_level >= GroupMember::GUEST } @@ -127,6 +128,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy rule { ~public_group & ~has_access }.prevent :read_counts + rule { ~can_read_group_member }.policy do + prevent :read_group_member + end + rule { ~can?(:read_group) }.policy do prevent :read_design_activity end @@ -308,6 +313,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy true end + def can_read_group_member? + !(@subject.private? && access_level == GroupMember::NO_ACCESS) + end + def resource_access_token_creation_allowed? resource_access_token_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 2ffafb7913..f9e2375ecc 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -744,6 +744,10 @@ class ProjectPolicy < BasePolicy prevent :register_project_runners end + rule { can?(:admin_project_member) }.policy do + enable :import_project_members_from_another_project + end + private def user_is_user? diff --git a/app/services/ci/pipeline_trigger_service.rb b/app/services/ci/pipeline_trigger_service.rb index 7746382b84..ecd84884d9 100644 --- a/app/services/ci/pipeline_trigger_service.rb +++ b/app/services/ci/pipeline_trigger_service.rb @@ -26,6 +26,7 @@ module Ci def create_pipeline_from_trigger(trigger) # this check is to not leak the presence of the project if user cannot read it return unless trigger.project == project + return unless can?(trigger.owner, :read_project, project) response = Ci::CreatePipelineService .new(project, trigger.owner, ref: params[:ref], variables_attributes: variables) diff --git a/app/services/members/import_project_team_service.rb b/app/services/members/import_project_team_service.rb index 5f4d5414cf..6efd65e257 100644 --- a/app/services/members/import_project_team_service.rb +++ b/app/services/members/import_project_team_service.rb @@ -29,7 +29,7 @@ module Members def import_project_team return false unless target_project.present? && source_project.present? && current_user.present? return false unless can?(current_user, :read_project_member, source_project) - return false unless can?(current_user, :admin_project_member, target_project) + return false unless can?(current_user, :import_project_members_from_another_project, target_project) target_project.team.import(source_project, current_user) end diff --git a/doc/api/scim.md b/doc/api/scim.md index ab3a181f5b..9c88997b94 100644 --- a/doc/api/scim.md +++ b/doc/api/scim.md @@ -170,13 +170,13 @@ Returns a `201` status code if successful. Fields that can be updated are: -| SCIM/IdP field | GitLab field | -|:---------------------------------|:---------------------------------------| -| `id/externalId` | `extern_uid` | -| `name.formatted` | `name` | -| `emails\[type eq "work"\].value` | `email` | -| `active` | Identity removal if `active` = `false` | -| `userName` | `username` | +| SCIM/IdP field | GitLab field | +|:---------------------------------|:-----------------------------------------------------------------------------| +| `id/externalId` | `extern_uid` | +| `name.formatted` | `name` ([Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/363058)) | +| `emails\[type eq "work"\].value` | `email` ([Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/363058)) | +| `active` | Identity removal if `active` = `false` | +| `userName` | `username` ([Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/363058)) | ```plaintext PATCH /api/scim/v2/groups/:group_path/Users/:id diff --git a/doc/user/group/subgroups/index.md b/doc/user/group/subgroups/index.md index a98f213bb3..03559c5459 100644 --- a/doc/user/group/subgroups/index.md +++ b/doc/user/group/subgroups/index.md @@ -86,6 +86,9 @@ For more information, view the [permissions table](../../permissions.md#group-me ## Subgroup membership +NOTE: +There is a bug that causes some pages in the parent group to be accessible by subgroup members. For more details, see [this issue](https://gitlab.com/gitlab-org/gitlab/-/issues/340421). + When you add a member to a group, that member is also added to all subgroups. The user's permissions are inherited from the group's parent. diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb index f26ac1318b..e0ccf2242c 100644 --- a/lib/api/helpers/members_helpers.rb +++ b/lib/api/helpers/members_helpers.rb @@ -15,6 +15,10 @@ module API public_send("find_#{source_type}!", id) # rubocop:disable GitlabSecurity/PublicSend end + def authorize_read_source_member!(source_type, source) + authorize! :"read_#{source_type}_member", source + end + def authorize_admin_source!(source_type, source) authorize! :"admin_#{source_type}", source end diff --git a/lib/api/members.rb b/lib/api/members.rb index 4798edc4dd..382c0275c2 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -29,6 +29,8 @@ module API get ":id/members" do source = find_source(source_type, params[:id]) + authorize_read_source_member!(source_type, source) + members = paginate(retrieve_members(source, params: params)) present_members members @@ -48,6 +50,8 @@ module API get ":id/members/all" do source = find_source(source_type, params[:id]) + authorize_read_source_member!(source_type, source) + members = paginate(retrieve_members(source, params: params, deep: true)) present_members members @@ -63,6 +67,8 @@ module API get ":id/members/:user_id" do source = find_source(source_type, params[:id]) + authorize_read_source_member!(source_type, source) + members = source_members(source) member = members.find_by!(user_id: params[:user_id]) @@ -80,6 +86,8 @@ module API get ":id/members/all/:user_id" do source = find_source(source_type, params[:id]) + authorize_read_source_member!(source_type, source) + members = find_all_members(source) member = members.find_by!(user_id: params[:user_id]) diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index ed68d6a87b..f095d88746 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -183,7 +183,7 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do end context 'with web terminal' do - let(:job) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline) } + let(:job) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline, user: user) } it 'exposes the terminal path' do expect(response).to have_gitlab_http_status(:ok) @@ -1284,7 +1284,7 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do context 'when job exists' do context 'and it has a terminal' do - let!(:job) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline) } + let!(:job) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline, user: user) } it 'has a job' do get_terminal(id: job.id) @@ -1295,7 +1295,7 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do end context 'and does not have a terminal' do - let!(:job) { create(:ci_build, :running, pipeline: pipeline) } + let!(:job) { create(:ci_build, :running, pipeline: pipeline, user: user) } it 'returns not_found' do get_terminal(id: job.id) @@ -1324,7 +1324,7 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do end describe 'GET #terminal_websocket_authorize' do - let!(:job) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline) } + let!(:job) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline, user: user) } before do project.add_developer(user) diff --git a/spec/features/security/group/private_access_spec.rb b/spec/features/security/group/private_access_spec.rb index fc1fb3e384..f733145b5e 100644 --- a/spec/features/security/group/private_access_spec.rb +++ b/spec/features/security/group/private_access_spec.rb @@ -97,7 +97,7 @@ RSpec.describe 'Private Group access' do it { is_expected.to be_allowed_for(:developer).of(group) } it { is_expected.to be_allowed_for(:reporter).of(group) } it { is_expected.to be_allowed_for(:guest).of(group) } - it { is_expected.to be_allowed_for(project_guest) } + it { is_expected.to be_denied_for(project_guest) } it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index 1ec749fb39..fee4d76ca8 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -405,4 +405,52 @@ RSpec.describe Ci::BuildPolicy do end end end + + describe 'ability :create_build_terminal' do + let(:project) { create(:project, :private) } + + subject { described_class.new(user, build) } + + context 'when user can update_build' do + before do + project.add_maintainer(user) + end + + context 'when job has terminal' do + before do + allow(build).to receive(:has_terminal?).and_return(true) + end + + context 'when current user is the job owner' do + before do + build.update!(user: user) + end + + it { expect_allowed(:create_build_terminal) } + end + + context 'when current user is not the job owner' do + it { expect_disallowed(:create_build_terminal) } + end + end + + context 'when job does not have terminal' do + before do + allow(build).to receive(:has_terminal?).and_return(false) + build.update!(user: user) + end + + it { expect_disallowed(:create_build_terminal) } + end + end + + context 'when user cannot update build' do + before do + project.add_guest(user) + allow(build).to receive(:has_terminal?).and_return(true) + end + + it { expect_disallowed(:create_build_terminal) } + end + end end diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index fb1c587433..3b0c53fa74 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -346,6 +346,36 @@ RSpec.describe ProjectPolicy do end end + context 'importing members from another project' do + %w(maintainer owner).each do |role| + context "with #{role}" do + let(:current_user) { send(role) } + + it { is_expected.to be_allowed(:import_project_members_from_another_project) } + end + end + + %w(guest reporter developer anonymous).each do |role| + context "with #{role}" do + let(:current_user) { send(role) } + + it { is_expected.to be_disallowed(:import_project_members_from_another_project) } + end + end + + context 'with an admin' do + let(:current_user) { admin } + + context 'when admin mode is enabled', :enable_admin_mode do + it { expect_allowed(:import_project_members_from_another_project) } + end + + context 'when admin mode is disabled' do + it { expect_disallowed(:import_project_members_from_another_project) } + end + end + end + it_behaves_like 'clusterable policies' do let_it_be(:clusterable) { create(:project, :repository) } let_it_be(:cluster) do diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 561d81f986..eb6fa9e1c3 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -180,6 +180,21 @@ RSpec.describe API::Members do expect(json_response).to be_an Array expect(json_response.map { |u| u['id'] }).to match_array [maintainer.id, developer.id, nested_user.id] end + + context 'with a subgroup' do + let(:group) { create(:group, :private)} + let(:subgroup) { create(:group, :private, parent: group)} + let(:project) { create(:project, group: subgroup) } + + before do + subgroup.add_developer(developer) + end + + it 'subgroup member cannot get parent group members list' do + get api("/groups/#{group.id}/members/all", developer) + expect(response).to have_gitlab_http_status(:forbidden) + end + end end shared_examples 'GET /:source_type/:id/members/(all/):user_id' do |source_type, all| diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb index 29d12b0dd0..9187ac9519 100644 --- a/spec/services/ci/pipeline_trigger_service_spec.rb +++ b/spec/services/ci/pipeline_trigger_service_spec.rb @@ -56,6 +56,15 @@ RSpec.describe Ci::PipelineTriggerService do end end + context 'when trigger owner does not have a permission to read a project' do + let(:params) { { token: trigger.token, ref: 'master', variables: nil } } + let(:trigger) { create(:ci_trigger, project: project, owner: create(:user)) } + + it 'does nothing' do + expect { result }.not_to change { Ci::Pipeline.count } + end + end + context 'when params have an existing trigger token' do context 'when params have an existing ref' do let(:params) { { token: trigger.token, ref: 'master', variables: nil } }