Merge branch 'james11/gitlab-ce-removable-group-owner' into 'master'

Prevent the last owner of a group from being able to delete themselves
by 'adding' themselves as a master

Replaces !1708.

Fixes #1111.

See merge request !1815

4 files changed, 73 insertions, 32 deletions

diff --git a/app/models/ability.rb b/app/models/ability.rb
index d01b3ae6f05..500af08d209 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -15,6 +15,7 @@ class Ability
       when "Group" then group_abilities(user, subject)
       when "Namespace" then namespace_abilities(user, subject)
       when "GroupMember" then group_member_abilities(user, subject)
+      when "ProjectMember" then project_member_abilities(user, subject)
       else []
       end.concat(global_abilities(user))
     end
@@ -231,19 +232,19 @@ class Ability
 
       # Only group masters and group owners can create new projects in group
       if group.has_master?(user) || group.has_owner?(user) || user.admin?
-        rules.push(*[
+        rules += [
           :create_projects,
           :admin_milestones
-        ])
+        ]
       end
 
       # Only group owner and administrators can admin group
       if group.has_owner?(user) || user.admin?
-        rules.push(*[
+        rules += [
           :admin_group,
           :admin_namespace,
           :admin_group_member
-        ])
+        ]
       end
 
       rules.flatten
@@ -254,16 +255,15 @@ class Ability
 
       # Only namespace owner and administrators can admin it
       if namespace.owner == user || user.admin?
-        rules.push(*[
+        rules += [
           :create_projects,
           :admin_namespace
-        ])
+        ]
       end
 
       rules.flatten
     end
 
-
     [:issue, :merge_request].each do |name|
       define_method "#{name}_abilities" do |user, subject|
         rules = []
@@ -304,15 +304,39 @@ class Ability
       rules = []
       target_user = subject.user
       group = subject.group
-      can_manage = group_abilities(user, group).include?(:admin_group_member)
 
-      if can_manage && (user != target_user)
-        rules << :update_group_member
-        rules << :destroy_group_member
+      unless group.last_owner?(target_user)
+        can_manage = group_abilities(user, group).include?(:admin_group_member)
+
+        if can_manage && user != target_user
+          rules << :update_group_member
+          rules << :destroy_group_member
+        end
+
+        if user == target_user
+          rules << :destroy_group_member
+        end
       end
 
-      if !group.last_owner?(user) && (can_manage || (user == target_user))
-        rules << :destroy_group_member
+      rules
+    end
+
+    def project_member_abilities(user, subject)
+      rules = []
+      target_user = subject.user
+      project = subject.project
+
+      unless target_user == project.owner
+        can_manage = project_abilities(user, project).include?(:admin_project_member)
+
+        if can_manage && user != target_user
+          rules << :update_project_member
+          rules << :destroy_project_member
+        end
+
+        if user == target_user
+          rules << :destroy_project_member
+        end
       end
 
       rules
@@ -320,10 +344,10 @@ class Ability
 
     def abilities
       @abilities ||= begin
-                       abilities = Six.new
-                       abilities << self
-                       abilities
-                     end
+        abilities = Six.new
+        abilities << self
+        abilities
+      end
     end
 
     private
diff --git a/app/models/group.rb b/app/models/group.rb
index 793a3b5ef2e..2c9e75496b9 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -20,8 +20,9 @@ require 'file_size_validator'
 class Group < Namespace
   include Gitlab::ConfigHelper
   include Referable
-
+  
   has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember'
+  alias_method :members, :group_members
   has_many :users, through: :group_members
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
@@ -110,10 +111,6 @@ class Group < Namespace
     has_owner?(user) && owners.size == 1
   end
 
-  def members
-    group_members
-  end
-
   def avatar_type
     unless self.avatar.image?
       self.errors.add :avatar, "only images allowed"
diff --git a/app/models/member.rb b/app/models/member.rb
index cae8caa23fb..28aee2e3799 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -30,13 +30,22 @@ class Member < ActiveRecord::Base
 
   validates :user, presence: true, unless: :invite?
   validates :source, presence: true
-  validates :user_id, uniqueness: { scope: [:source_type, :source_id], 
+  validates :user_id, uniqueness: { scope: [:source_type, :source_id],
                                     message: "already exists in source",
                                     allow_nil: true }
   validates :access_level, inclusion: { in: Gitlab::Access.all_values }, presence: true
-  validates :invite_email,  presence: { if: :invite? }, 
-                            email: { strict_mode: true, allow_nil: true }, 
-                            uniqueness: { scope: [:source_type, :source_id], allow_nil: true }
+  validates :invite_email,
+    presence: {
+      if: :invite?
+    },
+    email: {
+      strict_mode: true,
+      allow_nil: true
+    },
+    uniqueness: {
+      scope: [:source_type, :source_id],
+      allow_nil: true
+    }
 
   scope :invite, -> { where(user_id: nil) }
   scope :non_invite, -> { where("user_id IS NOT NULL") }
@@ -73,7 +82,7 @@ class Member < ActiveRecord::Base
 
     def add_user(members, user_id, access_level, current_user = nil)
       user = user_for_id(user_id)
-      
+
       # `user` can be either a User object or an email to be invited
       if user.is_a?(User)
         member = members.find_or_initialize_by(user_id: user.id)
@@ -82,10 +91,21 @@ class Member < ActiveRecord::Base
         member.invite_email = user
       end
 
-      member.created_by ||= current_user
-      member.access_level = access_level
+      if can_update_member?(current_user, member)
+        member.created_by ||= current_user
+        member.access_level = access_level
+
+        member.save
+      end
+    end
+
+    private
 
-      member.save
+    def can_update_member?(current_user, member)
+      # There is no current user for bulk actions, in which case anything is allowed
+      !current_user ||
+        current_user.can?(:update_group_member, member) ||
+        current_user.can?(:update_project_member, member)
     end
   end
 
@@ -95,7 +115,7 @@ class Member < ActiveRecord::Base
 
   def accept_invite!(new_user)
     return false unless invite?
-    
+
     self.invite_token = nil
     self.invite_accepted_at = Time.now.utc
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 9ea0d15497a..a099a67cf63 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -42,7 +42,7 @@ class Project < ActiveRecord::Base
   include Sortable
   include AfterCommitQueue
   include CaseSensitivity
-
+  
   extend Gitlab::ConfigHelper
   extend Enumerize