Support deleting users and groups via authctl

[shiv-tyagi]

Closes #640

This adds subcommands to authctl to allow deleting a user/group using CLI.

This also contains relevant tests to verify the functionality.

[codecov]

Codecov Report

❌ Patch coverage is 52.63158% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.85%. Comparing base (6f0c33d) to head (2922411).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/authctl/group/delete.go	4.76%	20 Missing ⚠️
cmd/authctl/user/delete.go	4.76%	20 Missing ⚠️
internal/users/manager.go	85.71%	3 Missing ⚠️
internal/users/db/groups.go	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1372      +/-   ##
==========================================
- Coverage   86.24%   85.85%   -0.40%     
==========================================
  Files          99      101       +2     
  Lines        6690     6785      +95     
  Branches      111      111              
==========================================
+ Hits         5770     5825      +55     
- Misses        860      904      +44     
+ Partials       60       56       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[shiv-tyagi]

Not sure why it is showing such a less coverage in cmd/authctl/* files. I have added quite some cases which should hit those lines. Please let me know if I am missing something. Thanks!

Edit: I think the reason is that those tests are building authctl and then running it using exec.Command(..) to test stuff... and not directly calling the go code in tests. But that way of testing is pre existing and I am following the same.

[nooreldeenmansour]

Thanks for the PR!

Note that #640 has a sub-issue (#830) around cleaning up broker-stored user data on deletion. Since it's related, it would be good to address it in the same PR.

[nooreldeenmansour]

Thanks for the PR! I did some experimentation and found a few issues worth addressing:

1. Home directory cleanup on user deletion

When a user is deleted, their home directory is left behind. We should either add a -r/--remove flag (similar to userdel ) to optionally clean it up, or at minimum make the warning message explicit that the home directory is preserved and requires manual cleanup.

2. Deleting a user's primary group should be blocked

We shouldn't allow deletion of a group that is the primary group of an existing user. This mirrors groupdel's behavior:

You may not remove the primary group of any existing user. You must remove the user before you remove the group.

It breaks the user's next login with: failed to update user "<username>": another system user exists with "<username>" name.

3. Deleting a user orphans their primary group

When a user is deleted, their primary group isn't cleaned up, leaving the database in an inconsistent state.

=== users query ===
<username>|10001|10001|<name>|/home/<username>

=== groups query ===
10000|<username>

The user can re-login, but is assigned a different GID 10001 (non-existent), while GID 10000 is orphaned. This also causes errors in id and groups:

<username>@authd-dev:~$ id
id: cannot find name for group ID 10001
uid=10001(<username>) gid=10001(10001) groups=10001(10001),24(cdrom),10000(<username>)

<username>@authd-dev:~$ groups
groups: cannot find name for group ID 10001
10001 cdrom <username>

4. Re-login after deletion assigns a new UID, breaking home directory ownership

If a deleted user logs back in and gets the same home directory, there is a chance they receive a different UID, meaning they will no longer own their home directory. checkHomeDirOwner detects UID/GID mismatch but only logs a warning in the jounal

[shiv-tyagi]

Thanks for taking a look @nooreldeenmansour.

I will work on closing #830 as well in the same PR.

Regarding the issues you have highlighted,

When a user is deleted, their home directory is left behind. We should either add a -r/--remove flag (similar to userdel ) to optionally clean it up

Adding a flag to optionally delete the home directory makes sense.

We shouldn't allow deletion of a group that is the primary group of an existing user.

Makes sense. I will take care of this.

When a user is deleted, their primary group isn't cleaned up, leaving the database in an inconsistent state.

I intentionally did it that way because I was unsure about the scenarios where some other user is also manually made part of another users primary group. I would like to hear your thoughts on how do we handle this situation? Should we implement it in a way that the user's primary group deletion happens only when only that user was part of it's group?

Re-login after deletion assigns a new UID, breaking home directory ownership

This is similar to the warning we show with the delete command. Once a user is deleted, its UID can be assigned to another user. Similarly when it is recreated, it might get a new UID. I think we can add the wordings for later in the warning message. Other than that, I couldn't figure out an easy way to handle that. Our warning message already suggests to lock user instead of deleting to make sure the UID stays reserved. Anything else you have in mind to handle this?

[nooreldeenmansour]

I would like to hear your thoughts on how do we handle this situation? Should we implement it in a way that the user's primary group deletion happens only when only that user was part of it's group?

I think it makes sense to handle this the standard UNIX way. As detailed in the userdel:

If USERGROUPS_ENAB is defined to yes in /etc/login.defs, userdel will delete the group with the same name as the user. To avoid inconsistencies in the passwd and group databases, userdel will check that this group is not used as a primary group for another user.

USERGROUPS_ENAB (boolean)
If set to yes, userdel will remove the user's group if it contains no more members.

Other than that, I couldn't figure out an easy way to handle that. Our warning message already suggests to lock user instead of deleting to make sure the UID stays reserved. Anything else you have in mind to handle this?

The warning message is likely sufficient, since it clearly states the UID drift as a potential consequence. I'll wait on @adombeck's suggestion regarding this

[shiv-tyagi]

I think it makes sense to handle this the standard UNIX way.

That sounds reasonable. For the scenario where a user's primary group has other members as well - as far as I understand, authd can only determine whether some other user belongs to a users primary group if the other user itself is managed by authd (please correct me if that’s not accurate).

Based on that, we could safely delete a user’s primary group when it has no other members, while also issuing a warning that any manually added users in that group would lose their group membership.

@nooreldeenmansour @adombeck WDYT?

P.S. I know you’re busy with the upcoming release, so feel free to respond whenever convenient.

[adombeck]

Thank you, @shiv-tyagi, for the PR and thank you @nooreldeenmansour for the excellent review!

Note that #640 has a sub-issue (#830) around cleaning up broker-stored user data on deletion. Since it's related, it would be good to address it in the same PR.

I agree that we should handle the deletion of the broker's user data before merging this PR, because it would be confusing if we still kept user data after running authctl delete user. However, since it requires adding a new API method to the broker, I expect it to be a quite big change itself, so maybe for the sake of having smaller PRs, which we can merge more quickly, it would make sense to do that in a separate PR which we merge before this one.

I would like to hear your thoughts on how do we handle this situation? Should we implement it in a way that the user's primary group deletion happens only when only that user was part of it's group?

I think it makes sense to handle this the standard UNIX way. As detailed in the userdel:

If USERGROUPS_ENAB is defined to yes in /etc/login.defs, userdel will delete the group with the same name as the user. To avoid inconsistencies in the passwd and group databases, userdel will check that this group is not used as a primary group for another user.

USERGROUPS_ENAB (boolean)
If set to yes, userdel will remove the user's group if it contains no more members.

I agree that we should handle it the same way userdel does when usergroups are enabled, but regardless of the value of USERGROUPS_ENAB. I don't see a use case for disabling this behavior in authd (and if there was one, the place to add the setting would be /etc/authd/authd.yaml).

Other than that, I couldn't figure out an easy way to handle that. Our warning message already suggests to lock user instead of deleting to make sure the UID stays reserved. Anything else you have in mind to handle this?

The warning message is likely sufficient, since it clearly states the UID drift as a potential consequence. I'll wait on @adombeck's suggestion regarding this

I agree, a warning message explaining the issue with file ownership and suggesting to disable the user instead is the best we can do.

[adombeck]

That sounds reasonable. For the scenario where a user's primary group has other members as well - as far as I understand, authd can only determine whether some other user belongs to a users primary group if the other user itself is managed by authd (please correct me if that’s not accurate).

We don't have to worry about that, because the primary groups only exist in the authd database, not /etc/group, so local users can't be added to those groups (related: #1374).