Refactor ability changes

5 files changed, 67 insertions, 71 deletions

diff --git a/app/models/ability.rb b/app/models/ability.rb
index eef481c8f8a..500af08d209 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -240,11 +240,11 @@ class Ability
 
       # Only group owner and administrators can admin group
       if group.has_owner?(user) || user.admin?
-        rules.push(*[
-                     :admin_group,
-                     :admin_namespace,
-                     :admin_group_member
-                   ])
+        rules += [
+          :admin_group,
+          :admin_namespace,
+          :admin_group_member
+        ]
       end
 
       rules.flatten
@@ -255,16 +255,15 @@ class Ability
 
       # Only namespace owner and administrators can admin it
       if namespace.owner == user || user.admin?
-        rules.push(*[
-                     :create_projects,
-                     :admin_namespace
-                   ])
+        rules += [
+          :create_projects,
+          :admin_namespace
+        ]
       end
 
       rules.flatten
     end
 
-
     [:issue, :merge_request].each do |name|
       define_method "#{name}_abilities" do |user, subject|
         rules = []
@@ -305,15 +304,18 @@ 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
-      end
+      unless group.last_owner?(target_user)
+        can_manage = group_abilities(user, group).include?(:admin_group_member)
 
-      if !group.last_owner?(user) && (can_manage || (user == target_user))
-        rules << :destroy_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
 
       rules
@@ -323,16 +325,20 @@ class Ability
       rules = []
       target_user = subject.user
       project = subject.project
-      can_manage = project_abilities(user, project).include?(:admin_project_member)
 
-      if can_manage && user != target_user && target_user != project.owner
-        rules << :update_project_member
-        rules << :destroy_project_member
-      end
+      unless target_user == project.owner
+        can_manage = project_abilities(user, project).include?(:admin_project_member)
 
-      if user == target_user && target_user != project.owner
-        rules << :destroy_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
     end
 
diff --git a/app/models/concerns/has_owners.rb b/app/models/concerns/has_owners.rb
deleted file mode 100644
index 53ef6e939dd..00000000000
--- a/app/models/concerns/has_owners.rb
+++ /dev/null
@@ -1,31 +0,0 @@
-# == Owners concern
-#
-# Contains owners functionality for groups
-#
-module HasOwners
-  extend ActiveSupport::Concern
-
-  def owners
-    @owners ||= members.owners.includes(:user).map(&:user)
-  end
-
-  def members
-    raise NotImplementedError, "Expected 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 11fde7ba6cd..2c9e75496b9 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -20,8 +20,7 @@ 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 :members, :group_members
   has_many :users, through: :group_members
@@ -66,6 +65,10 @@ 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)
@@ -92,6 +95,22 @@ 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 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 eed9f2537e9..28aee2e3799 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -34,16 +34,18 @@ class Member < ActiveRecord::Base
                                     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") }
@@ -100,7 +102,9 @@ class Member < ActiveRecord::Base
     private
 
     def can_update_member?(current_user, member)
-      !current_user || current_user.can?(:update_group_member, 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
diff --git a/app/models/project.rb b/app/models/project.rb
index 09465775786..a099a67cf63 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -42,8 +42,7 @@ class Project < ActiveRecord::Base
   include Sortable
   include AfterCommitQueue
   include CaseSensitivity
-  include HasOwners
-
+  
   extend Gitlab::ConfigHelper
   extend Enumerize
 
@@ -117,7 +116,6 @@ 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