Commit 22ba5d8a authored by Rémy Coutable's avatar Rémy Coutable

New :request_access ability to replace a ugly helper

- 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: default avatarRémy Coutable <remy@rymai.me>
parent aad62735
...@@ -12,17 +12,6 @@ module MembersHelper ...@@ -12,17 +12,6 @@ module MembersHelper
can?(current_user, action_member_permission(:admin, member), member.source) can?(current_user, action_member_permission(:admin, member), member.source)
end 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) def remove_member_message(member, user: nil)
user = current_user if defined?(current_user) user = current_user if defined?(current_user)
......
...@@ -157,10 +157,11 @@ class Ability ...@@ -157,10 +157,11 @@ class Ability
# Push abilities on the users team role # Push abilities on the users team role
rules.push(*project_team_rules(project.team, user)) rules.push(*project_team_rules(project.team, user))
if project.owner == user || owner = project.owner == user ||
(project.group && project.group.has_owner?(user)) || (project.group && project.group.has_owner?(user)) ||
user.admin? user.admin?
if owner
rules.push(*project_owner_rules) rules.push(*project_owner_rules)
end end
...@@ -169,6 +170,15 @@ class Ability ...@@ -169,6 +170,15 @@ class Ability
# Allow to read builds for internal projects # Allow to read builds for internal projects
rules << :read_build if project.public_builds? 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 end
if project.archived? if project.archived?
...@@ -345,8 +355,11 @@ class Ability ...@@ -345,8 +355,11 @@ class Ability
rules = [] rules = []
rules << :read_group if can_read_group?(user, group) 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 # Only group masters and group owners can create new projects
if group.has_master?(user) || group.has_owner?(user) || user.admin? if master
rules += [ rules += [
:create_projects, :create_projects,
:admin_milestones :admin_milestones
...@@ -354,7 +367,7 @@ class Ability ...@@ -354,7 +367,7 @@ class Ability
end end
# Only group owner and administrators can admin group # Only group owner and administrators can admin group
if group.has_owner?(user) || user.admin? if owner
rules += [ rules += [
:admin_group, :admin_group,
:admin_namespace, :admin_namespace,
...@@ -363,6 +376,10 @@ class Ability ...@@ -363,6 +376,10 @@ class Ability
] ]
end end
if (group.public? || (group.internal? && !user.external?))
rules << :request_access unless group.users.include?(user)
end
rules.flatten rules.flatten
end end
...@@ -484,7 +501,8 @@ class Ability ...@@ -484,7 +501,8 @@ class Ability
target_user = subject.user target_user = subject.user
project = subject.project 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) can_manage = project_abilities(user, project).include?(:admin_project_member)
if can_manage if can_manage
......
- 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) - if requester = source.requesters.find_by(user_id: current_user.id)
= link_to 'Withdraw Access Request', polymorphic_path([:leave, source, :members]), = link_to 'Withdraw Access Request', polymorphic_path([:leave, source, :members]),
method: :delete, method: :delete,
......
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
...@@ -5,9 +5,6 @@ feature 'Projects > Members > Group member cannot request access to his group pr ...@@ -5,9 +5,6 @@ feature 'Projects > Members > Group member cannot request access to his group pr
let(:group) { create(:group) } let(:group) { create(:group) }
let(:project) { create(:project, namespace: group) } let(:project) { create(:project, namespace: group) }
background do
end
scenario 'owner does not see the request access button' do scenario 'owner does not see the request access button' do
group.add_owner(user) group.add_owner(user)
login_and_visit_project_page(user) login_and_visit_project_page(user)
......
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
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
...@@ -57,72 +57,6 @@ describe MembersHelper do ...@@ -57,72 +57,6 @@ describe MembersHelper do
end end
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 describe '#remove_member_message' do
let(:requester) { build(:user) } let(:requester) { build(:user) }
let(:project) { create(:project) } let(:project) { create(:project) }
......
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