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

CHANGELOG

@@ -47,6 +47,7 @@ v 8.2.0
   - Fix trailing whitespace issue in merge request/issue title
   - Fix bug when milestone/label filter was empty for dashboard issues page
   - Add ability to create milestone in group projects from single form
+  - Prevent the last owner of a group from being able to delete themselves by 'adding' themselves as a master (James Lopez)
 
 v 8.1.4
   - Fix bug where manually merged branches in a MR would end up with an empty diff (Stan Hu)

app/controllers/groups/group_members_controller.rb

@@ -3,8 +3,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
   # Authorize
   before_action :authorize_read_group!
-  before_action :authorize_admin_group!, except: [:index, :leave]
-  before_action :authorize_admin_group_member!, only: [:create, :resend_invite]
+  before_action :authorize_admin_group_member!, except: [:index, :leave]
 
   def index
     @project = @group.projects.find(params[:project_id]) if params[:project_id]
@@ -17,7 +16,8 @@ class Groups::GroupMembersController < Groups::ApplicationController
     end
 
     @members = @members.order('access_level DESC').page(params[:page]).per(50)
-    @group_member = GroupMember.new
+
+    @group_member = @group.group_members.new
   end
 
   def create
@@ -27,24 +27,23 @@ class Groups::GroupMembersController < Groups::ApplicationController
   end
 
   def update
-    @member = @group.group_members.find(params[:id])
+    @group_member = @group.group_members.find(params[:id])
 
-    return render_403 unless can?(current_user, :update_group_member, @member)
+    return render_403 unless can?(current_user, :update_group_member, @group_member)
 
-    @member.update_attributes(member_params)
+    @group_member.update_attributes(member_params)
   end
 
   def destroy
     @group_member = @group.group_members.find(params[:id])
 
-    if can?(current_user, :destroy_group_member, @group_member)  # May fail if last owner.
-      @group_member.destroy
-      respond_to do |format|
-        format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' }
-        format.js { render nothing: true }
-      end
-    else
-      return render_403
+    return render_403 unless can?(current_user, :destroy_group_member, @group_member)
+
+    @group_member.destroy
+
+    respond_to do |format|
+      format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' }
+      format.js { render nothing: true }
     end
   end
 
@@ -63,10 +62,11 @@ class Groups::GroupMembersController < Groups::ApplicationController
   end
 
   def leave
-    @group_member = @group.group_members.where(user_id: current_user.id).first
+    @group_member = @group.group_members.find_by(user_id: current_user)
 
     if can?(current_user, :destroy_group_member, @group_member)
       @group_member.destroy
+
       redirect_to(dashboard_groups_path, notice: "You left #{group.name} group.")
     else
       if @group.last_owner?(current_user)

app/controllers/projects/project_members_controller.rb

@@ -1,6 +1,6 @@
 class Projects::ProjectMembersController < Projects::ApplicationController
   # Authorize
-  before_action :authorize_admin_project!, except: :leave
+  before_action :authorize_admin_project_member!, except: :leave
 
   def index
     @project_members = @project.project_members
@@ -29,10 +29,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController
     @project_member = @project.project_members.new
   end
 
-  def new
-    @project_member = @project.project_members.new
-  end
-
   def create
     @project.team.add_users(params[:user_ids].split(','), params[:access_level], current_user)
 
@@ -41,11 +37,17 @@ class Projects::ProjectMembersController < Projects::ApplicationController
 
   def update
     @project_member = @project.project_members.find(params[:id])
+
+    return render_403 unless can?(current_user, :update_project_member, @project_member)
+
     @project_member.update_attributes(member_params)
   end
 
   def destroy
     @project_member = @project.project_members.find(params[:id])
+
+    return render_403 unless can?(current_user, :destroy_project_member, @project_member)
+
     @project_member.destroy
 
     respond_to do |format|
@@ -71,16 +73,22 @@ class Projects::ProjectMembersController < Projects::ApplicationController
   end
 
   def leave
-    if @project.namespace == current_user.namespace
-      message = 'You can not leave your own project. Transfer or delete the project.'
-      return redirect_back_or_default(default: { action: 'index' }, options: { alert: message })
-    end
+    @project_member = @project.project_members.find_by(user_id: current_user)
 
-    @project.project_members.find_by(user_id: current_user).destroy
+    if can?(current_user, :destroy_project_member, @project_member)
+      @project_member.destroy
 
-    respond_to do |format|
-      format.html { redirect_to dashboard_projects_path }
-      format.js { render nothing: true }
+      respond_to do |format|
+        format.html { redirect_to dashboard_projects_path, notice: "You left the project." }
+        format.js { render nothing: true }
+      end
+    else
+      if current_user == @project.owner
+        message = 'You can not leave your own project. Transfer or delete the project.'
+        redirect_back_or_default(default: { action: 'index' }, options: { alert: message })
+      else
+        render_403
+      end
     end
   end
 

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

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"

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
+        member.save
+      end
+    end
+
+    private
+
+    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
 

app/models/project.rb

@@ -42,7 +42,7 @@ class Project < ActiveRecord::Base
   include Sortable
   include AfterCommitQueue
   include CaseSensitivity
-
+  
   extend Gitlab::ConfigHelper
   extend Enumerize
 

app/views/groups/group_members/_group_member.html.haml

@@ -1,6 +1,5 @@
 - user = member.user
 - return unless user || member.invite?
-- show_roles = true if show_roles.nil?
 
 %li{class: "#{dom_class(member)} js-toggle-container", id: dom_id(member)}
   %span{class: ("list-item-name" if show_controls)}
@@ -25,11 +24,11 @@
           = link_to member.created_by.name, user_path(member.created_by)
         = time_ago_with_tooltip(member.created_at)
 
-      - if show_controls && can?(current_user, :admin_group_member, member)
+      - if show_controls && can?(current_user, :admin_group_member, @group)
         = link_to resend_invite_group_group_member_path(@group, member), method: :post, class: "btn-xs btn", title: 'Resend invite' do
           Resend invite
 
-  - if show_roles
+  - if should_user_see_group_roles?(current_user, @group)
     %span.pull-right
       %strong= member.human_access
       - if show_controls
@@ -37,6 +36,7 @@
           = button_tag class: "btn-xs btn js-toggle-button",
                        title: 'Edit access level', type: 'button' do
             %i.fa.fa-pencil-square-o
+
         - if can?(current_user, :destroy_group_member, member)
            
           - if current_user == user

app/views/groups/group_members/index.html.haml

@@ -1,8 +1,6 @@
 - page_title "Members"
 - header_title group_title(@group, "Members", group_group_members_path(@group))
-- show_roles = should_user_see_group_roles?(current_user, @group)
-
-- if show_roles
+- if should_user_see_group_roles?(current_user, @group)
   %p.light
     Members of group have access to all group projects.
     Read more about permissions
@@ -32,7 +30,7 @@
       (#{@members.total_count})
   %ul.well-list
     - @members.each do |member|
-      = render 'groups/group_members/group_member', member: member, show_roles: show_roles, show_controls: true
+      = render 'groups/group_members/group_member', member: member, show_controls: true
 
 = paginate @members, theme: 'gitlab'
 

app/views/projects/project_members/_project_member.html.haml

@@ -24,18 +24,19 @@
           = link_to member.created_by.name, user_path(member.created_by)
         = time_ago_with_tooltip(member.created_at)
 
-      - if current_user_can_admin_project
+      - if can?(current_user, :admin_project_member, @project)
         = link_to resend_invite_namespace_project_project_member_path(@project.namespace, @project, member), method: :post, class: "btn-xs btn", title: 'Resend invite' do
           Resend invite
 
-  - if current_user_can_admin_project
-    - unless @project.personal? && user == current_user
-      .pull-right
-        %strong= member.human_access
+  - if can?(current_user, :admin_project_member, @project)
+    .pull-right
+      %strong= member.human_access
+      - if can?(current_user, :update_project_member, member)
         = button_tag class: "btn-xs btn js-toggle-button",
                      title: 'Edit access level', type: 'button' do
           %i.fa.fa-pencil-square-o
 
+      - if can?(current_user, :destroy_project_member, member)
          
         - if current_user == user
           = link_to leave_namespace_project_project_members_path(@project.namespace, @project), data: { confirm: leave_project_message(@project) }, method: :delete, class: "btn-xs btn btn-remove", title: 'Leave project' do

app/views/projects/project_members/_team.html.haml

@@ -1,5 +1,3 @@
-- can_admin_project = can?(current_user, :admin_project, @project)
-
 .panel.panel-default.prepend-top-20
   .panel-heading
     %strong #{@project.name}
@@ -8,4 +6,4 @@
       (#{members.count})
   %ul.well-list
     - members.each do |project_member|
-      = render 'project_member', member: project_member, current_user_can_admin_project: can_admin_project
+      = render 'project_member', member: project_member

app/views/projects/project_members/update.js.haml

@@ -1,3 +1,2 @@
-- can_admin_project = can?(current_user, :admin_project, @project)
 :plain
-  $("##{dom_id(@project_member)}").replaceWith('#{escape_javascript(render("project_member", member: @project_member, current_user_can_admin_project: can_admin_project))}');
+  $("##{dom_id(@project_member)}").replaceWith('#{escape_javascript(render("project_member", member: @project_member))}');

features/groups.feature

@@ -59,6 +59,14 @@ Feature: Groups
     When I select "Mike" as "Reporter"
     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

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)