feat: Authentication improvements#456
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds API key feature end-to-end: data models, repositories (in-memory, Redis), controllers and routes for CRUD/permissions, app state wiring, bootstrap config seeding (default key), and propagation of a new ApiKeyRepository generic (AKR) across controllers, domain, services, sockets, and tests. README gains Redis testing notes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Routes (/api-keys)
participant Ctrl as ApiKey Controller
participant S as AppState (AKR)
participant Repo as ApiKeyRepositoryTrait
Note over C,R: List API Keys
C->>R: GET /api-keys?page=1&per_page=50
R->>Ctrl: list_api_keys(query, state)
Ctrl->>S: api_key_repository()
S->>Repo: list_paginated(query)
Repo-->>Ctrl: PaginatedResult<ApiKeyRepoModel>
Ctrl-->>C: 200 Paginated ApiKeyResponse
Note over C,R: Create API Key
C->>R: POST /api-keys {name, permissions, allowed_origins?}
R->>Ctrl: create_api_key(req, state)
Ctrl->>Ctrl: ApiKeyRequest -> ApiKeyRepoModel (TryFrom)
Ctrl->>Repo: create(model)
Repo-->>Ctrl: ApiKeyRepoModel
Ctrl-->>C: 201 ApiResponse(success)
sequenceDiagram
autonumber
participant Boot as process_config_file
participant S as AppState (AKR)
participant Repo as ApiKeyRepositoryTrait
Note over Boot: Bootstrap default API key
Boot->>S: api_key_repository()
Boot->>Repo: has_entries()
alt empty
Boot->>Repo: create(default ApiKeyRepoModel)
Repo-->>Boot: created
else populated
Boot-->>Boot: skip creation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MCarlomagno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/services/plugins/socket.rs (3)
172-192: Unify bounds: addSend + SynctoJinhandle_connection.
listenrequiresJ: Send + Sync, whilehandle_connectiononly requires'static. Align these to avoid surprises and keep the futureSend-safe.Apply:
- J: JobProducerTrait + 'static, + J: JobProducerTrait + Send + Sync + 'static,
207-212: Don’t ignore write errors; propagate them (and optionally flush).Silently discarding I/O errors makes debugging hard and can drop responses.
Apply:
- let _ = w.write_all(response_str.as_bytes()).await; + w.write_all(response_str.as_bytes()) + .await + .map_err(|e| PluginError::SocketError(e.to_string()))?; + w.flush() + .await + .map_err(|e| PluginError::SocketError(e.to_string()))?;
231-233: Tests missing imports cause compile errors (write_all,shutdown,UnixStream).
AsyncWriteExtandUnixStreamaren’t in scope inside this submodule.Apply:
- use tokio::{ - io::{AsyncBufReadExt, BufReader}, - time::timeout, - }; + use tokio::{ + io::{AsyncBufReadExt, AsyncWriteExt, BufReader}, + net::UnixStream, + time::timeout, + };src/services/plugins/relayer_api.rs (1)
405-417: Fix recursion in trait impl: methods call themselves, causing stack overflow when invoked via the trait.Inside the RelayerApiTrait impl, each method calls
self.<method>with the same signature, which resolves to the trait method again. Use fully qualified syntax to delegate to the inherent methods on RelayerApi.@@ impl<J, RR, TR, NR, NFR, SR, TCR, PR, AKR> RelayerApiTrait<J, RR, TR, NR, NFR, SR, TCR, PR, AKR> for RelayerApi { @@ - async fn handle_request( + async fn handle_request( &self, request: Request, state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Response { - self.handle_request(request, state).await + RelayerApi::handle_request::<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>(self, request, state).await } @@ - async fn process_request( + async fn process_request( &self, request: Request, state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.process_request(request, state).await + RelayerApi::process_request::<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>(self, request, state).await } @@ - async fn handle_send_transaction( + async fn handle_send_transaction( &self, request: Request, state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_send_transaction(request, state).await + RelayerApi::handle_send_transaction::<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>(self, request, state).await } @@ - async fn handle_get_transaction( + async fn handle_get_transaction( &self, request: Request, state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_get_transaction(request, state).await + RelayerApi::handle_get_transaction::<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>(self, request, state).await } @@ - async fn handle_get_relayer_status( + async fn handle_get_relayer_status( &self, request: Request, state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_get_relayer_status(request, state).await + RelayerApi::handle_get_relayer_status::<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>(self, request, state).await } @@ - async fn handle_sign_transaction( + async fn handle_sign_transaction( &self, request: Request, state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_sign_transaction(request, state).await + RelayerApi::handle_sign_transaction::<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>(self, request, state).await } @@ - async fn handle_get_relayer_info( + async fn handle_get_relayer_info( &self, request: Request, state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_get_relayer_info(request, state).await + RelayerApi::handle_get_relayer_info::<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>(self, request, state).await } }Also applies to: 418-473
src/bootstrap/config_processor.rs (1)
287-300: Redis population check omits API keys; can cause duplicate default keys.If only API keys exist in Redis,
is_redis_populatedreturns false andprocess_api_keyinserts again. Include the API key repository in the population check.async fn is_redis_populated<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>( @@ - if app_state.plugin_repository.has_entries().await? { + if app_state.plugin_repository.has_entries().await? { return Ok(true); } + if app_state.api_key_repository.has_entries().await? { + return Ok(true); + } + Ok(false) }Optionally, make
process_api_keyidempotent by skipping creation when a"default"key already exists.Also applies to: 321-326
🧹 Nitpick comments (52)
README.md (2)
287-291: Grammar: “a single thread”Insert the article for clarity.
-3. Run the tests using single thread to avoid race conditions within suites: +3. Run the tests using a single thread to avoid race conditions within test suites:
279-283: Safer Redis run command (pin version + bind to localhost)Avoid :latest and restrict exposure to the host only.
docker run -d \ --name redis \ - -p 6379:6379 \ - redis:latest + -p 127.0.0.1:6379:6379 \ + redis:7-alpinesrc/api/routes/mod.rs (1)
12-12: Add docs bullet for new routeKeep the top-of-file route list in sync by adding “/api-keys”.
//! * `/notifications` - Notification management endpoints //! * `/signers` - Signer management endpoints +//! * `/api-keys` - API key management endpointssrc/models/api_key/request.rs (1)
1-8: Harden request model: deny unknown fields and add basic derives.Prevents silent typos in client payloads and eases testing/debugging.
Apply:
use serde::{Deserialize, Serialize}; -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Debug, Clone)] +#[serde(deny_unknown_fields)] /// Request payload to create an API key. pub struct ApiKeyRequest { pub name: String, pub permissions: Vec<String>, pub allowed_origins: Option<Vec<String>>, }src/domain/transaction/util.rs (1)
49-54: Nit: simplify error mapping.Use
map_err(Into::into)for brevity.- .map_err(|e| e.into()) + .map_err(Into::into)src/api/routes/api_keys.rs (3)
1-2: Minor wording tweak in module docs.“api keys operations” → “API key operations” for grammar/consistency.
9-16: Return type clarity.Returning
Result<HttpResponse, ApiError>is more explicit (and consistent with controllers). Currentimpl Responderworks only ifApiError: ResponseError.-use actix_web::{delete, get, post, web, Responder}; +use actix_web::{delete, get, post, web, HttpResponse, Responder}; @@ -async fn list_api_keys( +async fn list_api_keys( query: web::Query<PaginationQuery>, data: web::ThinData<DefaultAppState>, -) -> impl Responder { +) -> Result<HttpResponse, crate::models::ApiError> { api_key::list_api_keys(query.into_inner(), data).await }
52-53: Add basic route tests.Consider minimal happy-path tests for list/create/delete to prevent regressions and to ensure we never leak the API key value.
I can scaffold actix tests for these handlers if you want.
src/models/api_key/response.rs (2)
4-11: Prefer RFC3339 timestamps and avoid unnecessary Deserialize.
- Use RFC3339 for
created_at(current repo model usesUtc::now().to_string(), which isn’t RFC3339).Deserializeisn’t typically needed for response DTOs.-#[derive(Serialize, Deserialize, Debug, Clone)] +#[derive(Serialize, Debug, Clone)] pub struct ApiKeyResponse { @@ - pub created_at: String, + pub created_at: String, // ensure RFC3339 format at the sourceOutside this file (repository model), prefer:
// in src/models/api_key/repository.rs when setting created_at: created_at: Utc::now().to_rfc3339(),
13-25: UseFrominstead ofTryFrom(conversion is infallible).The mapping can’t fail and never uses the
ApiError. ImplementFrom<ApiKeyRepoModel>and drop theApiErrorimport.-use crate::models::{ApiError, ApiKeyRepoModel}; +use crate::models::ApiKeyRepoModel; @@ -impl TryFrom<ApiKeyRepoModel> for ApiKeyResponse { - type Error = ApiError; - - fn try_from(api_key: ApiKeyRepoModel) -> Result<Self, ApiError> { - Ok(ApiKeyResponse { - id: api_key.id, - name: api_key.name, - allowed_origins: api_key.allowed_origins, - created_at: api_key.created_at, - permissions: api_key.permissions, - }) - } -} +impl From<ApiKeyRepoModel> for ApiKeyResponse { + fn from(api_key: ApiKeyRepoModel) -> Self { + ApiKeyResponse { + id: api_key.id, + name: api_key.name, + allowed_origins: api_key.allowed_origins, + created_at: api_key.created_at, + permissions: api_key.permissions, + } + } +}src/api/controllers/plugin.rs (1)
33-65: Consider authorization via API keys (follow-up to TODO).With
AKRin scope, you can gate plugin calls by API key permissions (e.g., “plugins:invoke”) before executing.I can draft a light middleware/guard that resolves an API key from headers and checks
list_permissions.src/models/app_state.rs (1)
82-84: Nit: align impl bounds with struct bounds for consistency.Consider adding
+ Send + Sync + 'statictoJ(and others, if desired) in the impl header to mirror the struct definition and avoid confusion.-impl< - J: JobProducerTrait, +impl< + J: JobProducerTrait + Send + Sync + 'static, RR: RelayerRepository + Repository<RelayerRepoModel, String> + Send + Sync + 'static, @@ - PR: PluginRepositoryTrait + Send + Sync + 'static, - AKR: ApiKeyRepositoryTrait + Send + Sync + 'static, + PR: PluginRepositoryTrait + Send + Sync + 'static, + AKR: ApiKeyRepositoryTrait + Send + Sync + 'static, > AppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>src/models/api_key/repository.rs (4)
12-16: Ensure serde helpers are imported from a stable path.If
serialize_secret_string/deserialize_secret_stringare not re-exported atcrate::utils, import them from the concrete module to avoid fragile re-exports.Possible tweak:
-use crate::{ - models::{ApiError, ApiKeyRequest, SecretString}, - utils::{deserialize_secret_string, serialize_secret_string}, -}; +use crate::{ + models::{ApiError, ApiKeyRequest, SecretString}, + utils::serde::repository_encryption::{deserialize_secret_string, serialize_secret_string}, +};
19-20: Normalize timestamps to RFC 3339 with milliseconds (and keep UTC).Stringifying
Utc::now()can be locale/format dependent. Prefer explicit RFC 3339 for stable persistence and interop.Apply:
-use chrono::Utc; +use chrono::{SecondsFormat, Utc}- created_at: Utc::now().to_string(), + created_at: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true),- created_at: Utc::now().to_string(), + created_at: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true),Also applies to: 36-37, 53-54
49-49: Strengthen API key value generation.A UUIDv4 (~122 bits) is acceptable but many ecosystems expect longer, symbol-rich tokens. Consider 32 random bytes encoded with URL-safe base64 for ~256-bit entropy.
Example (no external deps beyond base64 + getrandom):
- value: SecretString::new(&Uuid::new_v4().to_string()), + value: SecretString::new(&generate_api_key_value()),fn generate_api_key_value() -> String { let mut buf = [0u8; 32]; getrandom::getrandom(&mut buf).expect("secure RNG"); base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(buf) }
58-94: Add a serde round-trip test to ensure the secret isn’t leaked in plaintext.Current tests don’t verify that
valueis encrypted on serialize and correctly decrypted on deserialize.Proposed test:
#[test] fn test_api_key_repo_model_serde_encryption_roundtrip() { let model = ApiKeyRepoModel::new( "x".into(), SecretString::new("super-secret"), vec!["relayer:all:execute".into()], vec!["https://example.com".into()], ); let json = serde_json::to_string(&model).unwrap(); assert!(!json.contains("super-secret"), "plaintext leaked in JSON"); let back: ApiKeyRepoModel = serde_json::from_str(&json).unwrap(); assert_eq!(back.value, SecretString::new("super-secret")); }src/domain/relayer/mod.rs (1)
358-366: All call sites now include the newAKRgeneric—no instances of the old arity remain.Optional: in modules still referencing
ThinData<AppState<…>>directly (e.g. domain/relayer/mod.rs, services/plugins/mod.rs, services/plugins/relayer_api.rs), import and use the existingThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>alias to reduce verbosity.src/services/plugins/socket.rs (2)
141-148: Spawning then immediately awaiting defeats concurrency.
tokio::spawn(...).awaitserializes connection handling. If you want concurrent connections, consider a JoinSet and drain on shutdown, or detach tasks and stream traces back via an mpsc channel.I can provide a JoinSet refactor if you want it.
259-262: Bump tiny timeout to reduce CI flakiness.100 ms is tight under load.
Apply:
- let result = timeout(Duration::from_millis(100), listen_handle).await; + let result = timeout(Duration::from_millis(1000), listen_handle).await;src/utils/mocks.rs (1)
287-296: Prefer constructor to ensure invariants; keep deterministic ID if needed.Using
ApiKeyRepoModel::new(...)centralizes defaults (e.g.,created_at), then overrideidfor test determinism.Option A (constructor + override):
- ApiKeyRepoModel { - id: "test-api-key".to_string(), - name: "test-name".to_string(), - value: SecretString::new("test-value"), - allowed_origins: vec!["*".to_string()], - permissions: vec!["relayer:all:execute".to_string()], - created_at: Utc::now().to_string(), - } + { + let mut key = ApiKeyRepoModel::new( + "test-name".to_string(), + SecretString::new("test-value"), + vec!["relayer:all:execute".to_string()], + vec!["*".to_string()], + ); + key.id = "test-api-key".to_string(); + key + }If deterministic ID isn’t required, just return
ApiKeyRepoModel::new(...).src/domain/relayer/util.rs (1)
73-87: AKR added toget_network_relayer— LGTM.Minor nit: avoid cloning
relayer_idbelow.Apply:
- let relayer_model = get_relayer_by_id(relayer_id.clone(), state).await?; + let relayer_model = get_relayer_by_id(relayer_id, state).await?;src/api/routes/relayer.rs (2)
193-216: Consider reordering route registration for clarity and future-proofing.Place the more specific "by-nonce" route before the generic "{transaction_id}" route to avoid accidental shadowing if patterns change later. Not strictly required today, but aligns better with the comment about registering literal segments first.
pub fn init(cfg: &mut web::ServiceConfig) { // Register routes with literal segments before routes with path parameters cfg.service(delete_pending_transactions); // /relayers/{id}/transactions/pending // Then register other routes - cfg.service(cancel_transaction); // /relayers/{id}/transactions/{tx_id} - cfg.service(replace_transaction); // /relayers/{id}/transactions/{tx_id} - cfg.service(get_transaction_by_id); // /relayers/{id}/transactions/{tx_id} - cfg.service(get_transaction_by_nonce); // /relayers/{id}/transactions/by-nonce/{nonce} + cfg.service(get_transaction_by_nonce); // /relayers/{id}/transactions/by-nonce/{nonce} + cfg.service(cancel_transaction); // /relayers/{id}/transactions/{tx_id} + cfg.service(replace_transaction); // /relayers/{id}/transactions/{tx_id} + cfg.service(get_transaction_by_id); // /relayers/{id}/transactions/{tx_id} cfg.service(send_transaction); // /relayers/{id}/transactions cfg.service(list_transactions); // /relayers/{id}/transactions cfg.service(get_relayer_status); // /relayers/{id}/status cfg.service(get_relayer_balance); // /relayers/{id}/balance cfg.service(sign); // /relayers/{id}/sign cfg.service(sign_typed_data); // /relayers/{id}/sign-typed-data cfg.service(sign_transaction); // /relayers/{id}/sign-transaction cfg.service(rpc); // /relayers/{id}/rpc cfg.service(get_relayer); // /relayers/{id} cfg.service(create_relayer); // /relayers cfg.service(update_relayer); // /relayers/{id} cfg.service(delete_relayer); // /relayers/{id} cfg.service(list_relayers); // /relayers }
369-501: Tests assert 500; plan to assert 401/403 once API key enforcement lands.When you add API key auth/permission checks, update these route registration tests to expect Unauthorized/Forbidden for missing/invalid credentials instead of 500. I can draft the follow-up once enforcement PR is ready.
src/api/controllers/api_key.rs (5)
19-21: Remove unused import.
eyre::Resultis unused.-use eyre::Result;
34-35: Fix doc comment typo ("plugin" → "API key").-/// The result of the plugin call. +/// The created API key response.
86-96: Simplify mapping; drop the redundant intermediate Vec.No functional change; just less allocation.
- let api_key_items: Vec<ApiKeyRepoModel> = api_keys.items.into_iter().collect(); - - // Subtract the "value" from the api key to avoid exposing it. - let api_key_items: Vec<ApiKeyResponse> = api_key_items - .into_iter() - .map(ApiKeyResponse::try_from) - .collect::<Result<Vec<ApiKeyResponse>, ApiError>>()?; + // Hide "value" for listings + let api_key_items: Vec<ApiKeyResponse> = api_keys + .items + .into_iter() + .map(ApiKeyResponse::try_from) + .collect::<Result<Vec<_>, ApiError>>()?;
111-132: Expose permissions format contract.Returning
Vec<String>is fine, but please document/validate the permission naming scheme (e.g.,relayer:all:execute) to keep clients from relying on undocumented variants. I can provide a lightweight enum/validator if helpful.
35-49: Authz TODO: enforce permissions on management endpoints.These endpoints mutate credentials; ensure they are gated (e.g.,
api-keys:write,api-keys:read) once enforcement lands. Also validateallowed_originsand reject"*"in production unless explicitly configured.src/repositories/api_key/api_key_in_memory.rs (2)
113-122: Lock helper is over-abstracted for a guaranteed-success lock.
acquire_lockwraps a lock that can't fail; consider inlining to reduce noise. Optional.
125-305: Tests cover the happy paths; please add edge cases.
- Duplicate create -> ConstraintViolation
- list_paginated with
page=0andper_page=0-> InvalidData- delete_by_id on missing -> NotFound
src/services/plugins/mod.rs (4)
89-110: Good: AKR added to call_plugin generics; minor consistency nit.The inherent call_plugin uses ThinDataAppState, but other places in this file use web::ThinData<AppState<...>>. Prefer the ThinDataAppState alias everywhere for consistency.
Apply:
- async fn call_plugin<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>( + async fn call_plugin<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>( @@ - state: Arc<ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>>, + state: Arc<ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>>,
140-161: Bound J with Send + Sync to match other paths and AppState expectations.Elsewhere J is constrained as Send + Sync; aligning avoids surprising impl differences.
- J: JobProducerTrait + 'static, + J: JobProducerTrait + Send + Sync + 'static,
159-160: Unify state type usage to the ThinDataAppState alias.Currently mixes web::ThinData<AppState<...>> and ThinDataAppState<...>.
- state: Arc<web::ThinData<AppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>>>, + state: Arc<ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>>,- state: Arc<web::ThinData<AppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>>>, + state: Arc<ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>>,Also applies to: 185-186
76-87: Avoid name collision between inherent and trait methods named call_plugin.The trait impl currently calls self.call_plugin, which resolves to the inherent method but is easy to misread as recursion. Rename the inherent method to execute_plugin and call that from the trait impl.
- async fn call_plugin<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>( + async fn execute_plugin<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>(- self.call_plugin(plugin, plugin_call_request, state).await + self.execute_plugin(plugin, plugin_call_request, state).awaitAlso applies to: 181-188
src/repositories/api_key/api_key_redis.rs (5)
85-86: Fix log message: refers to “Plugin” instead of “API key”.Avoid confusion in operational logs.
- warn!("Plugin {} not found in batch fetch", ids[i]); + warn!("API key {} not found in batch fetch", ids[i]);
301-314: Rename variables and log messages from “plugin” to “api key” in has_entries.Consistent terminology improves observability.
- let plugin_list_key = self.api_key_list_key(); - debug!("Checking if plugin entries exist"); - let exists: bool = conn - .exists(&plugin_list_key) + let api_key_list_key = self.api_key_list_key(); + debug!("Checking if API key entries exist"); + let exists: bool = conn + .exists(&api_key_list_key) .await .map_err(|e| self.map_redis_error(e, "has_entries_check"))?; - debug!("Plugin entries exist: {}", exists); + debug!("API key entries exist: {}", exists);
316-352: Rename variables and log messages from “plugin” to “api key” in drop_all_entries.Also makes intent clearer when reading metrics/logs.
- let plugin_list_key = self.api_key_list_key(); - debug!("Dropping all plugin entries"); - // Get all plugin IDs first - let plugin_ids: Vec<String> = conn - .smembers(&plugin_list_key) + let api_key_list_key = self.api_key_list_key(); + debug!("Dropping all API key entries"); + // Get all API key IDs first + let api_key_ids: Vec<String> = conn + .smembers(&api_key_list_key) .await .map_err(|e| self.map_redis_error(e, "drop_all_entries_get_ids"))?; - - if plugin_ids.is_empty() { - debug!("No plugin entries to drop"); + if api_key_ids.is_empty() { + debug!("No API key entries to drop"); return Ok(()); } // Use pipeline for atomic operations let mut pipe = redis::pipe(); pipe.atomic(); - // Delete all individual plugin entries - for plugin_id in &plugin_ids { - let plugin_key = self.api_key_key(plugin_id); - pipe.del(&plugin_key); + // Delete all individual API key entries + for api_key_id in &api_key_ids { + let api_key_key = self.api_key_key(api_key_id); + pipe.del(&api_key_key); } - // Delete the plugin list key - pipe.del(&plugin_list_key); + // Delete the API key list key + pipe.del(&api_key_list_key); @@ - debug!("Dropped {} plugin entries", plugin_ids.len()); + debug!("Dropped {} API key entries", api_key_ids.len());
373-389: Test setup only deletes the list key; leftover apikey: keys can leak across tests.*Either call drop_all_entries() before each test or delete the apikey:* keys by prefix to avoid flakiness.
533-535: Remove debug print from test.Keep tests quiet unless debugging.
- println!("result: {:?}", result);src/services/plugins/runner.rs (1)
64-86: Align J bounds with trait (add Send + Sync).Not strictly required here, but keeps constraints consistent across layers and prevents accidental relaxation.
- ) -> Result<ScriptResult, PluginError> - where - J: JobProducerTrait + 'static, + ) -> Result<ScriptResult, PluginError> + where + J: JobProducerTrait + Send + Sync + 'static,src/api/controllers/signer.rs (1)
35-49: Reduce generic noise with a controller state alias.The repeated bounds make signatures hard to scan. Consider a type alias to centralize them and keep handlers readable.
Example (outside this file’s ranges):
pub type CtrlState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR> = ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>;Then use CtrlState<...> in the signatures.
Also applies to: 74-88, 112-126, 155-170, 192-206
src/repositories/api_key/mod.rs (4)
53-58: Doc typo: says “plugin repository” in API key module.Update wording to avoid confusion in generated docs.
Apply:
-/// Enum wrapper for different plugin repository implementations +/// Enum wrapper for different API key repository implementations
136-143: Equality ignores the secret value (intentional?).PartialEq excludes value; if that’s by design (avoid secret comparisons), add a comment to make it explicit. If not, include it to prevent false positives in tests.
Apply:
-impl PartialEq for ApiKeyRepoModel { +// Note: we intentionally do NOT compare `value` (the secret) to avoid handling secrets in equality checks. +impl PartialEq for ApiKeyRepoModel {
145-169: Minor: test helpers and comments say “plugins”.Several comments still reference “plugins”; rename to “API keys” for clarity.
121-127: Rename stray “plugin” identifiers in API-key Redis storage
In src/repositories/api_key/api_key_redis.rs (≈lines 303–351), change:
- Variable
plugin_list_key→api_key_list_key- Calls to
self.api_key_list_key()remain, but assign intoapi_key_list_key- Debug messages:
• “Checking if API key entries exist”
• “API key entries exist: {}”
• “Dropping all API key entries”
• “No API key entries to drop”
• “Dropped {} API key entries”src/api/controllers/notification.rs (3)
36-50: Consider a shared state alias to reduce repetition.Same suggestion as in signer.rs: a CtrlState alias keeps handler signatures concise.
Also applies to: 76-90, 110-124, 150-165, 200-214
221-236: Helpful error detail on deletion — keep it, but cap length.Listing all relayer names is great; consider truncating or limiting count to avoid oversized responses on large sets.
100-137: Permissions enforcement is not yet wired.Now that AKR is threaded, add middleware/extractor to validate API key and check permissions per route (e.g., notification:read/list, notification:write, notification:delete).
I can draft an actix middleware + tests that:
- extracts API key from header,
- validates via ApiKeyRepositoryTrait,
- enforces route-scoped permissions.
Want me to open a follow-up PR?src/services/plugins/relayer_api.rs (1)
126-131: Reduce repeated bounds/noise with a small indirection.Consider a helper inherent method name change (e.g.,
handle_request_impl) and a macro to generate the repetitive where-clauses for the 6 handlers to improve maintainability.Also applies to: 157-161, 186-190, 244-248, 289-293, 328-332, 370-374
src/bootstrap/config_processor.rs (1)
1127-1149: Consider asserting secure defaults for the created API key in the test.If you adopt least-privilege defaults, assert
permissions.is_empty()andallowed_origins.is_empty()here.src/api/controllers/relayer.rs (2)
466-470: Return a proper 400 for empty IDs to align with the rest of the file.Current code returns 200 with an error payload. Prefer an HTTP error.
- if relayer_id.is_empty() || transaction_id.is_empty() { - return Ok(HttpResponse::Ok().json(ApiResponse::<()>::error( - "Invalid relayer or transaction ID".to_string(), - ))); - } + if relayer_id.is_empty() || transaction_id.is_empty() { + return Err(ApiError::BadRequest( + "Invalid relayer or transaction ID".to_string(), + )); + }
12-33: Plan for API-key permission enforcement (middleware/hook).Once permissions are defined, add a middleware/extractor (e.g., per-route guard) to check the presented key against required action scopes before entering handlers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
examples/launchtube-plugin-example/launchtube/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
CHANGELOG.md(3 hunks)README.md(1 hunks)src/api/controllers/api_key.rs(1 hunks)src/api/controllers/mod.rs(1 hunks)src/api/controllers/notification.rs(31 hunks)src/api/controllers/plugin.rs(6 hunks)src/api/controllers/relayer.rs(68 hunks)src/api/controllers/signer.rs(37 hunks)src/api/routes/api_keys.rs(1 hunks)src/api/routes/mod.rs(2 hunks)src/api/routes/notification.rs(1 hunks)src/api/routes/relayer.rs(4 hunks)src/api/routes/signer.rs(1 hunks)src/bootstrap/config_processor.rs(22 hunks)src/bootstrap/initialize_app_state.rs(9 hunks)src/domain/relayer/mod.rs(3 hunks)src/domain/relayer/util.rs(7 hunks)src/domain/transaction/util.rs(3 hunks)src/models/api_key/mod.rs(1 hunks)src/models/api_key/repository.rs(1 hunks)src/models/api_key/request.rs(1 hunks)src/models/api_key/response.rs(1 hunks)src/models/app_state.rs(9 hunks)src/models/mod.rs(1 hunks)src/repositories/api_key/api_key_in_memory.rs(1 hunks)src/repositories/api_key/api_key_redis.rs(1 hunks)src/repositories/api_key/mod.rs(1 hunks)src/repositories/mod.rs(1 hunks)src/services/plugins/mod.rs(9 hunks)src/services/plugins/relayer_api.rs(30 hunks)src/services/plugins/runner.rs(9 hunks)src/services/plugins/socket.rs(8 hunks)src/utils/mocks.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (24)
src/api/controllers/api_key.rs (4)
src/api/routes/api_keys.rs (4)
create_api_key(28-33)list_api_keys(11-16)get_api_key_permissions(19-24)delete_api_key(36-41)src/models/api_key/repository.rs (1)
try_from(44-55)src/models/api_key/response.rs (1)
try_from(16-24)src/models/api_response.rs (2)
success(33-40)paginated(60-67)
src/repositories/api_key/api_key_in_memory.rs (3)
src/models/api_key/repository.rs (1)
new(24-38)src/repositories/api_key/api_key_redis.rs (9)
new(25-39)create(117-154)get_by_id(209-236)list_paginated(156-207)count(289-299)list_permissions(238-247)delete_by_id(249-287)has_entries(301-314)drop_all_entries(316-352)src/repositories/api_key/mod.rs (16)
create(41-41)create(83-88)get_by_id(40-40)get_by_id(76-81)list_paginated(42-45)list_paginated(104-112)count(46-46)count(114-119)list_permissions(47-47)list_permissions(90-95)delete_by_id(48-48)delete_by_id(97-102)has_entries(49-49)has_entries(121-126)drop_all_entries(50-50)drop_all_entries(128-133)
src/api/routes/notification.rs (1)
src/utils/mocks.rs (1)
create_mock_app_state(176-271)
src/api/routes/mod.rs (1)
src/api/routes/api_keys.rs (1)
init(44-50)
src/api/routes/api_keys.rs (2)
src/api/controllers/api_key.rs (4)
list_api_keys(69-102)get_api_key_permissions(111-132)create_api_key(35-55)delete_api_key(141-159)src/api/routes/relayer.rs (1)
init(194-216)
src/bootstrap/initialize_app_state.rs (2)
src/repositories/api_key/mod.rs (2)
new_in_memory(61-63)new_redis(65-71)src/utils/mocks.rs (2)
create_mock_api_key(287-296)create_test_server_config(298-323)
src/models/api_key/response.rs (1)
src/models/api_key/repository.rs (1)
try_from(44-55)
src/repositories/api_key/api_key_redis.rs (4)
src/repositories/api_key/api_key_in_memory.rs (9)
new(36-40)create(55-59)list_paginated(66-89)get_by_id(61-64)list_permissions(96-105)delete_by_id(107-111)count(91-94)has_entries(113-116)drop_all_entries(118-122)src/repositories/api_key/mod.rs (17)
create(41-41)create(83-88)list_paginated(42-45)list_paginated(104-112)get_by_id(40-40)get_by_id(76-81)list_permissions(47-47)list_permissions(90-95)delete_by_id(48-48)delete_by_id(97-102)count(46-46)count(114-119)has_entries(49-49)has_entries(121-126)drop_all_entries(50-50)drop_all_entries(128-133)create_test_api_key(154-169)src/repositories/mod.rs (7)
create(59-59)list_paginated(62-65)get_by_id(60-60)delete_by_id(67-67)count(68-68)has_entries(71-71)drop_all_entries(76-76)src/repositories/plugin/plugin_redis.rs (2)
plugin_list_key(47-49)plugin_key(42-44)
src/services/plugins/socket.rs (1)
src/utils/mocks.rs (1)
create_mock_app_state(176-271)
src/api/routes/signer.rs (1)
src/utils/mocks.rs (1)
create_mock_app_state(176-271)
src/utils/mocks.rs (4)
src/models/app_state.rs (1)
api_key_repository(162-164)src/models/api_key/repository.rs (1)
new(24-38)src/repositories/api_key/api_key_in_memory.rs (1)
new(36-40)src/repositories/api_key/mod.rs (1)
new_in_memory(61-63)
src/api/controllers/plugin.rs (2)
src/services/plugins/mod.rs (3)
call_plugin(90-137)call_plugin(155-160)call_plugin(181-188)src/utils/mocks.rs (1)
create_mock_app_state(176-271)
src/api/controllers/signer.rs (1)
src/utils/mocks.rs (1)
create_mock_app_state(176-271)
src/models/api_key/repository.rs (2)
src/utils/serde/repository_encryption.rs (2)
deserialize_secret_string(61-80)serialize_secret_string(47-58)src/models/api_key/response.rs (1)
try_from(16-24)
src/domain/relayer/mod.rs (1)
src/api/controllers/relayer.rs (1)
create_relayer(129-212)
src/domain/transaction/util.rs (1)
src/api/controllers/relayer.rs (1)
get_transaction_by_id(450-479)
src/services/plugins/mod.rs (3)
src/api/controllers/plugin.rs (1)
call_plugin(33-65)src/utils/mocks.rs (1)
create_mock_app_state(176-271)src/services/plugins/runner.rs (2)
plugin_runner(218-219)plugin_runner(265-266)
src/repositories/api_key/mod.rs (3)
src/repositories/api_key/api_key_in_memory.rs (9)
get_by_id(61-64)create(55-59)list_paginated(66-89)count(91-94)list_permissions(96-105)delete_by_id(107-111)has_entries(113-116)drop_all_entries(118-122)new(36-40)src/repositories/api_key/api_key_redis.rs (10)
get_by_id(209-236)create(117-154)list_paginated(156-207)count(289-299)list_permissions(238-247)delete_by_id(249-287)has_entries(301-314)drop_all_entries(316-352)new(25-39)connection_manager(382-383)src/models/api_key/repository.rs (1)
new(24-38)
src/models/app_state.rs (2)
src/repositories/api_key/mod.rs (1)
new_in_memory(61-63)src/bootstrap/config_processor.rs (1)
create_test_app_state(419-463)
src/services/plugins/runner.rs (2)
src/utils/mocks.rs (1)
create_mock_app_state(176-271)src/services/plugins/mod.rs (1)
plugin_runner(239-240)
src/api/routes/relayer.rs (3)
src/models/api_key/repository.rs (1)
new(24-38)src/repositories/api_key/api_key_in_memory.rs (1)
new(36-40)src/repositories/api_key/mod.rs (1)
new_in_memory(61-63)
src/bootstrap/config_processor.rs (5)
src/models/api_key/repository.rs (1)
new(24-38)src/repositories/api_key/mod.rs (1)
new_in_memory(61-63)src/utils/mocks.rs (1)
create_test_server_config(298-323)src/models/app_state.rs (1)
create_test_app_state(174-218)src/repositories/api_key/api_key_in_memory.rs (1)
default(48-50)
src/api/controllers/notification.rs (1)
src/utils/mocks.rs (1)
create_mock_app_state(176-271)
src/api/controllers/relayer.rs (3)
src/domain/relayer/mod.rs (2)
create_relayer(361-365)create_relayer(383-482)src/domain/transaction/util.rs (1)
get_transaction_by_id(34-54)src/utils/mocks.rs (1)
create_mock_app_state(176-271)
🪛 LanguageTool
README.md
[grammar] ~287-~287: There might be a mistake here.
Context: ...ests you want to run. 3. Run the tests using single thread to avoid race conditions ...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: msrv
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: semgrep/ci
- GitHub Check: docker-scan
- GitHub Check: test
- GitHub Check: Analyze (rust)
🔇 Additional comments (56)
src/repositories/mod.rs (1)
32-34: LGTM: no name collisions detected
Thepub mod api_key; pub use api_key::*;matches the existing pattern and a full scan of allpub use *_::*insrc/**/*.rsshowed no duplicate exports across modules.CHANGELOG.md (1)
12-12: Typo fixes look good“intrinsic,” “execution,” and “transferring” corrections are correct.
Also applies to: 55-55, 165-165
src/api/routes/mod.rs (1)
29-30: Verify api_keys endpoint scope and documentationapi_keys routes are mounted under
/api/v1via
•web::scope("/api/v1").configure(api::routes::configure_routes)in main.rs:142
•.configure(api_keys::init)in src/api/routes/mod.rs:29Their paths in src/api/routes/api_keys.rs are:
• GET/api-keys
• GET/api-keys/{api_key_id}/permissions
• POST/api-keys
• DELETE/api-keys/{api_key_id}I didn’t find matching entries in README.md—please confirm the documentation includes these endpoints under the
/api/v1namespace.src/models/mod.rs (1)
53-55: No repository traits are re-exported under models::api_key
Themodels/api_key/mod.rsfile simply re-exports its ownrequest,response, andrepositorysubmodules—which define domain DTOs (ApiKeyRepoModel,PaginationQuery,RepositoryError, etc.)—and does not bring in anyApiKeyRepositoryTraitfromsrc/repositories.Likely an incorrect or invalid review comment.
src/api/routes/signer.rs (1)
75-75: Allcreate_mock_app_statecalls use six arguments; no stale five-arg call sites remain.src/api/controllers/mod.rs (1)
7-14: Add controllers export for API keys — LGTM.Doc and
pub mod api_key;addition are consistent with the PR scope.src/api/routes/notification.rs (1)
78-78: All create_mock_app_state calls use the new six-argument signature
Verified no remaining five-argument invocations.src/models/api_key/mod.rs (1)
1-8: Re-export layout is clean and keeps submodules private — good.Top-level
api_key::*surface is tidy and avoids leaking submodule paths.src/domain/transaction/util.rs (1)
16-18: Generic plumbing for ApiKey repository looks correct.Importing
ApiKeyRepositoryTraitto satisfy the newAKRbound is consistent with the broader PR changes.src/api/routes/api_keys.rs (4)
18-24: Permissions endpoint wiring looks good.Forwarding to the controller is correct and keeps the route thin.
26-33: Create endpoint safely avoids exposing secrets (via controller mapping).Good: request is
Json<ApiKeyRequest>and controller returns a response without the key value.
35-41: Delete endpoint OK.Interface and delegation look consistent with the controller API.
43-50: Route registration order is correct.Literal before parameterized avoids conflicts; registration looks good.
src/api/controllers/plugin.rs (6)
13-15: Imports updated correctly forAKR.The added
ApiKeyRepositoryTraitimport is necessary and consistent with state plumbing.
33-48: Generic bound extended withAKRis consistent.The function signature now matches the
PluginServiceexpectation ofThinDataAppState<..., AKR>.
57-59: PassingArc<ThinDataAppState<...>>is appropriate.Wrapping state in
Arcfor async plugin execution is reasonable. No clone-heavy ops here.
79-93:list_pluginssignature updated appropriately.Matches the threading of
AKRacross the app.
127-129: Test update matches newcreate_mock_app_statesignature.Looks good and keeps the test behavior unchanged.
33-48: AllThinDataAppStateusages include the newAKRparameter. Verified that everyThinDataAppState<…>now has nine generic arguments (includingAKR), so no call sites were missed.src/models/app_state.rs (9)
15-20: Imports correctly include API key repository types.
35-36: NewAKRbound onAppStateis consistent with usage.
53-55: api_key_repository field addition looks good.Public field matches existing style for other repositories.
58-60: ThinDataAppState alias updated correctly.
70-71: DefaultAppState includes ApiKeyRepositoryStorage.Keeps the default wiring symmetrical.
157-164: Getter for API key repository looks good.
183-184: Test type params updated appropriately.
216-217: Test wiring of in-memory ApiKey repo is correct.
291-299: New getter test is sound.Arc identity checks mirror other getter tests.
src/bootstrap/initialize_app_state.rs (2)
10-12: API key repository wiring looks correct.Imports, RepositoryCollection field, both InMemory/Redis initializations, and AppState exposure are consistent with the other repositories.
Also applies to: 29-29, 49-50, 90-94, 120-130
199-204: Ignore the.to_string()suggestion—passing the literal is correct
ApiKeyRepositoryTrait::get_by_idis defined to take&str, so calling it with a string literal is already type-correct.Likely an incorrect or invalid review comment.
src/models/api_key/repository.rs (1)
9-21: Repository model exposure looks good.Public fields align with repository usage, and the value is not surfaced by ApiKeyResponse.
src/domain/relayer/mod.rs (1)
29-31: Importing ApiKeyRepositoryTrait into the domain layer is appropriate.Keeps trait bounds centralized and consistent with controller/state changes.
src/services/plugins/socket.rs (3)
57-59: Import of ApiKeyRepositoryTrait is correct and consistent.Threading AKR through this module starts here; looks good.
110-131: AKR threaded throughlistensignature and bounds — LGTM.Bounds on RA/RR/TR/…/PR/AKR are consistent with the expanded AppState.
244-244: Verify allcreate_mock_app_statecall sites use the new signature – there are numerous usages across src/api/controllers, src/services/plugins, and src/api/routes; runrg -nC2 '\bcreate_mock_app_state\s*\('to confirm each call’s arguments align with the updated arity/order.
src/utils/mocks.rs (5)
16-22: Models imports updated for API keys — looks good.
24-28: Repository imports extended for AKR — OK.
229-234: Seeding API keys into in-memory repo — LGTM.Straightforward and consistent with other seed blocks.
268-270: AppState now carriesapi_key_repository— LGTM.
176-183: All create_mock_app_state callers updated to the new 6-parameter signature; no further changes needed.src/domain/relayer/util.rs (3)
22-24: ImportingApiKeyRepositoryTraitto match state generics — OK.
40-54: AKR added toget_relayer_by_idsignature and bounds — LGTM.
110-124: AKR added toget_network_relayer_by_model— LGTM.src/api/routes/relayer.rs (1)
240-367: AppState wiring looks sound.Creating minimal network/signer/relayer/transaction plus seeding an API key ensures endpoints resolve and avoids 404. No changes needed here.
src/services/plugins/mod.rs (1)
12-14: AKR threading into plugin service imports looks consistent.No issues spotted with exposing ApiKeyRepositoryTrait here.
src/services/plugins/runner.rs (2)
35-57: AKR is correctly threaded through PluginRunnerTrait.API surface stays consistent with the service layer.
97-110: Timeout path shuts down the socket before returning — good.This avoids server handle leaks on long-running scripts.
src/api/controllers/signer.rs (3)
18-20: AKR plumbed through imports and bounds looks correct.No functional change here; just wiring. LGTM.
171-173: Return a more specific status for “updates not allowed”.400 is generic; 405 Method Not Allowed (or 501 Not Implemented) better signals the contract.
Do we have ApiError variants for 405/501? If yes, prefer them here.
316-323: Test scaffolding aligns with new AppState (AKR).create_mock_app_state(None, …) correctly seeds the new api_key_repository. Good coverage.
src/api/controllers/notification.rs (2)
18-20: AKR trait inclusion looks correct.Wiring matches the new AppState. No functional change. LGTM.
285-293: Tests exercise pagination and CRUD with the new AKR.Scaffolding via create_mock_app_state(None, …) remains valid. Good breadth.
src/services/plugins/relayer_api.rs (1)
17-19: AKR is threaded consistently through traits, bounds, and state. LGTM.Also applies to: 67-80, 84-85, 90-91, 96-97, 102-103, 108-109, 114-115, 119-121, 127-146, 157-176, 186-205, 244-263, 289-308, 328-347, 370-389
src/bootstrap/config_processor.rs (1)
55-69: Wiring AKR through the config pipeline and tests looks correct.Signatures, drops, and tests align with the new repository. Nice.
Also applies to: 110-124, 149-163, 189-203, 231-245, 335-350, 371-371, 381-382, 1127-1149, 1208-1237, 461-463, 402-409, 428-429
src/api/controllers/relayer.rs (2)
29-31: AKR threaded across controller endpoints is consistent. LGTM.Also applies to: 47-61, 87-101, 129-143, 225-240, 297-311, 352-366, 384-398, 450-465, 492-507, 539-554, 586-599, 693-707, 733-747, 769-783, 805-820
417-421: No action needed—DefaultAppState includes ApiKeyRepositoryStorage
TheDefaultAppStatealias already listsApiKeyRepositoryStorageas its ninth type parameter, so the new API key repository is correctly wired.
| async fn process_api_key<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>( | ||
| server_config: &ServerConfig, | ||
| app_state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, | ||
| ) -> Result<()> | ||
| where | ||
| J: JobProducerTrait + Send + Sync + 'static, | ||
| RR: RelayerRepository + Repository<RelayerRepoModel, String> + Send + Sync + 'static, | ||
| TR: TransactionRepository + Repository<TransactionRepoModel, String> + Send + Sync + 'static, | ||
| NR: NetworkRepository + Repository<NetworkRepoModel, String> + Send + Sync + 'static, | ||
| NFR: Repository<NotificationRepoModel, String> + Send + Sync + 'static, | ||
| SR: Repository<SignerRepoModel, String> + Send + Sync + 'static, | ||
| TCR: TransactionCounterTrait + Send + Sync + 'static, | ||
| PR: PluginRepositoryTrait + Send + Sync + 'static, | ||
| AKR: ApiKeyRepositoryTrait + Send + Sync + 'static, | ||
| { | ||
| let api_key_model = ApiKeyRepoModel::new( | ||
| "default".to_string(), | ||
| server_config.api_key.clone(), | ||
| vec!["*".to_string()], | ||
| vec!["*".to_string()], | ||
| ); | ||
|
|
||
| app_state | ||
| .api_key_repository | ||
| .create(api_key_model) | ||
| .await | ||
| .wrap_err("Failed to create api key repository entry")?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Creating a default API key with wildcard permissions/origins is unsafe by default.
Shipping "*" for both permissions and allowed_origins can accidentally grant full access once enforcement lands. Default to least-privilege and require explicit config.
async fn process_api_key<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>(
@@
- let api_key_model = ApiKeyRepoModel::new(
- "default".to_string(),
- server_config.api_key.clone(),
- vec!["*".to_string()],
- vec!["*".to_string()],
- );
+ let api_key_model = ApiKeyRepoModel::new(
+ "default".to_string(),
+ server_config.api_key.clone(),
+ vec![], // no permissions by default
+ vec![], // no allowed origins by default
+ );If you still want permissive defaults in dev, gate them behind a config flag (e.g., ALLOW_WILDCARD_API_KEY=true) and keep prod safe-by-default.
| type Error = ApiError; | ||
|
|
||
| fn try_from(request: ApiKeyRequest) -> Result<Self, Self::Error> { | ||
| let allowed_origins = request.allowed_origins.unwrap_or(vec!["*".to_string()]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid permissive default for allowed_origins ("*").
Defaulting to "*" weakens origin checks by default. Safer options:
- Require
allowed_originsexplicitly in the request, or - Default to an empty list and treat that as “no origins allowed” until explicitly configured.
Happy to propose a validation layer and migration plan if you want to change the default.
🤖 Prompt for AI Agents
In src/models/api_key/repository.rs around line 45, the code currently defaults
allowed_origins to vec!["*".to_string()], which is too permissive; change this
to either require request.allowed_origins to be provided (return a validation
error when None) or default to an empty Vec<String> and treat that as “no
origins allowed”; implement the validation check where the request is parsed (or
add a short validation helper) so callers receive a clear error message if
origins are missing, and update any callers/tests to handle the new behavior.
| impl Clone for InMemoryApiKeyRepository { | ||
| fn clone(&self) -> Self { | ||
| // Try to get the current data, or use empty HashMap if lock fails | ||
| let data = self | ||
| .store | ||
| .try_lock() | ||
| .map(|guard| guard.clone()) | ||
| .unwrap_or_else(|_| HashMap::new()); | ||
|
|
||
| Self { | ||
| store: Mutex::new(data), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clone implementation can lose data under contention; switch to Arc<Mutex<...>>.
Falling back to an empty HashMap on try_lock silently drops data. Use Arc<Mutex<...>> and derive Clone to share the same store safely.
-use std::collections::HashMap;
+use std::collections::HashMap;
+use std::sync::Arc;
@@
#[derive(Debug)]
pub struct InMemoryApiKeyRepository {
- store: Mutex<HashMap<String, ApiKeyRepoModel>>,
+ store: Arc<Mutex<HashMap<String, ApiKeyRepoModel>>>,
}
-impl Clone for InMemoryApiKeyRepository {
- fn clone(&self) -> Self {
- // Try to get the current data, or use empty HashMap if lock fails
- let data = self
- .store
- .try_lock()
- .map(|guard| guard.clone())
- .unwrap_or_else(|_| HashMap::new());
-
- Self {
- store: Mutex::new(data),
- }
- }
-}
+impl Clone for InMemoryApiKeyRepository {
+ fn clone(&self) -> Self {
+ Self {
+ store: Arc::clone(&self.store),
+ }
+ }
+}
@@
pub fn new() -> Self {
Self {
- store: Mutex::new(HashMap::new()),
+ store: Arc::new(Mutex::new(HashMap::new())),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Clone for InMemoryApiKeyRepository { | |
| fn clone(&self) -> Self { | |
| // Try to get the current data, or use empty HashMap if lock fails | |
| let data = self | |
| .store | |
| .try_lock() | |
| .map(|guard| guard.clone()) | |
| .unwrap_or_else(|_| HashMap::new()); | |
| Self { | |
| store: Mutex::new(data), | |
| } | |
| } | |
| } | |
| use std::collections::HashMap; | |
| use std::sync::{Arc, Mutex}; | |
| #[derive(Debug)] | |
| pub struct InMemoryApiKeyRepository { | |
| store: Arc<Mutex<HashMap<String, ApiKeyRepoModel>>>, | |
| } | |
| impl Clone for InMemoryApiKeyRepository { | |
| fn clone(&self) -> Self { | |
| Self { | |
| store: Arc::clone(&self.store), | |
| } | |
| } | |
| } | |
| impl InMemoryApiKeyRepository { | |
| pub fn new() -> Self { | |
| Self { | |
| store: Arc::new(Mutex::new(HashMap::new())), | |
| } | |
| } | |
| // …rest of impl… | |
| } |
🤖 Prompt for AI Agents
In src/repositories/api_key/api_key_in_memory.rs around lines 20 to 33, the
current Clone impl attempts a try_lock and falls back to an empty HashMap which
can silently drop data under contention; change the repository's store field to
Arc<Mutex<HashMap<...>>> and derive/implement Clone so clones share the same Arc
instead of copying contents, update constructors and any usage to wrap the
existing Mutex in Arc::new(...) and remove the custom Clone impl so the derived
Clone is used.
| async fn create(&self, api_key: ApiKeyRepoModel) -> Result<ApiKeyRepoModel, RepositoryError> { | ||
| let mut store = Self::acquire_lock(&self.store).await?; | ||
| store.insert(api_key.id.clone(), api_key.clone()); | ||
| Ok(api_key) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Create should enforce uniqueness to match Redis behavior.
Currently overwrites existing keys; Redis impl returns ConstraintViolation. Align semantics.
async fn create(&self, api_key: ApiKeyRepoModel) -> Result<ApiKeyRepoModel, RepositoryError> {
- let mut store = Self::acquire_lock(&self.store).await?;
- store.insert(api_key.id.clone(), api_key.clone());
- Ok(api_key)
+ let mut store = Self::acquire_lock(&self.store).await?;
+ if store.contains_key(&api_key.id) {
+ return Err(RepositoryError::ConstraintViolation(format!(
+ "API Key with ID {} already exists",
+ api_key.id
+ )));
+ }
+ store.insert(api_key.id.clone(), api_key.clone());
+ Ok(api_key)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn create(&self, api_key: ApiKeyRepoModel) -> Result<ApiKeyRepoModel, RepositoryError> { | |
| let mut store = Self::acquire_lock(&self.store).await?; | |
| store.insert(api_key.id.clone(), api_key.clone()); | |
| Ok(api_key) | |
| } | |
| async fn create(&self, api_key: ApiKeyRepoModel) -> Result<ApiKeyRepoModel, RepositoryError> { | |
| let mut store = Self::acquire_lock(&self.store).await?; | |
| if store.contains_key(&api_key.id) { | |
| return Err(RepositoryError::ConstraintViolation(format!( | |
| "API Key with ID {} already exists", | |
| api_key.id | |
| ))); | |
| } | |
| store.insert(api_key.id.clone(), api_key.clone()); | |
| Ok(api_key) | |
| } |
| let start = ((query.page - 1) * query.per_page) as usize; | ||
| let end = (start + query.per_page as usize).min(all_ids.len()); | ||
|
|
||
| let ids_to_query = &all_ids[start..end]; | ||
| let items = self.get_by_ids(ids_to_query).await?; | ||
|
|
||
| Ok(PaginatedResult { | ||
| items: items.results.clone(), | ||
| total, | ||
| page: query.page, | ||
| per_page: query.per_page, | ||
| }) |
There was a problem hiding this comment.
Pagination can panic when page is out of range (start > len).
Slicing with all_ids[start..end] panics if start > all_ids.len(). Return an empty page instead.
- let start = ((query.page - 1) * query.per_page) as usize;
- let end = (start + query.per_page as usize).min(all_ids.len());
-
- let ids_to_query = &all_ids[start..end];
- let items = self.get_by_ids(ids_to_query).await?;
+ let start = ((query.page - 1) * query.per_page) as usize;
+ if start >= all_ids.len() {
+ return Ok(PaginatedResult {
+ items: vec![],
+ total,
+ page: query.page,
+ per_page: query.per_page,
+ });
+ }
+ let end = (start + query.per_page as usize).min(all_ids.len());
+ let ids_to_query = &all_ids[start..end];
+ let items = self.get_by_ids(ids_to_query).await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let start = ((query.page - 1) * query.per_page) as usize; | |
| let end = (start + query.per_page as usize).min(all_ids.len()); | |
| let ids_to_query = &all_ids[start..end]; | |
| let items = self.get_by_ids(ids_to_query).await?; | |
| Ok(PaginatedResult { | |
| items: items.results.clone(), | |
| total, | |
| page: query.page, | |
| per_page: query.per_page, | |
| }) | |
| let start = ((query.page - 1) * query.per_page) as usize; | |
| if start >= all_ids.len() { | |
| return Ok(PaginatedResult { | |
| items: vec![], | |
| total, | |
| page: query.page, | |
| per_page: query.per_page, | |
| }); | |
| } | |
| let end = (start + query.per_page as usize).min(all_ids.len()); | |
| let ids_to_query = &all_ids[start..end]; | |
| let items = self.get_by_ids(ids_to_query).await?; | |
| Ok(PaginatedResult { | |
| items: items.results.clone(), | |
| total, | |
| page: query.page, | |
| per_page: query.per_page, | |
| }) |
🤖 Prompt for AI Agents
In src/repositories/api_key/api_key_redis.rs around lines 195 to 206, the slice
all_ids[start..end] will panic when start >= all_ids.len(); change the logic to
detect when start is out of range and return an empty page instead of slicing:
if start >= all_ids.len() construct and return a PaginatedResult with an empty
items vector (and the original total, page, per_page) or set ids_to_query to an
empty slice and avoid calling get_by_ids; otherwise compute end as before and
proceed. Ensure no slicing is performed when start is out of bounds so the
function never panics.
| #[async_trait] | ||
| #[allow(dead_code)] | ||
| #[cfg_attr(test, automock)] | ||
| pub trait ApiKeyRepositoryTrait { | ||
| async fn get_by_id(&self, id: &str) -> Result<Option<ApiKeyRepoModel>, RepositoryError>; | ||
| async fn create(&self, api_key: ApiKeyRepoModel) -> Result<ApiKeyRepoModel, RepositoryError>; | ||
| async fn list_paginated( | ||
| &self, | ||
| query: PaginationQuery, | ||
| ) -> Result<PaginatedResult<ApiKeyRepoModel>, RepositoryError>; | ||
| async fn count(&self) -> Result<usize, RepositoryError>; | ||
| async fn list_permissions(&self, api_key_id: &str) -> Result<Vec<String>, RepositoryError>; | ||
| async fn delete_by_id(&self, api_key_id: &str) -> Result<(), RepositoryError>; | ||
| async fn has_entries(&self) -> Result<bool, RepositoryError>; | ||
| async fn drop_all_entries(&self) -> Result<(), RepositoryError>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify repository contract (duplicates and pagination invariants).
Trait docs should state:
- create must enforce uniqueness by id (error on duplicate) or be idempotent — pick one, but be consistent across backends.
- list_paginated must reject page == 0 or per_page == 0 (as Redis does).
I can add the docs and wire validations if you confirm the desired semantics.
🤖 Prompt for AI Agents
In src/repositories/api_key/mod.rs around lines 36–51, the ApiKeyRepositoryTrait
lacks documentation and contract guarantees: explicitly document whether create
is idempotent or must error on duplicate IDs (choose one and make all backends
follow it), and state that list_paginated must reject page == 0 or per_page == 0
(matching Redis); add doc comments to the trait and methods describing these
invariants, and ensure implementations perform the corresponding runtime
validations (return RepositoryError on duplicate create or make create
idempotent, and validate/reject invalid pagination args) and update/add tests to
cover these behaviors.
| async fn create(&self, api_key: ApiKeyRepoModel) -> Result<ApiKeyRepoModel, RepositoryError> { | ||
| match self { | ||
| ApiKeyRepositoryStorage::InMemory(repo) => repo.create(api_key).await, | ||
| ApiKeyRepositoryStorage::Redis(repo) => repo.create(api_key).await, | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent create semantics between backends (bug).
InMemory overwrites on duplicate id; Redis errors with ConstraintViolation. Behavior divergence will cause tests to pass while prod fails.
Preferred fix: enforce uniqueness in-memory too. Patch (in src/repositories/api_key/api_key_in_memory.rs):
// inside create(...)
let mut store = Self::acquire_lock(&self.store).await?;
if store.contains_key(&api_key.id) {
return Err(RepositoryError::ConstraintViolation(
format!("API Key with ID {} already exists", api_key.id),
));
}
store.insert(api_key.id.clone(), api_key.clone());
Ok(api_key)Update the duplicate test in this file accordingly (see separate comment).
🤖 Prompt for AI Agents
In src/repositories/api_key/mod.rs around lines 83 to 88, the create method
delegates to backends that currently have inconsistent semantics (InMemory
overwrites duplicates while Redis returns ConstraintViolation); update the
InMemory backend to enforce uniqueness: acquire the store lock, check if
store.contains_key(&api_key.id) and if so return
RepositoryError::ConstraintViolation with a descriptive message, otherwise
insert and return the created api_key; also update the duplicate-test in the
InMemory test file to expect a ConstraintViolation error instead of overwrite
behavior.
| async fn list_paginated( | ||
| &self, | ||
| query: PaginationQuery, | ||
| ) -> Result<PaginatedResult<ApiKeyRepoModel>, RepositoryError> { | ||
| match self { | ||
| ApiKeyRepositoryStorage::InMemory(repo) => repo.list_paginated(query).await, | ||
| ApiKeyRepositoryStorage::Redis(repo) => repo.list_paginated(query).await, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align pagination validation across backends.
Redis checks page/per_page > 0; InMemory doesn’t, risking wraparound on (page - 1). Add the same validation to InMemory.
Proposed (in src/repositories/api_key/api_key_in_memory.rs list_paginated):
if query.page == 0 { return Err(RepositoryError::InvalidData("Page number must be greater than 0".into())); }
if query.per_page == 0 { return Err(RepositoryError::InvalidData("Per page count must be greater than 0".into())); }🤖 Prompt for AI Agents
In src/repositories/api_key/mod.rs around lines 104-112 the InMemory backend's
list_paginated path lacks pagination validation present in Redis; update
src/repositories/api_key/api_key_in_memory.rs in the list_paginated method to
check if query.page == 0 and if query.per_page == 0 and return
Err(RepositoryError::InvalidData(...)) for each case (use the same error
messages/semantics as the Redis implementation) before performing any arithmetic
that assumes page/per_page > 0.
| #[tokio::test] | ||
| async fn test_api_key_repository_storage_add_duplicate() { | ||
| let storage = ApiKeyRepositoryStorage::new_in_memory(); | ||
| let api_key = create_test_api_key( | ||
| "test-api-key", | ||
| "test-name", | ||
| "test-value", | ||
| &["*"], | ||
| &["relayer:all:execute"], | ||
| ); | ||
|
|
||
| // Add the api key first time | ||
| storage.create(api_key.clone()).await.unwrap(); | ||
|
|
||
| // Try to add the same api key again - should succeed (overwrite) | ||
| let result = storage.create(api_key).await; | ||
| assert!(result.is_ok()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix duplicate-create test to match uniqueness contract.
Once InMemory rejects duplicates, update the test to assert an error.
Apply:
- async fn test_api_key_repository_storage_add_duplicate() {
+ async fn test_api_key_repository_storage_add_duplicate_errors() {
let storage = ApiKeyRepositoryStorage::new_in_memory();
let api_key = create_test_api_key(
"test-api-key",
"test-name",
"test-value",
&["*"],
&["relayer:all:execute"],
);
// Add the api key first time
storage.create(api_key.clone()).await.unwrap();
- // Try to add the same api key again - should succeed (overwrite)
- let result = storage.create(api_key).await;
- assert!(result.is_ok());
+ // Try to add the same api key again - should error with ConstraintViolation
+ let err = storage.create(api_key).await.unwrap_err();
+ assert!(matches!(err, RepositoryError::ConstraintViolation(_)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::test] | |
| async fn test_api_key_repository_storage_add_duplicate() { | |
| let storage = ApiKeyRepositoryStorage::new_in_memory(); | |
| let api_key = create_test_api_key( | |
| "test-api-key", | |
| "test-name", | |
| "test-value", | |
| &["*"], | |
| &["relayer:all:execute"], | |
| ); | |
| // Add the api key first time | |
| storage.create(api_key.clone()).await.unwrap(); | |
| // Try to add the same api key again - should succeed (overwrite) | |
| let result = storage.create(api_key).await; | |
| assert!(result.is_ok()); | |
| } | |
| #[tokio::test] | |
| async fn test_api_key_repository_storage_add_duplicate_errors() { | |
| let storage = ApiKeyRepositoryStorage::new_in_memory(); | |
| let api_key = create_test_api_key( | |
| "test-api-key", | |
| "test-name", | |
| "test-value", | |
| &["*"], | |
| &["relayer:all:execute"], | |
| ); | |
| // Add the api key first time | |
| storage.create(api_key.clone()).await.unwrap(); | |
| // Try to add the same api key again - should error with ConstraintViolation | |
| let err = storage.create(api_key).await.unwrap_err(); | |
| assert!(matches!(err, RepositoryError::ConstraintViolation(_))); | |
| } |
🤖 Prompt for AI Agents
In src/repositories/api_key/mod.rs around lines 213 to 230, the test
test_api_key_repository_storage_add_duplicate currently expects a second create
to succeed but the InMemory implementation now rejects duplicates; update the
test to assert an error instead of ok: after the first
storage.create(...).await.unwrap(), call storage.create(api_key).await and
assert that it returns an Err (e.g., use assert!(result.is_err()) or
result.unwrap_err()), and update the test comment to indicate duplicates should
be rejected.
dylankilkenny
left a comment
dylankilkenny
left a comment
There was a problem hiding this comment.
LGTM Marcos!
Will valid permissions be defined in the next PR? Also wondering about docs whether we should add them with this effort also
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests {} |
| pub name: String, | ||
| pub allowed_origins: Vec<String>, | ||
| pub created_at: String, | ||
| pub permissions: Vec<String>, |
There was a problem hiding this comment.
Is there a list of valid permissions?
There was a problem hiding this comment.
Not yet, in the next PR my plan is to actually implement them possibly using enums
There was a problem hiding this comment.
LGTM
I see that PR has description that docs and api key enforcement are in TODO.
I am not sure if that work will be part of another PR or this one.
We also need api key management permissions, to limit creation and deletion of api keys, in addition to tx related permissions.
|
@dylankilkenny @zeljkoX thanks for the reviews! As you pointed yes, the idea is to add
in next PR, I think we can merge this one as it doesnt modify any existing behaviour just adds unharmful endpoints, wdyt? |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/plugins/runner.rs (1)
99-116: Avoid leaking the socket listener task on script timeout.When the script times out you send the shutdown signal but immediately return without awaiting the spawned server. Await the JoinHandle to ensure clean shutdown and surface panics.
Apply this diff:
Err(_) => { // ensures the socket gets closed. let _ = shutdown_tx.send(()); - return Err(PluginError::ScriptTimeout(timeout_duration.as_secs())); + // ensure the listener finishes to avoid a leaked task + let _ = server_handle.await; + return Err(PluginError::ScriptTimeout(timeout_duration.as_secs())); }src/services/plugins/relayer_api.rs (1)
404-472: Trait impl recursively calls itself; results in infinite recursion. Use inherent methods explicitly.Each method in the RelayerApiTrait impl calls the same trait method (self.handle_*), causing unbounded recursion when invoked via the trait path (used in tests). Call the inherent RelayerApi methods instead.
Apply this diff:
impl<J, RR, TR, NR, NFR, SR, TCR, PR, AKR> RelayerApiTrait<J, RR, TR, NR, NFR, SR, TCR, PR, AKR> for RelayerApi { async fn handle_request( &self, request: Request, - state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, + state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Response { - self.handle_request(request, state).await + RelayerApi::handle_request(self, request, state).await } async fn process_request( &self, request: Request, - state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, + state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.process_request(request, state).await + RelayerApi::process_request(self, request, state).await } async fn handle_send_transaction( &self, request: Request, - state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, + state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_send_transaction(request, state).await + RelayerApi::handle_send_transaction(self, request, state).await } async fn handle_get_transaction( &self, request: Request, - state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, + state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_get_transaction(request, state).await + RelayerApi::handle_get_transaction(self, request, state).await } async fn handle_get_relayer_status( &self, request: Request, - state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, + state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_get_relayer_status(request, state).await + RelayerApi::handle_get_relayer_status(self, request, state).await } async fn handle_sign_transaction( &self, request: Request, - state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, + state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_sign_transaction(request, state).await + RelayerApi::handle_sign_transaction(self, request, state).await } async fn handle_get_relayer_info( &self, request: Request, - state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, + state: &ThinDataAppState<J, RR, TR, NR, NFR, SR, TCR, PR, AKR>, ) -> Result<Response, PluginError> { - self.handle_get_relayer_info(request, state).await + RelayerApi::handle_get_relayer_info(self, request, state).await } }
🧹 Nitpick comments (12)
src/services/plugins/script_executor.rs (1)
232-237: Stabilize the error assertion to avoid brittle string matching.Relying on the exact "Plugin executor failed" phrasing risks flakes across ts-node/executor variants. Assert on the semantic signal (non-empty stderr + presence of the TS symbol) and that no return value was produced.
- // TypeScript compilation errors are now returned in the error field - assert!(!result.error.is_empty()); - assert!(result.error.contains("Plugin executor failed")); - assert!(result.error.contains("logger")); + // TypeScript compilation errors are returned in the error field + assert!(!result.error.is_empty()); + assert!(result.error.contains("logger")); + assert!(result.return_value.is_empty());src/api/controllers/signer.rs (1)
314-322: De‑duplicate repeated test setup.create_mock_app_state(None, None, None, None, None, None).await is repeated across tests. Extract a tiny helper to reduce noise.
Example helper (place inside tests module):
async fn empty_app_state() -> DefaultAppState { create_mock_app_state(None, None, None, None, None, None).await }Then update calls, e.g.:
- let app_state = create_mock_app_state(None, None, None, None, None, None).await; + let app_state = empty_app_state().await;Also applies to: 340-357, 377-386
src/bootstrap/initialize_app_state.rs (1)
148-165: Minor: add an assertion for api_key_repository operations in in‑memory path.You already validate strong_count; consider a tiny CRUD smoke test like others (create/get) to mirror parity with Redis path.
src/repositories/api_key/api_key_redis.rs (3)
289-299: Count should match the chosen index type.If you adopt ZSET for ordering, use ZCARD here too.
- let count: u64 = conn - .scard(&api_key_list_key) + let count: u64 = conn + .zcard(&api_key_list_key) .await .map_err(|e| self.map_redis_error(e, "count_api_keys"))?;
301-314: has_entries uses EXISTS on the index key; prefer size > 0 to avoid false positives.EXISTS can be true for an empty ZSET. Use ZCARD > 0.
- let plugin_list_key = self.api_key_list_key(); - - debug!("Checking if plugin entries exist"); - - let exists: bool = conn - .exists(&plugin_list_key) - .await - .map_err(|e| self.map_redis_error(e, "has_entries_check"))?; - - debug!("Plugin entries exist: {}", exists); - Ok(exists) + let list_key = self.api_key_list_key(); + debug!("Checking if API key entries exist"); + let count: u64 = conn + .zcard(&list_key) + .await + .map_err(|e| self.map_redis_error(e, "has_entries_check"))?; + let exists = count > 0; + debug!("API key entries exist: {}", exists); + Ok(exists)
316-352: Copy/paste typos: “Plugin” logs and vars in ApiKey repo.Rename misleading log messages/vars to “api key”; also align drop_all_entries with ZSET.
- let plugin_list_key = self.api_key_list_key(); - - debug!("Dropping all plugin entries"); - - // Get all plugin IDs first - let plugin_ids: Vec<String> = conn - .smembers(&plugin_list_key) + let list_key = self.api_key_list_key(); + debug!("Dropping all API key entries"); + // Get all API key IDs first + let api_key_ids: Vec<String> = conn + .zrange(&list_key, 0, -1) .await .map_err(|e| self.map_redis_error(e, "drop_all_entries_get_ids"))?; - if plugin_ids.is_empty() { - debug!("No plugin entries to drop"); + if api_key_ids.is_empty() { + debug!("No API key entries to drop"); return Ok(()); } @@ - for plugin_id in &plugin_ids { - let plugin_key = self.api_key_key(plugin_id); - pipe.del(&plugin_key); + for api_key_id in &api_key_ids { + let key = self.api_key_key(api_key_id); + pipe.del(&key); } @@ - pipe.del(&plugin_list_key); + pipe.del(&list_key); @@ - debug!("Dropped {} plugin entries", plugin_ids.len()); + debug!("Dropped {} API key entries", api_key_ids.len());Also fix earlier message:
- warn!("Plugin {} not found in batch fetch", ids[i]); + warn!("API key {} not found in batch fetch", ids[i]);src/api/routes/relayer.rs (1)
382-500: Prefer “route is registered” assertions that don’t pin to 500.These checks break if handlers start returning 200/4xx. Assert “not 404” instead.
Apply:
- assert_eq!(resp.status(), StatusCode::INTERNAL_SERVER_ERROR); + assert_ne!(resp.status(), StatusCode::NOT_FOUND);Repeat the replacement for each route assertion in this test.
src/services/plugins/mod.rs (1)
81-88: Harden plugin path handling.Guard against absolute paths and “..” segments to avoid unintended file access.
Example check inside call_plugin before using script_path:
let p = Self::resolve_plugin_path(&plugin.path); let path = std::path::Path::new(&p).components().collect::<Vec<_>>(); use std::path::Component; if std::path::Path::new(&p).is_absolute() || path.iter().any(|c| matches!(c, Component::ParentDir)) { return Err(PluginError::InvalidPayload("Invalid plugin path".into())); } let script_path = p;Also applies to: 111-126
src/services/plugins/relayer_api.rs (1)
80-120: Use the ThinDataAppState alias in trait signatures for consistency.Minor consistency tweak: the trait uses web::ThinData directly while inherent methods use ThinDataAppState. Prefer the alias in both.
src/bootstrap/config_processor.rs (3)
23-52: Make seeding idempotent to avoid duplicates on re-processing.Before create, check for an existing key by name/value and skip or upsert.
Would you like a follow-up patch that adds a simple get_by_name helper and uses it here?
1127-1149: Nice targeted test for API key seeding. Consider asserting permissions/origins are empty after the safety change.Update assertions once defaults switch to least-privilege.
-assert_eq!(stored_api_keys.items[0].name, "default"); +assert_eq!(stored_api_keys.items[0].name, "default"); +assert!(stored_api_keys.items[0].permissions.is_empty()); +assert!(stored_api_keys.items[0].allowed_origins.is_empty());
1198-1237: process_config_file test: also verify API key seeding.Add a quick check that one default API key exists after processing.
let stored_plugin = plugin_repo.get_by_id("test-plugin-1").await?; assert_eq!(stored_plugin.unwrap().path, "/app/plugins/test.ts"); - Ok(()) + // Verify default API key was seeded + let api_keys = api_key_repo.list_paginated(PaginationQuery { page: 1, per_page: 10 }).await?; + assert_eq!(api_keys.items.len(), 1); + assert_eq!(api_keys.items[0].name, "default"); + + Ok(())Also applies to: 1259-1262
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
README.md(1 hunks)src/api/controllers/signer.rs(37 hunks)src/api/routes/relayer.rs(4 hunks)src/bootstrap/config_processor.rs(22 hunks)src/bootstrap/initialize_app_state.rs(9 hunks)src/domain/relayer/util.rs(7 hunks)src/repositories/api_key/api_key_redis.rs(1 hunks)src/services/plugins/mod.rs(9 hunks)src/services/plugins/relayer_api.rs(30 hunks)src/services/plugins/runner.rs(7 hunks)src/services/plugins/script_executor.rs(1 hunks)src/utils/mocks.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/mocks.rs
- src/domain/relayer/util.rs
- README.md
🧰 Additional context used
🧬 Code graph analysis (7)
src/repositories/api_key/api_key_redis.rs (4)
src/models/api_key/repository.rs (1)
new(24-38)src/repositories/api_key/api_key_in_memory.rs (9)
new(36-40)create(55-59)list_paginated(66-89)get_by_id(61-64)list_permissions(96-105)delete_by_id(107-111)count(91-94)has_entries(113-116)drop_all_entries(118-122)src/repositories/api_key/mod.rs (16)
create(41-41)create(83-88)list_paginated(42-45)list_paginated(104-112)get_by_id(40-40)get_by_id(76-81)list_permissions(47-47)list_permissions(90-95)delete_by_id(48-48)delete_by_id(97-102)count(46-46)count(114-119)has_entries(49-49)has_entries(121-126)drop_all_entries(50-50)drop_all_entries(128-133)src/repositories/plugin/plugin_redis.rs (2)
plugin_list_key(47-49)plugin_key(42-44)
src/bootstrap/initialize_app_state.rs (2)
src/repositories/api_key/mod.rs (2)
new_in_memory(61-63)new_redis(65-71)src/utils/mocks.rs (1)
create_mock_api_key(288-297)
src/api/controllers/signer.rs (1)
src/utils/mocks.rs (1)
create_mock_app_state(177-272)
src/services/plugins/runner.rs (2)
src/utils/mocks.rs (1)
create_mock_app_state(177-272)src/services/plugins/mod.rs (1)
plugin_runner(240-241)
src/services/plugins/mod.rs (3)
src/api/controllers/plugin.rs (1)
call_plugin(33-65)src/utils/mocks.rs (1)
create_mock_app_state(177-272)src/services/plugins/runner.rs (2)
plugin_runner(192-193)plugin_runner(243-244)
src/api/routes/relayer.rs (3)
src/models/api_key/repository.rs (1)
new(24-38)src/repositories/api_key/api_key_in_memory.rs (1)
new(36-40)src/repositories/api_key/mod.rs (1)
new_in_memory(61-63)
src/bootstrap/config_processor.rs (5)
src/models/api_key/repository.rs (1)
new(24-38)src/repositories/api_key/mod.rs (1)
new_in_memory(61-63)src/utils/mocks.rs (1)
create_test_server_config(299-324)src/models/app_state.rs (1)
create_test_app_state(174-218)src/repositories/api_key/api_key_in_memory.rs (1)
default(48-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: msrv
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Analyze (rust)
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: semgrep/ci
🔇 Additional comments (14)
src/services/plugins/script_executor.rs (1)
210-237: Guard tests when ts-node is unavailable to prevent CI flakes.If ts-node isn’t on PATH in some CI runners, these tests will fail with environment errors. Prefer skipping early.
// Add near the start of each test: if Command::new("ts-node").arg("--version").output().await.is_err() { eprintln!("skipping: ts-node not installed or not in PATH"); return; }src/api/controllers/signer.rs (1)
18-20: AKR threading looks consistent.Adding ApiKeyRepositoryTrait as AKR across controller signatures and state wiring is consistent and non‑intrusive. No functional regressions spotted.
Also applies to: 35-49, 74-88, 112-126, 155-170, 192-206
src/bootstrap/initialize_app_state.rs (1)
10-13: AppState wiring for ApiKey repository is solid.In‑memory and Redis branches initialize and attach api_key_repository consistently. Tests cover presence and basic CRUD. LGTM.
Also applies to: 21-30, 41-50, 86-94, 120-130
src/repositories/api_key/api_key_redis.rs (2)
156-171: Fix pagination: unordered SMEMBERS and out‑of‑range slice. Switch to ZSET + guards.Sets are unordered (unstable pages) and slicing can panic when start >= len. Use ZCARD/ZRANGE with bounds checks.
Apply:
- let total: u64 = conn - .scard(&api_key_list_key) + let total: u64 = conn + .zcard(&api_key_list_key) .await .map_err(|e| self.map_redis_error(e, "list_paginated_count"))?; @@ - // Get all IDs and paginate in memory - let all_ids: Vec<String> = conn - .smembers(&api_key_list_key) - .await - .map_err(|e| self.map_redis_error(e, "list_paginated_members"))?; - - let start = ((query.page - 1) * query.per_page) as usize; - let end = (start + query.per_page as usize).min(all_ids.len()); - - let ids_to_query = &all_ids[start..end]; - let items = self.get_by_ids(ids_to_query).await?; + let start = ((query.page - 1) * query.per_page) as i64; + if start as u64 >= total { + return Ok(PaginatedResult { + items: vec![], + total, + page: query.page, + per_page: query.per_page, + }); + } + let end = (start + query.per_page as i64 - 1).max(start); + let ids_page: Vec<String> = conn + .zrange(&api_key_list_key, start, end) + .await + .map_err(|e| self.map_redis_error(e, "list_paginated_range"))?; + let items = self.get_by_ids(&ids_page).await?;Also applies to: 175-179, 189-207
130-151: Creation is still racy; make it atomic with a Lua script.GET‑then‑SET can overwrite under concurrency. Use a single EVAL that checks existence and creates both the value and index.
Apply:
- let existing: Option<String> = conn - .get(&key) - .await - .map_err(|e| self.map_redis_error(e, "create_api_key_check"))?; - - if existing.is_some() { - return Err(RepositoryError::ConstraintViolation(format!( - "API Key with ID {} already exists", - entity.id - ))); - } - - // Use atomic pipeline for consistency - let mut pipe = redis::pipe(); - pipe.atomic(); - pipe.set(&key, json); - pipe.sadd(&list_key, &entity.id); - - pipe.exec_async(&mut conn) - .await - .map_err(|e| self.map_redis_error(e, "create_api_key"))?; + // Use a single atomic script: fail if key exists; otherwise SET, index with ZADD by created_at, and return 1. + let score_ms: i64 = chrono::DateTime::parse_from_rfc3339(&entity.created_at) + .map(|dt| dt.timestamp_millis()) + .unwrap_or_else(|_| chrono::Utc::now().timestamp_millis()); + let created: i32 = redis::Script::new(r#" + if redis.call('EXISTS', KEYS[1]) == 1 then + return 0 + end + redis.call('SET', KEYS[1], ARGV[1]) + redis.call('ZADD', KEYS[2], ARGV[2], ARGV[3]) + return 1 + "#) + .key(&key) + .key(&list_key) + .arg(&json) + .arg(score_ms) + .arg(&entity.id) + .invoke_async(&mut conn) + .await + .map_err(|e| self.map_redis_error(e, "create_api_key_tx"))?; + + if created == 0 { + return Err(RepositoryError::ConstraintViolation(format!( + "API Key with ID {} already exists", + entity.id + ))); + }Add at top:
+use chrono::{Utc};src/api/routes/relayer.rs (1)
240-251: Test AppState now seeds an API key.Seeding ApiKeyRepo in get_test_app_state aligns with new DefaultAppState shape. Good.
Also applies to: 256-367
src/services/plugins/mod.rs (1)
12-14: AKR propagation into plugin service/runner is coherent.Generic bounds and AppState threading are consistent. Tests cover runner generics. LGTM.
Also applies to: 90-110, 141-177, 182-189
src/services/plugins/runner.rs (2)
19-21: AKR threading through PluginRunner looks good.Bounds and state alias updates are consistent; tests exercise the new generic. No functional changes introduced here.
Also applies to: 35-43, 56-58, 65-73, 87-88
186-201: Tests updated for AKR are correct.Mock app state and run invocations include ApiKeyRepositoryStorage; compiles and maintains behavior.
Also applies to: 236-252
src/services/plugins/relayer_api.rs (1)
126-175: Business logic unchanged and correct under AKR threading.Handlers validate relayer state, deserialize payloads, and map domain errors to PluginError as before. No AKR usage yet, which aligns with staged rollout.
Also applies to: 185-241, 243-286, 288-326, 327-368, 369-401
src/bootstrap/config_processor.rs (4)
287-301: Confirm excluding API keys from the “is Redis populated” decision.You intentionally ignore api_key_repository when deciding to skip config processing. If an environment has only API keys, you’ll still process the rest, which may be desired. Please confirm.
371-372: Storage reset and seeding integrate AKR correctly.Dropping api_key_repository on reset and invoking process_api_key after other resources is sound.
Also applies to: 381-382
55-69: AKR generic propagation across processors and tests looks consistent.Type bounds and wiring compile cleanly and keep behavior unchanged.
Also applies to: 110-124, 149-163, 189-203, 231-245, 335-350, 398-409, 428-462
23-52: Creating a default API key with wildcard permissions/origins is unsafe-by-default.Seed least-privilege; don’t ship "*" (will grant full access once enforcement lands). Either gate permissive defaults behind an explicit dev flag or default to empty vectors.
Apply this minimal fix:
let api_key_model = ApiKeyRepoModel::new( "default".to_string(), server_config.api_key.clone(), - vec!["*".to_string()], - vec!["*".to_string()], + vec![], // no permissions by default + vec![], // no allowed origins by default );Optionally, add a config flag (e.g., allow_wildcard_api_key) and branch here for local/dev only.
| //! Redis-backed implementation of the ApiKeyRepository. | ||
|
|
||
| use crate::models::{ApiKeyRepoModel, PaginationQuery, RepositoryError}; | ||
| use crate::repositories::redis_base::RedisRepository; | ||
| use crate::repositories::{ApiKeyRepositoryTrait, BatchRetrievalResult, PaginatedResult}; | ||
| use async_trait::async_trait; | ||
| use redis::aio::ConnectionManager; | ||
| use redis::AsyncCommands; | ||
| use std::fmt; | ||
| use std::sync::Arc; | ||
| use tracing::{debug, error, warn}; | ||
|
|
||
| const API_KEY_PREFIX: &str = "apikey"; | ||
| const API_KEY_LIST_KEY: &str = "apikey_list"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Add chrono import required by atomic ZSET creation.
If you apply the ZSET changes, ensure chrono is imported:
use tracing::{debug, error, warn};
+use chrono::Utc;Run a quick search for lingering “Plugin” in this file:
🏁 Script executed:
#!/bin/bash
rg -n "Plugin|plugin_" src/repositories/api_key/api_key_redis.rs -nLength of output: 672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/repositories/api_key/api_key_redis.rs"
if [ ! -f "$file" ]; then
echo "FILE_NOT_FOUND: $file"
exit 0
fi
echo "== imports (1-80) =="
sed -n '1,80p' "$file"
echo
echo "== context (80-180) =="
sed -n '80,180p' "$file"
echo
echo "== SEARCH chrono / Utc =="
rg -n --line-number --context 2 "chrono::|\\bUtc\\b|Utc::now" "$file" || true
echo
echo "== SEARCH zadd / zset / zrange / zrem =="
rg -n --line-number --context 2 "zadd|ZADD|zset|ZSET|zrange|zrangeb|zrangebyscore|zrem|zremrange|zcard|zscore" "$file" || true
echo
echo "== SEARCH plugin identifiers =="
rg -n --line-number --context 2 "Plugin|plugin_|\\bplugin\\b" "$file" || true
echo
echo "== SEARCH api_key identifiers =="
rg -n --line-number --context 2 "api_key|apikey|API_KEY" "$file" || trueLength of output: 18452
Replace leftover "Plugin" identifiers; chrono import not required unless adding ZSET/timestamps
- Rename leftover "plugin" identifiers/log messages in src/repositories/api_key/api_key_redis.rs to "api_key" to avoid confusion. Occurrences: lines 85, 303, 308, 312, 318, 323–324, 328, 338–344, 350.
- Chrono: use chrono::Utc is already present in the test module (~line 360). No top-level chrono import is needed now — add use chrono::Utc only if you introduce ZSET/timestamp logic.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #456 +/- ##
=======================================
- Coverage 93.0% 93.0% -0.1%
=======================================
Files 224 230 +6
Lines 77456 78424 +968
=======================================
+ Hits 72105 72986 +881
- Misses 5351 5438 +87
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/api/routes/api_keys.rs (1)
58-115: Tighten assertions: check expected status codes, not just “not 404”.These are real handlers; assert 200/201 to catch regressions early.
Apply this diff:
@@ - // Test GET /api-keys - should not return 404 (route exists) + // Test GET /api-keys let req = test::TestRequest::get().uri("/api-keys").to_request(); let resp = test::call_service(&app, req).await; - assert_ne!( - resp.status(), - StatusCode::NOT_FOUND, - "GET /api-keys route not registered" - ); + assert_eq!(resp.status(), StatusCode::OK); @@ - // Test POST /api-keys - should not return 404 + // Test POST /api-keys let req = test::TestRequest::post() .uri("/api-keys") .set_json(serde_json::json!({ "name": "Test API Key", "permissions": ["relayer:all:execute"], "allowed_origins": ["*"] })) .to_request(); let resp = test::call_service(&app, req).await; - assert_ne!( - resp.status(), - StatusCode::NOT_FOUND, - "POST /api-keys route not registered" - ); + assert_eq!(resp.status(), StatusCode::CREATED); @@ - // Test GET /api-keys/{api_key_id}/permissions - should not return 404 + // Test GET /api-keys/{api_key_id}/permissions let req = test::TestRequest::get() .uri("/api-keys/test-id/permissions") .to_request(); let resp = test::call_service(&app, req).await; - assert_ne!( - resp.status(), - StatusCode::NOT_FOUND, - "GET /api-keys/{{api_key_id}}/permissions route not registered" - ); + assert_eq!(resp.status(), StatusCode::OK); @@ - // Test DELETE /api-keys/{api_key_id} - should not return 404 + // Test DELETE /api-keys/{api_key_id} let req = test::TestRequest::delete() .uri("/api-keys/test-id") .to_request(); let resp = test::call_service(&app, req).await; - assert_ne!( - resp.status(), - StatusCode::NOT_FOUND, - "DELETE /api-keys/{{api_key_id}} route not registered" - ); + assert_eq!(resp.status(), StatusCode::OK);src/api/controllers/api_key.rs (4)
84-87: Avoid needless allocation when re-mapping items.You convert to Vec and immediately map to ApiKeyResponse. Map directly from the iterator.
Apply this diff:
- let api_key_items: Vec<ApiKeyRepoModel> = api_keys.items.into_iter().collect(); - - // Subtract the "value" from the api key to avoid exposing it. - let api_key_items: Vec<ApiKeyResponse> = api_key_items - .into_iter() + // Strip the secret from each api key to avoid exposing it. + let api_key_items: Vec<ApiKeyResponse> = api_keys + .items + .into_iter() .map(ApiKeyResponse::try_from) .collect::<std::result::Result<Vec<ApiKeyResponse>, ApiError>>()?;
156-159: Consider 204 No Content for delete.Semantically, deletes often return 204 with an empty body. If you keep 200, returning a structured object (e.g., { id }) is preferable to a bare string.
Example:
- Ok(HttpResponse::Ok().json(ApiResponse::success(api_key_id))) + Ok(HttpResponse::NoContent().finish())
191-201: Strengthen tests to assert body semantics.Add assertions ensuring create returns the secret once (if intended) and list responses never include it.
Example additional tests (can be appended to this module):
#[actix_web::test] async fn test_secret_only_on_create_and_masked_in_list() { use actix_web::body::to_bytes; let app_state = create_mock_app_state(None, None, None, None, None, None).await; // Create let create_req = create_test_api_key_create_request("K"); let create_res = create_api_key(create_req, ThinData(app_state.clone())).await.unwrap(); assert_eq!(create_res.status(), 201); let body = to_bytes(create_res.into_body()).await.unwrap(); let v: serde_json::Value = serde_json::from_slice(&body).unwrap(); // Expect value present only if your API design allows it on create: // assert!(v["data"]["value"].is_string()); // List let list_res = list_api_keys(PaginationQuery { page: 1, per_page: 10 }, ThinData(app_state)).await.unwrap(); assert_eq!(list_res.status(), 200); let body = to_bytes(list_res.into_body()).await.unwrap(); let v: serde_json::Value = serde_json::from_slice(&body).unwrap(); // Ensure no secret is exposed in listing assert!(v["data"][0].get("value").is_none()); }
61-66: Guard page size to prevent abuse.If not enforced elsewhere, cap per_page server-side (e.g., 100) to avoid large reads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/api/controllers/api_key.rs(1 hunks)src/api/routes/api_keys.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/api/routes/api_keys.rs (3)
src/api/controllers/api_key.rs (4)
list_api_keys(69-102)get_api_key_permissions(111-132)create_api_key(35-55)delete_api_key(141-159)src/api/routes/relayer.rs (1)
init(194-216)src/utils/mocks.rs (1)
create_mock_app_state(177-272)
src/api/controllers/api_key.rs (5)
src/api/routes/api_keys.rs (4)
create_api_key(28-33)list_api_keys(11-16)get_api_key_permissions(19-24)delete_api_key(36-41)src/models/api_key/repository.rs (2)
try_from(44-55)new(24-38)src/models/api_key/response.rs (1)
try_from(16-24)src/models/api_response.rs (2)
success(33-40)paginated(60-67)src/utils/mocks.rs (1)
create_mock_app_state(177-272)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: Analyze (rust)
- GitHub Check: semgrep/ci
🔇 Additional comments (5)
src/api/routes/api_keys.rs (4)
9-16: Route handler shape is sound.Extractors and delegation to controller are idiomatic. Assuming ApiError implements ResponseError, returning Result<HttpResponse, ApiError> via impl Responder is fine.
43-50: Init wiring looks correct and ordering is sensible.Literal routes precede parameterized ones; matches the pattern in relayer routes.
1-2: Add OpenAPI docs stub when ready.Not blocking if deferred, but keep a docs module for these routes consistent with others.
118-138: Good coverage for query param parsing.Happy path presence check is fine here given controller-specific tests cover behavior.
src/api/controllers/api_key.rs (1)
54-55: Do not return the repo model from create; avoid leaking secret shape.Return a dedicated “created” DTO (with secret once) or the masked ApiKeyResponse. This aligns with list behavior and avoids coupling API to storage.
Example fix:
- Ok(HttpResponse::Created().json(ApiResponse::success(api_key))) + // Prefer a dedicated creation response (includes secret once): + let created = ApiKeyCreatedResponse::try_from(api_key)?; + Ok(HttpResponse::Created().json(ApiResponse::success(created))) + // If you don't want to return the secret even on create: + // Ok(HttpResponse::Created().json(ApiResponse::success(ApiKeyResponse::try_from(api_key)?)))
| use actix_web::HttpResponse; | ||
| use eyre::Result; | ||
|
|
There was a problem hiding this comment.
eyre::Result import breaks compilation; wrong generic arity in collect.
use eyre::Result; shadows std::result::Result. Then collect::<Result<_, ApiError>>()? uses the eyre alias, which only takes 1 type parameter. This won’t compile.
Apply this diff:
-use eyre::Result;
@@
- .collect::<Result<Vec<ApiKeyResponse>, ApiError>>()?;
+ .collect::<std::result::Result<Vec<ApiKeyResponse>, ApiError>>()?;Alternatively, just remove the eyre import; the prelude Result will work with two generics.
Also applies to: 89-93
🤖 Prompt for AI Agents
In src/api/controllers/api_key.rs around lines 19-21 (and also check lines
89-93), remove the `use eyre::Result;` import because it shadows the standard
Result and breaks usages like `collect::<Result<_, ApiError>>()`; instead rely
on the std Result (or explicitly use `std::result::Result`) so the two-generic
`Result<T, E>` resolves correctly; update any affected collect/type annotations
if needed to use `Result<_, ApiError>` from std or fully qualify as
`std::result::Result<_, ApiError>`.
Before merging just to check:
|
@zeljkoX It doesn't throw an error, but it creates a new API key under a different id |
3 participants
Summary
Created a new Repository to persist API keys
Added a couple of CRUD methods:
TODO:
Testing Process
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation