summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/finders/issuable_finder.rb2
-rw-r--r--app/finders/notes_finder.rb2
-rw-r--r--app/models/project.rb4
-rw-r--r--app/services/merge_requests/build_service.rb2
-rw-r--r--app/views/layouts/nav/_project.html.haml2
-rw-r--r--changelogs/unreleased/jej-use-issuable-finder-instead-of-access-check.yml4
-rw-r--r--lib/api/helpers.rb4
-rw-r--r--lib/api/issues.rb2
-rw-r--r--lib/gitlab/search_results.rb2
-rw-r--r--spec/lib/gitlab/project_search_results_spec.rb9
-rw-r--r--spec/lib/gitlab/search_results_spec.rb51
-rw-r--r--spec/models/project_spec.rb16
-rw-r--r--spec/requests/api/issues_spec.rb18
-rw-r--r--spec/services/merge_requests/build_service_spec.rb12
14 files changed, 97 insertions, 33 deletions
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 34ae66e76d6..e42d5afdd89 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -23,7 +23,7 @@ class IssuableFinder
attr_accessor :current_user, :params
- def initialize(current_user, params)
+ def initialize(current_user, params = {})
@current_user = current_user
@params = params
end
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index 0b7832e6583..a653a6d59c6 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -12,7 +12,7 @@ class NotesFinder
when "commit"
project.notes.for_commit_id(target_id).non_diff_notes
when "issue"
- project.issues.visible_to_user(current_user).find(target_id).notes.inc_author
+ IssuesFinder.new(current_user, project_id: project.id).find(target_id).notes.inc_author
when "merge_request"
project.merge_requests.find(target_id).mr_and_commit_notes.inc_author
when "snippet", "project_snippet"
diff --git a/app/models/project.rb b/app/models/project.rb
index e0062f3dbb4..3f291d3944a 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -679,9 +679,9 @@ class Project < ActiveRecord::Base
self.id
end
- def get_issue(issue_id)
+ def get_issue(issue_id, current_user)
if default_issues_tracker?
- issues.find_by(iid: issue_id)
+ IssuesFinder.new(current_user, project_id: id).find_by(iid: issue_id)
else
ExternalIssue.new(issue_id, self)
end
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index 404f75616b5..f24346428eb 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -65,7 +65,7 @@ module MergeRequests
commit = commits.first
merge_request.title = commit.title
merge_request.description ||= commit.description.try(:strip)
- elsif iid && (issue = merge_request.target_project.get_issue(iid)) && !issue.try(:confidential?)
+ elsif iid && issue = merge_request.target_project.get_issue(iid, current_user)
case issue
when Issue
merge_request.title = "Resolve \"#{issue.title}\""
diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index 99a58bbb676..701bcd3ab71 100644
--- a/app/views/layouts/nav/_project.html.haml
+++ b/app/views/layouts/nav/_project.html.haml
@@ -70,7 +70,7 @@
%span
Issues
- if @project.default_issues_tracker?
- %span.badge.count.issue_counter= number_with_delimiter(@project.issues.visible_to_user(current_user).opened.count)
+ %span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id).execute.opened.count)
- if project_nav_tab? :merge_requests
= nav_link(controller: :merge_requests) do
diff --git a/changelogs/unreleased/jej-use-issuable-finder-instead-of-access-check.yml b/changelogs/unreleased/jej-use-issuable-finder-instead-of-access-check.yml
new file mode 100644
index 00000000000..c0b6f50052c
--- /dev/null
+++ b/changelogs/unreleased/jej-use-issuable-finder-instead-of-access-check.yml
@@ -0,0 +1,4 @@
+---
+title: Replace issue access checks with use of IssuableFinder
+merge_request:
+author:
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 2a88b55aef3..4aa1bfa06eb 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -122,9 +122,7 @@ module API
end
def find_project_issue(id)
- issue = user_project.issues.find(id)
- not_found! unless can?(current_user, :read_issue, issue)
- issue
+ IssuesFinder.new(current_user, project_id: user_project.id).find(id)
end
def paginate(relation)
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 5edcacfeae8..26a508f5e10 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -122,7 +122,7 @@ module API
# GET /projects/:id/issues?milestone=1.0.0&state=closed
# GET /issues?iid=42
get ":id/issues" do
- issues = user_project.issues.inc_notes_with_associations.visible_to_user(current_user)
+ issues = IssuesFinder.new(current_user, project_id: user_project.id).execute.inc_notes_with_associations
issues = filter_issues_state(issues, params[:state]) unless params[:state].nil?
issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil?
issues = filter_by_iid(issues, params[:iid]) unless params[:iid].nil?
diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb
index 2690938fe82..47d8599e298 100644
--- a/lib/gitlab/search_results.rb
+++ b/lib/gitlab/search_results.rb
@@ -50,7 +50,7 @@ module Gitlab
end
def issues
- issues = Issue.visible_to_user(current_user).where(project_id: project_ids_relation)
+ issues = IssuesFinder.new(current_user).execute.where(project_id: project_ids_relation)
if query =~ /#(\d+)\z/
issues = issues.where(iid: $1)
diff --git a/spec/lib/gitlab/project_search_results_spec.rb b/spec/lib/gitlab/project_search_results_spec.rb
index 29abb4d4d07..14182dbc06b 100644
--- a/spec/lib/gitlab/project_search_results_spec.rb
+++ b/spec/lib/gitlab/project_search_results_spec.rb
@@ -22,6 +22,14 @@ describe Gitlab::ProjectSearchResults, lib: true do
it { expect(results.query).to eq('hello world') }
end
+ it 'does not list issues on private projects' do
+ issue = create(:issue, project: project)
+
+ results = described_class.new(user, project, issue.title)
+
+ expect(results.objects('issues')).not_to include issue
+ end
+
describe 'confidential issues' do
let(:query) { 'issue' }
let(:author) { create(:user) }
@@ -29,6 +37,7 @@ describe Gitlab::ProjectSearchResults, lib: true do
let(:non_member) { create(:user) }
let(:member) { create(:user) }
let(:admin) { create(:admin) }
+ let(:project) { create(:empty_project, :internal) }
let!(:issue) { create(:issue, project: project, title: 'Issue 1') }
let!(:security_issue_1) { create(:issue, :confidential, project: project, title: 'Security issue 1', author: author) }
let!(:security_issue_2) { create(:issue, :confidential, title: 'Security issue 2', project: project, assignee: assignee) }
diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb
index dfbefad6367..f23e3522625 100644
--- a/spec/lib/gitlab/search_results_spec.rb
+++ b/spec/lib/gitlab/search_results_spec.rb
@@ -12,35 +12,48 @@ describe Gitlab::SearchResults do
let!(:milestone) { create(:milestone, project: project, title: 'foo') }
let(:results) { described_class.new(user, Project.all, 'foo') }
- describe '#projects_count' do
- it 'returns the total amount of projects' do
- expect(results.projects_count).to eq(1)
+ context 'as a user with access' do
+ before do
+ project.team << [user, :developer]
end
- end
- describe '#issues_count' do
- it 'returns the total amount of issues' do
- expect(results.issues_count).to eq(1)
+ describe '#projects_count' do
+ it 'returns the total amount of projects' do
+ expect(results.projects_count).to eq(1)
+ end
end
- end
- describe '#merge_requests_count' do
- it 'returns the total amount of merge requests' do
- expect(results.merge_requests_count).to eq(1)
+ describe '#issues_count' do
+ it 'returns the total amount of issues' do
+ expect(results.issues_count).to eq(1)
+ end
+ end
+
+ describe '#merge_requests_count' do
+ it 'returns the total amount of merge requests' do
+ expect(results.merge_requests_count).to eq(1)
+ end
end
- end
- describe '#milestones_count' do
- it 'returns the total amount of milestones' do
- expect(results.milestones_count).to eq(1)
+ describe '#milestones_count' do
+ it 'returns the total amount of milestones' do
+ expect(results.milestones_count).to eq(1)
+ end
end
end
+ it 'does not list issues on private projects' do
+ private_project = create(:empty_project, :private)
+ issue = create(:issue, project: private_project, title: 'foo')
+
+ expect(results.objects('issues')).not_to include issue
+ end
+
describe 'confidential issues' do
- let(:project_1) { create(:empty_project) }
- let(:project_2) { create(:empty_project) }
- let(:project_3) { create(:empty_project) }
- let(:project_4) { create(:empty_project) }
+ let(:project_1) { create(:empty_project, :internal) }
+ let(:project_2) { create(:empty_project, :internal) }
+ let(:project_3) { create(:empty_project, :internal) }
+ let(:project_4) { create(:empty_project, :internal) }
let(:query) { 'issue' }
let(:limit_projects) { Project.where(id: [project_1.id, project_2.id, project_3.id]) }
let(:author) { create(:user) }
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 0245897938c..c519ad779eb 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -353,10 +353,15 @@ describe Project, models: true do
describe '#get_issue' do
let(:project) { create(:empty_project) }
let!(:issue) { create(:issue, project: project) }
+ let(:user) { create(:user) }
+
+ before do
+ project.team << [user, :developer]
+ end
context 'with default issues tracker' do
it 'returns an issue' do
- expect(project.get_issue(issue.iid)).to eq issue
+ expect(project.get_issue(issue.iid, user)).to eq issue
end
it 'returns count of open issues' do
@@ -364,7 +369,12 @@ describe Project, models: true do
end
it 'returns nil when no issue found' do
- expect(project.get_issue(999)).to be_nil
+ expect(project.get_issue(999, user)).to be_nil
+ end
+
+ it "returns nil when user doesn't have access" do
+ user = create(:user)
+ expect(project.get_issue(issue.iid, user)).to eq nil
end
end
@@ -374,7 +384,7 @@ describe Project, models: true do
end
it 'returns an ExternalIssue' do
- issue = project.get_issue('FOO-1234')
+ issue = project.get_issue('FOO-1234', user)
expect(issue).to be_kind_of(ExternalIssue)
expect(issue.iid).to eq 'FOO-1234'
expect(issue.project).to eq project
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 8f695438ea2..b9146f7840c 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -365,6 +365,24 @@ describe API::API, api: true do
let(:base_url) { "/projects/#{project.id}" }
let(:title) { milestone.title }
+ it "returns 404 on private projects for other users" do
+ private_project = create(:empty_project, :private)
+ create(:issue, project: private_project)
+
+ get api("/projects/#{private_project.id}/issues", non_member)
+
+ expect(response).to have_http_status(404)
+ end
+
+ it 'returns no issues when user has access to project but not issues' do
+ restricted_project = create(:empty_project, :public, issues_access_level: ProjectFeature::PRIVATE)
+ create(:issue, project: restricted_project)
+
+ get api("/projects/#{restricted_project.id}/issues", non_member)
+
+ expect(json_response).to eq([])
+ end
+
it 'returns project issues without confidential issues for non project members' do
get api("#{base_url}/issues", non_member)
expect(response).to have_http_status(200)
diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb
index 3a3f07ddcb9..2d3a945337d 100644
--- a/spec/services/merge_requests/build_service_spec.rb
+++ b/spec/services/merge_requests/build_service_spec.rb
@@ -24,6 +24,8 @@ describe MergeRequests::BuildService, services: true do
end
before do
+ project.team << [user, :guest]
+
allow(CompareService).to receive_message_chain(:new, :execute).and_return(compare)
end
@@ -166,6 +168,16 @@ describe MergeRequests::BuildService, services: true do
expect(merge_request.title).to eq("Resolve \"#{issue.title}\"")
end
+ context 'when issue is not accessible to user' do
+ before do
+ project.team.truncate
+ end
+
+ it 'uses branch title as the merge request title' do
+ expect(merge_request.title).to eq("#{issue.iid} fix issue")
+ end
+ end
+
context 'issue does not exist' do
let(:source_branch) { "#{issue.iid.succ}-fix-issue" }
View Lorry Controller Status