authd-oidc-brokers: Add API method for deleting broker's user data#1421
authd-oidc-brokers: Add API method for deleting broker's user data#1421adombeck merged 2 commits intocanonical:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1421 +/- ##
==========================================
- Coverage 80.48% 80.36% -0.13%
==========================================
Files 20 20
Lines 1030 1049 +19
==========================================
+ Hits 829 843 +14
- Misses 201 206 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
190b3c5 to
5976393
Compare
|
Hi @adombeck! This PR is waiting for your review? Can you please see this when you get time so that we can make some progress here? Thanks in advance! |
adombeck
left a comment
There was a problem hiding this comment.
Thanks a lot for this, @shiv-tyagi, and sorry for the delay. I noticed a potential security issue while reviewing this (#1442) and had to analyze it and go over it with our security team to ensure that it can't be exploited (in which case we would have had to fix it privately and coordinate a quick bugfix release).
There was a problem hiding this comment.
Pull request overview
This PR adds a broker-side DeleteUser D-Bus method so authd can instruct OIDC brokers to remove per-user cached data (tokens/password material) when a user is deleted via authctl (per issue #830, prerequisite for PR #1372).
Changes:
- Expose
DeleteUser(username)on the broker D-Bus API (service + introspection/XML). - Implement broker-side deletion of per-user data directories in
authd-oidc-brokers(plus path normalization refactor for issuer directories). - Add unit tests covering issuer path normalization (
UserDataDir) andDeleteUserbehavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| examplebroker/dbus.go | Adds D-Bus handler method DeleteUser forwarding to broker implementation. |
| examplebroker/com.ubuntu.auth.ExampleBroker.xml | Updates the ExampleBroker D-Bus interface XML to include DeleteUser. |
| examplebroker/broker.go | Adds example in-memory DeleteUser implementation. |
| authd-oidc-brokers/internal/dbusservice/methods.go | Adds D-Bus service method DeleteUser delegating to broker. |
| authd-oidc-brokers/internal/dbusservice/dbusservice.go | Extends introspection XML to advertise DeleteUser. |
| authd-oidc-brokers/internal/broker/export_test.go | Exposes userDataDir to tests via UserDataDir. |
| authd-oidc-brokers/internal/broker/broker_test.go | Adds tests for user data directory derivation and deletion behavior. |
| authd-oidc-brokers/internal/broker/broker.go | Introduces issuer normalization helper + DeleteUser implementation using os.RemoveAll. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5976393 to
b23fce3
Compare
Signed-off-by: Shiv Tyagi <shivtyagi3015@gmail.com>
b23fce3 to
f8a4a63
Compare
f8a4a63 to
53af15d
Compare
There was a problem hiding this comment.
Pull request overview
Adds a broker-side DeleteUser D-Bus API to allow authd/authctl to request deletion of per-user broker data (tokens/password cache), addressing issue #830 and unblocking the authd-side work in #1372.
Changes:
- Expose
DeleteUserover D-Bus for both the example broker and theauthd-oidc-brokersservice. - Implement broker-side deletion of user data (filesystem-backed in
authd-oidc-brokers, in-memory forexamplebroker). - Refactor issuer/user data dir derivation into helper functions and add/adjust tests for normalization, traversal protection, and deletion behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| examplebroker/dbus.go | Exposes DeleteUser method on the example broker’s D-Bus object. |
| examplebroker/com.ubuntu.auth.ExampleBroker.xml | Adds DeleteUser method to the example broker introspection XML. |
| examplebroker/broker.go | Adds example broker DeleteUser implementation (map-backed). |
| authd-oidc-brokers/internal/dbusservice/methods.go | Adds D-Bus handler method DeleteUser delegating to broker. |
| authd-oidc-brokers/internal/dbusservice/dbusservice.go | Adds DeleteUser to the service introspection string. |
| authd-oidc-brokers/internal/broker/export_test.go | Exposes internal helpers (userDataDir, normalizedIssuer) for tests. |
| authd-oidc-brokers/internal/broker/broker_test.go | Adds tests for issuer normalization, user data dir derivation, and deletion; adjusts NewSession traversal test. |
| authd-oidc-brokers/internal/broker/broker.go | Implements user data directory derivation helpers and DeleteUser that removes the user directory tree. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Looks good to me. However, codecov reports that two new lines in broker.go are missing coverage, see https://app.codecov.io/gh/canonical/authd/pull/1421?src=pr&el=tree
Could you add test cases to cover these as well? Thanks!
Signed-off-by: Shiv Tyagi <shivtyagi3015@gmail.com>
53af15d to
d026faa
Compare
|
@adombeck All passing now. . Please check. |
adombeck
left a comment
There was a problem hiding this comment.
Excellent, thanks a lot!
Closes #830
This PR adds the
DeleteUsermethod on the broker side to help delete user data (like password, token etc.) when a user is deleting using authctl.#1372 has the authd side of changes which would use this method. Hence this should be merged before that.