Commit 58f7134c authored by Douwe Maan's avatar Douwe Maan Committed by Alejandro Rodríguez

Merge branch 'jej-fix-missing-access-check-on-issues' into 'security'

Fix missing access checks on issue lookup using IssuableFinder

Split from !2024 to partially solve https://gitlab.com/gitlab-org/gitlab-ce/issues/23867

 - Potentially untested
💣 - No test coverage
🚥 - Test coverage of some sort exists (a test failed when error raised)
🚦 - Test coverage of return value (a test failed when nil used)
 - Permissions check tested

- [x]  app/controllers/projects/branches_controller.rb:39
  - `before_action :authorize_push_code!` helpes limit/prevent exploitation. Always checks for reporter access so fine with
    confidential issues, issues only visible to team, etc.
- [x] 🚥 app/models/cycle_analytics/summary.rb:9 [`.count`]
- [x]  app/controllers/projects/todos_controller.rb:19

- [x] Potential double render in app/controllers/projects/todos_controller.rb

- https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#cedccb227af9bfdf88802767cb58d43c2b977439_24_24

See merge request !2030
parent 1a05b196
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -43,6 +43,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)
......
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:)
......
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
......
---
title: Fix missing access checks on issue lookup using IssuableFinder
merge_request:
author:
......@@ -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
......
......@@ -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
......
......@@ -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(
......
......@@ -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,
......
......@@ -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,
......
......@@ -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,
......
......@@ -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,
......
......@@ -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,
......
......@@ -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
......
......@@ -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,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment