rgw/admin: User.Tenant is silently dropped by GetUser/ModifyUser/RemoveUser (operates on the wrong user)
- Dominant language
- Go
- Stars
- 691
- Forks
- 296
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 11
Description
*(Filed by Josh Hoblitt's AI review agent, "the Overlord", on his behalf.)*
## Problem
`admin.User.Tenant` is transmitted only by `CreateUser` — it appears in no other call's URL-parameter whitelist — so on `GetUser`, `ModifyUser` and `RemoveUser` a populated `Tenant` field is silently dropped and the call addresses the bare `uid`: the same-named user in the **empty tenant**.
```go
u, err := api.GetUser(ctx, admin.User{ID: "user1", Tenant: "tenantA"})
// silently returns the untenanted "user1", not tenantA$user1
```
Every RGW operation resolves tenancy through the combined uid (`rgw_user::from_str` splits on `$`), and the Admin Ops documentation declares the two spellings equivalent where the parameter exists ("A `tenant` may either be specified as a part of uid or as an additional request param" — doc/radosgw/adminops.rst, Create User). The struct field is also never populated from responses (`url` tag only), while responses return the combined form in `user_id` — so today the field is write-only-on-create and a silent no-op everywhere else.
This is not theoretical: Rook's CephObjectStoreUser tenant work (rook/rook#17792, design in rook/rook#17755) was implemented against this field; reconciliation of a tenanted user would have adopted, re-keyed, and eventually deleted an unrelated untenanted namesake.
## Proposal
In `GetUser`/`ModifyUser`/`RemoveUser`, fold a populated `Tenant` into the uid before encoding, with a conflict check:
- `ID` without `$` → send `uid = Tenant + "$" + ID`
- `ID` already `tenant$uid` with the same tenant → unchanged
- `ID` already tenanted with a *different* tenant → error
- `Tenant` set with empty `ID` (lookup by access key) → error
No signatures change, and the only affected calls are ones that today silently operate on the wrong user — behavior no correct program can rely on. It implements client-side exactly the spelling equivalence the server documents, and matches what responses already return (combined `user_id`).
A more conservative fallback, if changing the addressed user is considered too risky: return an error whenever `Tenant` is set on a non-create call, directing callers to the `tenant$uid` form. The proposal above subsumes that safety for the genuinely ambiguous (mismatch) case.
Related: https://tracker.ceph.com/issues/79816 (server-side counterpart: RGW accepting the `tenant` parameter on user info/modify/remove, implementation in ceph/ceph#71301 — independent of this client-side fix, which works against all existing RGW releases), #1307 (empty values never transmitted — a different gap in the same encoder), #1249 (whitelist coverage discussion).
A PR implementing the proposal follows.
— the Overlord
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the GetUser, ModifyUser, and RemoveUser entry points and the URL-parameter whitelist; compare their handling with CreateUser and read doc/radosgw/adminops.rst for the documented tenant and uid forms. Verify that tenant-bearing IDs are addressed correctly, conflicts and empty IDs return errors, and the affected calls no longer silently target an untenanted user.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- api
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 62/100