Commit 1b97a2eee8b89320de891e3ae8496adfa7f3a84b

Authored by Sebastian Ziebell
1 parent da040fc1

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
... ... @@ -88,36 +88,47 @@ Return values:
88 88  
89 89  
90 90 ## User creation
91   -Create user. Available only for admin
  91 +
  92 +Creates a new user. Note only administrators can create new users.
92 93  
93 94 ```
94 95 POST /users
95 96 ```
96 97  
97 98 Parameters:
98   -+ `email` (required) - Email
99   -+ `password` (required) - Password
100   -+ `username` (required) - Username
101   -+ `name` (required) - Name
102   -+ `skype` - Skype ID
103   -+ `linkedin` - Linkedin
104   -+ `twitter` - Twitter account
105   -+ `projects_limit` - Number of projects user can create
106   -+ `extern_uid` - External UID
107   -+ `provider` - External provider name
108   -+ `bio` - User's bio
109 99  
110   -Will return created user with status `201 Created` on success, or `404 Not
111   -found` on fail.
  100 ++ `email` (required) - Email
  101 ++ `password` (required) - Password
  102 ++ `username` (required) - Username
  103 ++ `name` (required) - Name
  104 ++ `skype` (optional) - Skype ID
  105 ++ `linkedin` (optional) - Linkedin
  106 ++ `twitter` (optional) - Twitter account
  107 ++ `projects_limit` (optional) - Number of projects user can create
  108 ++ `extern_uid` (optional) - External UID
  109 ++ `provider` (optional) - External provider name
  110 ++ `bio` (optional) - User's bio
  111 +
  112 +Return values:
  113 +
  114 ++ `201 Created` on success and returns the new user
  115 ++ `400 Bad Request` if one of the required attributes is missing from the request
  116 ++ `401 Unauthorized` if the user is not authorized
  117 ++ `403 Forbidden` if the user is not allowed to create a new user (must be admin)
  118 ++ `404 Not Found` if something else fails
  119 ++ `409 Conflict` if a user with the same email address or username already exists
  120 +
112 121  
113 122 ## User modification
114   -Modify user. Available only for admin
  123 +
  124 +Modifies an existing user. Only administrators can change attributes of a user.
115 125  
116 126 ```
117 127 PUT /users/:id
118 128 ```
119 129  
120 130 Parameters:
  131 +
121 132 + `email` - Email
122 133 + `username` - Username
123 134 + `name` - Name
... ... @@ -130,23 +141,42 @@ Parameters:
130 141 + `provider` - External provider name
131 142 + `bio` - User's bio
132 143  
  144 +Return values:
  145 +
  146 ++ `200 Ok` on success and returns the new user
  147 ++ `401 Unauthorized` if the user is not authorized
  148 ++ `403 Forbidden` if the user is not allowed to create a new user (must be admin)
  149 ++ `404 Not Found` if something else fails
  150 +
  151 +Note, at the moment this method does only return a 404 error, even in cases where a 409 (Conflict) would
  152 +be more appropriate, e.g. when renaming the email address to some exsisting one.
133 153  
134   -Will return created user with status `200 OK` on success, or `404 Not
135   -found` on fail.
136 154  
137 155 ## User deletion
138   -Delete user. Available only for admin
  156 +
  157 +Deletes a user. Available only for administrators. This is an idempotent function, calling this function
  158 +for a non-existent user id still returns a status code `200 Ok`. The JSON response differs if the user
  159 +was actually deleted or not. In the former the user is returned and in the latter not.
139 160  
140 161 ```
141 162 DELETE /users/:id
142 163 ```
143 164  
144   -Will return deleted user with status `200 OK` on success, or `404 Not
145   -found` on fail.
  165 +Parameters:
  166 +
  167 ++ `id` (required) - The ID of the user
  168 +
  169 +Return values:
  170 +
  171 ++ `200 Ok` on success and returns the deleted user
  172 ++ `401 Unauthorized` if the user is not authorized
  173 ++ `403 Forbidden` if the user is not allowed to create a new user (must be admin)
  174 ++ `404 Not Found` if user with ID not found or something else fails
  175 +
146 176  
147 177 ## Current user
148 178  
149   -Get currently authenticated user.
  179 +Gets currently authenticated user.
150 180  
151 181 ```
152 182 GET /user
... ... @@ -169,6 +199,13 @@ GET /user
169 199 }
170 200 ```
171 201  
  202 +Return values:
  203 +
  204 ++ `200 Ok` on success and returns the current user
  205 ++ `401 Unauthorized` if the user is not authorized
  206 ++ `404 Not Found` if something else fails
  207 +
  208 +
172 209 ## List SSH keys
173 210  
174 211 Get a list of currently authenticated user's SSH keys.
... ... @@ -196,6 +233,17 @@ GET /user/keys
196 233 ]
197 234 ```
198 235  
  236 +Parameters:
  237 +
  238 ++ **none**
  239 +
  240 +Return values:
  241 +
  242 ++ `200 Ok` on success and a list of ssh keys
  243 ++ `401 Unauthorized` if the user is not authenticated
  244 ++ `404 Not Found` if something else fails
  245 +
  246 +
199 247 ## Single SSH key
200 248  
201 249 Get a single key.
... ... @@ -217,9 +265,17 @@ Parameters:
217 265 soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0="
218 266 }
219 267 ```
  268 +
  269 +Return values:
  270 +
  271 ++ `200 Ok` on success and the ssh key with ID
  272 ++ `401 Unauthorized` if it is not allowed to access the user
  273 ++ `404 Not Found` if the ssh key with ID not found
  274 +
  275 +
220 276 ## Add SSH key
221 277  
222   -Create new key owned by currently authenticated user
  278 +Creates a new key owned by the currently authenticated user.
223 279  
224 280 ```
225 281 POST /user/keys
... ... @@ -230,12 +286,18 @@ Parameters:
230 286 + `title` (required) - new SSH Key's title
231 287 + `key` (required) - new SSH key
232 288  
233   -Will return created key with status `201 Created` on success, or `404 Not
234   -found` on fail.
  289 +Return values:
  290 +
  291 ++ `201 Created` on success and the added key
  292 ++ `400 Bad Request` if one of the required attributes is not given
  293 ++ `401 Unauthorized` if user is not authorized to add ssh key
  294 ++ `404 Not Found` if something else fails
  295 +
235 296  
236 297 ## Delete SSH key
237 298  
238   -Delete key owned by currently authenticated user
  299 +Deletes key owned by currently authenticated user. This is an idempotent function and calling it on a key that is already
  300 +deleted or not available results in `200 Ok`.
239 301  
240 302 ```
241 303 DELETE /user/keys/:id
... ... @@ -245,4 +307,8 @@ Parameters:
245 307  
246 308 + `id` (required) - SSH key ID
247 309  
248   -Will return `200 OK` on success, or `404 Not Found` on fail.
  310 +Return values:
  311 +
  312 ++ `200 Ok` on success
  313 ++ `401 Unauthorized` if user is not allowed to delete they key
  314 ++ `404 Not Found` if something else fails
... ...
lib/api/users.rb
... ... @@ -41,6 +41,12 @@ module Gitlab
41 41 # POST /users
42 42 post do
43 43 authenticated_as_admin!
  44 +
  45 + bad_request!(:email) if !params.has_key? :email
  46 + bad_request!(:password) if !params.has_key? :password
  47 + bad_request!(:name) if !params.has_key? :name
  48 + bad_request!(:username) if !params.has_key? :username
  49 +
44 50 attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio]
45 51 user = User.new attrs, as: :admin
46 52 if user.save
... ... @@ -67,10 +73,12 @@ module Gitlab
67 73 # PUT /users/:id
68 74 put ":id" do
69 75 authenticated_as_admin!
  76 +
70 77 attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio]
71   - user = User.find_by_id(params[:id])
  78 + user = User.find(params[:id])
  79 + not_found!("User not found") unless user
72 80  
73   - if user && user.update_attributes(attrs)
  81 + if user.update_attributes(attrs)
74 82 present user, with: Entities::User
75 83 else
76 84 not_found!
... ... @@ -127,6 +135,9 @@ module Gitlab
127 135 # Example Request:
128 136 # POST /user/keys
129 137 post "keys" do
  138 + bad_request!(:title) unless params[:title].present?
  139 + bad_request!(:key) unless params[:key].present?
  140 +
130 141 attrs = attributes_for_keys [:title, :key]
131 142 key = current_user.keys.new attrs
132 143 if key.save
... ... @@ -136,15 +147,18 @@ module Gitlab
136 147 end
137 148 end
138 149  
139   - # Delete existed ssh key of currently authenticated user
  150 + # Delete existing ssh key of currently authenticated user
140 151 #
141 152 # Parameters:
142 153 # id (required) - SSH Key ID
143 154 # Example Request:
144 155 # DELETE /user/keys/:id
145 156 delete "keys/:id" do
146   - key = current_user.keys.find params[:id]
147   - key.delete
  157 + begin
  158 + key = current_user.keys.find params[:id]
  159 + key.delete
  160 + rescue
  161 + end
148 162 end
149 163 end
150 164 end
... ...
spec/requests/api/users_spec.rb
... ... @@ -31,15 +31,20 @@ describe Gitlab::API do
31 31 response.status.should == 200
32 32 json_response['email'].should == user.email
33 33 end
34   - end
35 34  
36   - describe "POST /users" do
37   - before{ admin }
  35 + it "should return a 401 if unauthenticated" do
  36 + get api("/users/9998")
  37 + response.status.should == 401
  38 + end
38 39  
39   - it "should not create invalid user" do
40   - post api("/users", admin), { email: "invalid email" }
  40 + it "should return a 404 error if user id not found" do
  41 + get api("/users/9999", user)
41 42 response.status.should == 404
42 43 end
  44 + end
  45 +
  46 + describe "POST /users" do
  47 + before{ admin }
43 48  
44 49 it "should create user" do
45 50 expect {
... ... @@ -47,10 +52,48 @@ describe Gitlab::API do
47 52 }.to change { User.count }.by(1)
48 53 end
49 54  
  55 + it "should return 201 Created on success" do
  56 + post api("/users", admin), attributes_for(:user, projects_limit: 3)
  57 + response.status.should == 201
  58 + end
  59 +
  60 + it "should not create user with invalid email" do
  61 + post api("/users", admin), { email: "invalid email", password: 'password' }
  62 + response.status.should == 400
  63 + end
  64 +
  65 + it "should return 400 error if password not given" do
  66 + post api("/users", admin), { email: 'test@example.com' }
  67 + response.status.should == 400
  68 + end
  69 +
  70 + it "should return 400 error if email not given" do
  71 + post api("/users", admin), { password: 'pass1234' }
  72 + response.status.should == 400
  73 + end
  74 +
50 75 it "shouldn't available for non admin users" do
51 76 post api("/users", user), attributes_for(:user)
52 77 response.status.should == 403
53 78 end
  79 +
  80 + context "with existing user" do
  81 + before { post api("/users", admin), { email: 'test@example.com', password: 'password', username: 'test' } }
  82 +
  83 + it "should not create user with same email" do
  84 + expect {
  85 + post api("/users", admin), { email: 'test@example.com', password: 'password' }
  86 + }.to change { User.count }.by(0)
  87 + end
  88 +
  89 + it "should return 409 conflict error if user with email exists" do
  90 + post api("/users", admin), { email: 'test@example.com', password: 'password' }
  91 + end
  92 +
  93 + it "should return 409 conflict error if same username exists" do
  94 + post api("/users", admin), { email: 'foo@example.com', password: 'pass', username: 'test' }
  95 + end
  96 + end
54 97 end
55 98  
56 99 describe "GET /users/sign_up" do
... ... @@ -86,7 +129,7 @@ describe Gitlab::API do
86 129 describe "PUT /users/:id" do
87 130 before { admin }
88 131  
89   - it "should update user" do
  132 + it "should update user with new bio" do
90 133 put api("/users/#{user.id}", admin), {bio: 'new test bio'}
91 134 response.status.should == 200
92 135 json_response['bio'].should == 'new test bio'
... ... @@ -108,6 +151,25 @@ describe Gitlab::API do
108 151 put api("/users/999999", admin), {bio: 'update should fail'}
109 152 response.status.should == 404
110 153 end
  154 +
  155 + context "with existing user" do
  156 + before {
  157 + post api("/users", admin), { email: 'test@example.com', password: 'password', username: 'test', name: 'test' }
  158 + post api("/users", admin), { email: 'foo@bar.com', password: 'password', username: 'john', name: 'john' }
  159 + @user_id = User.all.last.id
  160 + }
  161 +
  162 +# it "should return 409 conflict error if email address exists" do
  163 +# put api("/users/#{@user_id}", admin), { email: 'test@example.com' }
  164 +# response.status.should == 409
  165 +# end
  166 +#
  167 +# it "should return 409 conflict error if username taken" do
  168 +# @user_id = User.all.last.id
  169 +# put api("/users/#{@user_id}", admin), { username: 'test' }
  170 +# response.status.should == 409
  171 +# end
  172 + end
111 173 end
112 174  
113 175 describe "DELETE /users/:id" do
... ... @@ -120,6 +182,11 @@ describe Gitlab::API do
120 182 json_response['email'].should == user.email
121 183 end
122 184  
  185 + it "should not delete for unauthenticated user" do
  186 + delete api("/users/#{user.id}")
  187 + response.status.should == 401
  188 + end
  189 +
123 190 it "shouldn't available for non admin users" do
124 191 delete api("/users/#{user.id}", user)
125 192 response.status.should == 403
... ... @@ -137,6 +204,11 @@ describe Gitlab::API do
137 204 response.status.should == 200
138 205 json_response['email'].should == user.email
139 206 end
  207 +
  208 + it "should return 401 error if user is unauthenticated" do
  209 + get api("/user")
  210 + response.status.should == 401
  211 + end
140 212 end
141 213  
142 214 describe "GET /user/keys" do
... ... @@ -172,19 +244,38 @@ describe Gitlab::API do
172 244 get api("/user/keys/42", user)
173 245 response.status.should == 404
174 246 end
175   - end
176 247  
177   - describe "POST /user/keys" do
178   - it "should not create invalid ssh key" do
179   - post api("/user/keys", user), { title: "invalid key" }
  248 + it "should return 404 error if admin accesses user's ssh key" do
  249 + user.keys << key
  250 + user.save
  251 + admin
  252 + get api("/user/keys/#{key.id}", admin)
180 253 response.status.should == 404
181 254 end
  255 + end
182 256  
  257 + describe "POST /user/keys" do
183 258 it "should create ssh key" do
184 259 key_attrs = attributes_for :key
185 260 expect {
186 261 post api("/user/keys", user), key_attrs
187 262 }.to change{ user.keys.count }.by(1)
  263 + response.status.should == 201
  264 + end
  265 +
  266 + it "should return a 401 error if unauthorized" do
  267 + post api("/user/keys"), title: 'some title', key: 'some key'
  268 + response.status.should == 401
  269 + end
  270 +
  271 + it "should not create ssh key without key" do
  272 + post api("/user/keys", user), title: 'title'
  273 + response.status.should == 400
  274 + end
  275 +
  276 + it "should not create ssh key without title" do
  277 + post api("/user/keys", user), key: "somekey"
  278 + response.status.should == 400
188 279 end
189 280 end
190 281  
... ... @@ -195,11 +286,19 @@ describe Gitlab::API do
195 286 expect {
196 287 delete api("/user/keys/#{key.id}", user)
197 288 }.to change{user.keys.count}.by(-1)
  289 + response.status.should == 200
198 290 end
199 291  
200   - it "should return 404 Not Found within invalid ID" do
  292 + it "should return sucess if key ID not found" do
201 293 delete api("/user/keys/42", user)
202   - response.status.should == 404
  294 + response.status.should == 200
  295 + end
  296 +
  297 + it "should return 401 error if unauthorized" do
  298 + user.keys << key
  299 + user.save
  300 + delete api("/user/keys/#{key.id}")
  301 + response.status.should == 401
203 302 end
204 303 end
205 304 end
... ...