Commit 0624d6a7 authored by Rémy Coutable's avatar Rémy Coutable Committed by Robert Speicher

Merge branch 'fix-project-hook-delete-permissions' into 'master'

Prevent users from deleting Webhooks via API they do not own

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15576

See merge request !1959
parent d2d36d5e
...@@ -18,7 +18,6 @@ v 8.7.1 (unreleased) ...@@ -18,7 +18,6 @@ v 8.7.1 (unreleased)
- Fix .gitlab-ci.yml parsing issue when hidde job is a template without script definition. !3849 - Fix .gitlab-ci.yml parsing issue when hidde job is a template without script definition. !3849
- Fix license detection to detect all license files, not only known licenses. !3878 - Fix license detection to detect all license files, not only known licenses. !3878
- Use the `can?` helper instead of `current_user.can?`. !3882 - Use the `can?` helper instead of `current_user.can?`. !3882
- Prevent users from deleting Webhooks via API they do not own
- Fix Error 500 due to stale cache when projects are renamed or transferred - Fix Error 500 due to stale cache when projects are renamed or transferred
- Fix .gitlab-ci.yml parsing issue when hidde job is a template without script definition - Fix .gitlab-ci.yml parsing issue when hidde job is a template without script definition
......
...@@ -103,10 +103,10 @@ module API ...@@ -103,10 +103,10 @@ module API
required_attributes! [:hook_id] required_attributes! [:hook_id]
begin begin
@hook = ProjectHook.find(params[:hook_id]) @hook = user_project.hooks.destroy(params[:hook_id])
@hook.destroy
rescue rescue
# ProjectHook can raise Error if hook_id not found # ProjectHook can raise Error if hook_id not found
not_found!("Error deleting hook #{params[:hook_id]}")
end end
end end
end end
......
...@@ -148,14 +148,24 @@ describe API::API, 'ProjectHooks', api: true do ...@@ -148,14 +148,24 @@ describe API::API, 'ProjectHooks', api: true do
expect(response.status).to eq(200) expect(response.status).to eq(200)
end end
it "should return success when deleting non existent hook" do it "should return a 404 error when deleting non existent hook" do
delete api("/projects/#{project.id}/hooks/42", user) delete api("/projects/#{project.id}/hooks/42", user)
expect(response.status).to eq(200) expect(response.status).to eq(404)
end end
it "should return a 405 error if hook id not given" do it "should return a 405 error if hook id not given" do
delete api("/projects/#{project.id}/hooks", user) delete api("/projects/#{project.id}/hooks", user)
expect(response.status).to eq(405) expect(response.status).to eq(405)
end end
it "shold return a 404 if a user attempts to delete project hooks he/she does not own" do
test_user = create(:user)
other_project = create(:project)
other_project.team << [test_user, :master]
delete api("/projects/#{other_project.id}/hooks/#{hook.id}", test_user)
expect(response.status).to eq(404)
expect(WebHook.exists?(hook.id)).to be_truthy
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment