From db9979bcad17e46ef2b151b018c5f7769d33ca77 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 27 Oct 2016 15:59:00 +0100 Subject: [PATCH 1/2] Fix project member access for group links `ProjectTeam#find_member` doesn't take group links into account. It was used in two places: 1. An admin view - it can stay here. 2. `ProjectTeam#member?`, which is often used to decide if a user has access to view something. This second part broke confidential issues viewing. `IssuesFinder` ends up delegating to `Project#authorized_for_user?`, which does consider group links, so users with access to the project via a group link could see confidential issues on the index page. However, `IssuesPolicy` used `ProjectTeam#member?`, so the same user couldn't view the issue when going to it directly. --- app/models/project_team.rb | 10 +- spec/policies/issues_policy_spec.rb | 163 ++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 8 deletions(-) create mode 100644 spec/policies/issues_policy_spec.rb diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 79d041d2775..a6e911df9bd 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -125,14 +125,8 @@ class ProjectTeam max_member_access(user.id) == Gitlab::Access::MASTER end - def member?(user, min_member_access = nil) - member = !!find_member(user.id) - - if min_member_access - member && max_member_access(user.id) >= min_member_access - else - member - end + def member?(user, min_member_access = Gitlab::Access::GUEST) + max_member_access(user.id) >= min_member_access end def human_max_access(user_id) diff --git a/spec/policies/issues_policy_spec.rb b/spec/policies/issues_policy_spec.rb new file mode 100644 index 00000000000..0dfd2772e42 --- /dev/null +++ b/spec/policies/issues_policy_spec.rb @@ -0,0 +1,163 @@ +require 'spec_helper' + +describe IssuePolicy, models: true do + let(:guest) { create(:user) } + let(:author) { create(:user) } + let(:assignee) { create(:user) } + let(:reporter) { create(:user) } + + def permissions(user, issue) + IssuePolicy.abilities(user, issue).to_set + end + + context 'a private project' do + let(:non_member) { create(:user) } + let(:project) { create(:empty_project, :private) } + let(:issue) { create(:issue, project: project, assignee: assignee, author: author) } + let(:issue_no_assignee) { create(:issue, project: project) } + + before do + project.team << [guest, :guest] + project.team << [author, :guest] + project.team << [assignee, :guest] + project.team << [reporter, :reporter] + end + + it 'does not allow non-members to read issues' do + expect(permissions(non_member, issue)).not_to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(non_member, issue_no_assignee)).not_to include(:read_issue, :update_issue, :admin_issue) + end + + it 'allows guests to read issues' do + expect(permissions(guest, issue)).to include(:read_issue) + expect(permissions(guest, issue)).not_to include(:update_issue, :admin_issue) + + expect(permissions(guest, issue_no_assignee)).to include(:read_issue) + expect(permissions(guest, issue_no_assignee)).not_to include(:update_issue, :admin_issue) + end + + it 'allows reporters to read, update, and admin issues' do + expect(permissions(reporter, issue)).to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(reporter, issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) + end + + it 'allows issue authors to read and update their issues' do + expect(permissions(author, issue)).to include(:read_issue, :update_issue) + expect(permissions(author, issue)).not_to include(:admin_issue) + + expect(permissions(author, issue_no_assignee)).to include(:read_issue) + expect(permissions(author, issue_no_assignee)).not_to include(:update_issue, :admin_issue) + end + + it 'allows issue assignees to read and update their issues' do + expect(permissions(assignee, issue)).to include(:read_issue, :update_issue) + expect(permissions(assignee, issue)).not_to include(:admin_issue) + + expect(permissions(assignee, issue_no_assignee)).to include(:read_issue) + expect(permissions(assignee, issue_no_assignee)).not_to include(:update_issue, :admin_issue) + end + + context 'with confidential issues' do + let(:confidential_issue) { create(:issue, :confidential, project: project, assignee: assignee, author: author) } + let(:confidential_issue_no_assignee) { create(:issue, :confidential, project: project) } + + it 'does not allow non-members to read confidential issues' do + expect(permissions(non_member, confidential_issue)).not_to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(non_member, confidential_issue_no_assignee)).not_to include(:read_issue, :update_issue, :admin_issue) + end + + it 'does not allow guests to read confidential issues' do + expect(permissions(guest, confidential_issue)).not_to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(guest, confidential_issue_no_assignee)).not_to include(:read_issue, :update_issue, :admin_issue) + end + + it 'allows reporters to read, update, and admin confidential issues' do + expect(permissions(reporter, confidential_issue)).to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(reporter, confidential_issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) + end + + it 'allows issue authors to read and update their confidential issues' do + expect(permissions(author, confidential_issue)).to include(:read_issue, :update_issue) + expect(permissions(author, confidential_issue)).not_to include(:admin_issue) + + expect(permissions(author, confidential_issue_no_assignee)).not_to include(:read_issue, :update_issue, :admin_issue) + end + + it 'allows issue assignees to read and update their confidential issues' do + expect(permissions(assignee, confidential_issue)).to include(:read_issue, :update_issue) + expect(permissions(assignee, confidential_issue)).not_to include(:admin_issue) + + expect(permissions(assignee, confidential_issue_no_assignee)).not_to include(:read_issue, :update_issue, :admin_issue) + end + end + end + + context 'a public project' do + let(:project) { create(:empty_project, :public) } + let(:issue) { create(:issue, project: project, assignee: assignee, author: author) } + let(:issue_no_assignee) { create(:issue, project: project) } + + before do + project.team << [guest, :guest] + project.team << [reporter, :reporter] + end + + it 'allows guests to read issues' do + expect(permissions(guest, issue)).to include(:read_issue) + expect(permissions(guest, issue)).not_to include(:update_issue, :admin_issue) + + expect(permissions(guest, issue_no_assignee)).to include(:read_issue) + expect(permissions(guest, issue_no_assignee)).not_to include(:update_issue, :admin_issue) + end + + it 'allows reporters to read, update, and admin issues' do + expect(permissions(reporter, issue)).to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(reporter, issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) + end + + it 'allows issue authors to read and update their issues' do + expect(permissions(author, issue)).to include(:read_issue, :update_issue) + expect(permissions(author, issue)).not_to include(:admin_issue) + + expect(permissions(author, issue_no_assignee)).to include(:read_issue) + expect(permissions(author, issue_no_assignee)).not_to include(:update_issue, :admin_issue) + end + + it 'allows issue assignees to read and update their issues' do + expect(permissions(assignee, issue)).to include(:read_issue, :update_issue) + expect(permissions(assignee, issue)).not_to include(:admin_issue) + + expect(permissions(assignee, issue_no_assignee)).to include(:read_issue) + expect(permissions(assignee, issue_no_assignee)).not_to include(:update_issue, :admin_issue) + end + + context 'with confidential issues' do + let(:confidential_issue) { create(:issue, :confidential, project: project, assignee: assignee, author: author) } + let(:confidential_issue_no_assignee) { create(:issue, :confidential, project: project) } + + it 'does not allow guests to read confidential issues' do + expect(permissions(guest, confidential_issue)).not_to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(guest, confidential_issue_no_assignee)).not_to include(:read_issue, :update_issue, :admin_issue) + end + + it 'allows reporters to read, update, and admin confidential issues' do + expect(permissions(reporter, confidential_issue)).to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(reporter, confidential_issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) + end + + it 'allows issue authors to read and update their confidential issues' do + expect(permissions(author, confidential_issue)).to include(:read_issue, :update_issue) + expect(permissions(author, confidential_issue)).not_to include(:admin_issue) + + expect(permissions(author, confidential_issue_no_assignee)).not_to include(:read_issue, :update_issue, :admin_issue) + end + + it 'allows issue assignees to read and update their confidential issues' do + expect(permissions(assignee, confidential_issue)).to include(:read_issue, :update_issue) + expect(permissions(assignee, confidential_issue)).not_to include(:admin_issue) + + expect(permissions(assignee, confidential_issue_no_assignee)).not_to include(:read_issue, :update_issue, :admin_issue) + end + end + end +end From af6cf695c43d0b5f189de05069a475fcea73e374 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 28 Oct 2016 15:11:32 +0100 Subject: [PATCH 2/2] Add specs for a user from a group link --- spec/policies/issues_policy_spec.rb | 30 +++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/spec/policies/issues_policy_spec.rb b/spec/policies/issues_policy_spec.rb index 0dfd2772e42..2b7b6cad654 100644 --- a/spec/policies/issues_policy_spec.rb +++ b/spec/policies/issues_policy_spec.rb @@ -5,6 +5,8 @@ describe IssuePolicy, models: true do let(:author) { create(:user) } let(:assignee) { create(:user) } let(:reporter) { create(:user) } + let(:group) { create(:group, :public) } + let(:reporter_from_group_link) { create(:user) } def permissions(user, issue) IssuePolicy.abilities(user, issue).to_set @@ -21,6 +23,10 @@ describe IssuePolicy, models: true do project.team << [author, :guest] project.team << [assignee, :guest] project.team << [reporter, :reporter] + + group.add_reporter(reporter_from_group_link) + + create(:project_group_link, group: group, project: project) end it 'does not allow non-members to read issues' do @@ -41,6 +47,11 @@ describe IssuePolicy, models: true do expect(permissions(reporter, issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) end + it 'allows reporters from group links to read, update, and admin issues' do + expect(permissions(reporter_from_group_link, issue)).to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(reporter_from_group_link, issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) + end + it 'allows issue authors to read and update their issues' do expect(permissions(author, issue)).to include(:read_issue, :update_issue) expect(permissions(author, issue)).not_to include(:admin_issue) @@ -76,6 +87,11 @@ describe IssuePolicy, models: true do expect(permissions(reporter, confidential_issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) end + it 'allows reporters from group links to read, update, and admin confidential issues' do + expect(permissions(reporter_from_group_link, confidential_issue)).to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(reporter_from_group_link, confidential_issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) + end + it 'allows issue authors to read and update their confidential issues' do expect(permissions(author, confidential_issue)).to include(:read_issue, :update_issue) expect(permissions(author, confidential_issue)).not_to include(:admin_issue) @@ -100,6 +116,10 @@ describe IssuePolicy, models: true do before do project.team << [guest, :guest] project.team << [reporter, :reporter] + + group.add_reporter(reporter_from_group_link) + + create(:project_group_link, group: group, project: project) end it 'allows guests to read issues' do @@ -115,6 +135,11 @@ describe IssuePolicy, models: true do expect(permissions(reporter, issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) end + it 'allows reporters from group links to read, update, and admin issues' do + expect(permissions(reporter_from_group_link, issue)).to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(reporter_from_group_link, issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) + end + it 'allows issue authors to read and update their issues' do expect(permissions(author, issue)).to include(:read_issue, :update_issue) expect(permissions(author, issue)).not_to include(:admin_issue) @@ -145,6 +170,11 @@ describe IssuePolicy, models: true do expect(permissions(reporter, confidential_issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) end + it 'allows reporter from group links to read, update, and admin confidential issues' do + expect(permissions(reporter_from_group_link, confidential_issue)).to include(:read_issue, :update_issue, :admin_issue) + expect(permissions(reporter_from_group_link, confidential_issue_no_assignee)).to include(:read_issue, :update_issue, :admin_issue) + end + it 'allows issue authors to read and update their confidential issues' do expect(permissions(author, confidential_issue)).to include(:read_issue, :update_issue) expect(permissions(author, confidential_issue)).not_to include(:admin_issue)