refactored permissions and added update_project_member ability logic. Also refactored owner methods to a concern.

5 files changed, 57 insertions, 24 deletions

diff --git a/app/models/ability.rb b/app/models/ability.rb
index b72178fa126..5beead0b75d 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
@@ -316,6 +317,23 @@ class Ability
       rules
     end
 
+    def project_member_abilities(user, subject)
+      rules = []
+      target_user = subject.user
+      project = subject.project
+      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 !project.last_owner?(user) && (can_manage || (user == target_user))
+        rules << :destroy_project_member
+      end
+      rules
+    end
+
     def abilities
       @abilities ||= begin
                        abilities = Six.new
diff --git a/app/models/concerns/has_owners.rb b/app/models/concerns/has_owners.rb
new file mode 100644
index 00000000000..735b2071721
--- /dev/null
+++ b/app/models/concerns/has_owners.rb
@@ -0,0 +1,31 @@
+# == Owners concern
+#
+# Contains owners functionality for groups
+#
+module HasOwners
+  extend ActiveSupport::Concern
+
+  def owners
+    @owners ||= my_members.owners.includes(:user).map(&:user)
+  end
+
+  def my_members
+    raise NotImplementedError, "Expected my_members to be defined in #{self.class.name}"
+  end
+
+  def add_owner(user, current_user = nil)
+    add_user(user, Gitlab::Access::OWNER, current_user)
+  end
+
+  def has_owner?(user)
+    owners.include?(user)
+  end
+
+  def has_master?(user)
+    members.masters.where(user_id: user).any?
+  end
+
+  def last_owner?(user)
+    has_owner?(user) && owners.size == 1
+  end
+end
diff --git a/app/models/group.rb b/app/models/group.rb
index 465c22d23ac..c9806f6fd6f 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -19,8 +19,10 @@ require 'file_size_validator'
 class Group < Namespace
   include Gitlab::ConfigHelper
   include Referable
+  include HasOwners
 
   has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember'
+  alias_method :my_members, :group_members
   has_many :users, through: :group_members
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
@@ -63,10 +65,6 @@ class Group < Namespace
     end
   end
 
-  def owners
-    @owners ||= group_members.owners.includes(:user).map(&:user)
-  end
-
   def add_users(user_ids, access_level, current_user = nil)
     user_ids.each do |user_id|
       Member.add_user(self.group_members, user_id, access_level, current_user)
@@ -93,22 +91,6 @@ class Group < Namespace
     add_user(user, Gitlab::Access::MASTER, current_user)
   end
 
-  def add_owner(user, current_user = nil)
-    add_user(user, Gitlab::Access::OWNER, current_user)
-  end
-
-  def has_owner?(user)
-    owners.include?(user)
-  end
-
-  def has_master?(user)
-    members.masters.where(user_id: user).any?
-  end
-
-  def last_owner?(user)
-    has_owner?(user) && owners.size == 1
-  end
-
   def members
     group_members
   end
diff --git a/app/models/member.rb b/app/models/member.rb
index 4651c8fff37..c565ee6bbce 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -82,8 +82,7 @@ class Member < ActiveRecord::Base
         member.invite_email = user
       end
 
-      project = members.first.respond_to?(:project)? members.first.project : nil
-      if can_update_member?(current_user, member, project)
+      if can_update_member?(current_user, member)
         member.created_by ||= current_user
         member.access_level = access_level
 
@@ -93,9 +92,10 @@ class Member < ActiveRecord::Base
 
     private
 
-    def can_update_member?(current_user, member, project)
+    def can_update_member?(current_user, member)
       !current_user || current_user.can?(:update_group_member, member) ||
-        (project && current_user.can?(:admin_project_member, project))
+        (member.respond_to?(:project) &&
+          current_user.can?(:update_project_member, member))
     end
   end
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 74b89aad499..79b7a6457d7 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -41,6 +41,7 @@ class Project < ActiveRecord::Base
   include Sortable
   include AfterCommitQueue
   include CaseSensitivity
+  include HasOwners
 
   extend Gitlab::ConfigHelper
   extend Enumerize
@@ -114,6 +115,7 @@ class Project < ActiveRecord::Base
   has_many :hooks,              dependent: :destroy, class_name: 'ProjectHook'
   has_many :protected_branches, dependent: :destroy
   has_many :project_members, dependent: :destroy, as: :source, class_name: 'ProjectMember'
+  alias_method :my_members, :project_members
   has_many :users, through: :project_members
   has_many :deploy_keys_projects, dependent: :destroy
   has_many :deploy_keys, through: :deploy_keys_projects