Commit 5fe4df90 authored by Douwe Maan's avatar Douwe Maan Committed by Rémy Coutable

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

## Which fixes are in this MR?

:warning: - Potentially untested  
:bomb: - No test coverage  
:traffic_light: - Test coverage of some sort exists (a test failed when error raised)  
:vertical_traffic_light: - Test coverage of return value (a test failed when nil used)  
:white_check_mark: - Permissions check tested

### Issue lookup without access check (security)

- [x] :white_check_mark: 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] :traffic_light: app/models/cycle_analytics/summary.rb:9 [`.count`] 
- [x] :white_check_mark: app/controllers/projects/todos_controller.rb:19

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

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

See merge request !2030
parent 859498b7
......@@ -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
......
......@@ -5,7 +5,7 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController
before_action :authorize_read_cycle_analytics!
def show
@cycle_analytics = CycleAnalytics.new(@project, from: parse_start_date)
@cycle_analytics = CycleAnalytics.new(@project, current_user, from: parse_start_date)
respond_to do |format|
format.html
......
......@@ -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)
......
......@@ -4,13 +4,14 @@ class CycleAnalytics
DEPLOYMENT_METRIC_STAGES = %i[production staging]
def initialize(project, from:)
def initialize(project, current_user, from:)
@project = project
@current_user = current_user
@from = from
end
def summary
@summary ||= Summary.new(@project, from: @from)
@summary ||= Summary.new(@project, @current_user, from: @from)
end
def issue
......
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:
......@@ -88,6 +88,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