Commit 6d37fe95 authored by Douwe Maan's avatar Douwe Maan Committed by Alejandro Rodríguez
Browse files

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/...
parent 742cee75
......@@ -36,7 +36,7 @@ def create
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 @@ def issuable
@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
......
......@@ -41,6 +41,14 @@ def execute
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 @@
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 @@
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 @@ def go
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 @@ def go
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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