diff options
22 files changed, 323 insertions, 65 deletions
diff --git a/app/assets/javascripts/behaviors/markdown/render_math.js b/app/assets/javascripts/behaviors/markdown/render_math.js index b2348cf0bad..7525fc76d16 100644 --- a/app/assets/javascripts/behaviors/markdown/render_math.js +++ b/app/assets/javascripts/behaviors/markdown/render_math.js @@ -66,16 +66,12 @@ class SafeMathRenderer { el.removeAttribute('style'); if (!forceRender && (this.totalMS >= MAX_RENDER_TIME_MS || text.length > MAX_MATH_CHARS)) { // Show unrendered math code - const wrapperElement = document.createElement('div'); const codeElement = document.createElement('pre'); codeElement.className = 'code'; codeElement.textContent = el.textContent; codeElement.dataset.mathStyle = el.dataset.mathStyle; - const { parentNode } = el; - parentNode.replaceChild(wrapperElement, el); - let message; if (text.length > MAX_MATH_CHARS) { message = sprintf( @@ -103,11 +99,11 @@ class SafeMathRenderer { </div> `; - if (!wrapperElement.classList.contains('lazy-alert-shown')) { + if (!el.classList.contains('lazy-alert-shown')) { // eslint-disable-next-line no-unsanitized/property - wrapperElement.innerHTML = html; - wrapperElement.append(codeElement); - wrapperElement.classList.add('lazy-alert-shown'); + el.innerHTML = html; + el.append(codeElement); + el.classList.add('lazy-alert-shown'); } // Render the next math @@ -125,6 +121,12 @@ class SafeMathRenderer { } try { + if (displayContainer.dataset.mathStyle === 'inline') { + displayContainer.classList.add('math-content-inline'); + } else { + displayContainer.classList.add('math-content-display'); + } + // eslint-disable-next-line no-unsanitized/property displayContainer.innerHTML = this.katex.renderToString(text, { displayMode: el.dataset.mathStyle === 'display', @@ -169,8 +171,7 @@ class SafeMathRenderer { render() { // Replace math blocks with a placeholder so they aren't rendered twice this.elements.forEach((el) => { - const placeholder = document.createElement('span'); - placeholder.style.display = 'none'; + const placeholder = document.createElement('div'); placeholder.dataset.mathStyle = el.dataset.mathStyle; placeholder.textContent = el.textContent; el.parentNode.replaceChild(placeholder, el); diff --git a/app/assets/stylesheets/framework/markdown_area.scss b/app/assets/stylesheets/framework/markdown_area.scss index b87fd3e67d4..62782de5402 100644 --- a/app/assets/stylesheets/framework/markdown_area.scss +++ b/app/assets/stylesheets/framework/markdown_area.scss @@ -137,6 +137,16 @@ border-radius: $border-radius-default $border-radius-default 0 0; } +.math-content-inline { + overflow: auto; + display: inline-flex; +} + +.math-content-display { + overflow: auto; + display: block; +} + @include media-breakpoint-down(xs) { .referenced-users { margin-right: 0; diff --git a/app/controllers/projects/error_tracking/projects_controller.rb b/app/controllers/projects/error_tracking/projects_controller.rb index 531bd327e43..372fbfdc183 100644 --- a/app/controllers/projects/error_tracking/projects_controller.rb +++ b/app/controllers/projects/error_tracking/projects_controller.rb @@ -5,7 +5,7 @@ module Projects class ProjectsController < Projects::ApplicationController respond_to :json - before_action :authorize_read_sentry_issue! + before_action :authorize_admin_sentry! feature_category :error_tracking urgency :low diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index a90a16e120c..06eb3fcc233 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -133,6 +133,8 @@ module MergeRequestsHelper _('Not available for private projects') elsif ProtectedBranch.protected?(merge_request.source_project, merge_request.source_branch) _('Not available for protected branches') + elsif !merge_request.author.can?(:push_code, merge_request.source_project) + _('Merge request author cannot push to target project') end end diff --git a/app/models/project.rb b/app/models/project.rb index 68196f0a757..5989584ce43 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -3456,6 +3456,7 @@ class Project < ApplicationRecord # Issue for N+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/49322 Gitlab::GitalyClient.allow_n_plus_1_calls do merge_requests_allowing_collaboration(branch_name).any? do |merge_request| + merge_request.author.can?(:push_code, self) && merge_request.can_be_merged_by?(user, skip_collaboration_check: true) end end diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 34754f4fc95..38521ae6090 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -121,7 +121,7 @@ class ProjectTeam def import(source_project, current_user) target_project = project - source_members = source_project.project_members.to_a + source_members = source_members_for_import(source_project) target_user_ids = target_project.project_members.pluck_user_ids importer_access_level = max_member_access(current_user.id) @@ -242,6 +242,10 @@ class ProjectTeam def member_user_ids Member.on_project_and_ancestors(project).select(:user_id) end + + def source_members_for_import(source_project) + source_project.project_members.to_a + end end ProjectTeam.prepend_mod_with('ProjectTeam') diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 38e6360f81d..a57b6f8daf7 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -289,6 +289,7 @@ class ProjectPolicy < BasePolicy enable :change_visibility_level enable :remove_project enable :archive_project + enable :link_forked_project enable :remove_fork_project enable :destroy_merge_request enable :destroy_issue @@ -545,6 +546,7 @@ class ProjectPolicy < BasePolicy enable :destroy_release enable :destroy_artifacts enable :admin_operations + enable :admin_sentry enable :read_deploy_token enable :create_deploy_token enable :destroy_deploy_token diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb index 1539e24df9d..67d690d64e7 100644 --- a/app/services/error_tracking/list_projects_service.rb +++ b/app/services/error_tracking/list_projects_service.rb @@ -5,6 +5,8 @@ module ErrorTracking private def perform + return error('Access denied', :unauthorized) unless can?(current_user, :admin_sentry, project) + unless project_error_tracking_setting.valid? return error(project_error_tracking_setting.errors.full_messages.join(', '), :bad_request) end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index d14c20426df..05626a2b8b8 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -389,6 +389,8 @@ - 2 - - mailers - 2 +- - members_destroyer_clean_up_group_protected_branch_rules + - 1 - - members_expiring_email_notification - 1 - - merge diff --git a/doc/operations/error_tracking.md b/doc/operations/error_tracking.md index 82982a03016..1ebbb047c01 100644 --- a/doc/operations/error_tracking.md +++ b/doc/operations/error_tracking.md @@ -161,7 +161,7 @@ GitLab provides a way to connect Sentry to your project. Prerequisites: -- You must have at least the Developer role for the project. +- You must have at least the Maintainer role for the project. To enable the Sentry integration: diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 069d117db17..c0222539c98 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -219,7 +219,7 @@ module API if params[:start_project] start_project = find_project!(params[:start_project]) - unless user_project.forked_from?(start_project) + unless can?(current_user, :read_code, start_project) && user_project.forked_from?(start_project) forbidden!("Project is not included in the fork network for #{start_project.full_name}") end end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 6d13512aad6..ac28effea43 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -701,7 +701,7 @@ module API requires :forked_from_id, type: String, desc: 'The ID of the project it was forked from', documentation: { example: 'gitlab' } end post ":id/fork/:forked_from_id", feature_category: :source_code_management do - authorize! :admin_project, user_project + authorize! :link_forked_project, user_project fork_from_project = find_project!(params[:forked_from_id]) diff --git a/lib/banzai/filter/asset_proxy_filter.rb b/lib/banzai/filter/asset_proxy_filter.rb index 00ffdd3d809..512c55381ec 100644 --- a/lib/banzai/filter/asset_proxy_filter.rb +++ b/lib/banzai/filter/asset_proxy_filter.rb @@ -22,13 +22,13 @@ module Banzai begin uri = URI.parse(original_src) + + next if uri.host.nil? && !original_src.start_with?('///') + next if asset_host_allowed?(uri.host) rescue StandardError - next + # Ignored end - next if uri.host.nil? && !original_src.start_with?('///') - next if asset_host_allowed?(uri.host) - element['src'] = asset_proxy_url(original_src) element['data-canonical-src'] = original_src end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bf468a3b209..e5a755f12b5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -29344,6 +29344,9 @@ msgstr "" msgid "Merge request approvals" msgstr "" +msgid "Merge request author cannot push to target project" +msgstr "" + msgid "Merge request change summary" msgstr "" @@ -57085,6 +57088,9 @@ msgstr[1] "" msgid "security Reports|There was an error creating the merge request" msgstr "" +msgid "security policy bot users cannot be added to other projects" +msgstr "" + msgid "selective_code_owner_removals can only be enabled when retain_approvals_on_push is enabled" msgstr "" diff --git a/spec/controllers/projects/error_tracking/projects_controller_spec.rb b/spec/controllers/projects/error_tracking/projects_controller_spec.rb index 7529c701b2b..63346faccbf 100644 --- a/spec/controllers/projects/error_tracking/projects_controller_spec.rb +++ b/spec/controllers/projects/error_tracking/projects_controller_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Projects::ErrorTracking::ProjectsController do let(:user) { create(:user) } it 'returns 404' do - project.add_guest(user) + project.add_developer(user) get :index, params: list_projects_params diff --git a/spec/features/markdown/math_spec.rb b/spec/features/markdown/math_spec.rb index 25459494a0c..0bc8f2146e9 100644 --- a/spec/features/markdown/math_spec.rb +++ b/spec/features/markdown/math_spec.rb @@ -24,9 +24,7 @@ RSpec.describe 'Math rendering', :js, feature_category: :team_planning do ``` MATH - issue = create(:issue, project: project, description: description) - - visit project_issue_path(project, issue) + create_and_visit_issue_with_description(description) expect(page).to have_selector('.katex .mord.mathnormal', text: 'b') expect(page).to have_selector('.katex-display .mord.mathnormal', text: 'b') @@ -40,9 +38,7 @@ RSpec.describe 'Math rendering', :js, feature_category: :team_planning do This link is valid $`\\href{https://gitlab.com}{Gitlab}`$. MATH - issue = create(:issue, project: project, description: description) - - visit project_issue_path(project, issue) + create_and_visit_issue_with_description(description) page.within '.description > .md' do # unfortunately there is no class selector for KaTeX's "unsupported command" @@ -59,9 +55,7 @@ RSpec.describe 'Math rendering', :js, feature_category: :team_planning do ``` MATH - issue = create(:issue, project: project, description: description) - - visit project_issue_path(project, issue) + create_and_visit_issue_with_description(description) page.within '.description > .md' do expect(page).to have_selector('.js-lazy-render-math-container', text: /math block exceeds 1000 characters/) @@ -75,11 +69,7 @@ RSpec.describe 'Math rendering', :js, feature_category: :team_planning do ``` MATH - issue = create(:issue, project: project, description: description) - - visit project_issue_path(project, issue) - - wait_for_requests + create_and_visit_issue_with_description(description) page.within '.description > .md' do expect(page).not_to have_selector('.katex-error') @@ -93,11 +83,7 @@ RSpec.describe 'Math rendering', :js, feature_category: :team_planning do ``` MATH - issue = create(:issue, project: project, description: description) - - visit project_issue_path(project, issue) - - wait_for_requests + create_and_visit_issue_with_description(description) page.within '.description > .md' do click_button 'Display anyway' @@ -113,11 +99,7 @@ RSpec.describe 'Math rendering', :js, feature_category: :team_planning do ``` MATH - issue = create(:issue, project: project, description: description) - - visit project_issue_path(project, issue) - - wait_for_requests + create_and_visit_issue_with_description(description) page.within '.description > .md' do expect(page).to have_selector('.katex-error', @@ -132,11 +114,7 @@ RSpec.describe 'Math rendering', :js, feature_category: :team_planning do ``` MATH - issue = create(:issue, project: project, description: description) - - visit project_issue_path(project, issue) - - wait_for_requests + create_and_visit_issue_with_description(description) page.within '.description > .md' do expect(page).to have_selector('.katex-error', text: /&lt;script&gt;/) @@ -162,4 +140,72 @@ RSpec.describe 'Math rendering', :js, feature_category: :team_planning do expect(page).not_to have_selector('.js-lazy-render-math') end end + + it 'uses math-content-display for display math', :js do + description = <<~MATH + ```math + 1 + 2 + ``` + MATH + + create_and_visit_issue_with_description(description) + + page.within '.description > .md' do + expect(page).to have_selector('.math-content-display') + end + end + + it 'uses math-content-inline for inline math', :js do + description = 'one $`1 + 2`$ two' + + create_and_visit_issue_with_description(description) + + page.within '.description > .md' do + expect(page).to have_selector('.math-content-inline') + end + end + + context 'when math tries to cover other elements on the page' do + it 'prevents hijacking for display math', :js do + description = <<~MATH + [test link](#) + + ```math + \\hskip{-200pt}\\href{https://example.com}{\\smash{\\raisebox{20em}{$\\smash{\\raisebox{20em}{$\\phantom{\\underset{\\underset{\\underset{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}}{\\underset{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}}}{\\underset{\\underset{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}}{}}}$}}$}}} + ``` + MATH + + issue = create_and_visit_issue_with_description(description) + + page.within '.description > .md' do + click_link 'test link' + + expect(page).to have_current_path(project_issue_path(project, issue)) + end + end + + it 'prevents hijacking for inline math', :js do + description = <<~MATH + [test link](#) $`\\hskip{-200pt}\\href{https://example.com}{\\smash{\\raisebox{20em}{$\\smash{\\raisebox{20em}{$\\phantom{\\underset{\\underset{\\underset{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}}{\\underset{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}}}{\\underset{\\underset{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}{\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}\\rule{20em}{20em}}}{}}}$}}$}}}`$ + MATH + + issue = create_and_visit_issue_with_description(description) + + page.within '.description > .md' do + click_link 'test link' + + expect(page).to have_current_path(project_issue_path(project, issue)) + end + end + end + + def create_and_visit_issue_with_description(description) + issue = create(:issue, project: project, description: description) + + visit project_issue_path(project, issue) + + wait_for_requests + + issue + end end diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb index b6c8653a563..3614cbe5011 100644 --- a/spec/helpers/merge_requests_helper_spec.rb +++ b/spec/helpers/merge_requests_helper_spec.rb @@ -234,4 +234,43 @@ RSpec.describe MergeRequestsHelper, feature_category: :code_review_workflow do it { expect(tab_count_display(merge_request, '10')).to eq('10') } end end + + describe '#allow_collaboration_unavailable_reason' do + subject { allow_collaboration_unavailable_reason(merge_request) } + + let(:merge_request) do + create(:merge_request, author: author, source_project: project, source_branch: generate(:branch)) + end + + let_it_be(:public_project) { create(:project, :small_repo, :public) } + let(:project) { public_project } + let(:forked_project) { fork_project(project) } + let(:author) { project.creator } + + context 'when the merge request allows collaboration for the user' do + before do + allow(merge_request).to receive(:can_allow_collaboration?).with(current_user).and_return(true) + end + + it { is_expected.to be_nil } + end + + context 'when the project is private' do + let(:project) { create(:project, :empty_repo, :private) } + + it { is_expected.to eq(_('Not available for private projects')) } + end + + context 'when the source branch is protected' do + let!(:protected_branch) { create(:protected_branch, project: project, name: merge_request.source_branch) } + + it { is_expected.to eq(_('Not available for protected branches')) } + end + + context 'when the merge request author cannot push to the source project' do + let(:author) { create(:user) } + + it { is_expected.to eq(_('Merge request author cannot push to target project')) } + end + end end diff --git a/spec/lib/banzai/filter/asset_proxy_filter_spec.rb b/spec/lib/banzai/filter/asset_proxy_filter_spec.rb index dc6ac52a8c2..7a34bf13c8f 100644 --- a/spec/lib/banzai/filter/asset_proxy_filter_spec.rb +++ b/spec/lib/banzai/filter/asset_proxy_filter_spec.rb @@ -89,6 +89,33 @@ RSpec.describe Banzai::Filter::AssetProxyFilter, feature_category: :team_plannin expect(doc.at_css('img')['data-canonical-src']).to eq src end + it 'replaces by default, even strings that do not look like URLs' do + src = 'oigjsie8787%$**(#(%0' + new_src = 'https://assets.example.com/1b893f9a71d66c99437f27e19b9a061a6f5d9391/6f69676a7369653837383725242a2a2823282530' + doc = filter(image(src), @context) + + expect(doc.at_css('img')['src']).to eq new_src + expect(doc.at_css('img')['data-canonical-src']).to eq src + end + + it 'replaces URL with non-ASCII characters' do + src = 'https://example.com/x?¬' + new_src = 'https://assets.example.com/2f29a8c7f13f3ae14dc18c154dbbd657d703e75f/68747470733a2f2f6578616d706c652e636f6d2f783fc2ac' + doc = filter(image(src), @context) + + expect(doc.at_css('img')['src']).to eq new_src + expect(doc.at_css('img')['data-canonical-src']).to eq src + end + + it 'replaces out-of-spec URL that would still be rendered in the browser' do + src = 'https://example.com/##' + new_src = 'https://assets.example.com/d7d0c845cc553d9430804c07e9456545ef3e6fe6/68747470733a2f2f6578616d706c652e636f6d2f2323' + doc = filter(image(src), @context) + + expect(doc.at_css('img')['src']).to eq new_src + expect(doc.at_css('img')['data-canonical-src']).to eq src + end + it 'skips internal images' do src = "#{Gitlab.config.gitlab.url}/test.png" doc = filter(image(src), @context) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index aedfc7fca53..46bf80b1e8f 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -6438,6 +6438,8 @@ RSpec.describe Project, factory_default: :keep, feature_category: :groups_and_pr let!(:merge_request) do create( :merge_request, + author: author, + state_id: state_id, target_project: target_project, target_branch: 'target-branch', source_project: project, @@ -6446,6 +6448,9 @@ RSpec.describe Project, factory_default: :keep, feature_category: :groups_and_pr ) end + let(:author) { project.creator } + let(:state_id) { MergeRequest.available_states[:opened] } + before do target_project.add_developer(user) end @@ -6455,10 +6460,12 @@ RSpec.describe Project, factory_default: :keep, feature_category: :groups_and_pr expect(project.merge_requests_allowing_push_to_user(user)).to include(merge_request) end - it 'does not include closed merge requests' do - merge_request.close + context 'when the merge requests are closed' do + let(:state_id) { MergeRequest.available_states[:closed] } - expect(project.merge_requests_allowing_push_to_user(user)).to be_empty + it 'does not include closed merge requests' do + expect(project.merge_requests_allowing_push_to_user(user)).to be_empty + end end it 'does not include merge requests for guest users' do @@ -6480,16 +6487,38 @@ RSpec.describe Project, factory_default: :keep, feature_category: :groups_and_pr end describe '#any_branch_allows_collaboration?' do - it 'allows access when there are merge requests open allowing collaboration', :sidekiq_might_not_need_inline do - expect(project.any_branch_allows_collaboration?(user)) - .to be_truthy - end + context 'when there is an open merge request allowing collaboration' do + it 'allows access', :sidekiq_might_not_need_inline do + expect(project.any_branch_allows_collaboration?(user)) + .to be_truthy + end + + context 'when the merge request author is not allowed to push_code' do + let(:author) { create(:user) } - it 'does not allow access when there are no merge requests open allowing collaboration' do - merge_request.close! + it 'returns false' do + expect(project.any_branch_allows_collaboration?(user)) + .to be_falsey + end + end - expect(project.any_branch_allows_collaboration?(user)) - .to be_falsey + context 'when the merge request is closed' do + let(:state_id) { MergeRequest.available_states[:closed] } + + it 'returns false' do + expect(project.any_branch_allows_collaboration?(user)) + .to be_falsey + end + end + + context 'when the merge request is merged' do + let(:state_id) { MergeRequest.available_states[:merged] } + + it 'returns false' do + expect(project.any_branch_allows_collaboration?(user)) + .to be_falsey + end + end end end @@ -6537,6 +6566,15 @@ RSpec.describe Project, factory_default: :keep, feature_category: :groups_and_pr .not_to exceed_query_limit(control).with_threshold(2) end end + + context 'when the merge request author is not allowed to push_code' do + let(:author) { create(:user) } + + it 'returns false' do + expect(project.branch_allows_collaboration?(user, 'awesome-feature-1')) + .to be_falsey + end + end end end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 8b9ac7cd588..90595c2d7f9 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -774,6 +774,62 @@ RSpec.describe API::Commits, feature_category: :source_code_management do end end + context 'when project repository access becomes restricted after being forked' do + let!(:fork_owner) { create(:user) } + let!(:forked_project) { fork_project(public_project, fork_owner, namespace: fork_owner.namespace, repository: true) } + let(:url) { "/projects/#{forked_project.id}/repository/commits" } + + before do + # Restrict repository visibility of the public project + public_project.merge_requests_access_level = 'private' + public_project.builds_access_level = 'private' + public_project.repository_access_level = 'private' + public_project.save! + + valid_c_params[:start_branch] = 'master' + valid_c_params[:branch] = 'patch' + valid_c_params[:start_project] = public_project.id + end + + after do + # Reopen repository visibility of the public project + public_project.merge_requests_access_level = 'enabled' + public_project.repository_access_level = 'enabled' + public_project.builds_access_level = 'enabled' + public_project.save! + end + + it 'returns a 403' do + post api(url, fork_owner), params: valid_c_params + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context 'when fork owner has no more access to a private repository' do + let_it_be(:private_project) { create(:project, :private, :repository) } + let_it_be(:fork_owner) { create(:user) } + let_it_be(:fork_owner_membership) { private_project.add_developer(fork_owner) } + let_it_be(:forked_project) { fork_project(private_project, fork_owner, namespace: fork_owner.namespace, repository: true) } + let(:url) { "/projects/#{forked_project.id}/repository/commits" } + + before do + # Restrict user from repository + Members::DestroyService.new(private_project.owner).execute(fork_owner_membership) + Sidekiq::Worker.drain_all + + valid_c_params[:start_branch] = 'master' + valid_c_params[:branch] = 'patch' + valid_c_params[:start_project] = private_project.id + end + + it 'returns a 402' do + post api(url, fork_owner), params: valid_c_params + + expect(response).to have_gitlab_http_status(:not_found) + end + end + context 'when the target project is not part of the fork network of start_project' do let(:unrelated_project) { create(:project, :public, :repository, creator: guest) } let(:url) { "/projects/#{unrelated_project.id}/repository/commits" } diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index e3e8df79a1d..12898060e22 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -3292,6 +3292,10 @@ RSpec.describe API::Projects, :aggregate_failures, feature_category: :groups_and let(:failed_status_code) { :not_found } end + it 'refreshes the forks count cache' do + expect(project_fork_source.forks_count).to be_zero + end + context 'user is a developer' do before do project_fork_target.add_developer(user) @@ -3304,15 +3308,23 @@ RSpec.describe API::Projects, :aggregate_failures, feature_category: :groups_and end end - it 'refreshes the forks count cache' do - expect(project_fork_source.forks_count).to be_zero - end - context 'user is maintainer' do before do project_fork_target.add_maintainer(user) end + it 'denies project to be forked from an existing project' do + post api(path, user) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context 'user is owner' do + before do + project_fork_target.add_owner(user) + end + context 'and user is a reporter of target group' do let_it_be_with_reload(:target_group) { create(:group, project_creation_level: ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS) } let_it_be_with_reload(:project_fork_target) { create(:project, namespace: target_group) } diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb index d91808edc8d..53dbbfd8c71 100644 --- a/spec/services/error_tracking/list_projects_service_spec.rb +++ b/spec/services/error_tracking/list_projects_service_spec.rb @@ -19,7 +19,7 @@ RSpec.describe ErrorTracking::ListProjectsService, feature_category: :integratio subject { described_class.new(project, user, params) } before do - project.add_reporter(user) + project.add_maintainer(user) end describe '#execute' do @@ -137,6 +137,16 @@ RSpec.describe ErrorTracking::ListProjectsService, feature_category: :integratio end end + context 'with user with insufficient permissions' do + before do + project.add_developer(user) + end + + it 'returns error' do + expect(result).to include(status: :error, message: 'Access denied', http_status: :unauthorized) + end + end + context 'with error tracking disabled' do before do expect(project).to receive(:error_tracking_setting).at_least(:once) |