From 384b827201de0dae9e5af62ab35b2d4735c38f93 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 24 Jul 2017 11:35:54 +0100 Subject: [PATCH] Don't treat anonymous users as owners when group has pending invites The `members` table can have entries where `user_id: nil`, because people can invite group members by email. We never want to include those as members, because it might cause confusion with the anonymous (logged out) user. --- app/models/group.rb | 6 +++++- app/policies/project_policy.rb | 3 ++- ...r-500-viewing-notes-with-anonymous-user.yml | 4 ++++ spec/models/ability_spec.rb | 8 ++++---- spec/models/group_spec.rb | 4 ++++ spec/policies/project_policy_spec.rb | 18 ++++++++++++++++++ 6 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/35444-error-500-viewing-notes-with-anonymous-user.yml diff --git a/app/models/group.rb b/app/models/group.rb index be56ec8b7883aa..73cef3c155320f 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -186,10 +186,14 @@ def add_owner(user, current_user = nil) end def has_owner?(user) + return false unless user + members_with_parents.owners.where(user_id: user).any? end def has_master?(user) + return false unless user + members_with_parents.masters.where(user_id: user).any? end @@ -258,7 +262,7 @@ def user_ids_for_project_authorizations end def members_with_parents - GroupMember.non_request.where(source_id: ancestors.pluck(:id).push(id)) + GroupMember.active.where(source_id: ancestors.pluck(:id).push(id)).where.not(user_id: nil) end def users_with_parents diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 59ce6f82f84998..d837fc88697877 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -12,7 +12,8 @@ def self.create_read_update_admin(name) desc "User is a project owner" condition :owner do - @user && project.owner == @user || (project.group && project.group.has_owner?(@user)) + (project.owner.present? && project.owner == @user) || + project.group&.has_owner?(@user) end desc "Project has public builds enabled" diff --git a/changelogs/unreleased/35444-error-500-viewing-notes-with-anonymous-user.yml b/changelogs/unreleased/35444-error-500-viewing-notes-with-anonymous-user.yml new file mode 100644 index 00000000000000..9b8bc1d0d9958b --- /dev/null +++ b/changelogs/unreleased/35444-error-500-viewing-notes-with-anonymous-user.yml @@ -0,0 +1,4 @@ +--- +title: Fix anonymous access to public projects in groups with pending invites +merge_request: +author: diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index dc7a0d80752b75..58f1a620ab4a8b 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -98,7 +98,7 @@ user2 = build(:user, external: true) users = [user1, user2] - expect(project).to receive(:owner).twice.and_return(user1) + expect(project).to receive(:owner).at_least(:once).and_return(user1) expect(described_class.users_that_can_read_project(users, project)) .to eq([user1]) @@ -109,7 +109,7 @@ user2 = build(:user, external: true) users = [user1, user2] - expect(project.team).to receive(:members).twice.and_return([user1]) + expect(project.team).to receive(:members).at_least(:once).and_return([user1]) expect(described_class.users_that_can_read_project(users, project)) .to eq([user1]) @@ -140,7 +140,7 @@ user2 = build(:user, external: true) users = [user1, user2] - expect(project).to receive(:owner).twice.and_return(user1) + expect(project).to receive(:owner).at_least(:once).and_return(user1) expect(described_class.users_that_can_read_project(users, project)) .to eq([user1]) @@ -151,7 +151,7 @@ user2 = build(:user, external: true) users = [user1, user2] - expect(project.team).to receive(:members).twice.and_return([user1]) + expect(project.team).to receive(:members).at_least(:once).and_return([user1]) expect(described_class.users_that_can_read_project(users, project)) .to eq([user1]) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 6cd1758e3fcbcc..bf0a1fe951eb8a 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -255,6 +255,7 @@ describe '#has_owner?' do before do @members = setup_group_members(group) + create(:group_member, :invited, :owner, group: group) end it { expect(group.has_owner?(@members[:owner])).to be_truthy } @@ -263,11 +264,13 @@ it { expect(group.has_owner?(@members[:reporter])).to be_falsey } it { expect(group.has_owner?(@members[:guest])).to be_falsey } it { expect(group.has_owner?(@members[:requester])).to be_falsey } + it { expect(group.has_owner?(nil)).to be_falsey } end describe '#has_master?' do before do @members = setup_group_members(group) + create(:group_member, :invited, :master, group: group) end it { expect(group.has_master?(@members[:owner])).to be_falsey } @@ -276,6 +279,7 @@ it { expect(group.has_master?(@members[:reporter])).to be_falsey } it { expect(group.has_master?(@members[:guest])).to be_falsey } it { expect(group.has_master?(@members[:requester])).to be_falsey } + it { expect(group.has_master?(nil)).to be_falsey } end describe '#lfs_enabled?' do diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index e43a8945e1ae63..4c1a90b87ef69d 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -139,6 +139,24 @@ def expect_disallowed(*permissions) end end + context 'when a project has pending invites, and the current user is anonymous' do + let(:group) { create(:group, :public) } + let(:project) { create(:empty_project, :public, namespace: group) } + let(:user_permissions) { [:read_issue_link, :create_project, :create_issue, :create_note, :upload_file] } + let(:anonymous_permissions) { guest_permissions - user_permissions } + + subject { described_class.new(nil, project) } + + before do + create(:group_member, :invited, group: group) + end + + it 'does not grant owner access' do + expect_allowed(*anonymous_permissions) + expect_disallowed(*user_permissions) + end + end + context 'abilities for non-public projects' do let(:project) { create(:empty_project, namespace: owner.namespace) } -- GitLab