Don't crash when removing a user that's not project member

The attempt to revoke project access for a user that was not member of the project results in a 500 Internal Server error where it actually should result in a 200 OK since after the operation, the user is not member of the project. This turns the operation into an idempotent call that can be repeated with no ill effects. Updated the spec and changed the code accordingly. However, the result differs slightly, as we can't return the users project access level if the user was not member. I'm not aware if anybody relies on the result of this call. Fixes #2832

Don't crash when removing a user that's not project member
The attempt to revoke project access for a user that was not member of the project results in a 500 Internal Server error where it actually should result in a 200 OK since after the operation, the user is not member of the project. This turns the operation into an idempotent call that can be repeated with no ill effects. Updated the spec and changed the code accordingly. However, the result differs slightly, as we can't return the users project access level if the user was not member. I'm not aware if anybody relies on the result of this call. Fixes #2832
Felix Gilcher
1 parent c72910a8
Showing 2 changed files with 16 additions and 1 deletions Show diff stats
lib/api/projects.rb
spec/requests/api/projects_spec.rb
@@ -132,7 +132,11 @@ module Gitlab
       delete ":id/members/:user_id" do
         authorize! :admin_project, user_project
         users_project = user_project.users_projects.find_by_user_id params[:user_id]
-        users_project.destroy
+        unless users_project.nil?
+          users_project.destroy
+        else
+          {:message => "Access revoked", :id => params[:user_id].to_i}
+        end
       end
  
       # Get project hooks
@@ -167,6 +167,17 @@ describe Gitlab::API do
     end
   end
  
+  describe "DELETE /projects/:id/members/:user_id" do
+    it "should return 200 OK when the user was not member" do
+      expect {
+        delete api("/projects/#{project.id}/members/1000000", user)
+      }.to change { UsersProject.count }.by(0)
+      response.status.should == 200
+      json_response['message'].should == "Access revoked"
+      json_response['id'].should == 1000000
+    end
+  end
+
   describe "GET /projects/:id/hooks" do
     it "should return project hooks" do
       get api("/projects/#{project.id}/hooks", user)
...	...	@@ -132,7 +132,11 @@ module Gitlab
132	132	delete ":id/members/:user_id" do
133	133	authorize! :admin_project, user_project
134	134	users_project = user_project.users_projects.find_by_user_id params[:user_id]
135		- users_project.destroy
	135	+ unless users_project.nil?
	136	+ users_project.destroy
	137	+ else
	138	+ {:message => "Access revoked", :id => params[:user_id].to_i}
	139	+ end
136	140	end
137	141
138	142	# Get project hooks
...	...
...	...	@@ -167,6 +167,17 @@ describe Gitlab::API do
167	167	end
168	168	end
169	169
	170	+ describe "DELETE /projects/:id/members/:user_id" do
	171	+ it "should return 200 OK when the user was not member" do
	172	+ expect {
	173	+ delete api("/projects/#{project.id}/members/1000000", user)
	174	+ }.to change { UsersProject.count }.by(0)
	175	+ response.status.should == 200
	176	+ json_response['message'].should == "Access revoked"
	177	+ json_response['id'].should == 1000000
	178	+ end
	179	+ end
	180	+
170	181	describe "GET /projects/:id/hooks" do
171	182	it "should return project hooks" do
172	183	get api("/projects/#{project.id}/hooks", user)
...	...