Merge branch 'fix-18717' into 'master'

Ensure that group owner cannot request access to a project of their group

## What does this MR do?

It fixes two things:

- 91ad995d69e1a0f8991fd896f1d9febc109273fe Ensure that group owner cannot request access to a project of their group
- ec3ff061148d556757e7cd486cdc6083d77acf34 Ensure group/project owners can see their members' access_level (see the commit message for details)

## Are there points in the code the reviewer needs to double check?

Not really, these are pretty simple fixes.

## Why was this MR needed?

Because there was an issue created!

## What are the relevant issue numbers?

Fixes #18717.

## Does this MR meet the acceptance criteria?

- [x] CHANGELOG is not needed since the bug is only present in a 8.9 RC
- [x] Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !4729

6 files changed, 137 insertions, 11 deletions

diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb
index 877c77050be..ec106418f2d 100644
--- a/app/helpers/members_helper.rb
+++ b/app/helpers/members_helper.rb
@@ -6,6 +6,12 @@ module MembersHelper
     "#{action}_#{member.type.underscore}".to_sym
   end
 
+  def default_show_roles(member)
+    can?(current_user, action_member_permission(:update, member), member) ||
+      can?(current_user, action_member_permission(:destroy, member), member) ||
+      can?(current_user, action_member_permission(:admin, member), member.source)
+  end
+
   def remove_member_message(member, user: nil)
     user = current_user if defined?(current_user)
 
diff --git a/app/views/shared/members/_access_request_buttons.html.haml b/app/views/shared/members/_access_request_buttons.html.haml
index ed0a6ebcf84..480e8ba6c85 100644
--- a/app/views/shared/members/_access_request_buttons.html.haml
+++ b/app/views/shared/members/_access_request_buttons.html.haml
@@ -1,12 +1,14 @@
 - member = source.members.find_by(user_id: current_user.id)
+- group_member = source.group.members.find_by(user_id: current_user.id) if source.respond_to?(:group) && source.group
 
-- if member
-  - if member.request?
-    = link_to 'Withdraw Access Request', polymorphic_path([:leave, source, :members]),
-              method: :delete,
-              data: { confirm: remove_member_message(member) },
+- unless group_member
+  - if member
+    - if member.request?
+      = link_to 'Withdraw Access Request', polymorphic_path([:leave, source, :members]),
+                method: :delete,
+                data: { confirm: remove_member_message(member) },
+                class: 'btn access-request-button hidden-xs'
+  - else
+    = link_to 'Request Access', polymorphic_path([:request_access, source, :members]),
+              method: :post,
               class: 'btn access-request-button hidden-xs'
-- else
-  = link_to 'Request Access', polymorphic_path([:request_access, source, :members]),
-            method: :post,
-            class: 'btn access-request-button hidden-xs'
diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml
index 0191814849a..a884e78e6e7 100644
--- a/app/views/shared/members/_member.html.haml
+++ b/app/views/shared/members/_member.html.haml
@@ -1,5 +1,4 @@
-- default_show_roles = can?(current_user, action_member_permission(:update, member), member) || can?(current_user, action_member_permission(:destroy, member), member)
-- show_roles = local_assigns.fetch(:show_roles, default_show_roles)
+- show_roles = local_assigns.fetch(:show_roles, default_show_roles(member))
 - show_controls = local_assigns.fetch(:show_controls, true)
 - user = member.user
 
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
new file mode 100644
index 00000000000..4d5d656f00c
--- /dev/null
+++ b/spec/features/projects/members/group_member_cannot_request_access_to_his_group_project_spec.rb
@@ -0,0 +1,50 @@
+require 'spec_helper'
+
+feature 'Projects > Members > Group member cannot request access to his group project', feature: true do
+  let(:user) { create(:user) }
+  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)
+
+    expect(page).not_to have_content 'Request Access'
+  end
+
+  scenario 'master does not see the request access button' do
+    group.add_master(user)
+    login_and_visit_project_page(user)
+
+    expect(page).not_to have_content 'Request Access'
+  end
+
+  scenario 'developer does not see the request access button' do
+    group.add_developer(user)
+    login_and_visit_project_page(user)
+
+    expect(page).not_to have_content 'Request Access'
+  end
+
+  scenario 'reporter does not see the request access button' do
+    group.add_reporter(user)
+    login_and_visit_project_page(user)
+
+    expect(page).not_to have_content 'Request Access'
+  end
+
+  scenario 'guest does not see the request access button' do
+    group.add_guest(user)
+    login_and_visit_project_page(user)
+
+    expect(page).not_to have_content 'Request Access'
+  end
+
+  def login_and_visit_project_page(user)
+    login_as(user)
+    visit namespace_project_path(project.namespace, project)
+  end
+end
diff --git a/spec/features/projects/members/group_requester_cannot_request_access_to_project_spec.rb b/spec/features/projects/members/group_requester_cannot_request_access_to_project_spec.rb
new file mode 100644
index 00000000000..c4ed92d2780
--- /dev/null
+++ b/spec/features/projects/members/group_requester_cannot_request_access_to_project_spec.rb
@@ -0,0 +1,21 @@
+require 'spec_helper'
+
+feature 'Projects > Members > Group requester cannot request access to project', feature: true do
+  let(:user) { create(:user) }
+  let(:owner) { create(:user) }
+  let(:group) { create(:group, :public) }
+  let(:project) { create(:project, :public, namespace: group) }
+
+  background do
+    group.add_owner(owner)
+    login_as(user)
+    visit group_path(group)
+    perform_enqueued_jobs { click_link 'Request Access' }
+    visit namespace_project_path(project.namespace, project)
+  end
+
+  scenario 'group requester does not see the request access / withdraw access request button' do
+    expect(page).not_to have_content 'Request Access'
+    expect(page).not_to have_content 'Withdraw Access Request'
+  end
+end
diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb
index 7998209b7b0..f75fdb739f6 100644
--- a/spec/helpers/members_helper_spec.rb
+++ b/spec/helpers/members_helper_spec.rb
@@ -9,6 +9,54 @@ describe MembersHelper do
     it { expect(action_member_permission(:admin, group_member)).to eq :admin_group_member }
   end
 
+  describe '#default_show_roles' do
+    let(:user) { double }
+    let(:member) { build(:project_member) }
+
+    before do
+      allow(helper).to receive(:current_user).and_return(user)
+      allow(helper).to receive(:can?).with(user, :update_project_member, member).and_return(false)
+      allow(helper).to receive(:can?).with(user, :destroy_project_member, member).and_return(false)
+      allow(helper).to receive(:can?).with(user, :admin_project_member, member.source).and_return(false)
+    end
+
+    context 'when the current cannot update, destroy or admin the passed member' do
+      it 'returns false' do
+        expect(helper.default_show_roles(member)).to be_falsy
+      end
+    end
+
+    context 'when the current can update the passed member' do
+      before do
+        allow(helper).to receive(:can?).with(user, :update_project_member, member).and_return(true)
+      end
+
+      it 'returns true' do
+        expect(helper.default_show_roles(member)).to be_truthy
+      end
+    end
+
+    context 'when the current can destroy the passed member' do
+      before do
+        allow(helper).to receive(:can?).with(user, :destroy_project_member, member).and_return(true)
+      end
+
+      it 'returns true' do
+        expect(helper.default_show_roles(member)).to be_truthy
+      end
+    end
+
+    context 'when the current can admin the passed member source' do
+      before do
+        allow(helper).to receive(:can?).with(user, :admin_project_member, member.source).and_return(true)
+      end
+
+      it 'returns true' do
+        expect(helper.default_show_roles(member)).to be_truthy
+      end
+    end
+  end
+
   describe '#remove_member_message' do
     let(:requester) { build(:user) }
     let(:project) { create(:project) }