Skip to content

[#5356] Delete GPG keys with user deletion #5357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[#5356] Delete GPG keys with user deletion #5357

wants to merge 1 commit into from

Conversation

JohnnyLeone
Copy link

Had the same issue with my instance. The following code fixed it for me, it extends the actual behavior of the existing functions.

@JohnnyLeone JohnnyLeone changed the title [5356] Delete GPG keys with user deletion [#5356] Delete GPG keys with user deletion Nov 18, 2018
@codecov-io
Copy link

Codecov Report

Merging #5357 into master will increase coverage by 0.01%.
The diff coverage is 35.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5357      +/-   ##
==========================================
+ Coverage   37.35%   37.36%   +0.01%     
==========================================
  Files         312      312              
  Lines       46436    46453      +17     
==========================================
+ Hits        17344    17355      +11     
- Misses      26604    26608       +4     
- Partials     2488     2490       +2
Impacted Files Coverage Δ
models/user.go 44.37% <18.18%> (-0.28%) ⬇️
models/gpg_key.go 55.45% <66.66%> (+0.21%) ⬆️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

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 e08c7e5...1730048. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 18, 2018
return nil
}

_, err := e.In("id", keyIDs).Delete(new(GPGKey))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just e.Where("owner_id =?", userID).Delete(new(GPGKey))?

Copy link
Author

@JohnnyLeone JohnnyLeone Nov 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice solution and makes this code a lot smaller. Also this check can simplify the public key deletion.

Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor comment or change needed. Otherwise LGTM.

@@ -239,6 +239,16 @@ func parseGPGKey(ownerID int64, e *openpgp.Entity) (*GPGKey, error) {
}, nil
}

// deleteGPGKeys does the actual key deletion but does not update authorized_keys file.
Copy link
Member

@sapk sapk Nov 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add for information that this method will not delete any related (subkey) compared to deleteGPGKey ? And you should loose the authorized_keys file part since it not related to GPG.
Something like :

Suggested change
// deleteGPGKeys does the actual key deletion but does not update authorized_keys file.
// deleteGPGKeys does the actual key deletion but does not delete any related key (subkey).

or add :
_, err := e.In("primary_key_id", keyIDs).Delete(new(GPGKey))
each solution is fine as the list passed should already contained any related keys in the original use case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But deleteGPGKeys does also delete all subkeys of a master key which are assigned to a user. More specifically: deleteGPGKeys deletes all keys of a specified user.

Copy link
Member

@sapk sapk Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just in case for later use in other part in code. Currently when you call this method here you pass all keys (and subkeys) owned by user. But if this method is used somewhere else there could be orphaned subkeys if not all the key are passed as argument. This is just to add a warning for later use (or add the delete of any sub key).

@lafriks
Copy link
Member

lafriks commented Nov 19, 2018

I think this also needs migration to delete gpg keys for already deleted users

@zeripath
Copy link
Contributor

Hi @JohnnyLeone thanks for your PR and issue reporting. I think #5429 replaces this, does that solve your issue? If so I think we can close this pr.

@lafriks
Copy link
Member

lafriks commented Dec 27, 2018

@JohnnyLeone can you fix requested changes or you need help with this?

@zeripath
Copy link
Contributor

Isn't this fixed by #5429?

@lafriks
Copy link
Member

lafriks commented Dec 27, 2018

@zeripath yes looks like it is, thanks, closing

@lafriks lafriks closed this Dec 27, 2018
@lafriks lafriks removed this from the 1.7.0 milestone Dec 27, 2018
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants