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

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)
 

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 ||
+        owner = user.admin? ||
-          (project.group && project.group.has_owner?(user)) ||
+                project.owner == user ||
-          user.admin?
+                (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

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,

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

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"

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

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)

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

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

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