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

sensuctl cannot delete users #2540

Closed
ccressent opened this issue Dec 15, 2018 · 8 comments
Closed

sensuctl cannot delete users #2540

ccressent opened this issue Dec 15, 2018 · 8 comments
Labels

Comments

@ccressent
Copy link
Contributor

ccressent commented Dec 15, 2018

sensuctl cannot delete users, only disable them.

Expected Behavior

A user delete sub-command lets me completely remove a user from the system.

Current Behavior

There is no user delete sub-command.
All I can do is disable a user.

Additionally, sensuctl user disable shows a confirmation message about deleting a user even though it only disables it.

Possible Solution

  1. Add a user delete sub-command, in line with most (all?) other resource types.
  2. Make a DELETE on /users/{id} actually delete a user instead of disabling it.
  3. Add a /users/{id}/disable endpoint to disable users (inverse of the existing /users/{id}/reinstate).

Steps to Reproduce (for bugs)

  1. sensuctl user create test --password password
  2. Try to completely remove that test user

Your Environment

  • Sensu version: 5.0.1
@echlebek
Copy link
Contributor

There is good reason to implement only soft deletes for users.

  • Provides the ability to associate actions with accounts that are no longer enabled
  • Gives a record of when a particular user account was disabled
  • Prevents creating a new user over-top of an existing deleted user

treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 2, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 2, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 2, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 3, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 4, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 5, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 5, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 6, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 6, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 6, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 6, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 6, 2019
treydock added a commit to treydock/sensu-puppet that referenced this issue Jan 8, 2019
@asachs01
Copy link

asachs01 commented Feb 4, 2019

@echlebek I get the logic for not doing a hard delete, but in my head there's a very clear delineation between "disable" and "delete" and it seems that sensuctl conflates the two. Would it be possible to have an option for both?

@treydock
Copy link
Contributor

treydock commented Feb 4, 2019

Being able to fully delete would be beneficial to the sensu-puppet module. One thing we'd like to support is deleting unmanaged users and the way this is done in Puppet is using code that assumes the resource goes away when purged.

@ghoneycutt
Copy link

There are also times when you really want to delete a user, such as your automation script that went sideways and added a ton of bogus users. Or when you screw up a user and want to start from scratch. Disabling is not deleting. This will also impact organizations where the username might be recycled and re-enabling is giving permissions the new user should not have.

@calebhailey calebhailey added the component:sensuctl Sensu CLI improvements label May 6, 2020
@stale
Copy link

stale bot commented Nov 2, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 2, 2020
@echlebek
Copy link
Contributor

echlebek commented Nov 2, 2020

@ccressent as the original filer, do you have any thoughts on this, now that's it been a while?

@stale stale bot removed the wontfix label Nov 2, 2020
@ccressent
Copy link
Contributor Author

I skimmed through the comments above, and my position is that this is still a valid issue given the number of thumb up reactions and more importantly for conceptual reasons that Aaron hinted at: disabling and deleting are 2 different things.

Eric raises concerns that are justified, but they can be alleviated:

There is good reason to implement only soft deletes for users.

  • Provides the ability to associate actions with accounts that are no longer enabled

Internally, actions should be associated with a unique ID that represents the user rather than a name. Names can change; IDs never change and are never deleted.

  • Gives a record of when a particular user account was disabled
  • Prevents creating a new user over-top of an existing deleted user

With a system based on unique IDs, there is no problem with homonyms because the name is not what really identifies a user; the ID is. That let's you "delete" a user by removing the name <-> ID association, while still keeping the correct context in your logs (the user ID).

Would it be easy to implement those ideas in Sensu? I don't know. The real question in my mind is "Is it worth it?". But at this stage I'd argue we are in the territory of product decisions rather than engineering decisions :)

@calebhailey
Copy link

Closing this due to inactivity, and because of reasons stated above which still hold true.

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

No branches or pull requests

6 participants