From b9b8440df6afd24ba540343c612e522f52bea0db Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 6 Jan 2023 22:30:08 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-7-stable-ee --- app/controllers/uploads_controller.rb | 2 ++ app/policies/group_policy.rb | 9 +++++- app/policies/project_policy.rb | 10 +++++-- .../error_tracking/list_projects_service.rb | 16 +++++++++-- spec/controllers/uploads_controller_spec.rb | 32 +++++++++++++++++----- .../error_tracking/list_projects_service_spec.rb | 30 ++++++++++++++++---- .../resource_access_token_shared_examples.rb | 23 ---------------- 7 files changed, 80 insertions(+), 42 deletions(-) diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 09419a4589d..66f715f32af 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -52,6 +52,8 @@ class UploadsController < ApplicationController # access to itself when a secret is given. # For instance, user avatars are readable by anyone, # while temporary, user snippet uploads are not. + return false if !current_user && public_visibility_restricted? + !secret? || can?(current_user, :update_user, model) when Appearance true diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 858c145de3f..8eea995529c 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -273,6 +273,9 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy rule { can?(:admin_group) & resource_access_token_feature_available }.policy do enable :read_resource_access_tokens enable :destroy_resource_access_tokens + end + + rule { can?(:admin_group) & resource_access_token_creation_allowed }.policy do enable :admin_setting_to_allow_project_access_token_creation end @@ -338,12 +341,16 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy true end + def resource_access_token_create_feature_available? + 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? + resource_access_token_create_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? end def valid_dependency_proxy_deploy_token diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 2a13fafa313..fd3dbb54d57 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -157,7 +157,9 @@ class ProjectPolicy < BasePolicy condition(:service_desk_enabled) { @subject.service_desk_enabled? } with_scope :subject - condition(:resource_access_token_feature_available) { resource_access_token_feature_available? } + condition(:resource_access_token_feature_available) do + resource_access_token_feature_available? + end condition(:resource_access_token_creation_allowed) { resource_access_token_creation_allowed? } # We aren't checking `:read_issue` or `:read_merge_request` in this case @@ -922,12 +924,16 @@ class ProjectPolicy < BasePolicy true end + def resource_access_token_create_feature_available? + true + end + def resource_access_token_creation_allowed? group = project.group return true unless group # always enable for projects in personal namespaces - resource_access_token_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? + resource_access_token_create_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? end def project diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb index 2f23d47029c..d52306ef805 100644 --- a/app/services/error_tracking/list_projects_service.rb +++ b/app/services/error_tracking/list_projects_service.rb @@ -2,6 +2,8 @@ module ErrorTracking class ListProjectsService < ErrorTracking::BaseService + MASKED_TOKEN_REGEX = /\A\*+\z/.freeze + private def perform @@ -20,23 +22,31 @@ module ErrorTracking def project_error_tracking_setting (super || project.build_error_tracking_setting).tap do |setting| + url_changed = !setting.api_url&.start_with?(params[:api_host]) + setting.api_url = ErrorTracking::ProjectErrorTrackingSetting.build_api_url_from( api_host: params[:api_host], organization_slug: 'org', project_slug: 'proj' ) - setting.token = token(setting) + setting.token = token(setting, url_changed) setting.enabled = true end end strong_memoize_attr :project_error_tracking_setting - def token(setting) + def token(setting, url_changed) + return if url_changed && masked_token? + # Use param token if not masked, otherwise use database token - return params[:token] unless /\A\*+\z/.match?(params[:token]) + return params[:token] unless masked_token? setting.token end + + def masked_token? + MASKED_TOKEN_REGEX.match?(params[:token]) + end end end diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index e128db8d1c1..3e9c56d3274 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -268,17 +268,35 @@ RSpec.describe UploadsController do end context "when not signed in" do - it "responds with status 200" do - get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" } + context "when restricted visibility level is not set to public" do + before do + stub_application_setting(restricted_visibility_levels: []) + end - expect(response).to have_gitlab_http_status(:ok) + it "responds with status 200" do + get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" } + + expect(response).to have_gitlab_http_status(:ok) + end + + it_behaves_like 'content publicly cached' do + subject do + get :show, params: { model: 'user', mounted_as: 'avatar', id: user.id, filename: 'dk.png' } + + response + end + end end - it_behaves_like 'content publicly cached' do - subject do - get :show, params: { model: 'user', mounted_as: 'avatar', id: user.id, filename: 'dk.png' } + context "when restricted visibility level is set to public" do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end - response + it "responds with status 401" do + get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" } + + expect(response).to have_gitlab_http_status(:unauthorized) end end end diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb index ce391bd1ca0..8408adcc21d 100644 --- a/spec/services/error_tracking/list_projects_service_spec.rb +++ b/spec/services/error_tracking/list_projects_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ErrorTracking::ListProjectsService do +RSpec.describe ErrorTracking::ListProjectsService, feature_category: :integrations do let_it_be(:user) { create(:user) } let_it_be(:project, reload: true) { create(:project) } @@ -51,15 +51,33 @@ RSpec.describe ErrorTracking::ListProjectsService do end context 'masked param token' do - let(:params) { ActionController::Parameters.new(token: "*********", api_host: new_api_host) } + let(:params) { ActionController::Parameters.new(token: "*********", api_host: api_host) } - before do - expect(error_tracking_setting).to receive(:list_sentry_projects) + context 'with the current api host' do + let(:api_host) { 'https://sentrytest.gitlab.com' } + + before do + expect(error_tracking_setting).to receive(:list_sentry_projects) .and_return({ projects: [] }) + end + + it 'uses database token' do + expect { subject.execute }.not_to change { error_tracking_setting.token } + end end - it 'uses database token' do - expect { subject.execute }.not_to change { error_tracking_setting.token } + context 'with a new api host' do + let(:api_host) { new_api_host } + + it 'returns an error' do + expect(result[:message]).to start_with('Token is a required field') + expect(error_tracking_setting).not_to be_valid + expect(error_tracking_setting).not_to receive(:list_sentry_projects) + end + + it 'resets the token' do + expect { subject.execute }.to change { error_tracking_setting.token }.from(token).to(nil) + end end end diff --git a/spec/support/shared_examples/policies/resource_access_token_shared_examples.rb b/spec/support/shared_examples/policies/resource_access_token_shared_examples.rb index 337ad024fc0..cc91b73449a 100644 --- a/spec/support/shared_examples/policies/resource_access_token_shared_examples.rb +++ b/spec/support/shared_examples/policies/resource_access_token_shared_examples.rb @@ -71,26 +71,3 @@ RSpec.shared_examples 'Self-managed Core resource access tokens' do end end end - -RSpec.shared_examples 'GitLab.com Core resource access tokens' do - before do - allow(::Gitlab).to receive(:com?).and_return(true) - stub_ee_application_setting(should_check_namespace_plan: true) - end - - context 'with owner access' do - let(:current_user) { owner } - - context 'create resource access tokens' do - it { is_expected.not_to be_allowed(:create_resource_access_tokens) } - end - - context 'read resource access tokens' do - it { is_expected.not_to be_allowed(:read_resource_access_tokens) } - end - - context 'destroy resource access tokens' do - it { is_expected.not_to be_allowed(:destroy_resource_access_tokens) } - end - end -end -- cgit v1.2.1