Remove User and UserId - #335
Conversation
WalkthroughReplaces handler-level Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as API Server
participant MW as JWT/API-Key Middleware
participant H as Handler
participant DB as Storage
C->>S: HTTP request (Authorization / API-Key)
S->>MW: validate token / key
MW->>DB: (optional) api_key lookup / ban check
DB-->>MW: user (id, class)
MW->>S: insert Authdata{sub, class} into request.extensions
S->>H: handler extracts Authdata (FromRequest)
H->>DB: call repo with user_id (Authdata.sub)
DB-->>H: data/result
H-->>C: HTTP response
sequenceDiagram
autonumber
participant Client
participant AuthHandler
participant JWTSvc
Client->>AuthHandler: login / refresh request
AuthHandler->>J WTSvc: build Claims { sub, class, exp }
JWTSvc-->>AuthHandler: signed access + refresh tokens
AuthHandler-->>Client: tokens (class embedded)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (4)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (26)
backend/api/src/handlers/auth/refresh_token.rs (1)
32-37: Avoid unwrap on JWT encode; unify error handling and compute now once.Unwrap can panic at runtime; prefer consistent error mapping and a single timestamp to avoid drift.
Apply:
- let token = encode( - &Header::default(), - &token_claims, - &EncodingKey::from_secret(arc.jwt_secret.as_bytes()), - ) - .unwrap(); + let token = encode( + &Header::default(), + &token_claims, + &EncodingKey::from_secret(arc.jwt_secret.as_bytes()), + ) + .map_err(Error::JwtError)?;Optionally also normalize time base:
- let token_claims = Claims { + let now = Utc::now(); + let token_claims = Claims { sub: old_refresh_token.claims.sub, - exp: (Utc::now() + Duration::days(1)).timestamp() as usize, + exp: (now + Duration::days(1)).timestamp() as usize, class: old_refresh_token.claims.class.clone(), }; @@ - let refresh_token_claims = Claims { + let refresh_token_claims = Claims { sub: old_refresh_token.claims.sub, - exp: (Utc::now() + Duration::days(90)).timestamp() as usize, + exp: (now + Duration::days(90)).timestamp() as usize, class: old_refresh_token.claims.class.clone(), };Also applies to: 45-51
backend/api/src/handlers/conversations/create_conversation.rs (1)
6-14: Fix OpenAPI response code and declare security.Handler returns 201 Created but docs say 200; also missing bearer security.
#[utoipa::path( post, operation_id = "Create conversation", tag = "Conversation", path = "/api/conversations", + security(("bearerAuth" = [])), responses( - (status = 200, description = "Successfully created the conversation and first message", body=Conversation), + (status = 201, description = "Successfully created the conversation and first message", body=Conversation), ) )]backend/api/src/handlers/conversations/create_conversation_message.rs (1)
6-14: Align OpenAPI with behavior and declare security.Docs say 200 but handler returns 201; add bearer security.
#[utoipa::path( post, operation_id = "Create conversation message", tag = "Conversation", path = "/api/conversations/messages", + security(("bearerAuth" = [])), responses( - (status = 200, description = "Successfully created the conversation's message", body=ConversationMessage), + (status = 201, description = "Successfully created the conversation's message", body=ConversationMessage), ) )]backend/api/src/handlers/auth/login.rs (1)
38-44: Don't unwrap JWT encode; propagate errors.
encode(...).unwrap()can panic. Use the same error mapping as the access token.- refresh_token = encode( + refresh_token = encode( &Header::default(), &refresh_token_claims, &EncodingKey::from_secret(arc.jwt_secret.as_bytes()), - ) - .unwrap(); + ) + .map_err(Error::JwtError)?;backend/api/src/handlers/series/create_series.rs (1)
12-12: OpenAPI doc/handler mismatch: handler returns 201 but docs say 200.Fix the doc to 201.
- (status = 200, description = "Successfully created the series", body=Series), + (status = 201, description = "Successfully created the series", body=Series),Also applies to: 22-22
backend/api/src/handlers/invitations/create_invitation.rs (2)
14-14: OpenAPI doc/handler mismatch: returning 201 but docs say 200.Align docs to 201.
- (status = 200, description = "Successfully sent the invitation", body=Invitation), + (status = 201, description = "Successfully sent the invitation", body=Invitation),Also applies to: 54-54
22-30: Enforce atomic check-and-decrement for invitationsIn
ConnectionPool::create_invitation(backend/storage/src/repositories/invitation_repository.rs), replace the unconditional decrement call (decrement_invitations_available) with a single SQL statement that both checksinvitations > 0and decrements (e.g.UPDATE users SET invitations = invitations - 1 WHERE id = $1 AND invitations > 0 RETURNING invitations), propagate an error if no row is returned (using?rather thanlet _ =), and remove or revisit the pre-check in the handler to avoid TOCTOU races.backend/storage/src/repositories/torrent_request_repository.rs (3)
11-21: API surface simplified to user_id: LGTM; make creation + initial vote transactionalThe signature and bindings look good. However, the create + initial vote path is non-transactional and risks partial writes.
- pub async fn create_torrent_request( + pub async fn create_torrent_request( &self, torrent_request: &mut UserCreatedTorrentRequest, - user_id: i64, + user_id: i64, ) -> Result<TorrentRequest> { - //TODO: make those requests transactional + // Make the creation + initial vote atomic let create_torrent_request_query = r#" INSERT INTO torrent_requests @@ - let created_torrent_request = - sqlx::query_as::<_, TorrentRequest>(create_torrent_request_query) + let mut tx = <ConnectionPool as Borrow<PgPool>>::borrow(self).begin().await?; + let created_torrent_request = + sqlx::query_as::<_, TorrentRequest>(create_torrent_request_query) .bind(torrent_request.title_group_id) .bind(user_id) @@ - .fetch_one(self.borrow()) + .fetch_one(&mut *tx) .await .map_err(Error::CouldNotCreateTorrentRequest)?; @@ - let _ = self - .create_torrent_request_vote(&torrent_request.initial_vote, user_id) - .await?; + let _ = self + .create_torrent_request_vote(&torrent_request.initial_vote, user_id) + .await?; + + tx.commit().await.map_err(Error::CouldNotCreateTorrentRequest)?;If create_torrent_request_vote requires a transaction handle, add an overload to accept &mut Transaction<'_, Postgres> and use it here.
Also applies to: 29-36, 50-55
123-164: Critical: bounty split over-credits on odd totals and risks overflow due to i32 castCurrent logic rounds half and applies the same share to both users, e.g., 3 → round(1.5)=2; both get 2 → 4 total credited > 3. Also casts to i32, risking truncation.
Apply deterministic integer split with remainder assigned to one party, and use i64 throughout:
- // Calculate the share for each user (50% each). - // Ensure floating-point division for accurate half-shares, then cast back to i64 for database. - let upload_share = (bounty_summary.total_upload as f32 / 2.0).round() as i32; - let bonus_share = (bounty_summary.total_bonus as f32 / 2.0).round() as i32; + // Split pot deterministically without over-crediting. + // Remainder goes to the filler to incentivize fulfillment. + let upload_half = bounty_summary.total_upload / 2; + let bonus_half = bounty_summary.total_bonus / 2; + let upload_rem = bounty_summary.total_upload % 2; + let bonus_rem = bounty_summary.total_bonus % 2; + let uploader_upload_share: i64 = upload_half; + let filler_upload_share: i64 = upload_half + upload_rem; + let uploader_bonus_share: i64 = bonus_half; + let filler_bonus_share: i64 = bonus_half + bonus_rem;And in the UPDATE use distinct shares per user (see next comment).
141-164: Use distinct shares per user and guard against races with conditional UPDATEUse separate shares (no double-credit), and ensure the request isn’t filled concurrently.
- sqlx::query!( + sqlx::query!( r#" UPDATE users SET - uploaded = users.uploaded + - CASE - WHEN users.id = $1 THEN $3 - WHEN users.id = $2 THEN $3 - ELSE 0 - END, - bonus_points = users.bonus_points + - CASE - WHEN users.id = $1 THEN $4 - WHEN users.id = $2 THEN $4 - ELSE 0 - END + uploaded = users.uploaded + + CASE + WHEN users.id = $1 THEN $3 -- uploader_upload_share + WHEN users.id = $2 THEN $4 -- filler_upload_share + ELSE 0 + END, + bonus_points = users.bonus_points + + CASE + WHEN users.id = $1 THEN $5 -- uploader_bonus_share + WHEN users.id = $2 THEN $6 -- filler_bonus_share + ELSE 0 + END WHERE users.id IN ($1, $2) "#, torrent_uploader_id, current_user_id, - upload_share, - bonus_share + uploader_upload_share, + filler_upload_share, + uploader_bonus_share, + filler_bonus_share ) .execute(&mut *tx) .await?; - sqlx::query!( + let updated = sqlx::query!( r#" UPDATE torrent_requests tr SET filled_by_torrent_id = $1, filled_by_user_id = $2, filled_at = NOW() WHERE - tr.id = $3 + tr.id = $3 AND tr.filled_at IS NULL "#, torrent_id, current_user_id, torrent_request_id ) - .execute(&mut *tx) - .await?; + .execute(&mut *tx) + .await?; + if updated.rows_affected() == 0 { + // Another filler won the race; rollback crediting to be safe. + return Err(Error::TorrentRequestAlreadyFilled); + }Also applies to: 168-181
backend/api/src/handlers/wiki/create_wiki_article.rs (1)
6-14: OpenAPI/status mismatch: return is 201 Created but docs declare 200 OK.
Align the spec with the actual response.Apply:
- (status = 200, description = "Successfully created the wiki article", body=WikiArticle), + (status = 201, description = "Successfully created the wiki article", body=WikiArticle),Also applies to: 26-26
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
6-14: OpenAPI/status mismatch: handler returns 201 but docs say 200.
Update the spec to 201 to match the Created response.- (status = 200, description = "Successfully created the artist affiliations", body=Vec<AffiliatedArtistHierarchy>), + (status = 201, description = "Successfully created the artist affiliations", body=Vec<AffiliatedArtistHierarchy>),Also applies to: 25-25
backend/api/src/handlers/edition_groups/create_edition_group.rs (1)
6-14: OpenAPI/status mismatch: returning 201 but docs state 200.
Update the spec to reflect Created.- (status = 200, description = "Successfully created the edition_group", body=EditionGroup), + (status = 201, description = "Successfully created the edition_group", body=EditionGroup),Also applies to: 22-22
backend/storage/src/repositories/auth_repository.rs (1)
112-128: Rename method and update its usage
Update the signature inauth_repository.rsand the call site injwt_middleware.rsto match the new name:--- backend/storage/src/repositories/auth_repository.rs @@ -112,7 +112,7 @@ - pub async fn find_user_id_with_api_key(&self, api_key: &str) -> Result<User> { + pub async fn find_user_with_api_key(&self, api_key: &str) -> Result<User> {--- backend/api/src/middlewares/jwt_middleware.rs @@ -123,7 +123,7 @@ - let user = match arc.pool.find_user_id_with_api_key(api_key).await { + let user = match arc.pool.find_user_with_api_key(api_key).await {backend/api/src/handlers/title_groups/create_title_group.rs (2)
25-35: Avoid tmdb_api_key unwrap; prevent panic when not configured.Unwrapping arc.tmdb_api_key will crash the request path if the key is missing. Either skip ratings gracefully or return a controlled error.
Apply:
- let rating_futures: Vec<_> = form - .external_links - .iter() - .filter(|link| link.contains("https://www.themoviedb.org/")) - .map(|link| get_tmdb_rating(link, arc.tmdb_api_key.clone().unwrap())) - .collect(); - let ratings: Vec<PublicRating> = join_all(rating_futures) - .await - .into_iter() - .filter_map(Result::ok) - .collect(); + let ratings: Vec<PublicRating> = if let Some(tmdb_key) = arc.tmdb_api_key.clone() { + let futures = form + .external_links + .iter() + .filter(|link| link.contains("https://www.themoviedb.org/")) + .map(|link| get_tmdb_rating(link, tmdb_key.clone())) + .collect::<Vec<_>>(); + join_all(futures) + .await + .into_iter() + .filter_map(Result::ok) + .collect() + } else { + Vec::new() + };
25-35: Panic risk inside get_tmdb_rating due to unwraps.get_tmdb_rating currently unwraps both URL extraction and TMDB client calls (see backend/api/src/handlers/external_db/get_tmdb_data.rs lines 1-24 in the provided snippet), which can bring down this handler despite filter_map(Result::ok). Replace unwraps with Result propagation.
Outside this file, change:
let (media_type, id) = extract_media_type_and_id(tmdb_url).unwrap();to:
let (media_type, id) = extract_media_type_and_id(tmdb_url) .map_err(|_| Error::InvalidTMDBUrl)?;Similarly, swap remaining .await.unwrap() calls on TMDB client for proper error mapping or ?.
backend/api/src/handlers/user_applications/update_user_application_status.rs (1)
25-33: Don’t hardcode role as a raw string.user.class == "staff" is fragile. Centralize an is_staff() helper or use a typed enum/const to avoid typos and future drift.
Example:
- if user.class != "staff" { + const STAFF: &str = "staff"; + if user.class.as_str() != STAFF { return Err(Error::InsufficientPrivileges); }backend/api/src/handlers/user_applications/get_user_applications.rs (1)
14-30: OpenAPI params don’t match the handler: dropchecked, typestatusas enum.
Doc currently advertisescheckedandstatus: String; code accepts onlystatus: UserApplicationStatus.#[utoipa::path( get, @@ params( - ("limit" = Option<i64>, Query, description = "Maximum number of applications to return (default: 50)"), - ("page" = Option<i64>, Query, description = "Page (default: 1)"), - ("status" = Option<String>, Query, description = "Filter by application status: 'pending', 'accepted', or 'rejected'"), - ("checked" = Option<bool>, Query, description = "Filter by checked status: true for checked (accepted/rejected), false for unchecked (pending)") + ("limit" = Option<i64>, Query, description = "Maximum number of applications to return (default: 50)"), + ("page" = Option<i64>, Query, description = "Page (default: 1)"), + ("status" = Option<UserApplicationStatus>, Query, description = "Filter by status: pending | accepted | rejected") ),backend/storage/src/repositories/torrent_request_vote_repository.rs (2)
14-20: TOCTOU: race between balance check and deduction can yield negatives.
Add guard conditions to the UPDATE so the single statement stays valid even under concurrency.- let current_user = self.find_user_with_id(user_id).await?; - if current_user.bonus_points - torrent_request_vote.bounty_bonus_points < 0 { + let current_user = self.find_user_with_id(user_id).await?; + if current_user.bonus_points - torrent_request_vote.bounty_bonus_points < 0 { return Err(Error::InsufficientBonusPointsForBounty); } if current_user.uploaded - torrent_request_vote.bounty_upload < 0 { return Err(Error::InsufficientUploadForBounty); } @@ - UPDATE users u - SET - uploaded = u.uploaded - $3, - bonus_points = u.bonus_points - $4 - WHERE u.id = (SELECT created_by_id FROM inserted_vote) + UPDATE users u + SET + uploaded = u.uploaded - $3, + bonus_points = u.bonus_points - $4 + WHERE u.id = (SELECT created_by_id FROM inserted_vote) + AND u.uploaded >= $3 + AND u.bonus_points >= $4 + RETURNING u.id ) SELECT - inserted_vote.* - FROM inserted_vote + inserted_vote.* + FROM inserted_vote + JOIN updated_user ON TRUE
15-20: Reject negative bounties early.
Ensurebounty_bonus_pointsandbounty_uploadare non-negative to avoid accidental crediting.- if current_user.bonus_points - torrent_request_vote.bounty_bonus_points < 0 { + if torrent_request_vote.bounty_bonus_points < 0 || torrent_request_vote.bounty_upload < 0 { + return Err(Error::BadRequest); + } + if current_user.bonus_points - torrent_request_vote.bounty_bonus_points < 0 { return Err(Error::InsufficientBonusPointsForBounty); }backend/api/src/handlers/forum/create_forum_thread.rs (1)
12-13: OpenAPI/docs say 200 but handler returns 201.
Align the documented response with the actual status.- (status = 200, description = "Successfully created the forum thread", body=ForumThread), + (status = 201, description = "Successfully created the forum thread", body=ForumThread),Also applies to: 25-25
backend/api/src/handlers/users/get_me.rs (2)
13-21: OpenAPI response type is incorrect vs actual payload shapeThe handler returns a composite object with peers, warnings, counts, and torrents, not a Profile. Update the schema to match the real response or refactor the handler to return a Profile.
Apply this minimal schema fix:
- responses( - (status = 200, description = "Successfully got the user's profile", body=Profile), - ) + responses( + (status = 200, description = "Successfully got the user's profile and related data", body=serde_json::Value), + )
65-73: Avoid unwrap() on JSON indexing; default safely to empty listsUnwrap can panic if "title_groups" is absent/null. Use a safe default to prevent 500s.
Apply:
- Ok(HttpResponse::Ok().json(json!({ - "user": current_user, - "peers":peers, - "user_warnings": user_warnings, - "unread_conversations_amount": unread_conversations_amount, - "unread_notifications_amount":unread_notifications_amount, - "last_five_uploaded_torrents": uploaded_torrents.get("title_groups").unwrap(), - "last_five_snatched_torrents": snatched_torrents.get("title_groups").unwrap() - }))) + let uploaded_title_groups = uploaded_torrents + .get("title_groups") + .cloned() + .unwrap_or_else(|| json!([])); + let snatched_title_groups = snatched_torrents + .get("title_groups") + .cloned() + .unwrap_or_else(|| json!([])); + + Ok(HttpResponse::Ok().json(json!({ + "user": current_user, + "peers": peers, + "user_warnings": user_warnings, + "unread_conversations_amount": unread_conversations_amount, + "unread_notifications_amount": unread_notifications_amount, + "last_five_uploaded_torrents": uploaded_title_groups, + "last_five_snatched_torrents": snatched_title_groups + })))backend/api/src/middlewares/jwt_middleware.rs (1)
48-51: Do not panic on malformed api_key headerto_str().expect(...) can panic and 500 the request. Return 401 instead.
Apply:
- } else if let Some(api_key) = req.headers().get("api_key") { - let api_key = api_key.to_str().expect("api_key malformed").to_owned(); + } else if let Some(api_key) = req.headers().get("api_key") { + let api_key = match api_key.to_str() { + Ok(v) => v.to_owned(), + Err(_) => return Err((ErrorUnauthorized("api_key malformed"), req)), + };backend/storage/src/repositories/torrent_repository.rs (1)
128-135: Panic risk: unwrap on enum parsing from user inputFeatures::from_str(...).ok().unwrap() will panic on invalid values. Either validate and error out, or safely skip invalid items.
Apply safe, non-panicking parsing (short-term fix). Prefer returning a 400 upstream after adding validation:
- .bind( - torrent_form - .features - .split(',') - .filter(|f| !f.is_empty()) - .map(|f| Features::from_str(f).ok().unwrap()) - .collect::<Vec<Features>>(), - ) + .bind({ + torrent_form + .features + .split(',') + .filter(|f| !f.is_empty()) + .filter_map(|f| Features::from_str(f.trim()).ok()) + .collect::<Vec<Features>>() + })Follow-up: add proper validation and return a domain error if any feature is invalid.
backend/storage/src/repositories/title_group_repository.rs (1)
238-242: Avoid unwrap() on possibly NULL JSON; return a typed errorUnwrap will panic if title_group_data is NULL (e.g., edge-case joins). Return a domain error instead.
Apply:
- Ok(title_group.title_group_data.unwrap()) + let data = title_group + .title_group_data + .ok_or(Error::TitleGroupNotFound)?; + Ok(data)
🧹 Nitpick comments (42)
backend/api/Cargo.toml (1)
13-13: Retain futures-util dependency
Usage found inbackend/api/src/middlewares/jwt_middleware.rs(line 9), so don’t remove it.
Optional: pinfutures = "0.3.31"inCargo.tomlto align its patch version withfutures-util.backend/api/src/handlers/users/get_registered_users.rs (1)
15-18: Centralize role checks: replace magic strings with a sharedUserClassenum
- Define a
UserClassenum (e.g. inbackend/api/src/models/user.rs) with variants for each role (Tracker,Staff, …) and implement anas_str()orDisplayfor string-based checks.- In get_registered_users.rs (line 17), change
- if user.class != "tracker" { + if user.class != UserClass::Tracker.as_str() { return Err(Error::InsufficientPrivileges); }- Replace all other inline
user.class == "staff"and"tracker"checks in backend handlers (e.g. edit_torrent.rs, get_registered_torrents.rs) and mirror equivalent constants or generated types in the front-end.backend/api/src/handlers/torrents/create_torrent.rs (1)
8-17: Document auth requirement in OpenAPI.Add bearer security to the path so clients know this endpoint is authenticated.
#[utoipa::path( post, operation_id = "Create torrent", tag = "Torrent", path = "/api/torrents", + security(("bearerAuth" = [])), request_body(content = UploadedTorrent, content_type = "multipart/form-data"), responses( (status = 201, description = "Successfully uploaded the torrent", body=Torrent), ) )]backend/api/src/handlers/search/search_torrents.rs (2)
31-34: Consider optional auth to keep search public while enabling personalization.If search must work for anonymous users, switch to Option and forward user.as_ref().map(|u| u.sub). If auth is mandatory, document it (see next comment).
-pub async fn exec( - form: web::Json<TorrentSearch>, - arc: web::Data<Arcadia>, - user: JwtAuthData, -) -> Result<HttpResponse> { - let search_results = arc.pool.search_torrents(&form, Some(user.sub)).await?; +pub async fn exec( + form: web::Json<TorrentSearch>, + arc: web::Data<Arcadia>, + user: Option<JwtAuthData>, +) -> Result<HttpResponse> { + let user_id = user.as_ref().map(|u| u.sub); + let search_results = arc.pool.search_torrents(&form, user_id).await?;
19-27: Clarify API design: GET with JSON body and auth docs.
- GET with web::Json is atypical; prefer POST with request_body or move filters to Query.
- If auth is required, add bearer security to the path.
#[utoipa::path( - get, + post, operation_id = "Search torrents", tag = "Search", path = "/api/search/torrents/lite", + security(("bearerAuth" = [])), + request_body=TorrentSearch, responses( (status = 200, description = "Title groups and their torrents found", body=TorrentSearchResults), ) )]Confirm intended access policy for this route (public vs authenticated).
backend/api/src/handlers/subscriptions/create_subscription.rs (1)
26-30: Correct identity propagation; consider idempotency and enum-typing.
- Using user.sub is correct.
- Ensure create_subscription is idempotent (no error on duplicates).
- Consider typing item as an enum to constrain allowed values and improve docs.
Example enum:
#[derive(Debug, Deserialize, utoipa::ToSchema)] #[serde(rename_all = "snake_case")] pub enum SubscriptionItem { Artist, Thread, Torrent /* ... */ } #[derive(Debug, Deserialize, IntoParams)] pub struct AddSubscriptionQuery { pub item_id: i64, pub item: SubscriptionItem, }backend/api/src/handlers/torrents/get_registered_torrents.rs (2)
16-20: Harden role checks; avoid string literals for authorization.
Using "tracker" as a raw string is brittle. Centralize roles (e.g., Role enum or JwtAuthData helpers like is_tracker()) to avoid typos and enable future changes without sweeping edits.
12-15: Document 403 in OpenAPI.
The handler returns InsufficientPrivileges but the spec only documents 200. Add 403 to keep the contract accurate.Apply this diff:
responses( - (status = 200, description = "All registered torrents", body=Vec<TorrentMinimal>), + (status = 200, description = "All registered torrents", body=Vec<TorrentMinimal>), + (status = 403, description = "Insufficient privileges"), )backend/api/src/handlers/title_groups/edit_title_group.rs (2)
23-29: Auth logic is correct; consider a staff fast-path and role helpers.
- Current check is fine. For staff, you can skip the pre-read to reduce one DB hit.
- Prefer centralized helpers (is_staff) or a Role enum over string comparisons.
Example minimal change:
pub async fn exec( form: web::Json<EditedTitleGroup>, arc: web::Data<Arcadia>, user: JwtAuthData, ) -> Result<HttpResponse> { - let title_group = arc.pool.find_title_group(form.id).await?; - - if title_group.created_by_id == user.sub || user.class == "staff" { + if user.class == "staff" { + let updated_title_group = arc.pool.update_title_group(&form, form.id).await?; + return Ok(HttpResponse::Ok().json(updated_title_group)); + } + let title_group = arc.pool.find_title_group(form.id).await?; + if title_group.created_by_id == user.sub { let updated_title_group = arc.pool.update_title_group(&form, title_group.id).await?; Ok(HttpResponse::Ok().json(updated_title_group)) } else { Err(Error::InsufficientPrivileges) } }
12-14: Add 403 to OpenAPI responses.
The handler can return InsufficientPrivileges; document it.responses( - (status = 200, description = "Successfully edited the title group", body=TitleGroup), + (status = 200, description = "Successfully edited the title group", body=TitleGroup), + (status = 403, description = "Insufficient privileges"), )backend/api/src/handlers/auth/login.rs (2)
33-37: Embedding role/class in refresh tokens risks privilege drift.
If a user's class changes after issuance, a 90-day refresh token carrying the old class can mint privileged access. Prefer one of:
- Store only sub/exp in refresh tokens and re-fetch class from DB on refresh; or
- Introduce a refresh-specific claims struct (no class) and/or a token_version/jti with server-side revocation.
Would you like me to draft a refresh flow change (separate RefreshClaims + refresh endpoint update)?
14-16: Augment OpenAPI with auth error responses.
Login may return 401/403 for invalid credentials or banned users; document them.responses( - (status = 200, description = "Successfully logged in", body=LoginResponse), + (status = 200, description = "Successfully logged in", body=LoginResponse), + (status = 401, description = "Invalid credentials"), + (status = 403, description = "Account banned"), )backend/storage/.sqlx/query-c5ef39624904dbc651eb31e96568a2d98359844aa26ccb5f9b99952d3ccc3d3c.json (1)
1-250: Descriptor looks consistent; confirm API key revocation/expiry filters.
Query fetches a full user row by API key and bans filter. If the schema supports key revocation or expiry, add those predicates to the SQL to prevent stale keys from authenticating.If only sub/class are needed by the caller, consider selecting the minimal columns to reduce I/O.
backend/storage/src/repositories/torrent_report_repository.rs (1)
21-24: SQL bind order/shape is correct; consider explicit columns in RETURNING.
UsingRETURNING *couples to table shape. Prefer returning explicit columns mapped by TorrentReport to harden against schema drift.backend/api/src/handlers/torrents/create_torrent_report.rs (2)
12-13: Use 201 Created for POST and align OpenAPI docs.Creation endpoint should return 201. Align handler and Utoipa.
- (status = 200, description = "Torrent successfully reported", body=TorrentReport), + (status = 201, description = "Torrent successfully reported", body=TorrentReport), @@ - Ok(HttpResponse::Ok().json(report)) + Ok(HttpResponse::Created().json(report))Also applies to: 22-22
18-20: Nit: preferauth(orjwt) overuserto avoid confusion with DB-loaded users.- user: JwtAuthData, + auth: JwtAuthData, @@ - let report = arc.pool.report_torrent(&form, user.sub).await?; + let report = arc.pool.report_torrent(&form, auth.sub).await?;backend/api/src/handlers/series/create_series.rs (1)
16-16: Nit: renameserietopayloadto avoid confusion with theseriesvalue.- serie: web::Json<UserCreatedSeries>, + payload: web::Json<UserCreatedSeries>, @@ - let series = arc.pool.create_series(&serie, user.sub).await?; + let series = arc.pool.create_series(&payload, user.sub).await?;backend/api/src/handlers/title_groups/get_title_group.rs (1)
18-21: Consider REST path param instead of query param forid.Optional: switch to
/api/title-groups/{id}withweb::Path<i64>for a more canonical GET by id.Also applies to: 24-27
backend/api/src/handlers/invitations/create_invitation.rs (2)
43-49: Avoid logging PII (email addresses).Reduce logging of recipient email; log the error only or a masked address.
- log::warn!( - "Failed to send invitation email to {}: {}", - invitation.receiver_email, - e - ); + log::warn!("Failed to send invitation email: {}", e);Also applies to: 51-51
22-25: Optional: narrow the user fetch to needed fields.Consider a repository method that returns only
{ id, username, invitations }to reduce payload/latency.backend/api/src/handlers/torrents/edit_torrent.rs (1)
23-28: Avoid magic string for role checks; centralize authorizationReplace
"staff"literal with a shared helper/const to prevent typos and ease future role changes.- if torrent.created_by_id == user.sub || user.class == "staff" { + if torrent.created_by_id == user.sub || is_staff(&user.class) {Outside this file:
pub fn is_staff(class: &str) -> bool { matches!(class, "staff" /* | "admin" etc. */) }backend/api/src/handlers/master_groups/create_master_group.rs (1)
12-13: OpenAPI response code mismatch (201 vs 200)Handler returns Created (201) but docs advertise 200. Update schema for accuracy.
- (status = 200, description = "Successfully created the master group", body=MasterGroup), + (status = 201, description = "Successfully created the master group", body=MasterGroup),backend/api/src/handlers/users/warn_user.rs (2)
1-1: JWT extractor migration + privilege gate: LGTM; avoid magic stringChange set looks correct. Consider centralizing the
"staff"check as noted elsewhere.Also applies to: 18-26
12-13: OpenAPI response code mismatch (201 vs 200)Handler returns Created (201) but docs advertise 200. Update for accuracy.
- (status = 200, description = "Successfully warned the user", body=UserWarning), + (status = 201, description = "Successfully warned the user", body=UserWarning),backend/api/src/handlers/forum/create_forum_post.rs (1)
12-13: Fix API doc status: endpoint returns 201 but docs say 200Update Utoipa response to match
HttpResponse::Created().- (status = 200, description = "Successfully created the forum post", body=ForumPost), + (status = 201, description = "Successfully created the forum post", body=ForumPost),backend/api/src/handlers/torrents/delete_torrent.rs (1)
22-25: Extractstaffrole check into a centralized helper
- Replace every direct comparison of
user.classto the"staff"literal (e.g. in delete_torrent.rs, edit_torrent.rs, get_user_applications.rs, update_user_application_status.rs, warn_user.rs, create_wiki_article.rs, edit_title_group.rs) with a single method (e.g.user.is_staff()) or a dedicatedRoleenum/constant to avoid string-drift and ensure consistent privilege checks.backend/api/src/handlers/torrent_requests/create_torrent_request_vote.rs (1)
14-15: Fix API doc status: handler returns 201 but docs say 200Align Utoipa response with
HttpResponse::Created().- (status = 200, description = "Successfully voted on the torrent_request", body=TorrentRequestVote), + (status = 201, description = "Successfully voted on the torrent_request", body=TorrentRequestVote),Also applies to: 27-27
backend/api/src/handlers/wiki/create_wiki_article.rs (1)
20-21: Avoid hard-coded role string for RBAC.
Comparing to "staff" as a raw string is brittle; prefer a Role enum or shared constant (e.g., STAFF) to prevent typos/drift across handlers.backend/api/src/handlers/torrent_requests/fill_torrent_request.rs (1)
29-29: Fix response typo: "succes" → "success".
Minor but user-visible.- Ok(HttpResponse::Ok().json(json!({"result": "succes"}))) + Ok(HttpResponse::Ok().json(json!({"result": "success"})))backend/api/src/handlers/subscriptions/remove_subscription.rs (1)
17-17: Grammar nit in OpenAPI description.
Use “unsubscribed from the item”.- (status = 200, description = "Successfully unsubscribed to the item"), + (status = 200, description = "Successfully unsubscribed from the item"),backend/api/src/handlers/torrent_requests/get_torrent_request.rs (1)
1-1: Auth extractor switch looks good; keep param name explicit.Change aligns with PR goal and avoids DB reads. Minor: name the unused param
_authfor clarity during future refactors.- _: JwtAuthData, + _auth: JwtAuthData,Also applies to: 26-27
backend/api/src/handlers/users/get_user.rs (1)
67-71: Avoid unwrap on response shaping.Unchecked unwrap may 500 if key is missing. Return null or an empty array instead.
- "last_five_uploaded_torrents": uploaded_torrents.get("title_groups").unwrap(), - "last_five_snatched_torrents": snatched_torrents.get("title_groups").unwrap() + "last_five_uploaded_torrents": uploaded_torrents + .get("title_groups") + .cloned() + .unwrap_or(serde_json::Value::Null), + "last_five_snatched_torrents": snatched_torrents + .get("title_groups") + .cloned() + .unwrap_or(serde_json::Value::Null)backend/api/src/handlers/title_groups/create_title_group.rs (1)
31-35: Log dropped rating errors for observability.filter_map(Result::ok) silently swallows failures. Add tracing at warn/debug to surface broken links or TMDB outages.
backend/storage/src/repositories/series_repository.rs (1)
14-17: Prefer explicit column list in RETURNING.RETURNING * is brittle against schema changes. Explicitly list columns to keep compile-time struct/DB alignment predictable.
backend/api/src/handlers/users/edit_user.rs (1)
19-20: Prefer a clearer name thanuserfor JWT claims (e.g.,auth).
Improves readability and avoids confusion with a DB-backedUser.-pub async fn exec( - form: web::Json<EditedUser>, - arc: web::Data<Arcadia>, - user: JwtAuthData, -) -> Result<HttpResponse> { - arc.pool.update_user(user.sub, &form).await?; +pub async fn exec( + form: web::Json<EditedUser>, + arc: web::Data<Arcadia>, + auth: JwtAuthData, +) -> Result<HttpResponse> { + arc.pool.update_user(auth.sub, &form).await?;backend/api/src/handlers/user_applications/get_user_applications.rs (2)
41-47: Validate page/limit inputs and cap to sane bounds.
Prevents excessive reads and undefined behavior for zero/negative values.- .find_user_applications( - query.limit.unwrap_or(50), - query.page.unwrap_or(1), + .find_user_applications( + query.limit.filter(|&n| n > 0).unwrap_or(50).min(200), + query.page.filter(|&n| n > 0).unwrap_or(1), query.status.clone(), )
37-39: Use a unified role-check helper or enum instead of hard-coded"staff"
Multiple handlers (e.g. inget_user_applications.rs,update_user_application_status.rs,create_wiki_article.rs, etc.) directly compareuser.classto"staff". Extract a helper (JwtAuthData::is_staff()) or introduce aUserClassenum (e.g.UserClass::Staff) and replace alluser.class ==/!= "staff"checks to prevent typos and future drift.backend/api/src/handlers/artists/create_artists.rs (2)
21-22: Minor: avoid shadowingartistsfor clarity.
Rename the local tocreated_artists.- let artists = arc.pool.create_artists(&artists, user.sub).await?; + let created_artists = arc.pool.create_artists(&artists, user.sub).await?; - Ok(HttpResponse::Created().json(artists)) + Ok(HttpResponse::Created().json(created_artists))
19-20: Consistent naming for claims object.
Considerauth: JwtAuthDataacross handlers to standardize signatures.- user: JwtAuthData, + auth: JwtAuthData, @@ - let created_artists = arc.pool.create_artists(&artists, user.sub).await?; + let created_artists = arc.pool.create_artists(&artists, auth.sub).await?;backend/api/src/middlewares/jwt_middleware.rs (2)
91-98: Surface operational errors for is_user_banned without leaking detailsConflating all errors to “account does not exist” hides operational issues. At least log them.
Apply:
- let Ok(banned) = arc.pool.is_user_banned(user_id).await else { - return Err(( - actix_web::error::ErrorUnauthorized("account does not exist"), - req, - )); - }; + let banned = match arc.pool.is_user_banned(user_id).await { + Ok(b) => b, + Err(e) => { + log::warn!("is_user_banned({}) failed: {}", user_id, e); + return Err((actix_web::error::ErrorUnauthorized("account does not exist"), req)); + } + };
74-89: Consider stricter JWT validation (iss/aud/nbf) as defense-in-depthValidation::default() only checks signature and exp by default. If your tokens carry iss/aud/nbf, set them in Validation to reduce token replay across environments.
Would you like me to propose a Validation configuration aligned with your token issuer/audience?
backend/storage/src/repositories/torrent_repository.rs (1)
326-349: Minor: reduce transaction hold time by moving user lookup before BEGINYou start a transaction, then fetch the user. Move find_user_with_id(user_id) before begin() to shorten lock duration.
Proposed outline:
- let user = self.find_user_with_id(user_id).await?;
- let mut tx = self.borrow::().begin().await?;
📜 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)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
backend/api/Cargo.toml(1 hunks)backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs(2 hunks)backend/api/src/handlers/artists/create_artists.rs(2 hunks)backend/api/src/handlers/auth/login.rs(2 hunks)backend/api/src/handlers/auth/refresh_token.rs(2 hunks)backend/api/src/handlers/conversations/create_conversation.rs(2 hunks)backend/api/src/handlers/conversations/create_conversation_message.rs(2 hunks)backend/api/src/handlers/conversations/get_conversation.rs(2 hunks)backend/api/src/handlers/edition_groups/create_edition_group.rs(2 hunks)backend/api/src/handlers/forum/create_forum_post.rs(2 hunks)backend/api/src/handlers/forum/create_forum_thread.rs(2 hunks)backend/api/src/handlers/gifts/create_gift.rs(2 hunks)backend/api/src/handlers/invitations/create_invitation.rs(2 hunks)backend/api/src/handlers/master_groups/create_master_group.rs(2 hunks)backend/api/src/handlers/mod.rs(0 hunks)backend/api/src/handlers/search/search_torrents.rs(2 hunks)backend/api/src/handlers/series/create_series.rs(2 hunks)backend/api/src/handlers/subscriptions/create_subscription.rs(2 hunks)backend/api/src/handlers/subscriptions/remove_subscription.rs(2 hunks)backend/api/src/handlers/title_groups/create_title_group.rs(4 hunks)backend/api/src/handlers/title_groups/create_title_group_comment.rs(2 hunks)backend/api/src/handlers/title_groups/edit_title_group.rs(2 hunks)backend/api/src/handlers/title_groups/get_title_group.rs(2 hunks)backend/api/src/handlers/torrent_requests/create_torrent_request.rs(2 hunks)backend/api/src/handlers/torrent_requests/create_torrent_request_vote.rs(2 hunks)backend/api/src/handlers/torrent_requests/fill_torrent_request.rs(2 hunks)backend/api/src/handlers/torrent_requests/get_torrent_request.rs(2 hunks)backend/api/src/handlers/torrents/create_torrent.rs(2 hunks)backend/api/src/handlers/torrents/create_torrent_report.rs(2 hunks)backend/api/src/handlers/torrents/delete_torrent.rs(3 hunks)backend/api/src/handlers/torrents/download_dottorrent_file.rs(2 hunks)backend/api/src/handlers/torrents/edit_torrent.rs(2 hunks)backend/api/src/handlers/torrents/get_registered_torrents.rs(2 hunks)backend/api/src/handlers/torrents/get_upload_information.rs(2 hunks)backend/api/src/handlers/user_applications/get_user_applications.rs(2 hunks)backend/api/src/handlers/user_applications/update_user_application_status.rs(2 hunks)backend/api/src/handlers/users/create_api_key.rs(2 hunks)backend/api/src/handlers/users/edit_user.rs(2 hunks)backend/api/src/handlers/users/get_me.rs(3 hunks)backend/api/src/handlers/users/get_registered_users.rs(2 hunks)backend/api/src/handlers/users/get_user.rs(3 hunks)backend/api/src/handlers/users/get_user_conversations.rs(2 hunks)backend/api/src/handlers/users/warn_user.rs(2 hunks)backend/api/src/handlers/wiki/create_wiki_article.rs(2 hunks)backend/api/src/middlewares/jwt_middleware.rs(3 hunks)backend/storage/.sqlx/query-c5ef39624904dbc651eb31e96568a2d98359844aa26ccb5f9b99952d3ccc3d3c.json(1 hunks)backend/storage/.sqlx/query-d97d331262b4156f941a88ef0e6237b02f581be5d5b13e75dd1e54727175c73e.json(0 hunks)backend/storage/src/models/user.rs(1 hunks)backend/storage/src/repositories/auth_repository.rs(2 hunks)backend/storage/src/repositories/series_repository.rs(2 hunks)backend/storage/src/repositories/title_group_comment_repository.rs(3 hunks)backend/storage/src/repositories/title_group_repository.rs(5 hunks)backend/storage/src/repositories/torrent_report_repository.rs(3 hunks)backend/storage/src/repositories/torrent_repository.rs(4 hunks)backend/storage/src/repositories/torrent_request_repository.rs(4 hunks)backend/storage/src/repositories/torrent_request_vote_repository.rs(2 hunks)backend/storage/src/repositories/user_repository.rs(1 hunks)
💤 Files with no reviewable changes (2)
- backend/api/src/handlers/mod.rs
- backend/storage/.sqlx/query-d97d331262b4156f941a88ef0e6237b02f581be5d5b13e75dd1e54727175c73e.json
🧰 Additional context used
🧬 Code graph analysis (10)
backend/api/src/handlers/users/get_user_conversations.rs (5)
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
exec(15-26)backend/api/src/handlers/conversations/create_conversation.rs (1)
exec(15-27)backend/api/src/handlers/conversations/create_conversation_message.rs (1)
exec(15-26)backend/api/src/handlers/conversations/get_conversation.rs (1)
exec(23-31)backend/api/src/handlers/search/search_torrents.rs (1)
exec(28-36)
backend/storage/src/repositories/auth_repository.rs (4)
frontend/src/services/api/userService.ts (1)
User(18-18)backend/storage/src/repositories/title_group_repository.rs (1)
sqlx(36-36)backend/storage/src/repositories/torrent_repository.rs (1)
sqlx(102-102)backend/storage/src/repositories/torrent_request_repository.rs (1)
sqlx(30-30)
backend/api/src/handlers/users/get_me.rs (2)
backend/api/src/handlers/conversations/create_conversation.rs (1)
exec(15-27)backend/api/src/handlers/gifts/create_gift.rs (1)
exec(15-31)
backend/api/src/handlers/torrents/get_registered_torrents.rs (3)
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
exec(15-26)backend/api/src/handlers/artists/create_artists.rs (1)
exec(16-24)backend/api/src/handlers/search/search_torrents.rs (1)
exec(28-36)
backend/storage/src/repositories/series_repository.rs (1)
backend/storage/src/connection_pool.rs (1)
borrow(23-25)
backend/api/src/handlers/users/get_registered_users.rs (7)
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
exec(15-26)backend/api/src/handlers/artists/create_artists.rs (1)
exec(16-24)backend/api/src/handlers/auth/login.rs (1)
exec(18-63)backend/api/src/handlers/auth/refresh_token.rs (1)
exec(18-56)backend/api/src/handlers/conversations/create_conversation.rs (1)
exec(15-27)backend/api/src/handlers/gifts/create_gift.rs (1)
exec(15-31)backend/api/src/handlers/invitations/create_invitation.rs (1)
exec(17-55)
backend/storage/src/repositories/torrent_report_repository.rs (1)
frontend/src/services/api/torrentService.ts (2)
TorrentReport(121-121)UserCreatedTorrentReport(119-119)
backend/api/src/handlers/torrents/get_upload_information.rs (4)
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
exec(15-26)backend/api/src/handlers/gifts/create_gift.rs (1)
exec(15-31)backend/api/src/handlers/invitations/create_invitation.rs (1)
exec(17-55)backend/api/src/handlers/search/search_torrents.rs (1)
exec(28-36)
backend/api/src/middlewares/jwt_middleware.rs (1)
backend/api/src/handlers/auth/refresh_token.rs (1)
decode(19-19)
backend/api/src/handlers/title_groups/create_title_group.rs (1)
backend/api/src/handlers/external_db/get_tmdb_data.rs (1)
get_tmdb_rating(106-138)
| user: JwtAuthData, | ||
| ) -> Result<HttpResponse> { | ||
| let affiliations = arc | ||
| .pool | ||
| .create_artists_affiliation(&artists, current_user_id.0) | ||
| .create_artists_affiliation(&artists, user.sub) | ||
| .await?; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify required authorization for creating affiliations.
If this operation was previously staff-only (common in similar create endpoints), we should keep parity by checking user.class or centralizing an is_staff guard.
Run to confirm prior/other handlers’ RBAC patterns:
🏁 Script executed:
#!/bin/bash
# Inspect affiliated-artist creation and nearby RBAC checks
rg -nP --type=rust -C3 'create_artists_affiliation|affiliated-artists|Affiliated Artist' backend/api/src
# Look for common privilege checks used elsewhere
rg -nP --type=rust -C2 'InsufficientPrivileges|user\.class\s*!=\s*"staff"|is_staff' backend/api/srcLength of output: 8589
Add staff-only authorization to create_affiliated_artists handler
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (lines 19–23) lacks the user.class != "staff" check—add the same guard used in other create endpoints to prevent unauthorized affiliation creation.
🤖 Prompt for AI Agents
In backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs
around lines 18 to 23, the handler does not enforce staff-only access; add the
same guard used in other create endpoints by checking if user.class != "staff"
immediately after extracting JwtAuthData and before calling
create_artists_affiliation, and return an appropriate HTTP 403/Unauthorized
response (with the same error type/format used elsewhere) when the check fails
so only staff can create affiliations.
| user: JwtAuthData, | ||
| ) -> Result<HttpResponse> { | ||
| let message = arc | ||
| .pool | ||
| .create_conversation_message(&message, current_user.id) | ||
| .create_conversation_message(&message, user.sub) | ||
| .await?; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Identity propagation via user.sub is correct; ensure membership checks.
Before insert, validate that user.sub is a participant of the conversation referenced in message.
🏁 Script executed:
#!/bin/bash
# Storage must check conversation membership for the author
rg -nP -C3 '(pub\s+async\s+fn|fn)\s+create_conversation_message\s*\('
rg -nP -C3 'CHECK|ensure|validate|membership|participant'Length of output: 43345
Enforce conversation-participation check in storage layer
In create_conversation_message (backend/storage/src/repositories/conversation_repository.rs:40), verify current_user_id is a participant of message.conversation_id before inserting the message and return a forbidden error if the user isn’t in that conversation.
🤖 Prompt for AI Agents
In backend/api/src/handlers/conversations/create_conversation_message.rs around
lines 18 to 23, update the storage layer call to enforce a participation check:
in backend/storage/src/repositories/conversation_repository.rs inside
create_conversation_message (around line 40) confirm that current_user_id is a
participant of message.conversation_id before performing the insert; if the user
is not a participant, return a forbidden error (map to an appropriate storage
error that the handler translates into an HTTP 403) otherwise proceed to insert
the message—ensure the check queries the conversation participants table (or
joins users_in_conversation) in a single transaction or prior lookup to avoid
TOCTOU and return a clear forbidden error when the check fails.
| user: JwtAuthData, | ||
| ) -> Result<HttpResponse> { | ||
| let current_user = arc.pool.find_user_with_id(user.sub).await?; | ||
| if current_user.bonus_points < gift.bonus_points { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid TOCTOU on balances; enforce checks atomically in storage.
Between reading balances and creating the gift, another transfer could occur. Ensure create_gift performs validation and deduction in a single DB transaction.
I can draft a transactional storage impl that validates and decrements in one step and returns domain errors.
Also applies to: 28-31
🏁 Script executed:
#!/bin/bash
# Inspect storage impl for atomicity (transaction, SELECT ... FOR UPDATE / balance checks).
rg -nP 'fn\s+create_gift|pub\s+async\s+fn\s+create_gift' -C6 backend/storageLength of output: 1226
🏁 Script executed:
#!/bin/bash
# Display implementation of decrement_bonus_points_and_freeleech_tokens to verify atomicity
rg -nP 'fn\s+decrement_bonus_points_and_freeleech_tokens' -C20 backend/storage/src/repositories/gift_repository.rsLength of output: 1326
Enforce atomic balance validation and deduction within storage.create_gift
Move the bonus_points/freeleech_tokens check into the same DB transaction by updating backend/storage/src/repositories/gift_repository.rs to:
UPDATE users
SET bonus_points = bonus_points - $1,
freeleech_tokens = freeleech_tokens - $2
WHERE id = $3
AND bonus_points >= $1
AND freeleech_tokens >= $2then verify rows_affected == 1 and return an “insufficient balance” error before inserting the gift.
🤖 Prompt for AI Agents
In backend/api/src/handlers/gifts/create_gift.rs around lines 18–21: the current
pre-check reads user's bonus_points before calling storage.create_gift, which
permits race conditions; instead implement the balance check and deduction
atomically inside backend/storage/src/repositories/gift_repository.rs by
replacing the separate read-with-check with a single UPDATE that decrements
bonus_points and freeleech_tokens only when both balances are sufficient (using
WHERE bonus_points >= $1 AND freeleech_tokens >= $2 AND id = $3), then verify
rows_affected == 1 and return an “insufficient balance” error if it is 0, before
proceeding to insert the gift in the same transaction.
| user: JwtAuthData, | ||
| ) -> Result<HttpResponse> { | ||
| // TODO : check if user can upload | ||
|
|
||
| let torrent = arc.pool.create_torrent(&form, ¤t_user).await?; | ||
| let torrent = arc.pool.create_torrent(&form, user.sub).await?; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use claims to gate upload permissions and verify storage signature.
- Add a role/claims check before persisting (e.g., based on user.class), otherwise any authenticated account can upload.
- Verify arcadia_storage::Pool::create_torrent now accepts an i64 user id; if it still expects a User/UserId, this will break.
Run to verify storage signatures and call sites:
Example inline guard (adjust roles/error type as needed):
pub async fn exec(
form: MultipartForm<UploadedTorrent>,
arc: web::Data<Arcadia>,
user: JwtAuthData,
) -> Result<HttpResponse> {
- // TODO : check if user can upload
+ // Enforce upload permissions from claims
+ match user.class.as_str() {
+ "Uploader" | "Staff" | "Moderator" => {}
+ _ => return Ok(HttpResponse::Forbidden().finish()),
+ }
let torrent = arc.pool.create_torrent(&form, user.sub).await?;🏁 Script executed:
#!/bin/bash
# Confirm storage signature uses a scalar user id
rg -nP -C2 '(pub\s+async\s+fn|fn)\s+create_torrent\s*\('
# Ensure no remaining create_torrent calls pass User/UserId
rg -nP -C2 '\.create_torrent\s*\('Length of output: 925
🏁 Script executed:
#!/bin/bash
sed -n '30,45p' backend/storage/src/repositories/torrent_repository.rsLength of output: 542
Enforce upload permissions based on user claims
The storage API’s create_torrent already accepts an i64 user_id, so passing user.sub matches the signature and introduces no breaking change. You still need to gate uploads by role—insert an authorization guard using user.class (or your chosen claim) before persisting:
pub async fn exec(
form: MultipartForm<UploadedTorrent>,
arc: web::Data<Arcadia>,
user: JwtAuthData,
) -> Result<HttpResponse> {
- // TODO: check if user can upload
+ // Enforce upload permissions from claims
+ match user.class.as_str() {
+ "Uploader" | "Staff" | "Moderator" => {}
+ _ => return Ok(HttpResponse::Forbidden().finish()),
+ }
let torrent = arc.pool.create_torrent(&form, user.sub).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.
| user: JwtAuthData, | |
| ) -> Result<HttpResponse> { | |
| // TODO : check if user can upload | |
| let torrent = arc.pool.create_torrent(&form, ¤t_user).await?; | |
| let torrent = arc.pool.create_torrent(&form, user.sub).await?; | |
| pub async fn exec( | |
| form: MultipartForm<UploadedTorrent>, | |
| arc: web::Data<Arcadia>, | |
| user: JwtAuthData, | |
| ) -> Result<HttpResponse> { | |
| // Enforce upload permissions from claims | |
| match user.class.as_str() { | |
| "Uploader" | "Staff" | "Moderator" => {} | |
| _ => return Ok(HttpResponse::Forbidden().finish()), | |
| } | |
| let torrent = arc.pool.create_torrent(&form, user.sub).await?; | |
| // …rest of handler… | |
| } |
🤖 Prompt for AI Agents
In backend/api/src/handlers/torrents/create_torrent.rs around lines 21 to 26,
add an authorization guard before calling arc.pool.create_torrent: check the
incoming JwtAuthData.user.class (or chosen claim) to ensure the user has the
required role/permission to upload, and if not return an appropriate
HttpResponse (Forbidden or Unauthorized); only proceed to call
arc.pool.create_torrent(&form, user.sub).await? when the role check passes.
Ensure the check is clear, returns early on failure, and does not change the
create_torrent signature.
| let user_url = &arc | ||
| .frontend_url | ||
| .join(&format!("/user/{}", current_user.id)) | ||
| .join(&format!("/user/{}", user.sub)) | ||
| .unwrap(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid potential panic on URL join; replace unwrap with safe construction or bubble error
unwrap() will crash the handler if base URL is malformed. Prefer safe string construction for this static path or map the error into your domain error.
Option A (safe string build):
- let user_url = &arc
- .frontend_url
- .join(&format!("/user/{}", user.sub))
- .unwrap();
+ let user_url = format!(
+ "{}/user/{}",
+ arc.frontend_url.as_str().trim_end_matches('/'),
+ user.sub
+ );Then below, keep &user_url.as_str() as-is.
Option B (bubble URL error; choose the right Error variant):
- let user_url = &arc
- .frontend_url
- .join(&format!("/user/{}", user.sub))
- .unwrap();
+ let user_url = arc
+ .frontend_url
+ .join(&format!("/user/{}", user.sub))
+ .map_err(|_| Error::InvalidUrl)?; // verify the exact variant/name
+ let user_url = &user_url;Quick check for an appropriate error variant name:
🏁 Script executed:
#!/bin/bash
# Look for a usable variant (e.g., InvalidUrl, UrlError, InternalError) in Error:
rg -nP --type=rust -C2 'enum\s+Error\b' $(fd Error -t f | tr '\n' ' ')
rg -nP --type=rust -C1 'Invalid.*Url|Url.*Invalid|Internal.*Error' arcadia_commonLength of output: 1456
🏁 Script executed:
#!/bin/bash
file=$(fd delete_torrent.rs -t f | head -n1)
echo "File: $file"
echo "---- Imports ----"
grep -n '^use ' "$file" || true
echo "---- Error imports ----"
grep -n 'use .*Error' "$file" || true
echo "---- Function signature ----"
grep -n 'fn delete_torrent' -C2 "$file" || true
echo "---- Context around join ----"
grep -nC3 'join' "$file" || trueLength of output: 1342
🏁 Script executed:
#!/bin/bash
ls -1 backend/common/src/error
echo "==== mod.rs ===="
sed -n '1,200p' backend/common/src/error/mod.rsLength of output: 6123
Avoid panic: remove unwrap() on URL join
Replace the unwrap() at backend/api/src/handlers/torrents/delete_torrent.rs:26–29 with manual string construction:
- let user_url = &arc
- .frontend_url
- .join(&format!("/user/{}", user.sub))
- .unwrap();
+ let user_url = format!(
+ "{}/user/{}",
+ arc.frontend_url.as_str().trim_end_matches('/'),
+ user.sub
+ );If you’d rather propagate errors, add an Error::InvalidFrontendUrl variant to arcadia_common::error::Error and map the join failure to it instead.
📝 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 user_url = &arc | |
| .frontend_url | |
| .join(&format!("/user/{}", current_user.id)) | |
| .join(&format!("/user/{}", user.sub)) | |
| .unwrap(); | |
| let user_url = format!( | |
| "{}/user/{}", | |
| arc.frontend_url.as_str().trim_end_matches('/'), | |
| user.sub | |
| ); |
🤖 Prompt for AI Agents
In backend/api/src/handlers/torrents/delete_torrent.rs around lines 26 to 29,
remove the call to .join(...).unwrap() which can panic; instead either (A) build
the URL as a string safely using formatting (e.g. let user_url =
format!("{}/user/{}", arc.frontend_url.as_str().trim_end_matches('/'),
user.sub); and use that String/Url::parse with proper error handling) or (B)
propagate the join failure by adding an Error::InvalidFrontendUrl variant to
arcadia_common::error::Error and map the Url::join error to that variant
(returning Err(Error::InvalidFrontendUrl(...)) from this handler); ensure no
unwrap remains and handle/return the error appropriately.
| pub async fn exec(arc: web::Data<Arcadia>, user: JwtAuthData) -> Result<HttpResponse> { | ||
| let mut current_user = arc.pool.find_user_with_id(user.sub).await?; | ||
| current_user.password_hash = String::from(""); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid ad-hoc scrubbing; return a sanitized projection
Zeroing password_hash is brittle and risks leaking other sensitive fields (e.g., passkeys) since current_user is serialized wholesale. Prefer fetching/returning a dedicated projection (e.g., Profile) that excludes secrets.
Proposed direction:
-pub async fn exec(arc: web::Data<Arcadia>, user: JwtAuthData) -> Result<HttpResponse> {
- let mut current_user = arc.pool.find_user_with_id(user.sub).await?;
- current_user.password_hash = String::from("");
+pub async fn exec(arc: web::Data<Arcadia>, user: JwtAuthData) -> Result<HttpResponse> {
+ // Repository should expose a method that returns a sanitized view:
+ let current_user: Profile = arc.pool.find_user_profile(user.sub).await?;I can add find_user_profile and wire the Profile mapping if you want.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/api/src/handlers/users/get_me.rs around lines 22 to 24, avoid ad-hoc
scrubbing by zeroing password_hash on the full user model; instead fetch and
return a dedicated sanitized projection (e.g., Profile) that omits secrets.
Implement or call a new repository method like find_user_profile(user_id) that
selects only non-sensitive fields (email, display_name, id, avatar, etc.), map
the DB row into the Profile DTO, and return that Profile from this handler
rather than serializing the full User struct with mutated fields. Ensure all
sensitive fields (password_hash, passkeys, tokens) are excluded from the
projection and update any types/handler signatures accordingly.
| _: JwtAuthData, | ||
| ) -> Result<HttpResponse> { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bug: passing target user's id as viewer id to search_torrents.
search_torrents likely expects the authenticated viewer id (for visibility/flags). Use JWT subject, not the profile’s id.
- _: JwtAuthData,
+ auth: JwtAuthData,
@@
- .search_torrents(&torrent_search, Some(user.id))
+ .search_torrents(&torrent_search, Some(auth.sub))
@@
- .search_torrents(&torrent_search, Some(user.id))
+ .search_torrents(&torrent_search, Some(auth.sub))Also applies to: 55-59, 63-66
🏁 Script executed:
#!/bin/bash
# Confirm search_torrents second arg semantics (viewer/current_user id).
rg -nP 'fn\s+search_torrents\s*\(' -C3 backend/storageLength of output: 617
🏁 Script executed:
#!/bin/bash
# Display documentation and implementation context for search_torrents
sed -n '350,420p' backend/storage/src/repositories/torrent_repository.rsLength of output: 2607
🏁 Script executed:
#!/bin/bash
# Locate calls to search_torrents in get_user handler to verify arguments
rg -nP 'search_torrents' -C3 backend/api/src/handlers/users/get_user.rsLength of output: 581
Use JWT subject (auth.sub) instead of user.id in search_torrents calls
Replace the unused _ : JwtAuthData binding with auth: JwtAuthData, and pass auth.sub rather than user.id to both .search_torrents invocations:
- _: JwtAuthData,
+ auth: JwtAuthData,
@@
- .search_torrents(&torrent_search, Some(user.id))
+ .search_torrents(&torrent_search, Some(auth.sub))
@@
- .search_torrents(&torrent_search, Some(user.id))
+ .search_torrents(&torrent_search, Some(auth.sub))🤖 Prompt for AI Agents
In backend/api/src/handlers/users/get_user.rs around lines 33 to 34, the
function currently binds the JWT auth as an unused `_ : JwtAuthData`; replace
that with `auth: JwtAuthData` and update both `.search_torrents` calls to pass
`auth.sub` instead of `user.id` so the searches use the JWT subject; ensure the
new `auth` variable is used and compiles.
| #[derive(Debug, Serialize, Deserialize, Clone)] | ||
| pub struct Claims { | ||
| pub sub: i64, | ||
| pub exp: usize, | ||
| pub class: String, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use i64 for exp to match JWT NumericDate and avoid usize portability traps.
exp as usize can differ across platforms; i64 (or u64) is the common, portable choice and aligns with chrono timestamps.
Apply:
#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct Claims {
pub sub: i64,
- pub exp: usize,
+ pub exp: i64,
pub class: String,
}Follow-ups outside this file (remove usize casts):
// login.rs, refresh_token.rs
exp: token_expiration_date.timestamp(),
// and
exp: (now + Duration::days(90)).timestamp(),To find all casts to usize for exp:
🏁 Script executed:
#!/bin/bash
rg -nP '\.timestamp\(\)\s+as\s+usize|pub\s+exp:\s*usize'Length of output: 564
Use i64 for exp in JWT claims and remove usize casts
- In backend/storage/src/models/user.rs, change
pub exp: usize,→pub exp: i64, - In backend/api/src/handlers/auth/refresh_token.rs (lines 28, 41) and
backend/api/src/handlers/auth/login.rs (lines 35, 48), removeas usizeso thattimestamp()(an i64) matches the new field type.
🤖 Prompt for AI Agents
In backend/storage/src/models/user.rs around lines 107 to 112, change the Claims
struct field type from `pub exp: usize,` to `pub exp: i64,`; then in
backend/api/src/handlers/auth/refresh_token.rs (around lines 28 and 41) and
backend/api/src/handlers/auth/login.rs (around lines 35 and 48) remove the `as
usize` casts when assigning `timestamp()` values so the i64 timestamp is
assigned directly to `exp`, ensuring types match and no unnecessary casting
remains.
|
Could you rename it to something like |
|
Also, storing other data than the user id in the token is usually a bad practice, especially the user class which might be updated in the db and not in the token, giving access to some routes while it shouldn't, and vice-versa. Anyways, moving away from jwt is planned and might need to be done before this ? |
|
I'm not very convinced that using authz is a bad practise. The fact that the role for example can change is a backend implementation detail. In any case we can implement a token invalidation logic which will render such tokens invalid shall the role change or we ban user. In reality a full fledged Auth server should deal with all these authn/authz tasks, but for our use case, I believe we can keep it simple for the time being |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
backend/api/src/handlers/conversations/create_conversation_message.rs (1)
12-13: Fix OpenAPI response code to match implementation (201 vs 200).You return Created (201) but the spec advertises 200. Align the doc to 201.
Apply:
- (status = 200, description = "Successfully created the conversation's message", body=ConversationMessage), + (status = 201, description = "Successfully created the conversation's message", body=ConversationMessage),Also applies to: 25-25
backend/api/src/handlers/master_groups/create_master_group.rs (1)
12-13: Fix OpenAPI response code to match implementation (201 vs 200).Handler returns Created; spec says 200.
Apply:
- (status = 200, description = "Successfully created the master group", body=MasterGroup), + (status = 201, description = "Successfully created the master group", body=MasterGroup),Also applies to: 22-22
backend/api/src/handlers/torrents/download_dottorrent_file.rs (1)
57-60: Serve the correct MIME type for .torrent files.Use application/x-bittorrent instead of octet-stream.
Apply:
- Ok(HttpResponse::Ok() - .insert_header(ContentType::octet_stream()) + Ok(HttpResponse::Ok() + .insert_header((actix_web::http::header::CONTENT_TYPE, "application/x-bittorrent")) .insert_header(cd) .body(torrent.file_contents))backend/api/src/handlers/conversations/create_conversation.rs (1)
12-13: Fix OpenAPI/status mismatch: code returns 201 but docs say 200Update the documented response to 201 to match HttpResponse::Created.
- (status = 200, description = "Successfully created the conversation and first message", body=Conversation), + (status = 201, description = "Successfully created the conversation and first message", body=Conversation),Also applies to: 26-27
backend/api/src/handlers/wiki/create_wiki_article.rs (1)
12-13: Fix OpenAPI/status mismatch: code returns 201 but docs say 200Align docs with the Created response.
- (status = 200, description = "Successfully created the wiki article", body=WikiArticle), + (status = 201, description = "Successfully created the wiki article", body=WikiArticle),Also applies to: 26-27
backend/api/src/handlers/forum/create_forum_post.rs (1)
12-13: Fix OpenAPI/status mismatch: code returns 201 but docs say 200Align response docs with HttpResponse::Created.
- (status = 200, description = "Successfully created the forum post", body=ForumPost), + (status = 201, description = "Successfully created the forum post", body=ForumPost),Also applies to: 22-23
backend/api/src/handlers/title_groups/create_title_group.rs (2)
25-35: Avoid panic: don’t unwrap TMDB API key; gate rating fetchesarc.tmdb_api_key.clone().unwrap() will panic when unset. Also avoid cloning the key per link.
- let rating_futures: Vec<_> = form - .external_links - .iter() - .filter(|link| link.contains("https://www.themoviedb.org/")) - .map(|link| get_tmdb_rating(link, arc.tmdb_api_key.clone().unwrap())) - .collect(); - let ratings: Vec<PublicRating> = join_all(rating_futures) - .await - .into_iter() - .filter_map(Result::ok) - .collect(); + let ratings: Vec<PublicRating> = if let Some(tmdb_key) = arc.tmdb_api_key.clone() { + let futs: Vec<_> = form + .external_links + .iter() + .filter(|link| link.contains("https://www.themoviedb.org/")) + .map(|link| get_tmdb_rating(link, tmdb_key.clone())) + .collect(); + join_all(futs).await.into_iter().filter_map(Result::ok).collect() + } else { + log::warn!("TMDB API key not configured; skipping rating enrichment"); + Vec::new() + };
29-31: Replace all unwraps inget_tmdb_ratingwith error propagationIn
backend/api/src/handlers/external_db/get_tmdb_data.rs, remove every.unwrap()inget_tmdb_rating(regex parsing, ID extraction, API‐key retrieval and.await.unwrap()on HTTP calls) and propagate failures via?so the server won’t panic on malformed URLs or network errors.backend/api/src/middlewares/jwt_middleware.rs (2)
46-51: Don’t panic on malformed API key; accept X-API-Key tooexpect() will crash the worker on invalid header encoding. Return 401 instead and accept the conventional X-API-Key header.
-} else if let Some(api_key) = req.headers().get("api_key") { - let api_key = api_key.to_str().expect("api_key malformed").to_owned(); - validate_api_key(req, &api_key).await +} else if let Some(hv) = req + .headers() + .get("api_key") + .or_else(|| req.headers().get("x-api-key")) +{ + match hv.to_str() { + Ok(api_key) => validate_api_key(req, api_key).await, + Err(_) => Err(( + actix_web::error::ErrorUnauthorized( + "authentication error, malformed API key header", + ), + req, + )), + }
74-89: JWT validation should restrict algorithms and optionally iss/audValidation::default() accepts whatever alg the token declares. Lock this down (e.g., HS256) and, if configured, validate issuer/audience.
-use jsonwebtoken::{decode, errors::ErrorKind, DecodingKey, Validation}; +use jsonwebtoken::{decode, errors::ErrorKind, Algorithm, DecodingKey, Validation}; - let validation = Validation::default(); + // Restrict algorithms and explicitly validate exp; optionally set iss/aud + let mut validation = Validation::new(Algorithm::HS256); + validation.validate_exp = true; + // if you have these in Arcadia config, wire them: + // if let Some(ref iss) = arc.jwt_issuer { validation.set_issuer(&[iss]); } + // if let Some(ref aud) = arc.jwt_audience { validation.set_audience(&[aud]); }backend/api/src/handlers/users/get_me.rs (3)
13-21: OpenAPI schema mismatch: response is not ProfileThe handler returns a composite object (user + peers + counts + torrents) but the doc claims body=Profile. Fix the schema or the response.
- responses( - (status = 200, description = "Successfully got the user's profile", body=Profile), - ) + responses( + (status = 200, description = "Successfully got the user's dashboard data", body = GetMeResponse), + )Add a typed response DTO (outside this hunk):
#[derive(serde::Serialize, utoipa::ToSchema)] pub struct GetMeResponse { pub user: Profile, pub peers: serde_json::Value, // replace with concrete type if available pub user_warnings: serde_json::Value, // replace with concrete type pub unread_conversations_amount: i64, pub unread_notifications_amount: i64, pub last_five_uploaded_torrents: serde_json::Value, // or concrete type pub last_five_snatched_torrents: serde_json::Value, // or concrete type }
45-55: Prefer typed results and avoid unwrap on map accesssearch_torrents(...).get("title_groups").unwrap() can panic. Either return a typed struct from storage or handle missing keys gracefully.
- let uploaded_torrents = arc + let uploaded_torrents = arc .pool .search_torrents(&torrent_search, Some(current_user.id)) .await?; @@ - let snatched_torrents = arc + let snatched_torrents = arc .pool .search_torrents(&torrent_search, Some(current_user.id)) .await?;And when extracting:
- "last_five_uploaded_torrents": uploaded_torrents.get("title_groups").unwrap(), - "last_five_snatched_torrents": snatched_torrents.get("title_groups").unwrap() + "last_five_uploaded_torrents": uploaded_torrents.get("title_groups").cloned().unwrap_or_default(), + "last_five_snatched_torrents": snatched_torrents.get("title_groups").cloned().unwrap_or_default()
65-73: Return a typed DTO instead of ad-hoc json!Move to a GetMeResponse struct (see above) and construct it without json!, which will catch type drift at compile time.
- Ok(HttpResponse::Ok().json(json!({ - "user": current_user, - "peers":peers, - "user_warnings": user_warnings, - "unread_conversations_amount": unread_conversations_amount, - "unread_notifications_amount":unread_notifications_amount, - "last_five_uploaded_torrents": uploaded_torrents.get("title_groups").unwrap(), - "last_five_snatched_torrents": snatched_torrents.get("title_groups").unwrap() - }))) + Ok(HttpResponse::Ok().json(GetMeResponse { + user: current_user, + peers, + user_warnings, + unread_conversations_amount, + unread_notifications_amount, + last_five_uploaded_torrents: uploaded_torrents + .get("title_groups") + .cloned() + .unwrap_or_default(), + last_five_snatched_torrents: snatched_torrents + .get("title_groups") + .cloned() + .unwrap_or_default(), + }))
♻️ Duplicate comments (9)
backend/api/src/handlers/conversations/create_conversation_message.rs (1)
1-1: Ensure storage enforces “author is participant” before insert.Passing user.sub is good; verify create_conversation_message checks membership and returns 403/forbidden on violation. Avoid TOCTOU by validating in the same transaction as insert.
Run:
#!/bin/bash # Find impl and inspect for participation check rg -nP -C3 '(pub\s+async\s+fn|fn)\s+create_conversation_message\s*\(' backend | cat rg -nP -C3 'participant|membership|users?_in_conversation|conversation_participants' backend/storage || trueAlso applies to: 18-19, 22-22
backend/api/src/handlers/torrents/create_torrent.rs (1)
21-26: Gate uploads by role using claims; replace TODO with an authorization check.Right now any authenticated user can upload. Enforce role(s) from JWT and fail fast.
Apply:
pub async fn exec( form: MultipartForm<UploadedTorrent>, arc: web::Data<Arcadia>, user: Authdata, ) -> Result<HttpResponse> { - // TODO : check if user can upload + // Enforce upload permissions from claims + let role = user.class.to_ascii_lowercase(); + if !matches!(role.as_str(), "uploader" | "staff" | "moderator") { + return Ok(HttpResponse::Forbidden().finish()); + } let torrent = arc.pool.create_torrent(&form, user.sub).await?;Also standardize role casing across handlers to avoid brittle string compares.
backend/api/src/handlers/torrents/delete_torrent.rs (1)
26-29: Remove unwrap() on URL join to avoid panic.Build the URL safely as a string; no runtime join, no panic.
Apply:
- let user_url = &arc - .frontend_url - .join(&format!("/user/{}", user.sub)) - .unwrap(); + let user_url = format!( + "{}/user/{}", + arc.frontend_url.as_str().trim_end_matches('/'), + user.sub + ); @@ - &user_url.as_str(), + &user_url.as_str(),Also applies to: 38-39
backend/api/src/handlers/torrents/edit_torrent.rs (1)
3-3: Duplicate: Authdata naming nit noted elsewhereSee prior comment about renaming Authdata → AuthData across the crate.
backend/api/src/handlers/conversations/create_conversation.rs (1)
1-1: Duplicate: Authdata naming nit noted elsewhereSee earlier note to rename Authdata → AuthData.
backend/api/src/handlers/wiki/create_wiki_article.rs (1)
1-1: Duplicate: Authdata naming nit noted elsewhereSee earlier note to rename Authdata → AuthData.
backend/api/src/handlers/forum/create_forum_post.rs (1)
1-1: Duplicate: Authdata naming nit noted elsewhereSee earlier note to rename Authdata → AuthData.
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
18-23: Add staff-only (or equivalent) authorization checkPrevious review requested enforcing RBAC here; please gate by user.class (e.g., staff) before calling create_artists_affiliation.
pub async fn exec( artists: web::Json<Vec<UserCreatedAffiliatedArtist>>, arc: web::Data<Arcadia>, user: Authdata, ) -> Result<HttpResponse> { + if user.class != "staff" { + return Err(arcadia_common::error::Error::InsufficientPrivileges); + } let affiliations = arc .pool .create_artists_affiliation(&artists, user.sub) .await?;If the project uses a different role or helper (e.g., is_staff(user.class)), reuse that for consistency.
backend/api/src/handlers/users/get_me.rs (1)
22-24: Avoid ad-hoc scrubbing; fetch a sanitized ProfileZeroing password_hash is brittle and risks leaking other secrets. Fetch a Profile projection instead.
-pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> { - let mut current_user = arc.pool.find_user_with_id(user.sub).await?; - current_user.password_hash = String::from(""); +pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> { + let current_user: Profile = arc.pool.find_user_profile(user.sub).await?;
🧹 Nitpick comments (12)
backend/api/src/handlers/conversations/create_conversation_message.rs (1)
6-14: Document the request body in the OpenAPI path.The handler accepts JSON; add request_body for accurate API docs.
Apply:
#[utoipa::path( post, operation_id = "Create conversation message", tag = "Conversation", path = "/api/conversations/messages", + request_body = UserCreatedConversationMessage, responses( (status = 201, description = "Successfully created the conversation's message", body=ConversationMessage), ) )]Also applies to: 16-23
backend/api/src/handlers/master_groups/create_master_group.rs (1)
6-14: Add request_body to OpenAPI for accurate docs.Declare the expected JSON payload.
Apply:
#[utoipa::path( post, operation_id = "Create master group", tag = "Master Group", path = "/api/master-groups", + request_body = UserCreatedMasterGroup, responses( (status = 201, description = "Successfully created the master group", body=MasterGroup), ) )]Also applies to: 15-21
backend/api/src/handlers/torrents/edit_torrent.rs (2)
23-28: Avoid stringly-typed role checksComparing user.class to a string is fragile. At minimum, use a constant; ideally, model roles as an enum and parse/validate at the middleware boundary.
Apply within this function:
- if torrent.created_by_id == user.sub || user.class == "staff" { + // Consider centralizing this constant or using an enum. + const STAFF: &str = "staff"; + if torrent.created_by_id == user.sub || user.class == STAFF {If you prefer an enum-based approach, I can draft it.
23-28: Optional: reduce resource-enumeration signalReturning InsufficientPrivileges after a successful find can leak that the torrent exists to unauthorized users. Consider responding with NotFound in both cases or normalizing errors to avoid ID enumeration.
backend/api/src/handlers/conversations/create_conversation.rs (1)
21-27: Avoid variable shadowing for clarityThe created entity shadows the request payload variable name. Rename the result.
- let conversation = arc + let created_conversation = arc .pool - .create_conversation(&mut conversation, user.sub) + .create_conversation(&mut conversation, user.sub) .await?; - - Ok(HttpResponse::Created().json(conversation)) + Ok(HttpResponse::Created().json(created_conversation))backend/api/src/handlers/wiki/create_wiki_article.rs (1)
20-22: Avoid stringly-typed role checksUse a constant or enum instead of a bare "staff" string to prevent typos and centralize role semantics.
- if user.class != "staff" { + const STAFF: &str = "staff"; + if user.class != STAFF { return Err(Error::InsufficientPrivileges); }backend/api/src/handlers/subscriptions/remove_subscription.rs (1)
26-27: Consider surfacing a 404 when nothing was deletedIf delete_subscription returns “0 rows affected,” return a 404 Not Found instead of a generic 200 to better signal idempotent no-ops vs. successful deletes.
- arc.pool - .delete_subscription(query.item_id, &query.item, user.sub) - .await?; + let affected = arc + .pool + .delete_subscription(query.item_id, &query.item, user.sub) + .await?; + if affected == 0 { + return Ok(HttpResponse::NotFound().finish()); + }backend/api/src/handlers/title_groups/create_title_group.rs (1)
17-18: Fix OpenAPI response code to match actual 201 CreatedThe handler returns Created but the schema advertises 200.
- (status = 200, description = "Successfully created the title_group", body=TitleGroup), + (status = 201, description = "Successfully created the title_group", body=TitleGroup),Also applies to: 53-53
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
11-13: Align OpenAPI status with 201 CreatedSchema says 200 but the handler returns Created.
- (status = 200, description = "Successfully created the artist affiliations", body=Vec<AffiliatedArtistHierarchy>), + (status = 201, description = "Successfully created the artist affiliations", body=Vec<AffiliatedArtistHierarchy>),Also applies to: 25-25
backend/api/src/middlewares/jwt_middleware.rs (2)
12-16: Type name/style: use AuthData and consider a typed roleRust types are CamelCase; prefer AuthData. Also consider an enum for class (e.g., UserClass) to avoid stringly-typed auth decisions.
-#[derive(Debug, Clone)] -pub struct Authdata { +#[derive(Debug, Clone)] +pub struct AuthData { pub sub: i64, pub class: String, }
18-29: Extractor looks good; align error text with middlewareFromRequest is fine. Consider reusing the same 401 message as authenticate_user for consistency.
backend/api/src/handlers/users/get_me.rs (1)
22-64: Latency: issue N+1 and sequential awaits; batch with try_joinMost calls here are independent. Run them concurrently to cut tail latency.
- let peers = arc.pool.get_user_peers(current_user.id).await; - let user_warnings = arc.pool.find_user_warnings(current_user.id).await; + use futures_util::try_join; + let (peers, user_warnings) = try_join!( + arc.pool.get_user_peers(current_user.id), + arc.pool.find_user_warnings(current_user.id), + )?; @@ - let uploaded_torrents = arc - .pool - .search_torrents(&torrent_search, Some(current_user.id)) - .await?; + let uploaded_fut = arc.pool.search_torrents(&torrent_search, Some(current_user.id)); @@ - let snatched_torrents = arc - .pool - .search_torrents(&torrent_search, Some(current_user.id)) - .await?; + let snatched_fut = arc.pool.search_torrents(&torrent_search, Some(current_user.id)); + let (uploaded_torrents, snatched_torrents) = try_join!(uploaded_fut, snatched_fut)?; @@ - let unread_conversations_amount = arc - .pool - .find_unread_conversations_amount(current_user.id) - .await?; - let unread_notifications_amount = arc - .pool - .find_unread_notifications_amount(current_user.id) - .await?; + let (unread_conversations_amount, unread_notifications_amount) = try_join!( + arc.pool.find_unread_conversations_amount(current_user.id), + arc.pool.find_unread_notifications_amount(current_user.id), + )?;
📜 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 selected for processing (41)
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs(2 hunks)backend/api/src/handlers/artists/create_artists.rs(2 hunks)backend/api/src/handlers/conversations/create_conversation.rs(2 hunks)backend/api/src/handlers/conversations/create_conversation_message.rs(2 hunks)backend/api/src/handlers/conversations/get_conversation.rs(2 hunks)backend/api/src/handlers/edition_groups/create_edition_group.rs(2 hunks)backend/api/src/handlers/forum/create_forum_post.rs(2 hunks)backend/api/src/handlers/forum/create_forum_thread.rs(2 hunks)backend/api/src/handlers/gifts/create_gift.rs(2 hunks)backend/api/src/handlers/invitations/create_invitation.rs(2 hunks)backend/api/src/handlers/master_groups/create_master_group.rs(2 hunks)backend/api/src/handlers/search/search_torrents.rs(2 hunks)backend/api/src/handlers/series/create_series.rs(2 hunks)backend/api/src/handlers/subscriptions/create_subscription.rs(2 hunks)backend/api/src/handlers/subscriptions/remove_subscription.rs(2 hunks)backend/api/src/handlers/title_groups/create_title_group.rs(4 hunks)backend/api/src/handlers/title_groups/create_title_group_comment.rs(2 hunks)backend/api/src/handlers/title_groups/edit_title_group.rs(2 hunks)backend/api/src/handlers/title_groups/get_title_group.rs(2 hunks)backend/api/src/handlers/torrent_requests/create_torrent_request.rs(2 hunks)backend/api/src/handlers/torrent_requests/create_torrent_request_vote.rs(2 hunks)backend/api/src/handlers/torrent_requests/fill_torrent_request.rs(2 hunks)backend/api/src/handlers/torrent_requests/get_torrent_request.rs(2 hunks)backend/api/src/handlers/torrents/create_torrent.rs(2 hunks)backend/api/src/handlers/torrents/create_torrent_report.rs(2 hunks)backend/api/src/handlers/torrents/delete_torrent.rs(3 hunks)backend/api/src/handlers/torrents/download_dottorrent_file.rs(2 hunks)backend/api/src/handlers/torrents/edit_torrent.rs(2 hunks)backend/api/src/handlers/torrents/get_registered_torrents.rs(2 hunks)backend/api/src/handlers/torrents/get_upload_information.rs(2 hunks)backend/api/src/handlers/user_applications/get_user_applications.rs(2 hunks)backend/api/src/handlers/user_applications/update_user_application_status.rs(2 hunks)backend/api/src/handlers/users/create_api_key.rs(2 hunks)backend/api/src/handlers/users/edit_user.rs(2 hunks)backend/api/src/handlers/users/get_me.rs(3 hunks)backend/api/src/handlers/users/get_registered_users.rs(2 hunks)backend/api/src/handlers/users/get_user.rs(3 hunks)backend/api/src/handlers/users/get_user_conversations.rs(2 hunks)backend/api/src/handlers/users/warn_user.rs(2 hunks)backend/api/src/handlers/wiki/create_wiki_article.rs(2 hunks)backend/api/src/middlewares/jwt_middleware.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- backend/api/src/handlers/torrents/get_registered_torrents.rs
- backend/api/src/handlers/users/get_user_conversations.rs
- backend/api/src/handlers/edition_groups/create_edition_group.rs
- backend/api/src/handlers/conversations/get_conversation.rs
- backend/api/src/handlers/forum/create_forum_thread.rs
- backend/api/src/handlers/torrent_requests/create_torrent_request_vote.rs
- backend/api/src/handlers/search/search_torrents.rs
- backend/api/src/handlers/title_groups/get_title_group.rs
- backend/api/src/handlers/users/warn_user.rs
- backend/api/src/handlers/torrent_requests/get_torrent_request.rs
- backend/api/src/handlers/torrent_requests/create_torrent_request.rs
- backend/api/src/handlers/invitations/create_invitation.rs
- backend/api/src/handlers/title_groups/create_title_group_comment.rs
- backend/api/src/handlers/torrent_requests/fill_torrent_request.rs
- backend/api/src/handlers/subscriptions/create_subscription.rs
- backend/api/src/handlers/users/get_registered_users.rs
- backend/api/src/handlers/series/create_series.rs
- backend/api/src/handlers/user_applications/update_user_application_status.rs
- backend/api/src/handlers/gifts/create_gift.rs
- backend/api/src/handlers/users/edit_user.rs
- backend/api/src/handlers/users/get_user.rs
- backend/api/src/handlers/user_applications/get_user_applications.rs
- backend/api/src/handlers/torrents/create_torrent_report.rs
- backend/api/src/handlers/title_groups/edit_title_group.rs
🧰 Additional context used
🧬 Code graph analysis (4)
backend/api/src/handlers/torrents/get_upload_information.rs (4)
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
exec(15-26)backend/api/src/handlers/gifts/create_gift.rs (1)
exec(15-31)backend/api/src/handlers/invitations/create_invitation.rs (1)
exec(17-55)backend/api/src/handlers/search/search_torrents.rs (1)
exec(28-36)
backend/api/src/handlers/users/get_me.rs (4)
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
exec(15-26)backend/api/src/handlers/conversations/create_conversation.rs (1)
exec(15-27)backend/api/src/handlers/gifts/create_gift.rs (1)
exec(15-31)backend/api/src/handlers/invitations/create_invitation.rs (1)
exec(17-55)
backend/api/src/middlewares/jwt_middleware.rs (1)
backend/api/src/handlers/auth/refresh_token.rs (1)
decode(19-19)
backend/api/src/handlers/title_groups/create_title_group.rs (1)
backend/api/src/handlers/external_db/get_tmdb_data.rs (1)
get_tmdb_rating(106-138)
⏰ 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). (4)
- GitHub Check: schema-check
- GitHub Check: build
- GitHub Check: build
- GitHub Check: schema-check
🔇 Additional comments (15)
backend/api/src/handlers/torrents/download_dottorrent_file.rs (1)
31-41: Confirm access control before download.
I didn’t see any ban, disabled-account, passkey or membership checks inget_torrent; ensure the caller’s rights (e.g.is_user_banned, account enabled, active passkey, membership rules) are validated before returning the torrent.backend/api/src/handlers/torrents/delete_torrent.rs (1)
25-25: Reintroducing DB read counters PR goal; confirm necessity.You fetch current_user from DB solely for display. If acceptable per PR scope, keep; otherwise consider passing the handler’s display info differently (e.g., claims, lookup by storage only when needed).
backend/api/src/handlers/users/create_api_key.rs (1)
19-22: LGTM: extractor swap and user.sub usageThe handler correctly switches to Authdata and passes user.sub to storage.
backend/api/src/handlers/torrents/edit_torrent.rs (1)
19-20: LGTM: handler signature updated to AuthdataExtractor change is consistent with the PR objective.
backend/api/src/handlers/conversations/create_conversation.rs (1)
23-24: No action needed:web::Json<UserCreatedConversation>implementsDerefMut, so&mut conversationcoerces to&mut UserCreatedConversation, matching thecreate_conversationsignature.backend/api/src/handlers/forum/create_forum_post.rs (1)
20-23: LGTM: switched to user.sub and returns 201The storage call and response are consistent with the new Authdata flow.
backend/api/src/handlers/subscriptions/remove_subscription.rs (1)
2-3: Authdata substitution looks correctUsing Authdata and passing user.sub to delete_subscription aligns with the PR objective and avoids the per-request User load.
Also applies to: 23-27
backend/api/src/handlers/title_groups/create_title_group.rs (3)
23-24: RBAC: confirm required role for creating title groupsIf this action is staff-only (or otherwise restricted), add an authorization guard using user.class to maintain parity with prior behavior.
Would you like me to scan the repo for where “create title group” was previously guarded and open a follow-up patch?
39-41: LGTM on identity flowPassing user.sub into create_title_group matches the storage API changes.
47-51: LGTM on affiliated artists linkageSetting title_group_id before create_artists_affiliation and using user.sub is correct.
backend/api/src/handlers/torrents/get_upload_information.rs (2)
22-28: Ensure tracker URL is configured; fail gracefully if missingIf arc.tracker.url can be None/misconfigured, return a 503 (or domain error) instead of relying on downstream assumptions.
-pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> { +pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> { let current_user = arc.pool.find_user_with_id(user.sub).await?; - let announce_url = get_announce_url( - current_user.passkey_upper, - current_user.passkey_lower, - arc.tracker.url.as_ref(), - ); + let tracker_url = match arc.tracker.url.as_ref() { + Some(u) => u, + None => { + log::error!("Tracker URL not configured"); + return Err(arcadia_common::error::Error::ServiceUnavailable); + } + }; + let announce_url = get_announce_url(current_user.passkey_upper, current_user.passkey_lower, tracker_url);If Error::ServiceUnavailable doesn’t exist, I can wire an appropriate variant.
22-24: DB lookup is justified hereFetching passkeys for announce_url generation requires a user load; this remains aligned with the PR’s intent to avoid unnecessary loads elsewhere.
backend/api/src/handlers/artists/create_artists.rs (2)
1-1: Authdata migration and identity propagation look goodSwitch to Authdata and passing user.sub into create_artists aligns with the storage API.
Also applies to: 19-22
16-20: Confirm RBAC on artist creationIf creating artists is restricted (e.g., staff), add the same guard using user.class for parity with other create endpoints.
I can scan for existing patterns (InsufficientPrivileges checks) and propose a consistent helper.
backend/api/src/middlewares/jwt_middleware.rs (1)
103-108: LGTM: minimal claims injected into request contextInsertion of only sub and class matches the PR goal. Keep it minimal.
| user: Authdata, | ||
| ) -> Result<HttpResponse> { | ||
| let master_group = arc | ||
| .pool | ||
| .create_master_group(&form, current_user_id.0) | ||
| .await?; | ||
| let master_group = arc.pool.create_master_group(&form, user.sub).await?; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enforce authorization (who can create master groups?).
Unless storage rejects unauthorized callers, add a claims-based guard (e.g., staff/mod only) for defense-in-depth.
Apply (adjust roles/policy as needed):
pub async fn exec(
form: web::Json<UserCreatedMasterGroup>,
arc: web::Data<Arcadia>,
user: Authdata,
) -> Result<HttpResponse> {
+ if user.class.to_ascii_lowercase() != "staff" {
+ return Ok(HttpResponse::Forbidden().finish());
+ }
let master_group = arc.pool.create_master_group(&form, user.sub).await?;🏁 Script executed:
#!/bin/bash
# Locate the definition of Authdata in the Rust codebase
rg -nP 'struct\s+Authdata\b' -C3 --type=rsLength of output: 71
🏁 Script executed:
#!/bin/bash
rg -n "Authdata" -C3 -g '*.rs'
rg -n "AuthData" -C3 -g '*.rs'Length of output: 48459
Enforce staff-only creation of master groups. Insert at the top of exec in backend/api/src/handlers/master_groups/create_master_group.rs before calling create_master_group:
pub async fn exec(
form: web::Json<UserCreatedMasterGroup>,
arc: web::Data<Arcadia>,
user: Authdata,
) -> Result<HttpResponse> {
+ if user.class.to_ascii_lowercase() != "staff" {
+ return Ok(HttpResponse::Forbidden().finish());
+ }
let master_group = arc.pool.create_master_group(&form, user.sub).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.
| user: Authdata, | |
| ) -> Result<HttpResponse> { | |
| let master_group = arc | |
| .pool | |
| .create_master_group(&form, current_user_id.0) | |
| .await?; | |
| let master_group = arc.pool.create_master_group(&form, user.sub).await?; | |
| pub async fn exec( | |
| form: web::Json<UserCreatedMasterGroup>, | |
| arc: web::Data<Arcadia>, | |
| user: Authdata, | |
| ) -> Result<HttpResponse> { | |
| if user.class.to_ascii_lowercase() != "staff" { | |
| return Ok(HttpResponse::Forbidden().finish()); | |
| } | |
| let master_group = arc.pool.create_master_group(&form, user.sub).await?; |
🤖 Prompt for AI Agents
In backend/api/src/handlers/master_groups/create_master_group.rs around lines 18
to 21, add a staff-only guard at the top of exec before calling
arc.pool.create_master_group: check the Authdata on the incoming user (e.g.
user.is_staff or user.roles.contains("staff") depending on the Authdata shape)
and if the user is not staff return early with a 403 Forbidden (or
Err(actix_web::error::ErrorForbidden(...))) instead of proceeding; place this
check immediately before the create_master_group call so non-staff cannot create
master groups.
| if user.class != "staff" { | ||
| return Err(Error::InsufficientPrivileges); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize role comparison; avoid case sensitivity traps.
Compare on lowercased role or centralize via a helper to keep policy consistent across handlers.
Apply:
- if user.class != "staff" {
+ if user.class.to_ascii_lowercase() != "staff" {
return Err(Error::InsufficientPrivileges);
}Optionally add a shared is_staff(&str) helper and reuse.
📝 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.
| if user.class != "staff" { | |
| return Err(Error::InsufficientPrivileges); | |
| } | |
| if user.class.to_ascii_lowercase() != "staff" { | |
| return Err(Error::InsufficientPrivileges); | |
| } |
🤖 Prompt for AI Agents
In backend/api/src/handlers/torrents/delete_torrent.rs around lines 22 to 24,
the code compares user.class to the literal "staff" with a case-sensitive
equality which can fail for different-cased role values; change the check to a
case-insensitive comparison (e.g., compare lowercased values) or, better, call a
shared helper is_staff(&str) that centralizes role normalization and returns
true for any casing variant, then use that helper here to return
Err(Error::InsufficientPrivileges) when it returns false.
| @@ -1,4 +1,4 @@ | |||
| use crate::{handlers::User, Arcadia}; | |||
| use crate::{middlewares::jwt_middleware::Authdata, Arcadia}; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use Rust type naming: rename Authdata → AuthData
Type names should be UpperCamelCase. Please rename the type and imports for consistency.
Run to locate all occurrences:
🏁 Script executed:
#!/bin/bash
rg -nP '\bAuthdata\b' -C1
ast-grep --pattern $'struct Authdata { $$$ }' || trueLength of output: 21798
Rename Authdata to AuthData
Update the struct declaration in backend/api/src/middlewares/jwt_middleware.rs:13 and all imports/uses of Authdata across the codebase to AuthData to comply with Rust UpperCamelCase type naming.
🤖 Prompt for AI Agents
In backend/api/src/handlers/users/create_api_key.rs around line 1, the import
uses the old type name `Authdata`; update this to `AuthData` and adjust all
other usages/imports accordingly. Also modify the struct declaration in
backend/api/src/middlewares/jwt_middleware.rs at line 13 from `Authdata` to
`AuthData`, update any pattern matches, type annotations, and imports across the
codebase to the new name, and run cargo check to catch remaining references.
| user: Authdata, | ||
| ) -> Result<HttpResponse> { | ||
| if current_user.class != "staff" { | ||
| if user.class != "staff" { | ||
| return Err(Error::InsufficientPrivileges); | ||
| } | ||
|
|
||
| let article = arc | ||
| .pool | ||
| .create_wiki_article(&article, current_user.id) | ||
| .await?; | ||
| let article = arc.pool.create_wiki_article(&article, user.sub).await?; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
JWT-embedded roles: ensure revocation and freshness
Since authorization depends on user.class from the token, confirm middleware verifies signature/alg, enforces short TTLs, and supports revocation on role changes (e.g., jti blacklist or auth_version/iat checks).
🏁 Script executed:
#!/bin/bash
# Inspect jwt_middleware for claim validation & revocation strategy
rg -nP 'struct\s+Authdata|Claims|class|sub|jti|iat' backend/api/src/middlewares/jwt_middleware.rs -n -C3 || trueLength of output: 1483
Enforce revocable, short-lived JWTs Signature and expiration are validated via decode() with default Validation in backend/api/src/middlewares/jwt_middleware.rs, but there’s no JTI- or auth_version/iat-based revocation—tokens remain valid even after role changes. Implement a revocation strategy (e.g. JTI blacklist or auth_version lookup against iat) and ensure issuance with a short TTL.
| let user_id = token_data.claims.sub; | ||
|
|
||
| let banned = arc.pool.is_user_banned(user_id).await; | ||
| let Ok(banned) = arc.pool.is_user_banned(user_id).await else { | ||
| return Err(( | ||
| actix_web::error::ErrorUnauthorized("account does not exist"), | ||
| req, | ||
| )); | ||
| }; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t collapse all DB errors into “account does not exist”
Differentiate NotFound vs transient/storage errors; return 500 for backend issues, 401 only when the user truly doesn’t exist.
- let Ok(banned) = arc.pool.is_user_banned(user_id).await else {
- return Err((
- actix_web::error::ErrorUnauthorized("account does not exist"),
- req,
- ));
- };
+ let banned = match arc.pool.is_user_banned(user_id).await {
+ Ok(b) => b,
+ // Replace StorageError::NotFound with your concrete type
+ Err(e) if e.to_string().contains("not found") => {
+ return Err((actix_web::error::ErrorUnauthorized("account does not exist"), req))
+ }
+ Err(e) => {
+ log::warn!("is_user_banned({}) failed: {}", user_id, e);
+ return Err((actix_web::error::ErrorInternalServerError("authentication backend error"), req));
+ }
+ };🏁 Script executed:
#!/bin/bash
# Search for occurrences and definition of `is_user_banned` to inspect its return type and error variants
rg -n --type=rs "is_user_banned" -C3Length of output: 63
🏁 Script executed:
#!/bin/bash
# Search for occurrences and definition of `is_user_banned` in Rust files
rg -n "is_user_banned" -C3 -g "*.rs"Length of output: 1284
🏁 Script executed:
ls -R backend/storage/srcLength of output: 1082
🏁 Script executed:
#!/bin/bash
# Locate the definition of is_user_banned with surrounding context
grep -R -n "pub async fn is_user_banned" -C5 backend/storage/src/repositories/user_repository.rsLength of output: 514
🏁 Script executed:
#!/bin/bash
# Find the Result alias and Error enum definitions in storage module
grep -R -n "type Result" -n backend/storage/src
grep -R -n "enum .*Error" -n backend/storage/src
# Show the full is_user_banned method implementation
sed -n '140,180p' backend/storage/src/repositories/user_repository.rsLength of output: 1054
🏁 Script executed:
grep -R -n "type Result" backend/storage/src
grep -R -n "enum .*Error" backend/storage/src
sed -n '1,200p' backend/storage/src/lib.rsLength of output: 214
🏁 Script executed:
#!/bin/bash
grep -R -n "token_data" -C5 backend/api/src/middlewares/jwt_middleware.rsLength of output: 1039
🏁 Script executed:
#!/bin/bash
sed -n '1,80p' backend/storage/src/repositories/user_repository.rsLength of output: 2392
Separate user-not-found from storage errors in is_user_banned and middleware
- In
backend/storage/src/repositories/user_repository.rs:is_user_banned, returnErr(Error::UserWithIdNotFound(user_id))instead ofOk(true)when no row is found. - In
backend/api/src/middlewares/jwt_middleware.rs, match on the concreteErrorvariants:Err(Error::UserWithIdNotFound(_))→ErrorUnauthorized("account does not exist")Err(_)→ErrorInternalServerError("authentication backend error")Ok(true)→ErrorUnauthorized("account is banned")Ok(false)→ continue processing
🤖 Prompt for AI Agents
In backend/api/src/middlewares/jwt_middleware.rs around lines 91 to 98, and
backend/storage/src/repositories/user_repository.rs, change the behavior so
missing users produce a specific Error variant and the middleware matches on it:
in user_repository.rs make is_user_banned return
Err(Error::UserWithIdNotFound(user_id)) when the lookup finds no row (instead of
Ok(true)); in jwt_middleware.rs replace the current let Ok(banned) = ... else {
... } pattern with an explicit match on the repository result and map variants
as follows — Err(Error::UserWithIdNotFound(_)) -> return
ErrorUnauthorized("account does not exist"), Err(_) -> return
ErrorInternalServerError("authentication backend error"), Ok(true) -> return
ErrorUnauthorized("account is banned"), Ok(false) -> continue processing the
request.
| let user = match arc.pool.find_user_id_with_api_key(api_key).await { | ||
| Ok(user) => user, | ||
| Err(e) => return Err((actix_web::error::ErrorUnauthorized(e.to_string()), req)), | ||
| }; | ||
|
|
||
| req.extensions_mut() | ||
| .insert(crate::handlers::UserId(user_id)); | ||
| req.extensions_mut().insert(Authdata { | ||
| sub: user.id, | ||
| class: user.class, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
API key path misses ban/last_seen checks and leaks error details
- Parity with JWT path: check bans and update last_seen.
- Don’t surface internal error messages to clients; log them and return a generic 401.
- let user = match arc.pool.find_user_id_with_api_key(api_key).await {
- Ok(user) => user,
- Err(e) => return Err((actix_web::error::ErrorUnauthorized(e.to_string()), req)),
- };
+ let user = match arc.pool.find_user_id_with_api_key(api_key).await {
+ Ok(user) => user,
+ Err(e) => {
+ log::info!("api key auth failed: {}", e);
+ return Err((actix_web::error::ErrorUnauthorized("invalid API key"), req));
+ }
+ };
+ // Enforce bans and update last_seen just like bearer flow
+ if let Ok(true) = arc.pool.is_user_banned(user.id).await {
+ return Err((actix_web::error::ErrorUnauthorized("account banned"), req));
+ }
+ let _ = arc.pool.update_last_seen(user.id).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 user = match arc.pool.find_user_id_with_api_key(api_key).await { | |
| Ok(user) => user, | |
| Err(e) => return Err((actix_web::error::ErrorUnauthorized(e.to_string()), req)), | |
| }; | |
| req.extensions_mut() | |
| .insert(crate::handlers::UserId(user_id)); | |
| req.extensions_mut().insert(Authdata { | |
| sub: user.id, | |
| class: user.class, | |
| }); | |
| let user = match arc.pool.find_user_id_with_api_key(api_key).await { | |
| Ok(user) => user, | |
| Err(e) => { | |
| log::info!("api key auth failed: {}", e); | |
| return Err((actix_web::error::ErrorUnauthorized("invalid API key"), req)); | |
| } | |
| }; | |
| // Enforce bans and update last_seen just like bearer flow | |
| if let Ok(true) = arc.pool.is_user_banned(user.id).await { | |
| return Err((actix_web::error::ErrorUnauthorized("account banned"), req)); | |
| } | |
| let _ = arc.pool.update_last_seen(user.id).await; | |
| req.extensions_mut().insert(Authdata { | |
| sub: user.id, | |
| class: user.class, | |
| }); |
🤖 Prompt for AI Agents
In backend/api/src/middlewares/jwt_middleware.rs around lines 123–131, the
API-key branch currently returns DB error text to the client and doesn't perform
ban/last_seen handling like the JWT branch; change it so that DB errors are
logged (do not leak e.to_string() to the response) and you return a generic 401,
then after successfully loading the user check user.banned (or equivalent) and
return a generic Unauthorized if banned, and finally call the existing last_seen
update code (await the update, log any update error but do not fail the request)
before inserting Authdata into req.extensions_mut().
|
Yea but having some logic to invalidate a jwt token kinda kills its advantage of not having to query the db... This video is quite interesting imo: https://www.youtube.com/watch?v=NXXiKl8g6Rw Sorry to insist a bit 😅 If you have good reasons to still use jwt over a more classic api key managed in the db (with expiration time etc, some of its logic is already implemented) then let me know! |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
backend/api/src/handlers/forum/create_forum_post.rs (1)
6-17: Fix OpenAPI response code: handler returns 201 but spec documents 200.Update the documented response to match the runtime status.
- (status = 200, description = "Successfully created the forum post", body=ForumPost), + (status = 201, description = "Created forum post", body = ForumPost),backend/api/src/handlers/edition_groups/create_edition_group.rs (2)
15-16: Fix OpenAPI response code mismatch (200 vs 201).Handler returns 201 (Created) but the spec documents 200. Align to 201.
- (status = 200, description = "Successfully created the edition_group", body=EditionGroup), + (status = 201, description = "Edition group created", body = EditionGroup),Also applies to: 25-25
6-17: Specify the request body in the OpenAPI path.Without
request_body, clients won’t see the input schema.#[utoipa::path( post, operation_id = "Create edition group", tag = "Edition Group", path = "/api/edition-groups", + request_body = UserCreatedEditionGroup, security( ("http" = ["Bearer"]) ),backend/api/src/handlers/series/create_series.rs (1)
15-15: OpenAPI response code mismatch (200 vs 201 Created)Handler returns
HttpResponse::Created()(201) but the doc advertises 200. Fix the spec to avoid client confusion.Apply:
- (status = 200, description = "Successfully created the series", body=Series), + (status = 201, description = "Successfully created the series", body=Series),Also applies to: 25-25
backend/api/src/handlers/users/get_me.rs (3)
13-24: OpenAPI schema is wrong (declares Profile, returns a composite object).Doc says body=Profile but the handler returns a dashboard-like JSON with multiple fields. Fix the spec immediately (quick fix below), or define a proper DTO.
Apply this quick-fix diff to align the spec:
#[utoipa::path( @@ - responses( - (status = 200, description = "Successfully got the user's profile", body=Profile), - ) + responses( + (status = 200, description = "Successfully got the current user's dashboard object", body=serde_json::Value), + ) )]Preferable follow-up: introduce a GetMeResponse DTO with typed fields and switch body=GetMeResponse.
48-58: Avoid unwraps in HTTP response building; they can panic.get("title_groups").unwrap() will 500 if the key is absent. Use a safe default.
Apply this diff:
@@ - let uploaded_torrents = arc + let uploaded_torrents = arc .pool .search_torrents(&torrent_search, Some(current_user.id)) .await?; @@ - let snatched_torrents = arc + let snatched_torrents = arc .pool .search_torrents(&torrent_search, Some(current_user.id)) .await?; + let last_five_uploaded_torrents = uploaded_torrents + .get("title_groups") + .cloned() + .unwrap_or_else(|| json!([])); + let last_five_snatched_torrents = snatched_torrents + .get("title_groups") + .cloned() + .unwrap_or_else(|| json!([])); @@ - "last_five_uploaded_torrents": uploaded_torrents.get("title_groups").unwrap(), - "last_five_snatched_torrents": snatched_torrents.get("title_groups").unwrap() + "last_five_uploaded_torrents": last_five_uploaded_torrents, + "last_five_snatched_torrents": last_five_snatched_torrentsNote: This assumes search_torrents returns a serde_json::Map<String, Value>. If it returns a typed struct, expose fields instead of map access.
Also applies to: 74-75
68-76: Don’t serialize the full user model in the “user” field.Even with password_hash blanked, other secrets/PII may leak. Ensure the “user” field is Profile (or a sanitized DTO).
If adopting Profile per earlier diff, this line stays as-is but remains safe:
- "user": current_user, + "user": current_user, // ProfilePrefer also defining a typed GetMeResponse DTO and returning that instead of ad-hoc json!.
backend/api/src/handlers/title_groups/edit_title_group.rs (1)
26-31: Hide resource existence and centralize role checks
- In the
elsebranch, returnErr(Error::NotFound)instead ofErr(Error::InsufficientPrivileges)to return 404 and prevent ID enumeration:- Err(Error::InsufficientPrivileges) + Err(Error::NotFound)- Replace all direct
user.class == "staff"comparisons with a centralized role abstraction (e.g. aRoleenum or anis_staff()helper) to eliminate scattered string literals.- Update this endpoint’s OpenAPI responses to use 404 (“not found or unauthorized”) for unauthorized cases.
backend/api/src/handlers/torrents/delete_torrent.rs (2)
33-41: Tighten user-facing copy; fix punctuation and spacing.Minor grammar/formatting issues in the message.
- let displayed_reason = format!( - "A torrent you were a seeder on, has been deleted. - Please remove it from your torrent client. - -Reason: {} - -Handled by: [url={}]{}[/url]", + let displayed_reason = format!( + "A torrent you were seeding has been deleted.\n\ +Please remove it from your torrent client.\n\n\ +Reason: {}\n\n\ +Handled by: [url={}]{}[/url]", &form.reason, &user_url.as_str(), - current_user.username + staff_username );
33-43: Escape BBCode-sensitive characters in user inputsUser-controlled
form.reasonandcurrent_user.usernameare embedded raw into BBCode, enabling injection or phishing.
- Escape/sanitize
form.reasonbefore formatting.- Escape/sanitize
current_user.usernamebefore formatting.backend/api/src/handlers/affiliated_artists/remove_affiliated_artists.rs (1)
28-36: Plug the authorization gap: Authdata is ignored; any user can delete arbitrary affiliations by idRight now the handler authenticates but does not authorize. Deletion is not scoped to the caller or privileged roles. Use Authdata, validate input, and scope the delete to the caller (or gate by class).
Apply a change like this (adjust repo API accordingly):
pub async fn exec( query: actix_web_lab::extract::Query<RemoveAffiliatedArtistsQuery>, arc: web::Data<Arcadia>, - _: Authdata, + user: Authdata, ) -> Result<HttpResponse> { - // TODO: add protection based on user class - arc.pool - .delete_artists_affiliation(&query.affiliation_ids) - .await?; + // Optional: staff-only bypass based on class. Otherwise, scope to caller. + // if !matches!(user.class.as_str(), "Admin" | "Moderator") { ... } + + // Basic validation + dedup to keep IN() lists sane. + let mut ids = query.affiliation_ids.clone(); + ids.sort_unstable(); + ids.dedup(); + if ids.is_empty() { + return Ok(HttpResponse::BadRequest().json(json!({"error": "affiliation_ids cannot be empty"}))); + } + if ids.len() > 1000 { + return Ok(HttpResponse::PayloadTooLarge().json(json!({"error": "too many affiliation_ids"}))); + } + + // Scope deletion to the caller to prevent cross-user deletes. + arc.pool + .delete_artists_affiliation_for_user(user.sub, &ids) + .await?; - Ok(HttpResponse::Ok().json(json!({"result": "success"}))) + Ok(HttpResponse::NoContent().finish()) }backend/api/src/handlers/conversations/create_conversation.rs (1)
15-16: OpenAPI/docs mismatch: handler returns 201 but spec says 200Clients will codegen against 200 while the server sends 201. Align the spec.
responses( - (status = 200, description = "Successfully created the conversation and first message", body=Conversation), + (status = 201, description = "Successfully created the conversation and first message", body=Conversation), ) )] @@ - Ok(HttpResponse::Created().json(conversation)) + Ok(HttpResponse::Created().json(conversation))Optionally add the request body to the spec for clarity:
#[utoipa::path( post, @@ - responses( + request_body = UserCreatedConversation, + responses(Also applies to: 29-30
♻️ Duplicate comments (4)
backend/api/src/handlers/users/create_api_key.rs (1)
1-1: Use Rust type naming: AuthData (UpperCamelCase).Rename the imported type to
AuthDatafor idiomatic Rust and consistency with prior feedback.-use crate::{middlewares::jwt_middleware::Authdata, Arcadia}; +use crate::{middlewares::jwt_middleware::AuthData, Arcadia};backend/api/src/handlers/users/get_me.rs (1)
25-27: Stop ad-hoc scrubbing; fetch a sanitized projection.Zeroing password_hash on the full User is brittle and risks leaking other sensitive fields. Fetch a Profile (or similar) directly.
Apply this diff:
pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> { - let mut current_user = arc.pool.find_user_with_id(user.sub).await?; - current_user.password_hash = String::from(""); + let current_user: Profile = arc.pool.find_user_profile(user.sub).await?;backend/api/src/handlers/torrents/delete_torrent.rs (2)
25-27: Normalize role check and avoid brittle string comparison.Make the check case-insensitive or centralize via an is_staff(&str) helper. Also confirm your token-invalidation path covers role changes; for admin actions like deletes, consider verifying against DB if feasible.
- if user.class != "staff" { + if user.class.to_ascii_lowercase() != "staff" { return Err(Error::InsufficientPrivileges); }
29-32: Remove unwrap() on URL join; avoid panic and simplify.Build the URL string safely; also drop the unnecessary extra borrow at usage.
- let user_url = &arc - .frontend_url - .join(&format!("/user/{}", user.sub)) - .unwrap(); + let user_url = format!( + "{}/user/{}", + arc.frontend_url.as_str().trim_end_matches('/'), + user.sub + ); @@ - &user_url.as_str(), + user_url.as_str(),Also applies to: 41-41
🧹 Nitpick comments (26)
backend/api/src/handlers/forum/create_forum_post.rs (1)
1-1: Type naming nit: preferAuthData(CamelCase) overAuthdata.Rust types conventionally use CamelCase; consider renaming project-wide. If you do, this file would change as:
-use crate::{middlewares::jwt_middleware::Authdata, Arcadia}; +use crate::{middlewares::jwt_middleware::AuthData, Arcadia};backend/api/src/handlers/users/create_api_key.rs (2)
24-27: Optional: include Location header for 201 Created and add an audit log.Improves REST semantics and observability.
- Ok(HttpResponse::Created().json(created_api_key)) + // tracing::info!(user_id = auth.sub, api_key_id = created_api_key.id, "created API key"); + Ok(HttpResponse::Created() + .append_header(("Location", format!("/api/users/api-keys/{}", created_api_key.id))) + .json(created_api_key))
22-22: Rename extractor parameter for clarity
Change the handler signature fromuser: Authdata,to
- user: Authdata, + auth: Authdata,so it’s clear this isn’t a full
Userbut JWT claims.backend/api/src/handlers/conversations/get_conversation.rs (2)
1-1: Alias the type to idiomatic CamelCase and reduce coupling to middleware pathRust types are CamelCase; aliasing also insulates this handler from a future move of the type.
-use crate::{middlewares::jwt_middleware::Authdata, Arcadia}; +use crate::{middlewares::jwt_middleware::Authdata as AuthData, Arcadia};
29-31: Clarify intent: name the param for claims and make the boolean self-documenting
- "user" suggests a loaded entity; this is JWT claims.
- Add an inline name to the boolean to avoid the “what does true mean?” question at call sites.
-pub async fn exec( - query: web::Query<GetConversationQuery>, - arc: web::Data<Arcadia>, - user: Authdata, -) -> Result<HttpResponse> { - let conversation_with_messages = arc.pool.find_conversation(query.id, user.sub, true).await?; +pub async fn exec( + query: web::Query<GetConversationQuery>, + arc: web::Data<Arcadia>, + auth: AuthData, +) -> Result<HttpResponse> { + let conversation_with_messages = + arc.pool.find_conversation(query.id, auth.sub, /* include_messages */ true).await?;backend/api/src/handlers/users/get_user_conversations.rs (1)
1-1: Name the type idiomatically and avoid exposing middleware pathConsider renaming Authdata to AuthData across the codebase (Rust style), or at least alias it here to keep handler signatures clean and decoupled from the middleware module path.
Possible local alias:
-use crate::{middlewares::jwt_middleware::Authdata, Arcadia}; +use crate::{middlewares::jwt_middleware::Authdata as AuthData, Arcadia};and
-pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> { +pub async fn exec(arc: web::Data<Arcadia>, user: AuthData) -> Result<HttpResponse> {backend/api/src/handlers/edition_groups/create_edition_group.rs (4)
11-13: Document all supported auth mechanisms.If API keys are also accepted (per PR discussion), add the corresponding security scheme to the endpoint.
security( - ("http" = ["Bearer"]) + ("http" = ["Bearer"]), + // Add if defined globally in the OpenAPI components + ("ApiKeyAuth" = []) ),Please confirm the exact scheme name as defined in your utoipa components before applying.
1-1: Type naming and placement: prefer AuthData and a generic module.Rename
Authdata→AuthData(Rust type casing) and consider moving it out ofjwt_middlewareto a neutralauthmodule to reflect non-JWT sources (e.g., API keys).
21-22: Parameter naming and intent.Rename
usertoauth(orauth_data) for clarity since it carries claims, not a loadedUser.-pub async fn exec( - form: web::Json<UserCreatedEditionGroup>, - arc: web::Data<Arcadia>, - user: Authdata, -) -> Result<HttpResponse> { +pub async fn exec( + form: web::Json<UserCreatedEditionGroup>, + arc: web::Data<Arcadia>, + auth: Authdata, +) -> Result<HttpResponse> { - let edition_group = arc.pool.create_edition_group(&form, user.sub).await?; + let edition_group = arc.pool.create_edition_group(&form, auth.sub).await?;
1-1: Add Location header on 201 Created.Return the URI of the newly created resource to improve client UX.
use actix_web::{web, HttpResponse}; +use actix_web::http::header; @@ - Ok(HttpResponse::Created().json(edition_group)) + let location = format!("/api/edition-groups/{}", edition_group.id); + Ok(HttpResponse::Created() + .append_header((header::LOCATION, location)) + .json(edition_group))Also applies to: 25-25
backend/api/src/handlers/series/create_series.rs (2)
1-1: Rust casing: rename Authdata → AuthData for type naming consistencyMatches Rust’s CamelCase type convention and improves readability. Coordinate with the type definition and imports project-wide.
Example for this file (after global rename):
-use crate::{middlewares::jwt_middleware::Authdata, Arcadia}; +use crate::{middlewares::jwt_middleware::AuthData, Arcadia}; @@ - user: Authdata, + user: AuthData,Also applies to: 21-21
19-19: Minor naming nit: renameserietonew_seriesorpayload“Series” is both singular and plural;
serieis uncommon. A clearer name reduces confusion.-pub async fn exec( - serie: web::Json<UserCreatedSeries>, +pub async fn exec( + new_series: web::Json<UserCreatedSeries>, @@ - let series = arc.pool.create_series(&serie, user.sub).await?; + let series = arc.pool.create_series(&new_series, user.sub).await?;Also applies to: 23-23
backend/api/src/handlers/users/get_me.rs (2)
40-58: Parallelize I/O-bound calls to reduce latency.Peers, warnings, both searches, and unread counts can be fetched concurrently.
Example:
use futures::try_join; let (peers, user_warnings) = try_join!( arc.pool.get_user_peers(current_user.id), arc.pool.find_user_warnings(current_user.id), )?; let uploaded = arc.pool.search_torrents(&uploaded_q, Some(current_user.id)); let snatched = arc.pool.search_torrents(&snatched_q, Some(current_user.id)); let (uploaded_torrents, snatched_torrents) = try_join!(uploaded, snatched)?; let (unread_conv, unread_notif) = try_join!( arc.pool.find_unread_conversations_amount(current_user.id), arc.pool.find_unread_notifications_amount(current_user.id), )?;Also applies to: 60-66
25-25: Optional: return a typed DTO instead of serde_json::json!.Typed responses help OpenAPI, prevent shape drift, and improve testability.
Example DTO (adjust field types as appropriate):
#[derive(serde::Serialize, utoipa::ToSchema)] pub struct GetMeResponse { pub user: Profile, // Replace serde_json::Value with concrete types if available: pub peers: serde_json::Value, pub user_warnings: serde_json::Value, pub unread_conversations_amount: i64, pub unread_notifications_amount: i64, pub last_five_uploaded_torrents: serde_json::Value, pub last_five_snatched_torrents: serde_json::Value, }backend/api/src/handlers/title_groups/edit_title_group.rs (2)
4-4: Prefer idiomatic naming and a generic auth context aliasAlias the extractor to AuthData (CamelCase) and decouple callers from the jwt_middleware type name. This eases a future rename to a generic, non-JWT-specific context if API keys are also supported.
-use crate::{middlewares::jwt_middleware::Authdata, Arcadia}; +use crate::{middlewares::jwt_middleware::Authdata as AuthData, Arcadia};
22-23: Rename the parameter to ‘auth’ to avoid “user”-semantics confusionThis handler no longer receives a full User; naming it auth clarifies intent and reduces accidental usages that assume DB-backed fields.
- user: Authdata, + auth: AuthData,Also update the use-site:
- if title_group.created_by_id == user.sub || user.class == "staff" { + if title_group.created_by_id == auth.sub || auth.class == "staff" {backend/api/src/handlers/torrents/delete_torrent.rs (3)
4-4: Use Rust casing (AuthData) and decouple handler from middleware name.Alias the import to PascalCase and use the alias in the signature. This keeps naming idiomatic and lets you later re-export AuthData from a stable module without touching handlers.
-use crate::{middlewares::jwt_middleware::Authdata, Arcadia}; +use crate::{middlewares::jwt_middleware::Authdata as AuthData, Arcadia}; @@ - user: Authdata, + user: AuthData,Also applies to: 23-23
28-28: Avoid full user fetch when only username is needed.This endpoint still hits the DB solely to render the moderator’s name. Add a lightweight query (e.g., find_username_by_id) to reduce I/O and payload size.
- let current_user = arc.pool.find_user_with_id(user.sub).await?; + let staff_username = arc.pool.find_username_by_id(user.sub).await?; @@ - current_user.username + staff_usernameAlso applies to: 42-42
21-21: Avoid mutating the extractor; operate on the inner payload.Consume Json to a local payload, mutate, then pass a clear &payload to storage.
- mut form: web::Json<TorrentToDelete>, + form: web::Json<TorrentToDelete>, @@ - form.displayed_reason = Some(displayed_reason); - arc.pool.remove_torrent(&form, user.sub).await?; + let mut payload = form.into_inner(); + payload.displayed_reason = Some(displayed_reason); + arc.pool.remove_torrent(&payload, user.sub).await?;Also applies to: 45-46
backend/api/src/handlers/affiliated_artists/remove_affiliated_artists.rs (3)
13-24: Document the query parameters and correct the success status in OpenAPIExpose the query schema and align the documented status with the handler.
#[utoipa::path( delete, operation_id = "Delete artist affiliation", tag = "Affiliated Artist", path = "/api/affiliated-artists", security( ("http" = ["Bearer"]) ), + params(RemoveAffiliatedArtistsQuery), responses( - (status = 200, description = "Successfully removed the artist affiliations"), + (status = 204, description = "Successfully removed the artist affiliations"), ) )]
26-27: Prefer stable extractor over labs: use web::Queryactix_web’s stable Query extractor is sufficient here.
- query: actix_web_lab::extract::Query<RemoveAffiliatedArtistsQuery>, + query: web::Query<RemoveAffiliatedArtistsQuery>,
1-1: Type naming: consider AuthData instead of AuthdataRust type names are conventionally in CamelCase with word boundaries. Renaming across the crate would improve readability.
backend/api/src/handlers/conversations/create_conversation.rs (1)
29-30: Return Location header for the created resourceHelps clients discover the new resource URI.
- Ok(HttpResponse::Created().json(conversation)) + Ok( + HttpResponse::Created() + .append_header(("Location", format!("/api/conversations/{}", conversation.id))) + .json(conversation), + )backend/api/src/handlers/artists/create_artists.rs (3)
24-27: Avoid variable shadowing for clarityShadowing the input payload name makes the code harder to scan.
- let artists = arc.pool.create_artists(&artists, user.sub).await?; - - Ok(HttpResponse::Created().json(artists)) + let created_artists = arc.pool.create_artists(&artists, user.sub).await?; + + Ok(HttpResponse::Created().json(created_artists))
6-18: Document the request body in OpenAPIMake the input type explicit for better codegen/dev UX.
#[utoipa::path( post, @@ - responses( + request_body = Vec<UserCreatedArtist>, + responses( (status = 201, description = "Successfully created the artists, returned in the same order as the one sent. In the case of a db conflict (duplicate), the existing entry is returned (can be seen with the created_at attribute).", body=Vec<Artist>), ) )]
1-1: Type naming: consider AuthData instead of AuthdataConsistent CamelCase word boundaries improve readability across the API surface.
📜 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 selected for processing (39)
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs(2 hunks)backend/api/src/handlers/affiliated_artists/remove_affiliated_artists.rs(2 hunks)backend/api/src/handlers/artists/create_artists.rs(2 hunks)backend/api/src/handlers/conversations/create_conversation.rs(2 hunks)backend/api/src/handlers/conversations/create_conversation_message.rs(2 hunks)backend/api/src/handlers/conversations/get_conversation.rs(2 hunks)backend/api/src/handlers/edition_groups/create_edition_group.rs(2 hunks)backend/api/src/handlers/forum/create_forum_post.rs(2 hunks)backend/api/src/handlers/forum/create_forum_thread.rs(2 hunks)backend/api/src/handlers/gifts/create_gift.rs(2 hunks)backend/api/src/handlers/invitations/create_invitation.rs(2 hunks)backend/api/src/handlers/master_groups/create_master_group.rs(2 hunks)backend/api/src/handlers/series/create_series.rs(2 hunks)backend/api/src/handlers/subscriptions/create_subscription.rs(2 hunks)backend/api/src/handlers/subscriptions/remove_subscription.rs(2 hunks)backend/api/src/handlers/title_groups/create_title_group.rs(4 hunks)backend/api/src/handlers/title_groups/create_title_group_comment.rs(2 hunks)backend/api/src/handlers/title_groups/edit_title_group.rs(2 hunks)backend/api/src/handlers/title_groups/get_title_group.rs(2 hunks)backend/api/src/handlers/torrent_requests/create_torrent_request.rs(2 hunks)backend/api/src/handlers/torrent_requests/create_torrent_request_vote.rs(2 hunks)backend/api/src/handlers/torrent_requests/fill_torrent_request.rs(2 hunks)backend/api/src/handlers/torrent_requests/get_torrent_request.rs(2 hunks)backend/api/src/handlers/torrents/create_torrent.rs(2 hunks)backend/api/src/handlers/torrents/create_torrent_report.rs(2 hunks)backend/api/src/handlers/torrents/delete_torrent.rs(3 hunks)backend/api/src/handlers/torrents/download_dottorrent_file.rs(2 hunks)backend/api/src/handlers/torrents/edit_torrent.rs(2 hunks)backend/api/src/handlers/torrents/get_registered_torrents.rs(2 hunks)backend/api/src/handlers/torrents/get_upload_information.rs(2 hunks)backend/api/src/handlers/user_applications/update_user_application_status.rs(2 hunks)backend/api/src/handlers/users/create_api_key.rs(2 hunks)backend/api/src/handlers/users/edit_user.rs(2 hunks)backend/api/src/handlers/users/get_me.rs(3 hunks)backend/api/src/handlers/users/get_registered_users.rs(2 hunks)backend/api/src/handlers/users/get_user.rs(3 hunks)backend/api/src/handlers/users/get_user_conversations.rs(2 hunks)backend/api/src/handlers/users/warn_user.rs(2 hunks)backend/api/src/handlers/wiki/create_wiki_article.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (27)
- backend/api/src/handlers/title_groups/create_title_group_comment.rs
- backend/api/src/handlers/gifts/create_gift.rs
- backend/api/src/handlers/conversations/create_conversation_message.rs
- backend/api/src/handlers/master_groups/create_master_group.rs
- backend/api/src/handlers/subscriptions/create_subscription.rs
- backend/api/src/handlers/users/get_registered_users.rs
- backend/api/src/handlers/forum/create_forum_thread.rs
- backend/api/src/handlers/torrents/create_torrent_report.rs
- backend/api/src/handlers/user_applications/update_user_application_status.rs
- backend/api/src/handlers/users/edit_user.rs
- backend/api/src/handlers/torrents/create_torrent.rs
- backend/api/src/handlers/torrents/edit_torrent.rs
- backend/api/src/handlers/wiki/create_wiki_article.rs
- backend/api/src/handlers/users/get_user.rs
- backend/api/src/handlers/torrent_requests/fill_torrent_request.rs
- backend/api/src/handlers/torrents/get_upload_information.rs
- backend/api/src/handlers/torrent_requests/create_torrent_request_vote.rs
- backend/api/src/handlers/subscriptions/remove_subscription.rs
- backend/api/src/handlers/title_groups/get_title_group.rs
- backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs
- backend/api/src/handlers/invitations/create_invitation.rs
- backend/api/src/handlers/torrent_requests/get_torrent_request.rs
- backend/api/src/handlers/torrents/download_dottorrent_file.rs
- backend/api/src/handlers/torrents/get_registered_torrents.rs
- backend/api/src/handlers/torrent_requests/create_torrent_request.rs
- backend/api/src/handlers/users/warn_user.rs
- backend/api/src/handlers/title_groups/create_title_group.rs
🧰 Additional context used
🧬 Code graph analysis (2)
backend/api/src/handlers/users/get_user_conversations.rs (3)
backend/api/src/handlers/affiliated_artists/create_affiliated_artists.rs (1)
exec(18-29)backend/api/src/handlers/conversations/create_conversation.rs (1)
exec(18-30)backend/api/src/handlers/conversations/get_conversation.rs (1)
exec(26-34)
backend/api/src/handlers/users/get_me.rs (3)
backend/api/src/handlers/artists/create_artists.rs (1)
exec(19-27)backend/api/src/handlers/conversations/create_conversation.rs (1)
exec(18-30)backend/api/src/handlers/gifts/create_gift.rs (1)
exec(18-34)
⏰ 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). (4)
- GitHub Check: schema-check
- GitHub Check: build
- GitHub Check: schema-check
- GitHub Check: build
🔇 Additional comments (10)
backend/api/src/handlers/forum/create_forum_post.rs (1)
21-23: Authdata.sub is i64 and matches create_forum_post’s expected i64 parameter; no conversion required.backend/api/src/handlers/users/create_api_key.rs (1)
24-24: Authdata.sub is already an i64 and matches the storage API; no parsing or conversion needed.backend/api/src/handlers/conversations/get_conversation.rs (1)
29-31: Authdata.sub (i64) aligns with find_conversation’s current_user_id parameter (i64); no conversion needed.backend/api/src/handlers/users/get_user_conversations.rs (1)
20-20: No action needed:Authdata.subis correctly typed
Authdata.subis defined asi64injwt_middleware.rsandfind_user_conversationsexpects ani64, so the types already align.backend/api/src/handlers/edition_groups/create_edition_group.rs (1)
23-23: Verify ID type and canonical mapping.Ensure
user.subis the canonical DB user primary key type expected bycreate_edition_group(e.g., i64 vs Uuid). If not guaranteed across auth methods, add an explicit conversion/validation at the handler boundary.backend/api/src/handlers/series/create_series.rs (1)
21-22: Direct Authdata extractor is valid
Authdata has animpl FromRequestin jwt_middleware.rs (line 18), so using it here will yield proper 401/403 on missing or invalid auth.backend/api/src/handlers/users/get_me.rs (2)
1-1: Consider Rust naming convention for AuthData
The codebase consistently defines and imports the type asAuthdata(e.g. inmiddlewares/jwt_middleware.rsand all handler modules). To follow CamelCase for multi-word names, rename the struct and all its references toAuthData. This is a wide-reaching change—confirm the desired canonical name and update every occurrence accordingly.
28-29: Incorrect error propagation suggestion
Bothget_user_peersandfind_user_warningsreturnVec<…>, notResult<…, _>, so you can’t use?here; any errors are handled (or panicked on) inside the repository layer. If you want to propagate database errors, change those methods to returnResult<Vec<…>, sqlx::Error>.Likely an incorrect or invalid review comment.
backend/api/src/handlers/torrents/delete_torrent.rs (1)
46-46: No action needed: AuthData.sub (i64) aligns with storage APIs. Verified thatAuthdata.sub,Claims.sub,find_user_with_id(id: i64), andremove_torrent(..., current_user_id: i64)all usei64.backend/api/src/handlers/conversations/create_conversation.rs (1)
26-27: Types are consistent:Authdata.subisi64andcreate_conversationaccepts ani64, so no changes are needed.
| pub async fn exec( | ||
| artists: web::Json<Vec<UserCreatedArtist>>, | ||
| arc: web::Data<Arcadia>, | ||
| current_user_id: UserId, | ||
| user: Authdata, | ||
| ) -> Result<HttpResponse> { | ||
| let artists = arc.pool.create_artists(&artists, current_user_id.0).await?; | ||
| let artists = arc.pool.create_artists(&artists, user.sub).await?; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add basic payload validation and upper bound
Prevent empty submissions and unbounded bulk requests.
pub async fn exec(
artists: web::Json<Vec<UserCreatedArtist>>,
arc: web::Data<Arcadia>,
user: Authdata,
) -> Result<HttpResponse> {
+ if artists.is_empty() {
+ return Ok(HttpResponse::BadRequest().json(json!({"error": "payload cannot be empty"})));
+ }
+ if artists.len() > 1000 {
+ return Ok(HttpResponse::PayloadTooLarge().json(json!({"error": "too many artists"})));
+ }
let created_artists = arc.pool.create_artists(&artists, user.sub).await?;
Ok(HttpResponse::Created().json(created_artists))
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/api/src/handlers/artists/create_artists.rs around lines 19 to 25, add
basic payload validation before calling arc.pool.create_artists: check that the
incoming artists list is not empty (return an HTTP 400 Bad Request with a clear
message if it is), enforce an upper bound (define a MAX_ARTISTS constant, e.g.
100, and return 400 if artists.len() > MAX_ARTISTS), and perform simple per-item
validation (e.g. required fields like name are non-empty) returning 400 on the
first invalid item; only call arc.pool.create_artists when validations pass.
| user: Authdata, | ||
| ) -> Result<HttpResponse> { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Param rename looks good; confirm extractor type used here.
If you adopt the AuthData rename, mirror it in the signature:
- user: Authdata,
+ user: AuthData,To verify types and usages, run:
🏁 Script executed:
#!/bin/bash
# Confirm Authdata/AuthData definition and `sub` field type
rg -nP -C3 'struct\s+Authdata\b|struct\s+AuthData\b'
rg -nP -C2 '\bsub\s*:\s*' | head
# Confirm storage API signature
rg -nP -C3 'fn\s+create_forum_post\s*\('Length of output: 1811
Rename Authdata to AuthData in both definition and usage
- backend/api/src/middlewares/jwt_middleware.rs:13 change
pub struct Authdata→pub struct AuthData - backend/api/src/handlers/forum/create_forum_post.rs:21 update
user: Authdata→user: AuthData
🤖 Prompt for AI Agents
In backend/api/src/middlewares/jwt_middleware.rs around line 13 and
backend/api/src/handlers/forum/create_forum_post.rs around lines 21-22, the
struct name `Authdata` should be renamed to `AuthData` where defined and
referenced; rename the pub struct declaration to `AuthData`, update any
derives/attributes if present, then update the handler signature from `user:
Authdata` to `user: AuthData` and fix any other references/imports across the
codebase (adjust use paths if needed) and run cargo check to ensure no remaining
references to the old name.
| let forum_post = arc.pool.create_forum_post(&forum_post, user.sub).await?; | ||
|
|
||
| Ok(HttpResponse::Created().json(forum_post)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Decouple storage from actix’s web::Json and avoid shadowing.
Pass the inner model to storage and use a distinct binding name.
- let forum_post = arc.pool.create_forum_post(&forum_post, user.sub).await?;
-
- Ok(HttpResponse::Created().json(forum_post))
+ let created = arc.pool.create_forum_post(&forum_post.into_inner(), user.sub).await?;
+ Ok(HttpResponse::Created().json(created))If create_forum_post takes the value (not a reference), drop the &:
- let created = arc.pool.create_forum_post(&forum_post.into_inner(), user.sub).await?;
+ let created = arc.pool.create_forum_post(forum_post.into_inner(), user.sub).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 forum_post = arc.pool.create_forum_post(&forum_post, user.sub).await?; | |
| Ok(HttpResponse::Created().json(forum_post)) | |
| let created = arc.pool.create_forum_post(&forum_post.into_inner(), user.sub).await?; | |
| Ok(HttpResponse::Created().json(created)) |
🤖 Prompt for AI Agents
In backend/api/src/handlers/forum/create_forum_post.rs around lines 23 to 25,
the handler currently passes the actix web::Json wrapper and reuses the same
name for the storage result; extract the inner model from the web::Json (e.g.,
let new_post = forum_post.into_inner();), pass that value to
arc.pool.create_forum_post (remove the & if the function takes ownership), bind
the storage result to a distinct name (e.g., created_post) and return that in
the HttpResponse::Created().json(created_post) to avoid shadowing and decouple
storage from actix types.
| user: Authdata, | ||
| ) -> Result<HttpResponse> { | ||
| let series = arc.pool.create_series(&serie, ¤t_user).await?; | ||
| let series = arc.pool.create_series(&serie, user.sub).await?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid “stringly-typed” IDs: pass a typed user_id instead of raw sub
user.sub is typically a string. Prefer a typed ID (e.g., Uuid/i64) at the middleware boundary and expose it as user.user_id. This prevents accidental schema drift and parse failures deep in storage.
Once you confirm the intended ID type, I can provide a precise patch. Example directions:
- If Uuid: parse in middleware and expose
Uuidin Authdata; then callcreate_series(&serie, user.user_id). - If i64: parse once in middleware; same call as above.
| pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> { | ||
| let conversations = arc.pool.find_user_conversations(user.sub).await?; | ||
|
|
||
| Ok(HttpResponse::Ok().json(json!({"conversations": conversations}))) |
There was a problem hiding this comment.
OpenAPI schema mismatch: handler returns a wrapper object but schema advertises raw ConversationsOverview
You return {"conversations": ...} but the utoipa response declares body=ConversationsOverview. This will generate incorrect API docs/clients.
Apply:
+#[derive(serde::Serialize, utoipa::ToSchema)]
+struct GetUserConversationsResponse {
+ conversations: ConversationsOverview,
+}
+
-pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> {
+pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> {
let conversations = arc.pool.find_user_conversations(user.sub).await?;
-
- Ok(HttpResponse::Ok().json(json!({"conversations": conversations})))
+ Ok(HttpResponse::Ok().json(GetUserConversationsResponse { conversations }))
}And outside the selected lines, update the docs and remove the now-unused import:
- responses(
- (status = 200, description = "Found the conversations and some of their metadata", body=ConversationsOverview),
- )
+ responses(
+ (status = 200, description = "Found the conversations and some of their metadata", body=GetUserConversationsResponse),
+ (status = 401, description = "Unauthorized"),
+ )-use serde_json::json;📝 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.
| pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> { | |
| let conversations = arc.pool.find_user_conversations(user.sub).await?; | |
| Ok(HttpResponse::Ok().json(json!({"conversations": conversations}))) | |
| // Add at top of file, alongside other response types: | |
| #[derive(serde::Serialize, utoipa::ToSchema)] | |
| struct GetUserConversationsResponse { | |
| conversations: ConversationsOverview, | |
| } | |
| pub async fn exec(arc: web::Data<Arcadia>, user: Authdata) -> Result<HttpResponse> { | |
| let conversations = arc.pool.find_user_conversations(user.sub).await?; | |
| Ok(HttpResponse::Ok().json(GetUserConversationsResponse { conversations })) | |
| } |
🤖 Prompt for AI Agents
In backend/api/src/handlers/users/get_user_conversations.rs around lines 19 to
22, the handler currently returns a wrapper object {"conversations":
conversations} which conflicts with the OpenAPI/utoipa declaration
body=ConversationsOverview; change the response to return the raw
ConversationsOverview value (i.e., respond with the conversations directly, not
wrapped), update any utoipa docs elsewhere to match this concrete return shape,
and remove any now-unused import(s) that were only needed for the wrapper.
|
The code at the moment has at least 2 calls on every request, one to check if use is banned and another to fetch the user from the db. Both queries are avoidable. With the two PRs we got it to 0 db requests while maintain security. Invalidating a token (you can check the other PR) requires no interactions with the db. Instead we utilise an in memory cache (Redis) and a simple check which verifies if token is invalidated. |
|
|
||
| pub async fn find_user_id_with_api_key(&self, api_key: &str) -> Result<i64> { | ||
| let user_id = sqlx::query_scalar!( | ||
| pub async fn find_user_id_with_api_key(&self, api_key: &str) -> Result<User> { |
There was a problem hiding this comment.
this should be renamed to find_user_with_api_key then ?
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/storage/src/repositories/auth_repository.rs (2)
149-195: Retry loop reuses an aborted transaction after a unique violationOn a 23505, the transaction is left in an aborted state; continuing the loop with the same tx will fail with “current transaction is aborted.” Either start a new transaction per attempt or (simpler) drop the transaction entirely for this single-row insert.
Minimal fix: remove the transaction and retry on collision.
- let mut tx = <ConnectionPool as Borrow<PgPool>>::borrow(self) - .begin() - .await?; - loop { - let api_key: String = Alphanumeric.sample_string(&mut rng(), 40); + let candidate: String = Alphanumeric.sample_string(&mut rng(), 40); - let api_key = sqlx::query_as!( + let api_key = sqlx::query_as!( APIKey, r#" INSERT INTO api_keys (name, value, user_id) VALUES ($1, $2, $3) RETURNING * "#, created_api_key.name, - api_key, + candidate, current_user_id ) - .fetch_one(&mut *tx) + .fetch_one(self.borrow()) .await; match api_key { - Ok(api_key) => { - tx.commit().await?; - - return Ok(api_key); - } + Ok(api_key) => return Ok(api_key), Err(api_key_error) => { - return Err(match &api_key_error { - sqlx::Error::Database(database_error) => { - let code = database_error.code(); - // 23505 is the code for "unique violation", which means we didn't generate a unique API key - if let Some(code) = code - && code == "23505" - { - // Try again (jump to next iteration of loop) - continue; - } - - Error::CouldNotCreateAPIKey(api_key_error) - } - _ => Error::CouldNotCreateAPIKey(api_key_error), - }); + if let sqlx::Error::Database(db_err) = &api_key_error { + // 23505: unique violation – regenerate and retry + if db_err.code().as_deref() == Some("23505") { + continue; + } + } + return Err(Error::CouldNotCreateAPIKey(api_key_error)); } } }Optionally cap retries (e.g., 5) and surface a specific error if exhausted.
103-107: Avoid unwrap on parsed password hashPasswordHash::new can fail on malformed hashes and currently panics. Map errors to WrongUsernameOrPassword.
- let parsed_hash = PasswordHash::new(&user.password_hash); - - Argon2::default() - .verify_password(login.password.as_bytes(), &parsed_hash.unwrap()) - .map_err(|_| Error::WrongUsernameOrPassword)?; + let parsed_hash = PasswordHash::new(&user.password_hash) + .map_err(|_| Error::WrongUsernameOrPassword)?; + + Argon2::default() + .verify_password(login.password.as_bytes(), &parsed_hash) + .map_err(|_| Error::WrongUsernameOrPassword)?;
♻️ Duplicate comments (1)
backend/storage/src/repositories/auth_repository.rs (1)
112-128: Rename and return type change look goodRenaming to find_user_with_api_key and returning User removes an extra round-trip (no follow-up find_user_with_id) and aligns with the prior naming feedback.
🧹 Nitpick comments (1)
backend/storage/src/repositories/auth_repository.rs (1)
116-121: Sanitize auth query to avoid leaking sensitive fields
ReplaceSELECT u.*with an explicit projection into a minimal auth-specific struct (e.g.AuthUser { id, class, username, … }) so thatpassword_hashand other restricted fields don’t flow across layers.
Theapi_keystable has no revoked/active flag—if you require key revocation support, add a status column (e.g.revoked BOOLEAN) and include it in theWHEREclause.
📜 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 selected for processing (4)
backend/api/.env.example(1 hunks)backend/api/src/middlewares/jwt_middleware.rs(3 hunks)backend/storage/.env.example(1 hunks)backend/storage/src/repositories/auth_repository.rs(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/api/.env.example
- backend/storage/.env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/api/src/middlewares/jwt_middleware.rs
🧰 Additional context used
🧬 Code graph analysis (1)
backend/storage/src/repositories/auth_repository.rs (3)
backend/storage/src/repositories/title_group_repository.rs (1)
sqlx(36-36)backend/storage/src/repositories/torrent_request_repository.rs (1)
sqlx(30-30)backend/storage/src/repositories/torrent_repository.rs (1)
sqlx(102-102)
⏰ 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). (4)
- GitHub Check: schema-check
- GitHub Check: build
- GitHub Check: schema-check
- GitHub Check: build
2 participants
JwtAuthDatastruct with user fields that are used by the majority of the endpointsSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores