Merge remote-tracking branch 'origin/master' into pipeline-hooks-without-slack

# Conflicts:
#	app/models/ci/pipeline.rb
#	app/services/ci/create_pipeline_service.rb
#	spec/models/project_services/hipchat_service_spec.rb

8 files changed, 282 insertions, 229 deletions

diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb
new file mode 100644
index 00000000000..d02b469dac8
--- /dev/null
+++ b/lib/api/access_requests.rb
@@ -0,0 +1,90 @@
+module API
+  class AccessRequests < Grape::API
+    before { authenticate! }
+
+    helpers ::API::Helpers::MembersHelpers
+
+    %w[group project].each do |source_type|
+      resource source_type.pluralize do
+        # Get a list of group/project access requests viewable by the authenticated user.
+        #
+        # Parameters:
+        #   id (required) - The group/project ID
+        #
+        # Example Request:
+        #  GET /groups/:id/access_requests
+        #  GET /projects/:id/access_requests
+        get ":id/access_requests" do
+          source = find_source(source_type, params[:id])
+          authorize_admin_source!(source_type, source)
+
+          access_requesters = paginate(source.requesters.includes(:user))
+
+          present access_requesters.map(&:user), with: Entities::AccessRequester, access_requesters: access_requesters
+        end
+
+        # Request access to the group/project
+        #
+        # Parameters:
+        #   id (required) - The group/project ID
+        #
+        # Example Request:
+        #  POST /groups/:id/access_requests
+        #  POST /projects/:id/access_requests
+        post ":id/access_requests" do
+          source = find_source(source_type, params[:id])
+          access_requester = source.request_access(current_user)
+
+          if access_requester.persisted?
+            present access_requester.user, with: Entities::AccessRequester, access_requester: access_requester
+          else
+            render_validation_error!(access_requester)
+          end
+        end
+
+        # Approve a group/project access request
+        #
+        # Parameters:
+        #   id (required) - The group/project ID
+        #   user_id (required) - The user ID of the access requester
+        #   access_level (optional) - Access level
+        #
+        # Example Request:
+        #   PUT /groups/:id/access_requests/:user_id/approve
+        #   PUT /projects/:id/access_requests/:user_id/approve
+        put ':id/access_requests/:user_id/approve' do
+          required_attributes! [:user_id]
+          source = find_source(source_type, params[:id])
+          authorize_admin_source!(source_type, source)
+
+          member = source.requesters.find_by!(user_id: params[:user_id])
+          if params[:access_level]
+            member.update(access_level: params[:access_level])
+          end
+          member.accept_request
+
+          status :created
+          present member.user, with: Entities::Member, member: member
+        end
+
+        # Deny a group/project access request
+        #
+        # Parameters:
+        #   id (required) - The group/project ID
+        #   user_id (required) - The user ID of the access requester
+        #
+        # Example Request:
+        #   DELETE /groups/:id/access_requests/:user_id
+        #   DELETE /projects/:id/access_requests/:user_id
+        delete ":id/access_requests/:user_id" do
+          required_attributes! [:user_id]
+          source = find_source(source_type, params[:id])
+
+          access_requester = source.requesters.find_by!(user_id: params[:user_id])
+
+          ::Members::DestroyService.new(access_requester, current_user).execute
+        end
+      end
+    end
+  end
+end
diff --git a/lib/api/api.rb b/lib/api/api.rb
index 6cd4a853dbe..d43af3f24e9 100644
--- a/lib/api/api.rb
+++ b/lib/api/api.rb
@@ -3,6 +3,10 @@ module API
     include APIGuard
     version 'v3', using: :path
 
+    rescue_from Gitlab::Access::AccessDeniedError do
+      rack_response({ 'message' => '403 Forbidden' }.to_json, 403)
+    end
+
     rescue_from ActiveRecord::RecordNotFound do
       rack_response({ 'message' => '404 Not found' }.to_json, 404)
     end
@@ -32,6 +36,7 @@ module API
     # Ensure the namespace is right, otherwise we might load Grape::API::Helpers
     helpers ::API::Helpers
 
+    mount ::API::AccessRequests
     mount ::API::AwardEmoji
     mount ::API::Branches
     mount ::API::Builds
@@ -40,19 +45,18 @@ module API
     mount ::API::DeployKeys
     mount ::API::Environments
     mount ::API::Files
-    mount ::API::GroupMembers
     mount ::API::Groups
     mount ::API::Internal
     mount ::API::Issues
     mount ::API::Keys
     mount ::API::Labels
     mount ::API::LicenseTemplates
+    mount ::API::Members
     mount ::API::MergeRequests
     mount ::API::Milestones
     mount ::API::Namespaces
     mount ::API::Notes
     mount ::API::ProjectHooks
-    mount ::API::ProjectMembers
     mount ::API::ProjectSnippets
     mount ::API::Projects
     mount ::API::Repositories
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 0178bbe7d1d..e8a3a6a9df0 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -92,9 +92,17 @@ module API
       end
     end
 
-    class ProjectMember < UserBasic
+    class Member < UserBasic
       expose :access_level do |user, options|
-        options[:project].project_members.find_by(user_id: user.id).access_level
+        member = options[:member] || options[:members].find { |m| m.user_id == user.id }
+        member.access_level
+      end
+    end
+
+    class AccessRequester < UserBasic
+      expose :requested_at do |user, options|
+        access_requester = options[:access_requester] || options[:access_requesters].find { |m| m.user_id == user.id }
+        access_requester.requested_at
       end
     end
 
@@ -109,12 +117,6 @@ module API
       expose :shared_projects, using: Entities::Project
     end
 
-    class GroupMember < UserBasic
-      expose :access_level do |user, options|
-        options[:group].group_members.find_by(user_id: user.id).access_level
-      end
-    end
-
     class RepoBranch < Grape::Entity
       expose :name
 
@@ -326,7 +328,7 @@ module API
       expose :id, :path, :kind
     end
 
-    class Member < Grape::Entity
+    class MemberAccess < Grape::Entity
       expose :access_level
       expose :notification_level do |member, options|
         if member.notification_setting
@@ -335,10 +337,10 @@ module API
       end
     end
 
-    class ProjectAccess < Member
+    class ProjectAccess < MemberAccess
     end
 
-    class GroupAccess < Member
+    class GroupAccess < MemberAccess
     end
 
     class ProjectService < Grape::Entity
diff --git a/lib/api/group_members.rb b/lib/api/group_members.rb
deleted file mode 100644
index dbe5bb08d3f..00000000000
--- a/lib/api/group_members.rb
+++ /dev/null
@@ -1,87 +0,0 @@
-module API
-  class GroupMembers < Grape::API
-    before { authenticate! }
-
-    resource :groups do
-      # Get a list of group members viewable by the authenticated user.
-      #
-      # Example Request:
-      #  GET /groups/:id/members
-      get ":id/members" do
-        group = find_group(params[:id])
-        users = group.users
-        present users, with: Entities::GroupMember, group: group
-      end
-
-      # Add a user to the list of group members
-      #
-      # Parameters:
-      #   id (required) - group id
-      #   user_id (required) - the users id
-      #   access_level (required) - Project access level
-      # Example Request:
-      #  POST /groups/:id/members
-      post ":id/members" do
-        group = find_group(params[:id])
-        authorize! :admin_group, group
-        required_attributes! [:user_id, :access_level]
-
-        unless validate_access_level?(params[:access_level])
-          render_api_error!("Wrong access level", 422)
-        end
-
-        if group.group_members.find_by(user_id: params[:user_id])
-          render_api_error!("Already exists", 409)
-        end
-
-        group.add_users([params[:user_id]], params[:access_level], current_user)
-        member = group.group_members.find_by(user_id: params[:user_id])
-        present member.user, with: Entities::GroupMember, group: group
-      end
-
-      # Update group member
-      #
-      # Parameters:
-      #   id (required) - The ID of a group
-      #   user_id (required) - The ID of a group member
-      #   access_level (required) - Project access level
-      # Example Request:
-      #   PUT /groups/:id/members/:user_id
-      put ':id/members/:user_id' do
-        group = find_group(params[:id])
-        authorize! :admin_group, group
-        required_attributes! [:access_level]
-
-        group_member = group.group_members.find_by(user_id: params[:user_id])
-        not_found!('User can not be found') if group_member.nil?
-
-        if group_member.update_attributes(access_level: params[:access_level])
-          @member = group_member.user
-          present @member, with: Entities::GroupMember, group: group
-        else
-          handle_member_errors group_member.errors
-        end
-      end
-
-      # Remove member.
-      #
-      # Parameters:
-      #   id (required) - group id
-      #   user_id (required) - the users id
-      #
-      # Example Request:
-      #   DELETE /groups/:id/members/:user_id
-      delete ":id/members/:user_id" do
-        group = find_group(params[:id])
-        authorize! :admin_group, group
-        member = group.group_members.find_by(user_id: params[:user_id])
-
-        if member.nil?
-          render_api_error!("404 Not Found - user_id:#{params[:user_id]} not a member of group #{group.name}", 404)
-        else
-          member.destroy
-        end
-      end
-    end
-  end
-end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 130509cdad6..d0469d6602d 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -28,7 +28,7 @@ module API
 
       # If the sudo is the current user do nothing
       if identifier && !(@current_user.id == identifier || @current_user.username == identifier)
-        render_api_error!('403 Forbidden: Must be admin to use sudo', 403) unless @current_user.is_admin?
+        forbidden!('Must be admin to use sudo') unless @current_user.is_admin?
         @current_user = User.by_username_or_id(identifier)
         not_found!("No user id or username for: #{identifier}") if @current_user.nil?
       end
@@ -49,16 +49,15 @@ module API
 
     def user_project
       @project ||= find_project(params[:id])
-      @project || not_found!("Project")
     end
 
     def find_project(id)
       project = Project.find_with_namespace(id) || Project.find_by(id: id)
 
-      if project && can?(current_user, :read_project, project)
+      if can?(current_user, :read_project, project)
         project
       else
-        nil
+        not_found!('Project')
       end
     end
 
@@ -89,11 +88,7 @@ module API
     end
 
     def find_group(id)
-      begin
-        group = Group.find(id)
-      rescue ActiveRecord::RecordNotFound
-        group = Group.find_by!(path: id)
-      end
+      group = Group.find_by(path: id) || Group.find_by(id: id)
 
       if can?(current_user, :read_group, group)
         group
@@ -135,7 +130,7 @@ module API
     end
 
     def authorize!(action, subject)
-      forbidden! unless abilities.allowed?(current_user, action, subject)
+      forbidden! unless can?(current_user, action, subject)
     end
 
     def authorize_push_project
@@ -197,10 +192,6 @@ module API
       errors
     end
 
-    def validate_access_level?(level)
-      Gitlab::Access.options_with_owner.values.include? level.to_i
-    end
-
     # Checks the occurrences of datetime attributes, each attribute if present in the params hash must be in ISO 8601
     # format (YYYY-MM-DDTHH:MM:SSZ) or a Bad Request error is invoked.
     #
@@ -411,11 +402,6 @@ module API
       File.read(Gitlab.config.gitlab_shell.secret_file).chomp
     end
 
-    def handle_member_errors(errors)
-      error!(errors[:access_level], 422) if errors[:access_level].any?
-      not_found!(errors)
-    end
-
     def send_git_blob(repository, blob)
       env['api.format'] = :txt
       content_type 'text/plain'
diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb
new file mode 100644
index 00000000000..90114f6f667
--- /dev/null
+++ b/lib/api/helpers/members_helpers.rb
@@ -0,0 +1,13 @@
+module API
+  module Helpers
+    module MembersHelpers
+      def find_source(source_type, id)
+        public_send("find_#{source_type}", id)
+      end
+
+      def authorize_admin_source!(source_type, source)
+        authorize! :"admin_#{source_type}", source
+      end
+    end
+  end
+end
diff --git a/lib/api/members.rb b/lib/api/members.rb
new file mode 100644
index 00000000000..2fae83f60b2
--- /dev/null
+++ b/lib/api/members.rb
@@ -0,0 +1,155 @@
+module API
+  class Members < Grape::API
+    before { authenticate! }
+
+    helpers ::API::Helpers::MembersHelpers
+
+    %w[group project].each do |source_type|
+      resource source_type.pluralize do
+        # Get a list of group/project members viewable by the authenticated user.
+        #
+        # Parameters:
+        #   id (required) - The group/project ID
+        #   query         - Query string
+        #
+        # Example Request:
+        #   GET /groups/:id/members
+        #   GET /projects/:id/members
+        get ":id/members" do
+          source = find_source(source_type, params[:id])
+
+          members = source.members.includes(:user)
+          members = members.joins(:user).merge(User.search(params[:query])) if params[:query]
+          members = paginate(members)
+
+          present members.map(&:user), with: Entities::Member, members: members
+        end
+
+        # Get a group/project member
+        #
+        # Parameters:
+        #   id (required) - The group/project ID
+        #   user_id (required) - The user ID of the member
+        #
+        # Example Request:
+        #   GET /groups/:id/members/:user_id
+        #   GET /projects/:id/members/:user_id
+        get ":id/members/:user_id" do
+          source = find_source(source_type, params[:id])
+
+          members = source.members
+          member = members.find_by!(user_id: params[:user_id])
+
+          present member.user, with: Entities::Member, member: member
+        end
+
+        # Add a new group/project member
+        #
+        # Parameters:
+        #   id (required) - The group/project ID
+        #   user_id (required) - The user ID of the new member
+        #   access_level (required) - A valid access level
+        #
+        # Example Request:
+        #   POST /groups/:id/members
+        #   POST /projects/:id/members
+        post ":id/members" do
+          source = find_source(source_type, params[:id])
+          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
+          # for both project and group members in 9.0!
+          conflict!('Member already exists') if source_type == 'group' && member
+
+          unless member
+            source.add_user(params[:user_id], params[:access_level], current_user)
+            member = source.members.find_by(user_id: params[:user_id])
+          end
+
+          if member
+            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]))
+            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)
+            render_validation_error!(member)
+          end
+        end
+
+        # Update a group/project member
+        #
+        # Parameters:
+        #   id (required) - The group/project ID
+        #   user_id (required) - The user ID of the member
+        #   access_level (required) - A valid access level
+        #
+        # Example Request:
+        #   PUT /groups/:id/members/:user_id
+        #   PUT /projects/:id/members/:user_id
+        put ":id/members/:user_id" do
+          source = find_source(source_type, params[:id])
+          authorize_admin_source!(source_type, source)
+          required_attributes! [:user_id, :access_level]
+
+          member = source.members.find_by!(user_id: params[:user_id])
+
+          if member.update_attributes(access_level: params[:access_level])
+            present member.user, with: Entities::Member, member: member
+          else
+            # 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)
+            render_validation_error!(member)
+          end
+        end
+
+        # Remove a group/project member
+        #
+        # Parameters:
+        #   id (required) - The group/project ID
+        #   user_id (required) - The user ID of the member
+        #
+        # Example Request:
+        #   DELETE /groups/:id/members/:user_id
+        #   DELETE /projects/:id/members/:user_id
+        delete ":id/members/:user_id" do
+          source = find_source(source_type, params[:id])
+          required_attributes! [:user_id]
+
+          # This is to ensure back-compatibility but find_by! should be used
+          # in that casse in 9.0!
+          member = source.members.find_by(user_id: params[:user_id])
+
+          # This is to ensure back-compatibility but this should be removed in
+          # favor of find_by! in 9.0!
+          not_found!("Member: user_id:#{params[:user_id]}") if source_type == 'group' && member.nil?
+
+          # This is to ensure back-compatibility but 204 behavior should be used
+          # for all DELETE endpoints in 9.0!
+          if member.nil?
+            { message: "Access revoked", id: params[:user_id].to_i }
+          else
+            ::Members::DestroyService.new(member, current_user).execute
+
+            present member.user, with: Entities::Member, member: member
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/api/project_members.rb b/lib/api/project_members.rb
deleted file mode 100644
index 6a0b3e7d134..00000000000
--- a/lib/api/project_members.rb
+++ /dev/null
@@ -1,110 +0,0 @@
-module API
-  # Projects members API
-  class ProjectMembers < Grape::API
-    before { authenticate! }
-
-    resource :projects do
-      # Get a project team members
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      #   query         - Query string
-      # Example Request:
-      #   GET /projects/:id/members
-      get ":id/members" do
-        if params[:query].present?
-          @members = paginate user_project.users.where("username LIKE ?", "%#{params[:query]}%")
-        else
-          @members = paginate user_project.users
-        end
-        present @members, with: Entities::ProjectMember, project: user_project
-      end
-
-      # Get a project team members
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      #   user_id (required) - The ID of a user
-      # Example Request:
-      #   GET /projects/:id/members/:user_id
-      get ":id/members/:user_id" do
-        @member = user_project.users.find params[:user_id]
-        present @member, with: Entities::ProjectMember, project: user_project
-      end
-
-      # Add a new project team member
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      #   user_id (required) - The ID of a user
-      #   access_level (required) - Project access level
-      # Example Request:
-      #   POST /projects/:id/members
-      post ":id/members" do
-        authorize! :admin_project, user_project
-        required_attributes! [:user_id, :access_level]
-
-        # either the user is already a team member or a new one
-        project_member = user_project.project_member(params[:user_id])
-        if project_member.nil?
-          project_member = user_project.project_members.new(
-            user_id: params[:user_id],
-            access_level: params[:access_level]
-          )
-        end
-
-        if project_member.save
-          @member = project_member.user
-          present @member, with: Entities::ProjectMember, project: user_project
-        else
-          handle_member_errors project_member.errors
-        end
-      end
-
-      # Update project team member
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      #   user_id (required) - The ID of a team member
-      #   access_level (required) - Project access level
-      # Example Request:
-      #   PUT /projects/:id/members/:user_id
-      put ":id/members/:user_id" do
-        authorize! :admin_project, user_project
-        required_attributes! [:access_level]
-
-        project_member = user_project.project_members.find_by(user_id: params[:user_id])
-        not_found!("User can not be found") if project_member.nil?
-
-        if project_member.update_attributes(access_level: params[:access_level])
-          @member = project_member.user
-          present @member, with: Entities::ProjectMember, project: user_project
-        else
-          handle_member_errors project_member.errors
-        end
-      end
-
-      # Remove a team member from project
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      #   user_id (required) - The ID of a team member
-      # Example Request:
-      #   DELETE /projects/:id/members/:user_id
-      delete ":id/members/:user_id" do
-        project_member = user_project.project_members.find_by(user_id: params[:user_id])
-
-        unless current_user.can?(:admin_project, user_project) ||
-                current_user.can?(:destroy_project_member, project_member)
-          forbidden!
-        end
-
-        if project_member.nil?
-          { message: "Access revoked", id: params[:user_id].to_i }
-        else
-          project_member.destroy
-        end
-      end
-    end
-  end
-end