feat(auth): per-user provider ownership enforcement#19
Conversation
Extend the openshell.ai/owner label pattern (from sandbox ownership in PR #18) to providers, enabling OIDC-authenticated users to create and manage their own providers on a shared gateway without admin privileges. Changes: - Relax provider CRUD methods from admin to user role in rpc_auth annotations (ImportProviderProfiles/DeleteProviderProfile remain admin) - Stamp owner label on provider create via stamp_owner() - Add ownership checks (check_provider_owner) to get, update, delete, configure-refresh, rotate-credential, and delete-refresh handlers - Filter list_providers to return own + shared for regular users, all for admins - Make ownership.rs docstring entity-generic (shared with sandboxes) - Update authz tests to reflect relaxed role requirements - Add 12 ownership isolation unit tests covering create/get/list/update/ delete with owner, non-owner, admin, and shared provider scenarios Implements kagenti/kagenti#1995 (partial: Tasks 1-2, 5 complete; Tasks 3-4 deferred for follow-up). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
When attaching a provider to a sandbox, verify the provider is either owned by the caller or shared (no owner label). This prevents user A from referencing user B's private provider in their sandbox. Adds two tests: one for the rejection case and one confirming shared providers remain attachable by any user. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
Solid, well-tested authz change extending the openshell.ai/owner label pattern from sandboxes (#18) to providers — RBAC relaxed admin→user with enforcement moved into handlers, admin/anonymous backward-compat preserved.
Blocker: CI is red — the Rust clippy gate fails with 10 -D warnings errors (1 in production code at the list filter, 9 in the new test module). Merge is blocked until clippy is clean: cargo clippy --workspace --all-targets -- -D warnings.
Two non-blocking correctness notes inline (per-user list pagination, and the skip-on-not-found ownership guard in update/delete).
Areas reviewed: Rust authz/ownership/provider/sandbox, tests, CI. 3 commits, all signed-off. No .claude/.vscode changes.
Assisted-By: Claude Code
| .into_iter() | ||
| .filter(|provider| { | ||
| let labels = provider.metadata.as_ref().map(|m| &m.labels); | ||
| match labels.and_then(|l| l.get(OWNER_LABEL)) { |
There was a problem hiding this comment.
must-fix (CI blocker): clippy -D warnings is failing the build. This line trips clippy::option_if_let_else; apply the suggested rewrite:
labels.and_then(|l| l.get(OWNER_LABEL)).map_or(true, |v| *v == owner_value)The other 9 errors are in the new test module: unnecessary_qualification (~4783-4808), redundant_closure (~5003-5048), and iter_on_single_items (~5072/5108). Run cargo clippy --workspace --all-targets -- -D warnings locally to clear all of them before merge.
| _ => None, // Anonymous/sandbox/none → see all (backward compat) | ||
| }; | ||
|
|
||
| let all_providers = list_provider_records(state.store.as_ref(), limit, offset).await?; |
There was a problem hiding this comment.
suggestion: the owner filter is applied after list_provider_records(limit, offset). For a non-admin, if the first limit rows happen to be owned by others, they get an empty/truncated page even when they own providers further down — per-user pagination is effectively broken. It never leaks others' records (it under-returns, so it's safe), but consider pushing the owner filter into the store query, or looping until limit owned rows are collected.
| .get_message_by_name::<Provider>(provider_name) | ||
| .await | ||
| .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))?; | ||
| if let Some(ref existing) = existing { |
There was a problem hiding this comment.
suggestion: this update guard (and the delete guard ~1702) skips the ownership check when the record isn't found (if let Some(ref existing)) or the name is empty, silently proceeding. handle_get/rotate instead do .ok_or_else(not_found) and then check. Low risk here, but the inconsistent guard pattern is worth aligning — prefer erroring on not-found over skipping the check.
- Fix clippy warnings: unnecessary qualifications, redundant closures, iter_on_single_items, map_or→is_none_or - Fix list pagination for non-admins: loop in batches to guarantee `limit` visible results despite post-fetch filtering - Align update/delete ownership guards to return NOT_FOUND early when the provider doesn't exist, matching get/rotate behavior Signed-off-by: Paolo Dettori <paolo@dettori.dev> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
1 participant
Summary
openshell.ai/ownerlabel pattern (from sandbox ownership in feat(auth): per-user sandbox ownership enforcement #18) to providersImplements kagenti/kagenti#1995 (core enforcement; name-scoping and sandbox-binding alignment deferred to follow-up).
Changes
#[rpc_auth]fromrole = "admin"→role = "user"(exceptImportProviderProfiles/DeleteProviderProfile)openshell.ai/ownerlabel on provider createcheck_provider_ownerguard to get, update, delete, configure-refresh, rotate-credential, delete-refreshlist_providersto own + shared for regular users, all for adminsTest plan
Assisted-By: Claude Code