update: store API keys as a user type in store metadata#1628
Merged
nikhilsinhaparseable merged 5 commits intoparseablehq:mainfrom Apr 26, 2026
Merged
update: store API keys as a user type in store metadata#1628nikhilsinhaparseable merged 5 commits intoparseablehq:mainfrom
nikhilsinhaparseable merged 5 commits intoparseablehq:mainfrom
Conversation
Contributor
WalkthroughAPI-key persistence and metastore APIs were removed; API-key auth now resolves Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Request
participant Middleware as HTTP Middleware
participant KeyLookup as API Key Lookup
participant RBAC as RBAC Engine
participant SessionMgr as Session Manager
participant Handler as Request Handler
Client->>Middleware: Request + x-api-key header
Middleware->>KeyLookup: find_api_key_user(tenant, key)
KeyLookup-->>Middleware: ApiKeyUser (UserType::ApiKey)
Middleware->>RBAC: Build permissions from roles
RBAC-->>Middleware: Permission set / RoleBuilder
Middleware->>SessionMgr: Create ephemeral session (5m)
SessionMgr-->>Middleware: SessionKey
Middleware->>Middleware: Insert SessionKey into request extensions
Middleware->>Handler: Forward request with session
Handler->>RBAC: Authorize using session permissions
RBAC-->>Handler: Authorization result
Handler-->>Client: Response
Middleware->>SessionMgr: Drop guard removes session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apikeys.rs (1)
89-103:⚠️ Potential issue | 🟠 MajorDelegate to
RBACError's status code instead of forcing 500.
ApiKeyError::Rbac(_)is currently grouped with storage and anyhow failures, forcing all RBAC errors to returnINTERNAL_SERVER_ERROR. However,RBACError's ownResponseErrorimplementation returns appropriate status codes:BAD_REQUESTfor validation failures (UserExists, RoleValidationError, etc.),NOT_FOUNDfor UserDoesNotExist, and onlyINTERNAL_SERVER_ERRORfor actual storage/network faults. This wrapper masks those client-side errors as 500s.Suggested fix
impl actix_web::ResponseError for ApiKeyError { fn status_code(&self) -> actix_web::http::StatusCode { match self { ApiKeyError::KeyNotFound(_) => actix_web::http::StatusCode::NOT_FOUND, ApiKeyError::DuplicateKeyName(_) => actix_web::http::StatusCode::CONFLICT, ApiKeyError::Unauthorized(_) => actix_web::http::StatusCode::FORBIDDEN, - ApiKeyError::Storage(_) | ApiKeyError::Rbac(_) | ApiKeyError::Anyhow(_) => { + ApiKeyError::Storage(_) | ApiKeyError::Anyhow(_) => { actix_web::http::StatusCode::INTERNAL_SERVER_ERROR } + ApiKeyError::Rbac(err) => actix_web::ResponseError::status_code(err), } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/apikeys.rs` around lines 89 - 103, ApiKeyError currently maps ApiKeyError::Rbac(_) to INTERNAL_SERVER_ERROR, masking RBACError's proper HTTP status; update the ResponseError impl for ApiKeyError so the match arm delegates to the wrapped RBACError's status_code (e.g., match ApiKeyError::Rbac(inner) => inner.status_code()), leaving other arms unchanged and importing/using the RBACError type if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/apikeys.rs`:
- Around line 89-103: ApiKeyError currently maps ApiKeyError::Rbac(_) to
INTERNAL_SERVER_ERROR, masking RBACError's proper HTTP status; update the
ResponseError impl for ApiKeyError so the match arm delegates to the wrapped
RBACError's status_code (e.g., match ApiKeyError::Rbac(inner) =>
inner.status_code()), leaving other arms unchanged and importing/using the
RBACError type if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 318c6373-123f-474b-95d8-b796f4042e9c
📒 Files selected for processing (12)
src/apikeys.rssrc/handlers/http/middleware.rssrc/handlers/http/modal/query/querier_rbac.rssrc/handlers/http/rbac.rssrc/metastore/metastore_traits.rssrc/metastore/metastores/object_store_metastore.rssrc/prism/home/mod.rssrc/rbac/mod.rssrc/rbac/user.rssrc/rbac/utils.rssrc/storage/mod.rssrc/storage/object_storage.rs
💤 Files with no reviewable changes (3)
- src/storage/object_storage.rs
- src/metastore/metastore_traits.rs
- src/storage/mod.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/handlers/http/rbac.rs (1)
48-48: KeepUPDATE_LOCKvisibility narrow unless external crates must access it.If this lock is only consumed internally, prefer
pub(crate)to avoid exposing a global sync primitive as part of a broader public surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/rbac.rs` at line 48, The global Mutex named UPDATE_LOCK is currently declared as public (pub) but should have narrowed visibility; change its declaration from pub to pub(crate) (i.e., make UPDATE_LOCK pub(crate): Mutex<()> = Mutex::const_new(()) in rbac.rs) so the synchronization primitive is only exposed within the crate unless you explicitly need external access; update any internal references if needed to compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/handlers/http/rbac.rs`:
- Line 48: The global Mutex named UPDATE_LOCK is currently declared as public
(pub) but should have narrowed visibility; change its declaration from pub to
pub(crate) (i.e., make UPDATE_LOCK pub(crate): Mutex<()> = Mutex::const_new(())
in rbac.rs) so the synchronization primitive is only exposed within the crate
unless you explicitly need external access; update any internal references if
needed to compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d02ae5b-ee2c-4d35-8a19-f3201c6f61f8
📒 Files selected for processing (1)
src/handlers/http/rbac.rs
praveen5959
approved these changes
Apr 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit