From 1b97a2eee8b89320de891e3ae8496adfa7f3a84b Mon Sep 17 00:00:00 2001
From: Sebastian Ziebell <sebastian.ziebell@asquera.de>
Date: Wed, 20 Feb 2013 12:10:51 +0100
Subject: [PATCH] API: fixes return codes, documentation updated with status
 codes, tests added

The users API updated with return codes, e.g. if required parameters are missing
a `400 Bad Request` error is returned instead of `404`. Fixes return codes of functions,
e.g. deletion of a ssh key is an idempotent function now.
The API documentation is updated to reflect the current status of the API. Descriptions
are more detailed and complete, infos to return values are added to all functions.
---
 doc/api/users.md                | 118 +++++++++++++++++++++++-------
 lib/api/users.rb                |  26 +++++--
 spec/requests/api/users_spec.rb | 125 ++++++++++++++++++++++++++++----
 3 files changed, 224 insertions(+), 45 deletions(-)

diff --git a/doc/api/users.md b/doc/api/users.md
index e5893638..96aebffa 100644
--- a/doc/api/users.md
+++ b/doc/api/users.md
@@ -88,36 +88,47 @@ Return values:
 
 
 ## User creation
-Create user. Available only for admin
+
+Creates a new user. Note only administrators can create new users.
 
 ```
 POST /users
 ```
 
 Parameters:
-+ `email` (required)                  - Email
-+ `password` (required)               - Password
-+ `username` (required)               - Username
-+ `name` (required)                   - Name
-+ `skype`                             - Skype ID
-+ `linkedin`                          - Linkedin
-+ `twitter`                           - Twitter account
-+ `projects_limit`                    - Number of projects user can create
-+ `extern_uid`                        - External UID
-+ `provider`                          - External provider name
-+ `bio`                               - User's bio
 
-Will return created user with status `201 Created` on success, or `404 Not
-found` on fail.
++ `email` (required)          - Email
++ `password` (required)       - Password
++ `username` (required)       - Username
++ `name` (required)           - Name
++ `skype` (optional)          - Skype ID
++ `linkedin` (optional)       - Linkedin
++ `twitter` (optional)        - Twitter account
++ `projects_limit` (optional) - Number of projects user can create
++ `extern_uid` (optional)     - External UID
++ `provider` (optional)       - External provider name
++ `bio` (optional)            - User's bio
+
+Return values:
+
++ `201 Created` on success and returns the new user
++ `400 Bad Request` if one of the required attributes is missing from the request
++ `401 Unauthorized` if the user is not authorized
++ `403 Forbidden` if the user is not allowed to create a new user (must be admin)
++ `404 Not Found` if something else fails
++ `409 Conflict` if a user with the same email address or username already exists
+
 
 ## User modification
-Modify user. Available only for admin
+
+Modifies an existing user. Only administrators can change attributes of a user.
 
 ```
 PUT /users/:id
 ```
 
 Parameters:
+
 + `email`                             - Email
 + `username`                          - Username
 + `name`                              - Name
@@ -130,23 +141,42 @@ Parameters:
 + `provider`                          - External provider name
 + `bio`                               - User's bio
 
+Return values:
+
++ `200 Ok` on success and returns the new user
++ `401 Unauthorized` if the user is not authorized
++ `403 Forbidden` if the user is not allowed to create a new user (must be admin)
++ `404 Not Found` if something else fails
+
+Note, at the moment this method does only return a 404 error, even in cases where a 409 (Conflict) would
+be more appropriate, e.g. when renaming the email address to some exsisting one.
 
-Will return created user with status `200 OK` on success, or `404 Not
-found` on fail.
 
 ## User deletion
-Delete user. Available only for admin
+
+Deletes a user. Available only for administrators. This is an idempotent function, calling this function
+for a non-existent user id still returns a status code `200 Ok`. The JSON response differs if the user
+was actually deleted or not. In the former the user is returned and in the latter not.
 
 ```
 DELETE /users/:id
 ```
 
-Will return deleted user with status `200 OK` on success, or `404 Not
-found` on fail.
+Parameters:
+
++ `id` (required) - The ID of the user
+
+Return values:
+
++ `200 Ok` on success and returns the deleted user
++ `401 Unauthorized` if the user is not authorized
++ `403 Forbidden` if the user is not allowed to create a new user (must be admin)
++ `404 Not Found` if user with ID not found or something else fails
+
 
 ## Current user
 
-Get currently authenticated user.
+Gets currently authenticated user.
 
 ```
 GET /user
@@ -169,6 +199,13 @@ GET /user
 }
 ```
 
+Return values:
+
++ `200 Ok` on success and returns the current user
++ `401 Unauthorized` if the user is not authorized
++ `404 Not Found` if something else fails
+
+
 ## List SSH keys
 
 Get a list of currently authenticated user's SSH keys.
@@ -196,6 +233,17 @@ GET /user/keys
 ]
 ```
 
+Parameters:
+
++ **none**
+
+Return values:
+
++ `200 Ok` on success and a list of ssh keys
++ `401 Unauthorized` if the user is not authenticated
++ `404 Not Found` if something else fails
+
+
 ## Single SSH key
 
 Get a single key.
@@ -217,9 +265,17 @@ Parameters:
       soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0="
 }
 ```
+
+Return values:
+
++ `200 Ok` on success and the ssh key with ID
++ `401 Unauthorized` if it is not allowed to access the user
++ `404 Not Found` if the ssh key with ID not found
+
+
 ## Add SSH key
 
-Create new key owned by currently authenticated user
+Creates a new key owned by the currently authenticated user.
 
 ```
 POST /user/keys
@@ -230,12 +286,18 @@ Parameters:
 + `title` (required) - new SSH Key's title
 + `key` (required) - new SSH key
 
-Will return created key with status `201 Created` on success, or `404 Not
-found` on fail.
+Return values:
+
++ `201 Created` on success and the added key
++ `400 Bad Request` if one of the required attributes is not given
++ `401 Unauthorized` if user is not authorized to add ssh key
++ `404 Not Found` if something else fails
+
 
 ## Delete SSH key
 
-Delete key owned by currently authenticated user
+Deletes key owned by currently authenticated user. 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
@@ -245,4 +307,8 @@ Parameters:
 
 + `id` (required) - SSH key ID
 
-Will return `200 OK` on success, or `404 Not Found` on fail.
+Return values:
+
++ `200 Ok` on success
++ `401 Unauthorized` if user is not allowed to delete they key
++ `404 Not Found` if something else fails
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 7ea90c75..b9dce58a 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -41,6 +41,12 @@ module Gitlab
       #   POST /users
       post do
         authenticated_as_admin!
+
+        bad_request!(:email)    if !params.has_key? :email
+        bad_request!(:password) if !params.has_key? :password
+        bad_request!(:name)     if !params.has_key? :name
+        bad_request!(:username) if !params.has_key? :username
+
         attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio]
         user = User.new attrs, as: :admin
         if user.save
@@ -67,10 +73,12 @@ module Gitlab
       #   PUT /users/:id
       put ":id" do
         authenticated_as_admin!
-        attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio]
-        user = User.find_by_id(params[:id])
 
-        if user && user.update_attributes(attrs)
+        attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio]
+        user = User.find(params[:id])
+        not_found!("User not found") unless user
+
+        if user.update_attributes(attrs)
           present user, with: Entities::User
         else
           not_found!
@@ -127,6 +135,9 @@ module Gitlab
       # Example Request:
       #   POST /user/keys
       post "keys" do
+        bad_request!(:title) unless params[:title].present?
+        bad_request!(:key) unless params[:key].present?
+
         attrs = attributes_for_keys [:title, :key]
         key = current_user.keys.new attrs
         if key.save
@@ -136,15 +147,18 @@ module Gitlab
         end
       end
 
-      # Delete existed ssh key of currently authenticated user
+      # Delete existing ssh key of currently authenticated user
       #
       # Parameters:
       #   id (required) - SSH Key ID
       # Example Request:
       #   DELETE /user/keys/:id
       delete "keys/:id" do
-        key = current_user.keys.find params[:id]
-        key.delete
+        begin
+          key = current_user.keys.find params[:id]
+          key.delete
+        rescue
+        end
       end
     end
   end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 1645117e..b0cf1265 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -31,26 +31,69 @@ describe Gitlab::API do
       response.status.should == 200
       json_response['email'].should == user.email
     end
+
+    it "should return a 401 if unauthenticated" do
+      get api("/users/9998")
+      response.status.should == 401
+    end
+
+    it "should return a 404 error if user id not found" do
+      get api("/users/9999", user)
+      response.status.should == 404
+    end
   end
 
   describe "POST /users" do
     before{ admin }
 
-    it "should not create invalid user" do
-      post api("/users", admin), { email: "invalid email" }
-      response.status.should == 404
-    end
-
     it "should create user" do
       expect {
         post api("/users", admin), attributes_for(:user, projects_limit: 3)
       }.to change { User.count }.by(1)
     end
 
+    it "should return 201 Created on success" do
+      post api("/users", admin), attributes_for(:user, projects_limit: 3)
+      response.status.should == 201
+    end
+
+    it "should not create user with invalid email" do
+      post api("/users", admin), { email: "invalid email", password: 'password' }
+      response.status.should == 400
+    end
+
+    it "should return 400 error if password not given" do
+      post api("/users", admin), { email: 'test@example.com' }
+      response.status.should == 400
+    end
+
+    it "should return 400 error if email not given" do
+      post api("/users", admin), { password: 'pass1234' }
+      response.status.should == 400
+    end
+
     it "shouldn't available for non admin users" do
       post api("/users", user), attributes_for(:user)
       response.status.should == 403
     end
+
+    context "with existing user" do
+      before { post api("/users", admin), { email: 'test@example.com', password: 'password', username: 'test' } }
+
+      it "should not create user with same email" do
+        expect {
+          post api("/users", admin), { email: 'test@example.com', password: 'password' }
+        }.to change { User.count }.by(0)
+      end
+
+      it "should return 409 conflict error if user with email exists" do
+        post api("/users", admin), { email: 'test@example.com', password: 'password' }
+      end
+
+      it "should return 409 conflict error if same username exists" do
+        post api("/users", admin), { email: 'foo@example.com', password: 'pass', username: 'test' }
+      end
+    end
   end
 
   describe "GET /users/sign_up" do
@@ -86,7 +129,7 @@ describe Gitlab::API do
   describe "PUT /users/:id" do
     before { admin }
 
-    it "should update user" do
+    it "should update user with new bio" do
       put api("/users/#{user.id}", admin), {bio: 'new test bio'}
       response.status.should == 200
       json_response['bio'].should == 'new test bio'
@@ -108,6 +151,25 @@ describe Gitlab::API do
       put api("/users/999999", admin), {bio: 'update should fail'}
       response.status.should == 404
     end
+
+    context "with existing user" do
+      before {
+        post api("/users", admin), { email: 'test@example.com', password: 'password', username: 'test', name: 'test' }
+        post api("/users", admin), { email: 'foo@bar.com', password: 'password', username: 'john', name: 'john' }
+        @user_id = User.all.last.id
+      }
+
+#      it "should return 409 conflict error if email address exists" do
+#        put api("/users/#{@user_id}", admin), { email: 'test@example.com' }
+#        response.status.should == 409
+#      end
+#
+#      it "should return 409 conflict error if username taken" do
+#        @user_id = User.all.last.id
+#        put api("/users/#{@user_id}", admin), { username: 'test' }
+#        response.status.should == 409
+#      end
+    end
   end
 
   describe "DELETE /users/:id" do
@@ -120,6 +182,11 @@ describe Gitlab::API do
       json_response['email'].should == user.email
     end
 
+    it "should not delete for unauthenticated user" do
+      delete api("/users/#{user.id}")
+      response.status.should == 401
+    end
+
     it "shouldn't available for non admin users" do
       delete api("/users/#{user.id}", user)
       response.status.should == 403
@@ -137,6 +204,11 @@ describe Gitlab::API do
       response.status.should == 200
       json_response['email'].should == user.email
     end
+
+    it "should return 401 error if user is unauthenticated" do
+      get api("/user")
+      response.status.should == 401
+    end
   end
 
   describe "GET /user/keys" do
@@ -172,19 +244,38 @@ describe Gitlab::API do
       get api("/user/keys/42", user)
       response.status.should == 404
     end
+
+    it "should return 404 error if admin accesses user's ssh key" do
+      user.keys << key
+      user.save
+      admin
+      get api("/user/keys/#{key.id}", admin)
+      response.status.should == 404
+    end
   end
 
   describe "POST /user/keys" do
-    it "should not create invalid ssh key" do
-      post api("/user/keys", user), { title: "invalid key" }
-      response.status.should == 404
-    end
-
     it "should create ssh key" do
       key_attrs = attributes_for :key
       expect {
         post api("/user/keys", user), key_attrs
       }.to change{ user.keys.count }.by(1)
+      response.status.should == 201
+    end
+
+    it "should return a 401 error if unauthorized" do
+      post api("/user/keys"), title: 'some title', key: 'some key'
+      response.status.should == 401
+    end
+
+    it "should not create ssh key without key" do
+      post api("/user/keys", user), title: 'title'
+      response.status.should == 400
+    end
+
+    it "should not create ssh key without title" do
+      post api("/user/keys", user), key: "somekey"
+      response.status.should == 400
     end
   end
 
@@ -195,11 +286,19 @@ describe Gitlab::API do
       expect {
         delete api("/user/keys/#{key.id}", user)
       }.to change{user.keys.count}.by(-1)
+      response.status.should == 200
     end
 
-    it "should return 404 Not Found within invalid ID" do
+    it "should return sucess if key ID not found" do
       delete api("/user/keys/42", user)
-      response.status.should == 404
+      response.status.should == 200
+    end
+
+    it "should return 401 error if unauthorized" do
+      user.keys << key
+      user.save
+      delete api("/user/keys/#{key.id}")
+      response.status.should == 401
     end
   end
 end