Skip to content
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

Adding delete user #524

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Conversation

wizedkyle
Copy link
Contributor

Issue

#507

Description of the Change

Added an endpoint that supports deleting a user from a CNA organisation. This would allow CNAs to remove users from their CNA organisation who may have left their organisation.

DELETE /org/:shortname/user/:username -> CNA Admin or Secretariat

Alternate Designs

The alternate design was to keep leveraging the update user endpoint to disable the user account however, this would lead to CNAs having to ensure the users in the CNA organisation are enabled/disabled correctly.

Possible Drawbacks

None that I think of currently.

Verification Process

I did manual testing as well as created tests to cover the endpoint.

Release Notes

Added support for deleting users from a CNA if the requester is a CNA admin or secretariat.

@mattrbianchi mattrbianchi self-assigned this Oct 29, 2021
@mattrbianchi mattrbianchi self-requested a review October 29, 2021 14:31
@mattrbianchi
Copy link
Contributor

@wizedkyle First, thank you for making the largest outside contribution to date! This is what will help the CVE Services meet community needs as they grow.

The reason we do not initially have a way to delete users is for auditability. A CVE ID currently has a requested_by object that is tied to both the user's organization and them. This very long aggregate query helps show what I'm talking about. We will need a way to preserve auditability (who requested this ID) before we can entertain the option of removing users altogether.

A thought that may come to mind is "why not put the username into the CVE ID and remove the link?". It was discussed at the time that a username itself may be considered PII under GDPR as people tend to use their personal names as usernames. So if we were required to remove the individual's PII from our system, we would have to update every single CVE ID that person requested to be reserved. Or, with the current design, we could redact (through an update) their username! Trade offs, am I right?

I do understand the desire for a more permanent account revocation, but there are inherent data relationship issues we need to work out within a non-relational database (document db) before we can simply delete users. I'd strongly welcome opening a GitHub discussion to talk CVE Services' "schema" / "model" / "document" design.

@wizedkyle
Copy link
Contributor Author

Thank you for the context @mattrbianchi I was slightly concerned there was a caveat like this lying around.

I will open up a discussion around this as I think being able to remove users is beneficial but suffice audibility is maintained.

Also are these the sorts of topics that get raised through the AWG weekly meeting?

@chandanbn
Copy link

Keeping track of who did what and when is important. Users should be marked inactive if they are no longer authorized by their CNA.

Perhaps a user can be safely deleted only if they never had any activity, i.e., no records or IDs reference the user.

@wizedkyle
Copy link
Contributor Author

Definitely agree that being able to audit actions is very important. I am wondering if there is a way to create essentially an audit collection that uses a CVE ID as a primary key and actions against that CVE ID are recorded but this goes more into discussion territory.

Deleting a user who has had no activity is a good idea as that would be beneficial if you accidentally add someone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants