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: Rémy Coutable <remy@rymai.me>

8 files changed, 73 insertions, 87 deletions

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) }