summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/projects/branches_controller.rb2
-rw-r--r--app/controllers/projects/cycle_analytics_controller.rb2
-rw-r--r--app/controllers/projects/todos_controller.rb8
-rw-r--r--app/finders/issuable_finder.rb8
-rw-r--r--app/models/cycle_analytics.rb5
-rw-r--r--app/models/cycle_analytics/summary.rb5
-rw-r--r--changelogs/unreleased/jej-fix-missing-access-check-on-issues.yml4
-rw-r--r--spec/controllers/projects/branches_controller_spec.rb18
-rw-r--r--spec/controllers/projects/todo_controller_spec.rb17
-rw-r--r--spec/models/cycle_analytics/code_spec.rb2
-rw-r--r--spec/models/cycle_analytics/issue_spec.rb2
-rw-r--r--spec/models/cycle_analytics/plan_spec.rb2
-rw-r--r--spec/models/cycle_analytics/production_spec.rb2
-rw-r--r--spec/models/cycle_analytics/review_spec.rb2
-rw-r--r--spec/models/cycle_analytics/staging_spec.rb2
-rw-r--r--spec/models/cycle_analytics/summary_spec.rb2
-rw-r--r--spec/models/cycle_analytics/test_spec.rb2
17 files changed, 62 insertions, 23 deletions
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,