From 22ba5d8a7f0920f39ba33bdc4af54531ffe40b1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 5 Jul 2016 14:24:58 +0200 Subject: New :request_access ability to replace a ugly helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Group / project members cannot request access - Group members cannot request access to a group's project This addresses an issue where project owners could request access to their own project, leading to UI inconsistency where their requester status would replace their owner status. Signed-off-by: Rémy Coutable --- app/helpers/members_helper.rb | 11 ---- app/models/ability.rb | 30 ++++++++-- .../members/_access_request_buttons.html.haml | 2 +- ...er_cannot_request_access_to_his_project_spec.rb | 16 ++++++ ...not_request_access_to_his_group_project_spec.rb | 3 - ...er_cannot_request_access_to_his_project_spec.rb | 16 ++++++ ...er_cannot_request_access_to_his_project_spec.rb | 16 ++++++ spec/helpers/members_helper_spec.rb | 66 ---------------------- 8 files changed, 73 insertions(+), 87 deletions(-) create mode 100644 spec/features/groups/members/member_cannot_request_access_to_his_project_spec.rb create mode 100644 spec/features/projects/members/member_cannot_request_access_to_his_project_spec.rb create mode 100644 spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb index c70cd19b587..ec106418f2d 100644 --- a/app/helpers/members_helper.rb +++ b/app/helpers/members_helper.rb @@ -12,17 +12,6 @@ module MembersHelper can?(current_user, action_member_permission(:admin, member), member.source) end - def can_see_request_access_button?(source) - source_parent = source.respond_to?(:group) && source.group - - return false if source_parent && source.group.members.exists?(user_id: current_user.id) - return false if source_parent && source.group.requesters.exists?(user_id: current_user.id) - return false if source.members.exists?(user_id: current_user.id) - return true if source.requesters.exists?(user_id: current_user.id) - - true - end - def remove_member_message(member, user: nil) user = current_user if defined?(current_user) diff --git a/app/models/ability.rb b/app/models/ability.rb index ba1f2ae4075..ec4ef287421 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -157,10 +157,11 @@ class Ability # Push abilities on the users team role rules.push(*project_team_rules(project.team, user)) - if project.owner == user || - (project.group && project.group.has_owner?(user)) || - user.admin? + owner = project.owner == user || + (project.group && project.group.has_owner?(user)) || + user.admin? + if owner rules.push(*project_owner_rules) end @@ -169,6 +170,15 @@ class Ability # Allow to read builds for internal projects rules << :read_build if project.public_builds? + + group_member = + project.group && + ( + project.group.members.exists?(user_id: user.id) || + project.group.requesters.exists?(user_id: user.id) + ) + + rules << :request_access unless owner || project.team.member?(user) || group_member end if project.archived? @@ -345,8 +355,11 @@ class Ability rules = [] rules << :read_group if can_read_group?(user, group) + owner = group.has_owner?(user) || user.admin? + master = owner || user.admin? + # Only group masters and group owners can create new projects - if group.has_master?(user) || group.has_owner?(user) || user.admin? + if master rules += [ :create_projects, :admin_milestones @@ -354,7 +367,7 @@ class Ability end # Only group owner and administrators can admin group - if group.has_owner?(user) || user.admin? + if owner rules += [ :admin_group, :admin_namespace, @@ -363,6 +376,10 @@ class Ability ] end + if (group.public? || (group.internal? && !user.external?)) + rules << :request_access unless group.users.include?(user) + end + rules.flatten end @@ -484,7 +501,8 @@ class Ability target_user = subject.user project = subject.project - unless target_user == project.owner + # Allow owners that requested access to their own project to destroy themselves + if target_user != project.owner || subject.request? can_manage = project_abilities(user, project).include?(:admin_project_member) if can_manage diff --git a/app/views/shared/members/_access_request_buttons.html.haml b/app/views/shared/members/_access_request_buttons.html.haml index 35dcdccc921..eff914398bb 100644 --- a/app/views/shared/members/_access_request_buttons.html.haml +++ b/app/views/shared/members/_access_request_buttons.html.haml @@ -1,4 +1,4 @@ -- if can_see_request_access_button?(source) +- if can?(current_user, :request_access, source) - if requester = source.requesters.find_by(user_id: current_user.id) = link_to 'Withdraw Access Request', polymorphic_path([:leave, source, :members]), method: :delete, diff --git a/spec/features/groups/members/member_cannot_request_access_to_his_project_spec.rb b/spec/features/groups/members/member_cannot_request_access_to_his_project_spec.rb new file mode 100644 index 00000000000..37c433cc09a --- /dev/null +++ b/spec/features/groups/members/member_cannot_request_access_to_his_project_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +feature 'Groups > Members > Member cannot request access to his project', feature: true do + let(:member) { create(:user) } + let(:group) { create(:group) } + + background do + group.add_developer(member) + login_as(member) + visit group_path(group) + end + + scenario 'member does not see the request access button' do + expect(page).not_to have_content 'Request Access' + end +end diff --git a/spec/features/projects/members/group_member_cannot_request_access_to_his_group_project_spec.rb b/spec/features/projects/members/group_member_cannot_request_access_to_his_group_project_spec.rb index 4d5d656f00c..ff9b6007806 100644 --- a/spec/features/projects/members/group_member_cannot_request_access_to_his_group_project_spec.rb +++ b/spec/features/projects/members/group_member_cannot_request_access_to_his_group_project_spec.rb @@ -5,9 +5,6 @@ feature 'Projects > Members > Group member cannot request access to his group pr let(:group) { create(:group) } let(:project) { create(:project, namespace: group) } - background do - end - scenario 'owner does not see the request access button' do group.add_owner(user) login_and_visit_project_page(user) diff --git a/spec/features/projects/members/member_cannot_request_access_to_his_project_spec.rb b/spec/features/projects/members/member_cannot_request_access_to_his_project_spec.rb new file mode 100644 index 00000000000..9564347e733 --- /dev/null +++ b/spec/features/projects/members/member_cannot_request_access_to_his_project_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +feature 'Projects > Members > Member cannot request access to his project', feature: true do + let(:member) { create(:user) } + let(:project) { create(:project) } + + background do + project.team << [member, :developer] + login_as(member) + visit namespace_project_path(project.namespace, project) + end + + scenario 'member does not see the request access button' do + expect(page).not_to have_content 'Request Access' + end +end diff --git a/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb b/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb new file mode 100644 index 00000000000..0e54c4fdf20 --- /dev/null +++ b/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +feature 'Projects > Members > Owner cannot request access to his project', feature: true do + let(:owner) { create(:user) } + let(:project) { create(:project) } + + background do + project.team << [owner, :owner] + login_as(owner) + visit namespace_project_path(project.namespace, project) + end + + scenario 'owner does not see the request access button' do + expect(page).not_to have_content 'Request Access' + end +end diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb index 7b2155e9a4e..f75fdb739f6 100644 --- a/spec/helpers/members_helper_spec.rb +++ b/spec/helpers/members_helper_spec.rb @@ -57,72 +57,6 @@ describe MembersHelper do end end - describe '#can_see_request_access_button?' do - let(:user) { create(:user) } - let(:group) { create(:group, :public) } - let(:project) { create(:project, :public, group: group) } - - before do - allow(helper).to receive(:current_user).and_return(user) - end - - context 'source is a group' do - context 'current_user is not a member' do - it 'returns true' do - expect(helper.can_see_request_access_button?(group)).to be_truthy - end - end - - context 'current_user is a member' do - it 'returns false' do - group.add_owner(user) - - expect(helper.can_see_request_access_button?(group)).to be_falsy - end - end - - context 'current_user is a requester' do - it 'returns true' do - group.request_access(user) - - expect(helper.can_see_request_access_button?(group)).to be_truthy - end - end - end - - context 'source is a project' do - context 'current_user is not a member' do - it 'returns true' do - expect(helper.can_see_request_access_button?(project)).to be_truthy - end - end - - context 'current_user is a group member' do - it 'returns false' do - group.add_owner(user) - - expect(helper.can_see_request_access_button?(project)).to be_falsy - end - end - - context 'current_user is a group requester' do - it 'returns false' do - group.request_access(user) - - expect(helper.can_see_request_access_button?(project)).to be_falsy - end - end - - context 'current_user is a member' do - it 'returns false' do - project.team << [user, :master] - - expect(helper.can_see_request_access_button?(project)).to be_falsy - end - end - end - end - describe '#remove_member_message' do let(:requester) { build(:user) } let(:project) { create(:project) } -- cgit v1.2.1 From 9ea80a196f14f55599ab9c9831788dd970a36966 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 5 Jul 2016 16:58:27 +0200 Subject: Fix condition in Ability and start with cheaper checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/ability.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index ec4ef287421..2c0fd0338fd 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -157,9 +157,9 @@ class Ability # Push abilities on the users team role rules.push(*project_team_rules(project.team, user)) - owner = project.owner == user || - (project.group && project.group.has_owner?(user)) || - user.admin? + owner = user.admin? || + project.owner == user || + (project.group && project.group.has_owner?(user)) if owner rules.push(*project_owner_rules) @@ -178,7 +178,7 @@ class Ability project.group.requesters.exists?(user_id: user.id) ) - rules << :request_access unless owner || project.team.member?(user) || group_member + rules << :request_access unless owner || group_member || project.team.member?(user) end if project.archived? @@ -355,8 +355,8 @@ class Ability rules = [] rules << :read_group if can_read_group?(user, group) - owner = group.has_owner?(user) || user.admin? - master = owner || user.admin? + owner = user.admin? || group.has_owner?(user) + master = owner || group.has_master?(user) # Only group masters and group owners can create new projects if master @@ -376,7 +376,7 @@ class Ability ] end - if (group.public? || (group.internal? && !user.external?)) + if group.public? || (group.internal? && !user.external?) rules << :request_access unless group.users.include?(user) end -- cgit v1.2.1 From 19b80e82521384284227b31003889c9ac41b7c8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 5 Jul 2016 18:55:35 +0200 Subject: Add a migration to remove requesters that are owners of their project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/ability.rb | 22 ++++++------ ...0705163108_remove_requesters_that_are_owners.rb | 40 ++++++++++++++++++++++ db/schema.rb | 2 +- 3 files changed, 53 insertions(+), 11 deletions(-) create mode 100644 db/migrate/20160705163108_remove_requesters_that_are_owners.rb diff --git a/app/models/ability.rb b/app/models/ability.rb index 2c0fd0338fd..eeb0ceba081 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -171,14 +171,9 @@ class Ability # Allow to read builds for internal projects rules << :read_build if project.public_builds? - group_member = - project.group && - ( - project.group.members.exists?(user_id: user.id) || - project.group.requesters.exists?(user_id: user.id) - ) - - rules << :request_access unless owner || group_member || project.team.member?(user) + unless owner || project.team.member?(user) || project_group_member?(project, user) + rules << :request_access + end end if project.archived? @@ -501,8 +496,7 @@ class Ability target_user = subject.user project = subject.project - # Allow owners that requested access to their own project to destroy themselves - if target_user != project.owner || subject.request? + unless target_user == project.owner can_manage = project_abilities(user, project).include?(:admin_project_member) if can_manage @@ -582,5 +576,13 @@ class Ability rules end + + def project_group_member?(project, user) + project.group && + ( + project.group.members.exists?(user_id: user.id) || + project.group.requesters.exists?(user_id: user.id) + ) + end end end diff --git a/db/migrate/20160705163108_remove_requesters_that_are_owners.rb b/db/migrate/20160705163108_remove_requesters_that_are_owners.rb new file mode 100644 index 00000000000..1fca230c019 --- /dev/null +++ b/db/migrate/20160705163108_remove_requesters_that_are_owners.rb @@ -0,0 +1,40 @@ +class RemoveRequestersThatAreOwners < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + def up + # Delete requesters that are owner of their projects and actually requested + # access to it + execute <<-SQL + DELETE FROM members + WHERE members.source_type = 'Project' + AND members.type = 'ProjectMember' + AND members.requested_at IS NOT NULL + AND members.user_id = ( + SELECT namespaces.owner_id + FROM namespaces + JOIN projects ON namespaces.id = projects.namespace_id + WHERE namespaces.type IS NULL + AND projects.id = members.source_id + AND namespaces.owner_id = members.user_id); + SQL + + # Delete requesters that are owner of their project's group and actually requested + # access to it + execute <<-SQL + DELETE FROM members + WHERE members.source_type = 'Project' + AND members.type = 'ProjectMember' + AND members.requested_at IS NOT NULL + AND members.user_id = ( + SELECT namespaces.owner_id + FROM namespaces + JOIN projects ON namespaces.id = projects.namespace_id + WHERE namespaces.type = 'Group' + AND projects.id = members.source_id + AND namespaces.owner_id = members.user_id); + SQL + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index 5b9ed985fac..c1e88c1ed7e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160703180340) do +ActiveRecord::Schema.define(version: 20160705163108) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" -- cgit v1.2.1