From de990aa15829d0ab182ad5a55b4c527846c0d39c Mon Sep 17 00:00:00 2001
From: James Lopez <james.lopez@vodafone.com>
Date: Thu, 29 Oct 2015 16:10:27 +0000
Subject: fixed last group owner issue and added test

---
 app/models/member.rb     |  9 +++++----
 features/groups.feature  |  8 ++++++++
 features/steps/groups.rb | 18 ++++++++++++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/app/models/member.rb b/app/models/member.rb
index cae8caa23fb..bc7e70178e1 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -81,11 +81,12 @@ class Member < ActiveRecord::Base
         member = members.build
         member.invite_email = user
       end
+      if !current_user || current_user.can?(:update_group_member, member)
+        member.created_by ||= current_user
+        member.access_level = access_level
 
-      member.created_by ||= current_user
-      member.access_level = access_level
-
-      member.save
+        member.save
+      end
     end
   end
 
diff --git a/features/groups.feature b/features/groups.feature
index db37fa3b375..7777c0298a4 100644
--- a/features/groups.feature
+++ b/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
diff --git a/features/steps/groups.rb b/features/steps/groups.rb
index 69ddfa42c06..5fd4dea5cd6 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)
-- 
cgit v1.2.1


From 6aa9c21ac0e3f4860f9021718900326ea0575151 Mon Sep 17 00:00:00 2001
From: James Lopez <james.lopez@vodafone.com>
Date: Fri, 30 Oct 2015 19:55:19 +0000
Subject: fix issue with adding members to project (spotted by test)

---
 app/models/member.rb | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/app/models/member.rb b/app/models/member.rb
index bc7e70178e1..4651c8fff37 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -73,7 +73,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)
@@ -81,13 +81,22 @@ class Member < ActiveRecord::Base
         member = members.build
         member.invite_email = user
       end
-      if !current_user || current_user.can?(:update_group_member, member)
+
+      project = members.first.respond_to?(:project)? members.first.project : nil
+      if can_update_member?(current_user, member, project)
         member.created_by ||= current_user
         member.access_level = access_level
 
         member.save
       end
     end
+
+    private
+
+    def can_update_member?(current_user, member, project)
+      !current_user || current_user.can?(:update_group_member, member) ||
+        (project && current_user.can?(:admin_project_member, project))
+    end
   end
 
   def invite?
-- 
cgit v1.2.1


From 1b14bc59570a625365fef232f8c57919f76b3e2a Mon Sep 17 00:00:00 2001
From: James Lopez <james.lopez@vodafone.com>
Date: Tue, 3 Nov 2015 11:11:56 +0000
Subject: refactored permissions and added update_project_member ability logic.
 Also refactored owner methods to a concern.

---
 app/models/ability.rb             | 18 ++++++++++++++++++
 app/models/concerns/has_owners.rb | 31 +++++++++++++++++++++++++++++++
 app/models/group.rb               | 22 ++--------------------
 app/models/member.rb              |  8 ++++----
 app/models/project.rb             |  2 ++
 5 files changed, 57 insertions(+), 24 deletions(-)
 create mode 100644 app/models/concerns/has_owners.rb

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
-- 
cgit v1.2.1


From c6a0f109cd393e951f4d700b06490d819e6792ba Mon Sep 17 00:00:00 2001
From: James Lopez <james.lopez@vodafone.com>
Date: Wed, 11 Nov 2015 15:42:27 +0000
Subject: refactored code as projects only have one owner. Kept some
 refactoring in place (has_owners concern)

---
 app/models/ability.rb             | 52 +++++++++++++++++++--------------------
 app/models/concerns/has_owners.rb |  4 +--
 app/models/group.rb               |  6 +----
 app/models/member.rb              | 13 +++++-----
 4 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/app/models/ability.rb b/app/models/ability.rb
index 5beead0b75d..b46c4943bf5 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -6,17 +6,17 @@ class Ability
       return [] if user.blocked?
 
       case subject.class.name
-      when "Project" then project_abilities(user, subject)
-      when "Issue" then issue_abilities(user, subject)
-      when "Note" then note_abilities(user, subject)
-      when "ProjectSnippet" then project_snippet_abilities(user, subject)
-      when "PersonalSnippet" then personal_snippet_abilities(user, subject)
-      when "MergeRequest" then merge_request_abilities(user, subject)
-      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 []
+        when "Project" then project_abilities(user, subject)
+        when "Issue" then issue_abilities(user, subject)
+        when "Note" then note_abilities(user, subject)
+        when "ProjectSnippet" then project_snippet_abilities(user, subject)
+        when "PersonalSnippet" then personal_snippet_abilities(user, subject)
+        when "MergeRequest" then merge_request_abilities(user, subject)
+        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
 
@@ -232,17 +232,17 @@ 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(*[
-          :create_projects,
-        ])
+                     :create_projects,
+                   ])
       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
@@ -254,9 +254,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
@@ -323,12 +323,12 @@ class Ability
       project = subject.project
       can_manage = project_abilities(user, project).include?(:admin_project_member)
 
-      if can_manage && (user != target_user)
+      if can_manage && user != target_user && target_user != project.owner
         rules << :update_project_member
         rules << :destroy_project_member
       end
 
-      if !project.last_owner?(user) && (can_manage || (user == target_user))
+      if user == target_user && target_user != project.owner
         rules << :destroy_project_member
       end
       rules
@@ -336,10 +336,10 @@ class Ability
 
     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
index 735b2071721..07f3428b481 100644
--- a/app/models/concerns/has_owners.rb
+++ b/app/models/concerns/has_owners.rb
@@ -6,10 +6,10 @@ module HasOwners
   extend ActiveSupport::Concern
 
   def owners
-    @owners ||= my_members.owners.includes(:user).map(&:user)
+    @owners ||= members.owners.includes(:user).map(&:user)
   end
 
-  def my_members
+  def members
     raise NotImplementedError, "Expected my_members to be defined in #{self.class.name}"
   end
 
diff --git a/app/models/group.rb b/app/models/group.rb
index c9806f6fd6f..9503d8b0f1c 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -22,7 +22,7 @@ class Group < Namespace
   include HasOwners
 
   has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember'
-  alias_method :my_members, :group_members
+  alias_method :members, :group_members
   has_many :users, through: :group_members
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
@@ -91,10 +91,6 @@ class Group < Namespace
     add_user(user, Gitlab::Access::MASTER, current_user)
   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 c565ee6bbce..30160c813e1 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -30,13 +30,13 @@ 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") }
@@ -94,8 +94,7 @@ class Member < ActiveRecord::Base
 
     def can_update_member?(current_user, member)
       !current_user || current_user.can?(:update_group_member, member) ||
-        (member.respond_to?(:project) &&
-          current_user.can?(:update_project_member, member))
+        current_user.can?(:update_project_member, member)
     end
   end
 
@@ -105,7 +104,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
 
-- 
cgit v1.2.1


From fd1b5d6479ca05420dfd9f13ac6af0f53b5b858d Mon Sep 17 00:00:00 2001
From: James Lopez <james.lopez@vodafone.com>
Date: Wed, 11 Nov 2015 15:54:28 +0000
Subject: updated exception

---
 app/models/concerns/has_owners.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/concerns/has_owners.rb b/app/models/concerns/has_owners.rb
index 07f3428b481..53ef6e939dd 100644
--- a/app/models/concerns/has_owners.rb
+++ b/app/models/concerns/has_owners.rb
@@ -10,7 +10,7 @@ module HasOwners
   end
 
   def members
-    raise NotImplementedError, "Expected my_members to be defined in #{self.class.name}"
+    raise NotImplementedError, "Expected members to be defined in #{self.class.name}"
   end
 
   def add_owner(user, current_user = nil)
-- 
cgit v1.2.1


From 77dd561796adb94236422fb9da50482271fadbb1 Mon Sep 17 00:00:00 2001
From: James Lopez <james.lopez@vodafone.com>
Date: Thu, 12 Nov 2015 08:43:30 +0000
Subject: fixing rubocop indents

---
 app/models/ability.rb | 22 +++++++++++-----------
 app/models/member.rb  |  4 +++-
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/app/models/ability.rb b/app/models/ability.rb
index b46c4943bf5..6526cc6bc6a 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -6,17 +6,17 @@ class Ability
       return [] if user.blocked?
 
       case subject.class.name
-        when "Project" then project_abilities(user, subject)
-        when "Issue" then issue_abilities(user, subject)
-        when "Note" then note_abilities(user, subject)
-        when "ProjectSnippet" then project_snippet_abilities(user, subject)
-        when "PersonalSnippet" then personal_snippet_abilities(user, subject)
-        when "MergeRequest" then merge_request_abilities(user, subject)
-        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 []
+      when "Project" then project_abilities(user, subject)
+      when "Issue" then issue_abilities(user, subject)
+      when "Note" then note_abilities(user, subject)
+      when "ProjectSnippet" then project_snippet_abilities(user, subject)
+      when "PersonalSnippet" then personal_snippet_abilities(user, subject)
+      when "MergeRequest" then merge_request_abilities(user, subject)
+      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
 
diff --git a/app/models/member.rb b/app/models/member.rb
index 30160c813e1..ab5a8e6228d 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -36,7 +36,9 @@ class Member < ActiveRecord::Base
   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 }
+            uniqueness: { scope: [:source_type,
+                                  :source_id],
+                          allow_nil: true }
 
   scope :invite, -> { where(user_id: nil) }
   scope :non_invite, -> { where("user_id IS NOT NULL") }
-- 
cgit v1.2.1


From e11bfa6b86c5b1b838e7e438bcaa599df668f7be Mon Sep 17 00:00:00 2001
From: James Lopez <james.lopez@vodafone.com>
Date: Thu, 12 Nov 2015 08:44:00 +0000
Subject: fixing rubocop indents

---
 app/models/member.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/models/member.rb b/app/models/member.rb
index ab5a8e6228d..a7c599b3598 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -35,7 +35,8 @@ class Member < ActiveRecord::Base
                                     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 },
+            email: { strict_mode: true,
+                     allow_nil: true },
             uniqueness: { scope: [:source_type,
                                   :source_id],
                           allow_nil: true }
-- 
cgit v1.2.1


From 56ea71a3b19396b01b3b8495337fbf22c534524f Mon Sep 17 00:00:00 2001
From: James Lopez <james.lopez@vodafone.com>
Date: Thu, 12 Nov 2015 12:29:01 +0000
Subject: fixing rubocop - random code not related to the changes

---
 app/models/member.rb | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/app/models/member.rb b/app/models/member.rb
index a7c599b3598..eed9f2537e9 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -35,11 +35,15 @@ class Member < ActiveRecord::Base
                                     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 }
+            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") }
-- 
cgit v1.2.1


From ecb83afabcb69d80995b56323bb89b1ee1176225 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
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.1


From e3fe3da63d23981f5a0f3bd629046cbe0533a132 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Tue, 17 Nov 2015 15:51:40 +0100
Subject: Use project member abilities more extensively

---
 app/controllers/groups/group_members_controller.rb | 30 +++++++++----------
 .../projects/project_members_controller.rb         | 34 +++++++++++++---------
 .../groups/group_members/_group_member.html.haml   |  6 ++--
 app/views/groups/group_members/index.html.haml     |  6 ++--
 .../project_members/_project_member.html.haml      | 11 +++----
 app/views/projects/project_members/_team.html.haml |  4 +--
 app/views/projects/project_members/update.js.haml  |  3 +-
 7 files changed, 49 insertions(+), 45 deletions(-)

diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index b25957a06e2..0e902c4bb43 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/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)
diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index 9de5269cd25..07eb94e4f48 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/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
 
diff --git a/app/views/groups/group_members/_group_member.html.haml b/app/views/groups/group_members/_group_member.html.haml
index 3c19381321a..be94b1abc11 100644
--- a/app/views/groups/group_members/_group_member.html.haml
+++ b/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)
           &nbsp;
           - if current_user == user
diff --git a/app/views/groups/group_members/index.html.haml b/app/views/groups/group_members/index.html.haml
index 15d289471c9..d4ad33a8bf1 100644
--- a/app/views/groups/group_members/index.html.haml
+++ b/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'
 
diff --git a/app/views/projects/project_members/_project_member.html.haml b/app/views/projects/project_members/_project_member.html.haml
index 76c46d1d806..f07cd97e63d 100644
--- a/app/views/projects/project_members/_project_member.html.haml
+++ b/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)
         &nbsp;
         - 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
diff --git a/app/views/projects/project_members/_team.html.haml b/app/views/projects/project_members/_team.html.haml
index 615c425e59a..b807fb2cc9d 100644
--- a/app/views/projects/project_members/_team.html.haml
+++ b/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
diff --git a/app/views/projects/project_members/update.js.haml b/app/views/projects/project_members/update.js.haml
index 811b1858821..2fb3a41d541 100644
--- a/app/views/projects/project_members/update.js.haml
+++ b/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))}');
-- 
cgit v1.2.1


From d60a23b718abc365651f146de678f20367ef25b1 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Tue, 17 Nov 2015 15:53:53 +0100
Subject: Add changelog item

---
 CHANGELOG | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG b/CHANGELOG
index 3c22df7c9a3..f7c6b09e7b2 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -45,6 +45,7 @@ v 8.2.0 (unreleased)
   - 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)
-- 
cgit v1.2.1