Grapify the users API

Signed-off-by: Rémy Coutable <remy@rymai.me>

doc/api/users.md

@@ -371,24 +371,24 @@ Parameters:
 Get a list of a specified user's SSH keys. Available only for admin
 
 ```
-GET /users/:uid/keys
+GET /users/:id/keys
 ```
 
 Parameters:
 
-- `uid` (required) - id of specified user
+- `id` (required) - id of specified user
 
 ## Single SSH key
 
 Get a single key.
 
 ```
-GET /user/keys/:id
+GET /user/keys/:key_id
 ```
 
 Parameters:
 
-- `id` (required) - The ID of an SSH key
+- `key_id` (required) - The ID of an SSH key
 
 ```json
 {
@@ -460,25 +460,25 @@ This is an idempotent function and calling it on a key that is already deleted
 or not available results in `200 OK`.
 
 ```
-DELETE /user/keys/:id
+DELETE /user/keys/:key_id
 ```
 
 Parameters:
 
-- `id` (required) - SSH key ID
+- `key_id` (required) - SSH key ID
 
 ## Delete SSH key for given user
 
 Deletes key owned by a specified user. Available only for admin.
 
 ```
-DELETE /users/:uid/keys/:id
+DELETE /users/:id/keys/:key_id
 ```
 
 Parameters:
 
-- `uid` (required) - id of specified user
-- `id` (required)  - SSH key ID
+- `id` (required) - id of specified user
+- `key_id` (required)  - SSH key ID
 
 Will return `200 OK` on success, or `404 Not found` if either user or key cannot be found.
 
@@ -512,24 +512,24 @@ Parameters:
 Get a list of a specified user's emails. Available only for admin
 
 ```
-GET /users/:uid/emails
+GET /users/:id/emails
 ```
 
 Parameters:
 
-- `uid` (required) - id of specified user
+- `id` (required) - id of specified user
 
 ## Single email
 
 Get a single email.
 
 ```
-GET /user/emails/:id
+GET /user/emails/:email_id
 ```
 
 Parameters:
 
-- `id` (required) - email ID
+- `email_id` (required) - email ID
 
 ```json
 {
@@ -592,25 +592,25 @@ This is an idempotent function and calling it on a email that is already deleted
 or not available results in `200 OK`.
 
 ```
-DELETE /user/emails/:id
+DELETE /user/emails/:email_id
 ```
 
 Parameters:
 
-- `id` (required) - email ID
+- `email_id` (required) - email ID
 
 ## Delete email for given user
 
 Deletes email owned by a specified user. Available only for admin.
 
 ```
-DELETE /users/:uid/emails/:id
+DELETE /users/:id/emails/:email_id
 ```
 
 Parameters:
 
-- `uid` (required) - id of specified user
-- `id` (required)  - email ID
+- `id` (required) - id of specified user
+- `email_id` (required)  - email ID
 
 Will return `200 OK` on success, or `404 Not found` if either user or email cannot be found.
 
@@ -619,12 +619,12 @@ Will return `200 OK` on success, or `404 Not found` if either user or email cann
 Blocks the specified user.  Available only for admin.
 
 ```
-PUT /users/:uid/block
+PUT /users/:id/block
 ```
 
 Parameters:
 
-- `uid` (required) - id of specified user
+- `id` (required) - id of specified user
 
 Will return `200 OK` on success, `404 User Not Found` is user cannot be found or
 `403 Forbidden` when trying to block an already blocked user by LDAP synchronization.
@@ -634,12 +634,12 @@ Will return `200 OK` on success, `404 User Not Found` is user cannot be found or
 Unblocks the specified user.  Available only for admin.
 
 ```
-PUT /users/:uid/unblock
+PUT /users/:id/unblock
 ```
 
 Parameters:
 
-- `uid` (required) - id of specified user
+- `id` (required) - id of specified user
 
 Will return `200 OK` on success, `404 User Not Found` is user cannot be found or
 `403 Forbidden` when trying to unblock a user blocked by LDAP synchronization.

lib/api/users.rb

@@ -4,91 +4,95 @@ module API
     before { authenticate! }
 
     resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
-      # Get a users list
-      #
-      # Example Request:
-      #  GET /users
-      #  GET /users?search=Admin
-      #  GET /users?username=root
-      #  GET /users?active=true
-      #  GET /users?external=true
-      #  GET /users?blocked=true
+      helpers do
+        params :optional_attributes do
+          optional :skype, type: String, desc: 'The Skype username'
+          optional :linkedin, type: String, desc: 'The LinkedIn username'
+          optional :twitter, type: String, desc: 'The Twitter username'
+          optional :website_url, type: String, desc: 'The website of the user'
+          optional :organization, type: String, desc: 'The organization of the user'
+          optional :projects_limit, type: Integer, desc: 'The number of projects a user can create'
+          optional :extern_uid, type: Integer, desc: 'The external authentication provider UID'
+          optional :provider, type: String, desc: 'The external provider'
+          optional :bio, type: String, desc: 'The biography of the user'
+          optional :location, type: String, desc: 'The location of the user'
+          optional :admin, type: Boolean, desc: 'Flag indicating the user is an administrator'
+          optional :can_create_group, type: Boolean, desc: 'Flag indicating the user can create groups'
+          optional :confirm, type: Boolean, desc: 'Flag indicating the account needs to be confirmed'
+          optional :external, type: Boolean, desc: 'Flag indicating the user is an external user'
+          all_or_none_of :extern_uid, :provider
+        end
+      end
+
+      desc 'Get the list of users' do
+        success Entities::UserBasic
+      end
+      params do
+        optional :username, type: String, desc: 'Get a single user with a specific username'
+        optional :search, type: String, desc: 'Search for a username'
+        optional :active, type: Boolean, default: false, desc: 'Filters only active users'
+        optional :external, type: Boolean, default: false, desc: 'Filters only external users'
+        optional :blocked, type: Boolean, default: false, desc: 'Filters only blocked users'
+        optional :skip_ldap, type: Boolean, default: false, desc: 'Skip LDAP users'
+      end
       get do
         unless can?(current_user, :read_users_list, nil)
           render_api_error!("Not authorized.", 403)
         end
 
         if params[:username].present?
-          @users = User.where(username: params[:username])
+          users = User.where(username: params[:username])
         else
-          skip_ldap = params[:skip_ldap].present? && params[:skip_ldap] == 'true'
-          @users = User.all
-          @users = @users.active if to_boolean(params[:active])
-          @users = @users.non_ldap if skip_ldap
-          @users = @users.search(params[:search]) if params[:search].present?
-          @users = @users.blocked if to_boolean(params[:blocked])
-          @users = @users.external if to_boolean(params[:external]) && current_user.is_admin?
-          @users = paginate @users
-        end
-
-        if current_user.is_admin?
-          present @users, with: Entities::UserFull
-        else
-          present @users, with: Entities::UserBasic
+          users = User.all
+          users = users.active if params[:active]
+          users = users.non_ldap if params[:skip_ldap]
+          users = users.search(params[:search]) if params[:search].present?
+          users = users.blocked if params[:blocked]
+          users = users.external if params[:external] && current_user.is_admin?
         end
+
+        entity = current_user.is_admin? ? Entities::UserFull : Entities::UserBasic
+        present paginate(users), with: entity
       end
 
-      # Get a single user
-      #
-      # Parameters:
-      #   id (required) - The ID of a user
-      # Example Request:
-      #   GET /users/:id
+      desc 'Get a single user' do
+        success Entities::UserBasic
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+      end
       get ":id" do
-        @user = User.find(params[:id])
+        user = User.find_by(id: params[:id])
+        not_found!('User') unless user
 
         if current_user && current_user.is_admin?
-          present @user, with: Entities::UserFull
-        elsif can?(current_user, :read_user, @user)
-          present @user, with: Entities::User
+          present user, with: Entities::UserFull
+        elsif can?(current_user, :read_user, user)
+          present user, with: Entities::User
         else
           render_api_error!("User not found.", 404)
         end
       end
 
-      # Create user. Available only for admin
-      #
-      # Parameters:
-      #   email (required)                  - Email
-      #   password (required)               - Password
-      #   name (required)                   - Name
-      #   username (required)               - Name
-      #   skype                             - Skype ID
-      #   linkedin                          - Linkedin
-      #   twitter                           - Twitter account
-      #   website_url                       - Website url
-      #   organization                      - Organization
-      #   projects_limit                    - Number of projects user can create
-      #   extern_uid                        - External authentication provider UID
-      #   provider                          - External provider
-      #   bio                               - Bio
-      #   location                          - Location of the user
-      #   admin                             - User is admin - true or false (default)
-      #   can_create_group                  - User can create groups - true or false
-      #   confirm                           - Require user confirmation - true (default) or false
-      #   external                          - Flags the user as external - true or false(default)
-      # Example Request:
-      #   POST /users
+      desc 'Create a user. Available only for admins.' do
+        success Entities::UserFull
+      end
+      params do
+        requires :email, type: String, desc: 'The email of the user'
+        requires :password, type: String, desc: 'The password of the new user'
+        requires :name, type: String, desc: 'The name of the user'
+        requires :username, type: String, desc: 'The username of the user'
+        use :optional_attributes
+      end
       post do
         authenticated_as_admin!
-        required_attributes! [:email, :password, :name, :username]
-        attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :bio, :location, :can_create_group, :admin, :confirm, :external, :organization]
-        admin = attrs.delete(:admin)
-        confirm = !(attrs.delete(:confirm) =~ /(false|f|no|0)$/i)
-        user = User.build_user(attrs)
-        user.admin = admin unless admin.nil?
+
+        # Filter out params which are used later
+        identity_attrs = params.slice(:provider, :extern_uid)
+        confirm = params.delete(:confirm)
+
+        user = User.build_user(declared_params(include_missing: false))
         user.skip_confirmation! unless confirm
-        identity_attrs = attributes_for_keys [:provider, :extern_uid]
 
         if identity_attrs.any?
           user.identities.build(identity_attrs)
@@ -109,46 +113,40 @@ module API
         end
       end
 
-      # Update user. Available only for admin
-      #
-      # Parameters:
-      #   email                             - Email
-      #   name                              - Name
-      #   password                          - Password
-      #   skype                             - Skype ID
-      #   linkedin                          - Linkedin
-      #   twitter                           - Twitter account
-      #   website_url                       - Website url
-      #   organization                      - Organization
-      #   projects_limit                    - Limit projects each user can create
-      #   bio                               - Bio
-      #   location                          - Location of the user
-      #   admin                             - User is admin - true or false (default)
-      #   can_create_group                  - User can create groups - true or false
-      #   external                          - Flags the user as external - true or false(default)
-      # Example Request:
-      #   PUT /users/:id
+      desc 'Update a user. Available only for admins.' do
+        success Entities::UserFull
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+        optional :email, type: String, desc: 'The email of the user'
+        optional :password, type: String, desc: 'The password of the new user'
+        optional :name, type: String, desc: 'The name of the user'
+        optional :username, type: String, desc: 'The username of the user'
+        use :optional_attributes
+        at_least_one_of :email, :password, :name, :username, :skype, :linkedin,
+                        :twitter, :website_url, :organization, :projects_limit,
+                        :extern_uid, :provider, :bio, :location, :admin,
+                        :can_create_group, :confirm, :external
+      end
       put ":id" do
         authenticated_as_admin!
 
-        attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :website_url, :projects_limit, :username, :bio, :location, :can_create_group, :admin, :external, :organization]
-        user = User.find(params[:id])
+        user = User.find_by(id: params.delete(:id))
         not_found!('User') unless user
 
-        admin = attrs.delete(:admin)
-        user.admin = admin unless admin.nil?
-
-        conflict!('Email has already been taken') if attrs[:email] &&
-            User.where(email: attrs[:email]).
+        conflict!('Email has already been taken') if params[:email] &&
+            User.where(email: params[:email]).
                 where.not(id: user.id).count > 0
 
-        conflict!('Username has already been taken') if attrs[:username] &&
-            User.where(username: attrs[:username]).
+        conflict!('Username has already been taken') if params[:username] &&
+            User.where(username: params[:username]).
                 where.not(id: user.id).count > 0
 
-        identity_attrs = attributes_for_keys [:provider, :extern_uid]
+        identity_attrs = params.slice(:provider, :extern_uid)
+
         if identity_attrs.any?
           identity = user.identities.find_by(provider: identity_attrs[:provider])
+
           if identity
             identity.update_attributes(identity_attrs)
           else
@@ -157,28 +155,33 @@ module API
           end
         end
 
-        if user.update_attributes(attrs)
+        # Delete already handled parameters
+        params.delete(:extern_uid)
+        params.delete(:provider)
+
+        if user.update_attributes(declared_params(include_missing: false))
           present user, with: Entities::UserFull
         else
           render_validation_error!(user)
         end
       end
 
-      # Add ssh key to a specified user. Only available to admin users.
-      #
-      # Parameters:
-      #   id (required) - The ID of a user
-      #   key (required) - New SSH Key
-      #   title (required) - New SSH Key's title
-      # Example Request:
-      #   POST /users/:id/keys
+      desc 'Add an SSH key to a specified user. Available only for admins.' do
+        success Entities::SSHKey
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+        requires :key, type: String, desc: 'The new SSH key'
+        requires :title, type: String, desc: 'The title of the new SSH key'
+      end
       post ":id/keys" do
         authenticated_as_admin!
-        required_attributes! [:title, :key]
 
-        user = User.find(params[:id])
-        attrs = attributes_for_keys [:title, :key]
-        key = user.keys.new attrs
+        user = User.find_by(id: params.delete(:id))
+        not_found!('User') unless user
+
+        key = user.keys.new(declared_params(include_missing: false))
+
         if key.save
           present key, with: Entities::SSHKey
         else
@@ -186,55 +189,55 @@ module API
         end
       end
 
-      # Get ssh keys of a specified user. Only available to admin users.
-      #
-      # Parameters:
-      #   uid (required) - The ID of a user
-      # Example Request:
-      #   GET /users/:uid/keys
-      get ':uid/keys' do
+      desc 'Get the SSH keys of a specified user. Available only for admins.' do
+        success Entities::SSHKey
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+      end
+      get ':id/keys' do
         authenticated_as_admin!
-        user = User.find_by(id: params[:uid])
+
+        user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
         present user.keys, with: Entities::SSHKey
       end
 
-      # Delete existing ssh key of a specified user. Only available to admin
-      # users.
-      #
-      # Parameters:
-      #   uid (required) - The ID of a user
-      #   id (required) - SSH Key ID
-      # Example Request:
-      #   DELETE /users/:uid/keys/:id
-      delete ':uid/keys/:id' do
+      desc 'Delete an existing SSH key from a specified user. Available only for admins.' do
+        success Entities::SSHKey
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+        requires :key_id, type: Integer, desc: 'The ID of the SSH key'
+      end
+      delete ':id/keys/:key_id' do
         authenticated_as_admin!
-        user = User.find_by(id: params[:uid])
+
+        user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
-        begin
-          key = user.keys.find params[:id]
-          key.destroy
-        rescue ActiveRecord::RecordNotFound
-          not_found!('Key')
-        end
+        key = user.keys.find_by(id: params[:key_id])
+        not_found!('Key') unless key
+
+        present key.destroy, with: Entities::SSHKey
       end
 
-      # Add email to a specified user. Only available to admin users.
-      #
-      # Parameters:
-      #   id (required) - The ID of a user
-      #   email (required) - Email address
-      # Example Request:
-      #   POST /users/:id/emails
+      desc 'Add an email address to a specified user. Available only for admins.' do
+        success Entities::Email
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+        requires :email, type: String, desc: 'The email of the user'
+      end
       post ":id/emails" do
         authenticated_as_admin!
-        required_attributes! [:email]
 
-        user = User.find(params[:id])
-        attrs = attributes_for_keys [:email]
-        email = user.emails.new attrs
+        user = User.find_by(id: params.delete(:id))
+        not_found!('User') unless user
+
+        email = user.emails.new(declared_params(include_missing: false))
+
         if email.save
           NotificationService.new.new_email(email)
           present email, with: Entities::Email
@@ -243,98 +246,91 @@ module API
         end
       end
 
-      # Get emails of a specified user. Only available to admin users.
-      #
-      # Parameters:
-      #   uid (required) - The ID of a user
-      # Example Request:
-      #   GET /users/:uid/emails
-      get ':uid/emails' do
+      desc 'Get the emails addresses of a specified user. Available only for admins.' do
+        success Entities::Email
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+      end
+      get ':id/emails' do
         authenticated_as_admin!
-        user = User.find_by(id: params[:uid])
+        user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
         present user.emails, with: Entities::Email
       end
 
-      # Delete existing email of a specified user. Only available to admin
-      # users.
-      #
-      # Parameters:
-      #   uid (required) - The ID of a user
-      #   id (required) - Email ID
-      # Example Request:
-      #   DELETE /users/:uid/emails/:id
-      delete ':uid/emails/:id' do
+      desc 'Delete an email address of a specified user. Available only for admins.' do
+        success Entities::Email
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+        requires :email_id, type: Integer, desc: 'The ID of the email'
+      end
+      delete ':id/emails/:email_id' do
         authenticated_as_admin!
-        user = User.find_by(id: params[:uid])
+        user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
-        begin
-          email = user.emails.find params[:id]
-          email.destroy
+        email = user.emails.find_by(id: params[:email_id])
+        not_found!('Email') unless email
 
+        email.destroy
         user.update_secondary_emails!
-        rescue ActiveRecord::RecordNotFound
-          not_found!('Email')
-        end
       end
 
-      # Delete user. Available only for admin
-      #
-      # Example Request:
-      #   DELETE /users/:id
+      desc 'Delete a user. Available only for admins.' do
+        success Entities::Email
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+      end
       delete ":id" do
         authenticated_as_admin!
         user = User.find_by(id: params[:id])
+        not_found!('User') unless user
 
-        if user
         DeleteUserService.new(current_user).execute(user)
-        else
-          not_found!('User')
-        end
       end
 
-      # Block user. Available only for admin
-      #
-      # Example Request:
-      #   PUT /users/:id/block
+      desc 'Block a user. Available only for admins.'
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+      end
       put ':id/block' do
         authenticated_as_admin!
         user = User.find_by(id: params[:id])
+        not_found!('User') unless user
 
-        if !user
-          not_found!('User')
-        elsif !user.ldap_blocked?
+        if !user.ldap_blocked?
           user.block
         else
           forbidden!('LDAP blocked users cannot be modified by the API')
         end
       end
 
-      # Unblock user. Available only for admin
-      #
-      # Example Request:
-      #   PUT /users/:id/unblock
+      desc 'Unblock a user. Available only for admins.'
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+      end
       put ':id/unblock' do
         authenticated_as_admin!
         user = User.find_by(id: params[:id])
+        not_found!('User') unless user
 
-        if !user
-          not_found!('User')
-        elsif user.ldap_blocked?
+        if user.ldap_blocked?
           forbidden!('LDAP blocked users cannot be unblocked by the API')
         else
           user.activate
         end
       end
 
-      desc 'Get contribution events of a specified user' do
+      desc 'Get the contribution events of a specified user' do
         detail 'This feature was introduced in GitLab 8.13.'
         success Entities::Event
       end
       params do
-        requires :id, type: String, desc: 'The user ID'
+        requires :id, type: Integer, desc: 'The ID of the user'
       end
       get ':id/events' do
         user = User.find_by(id: params[:id])
@@ -351,43 +347,43 @@ module API
     end
 
     resource :user do
-      # Get currently authenticated user
-      #
-      # Example Request:
-      #   GET /user
+      desc 'Get the currently authenticated user' do
+        success Entities::UserFull
+      end
       get do
-        present @current_user, with: Entities::UserFull
+        present current_user, with: Entities::UserFull
       end
 
-      # Get currently authenticated user's keys
-      #
-      # Example Request:
-      #   GET /user/keys
+      desc "Get the currently authenticated user's SSH keys" do
+        success Entities::SSHKey
+      end
       get "keys" do
         present current_user.keys, with: Entities::SSHKey
       end
 
-      # Get single key owned by currently authenticated user
-      #
-      # Example Request:
-      #   GET /user/keys/:id
-      get "keys/:id" do
-        key = current_user.keys.find params[:id]
+      desc 'Get a single key owned by currently authenticated user' do
+        success Entities::SSHKey
+      end
+      params do
+        requires :key_id, type: Integer, desc: 'The ID of the SSH key'
+      end
+      get "keys/:key_id" do
+        key = current_user.keys.find_by(id: params[:key_id])
+        not_found!('Key') unless key
+
         present key, with: Entities::SSHKey
       end
 
-      # Add new ssh key to currently authenticated user
-      #
-      # Parameters:
-      #   key (required) - New SSH Key
-      #   title (required) - New SSH Key's title
-      # Example Request:
-      #   POST /user/keys
+      desc 'Add a new SSH key to the currently authenticated user' do
+        success Entities::SSHKey
+      end
+      params do
+        requires :key, type: String, desc: 'The new SSH key'
+        requires :title, type: String, desc: 'The title of the new SSH key'
+      end
       post "keys" do
-        required_attributes! [:title, :key]
+        key = current_user.keys.new(declared_params)
 
-        attrs = attributes_for_keys [:title, :key]
-        key = current_user.keys.new attrs
         if key.save
           present key, with: Entities::SSHKey
         else
@@ -395,48 +391,48 @@ module API
         end
       end
 
-      # Delete existing ssh key of currently authenticated user
-      #
-      # Parameters:
-      #   id (required) - SSH Key ID
-      # Example Request:
-      #   DELETE /user/keys/:id
-      delete "keys/:id" do
-        begin
-          key = current_user.keys.find params[:id]
-          key.destroy
-        rescue
+      desc 'Delete an SSH key from the currently authenticated user' do
+        success Entities::SSHKey
+      end
+      params do
+        requires :key_id, type: Integer, desc: 'The ID of the SSH key'
       end
+      delete "keys/:key_id" do
+        key = current_user.keys.find_by(id: params[:key_id])
+        not_found!('Key') unless key
+
+        present key.destroy, with: Entities::SSHKey
       end
 
-      # Get currently authenticated user's emails
-      #
-      # Example Request:
-      #   GET /user/emails
+      desc "Get the currently authenticated user's email addresses" do
+        success Entities::Email
+      end
       get "emails" do
         present current_user.emails, with: Entities::Email
       end
 
-      # Get single email owned by currently authenticated user
-      #
-      # Example Request:
-      #   GET /user/emails/:id
-      get "emails/:id" do
-        email = current_user.emails.find params[:id]
+      desc 'Get a single email address owned by the currently authenticated user' do
+        success Entities::Email
+      end
+      params do
+        requires :email_id, type: Integer, desc: 'The ID of the email'
+      end
+      get "emails/:email_id" do
+        email = current_user.emails.find_by(id: params[:email_id])
+        not_found!('Email') unless email
+
         present email, with: Entities::Email
       end
 
-      # Add new email to currently authenticated user
-      #
-      # Parameters:
-      #   email (required) - Email address
-      # Example Request:
-      #   POST /user/emails
+      desc 'Add new email address to the currently authenticated user' do
+        success Entities::Email
+      end
+      params do
+        requires :email, type: String, desc: 'The new email'
+      end
       post "emails" do
-        required_attributes! [:email]
+        email = current_user.emails.new(declared_params)
 
-        attrs = attributes_for_keys [:email]
-        email = current_user.emails.new attrs
         if email.save
           NotificationService.new.new_email(email)
           present email, with: Entities::Email
@@ -445,20 +441,16 @@ module API
         end
       end
 
-      # Delete existing email of currently authenticated user
-      #
-      # Parameters:
-      #   id (required) - EMail ID
-      # Example Request:
-      #   DELETE /user/emails/:id
-      delete "emails/:id" do
-        begin
-          email = current_user.emails.find params[:id]
-          email.destroy
+      desc 'Delete an email address from the currently authenticated user'
+      params do
+        requires :email_id, type: Integer, desc: 'The ID of the email'
+      end
+      delete "emails/:email_id" do
+        email = current_user.emails.find_by(id: params[:email_id])
+        not_found!('Email') unless email
 
+        email.destroy
         current_user.update_secondary_emails!
-        rescue
-        end
       end
     end
   end

spec/requests/api/users_spec.rb

@@ -120,7 +120,7 @@ describe API::API, api: true  do
     it "returns a 404 error if user id not found" do
       get api("/users/9999", user)
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 Not found')
+      expect(json_response['message']).to eq('404 User Not Found')
     end
 
     it "returns a 404 for invalid ID" do
@@ -371,7 +371,7 @@ describe API::API, api: true  do
     it "returns 404 for non-existing user" do
       put api("/users/999999", admin), { bio: 'update should fail' }
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 Not found')
+      expect(json_response['message']).to eq('404 User Not Found')
     end
 
     it "returns a 404 if invalid ID" do
@@ -399,6 +399,18 @@ describe API::API, api: true  do
         to eq([Gitlab::Regex.namespace_regex_message])
     end
 
+    it 'returns 400 if provider is missing for identity update' do
+      put api("/users/#{omniauth_user.id}", admin), extern_uid: '654321'
+
+      expect(response).to have_http_status(400)
+    end
+
+    it 'returns 400 if external UID is missing for identity update' do
+      put api("/users/#{omniauth_user.id}", admin), provider: 'ldap'
+
+      expect(response).to have_http_status(400)
+    end
+
     context "with existing user" do
       before do
         post api("/users", admin), { email: 'test@example.com', password: 'password', username: 'test', name: 'test' }
@@ -426,14 +438,16 @@ describe API::API, api: true  do
 
     it "does not create invalid ssh key" do
       post api("/users/#{user.id}/keys", admin), { title: "invalid key" }
+
       expect(response).to have_http_status(400)
-      expect(json_response['message']).to eq('400 (Bad request) "key" not given')
+      expect(json_response['error']).to eq('key is missing')
     end
 
     it 'does not create key without title' do
       post api("/users/#{user.id}/keys", admin), key: 'some key'
+
       expect(response).to have_http_status(400)
-      expect(json_response['message']).to eq('400 (Bad request) "title" not given')
+      expect(json_response['error']).to eq('title is missing')
     end
 
     it "creates ssh key" do
@@ -449,7 +463,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe 'GET /user/:uid/keys' do
+  describe 'GET /user/:id/keys' do
     before { admin }
 
     context 'when unauthenticated' do
@@ -477,7 +491,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe 'DELETE /user/:uid/keys/:id' do
+  describe 'DELETE /user/:id/keys/:key_id' do
     before { admin }
 
     context 'when unauthenticated' do
@@ -518,8 +532,9 @@ describe API::API, api: true  do
 
     it "does not create invalid email" do
       post api("/users/#{user.id}/emails", admin), {}
+
       expect(response).to have_http_status(400)
-      expect(json_response['message']).to eq('400 (Bad request) "email" not given')
+      expect(json_response['error']).to eq('email is missing')
     end
 
     it "creates email" do
@@ -536,7 +551,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe 'GET /user/:uid/emails' do
+  describe 'GET /user/:id/emails' do
     before { admin }
 
     context 'when unauthenticated' do
@@ -570,7 +585,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe 'DELETE /user/:uid/emails/:id' do
+  describe 'DELETE /user/:id/emails/:email_id' do
     before { admin }
 
     context 'when unauthenticated' do
@@ -685,7 +700,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe "GET /user/keys/:id" do
+  describe "GET /user/keys/:key_id" do
     it "returns single key" do
       user.keys << key
       user.save
@@ -698,7 +713,7 @@ describe API::API, api: true  do
       get api("/user/keys/42", user)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 Not found')
+      expect(json_response['message']).to eq('404 Key Not Found')
     end
 
     it "returns 404 error if admin accesses user's ssh key" do
@@ -707,7 +722,7 @@ describe API::API, api: true  do
       admin
       get api("/user/keys/#{key.id}", admin)
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 Not found')
+      expect(json_response['message']).to eq('404 Key Not Found')
     end
 
     it "returns 404 for invalid ID" do
@@ -733,14 +748,16 @@ describe API::API, api: true  do
 
     it "does not create ssh key without key" do
       post api("/user/keys", user), title: 'title'
+
       expect(response).to have_http_status(400)
-      expect(json_response['message']).to eq('400 (Bad request) "key" not given')
+      expect(json_response['error']).to eq('key is missing')
     end
 
     it 'does not create ssh key without title' do
       post api('/user/keys', user), key: 'some key'
+
       expect(response).to have_http_status(400)
-      expect(json_response['message']).to eq('400 (Bad request) "title" not given')
+      expect(json_response['error']).to eq('title is missing')
     end
 
     it "does not create ssh key without title" do
@@ -749,7 +766,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe "DELETE /user/keys/:id" do
+  describe "DELETE /user/keys/:key_id" do
     it "deletes existed key" do
       user.keys << key
       user.save
@@ -759,9 +776,11 @@ describe API::API, api: true  do
       expect(response).to have_http_status(200)
     end
 
-    it "returns success if key ID not found" do
+    it "returns 404 if key ID not found" do
       delete api("/user/keys/42", user)
-      expect(response).to have_http_status(200)
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 Key Not Found')
     end
 
     it "returns 401 error if unauthorized" do
@@ -798,7 +817,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe "GET /user/emails/:id" do
+  describe "GET /user/emails/:email_id" do
     it "returns single email" do
       user.emails << email
       user.save
@@ -810,7 +829,7 @@ describe API::API, api: true  do
     it "returns 404 Not Found within invalid ID" do
       get api("/user/emails/42", user)
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 Not found')
+      expect(json_response['message']).to eq('404 Email Not Found')
     end
 
     it "returns 404 error if admin accesses user's email" do
@@ -819,7 +838,7 @@ describe API::API, api: true  do
       admin
       get api("/user/emails/#{email.id}", admin)
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 Not found')
+      expect(json_response['message']).to eq('404 Email Not Found')
     end
 
     it "returns 404 for invalid ID" do
@@ -845,12 +864,13 @@ describe API::API, api: true  do
 
     it "does not create email with invalid email" do
       post api("/user/emails", user), {}
+
       expect(response).to have_http_status(400)
-      expect(json_response['message']).to eq('400 (Bad request) "email" not given')
+      expect(json_response['error']).to eq('email is missing')
     end
   end
 
-  describe "DELETE /user/emails/:id" do
+  describe "DELETE /user/emails/:email_id" do
     it "deletes existed email" do
       user.emails << email
       user.save
@@ -860,9 +880,11 @@ describe API::API, api: true  do
       expect(response).to have_http_status(200)
     end
 
-    it "returns success if email ID not found" do
+    it "returns 404 if email ID not found" do
       delete api("/user/emails/42", user)
-      expect(response).to have_http_status(200)
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 Email Not Found')
     end
 
     it "returns 401 error if unauthorized" do
@@ -872,10 +894,10 @@ describe API::API, api: true  do
       expect(response).to have_http_status(401)
     end
 
-    it "returns a 404 for invalid ID" do
-      delete api("/users/emails/ASDF", admin)
+    it "returns 400 for invalid ID" do
+      delete api("/user/emails/ASDF", admin)
 
-      expect(response).to have_http_status(404)
+      expect(response).to have_http_status(400)
     end
   end