From ec0061a95cbba02286b2c143048c93d8f26ff5f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 16 Sep 2016 17:54:21 +0200
Subject: Allow Member.add_user to handle access requesters
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Changes include:

- Ensure Member.add_user is not called directly when not necessary
- New GroupMember.add_users_to_group to have the same abstraction level as for Project
- Refactor Member.add_user to take a source instead of an array of members
- Fix Rubocop offenses
- Always use Project#add_user instead of project.team.add_user
- Factorize users addition as members in Member.add_users_to_source
- Make access_level a keyword argument in GroupMember.add_users_to_group and ProjectMember.add_users_to_projects
- Destroy any requester before adding them as a member
- Improve the way we handle access requesters in Member.add_user
  Instead of removing the requester and creating a new member,
  we now simply accepts their access request. This way, they will
  receive a "access request granted" email.
- Fix error that was previously silently ignored
- Stop raising when access level is invalid in Member, let Rails validation do their work

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/models/group.rb                                |  36 +--
 app/models/member.rb                               |  79 ++++---
 app/models/members/group_member.rb                 |  16 ++
 app/models/members/project_member.rb               |  44 ++--
 app/models/project.rb                              |   5 +-
 app/models/project_team.rb                         |  14 +-
 db/fixtures/development/06_teams.rb                |   2 +-
 lib/api/members.rb                                 |  17 +-
 lib/gitlab/access.rb                               |   4 +
 .../projects/templates_controller_spec.rb          |   2 +-
 spec/factories/project_members.rb                  |  23 +-
 .../members/owner_cannot_leave_project_spec.rb     |   4 +-
 ...er_cannot_request_access_to_his_project_spec.rb |   4 +-
 spec/features/projects_spec.rb                     |   6 +-
 spec/finders/joined_groups_finder_spec.rb          |   2 +-
 spec/finders/projects_finder_spec.rb               |   2 +-
 spec/lib/gitlab/template/issue_template_spec.rb    |   6 +-
 .../gitlab/template/merge_request_template_spec.rb |   6 +-
 spec/mailers/notify_spec.rb                        |  50 +++--
 spec/models/issue_spec.rb                          |   2 +-
 spec/models/member_spec.rb                         | 243 +++++++++++++++++----
 spec/models/members/group_member_spec.rb           |  27 +++
 spec/models/members/project_member_spec.rb         |  50 +++--
 spec/models/project_spec.rb                        |   2 +-
 spec/requests/api/merge_requests_spec.rb           |   4 +-
 25 files changed, 431 insertions(+), 219 deletions(-)

diff --git a/app/models/group.rb b/app/models/group.rb
index aefb94b2ada..a2f88cca828 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -102,40 +102,44 @@ class Group < Namespace
     self[:lfs_enabled]
   end
 
-  def add_users(user_ids, access_level, current_user: nil, expires_at: nil)
-    user_ids.each do |user_id|
-      Member.add_user(
-        self.group_members,
-        user_id,
-        access_level,
-        current_user: current_user,
-        expires_at: expires_at
-      )
-    end
+  def add_users(users, access_level, current_user: nil, expires_at: nil)
+    GroupMember.add_users_to_group(
+      self,
+      users,
+      access_level,
+      current_user: current_user,
+      expires_at: expires_at
+    )
   end
 
   def add_user(user, access_level, current_user: nil, expires_at: nil)
-    add_users([user], access_level, current_user: current_user, expires_at: expires_at)
+    GroupMember.add_user(
+      self,
+      user,
+      access_level,
+      current_user: current_user,
+      expires_at: expires_at
+    )
   end
 
   def add_guest(user, current_user = nil)
-    add_user(user, Gitlab::Access::GUEST, current_user: current_user)
+    add_user(user, :guest, current_user: current_user)
   end
 
   def add_reporter(user, current_user = nil)
-    add_user(user, Gitlab::Access::REPORTER, current_user: current_user)
+    add_user(user, :reporter, current_user: current_user)
   end
 
   def add_developer(user, current_user = nil)
-    add_user(user, Gitlab::Access::DEVELOPER, current_user: current_user)
+    add_user(user, :developer, current_user: current_user)
   end
 
   def add_master(user, current_user = nil)
-    add_user(user, Gitlab::Access::MASTER, current_user: current_user)
+    add_user(user, :master, current_user: current_user)
   end
 
   def add_owner(user, current_user = nil)
-    add_user(user, Gitlab::Access::OWNER, current_user: current_user)
+    add_user(user, :owner, current_user: current_user)
   end
 
   def has_owner?(user)
diff --git a/app/models/member.rb b/app/models/member.rb
index 69406379948..265c11ca113 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -80,49 +80,70 @@ class Member < ActiveRecord::Base
       find_by(invite_token: invite_token)
     end
 
-    # This method is used to find users that have been entered into the "Add members" field.
-    # These can be the User objects directly, their IDs, their emails, or new emails to be invited.
-    def user_for_id(user_id)
-      return user_id if user_id.is_a?(User)
-
-      user = User.find_by(id: user_id)
-      user ||= User.find_by(email: user_id)
-      user ||= user_id
-      user
-    end
-
-    def add_user(members, user_id, access_level, current_user: nil, expires_at: nil)
-      user = user_for_id(user_id)
+    def add_user(source, user, access_level, current_user: nil, expires_at: nil)
+      user = retrieve_user(user)
+      access_level = retrieve_access_level(access_level)
 
       # `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)
+      member =
+        if user.is_a?(User)
+          source.members.find_by(user_id: user.id) ||
+          source.requesters.find_by(user_id: user.id) ||
+          source.members.build(user_id: user.id)
+        else
+          source.members.build(invite_email: user)
+        end
+
+      return member unless can_update_member?(current_user, member)
+
+      member.attributes = {
+        created_by: member.created_by || current_user,
+        access_level: access_level,
+        expires_at: expires_at
+      }
+
+      if member.request?
+        member.accept_request
       else
-        member = members.build
-        member.invite_email = user
+        member.save
       end
 
-      if can_update_member?(current_user, member) || project_creator?(member, access_level)
-        member.created_by ||= current_user
-        member.access_level = access_level
-        member.expires_at = expires_at
+      member
+    end
 
-        member.save
-      end
+    def access_levels
+      Gitlab::Access.sym_options
     end
 
     private
 
+    # This method is used to find users that have been entered into the "Add members" field.
+    # These can be the User objects directly, their IDs, their emails, or new emails to be invited.
+    def retrieve_user(user)
+      return user if user.is_a?(User)
+
+      User.find_by(id: user) || User.find_by(email: user) || user
+    end
+
+    def retrieve_access_level(access_level)
+      access_levels.fetch(access_level) { access_level.to_i }
+    end
+
     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)
+      !current_user || current_user.can?(:"update_#{member.type.underscore}", member)
     end
 
-    def project_creator?(member, access_level)
-      member.new_record? && member.owner? &&
-        access_level.to_i == ProjectMember::MASTER
+    def add_users_to_source(source, users, access_level, current_user: nil, expires_at: nil)
+      users.each do |user|
+        add_user(
+          source,
+          user,
+          access_level,
+          current_user: current_user,
+          expires_at: expires_at
+        )
+      end
     end
   end
 
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 2f13d339c89..1b54a85d064 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -12,6 +12,22 @@ class GroupMember < Member
     Gitlab::Access.options_with_owner
   end
 
+  def self.access_levels
+    Gitlab::Access.sym_options_with_owner
+  end
+
+  def self.add_users_to_group(group, users, access_level, current_user: nil, expires_at: nil)
+    self.transaction do
+      add_users_to_source(
+        group,
+        users,
+        access_level,
+        current_user: current_user,
+        expires_at: expires_at
+      )
+    end
+  end
+
   def group
     source
   end
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index ec2d40eb11c..125f26369d7 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -34,36 +34,20 @@ class ProjectMember < Member
     #     :master
     #   )
     #
-    def add_users_to_projects(project_ids, user_ids, access, current_user: nil, expires_at: nil)
-      access_level = if roles_hash.has_key?(access)
-                       roles_hash[access]
-                     elsif roles_hash.values.include?(access.to_i)
-                       access
-                     else
-                       raise "Non valid access"
-                     end
-
-      users = user_ids.map { |user_id| Member.user_for_id(user_id) }
-
-      ProjectMember.transaction do
+    def add_users_to_projects(project_ids, users, access_level, current_user: nil, expires_at: nil)
+      self.transaction do
         project_ids.each do |project_id|
           project = Project.find(project_id)
 
-          users.each do |user|
-            Member.add_user(
-              project.project_members,
-              user,
-              access_level,
-              current_user: current_user,
-              expires_at: expires_at
-            )
-          end
+          add_users_to_source(
+            project,
+            users,
+            access_level,
+            current_user: current_user,
+            expires_at: expires_at
+          )
         end
       end
-
-      true
-    rescue
-      false
     end
 
     def truncate_teams(project_ids)
@@ -84,13 +68,15 @@ class ProjectMember < Member
       truncate_teams [project.id]
     end
 
-    def roles_hash
-      Gitlab::Access.sym_options
-    end
-
     def access_level_roles
       Gitlab::Access.options
     end
+
+    private
+
+    def can_update_member?(current_user, member)
+      super || (member.owner? && member.new_record?)
+    end
   end
 
   def access_field
diff --git a/app/models/project.rb b/app/models/project.rb
index 7265cb55594..507228606df 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -146,6 +146,7 @@ class Project < ActiveRecord::Base
 
   delegate :name, to: :owner, allow_nil: true, prefix: true
   delegate :members, to: :team, prefix: true
+  delegate :add_user, to: :team
 
   # Validations
   validates :creator, presence: true, on: :create
@@ -1016,10 +1017,6 @@ class Project < ActiveRecord::Base
     project_members.find_by(user_id: user)
   end
 
-  def add_user(user, access_level, current_user: nil, expires_at: nil)
-    team.add_user(user, access_level, current_user: current_user, expires_at: expires_at)
-  end
-
   def default_branch
     @default_branch ||= repository.root_ref if repository.exists?
   end
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index d9ce5088903..79d041d2775 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -33,18 +33,24 @@ class ProjectTeam
     member
   end
 
-  def add_users(users, access, current_user: nil, expires_at: nil)
+  def add_users(users, access_level, current_user: nil, expires_at: nil)
     ProjectMember.add_users_to_projects(
       [project.id],
       users,
-      access,
+      access_level,
       current_user: current_user,
       expires_at: expires_at
     )
   end
 
-  def add_user(user, access, current_user: nil, expires_at: nil)
-    add_users([user], access, current_user: current_user, expires_at: expires_at)
+  def add_user(user, access_level, current_user: nil, expires_at: nil)
+    ProjectMember.add_user(
+      project,
+      user,
+      access_level,
+      current_user: current_user,
+      expires_at: expires_at
+    )
   end
 
   # Remove all users from project team
diff --git a/db/fixtures/development/06_teams.rb b/db/fixtures/development/06_teams.rb
index 3e8cdcd67b4..9739a5ac8d5 100644
--- a/db/fixtures/development/06_teams.rb
+++ b/db/fixtures/development/06_teams.rb
@@ -1,7 +1,7 @@
 Gitlab::Seeder.quiet do
   Group.all.each do |group|
     User.all.sample(4).each do |user|
-      if group.add_users([user.id], Gitlab::Access.values.sample)
+      if group.add_user(user, Gitlab::Access.values.sample).persisted?
         print '.'
       else
         print 'F'
diff --git a/lib/api/members.rb b/lib/api/members.rb
index 37f0a6512f4..a18ce769e29 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -59,13 +59,6 @@ module API
           authorize_admin_source!(source_type, source)
           required_attributes! [:user_id, :access_level]
 
-          access_requester = source.requesters.find_by(user_id: params[:user_id])
-          if access_requester
-            # We pass current_user = access_requester so that the requester doesn't
-            # receive a "access denied" email
-            ::Members::DestroyService.new(access_requester, access_requester.user).execute
-          end
-
           member = source.members.find_by(user_id: params[:user_id])
 
           # This is to ensure back-compatibility but 409 behavior should be used
@@ -73,18 +66,12 @@ module API
           conflict!('Member already exists') if source_type == 'group' && member
 
           unless member
-            source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
-            member = source.members.find_by(user_id: params[:user_id])
+            member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
           end
 
-          if member
+          if member.persisted? && member.valid?
             present member.user, with: Entities::Member, member: member
           else
-            # Since `source.add_user` doesn't return a member object, we have to
-            # build a new one and populate its errors in order to render them.
-            member = source.members.build(attributes_for_keys([:user_id, :access_level, :expires_at]))
-            member.valid? # populate the errors
-
             # This is to ensure back-compatibility but 400 behavior should be used
             # for all validation errors in 9.0!
             render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level)
diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb
index a533bac2692..9b484a2ecfd 100644
--- a/lib/gitlab/access.rb
+++ b/lib/gitlab/access.rb
@@ -53,6 +53,10 @@ module Gitlab
         }
       end
 
+      def sym_options_with_owner
+        sym_options.merge(owner: OWNER)
+      end
+
       def protection_options
         {
           "Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE,
diff --git a/spec/controllers/projects/templates_controller_spec.rb b/spec/controllers/projects/templates_controller_spec.rb
index 7b3a26d7ca7..19a152bcb05 100644
--- a/spec/controllers/projects/templates_controller_spec.rb
+++ b/spec/controllers/projects/templates_controller_spec.rb
@@ -13,7 +13,7 @@ describe Projects::TemplatesController do
   end
 
   before do
-    project.team.add_user(user, Gitlab::Access::MASTER)
+    project.add_user(user, Gitlab::Access::MASTER)
     project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
   end
 
diff --git a/spec/factories/project_members.rb b/spec/factories/project_members.rb
index cf3659ba275..1ddb305a8af 100644
--- a/spec/factories/project_members.rb
+++ b/spec/factories/project_members.rb
@@ -4,24 +4,9 @@ FactoryGirl.define do
     project
     master
 
-    trait :guest do
-      access_level ProjectMember::GUEST
-    end
-
-    trait :reporter do
-      access_level ProjectMember::REPORTER
-    end
-
-    trait :developer do
-      access_level ProjectMember::DEVELOPER
-    end
-
-    trait :master do
-      access_level ProjectMember::MASTER
-    end
-
-    trait :owner do
-      access_level ProjectMember::OWNER
-    end
+    trait(:guest)     { access_level ProjectMember::GUEST }
+    trait(:reporter)  { access_level ProjectMember::REPORTER }
+    trait(:developer) { access_level ProjectMember::DEVELOPER }
+    trait(:master)    { access_level ProjectMember::MASTER }
   end
 end
diff --git a/spec/features/projects/members/owner_cannot_leave_project_spec.rb b/spec/features/projects/members/owner_cannot_leave_project_spec.rb
index 67811b1048e..6e948b7a616 100644
--- a/spec/features/projects/members/owner_cannot_leave_project_spec.rb
+++ b/spec/features/projects/members/owner_cannot_leave_project_spec.rb
@@ -1,12 +1,10 @@
 require 'spec_helper'
 
 feature 'Projects > Members > Owner cannot leave project', feature: true do
-  let(:owner) { create(:user) }
   let(:project) { create(:project) }
 
   background do
-    project.team << [owner, :owner]
-    login_as(owner)
+    login_as(project.owner)
     visit namespace_project_path(project.namespace, project)
   end
 
diff --git a/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb b/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb
index 0e54c4fdf20..4ca9272b9c1 100644
--- a/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb
+++ b/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb
@@ -1,12 +1,10 @@
 require 'spec_helper'
 
 feature 'Projects > Members > Owner cannot request access to his project', feature: true do
-  let(:owner) { create(:user) }
   let(:project) { create(:project) }
 
   background do
-    project.team << [owner, :owner]
-    login_as(owner)
+    login_as(project.owner)
     visit namespace_project_path(project.namespace, project)
   end
 
diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb
index 2242cb6236a..c30d38b6508 100644
--- a/spec/features/projects_spec.rb
+++ b/spec/features/projects_spec.rb
@@ -82,7 +82,7 @@ feature 'Project', feature: true do
 
     before do
       login_with(user)
-      project.team.add_user(user, Gitlab::Access::MASTER)
+      project.add_user(user, Gitlab::Access::MASTER)
       visit namespace_project_path(project.namespace, project)
     end
 
@@ -101,8 +101,8 @@ feature 'Project', feature: true do
     context 'on issues page', js: true do
       before do
         login_with(user)
-        project.team.add_user(user, Gitlab::Access::MASTER)
-        project2.team.add_user(user, Gitlab::Access::MASTER)
+        project.add_user(user, Gitlab::Access::MASTER)
+        project2.add_user(user, Gitlab::Access::MASTER)
         visit namespace_project_issue_path(project.namespace, project, issue)
       end
 
diff --git a/spec/finders/joined_groups_finder_spec.rb b/spec/finders/joined_groups_finder_spec.rb
index f90a8e007c8..29a47e005a6 100644
--- a/spec/finders/joined_groups_finder_spec.rb
+++ b/spec/finders/joined_groups_finder_spec.rb
@@ -43,7 +43,7 @@ describe JoinedGroupsFinder do
       context 'if profile visitor is in one of the private group projects' do
         before do
           project = create(:project, :private, group: private_group, name: 'B', path: 'B')
-          project.team.add_user(profile_visitor, Gitlab::Access::DEVELOPER)
+          project.add_user(profile_visitor, Gitlab::Access::DEVELOPER)
         end
 
         it 'shows group' do
diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb
index 7a3a74335e8..13bda5f7c5a 100644
--- a/spec/finders/projects_finder_spec.rb
+++ b/spec/finders/projects_finder_spec.rb
@@ -38,7 +38,7 @@ describe ProjectsFinder do
 
       describe 'with private projects' do
         before do
-          private_project.team.add_user(user, Gitlab::Access::MASTER)
+          private_project.add_user(user, Gitlab::Access::MASTER)
         end
 
         it do
diff --git a/spec/lib/gitlab/template/issue_template_spec.rb b/spec/lib/gitlab/template/issue_template_spec.rb
index f770857e958..d2d334e6413 100644
--- a/spec/lib/gitlab/template/issue_template_spec.rb
+++ b/spec/lib/gitlab/template/issue_template_spec.rb
@@ -10,7 +10,7 @@ describe Gitlab::Template::IssueTemplate do
   let(:file_path_3) { '.gitlab/issue_templates/feature_proposal.md' }
 
   before do
-    project.team.add_user(user, Gitlab::Access::MASTER)
+    project.add_user(user, Gitlab::Access::MASTER)
     project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
     project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false)
     project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false)
@@ -53,7 +53,7 @@ describe Gitlab::Template::IssueTemplate do
 
     context 'when repo is bare or empty' do
       let(:empty_project) { create(:empty_project) }
-      before { empty_project.team.add_user(user, Gitlab::Access::MASTER) }
+      before { empty_project.add_user(user, Gitlab::Access::MASTER) }
 
       it "returns empty array" do
         templates = subject.by_category('', empty_project)
@@ -78,7 +78,7 @@ describe Gitlab::Template::IssueTemplate do
     context "when repo is empty" do
       let(:empty_project) { create(:empty_project) }
 
-      before { empty_project.team.add_user(user, Gitlab::Access::MASTER) }
+      before { empty_project.add_user(user, Gitlab::Access::MASTER) }
 
       it "raises file not found" do
         issue_template = subject.new('.gitlab/issue_templates/not_existent.md', empty_project)
diff --git a/spec/lib/gitlab/template/merge_request_template_spec.rb b/spec/lib/gitlab/template/merge_request_template_spec.rb
index bb0f68043fa..ddf68c4cf78 100644
--- a/spec/lib/gitlab/template/merge_request_template_spec.rb
+++ b/spec/lib/gitlab/template/merge_request_template_spec.rb
@@ -10,7 +10,7 @@ describe Gitlab::Template::MergeRequestTemplate do
   let(:file_path_3) { '.gitlab/merge_request_templates/feature_proposal.md' }
 
   before do
-    project.team.add_user(user, Gitlab::Access::MASTER)
+    project.add_user(user, Gitlab::Access::MASTER)
     project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
     project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false)
     project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false)
@@ -53,7 +53,7 @@ describe Gitlab::Template::MergeRequestTemplate do
 
     context 'when repo is bare or empty' do
       let(:empty_project) { create(:empty_project) }
-      before { empty_project.team.add_user(user, Gitlab::Access::MASTER) }
+      before { empty_project.add_user(user, Gitlab::Access::MASTER) }
 
       it "returns empty array" do
         templates = subject.by_category('', empty_project)
@@ -78,7 +78,7 @@ describe Gitlab::Template::MergeRequestTemplate do
     context "when repo is empty" do
       let(:empty_project) { create(:empty_project) }
 
-      before { empty_project.team.add_user(user, Gitlab::Access::MASTER) }
+      before { empty_project.add_user(user, Gitlab::Access::MASTER) }
 
       it "raises file not found" do
         issue_template = subject.new('.gitlab/merge_request_templates/not_existent.md', empty_project)
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 0363bc74939..0f69119e82e 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -492,21 +492,22 @@ describe Notify do
       end
     end
 
-    def invite_to_project(project:, email:, inviter:)
-      Member.add_user(
-        project.project_members,
-        'toto@example.com',
-        Gitlab::Access::DEVELOPER,
-        current_user: inviter
+    def invite_to_project(project, inviter:)
+      create(
+        :project_member,
+        :developer,
+        project: project,
+        invite_token: '1234',
+        invite_email: 'toto@example.com',
+        user: nil,
+        created_by: inviter
       )
-
-      project.project_members.invite.last
     end
 
     describe 'project invitation' do
       let(:project) { create(:project) }
       let(:master) { create(:user).tap { |u| project.team << [u, :master] } }
-      let(:project_member) { invite_to_project(project: project, email: 'toto@example.com', inviter: master) }
+      let(:project_member) { invite_to_project(project, inviter: master) }
 
       subject { Notify.member_invited_email('project', project_member.id, project_member.invite_token) }
 
@@ -525,10 +526,10 @@ describe Notify do
 
     describe 'project invitation accepted' do
       let(:project) { create(:project) }
-      let(:invited_user) { create(:user) }
+      let(:invited_user) { create(:user, name: 'invited user') }
       let(:master) { create(:user).tap { |u| project.team << [u, :master] } }
       let(:project_member) do
-        invitee = invite_to_project(project: project, email: 'toto@example.com', inviter: master)
+        invitee = invite_to_project(project, inviter: master)
         invitee.accept_invite!(invited_user)
         invitee
       end
@@ -552,7 +553,7 @@ describe Notify do
       let(:project) { create(:project) }
       let(:master) { create(:user).tap { |u| project.team << [u, :master] } }
       let(:project_member) do
-        invitee = invite_to_project(project: project, email: 'toto@example.com', inviter: master)
+        invitee = invite_to_project(project, inviter: master)
         invitee.decline_invite!
         invitee
       end
@@ -744,21 +745,22 @@ describe Notify do
       end
     end
 
-    def invite_to_group(group:, email:, inviter:)
-      Member.add_user(
-        group.group_members,
-        'toto@example.com',
-        Gitlab::Access::DEVELOPER,
-        current_user: inviter
+    def invite_to_group(group, inviter:)
+      create(
+        :group_member,
+        :developer,
+        group: group,
+        invite_token: '1234',
+        invite_email: 'toto@example.com',
+        user: nil,
+        created_by: inviter
       )
-
-      group.group_members.invite.last
     end
 
     describe 'group invitation' do
       let(:group) { create(:group) }
       let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } }
-      let(:group_member) { invite_to_group(group: group, email: 'toto@example.com', inviter: owner) }
+      let(:group_member) { invite_to_group(group, inviter: owner) }
 
       subject { Notify.member_invited_email('group', group_member.id, group_member.invite_token) }
 
@@ -777,10 +779,10 @@ describe Notify do
 
     describe 'group invitation accepted' do
       let(:group) { create(:group) }
-      let(:invited_user) { create(:user) }
+      let(:invited_user) { create(:user, name: 'invited user') }
       let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } }
       let(:group_member) do
-        invitee = invite_to_group(group: group, email: 'toto@example.com', inviter: owner)
+        invitee = invite_to_group(group, inviter: owner)
         invitee.accept_invite!(invited_user)
         invitee
       end
@@ -804,7 +806,7 @@ describe Notify do
       let(:group) { create(:group) }
       let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } }
       let(:group_member) do
-        invitee = invite_to_group(group: group, email: 'toto@example.com', inviter: owner)
+        invitee = invite_to_group(group, inviter: owner)
         invitee.decline_invite!
         invitee
       end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 3259f795296..3b8b743af2d 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -494,7 +494,7 @@ describe Issue, models: true do
 
     context 'with an admin user' do
       let(:project) { create(:empty_project) }
-      let(:user) { create(:user, admin: true) }
+      let(:user) { create(:admin) }
 
       it 'returns true for a regular issue' do
         issue = build(:issue, project: project)
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index 0b1634f654a..a9d3fcc6587 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -74,22 +74,17 @@ describe Member, models: true do
       @blocked_master = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::MASTER)
       @blocked_developer = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::DEVELOPER)
 
-      Member.add_user(
-        project.members,
-        'toto1@example.com',
-        Gitlab::Access::DEVELOPER,
-        current_user: @master_user
-      )
-      @invited_member = project.members.invite.find_by_invite_email('toto1@example.com')
+      @invited_member = create(:project_member, :developer,
+                              project: project,
+                              invite_token: '1234',
+                              invite_email: 'toto1@example.com')
 
       accepted_invite_user = build(:user, state: :active)
-      Member.add_user(
-        project.members,
-        'toto2@example.com',
-        Gitlab::Access::DEVELOPER,
-        current_user: @master_user
-      )
-      @accepted_invite_member = project.members.invite.find_by_invite_email('toto2@example.com').tap { |u| u.accept_invite!(accepted_invite_user) }
+      @accepted_invite_member = create(:project_member, :developer,
+                                      project: project,
+                                      invite_token: '1234',
+                                      invite_email: 'toto2@example.com').
+                                      tap { |u| u.accept_invite!(accepted_invite_user) }
 
       requested_user = create(:user).tap { |u| project.request_access(u) }
       @requested_member = project.requesters.find_by(user_id: requested_user.id)
@@ -176,39 +171,209 @@ describe Member, models: true do
     it { is_expected.to respond_to(:user_email) }
   end
 
-  describe ".add_user" do
-    let!(:user)    { create(:user) }
-    let(:project) { create(:project) }
+  describe '.add_user' do
+    %w[project group].each do |source_type|
+      context "when source is a #{source_type}" do
+        let!(:source) { create(source_type) }
+        let!(:user) { create(:user) }
+        let!(:admin) { create(:admin) }
 
-    context "when called with a user id" do
-      it "adds the user as a member" do
-        Member.add_user(project.project_members, user.id, ProjectMember::MASTER)
+        it 'returns a <Source>Member object' do
+          member = described_class.add_user(source, user, :master)
 
-        expect(project.users).to include(user)
-      end
-    end
+          expect(member).to be_a "#{source_type.classify}Member".constantize
+          expect(member).to be_persisted
+        end
 
-    context "when called with a user object" do
-      it "adds the user as a member" do
-        Member.add_user(project.project_members, user, ProjectMember::MASTER)
+        it 'sets members.created_by to the given current_user' do
+          member = described_class.add_user(source, user, :master, current_user: admin)
 
-        expect(project.users).to include(user)
-      end
-    end
+          expect(member.created_by).to eq(admin)
+        end
 
-    context "when called with a known user email" do
-      it "adds the user as a member" do
-        Member.add_user(project.project_members, user.email, ProjectMember::MASTER)
+        it 'sets members.expires_at to the given expires_at' do
+          member = described_class.add_user(source, user, :master, expires_at: Date.new(2016, 9, 22))
 
-        expect(project.users).to include(user)
-      end
-    end
+          expect(member.expires_at).to eq(Date.new(2016, 9, 22))
+        end
+
+        described_class.access_levels.each do |sym_key, int_access_level|
+          it "accepts the :#{sym_key} symbol as access level" do
+            expect(source.users).not_to include(user)
+
+            member = described_class.add_user(source, user.id, sym_key)
+
+            expect(member.access_level).to eq(int_access_level)
+            expect(source.users.reload).to include(user)
+          end
+
+          it "accepts the #{int_access_level} integer as access level" do
+            expect(source.users).not_to include(user)
+
+            member = described_class.add_user(source, user.id, int_access_level)
+
+            expect(member.access_level).to eq(int_access_level)
+            expect(source.users.reload).to include(user)
+          end
+        end
+
+        context 'with no current_user' do
+          context 'when called with a known user id' do
+            it 'adds the user as a member' do
+              expect(source.users).not_to include(user)
+
+              described_class.add_user(source, user.id, :master)
+
+              expect(source.users.reload).to include(user)
+            end
+          end
+
+          context 'when called with an unknown user id' do
+            it 'adds the user as a member' do
+              expect(source.users).not_to include(user)
+
+              described_class.add_user(source, 42, :master)
+
+              expect(source.users.reload).not_to include(user)
+            end
+          end
+
+          context 'when called with a user object' do
+            it 'adds the user as a member' do
+              expect(source.users).not_to include(user)
+
+              described_class.add_user(source, user, :master)
+
+              expect(source.users.reload).to include(user)
+            end
+          end
+
+          context 'when called with a requester user object' do
+            before do
+              source.request_access(user)
+            end
+
+            it 'adds the requester as a member' do
+              expect(source.users).not_to include(user)
+              expect(source.requesters.exists?(user_id: user)).to be_truthy
+
+              expect { described_class.add_user(source, user, :master) }.
+                to raise_error(Gitlab::Access::AccessDeniedError)
+
+              expect(source.users.reload).not_to include(user)
+              expect(source.requesters.reload.exists?(user_id: user)).to be_truthy
+            end
+          end
+
+          context 'when called with a known user email' do
+            it 'adds the user as a member' do
+              expect(source.users).not_to include(user)
+
+              described_class.add_user(source, user.email, :master)
+
+              expect(source.users.reload).to include(user)
+            end
+          end
+
+          context 'when called with an unknown user email' do
+            it 'creates an invited member' do
+              expect(source.users).not_to include(user)
+
+              described_class.add_user(source, 'user@example.com', :master)
+
+              expect(source.members.invite.pluck(:invite_email)).to include('user@example.com')
+            end
+          end
+        end
+
+        context 'when current_user can update member' do
+          it 'creates the member' do
+            expect(source.users).not_to include(user)
+
+            described_class.add_user(source, user, :master, current_user: admin)
+
+            expect(source.users.reload).to include(user)
+          end
+
+          context 'when called with a requester user object' do
+            before do
+              source.request_access(user)
+            end
+
+            it 'adds the requester as a member' do
+              expect(source.users).not_to include(user)
+              expect(source.requesters.exists?(user_id: user)).to be_truthy
+
+              described_class.add_user(source, user, :master, current_user: admin)
+
+              expect(source.users.reload).to include(user)
+              expect(source.requesters.reload.exists?(user_id: user)).to be_falsy
+            end
+          end
+        end
+
+        context 'when current_user cannot update member' do
+          it 'does not create the member' do
+            expect(source.users).not_to include(user)
+
+            member = described_class.add_user(source, user, :master, current_user: user)
+
+            expect(source.users.reload).not_to include(user)
+            expect(member).not_to be_persisted
+          end
+
+          context 'when called with a requester user object' do
+            before do
+              source.request_access(user)
+            end
+
+            it 'does not destroy the requester' do
+              expect(source.users).not_to include(user)
+              expect(source.requesters.exists?(user_id: user)).to be_truthy
+
+              described_class.add_user(source, user, :master, current_user: user)
+
+              expect(source.users.reload).not_to include(user)
+              expect(source.requesters.exists?(user_id: user)).to be_truthy
+            end
+          end
+        end
+
+        context 'when member already exists' do
+          before do
+            source.add_user(user, :developer)
+          end
+
+          context 'with no current_user' do
+            it 'updates the member' do
+              expect(source.users).to include(user)
+
+              described_class.add_user(source, user, :master)
+
+              expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MASTER)
+            end
+          end
+
+          context 'when current_user can update member' do
+            it 'updates the member' do
+              expect(source.users).to include(user)
+
+              described_class.add_user(source, user, :master, current_user: admin)
+
+              expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MASTER)
+            end
+          end
+
+          context 'when current_user cannot update member' do
+            it 'does not update the member' do
+              expect(source.users).to include(user)
 
-    context "when called with an unknown user email" do
-      it "adds a member invite" do
-        Member.add_user(project.project_members, "user@example.com", ProjectMember::MASTER)
+              described_class.add_user(source, user, :master, current_user: user)
 
-        expect(project.project_members.invite.pluck(:invite_email)).to include("user@example.com")
+              expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::DEVELOPER)
+            end
+          end
+        end
       end
     end
   end
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index 56fa7fa6134..370aeb9e0a9 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -1,6 +1,33 @@
 require 'spec_helper'
 
 describe GroupMember, models: true do
+  describe '.access_level_roles' do
+    it 'returns Gitlab::Access.options_with_owner' do
+      expect(described_class.access_level_roles).to eq(Gitlab::Access.options_with_owner)
+    end
+  end
+
+  describe '.access_levels' do
+    it 'returns Gitlab::Access.options_with_owner' do
+      expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner)
+    end
+  end
+
+  describe '.add_users_to_group' do
+    it 'adds the given users to the given group' do
+      group = create(:group)
+      users = create_list(:user, 2)
+
+      described_class.add_users_to_group(
+        group,
+        [users.first.id, users.second],
+        described_class::MASTER
+      )
+
+      expect(group.users).to include(users.first, users.second)
+    end
+  end
+
   describe 'notifications' do
     describe "#after_create" do
       it "sends email to user" do
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index 805c15a4e5e..d85a1c1e3b2 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -15,6 +15,26 @@ describe ProjectMember, models: true do
     it { is_expected.to include_module(Gitlab::ShellAdapter) }
   end
 
+  describe '.access_level_roles' do
+    it 'returns Gitlab::Access.options' do
+      expect(described_class.access_level_roles).to eq(Gitlab::Access.options)
+    end
+  end
+
+  describe '.add_user' do
+    context 'when called with the project owner' do
+      it 'adds the user as a member' do
+        project = create(:empty_project)
+
+        expect(project.users).not_to include(project.owner)
+
+        described_class.add_user(project, project.owner, :master, current_user: project.owner)
+
+        expect(project.users.reload).to include(project.owner)
+      end
+    end
+  end
+
   describe '#real_source_type' do
     subject { create(:project_member).real_source_type }
 
@@ -50,7 +70,7 @@ describe ProjectMember, models: true do
     end
   end
 
-  describe :import_team do
+  describe '.import_team' do
     before do
       @project_1 = create :project
       @project_2 = create :project
@@ -81,25 +101,21 @@ describe ProjectMember, models: true do
   end
 
   describe '.add_users_to_projects' do
-    before do
-      @project_1 = create :project
-      @project_2 = create :project
+    it 'adds the given users to the given projects' do
+      projects = create_list(:empty_project, 2)
+      users = create_list(:user, 2)
 
-      @user_1 = create :user
-      @user_2 = create :user
-
-      ProjectMember.add_users_to_projects(
-        [@project_1.id, @project_2.id],
-        [@user_1.id, @user_2.id],
-        ProjectMember::MASTER
-      )
-    end
+      described_class.add_users_to_projects(
+        [projects.first.id, projects.second],
+        [users.first.id, users.second],
+        described_class::MASTER)
 
-    it { expect(@project_1.users).to include(@user_1) }
-    it { expect(@project_1.users).to include(@user_2) }
+      expect(projects.first.users).to include(users.first)
+      expect(projects.first.users).to include(users.second)
 
-    it { expect(@project_2.users).to include(@user_1) }
-    it { expect(@project_2.users).to include(@user_2) }
+      expect(projects.second.users).to include(users.first)
+      expect(projects.second.users).to include(users.second)
+    end
   end
 
   describe '.truncate_teams' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 83f61f0af0a..50423d027ac 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -836,7 +836,7 @@ describe Project, models: true do
 
     describe 'when a user has access to a project' do
       before do
-        project.team.add_user(user, Gitlab::Access::MASTER)
+        project.add_user(user, Gitlab::Access::MASTER)
       end
 
       it { is_expected.to eq([project]) }
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index a7930c59df9..b813ee967f8 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -15,7 +15,7 @@ describe API::API, api: true  do
   let(:milestone)   { create(:milestone, title: '1.0.0', project: project) }
 
   before do
-    project.team << [user, :reporters]
+    project.team << [user, :reporter]
   end
 
   describe "GET /projects/:id/merge_requests" do
@@ -299,7 +299,7 @@ describe API::API, api: true  do
       let!(:unrelated_project) { create(:project,  namespace: create(:user).namespace, creator_id: user2.id) }
 
       before :each do |each|
-        fork_project.team << [user2, :reporters]
+        fork_project.team << [user2, :reporter]
       end
 
       it "returns merge_request" do
-- 
cgit v1.2.1