Merge branch '18790-dont-show-request-button-to-project-owner' into 'master'

Don't show "request access" button to project owners

This MR fixes an issue where project owners that are not in the project's members list (I believe this is how we handled project owners before, now we seem to create a "Master" member for the project creator) would see the `Request Access` button.

This MR fixes this issue in a clean way by adding a new `:request_access` ability to replace an ugly helper.

It also give project owners the  ability to update & destroy a requester that would happen to be themselves (since owners could request access to their own project before this MR).

Related to #18790.

See merge request !5091

10 files changed, 115 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..eeb0ceba081 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 = user.admin? ||
+                project.owner == user ||
+                (project.group && project.group.has_owner?(user))
 
+        if owner
           rules.push(*project_owner_rules)
         end
 
@@ -169,6 +170,10 @@ class Ability
 
           # Allow to read builds for internal projects
           rules << :read_build if project.public_builds?
+
+          unless owner || project.team.member?(user) || project_group_member?(project, user)
+            rules << :request_access
+          end
         end
 
         if project.archived?
@@ -345,8 +350,11 @@ class Ability
       rules = []
       rules << :read_group if can_read_group?(user, group)
 
+      owner = user.admin? || group.has_owner?(user)
+      master = owner || group.has_master?(user)
+
       # 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 +362,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 +371,10 @@ class Ability
         ]
       end
 
+      if group.public? || (group.internal? && !user.external?)
+        rules << :request_access unless group.users.include?(user)
+      end
+
       rules.flatten
     end
 
@@ -564,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/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/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"
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) }