Add member: Always return 409 when a member exists

changelogs/unreleased/20732_member_exists_409.yml

+---
+title: 'Add member: Always return 409 when a member exists'
+merge_request:
+author:

doc/api/v3_to_v4.md

@@ -12,3 +12,4 @@ changes are in V4:
 - Endpoints under `projects/merge_request/:id` have been removed (use: `projects/merge_requests/:id`)
 - Project snippets do not return deprecated field `expires_at`
 - Endpoints under `projects/:id/keys` have been removed (use `projects/:id/deploy_keys`)
+- Status 409 returned for POST `project/:id/members` when a member already exists

lib/api/api.rb

@@ -7,6 +7,7 @@ module API
     version 'v3', using: :path do
       mount ::API::V3::DeployKeys
       mount ::API::V3::Issues
+      mount ::API::V3::Members
       mount ::API::V3::MergeRequests
       mount ::API::V3::Projects
       mount ::API::V3::ProjectSnippets

lib/api/members.rb

@@ -56,16 +56,9 @@ module API
 
           member = source.members.find_by(user_id: params[:user_id])
 
-          # We need this explicit check because `source.add_user` doesn't
-          # currently return the member created so it would return 201 even if
-          # the member already existed...
-          # The `source_type == 'group'` check 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
-            member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
-          end
+          conflict!('Member already exists') if member
+
+          member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
 
           if member.persisted? && member.valid?
             present member.user, with: Entities::Member, member: member

lib/api/v3/members.rb

+module API
+  module V3
+    class Members < Grape::API
+      include PaginationParams
+
+      before { authenticate! }
+
+      helpers ::API::Helpers::MembersHelpers
+
+      %w[group project].each do |source_type|
+        params do
+          requires :id, type: String, desc: "The #{source_type} ID"
+        end
+        resource source_type.pluralize do
+          desc 'Gets a list of group or project members viewable by the authenticated user.' do
+            success ::API::Entities::Member
+          end
+          params do
+            optional :query, type: String, desc: 'A query string to search for members'
+            use :pagination
+          end
+          get ":id/members" do
+            source = find_source(source_type, params[:id])
+
+            users = source.users
+            users = users.merge(User.search(params[:query])) if params[:query]
+
+            present paginate(users), with: ::API::Entities::Member, source: source
+          end
+
+          desc 'Gets a member of a group or project.' do
+            success ::API::Entities::Member
+          end
+          params do
+            requires :user_id, type: Integer, desc: 'The user ID of the member'
+          end
+          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: ::API::Entities::Member, member: member
+          end
+
+          desc 'Adds a member to a group or project.' do
+            success ::API::Entities::Member
+          end
+          params do
+            requires :user_id, type: Integer, desc: 'The user ID of the new member'
+            requires :access_level, type: Integer, desc: 'A valid access level (defaults: `30`, developer access level)'
+            optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY'
+          end
+          post ":id/members" do
+            source = find_source(source_type, params[:id])
+            authorize_admin_source!(source_type, source)
+
+            member = source.members.find_by(user_id: params[:user_id])
+
+            # We need this explicit check because `source.add_user` doesn't
+            # currently return the member created so it would return 201 even if
+            # the member already existed...
+            # The `source_type == 'group'` check 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
+              member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
+            end
+            if member.persisted? && member.valid?
+              present member.user, with: ::API::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
+
+          desc 'Updates a member of a group or project.' do
+            success ::API::Entities::Member
+          end
+          params do
+            requires :user_id, type: Integer, desc: 'The user ID of the new member'
+            requires :access_level, type: Integer, desc: 'A valid access level'
+            optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY'
+          end
+          put ":id/members/:user_id" do
+            source = find_source(source_type, params[:id])
+            authorize_admin_source!(source_type, source)
+
+            member = source.members.find_by!(user_id: params[:user_id])
+            attrs = attributes_for_keys [:access_level, :expires_at]
+
+            if member.update_attributes(attrs)
+              present member.user, with: ::API::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
+
+          desc 'Removes a user from a group or project.'
+          params do
+            requires :user_id, type: Integer, desc: 'The user ID of the member'
+          end
+          delete ":id/members/:user_id" do
+            source = find_source(source_type, params[: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(source, current_user, declared_params).execute
+
+              present member.user, with: ::API::Entities::Member, member: member
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/requests/api/members_spec.rb

@@ -145,11 +145,11 @@ describe API::Members, api: true  do
         end
       end
 
-      it "returns #{source_type == 'project' ? 201 : 409} if member already exists" do
+      it "returns 409 if member already exists" do
         post api("/#{source_type.pluralize}/#{source.id}/members", master),
              user_id: master.id, access_level: Member::MASTER
 
-        expect(response).to have_http_status(source_type == 'project' ? 201 : 409)
+        expect(response).to have_http_status(409)
       end
 
       it 'returns 400 when user_id is not given' do