diff options
| -rw-r--r-- | app/models/ability.rb | 44 | ||||
| -rw-r--r-- | app/models/concerns/has_owners.rb | 31 | ||||
| -rw-r--r-- | app/models/group.rb | 26 | ||||
| -rw-r--r-- | app/models/member.rb | 34 | ||||
| -rw-r--r-- | app/models/project.rb | 2 | ||||
| -rw-r--r-- | features/groups.feature | 8 | ||||
| -rw-r--r-- | features/steps/groups.rb | 18 |
7 files changed, 117 insertions, 46 deletions
diff --git a/app/models/ability.rb b/app/models/ability.rb index d01b3ae6f05..eef481c8f8a 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(*[ - :admin_group, - :admin_namespace, - :admin_group_member - ]) + :admin_group, + :admin_namespace, + :admin_group_member + ]) end rules.flatten @@ -255,9 +256,9 @@ class Ability # Only namespace owner and administrators can admin it if namespace.owner == user || user.admin? rules.push(*[ - :create_projects, - :admin_namespace - ]) + :create_projects, + :admin_namespace + ]) end rules.flatten @@ -318,12 +319,29 @@ 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 && target_user != project.owner + rules << :update_project_member + rules << :destroy_project_member + end + + if user == target_user && target_user != project.owner + rules << :destroy_project_member + end + rules + end + 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/concerns/has_owners.rb b/app/models/concerns/has_owners.rb new file mode 100644 index 00000000000..53ef6e939dd --- /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 ||= 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 793a3b5ef2e..11fde7ba6cd 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -20,8 +20,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 :members, :group_members has_many :users, through: :group_members validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } @@ -64,10 +66,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) @@ -94,26 +92,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 - 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..eed9f2537e9 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -30,13 +30,20 @@ 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 +80,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 +89,19 @@ 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) + !current_user || current_user.can?(:update_group_member, member) || + current_user.can?(:update_project_member, member) end end @@ -95,7 +111,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..09465775786 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -42,6 +42,7 @@ class Project < ActiveRecord::Base include Sortable include AfterCommitQueue include CaseSensitivity + include HasOwners extend Gitlab::ConfigHelper extend Enumerize @@ -116,6 +117,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 diff --git a/features/groups.feature b/features/groups.feature index 615eff6a330..abf3769a844 100644 --- a/features/groups.feature +++ b/features/groups.feature @@ -60,6 +60,14 @@ Feature: Groups Then I should see "Mike" in team list as "Reporter" @javascript + Scenario: Ignore add user to group when is already Owner + Given gitlab user "Mike" + When I visit group "Owned" members page + And I click link "Add members" + When I select "Mike" as "Reporter" + Then I should see "Mike" in team list as "Owner" + + @javascript Scenario: Invite user to group When I visit group "Owned" members page And I click link "Add members" diff --git a/features/steps/groups.rb b/features/steps/groups.rb index a8fba2406ae..9c0313537b1 100644 --- a/features/steps/groups.rb +++ b/features/steps/groups.rb @@ -48,6 +48,17 @@ class Spinach::Features::Groups < Spinach::FeatureSteps click_button "Add users to group" end + step 'I select "Mike" as "Master"' do + user = User.find_by(name: "Mike") + + page.within ".users-group-form" do + select2(user.id, from: "#user_ids", multiple: true) + select "Master", from: "access_level" + end + + click_button "Add users to group" + end + step 'I should see "Mike" in team list as "Reporter"' do page.within '.well-list' do expect(page).to have_content('Mike') @@ -55,6 +66,13 @@ class Spinach::Features::Groups < Spinach::FeatureSteps end end + step 'I should see "Mike" in team list as "Owner"' do + page.within '.well-list' do + expect(page).to have_content('Mike') + expect(page).to have_content('Owner') + end + end + step 'I select "sjobs@apple.com" as "Reporter"' do page.within ".users-group-form" do select2("sjobs@apple.com", from: "#user_ids", multiple: true) |