feat(provider): scoped provider name uniqueness#20
Conversation
Allow multiple users to create providers with the same visible name by
storing them with an owner-prefixed DB key ({owner}/{name}). Resolution
uses two-pass lookup: try owned first, fall back to shared.
Key changes:
- ownership.rs: scoped_name, display_name, owner_prefix,
scoped_name_for_principal, resolve_scoped_name helpers + unit tests
- provider.rs: create stamps scoped key, get/update/delete resolve via
two-pass, list filters by owner, responses strip prefix
- sandbox.rs: spec.providers stores DB keys, attach/detach resolve
user-visible names, all response paths strip prefixes
Zero DB migration required — existing partial unique index on
(object_type, name) already supports scoped keys.
Closes: kagenti/kagenti#1996
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.
Clean, well-tested implementation of Option B (name prefixing, no migration). Owned→shared resolution and response prefix-stripping are consistent across get/update/delete/list and the sandbox attach/detach paths, and the delete-while-attached guard (sandboxes_using_provider) correctly matches on the scoped key.
One blocker, three non-blocking notes:
- ❌ CI:
cargo fmt --checkis failing — runcargo fmtand amend (blocks the Rust gate). - The three inline suggestions/nit are edge cases and don't block.
Leaving this as a plain comment (no approve/request-changes) per your call.
Assisted-By: Claude Code
| } | ||
|
|
||
| // Resolve user-visible name to DB key (spec stores DB keys) | ||
| let provider_db_key = resolve_provider_db_key( |
There was a problem hiding this comment.
must-fix (CI): the rustfmt --check gate is red across provider.rs, sandbox.rs, and the new tests — purely mechanical. Run cargo fmt and amend; the Rust check should then pass.
| .ok_or_else(|| Status::internal("sandbox spec is missing"))?; | ||
|
|
||
| let provider_name = request.provider_name.clone(); | ||
| let provider_name_for_cas = provider_db_key.clone(); |
There was a problem hiding this comment.
suggestion — detach resolution ambiguity. Detach re-resolves owned-first via resolve_provider_db_key, then retain(|name| name != &provider_name_for_cas). If a user owns alice/openai and their sandbox has the shared openai attached (attached before they created their own), resolution returns alice/openai, retain matches nothing → silent detached=false, stranding the shared reference. Consider detaching by matching the stored spec entries (try both the owned and shared db-key variants, or match on display_name) rather than a single re-resolved key.
| } | ||
|
|
||
| /// Strip owner prefixes from sandbox spec.providers before returning to user. | ||
| fn strip_sandbox_provider_prefixes(mut sandbox: Sandbox) -> Sandbox { |
There was a problem hiding this comment.
suggestion — admin addressability. strip_sandbox_provider_prefixes (and strip_name_prefix in provider.rs) strip the owner prefix unconditionally, including for admins. The PR says admins access by raw/scoped name, but list/get never surface the scoped key — so an admin sees N identical openai entries with no way to discover or address a specific user's provider. Consider preserving the scoped key in responses for admin principals.
|
|
||
| /// Internal lookup by exact DB key (scoped or unscoped). Used by sandbox env | ||
| /// resolution and inference routing where the stored key is already correct. | ||
| pub(super) async fn get_provider_record_by_db_key( |
There was a problem hiding this comment.
nit — redaction footgun. get_provider_record_by_db_key drops the redact_provider_credentials that the old get_provider_record applied. It's safe today only because the sole caller (providers_for_sandbox) re-wraps with prepare_provider_response. A doc note ("returns UN-redacted credentials; caller must redact before returning to users") or a debug-assert would guard against a future credential leak.
- Run cargo fmt to fix CI rustfmt gate - Fix detach resolution ambiguity: try both owned and shared DB key variants when detaching, so a shared provider attached before the user created their own can still be detached - Add safety doc note to get_provider_record_by_db_key warning callers about un-redacted credentials - Add TODO notes for admin addressability (preserve scoped keys in responses for admin principals) Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
- Add `let admin_role = admin_role_name(state)` in CreateSandbox handler - Update test call sites for get_provider_record (now takes 4 args) Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
The spec variable needs to be declared `mut` since we assign resolved_providers to spec.providers before passing it downstream. Removes the redundant re-binding that was placed after the assignment. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
- Replace match in display_name with map_or (option_if_let_else) - Collapse nested if-let in create_provider (collapsible_if) - Remove needless borrow in detach handler (needless_borrow) - Allow dead_code on owner_prefix and scoped_name_for_principal (used in tests; needed for admin addressability TODO) Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
The two-pass resolution (owned → shared fallback) failed to detect
providers owned by other users — returning NotFound instead of
PermissionDenied — and admin callers couldn't resolve user-scoped
providers by display name.
Add find_by_name_suffix to the Store (SQL LIKE '%/{name}') and
rewrite resolve_scoped_name with three-step logic:
1. Try {caller}/{name} (owned)
2. Try {name} (shared/legacy) with post-resolution ownership check
3. Suffix scan for cross-user detection (admin gets access, others
get PermissionDenied)
Fixes: non_owner_denied_get, non_owner_denied_update,
non_owner_denied_delete, admin_can_get_any_provider,
attach_sandbox_provider_rejects_other_users_provider
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
- Backtick `PermissionDenied` in doc comments - Collapse nested if-let + if into single let-chain Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Non-admin callers who try to resolve a provider name owned by another user (via suffix scan) now get NotFound rather than PermissionDenied. This prevents leaking resource existence to unauthorized callers and ensures consistent behavior when a user deletes their own provider while another user has the same name. The PermissionDenied path remains active for the shared/legacy fallback (Step 2) where the record's owner label is directly checked — this covers providers stored under raw names with explicit ownership. Admin callers continue to resolve any user's provider via suffix scan. 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>
Upgrade gateway and supervisor images to v0.0.56-rc.3 which adds scoped provider name uniqueness (kagenti/OpenShell#20). This allows multiple users to independently create providers with the same visible name, enabling per-user credential isolation on shared gateways. Also bumps compute driver to v0.1.0-rc.7 and fixes deploy-tenant.sh to override supervisorImage.tag when --image-tag is specified. Closes kagenti#1995 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
1 participant
Summary
Implements scoped provider name uniqueness (kagenti/kagenti#1996), allowing multiple users to create providers with the same visible name. Each user's provider is stored with an owner-prefixed DB key (
{sanitized_owner}/{name}), while shared/legacy providers keep their raw name.Design choice: Option B (name prefixing) — zero DB migration required. The existing
objects_name_uqpartial unique index on(object_type, name)already supports this since the stored name changes.Key changes
auth/ownership.rs: New helpers —scoped_name,display_name,owner_prefix,scoped_name_for_principal,resolve_scoped_name+ comprehensive unit testsgrpc/provider.rs: Create stamps scoped key, get/update/delete resolve via two-pass (owned → shared), list filters by owner, all responses strip owner prefixgrpc/sandbox.rs:spec.providersstores DB keys internally, attach/detach resolve user-visible names, all response paths strip prefixes before returning to usersResolution order
{owner}/{name}(user's own provider){name}(shared/legacy provider)Behavioral guarantees
openai), never the scoped DB key (alice-uuid/openai)openaiwithout conflictTest plan
Assisted-By: Claude Code