Delete a user's public key via admin api (closes #3014) #3059

vtemian · 2017-12-02T09:44:02Z

Let admin delete a user's key via API

codecov-io · 2017-12-02T10:01:50Z

Codecov Report

Merging #3059 into master will increase coverage by 0.44%.
The diff coverage is 74%.

@@            Coverage Diff             @@
##           master    #3059      +/-   ##
==========================================
+ Coverage   33.63%   34.07%   +0.44%     
==========================================
  Files         273      273              
  Lines       39954    39998      +44     
==========================================
+ Hits        13437    13629     +192     
+ Misses      24610    24429     -181     
- Partials     1907     1940      +33

Impacted Files	Coverage Δ
routers/api/v1/user/key.go	`8.33% <0%> (+4.76%)`	⬆️
models/ssh_key.go	`32.66% <100%> (+20.22%)`	⬆️
routers/api/v1/api.go	`76.34% <100%> (+1.34%)`	⬆️
routers/api/v1/admin/user.go	`23.3% <80%> (+23.3%)`	⬆️
models/repo_indexer.go	`51.98% <0%> (-1%)`	⬇️
models/error.go	`33.63% <0%> (+0.91%)`	⬆️
modules/process/manager.go	`81.15% <0%> (+4.34%)`	⬆️
routers/api/v1/convert/convert.go	`67.8% <0%> (+6.16%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7fb6e3...6bf1fbf. Read the comment docs.

lafriks · 2017-12-02T13:31:45Z

Can you also add test that adds and later deletes that key?
Also test for deleting not existing key and test deleting key with user that does not have rights to do that would be great

vtemian · 2017-12-02T15:25:03Z

@lafriks done!

ethantkoenig

Mostly looks good, just a few comments

ethantkoenig · 2017-12-02T16:42:28Z

routers/api/v1/admin/user.go

+	// - name: id
+	//   in: path
+	//   description: key's id to delete
+	//   type: string


type: integer

ethantkoenig · 2017-12-02T16:42:30Z

routers/api/v1/admin/user.go

+	//   required: true
+	// - name: id
+	//   in: path
+	//   description: key's id to delete


nit: "id of the key to delete"

ethantkoenig · 2017-12-02T16:43:30Z

routers/api/v1/admin/user.go

+func DeleteUserPublicKey(ctx *context.APIContext) {
+	// swagger:operation DELETE /admin/users/{username}/keys/{id} admin adminDeleteUserPublicKey
+	// ---
+	// summary: Delete a user's public key on behalf of a user


nit: "Delete a user's public key". Don't need to repeat "user" twice.

ethantkoenig · 2017-12-02T16:48:53Z

routers/api/v1/admin/user.go

+		return
+	}
+
+	if _, err := models.GetPublicKeyByID(ctx.ParamsInt64(":id")); err != nil {


It would be nice if we didn't have to call GetPublicKeyByID before calling DeletePublicKey, since DeletePublicKey already calls GetPublicKeyByID. Perhaps it's worthwhile to have DeletePublicKey return a ErrKeyNotExist when you try to delete a non-existent key?

Yeap, I wanted to do that, but my concern was that right now DeletePublicKey returns nil if it encounters an ErrKeyNotExist error and maybe, somewhere in the code, somebody is relying on that. (even though is not a sane behavior)

Is it safe to return ErrKeyNotExist ?

That function is used only in one file, so additional checks for that could be added for that error

ethantkoenig · 2017-12-02T16:49:37Z

integrations/api_admin_test.go

+	// user1 is an admin user
+	session := loginUser(t, "user1")
+
+	req := NewRequestf(t, "DELETE", "/api/v1/admin/users/user1/keys/99999")


nit: please use models.NonexistentID instead of 99999

vtemian · 2017-12-04T08:14:51Z

@ethantkoenig done

lafriks · 2017-12-04T13:57:15Z

public/swagger.v1.json

@@ -36,6 +36,7 @@
        ],
        "summary": "Create a user",
        "operationId": "adminCreateUser",
+        "security": null,


Why these changes?

I've run swagger generate spec and this was the output. I didn't want to manually change it since it's easier to maintain this way.

ethantkoenig · 2017-12-06T06:43:55Z

LGTM

lafriks · 2017-12-06T08:19:37Z

LGTM

vtemian changed the title ~~Delete a user's public key via admin api~~ Delete a user's public key via admin api (closes #3014) Dec 2, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 2, 2017

lunny added this to the 1.4.0 milestone Dec 2, 2017

lunny added the modifies/api This PR adds API routes or modifies them label Dec 2, 2017

vtemian force-pushed the delete-user-key-as-admin-3014 branch from f3bea44 to 7d646ed Compare December 2, 2017 13:24

ethantkoenig suggested changes Dec 2, 2017

View reviewed changes

vtemian force-pushed the delete-user-key-as-admin-3014 branch from 565c4b7 to 0e3388f Compare December 2, 2017 18:28

vtemian added 6 commits December 4, 2017 10:13

Delete a user's public key via admin api

046bcbf

Test admin ssh endpoint for creating a new ssh key

53aeb46

Adapt public ssh key test to also test the delete operation

fba98ba

Test that deleting a missing key will result in a 404

c9786a5

Test that a normal user can't delete another user's ssh key

19fa201

Make DeletePublicKey return err

65871ea

vtemian force-pushed the delete-user-key-as-admin-3014 branch from 0e3388f to 65871ea Compare December 4, 2017 08:13

Update swagger doc

02300f5

lafriks requested changes Dec 4, 2017

View reviewed changes

lafriks approved these changes Dec 4, 2017

View reviewed changes

ethantkoenig approved these changes Dec 6, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 6, 2017

lafriks approved these changes Dec 6, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 6, 2017

Merge branch 'master' into delete-user-key-as-admin-3014

6bf1fbf

lafriks merged commit 469ab99 into go-gitea:master Dec 6, 2017

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete a user's public key via admin api (closes #3014) #3059

Delete a user's public key via admin api (closes #3014) #3059

vtemian commented Dec 2, 2017

codecov-io commented Dec 2, 2017 •

edited

lafriks commented Dec 2, 2017

vtemian commented Dec 2, 2017

ethantkoenig left a comment

ethantkoenig Dec 2, 2017

ethantkoenig Dec 2, 2017

ethantkoenig Dec 2, 2017

ethantkoenig Dec 2, 2017

vtemian Dec 2, 2017

lafriks Dec 2, 2017

ethantkoenig Dec 2, 2017

vtemian commented Dec 4, 2017

lafriks Dec 4, 2017

vtemian Dec 4, 2017

ethantkoenig commented Dec 6, 2017

lafriks commented Dec 6, 2017

Delete a user's public key via admin api (closes #3014) #3059

Delete a user's public key via admin api (closes #3014) #3059

Conversation

vtemian commented Dec 2, 2017

codecov-io commented Dec 2, 2017 • edited

Codecov Report

lafriks commented Dec 2, 2017

vtemian commented Dec 2, 2017

ethantkoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtemian commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig commented Dec 6, 2017

lafriks commented Dec 6, 2017

codecov-io commented Dec 2, 2017 •

edited