From ecb83afabcb69d80995b56323bb89b1ee1176225 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 17 Nov 2015 15:49:37 +0100 Subject: Refactor ability changes --- app/models/ability.rb | 54 ++++++++++++++++++++++----------------- app/models/concerns/has_owners.rb | 31 ---------------------- app/models/group.rb | 23 +++++++++++++++-- app/models/member.rb | 26 +++++++++++-------- app/models/project.rb | 4 +-- 5 files changed, 67 insertions(+), 71 deletions(-) delete mode 100644 app/models/concerns/has_owners.rb 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 -- cgit v1.2.3