diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index 6b9f37983c4..89d84809e3a 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -36,7 +36,7 @@ class Projects::BranchesController < Projects::ApplicationController execute(branch_name, ref) if params[:issue_iid] - issue = @project.issues.find_by(iid: params[:issue_iid]) + issue = IssuesFinder.new(current_user, project_id: @project.id).find_by(iid: params[:issue_iid]) SystemNoteService.new_issue_branch(issue, @project, current_user, branch_name) if issue end diff --git a/app/controllers/projects/cycle_analytics_controller.rb b/app/controllers/projects/cycle_analytics_controller.rb index fd263960b93..ac639ef015b 100644 --- a/app/controllers/projects/cycle_analytics_controller.rb +++ b/app/controllers/projects/cycle_analytics_controller.rb @@ -6,7 +6,7 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController before_action :authorize_read_cycle_analytics! def show - @cycle_analytics = ::CycleAnalytics.new(@project, from: start_date(cycle_analytics_params)) + @cycle_analytics = ::CycleAnalytics.new(@project, current_user, from: start_date(cycle_analytics_params)) stats_values, cycle_analytics_json = generate_cycle_analytics_data diff --git a/app/controllers/projects/todos_controller.rb b/app/controllers/projects/todos_controller.rb index 5685d0f4e7c..52517381c65 100644 --- a/app/controllers/projects/todos_controller.rb +++ b/app/controllers/projects/todos_controller.rb @@ -16,13 +16,7 @@ class Projects::TodosController < Projects::ApplicationController @issuable ||= begin case params[:issuable_type] when "issue" - issue = @project.issues.find(params[:issuable_id]) - - if can?(current_user, :read_issue, issue) - issue - else - render_404 - end + IssuesFinder.new(current_user, project_id: @project.id).find(params[:issuable_id]) when "merge_request" @project.merge_requests.find(params[:issuable_id]) end diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index a48f22cee07..36005035f68 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -41,6 +41,14 @@ class IssuableFinder sort(items) end + def find(*params) + execute.find(*params) + end + + def find_by(*params) + execute.find_by(*params) + end + def group return @group if defined?(@group) diff --git a/app/models/cycle_analytics.rb b/app/models/cycle_analytics.rb index cb8e088d21d..ba4ee6fcf9d 100644 --- a/app/models/cycle_analytics.rb +++ b/app/models/cycle_analytics.rb @@ -1,14 +1,15 @@ class CycleAnalytics STAGES = %i[issue plan code test review staging production].freeze - def initialize(project, from:) + def initialize(project, current_user, from:) @project = project + @current_user = current_user @from = from @fetcher = Gitlab::CycleAnalytics::MetricsFetcher.new(project: project, from: from, branch: nil) end def summary - @summary ||= Summary.new(@project, from: @from) + @summary ||= Summary.new(@project, @current_user, from: @from) end def permissions(user:) diff --git a/app/models/cycle_analytics/summary.rb b/app/models/cycle_analytics/summary.rb index b46db449bf3..82f53d17ddd 100644 --- a/app/models/cycle_analytics/summary.rb +++ b/app/models/cycle_analytics/summary.rb @@ -1,12 +1,13 @@ class CycleAnalytics class Summary - def initialize(project, from:) + def initialize(project, current_user, from:) @project = project + @current_user = current_user @from = from end def new_issues - @project.issues.created_after(@from).count + IssuesFinder.new(@current_user, project_id: @project.id).execute.created_after(@from).count end def commits diff --git a/changelogs/unreleased/jej-fix-missing-access-check-on-issues.yml b/changelogs/unreleased/jej-fix-missing-access-check-on-issues.yml new file mode 100644 index 00000000000..844fba9a107 --- /dev/null +++ b/changelogs/unreleased/jej-fix-missing-access-check-on-issues.yml @@ -0,0 +1,4 @@ +--- +title: Fix missing access checks on issue lookup using IssuableFinder +merge_request: +author: diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb index f7cf006efd6..b88586b8678 100644 --- a/spec/controllers/projects/branches_controller_spec.rb +++ b/spec/controllers/projects/branches_controller_spec.rb @@ -94,6 +94,24 @@ describe Projects::BranchesController do branch_name: branch, issue_iid: issue.iid end + + context 'without issue feature access' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) + project.team.truncate + end + + it "doesn't post a system note" do + expect(SystemNoteService).not_to receive(:new_issue_branch) + + post :create, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + branch_name: branch, + issue_iid: issue.iid + end + end end end diff --git a/spec/controllers/projects/todo_controller_spec.rb b/spec/controllers/projects/todo_controller_spec.rb index 936320a3709..193a3f6b5a3 100644 --- a/spec/controllers/projects/todo_controller_spec.rb +++ b/spec/controllers/projects/todo_controller_spec.rb @@ -4,7 +4,7 @@ describe Projects::TodosController do include ApiHelpers let(:user) { create(:user) } - let(:project) { create(:project) } + let(:project) { create(:empty_project) } let(:issue) { create(:issue, project: project) } let(:merge_request) { create(:merge_request, source_project: project) } @@ -42,7 +42,7 @@ describe Projects::TodosController do end end - context 'when not authorized' do + context 'when not authorized for project' do it 'does not create todo for issue that user has no access to' do sign_in(user) expect do @@ -60,6 +60,19 @@ describe Projects::TodosController do expect(response).to have_http_status(302) end end + + context 'when not authorized for issue' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) + sign_in(user) + end + + it "doesn't create todo" do + expect{ go }.not_to change { user.todos.count } + expect(response).to have_http_status(404) + end + end end end diff --git a/spec/models/cycle_analytics/code_spec.rb b/spec/models/cycle_analytics/code_spec.rb index 7691d690db0..7771785ead3 100644 --- a/spec/models/cycle_analytics/code_spec.rb +++ b/spec/models/cycle_analytics/code_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#code', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, user, from: from_date) } context 'with deployment' do generate_cycle_analytics_spec( diff --git a/spec/models/cycle_analytics/issue_spec.rb b/spec/models/cycle_analytics/issue_spec.rb index f649b44d367..5ed3d37f2fb 100644 --- a/spec/models/cycle_analytics/issue_spec.rb +++ b/spec/models/cycle_analytics/issue_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#issue', models: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, user, from: from_date) } generate_cycle_analytics_spec( phase: :issue, diff --git a/spec/models/cycle_analytics/plan_spec.rb b/spec/models/cycle_analytics/plan_spec.rb index 2cdefbeef21..baf3e3241a1 100644 --- a/spec/models/cycle_analytics/plan_spec.rb +++ b/spec/models/cycle_analytics/plan_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#plan', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, user, from: from_date) } generate_cycle_analytics_spec( phase: :plan, diff --git a/spec/models/cycle_analytics/production_spec.rb b/spec/models/cycle_analytics/production_spec.rb index 1f5e5cab92d..21b9c6e7150 100644 --- a/spec/models/cycle_analytics/production_spec.rb +++ b/spec/models/cycle_analytics/production_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#production', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, user, from: from_date) } generate_cycle_analytics_spec( phase: :production, diff --git a/spec/models/cycle_analytics/review_spec.rb b/spec/models/cycle_analytics/review_spec.rb index 0ed080a42b1..158621d59a4 100644 --- a/spec/models/cycle_analytics/review_spec.rb +++ b/spec/models/cycle_analytics/review_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#review', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, user, from: from_date) } generate_cycle_analytics_spec( phase: :review, diff --git a/spec/models/cycle_analytics/staging_spec.rb b/spec/models/cycle_analytics/staging_spec.rb index af1c4477ddb..dad653964b7 100644 --- a/spec/models/cycle_analytics/staging_spec.rb +++ b/spec/models/cycle_analytics/staging_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#staging', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, user, from: from_date) } generate_cycle_analytics_spec( phase: :staging, diff --git a/spec/models/cycle_analytics/summary_spec.rb b/spec/models/cycle_analytics/summary_spec.rb index 9d67bc82cba..725bc68b25f 100644 --- a/spec/models/cycle_analytics/summary_spec.rb +++ b/spec/models/cycle_analytics/summary_spec.rb @@ -4,7 +4,7 @@ describe CycleAnalytics::Summary, models: true do let(:project) { create(:project) } let(:from) { Time.now } let(:user) { create(:user, :admin) } - subject { described_class.new(project, from: from) } + subject { described_class.new(project, user, from: from) } describe "#new_issues" do it "finds the number of issues created after the 'from date'" do diff --git a/spec/models/cycle_analytics/test_spec.rb b/spec/models/cycle_analytics/test_spec.rb index 02ddfeed9c1..2313724e8f3 100644 --- a/spec/models/cycle_analytics/test_spec.rb +++ b/spec/models/cycle_analytics/test_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#test', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, user, from: from_date) } generate_cycle_analytics_spec( phase: :test,