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 63.46154% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.56%. Comparing base (1f7d986) to head (949d410).

Files with missing lines	Patch %	Lines
cmd/authctl/group/delete.go	4.76%	20 Missing ⚠️
cmd/authctl/user/delete.go	9.09%	20 Missing ⚠️
internal/users/manager.go	82.50%	7 Missing ⚠️
internal/services/user/user.go	87.87%	4 Missing ⚠️
internal/users/db/users.go	81.25%	3 Missing ⚠️
internal/users/db/groups.go	83.33%	2 Missing ⚠️
internal/brokers/localbroker.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1372      +/-   ##
==========================================
- Coverage   86.94%   86.56%   -0.39%     
==========================================
  Files          93       95       +2     
  Lines        6366     6519     +153     
  Branches      111      111              
==========================================
+ Hits         5535     5643     +108     
- Misses        775      820      +45     
  Partials       56       56              

☔ 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).

[shiv-tyagi]

Hi @adombeck @nooreldeenmansour

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

I tried implementing this in authd's User service. I am getting error deleting the home directory. When I checked I found that the authd's service is configured in a way that it cannot touch stuff inside /home. Would it be okay to add /home to the list of paths the authd process is allowed to read write?

Edit: the process would also require CAP_DAC_OVERRIDE, I just checked.

[3v1n0]

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.

Also note that when targeting ubuntu we should also ensure that deluser (as we try to do with adduser) policies are respected too.

[adombeck]

When I checked I found that the authd's service is configured in a way that it cannot touch stuff inside /home. Would it be okay to add /home to the list of paths the authd process is allowed to read write?

Yeah, we'll haven to loosen the restrictions on the authd systemd service (see debian/authd.service.in). ReadWritePaths=-/home should work for directories below /home, but home directories could also be elsewhere. We could try allow-list other directories, but maybe we should just remove the filesystem restrictions we currently have, because they don't really provide an effective security boundary anyway. I'll create an issue where we can discuss that.

[adombeck]

Yeah, we'll haven to loosen the restrictions on the authd systemd service (see debian/authd.service.in)

Actually, after taking a closer look, I don't see anything in our existing service definition which would prevent writing to /home. Which error do you see? Can you commit the changes so I can try to reproduce it?

[adombeck]

Also note that when targeting ubuntu we should also ensure that deluser (as we try to do with adduser) policies are respected too.

Which policies are you referring to, @3v1n0?

[shiv-tyagi]

@adombeck

I have pushed my changes. This includes the broker side changes as well (I need to organise the commits better).

Actually, after taking a closer look, I don't see anything in our existing service definition which would prevent writing to /home. Which error do you see? Can you commit the changes so I can try to reproduce it?

^ this is the error on the current systemd unit definition.

I checked again, turned out that it is a missing CAP_DAC_OVERRIDE capability in the authd process.
(earlier I was under an impression that not adding /home to ReadWritePaths might have been preventing it, but it looks like just doing that is not enough and if we add the above mentioned capability to the process, later is not even needed.)

[adombeck]

I checked again, turned out that it is a missing CAP_DAC_OVERRIDE capability in the authd process.

That makes sense. Then let's add CAP_DAC_OVERRIDE to the CapabilityBoundingSet and explain in the comment above why we need it.

[shiv-tyagi]

Hey @adombeck! I have submitted #1421 which contains the broker side of changes for user data deletion as suggested by you.

This PR is also ready for another round of review. I have addressed the comments posted by @nooreldeenmansour. Please take a look and feel free to suggest improvements.

[shiv-tyagi]

TestUserDeleteCommand is failing because I have removed broker side changes from this PR to help with the review. I will rebase when #1421 is merged and they should pass.

[shiv-tyagi]

TestUserDeleteCommand is failing because I have removed broker side changes from this PR to help with the review. I will rebase when #1421 is merged and they should pass.

@adombeck The tests are passing now. You can do a review whenever you get time. Thanks.

[Copilot]

Pull request overview

Adds end-to-end support for deleting authd-managed users and groups via authctl, including new gRPC APIs, DB/user-manager operations, broker-side cleanup hooks, and CLI confirmation/warnings.

Changes:

• Introduces DeleteUser / DeleteGroup RPCs and implements server-side behavior (including broker cleanup for user deletion).
• Adds user-manager + DB support for deleting users/groups, including guarding against deleting groups that are primary for existing users.
• Adds authctl user delete / authctl group delete commands with confirmation prompts, --yes, and --remove (user home) options, plus extensive golden/testdata updates.

Reviewed changes

Copilot reviewed 76 out of 76 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
internal/users/manager.go	Implements Manager.DeleteUser/DeleteGroup and primary-group checks.
internal/users/manager_test.go	Adds unit tests for user/group deletion and primary-group detection.
internal/users/export_test.go	Exposes IsGroupPrimaryForUsers for tests.
internal/users/defs.go	Adds GroupIsPrimaryError surfaced through gRPC.
internal/users/db/users.go	Adds query to list users with a given primary GID.
internal/users/db/groups.go	Adds DB-level DeleteGroup implementation.
internal/users/db/db_test.go	Adds DB-level tests for DeleteGroup.
internal/users/db/testdata/golden/TestDeleteGroup/Deleting_sole_group_of_a_user_removes_group_and_memberships_but_keeps_user	Golden output for DB group deletion scenario.
internal/users/db/testdata/golden/TestDeleteGroup/Deleting_shared_group_removes_only_that_group_and_its_memberships	Golden output for DB shared-group deletion scenario.
internal/users/testdata/db/multiple_users_and_groups_with_tmp_home.db.yaml	Adds DB fixture with temp home paths for delete-user tests.
internal/users/testdata/db/multiple_users_and_groups_with_non_primary_group.db.yaml	Adds DB fixture for deleting a non-primary group.
internal/users/testdata/db/group_primary_check.db.yaml	Adds DB fixture for detecting primary-group users.
internal/users/testdata/golden/TestDeleteUser/Successfully_delete_user	Golden output for delete-user behavior.
internal/users/testdata/golden/TestDeleteUser/Successfully_delete_user_and_remove_home	Golden output for delete-user with home removal.
internal/users/testdata/golden/TestDeleteUser/Successfully_delete_user_keeps_other_users_in_shared_group	Golden output ensuring shared group memberships persist for remaining users.
internal/users/testdata/golden/TestDeleteUser/Successfully_delete_user_removes_them_from_local_groups	Golden output ensuring local group file updates on user deletion.
internal/users/testdata/golden/TestDeleteGroup/Successfully_delete_group_keeps_its_members_in_the_db	Golden output for manager-level group deletion where members remain.
internal/users/testdata/golden/TestDeleteGroup/Successfully_delete_shared_group_leaves_other_groups_intact	Golden output for manager-level shared group deletion.
internal/users/testdata/golden/TestIsGroupPrimaryForUsers/Returns_users_for_which_the_group_is_primary	Golden output for primary-group lookup.
internal/users/testdata/golden/TestIsGroupPrimaryForUsers/Returns_multiple_users_when_group_is_primary_for_several	Golden output for primary-group lookup with multiple users.
internal/users/testdata/golden/TestIsGroupPrimaryForUsers/Returns_empty_slice_when_no_user_has_it_as_primary	Golden output for no matches.
internal/users/testdata/golden/TestIsGroupPrimaryForUsers/Returns_empty_slice_for_shared_group_that_is_not_primary	Golden output for non-primary shared group.
internal/services/user/user.go	Adds DeleteUser/DeleteGroup RPC handlers and maps GroupIsPrimaryError to gRPC.
internal/services/user/user_test.go	Adds service tests for delete user/group paths (uppercase handling, broker failures, perms).
internal/services/user/testdata/delete-user.db.yaml	Adds service DB fixture covering delete-user scenarios.
internal/services/user/testdata/golden/TestDeleteUser/Successfully_delete_user	Golden output for service delete-user.
internal/services/user/testdata/golden/TestDeleteUser/Successfully_delete_user_with_uppercase	Golden output verifying case normalization.
internal/services/user/testdata/golden/TestDeleteGroup/Successfully_delete_group	Golden output for service delete-group.
internal/services/user/testdata/golden/TestDeleteGroup/Successfully_delete_group_with_uppercase	Golden output verifying case normalization for groups.
internal/services/testdata/golden/TestRegisterGRPCServices	Updates golden list of registered RPC methods.
internal/proto/authd/authd.proto	Adds DeleteUserRequest/DeleteGroupRequest and service methods.
internal/proto/authd/authd.pb.go	Regenerates protobuf types for new messages.
internal/proto/authd/authd_grpc.pb.go	Regenerates gRPC stubs for new RPC methods.
internal/brokers/broker.go	Extends broker interface and adds Broker.DeleteUser.
internal/brokers/dbusbroker.go	Implements broker-side DeleteUser over D-Bus.
internal/brokers/localbroker.go	Adds stub DeleteUser to satisfy broker interface.
internal/brokers/manager.go	Exports BrokerFromID and updates internal callers.
internal/brokers/broker_test.go	Adds tests for broker DeleteUser.
internal/testutils/broker.go	Extends broker mock to support DeleteUser and forced errors.
cmd/authctl/user/user.go	Registers authctl user delete subcommand.
cmd/authctl/user/delete.go	Implements authctl user delete with confirmation + --remove.
cmd/authctl/user/delete_test.go	Integration-style tests for authctl user delete.
cmd/authctl/user/testdata/db/multiple_users_and_groups_with_tmp_home.db.yaml	Test DB fixture for authctl user delete (temp home dirs).
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Delete_user_success	Golden output for user deletion.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Delete_with_remove_flag_removes_home_dir	Golden output for --remove behavior.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Delete_without_remove_flag_keeps_home_dir	Golden output for keeping home directory.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Delete_with_remove_flag_succeeds_when_home_dir_does_not_exist	Golden output for missing home dir.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Error_when_user_does_not_exist	Golden error output for NotFound.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Error_when_authd_is_unavailable	Golden error output for unavailable daemon.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Confirmation_prompt_accepted_with_y	Golden output for confirmation flow.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Confirmation_prompt_accepted_with_yes	Golden output for confirmation flow.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Confirmation_prompt_accepted_case_insensitively	Golden output for confirmation flow.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Confirmation_prompt_aborted_with_n	Golden output for abort flow.
cmd/authctl/user/testdata/golden/TestUserDeleteCommand/Confirmation_prompt_aborted_with_empty_input	Golden output for abort flow.
cmd/authctl/user/testdata/golden/TestUserCommand/Usage_message_when_no_args	Updates command list to include delete.
cmd/authctl/user/testdata/golden/TestUserCommand/Help_flag	Updates help output to include delete.
cmd/authctl/user/testdata/golden/TestUserCommand/Error_on_invalid_flag	Updates usage output to include delete.
cmd/authctl/user/testdata/golden/TestUserCommand/Error_on_invalid_command	Updates usage output to include delete.
cmd/authctl/group/group.go	Registers authctl group delete subcommand.
cmd/authctl/group/delete.go	Implements authctl group delete with confirmation.
cmd/authctl/group/delete_test.go	Integration-style tests for authctl group delete.
cmd/authctl/group/testdata/db/multiple_users_and_groups.db.yaml	Test DB fixture for authctl group delete scenarios.
cmd/authctl/group/testdata/golden/TestGroupDeleteCommand/Delete_group_success	Golden output for group deletion.
cmd/authctl/group/testdata/golden/TestGroupDeleteCommand/Error_when_group_does_not_exist	Golden error output for NotFound.
cmd/authctl/group/testdata/golden/TestGroupDeleteCommand/Error_when_authd_is_unavailable	Golden error output for unavailable daemon.
cmd/authctl/group/testdata/golden/TestGroupDeleteCommand/Error_when_group_is_primary_group_of_a_user	Golden error output for primary-group constraint.
cmd/authctl/group/testdata/golden/TestGroupDeleteCommand/Confirmation_prompt_accepted_with_y	Golden output for confirmation flow.
cmd/authctl/group/testdata/golden/TestGroupDeleteCommand/Confirmation_prompt_accepted_with_yes	Golden output for confirmation flow.
cmd/authctl/group/testdata/golden/TestGroupDeleteCommand/Confirmation_prompt_accepted_case_insensitively	Golden output for confirmation flow.
cmd/authctl/group/testdata/golden/TestGroupDeleteCommand/Confirmation_prompt_aborted_with_n	Golden output for abort flow.
cmd/authctl/group/testdata/golden/TestGroupDeleteCommand/Confirmation_prompt_aborted_with_empty_input	Golden output for abort flow.
cmd/authctl/group/testdata/golden/TestGroupCommand/Usage_message_when_no_args	Updates command list to include delete.
cmd/authctl/group/testdata/golden/TestGroupCommand/Help_flag	Updates help output to include delete.
cmd/authctl/group/testdata/golden/TestGroupCommand/Error_on_invalid_flag	Updates usage output to include delete.
cmd/authctl/group/testdata/golden/TestGroupCommand/Error_on_invalid_command	Updates usage output to include delete.
debian/authd.service.in	Updates capability bounding set for delete-home support.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The deferred errors.Join(err, unlockEntries()) is ineffective here because DeleteUser doesn’t use a named return value. Any error from unlockEntries() will be silently dropped, unlike other methods in this file that use a named err return. Consider switching DeleteUser to func (...) (err error) (or explicitly calling unlockEntries() and handling/joining its error before returning) to preserve unlock failures consistently.

[Copilot]

UsersWithPrimaryGroup builds its result order from the database without an ORDER BY. That can make IsGroupPrimaryForUsers output (and GroupIsPrimaryError messages) nondeterministic across SQLite versions/plans. Adding an explicit order (e.g. ORDER BY name or ORDER BY uid) would make results stable and avoid flaky tests / inconsistent error strings.

[Copilot]

DeleteUser deletes the DB user before resolving the broker instance. If BrokerFromID (or the subsequent broker.DeleteUser) fails, this RPC returns an error even though the user has already been removed from the authd DB. To avoid partial-failure semantics, consider resolving the broker (and verifying it exists) before deleting the user, or treat broker-side cleanup failures as warnings and still return success after DB deletion.

[Copilot]

This log call uses context.TODO() instead of the provided ctx, which drops request-scoped context (cancellation, trace IDs, etc.) from logs. Use ctx here to keep logging consistent with the rest of the broker methods.