Platform apis #13
Platform apis #13
Conversation
WalkthroughThis pull request introduces a platform email verification feature. It adds database migrations to create a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebServer
participant DB
participant VerificationModel
Client->>WebServer: Registration or verification request
WebServer->>DB: Invoke create_platform_email_verification()
DB->>VerificationModel: Insert new verification record
VerificationModel-->>DB: Record inserted
DB-->>WebServer: Return verification record
WebServer-->>Client: Send response (verification details)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 100000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
AnthonyRonning
force-pushed
the
platform-apis
branch
from
8373426 to
bcacfe6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/web/platform/login_routes.rs (1)
196-204: Consider enhancing error handling with more specific error types.The error handling could be more specific to help with debugging and user feedback. Consider mapping database errors to specific API errors instead of always returning InternalServerError.
let verification = match data.db.create_platform_email_verification(new_verification) { Ok(v) => v, Err(e) => { error!("Error creating platform email verification: {:?}", e); - return Err(ApiError::InternalServerError); + return match e { + DBError::DuplicateKey => Err(ApiError::EmailAlreadyExists), + DBError::PlatformEmailVerificationError(_) => Err(ApiError::BadRequest), + _ => { + error!("Unexpected database error: {:?}", e); + Err(ApiError::InternalServerError) + } + }; } };src/main.rs (2)
1081-1084: Avoid logging potentially sensitive user data.
You are logging the exact user email in the warning. This can reveal PII in logs. Consider anonymizing or removing the email from the log message to protect user privacy.Possible fix:
- warn!("Could not find platform user by email: {email}"); + warn!("Could not find platform user by email.");
1093-1110: Use secure memory handling for decrypted password hash.
Currently, the decrypted password hash is kept in memory, which might pose a security risk if the process or logs are compromised. Consider zeroizing or clearing the buffer after verification to minimize exposure.Example approach for secure memory cleanup (pseudocode):
let decrypted_password_hash = String::from_utf8(decrypted_password_bytes) .map_err(|e| Error::EncryptionError(format!("Failed to decode UTF-8: {}", e)))?; let result = task::spawn_blocking(move || verify_password(password, &decrypted_password_hash)).await?; // Attempt to clear the memory holding the password zeroize::Zeroizing::new(decrypted_password_hash);src/db.rs (1)
324-359: New trait methods for platform email verifications appear coherent.
The CRUD and verification methods enable comprehensive handling of email verification state. Consider these points:
- Ensure these verification records are indexed properly in the database for best performance.
- Validate that the single “by_platform_user_id” record assumption aligns with business requirements. If multiple records per user are possible, we may need a different approach (e.g., retrieving a list).
Example suggestions:
- Index: Confirm there's an index on
platform_user_idandverification_codeto speed up lookups.- Multiplicity: If users can trigger multiple verifications concurrently, consider returning all. Otherwise, a single record approach is fine.
🛑 Comments failed to post (3)
src/web/platform/login_routes.rs (1)
4-7:
⚠️ Potential issueFix formatting issues.
The CI pipeline indicates formatting issues. Run
cargo fmtto fix the code style.src/web/platform/org_routes.rs (2)
459-474: 🛠️ Refactor suggestion
Redundant pattern matching warning at line 460.
Instead ofif let Some(_) = ..., use.is_some()to align with the Rust CI suggestion.-if let Some(_) = data +if data .db .get_org_project_by_name_and_org(&create_request.name, org.id) .map_err(|e| { error!("Failed to check for existing project: {:?}", e); ApiError::InternalServerError })? -{ +.is_some() {📝 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.// Check if a project with the same name already exists in this organization if data .db .get_org_project_by_name_and_org(&create_request.name, org.id) .map_err(|e| { error!("Failed to check for existing project: {:?}", e); ApiError::InternalServerError })? .is_some() { // Project with this name already exists error!( "Project with name '{}' already exists in this organization", &create_request.name ); return Err(ApiError::BadRequest); }🧰 Tools
🪛 GitHub Actions: Rust CI
[error] 460-460: redundant pattern matching, consider using
is_some()
575-596: 🛠️ Refactor suggestion
Redundant pattern matching warning at line 580.
Similar to the prior fix; replaceif let Some(_) = ...with.is_some().-if let Some(_) = data +if data .db .get_org_project_by_name_and_org(&name, org.id) .map_err(|e| { error!("Failed to check for existing project: {:?}", e); ApiError::InternalServerError })? -{ +.is_some() {📝 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.// Update the project let mut updated_project = project; if let Some(name) = update_request.name { // If name is changing, check if it conflicts with an existing project if name != updated_project.name { if data .db .get_org_project_by_name_and_org(&name, org.id) .map_err(|e| { error!("Failed to check for existing project: {:?}", e); ApiError::InternalServerError })? .is_some() { // Project with this name already exists error!( "Project with name '{}' already exists in this organization", &name ); return Err(ApiError::BadRequest); } } updated_project.name = name;🧰 Tools
🪛 GitHub Actions: Rust CI
[error] 580-580: redundant pattern matching, consider using
is_some()
AnthonyRonning
force-pushed
the
platform-apis
branch
from
bcacfe6 to
ce42947
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/main.rs (2)
1081-1084: Be cautious about logging user emails in warnings.
While logging is helpful for debugging, logging user email addresses may leak PII. Consider masking or removing it to protect user privacy, especially in production.
1978-1982: New platform routes re-enabled.
Uncommenting these lines successfully restores the platform login and org routes withvalidate_platform_jwt. Be sure to add integration tests to confirm functionality.src/db.rs (2)
85-88: Added DBError variants for platform email verification.
Expanding theDBErrorenum with verification-specific variants helps improve error granularity. Confirm that all newly introduced error branches are tested and handled across the codebase.
324-359: Trait extension for platform email verification methods.
Adding these CRUD and “verify” methods for emails clarifies the workflow for platform-level verification. Verify that transaction boundaries and concurrency edge cases (e.g., multiple calls to the same code) are safely handled.src/models/platform_email_verification.rs (2)
18-28: Data model forPlatformEmailVerification.
Storing timestamps as UTC and including anexpires_atfield is a good design. The attribute metadata aligns with Diesel's macros. Ensure indexes are present to optimize frequent lookups (especially byplatform_user_idorverification_code).
104-133: Constructor and insert method for new verifications.
Auto-generating theverification_codeviaUuid::new_v4()is straightforward. The code is cohesive and easy to maintain. Consider logging or metering how often verifications are requested to detect spam or repeated attempts.
🛑 Comments failed to post (2)
src/web/platform/org_routes.rs (2)
939-965: 🛠️ Refactor suggestion
Extract common validation logic into a helper function.
There's significant code duplication in project-related routes for validating org/project relationships and user permissions. Consider extracting this into a helper function.
Here's a suggested implementation:
/// Helper function to validate org, project, and user permissions async fn validate_project_access( db: &impl DBConnection, org_id: Uuid, project_id: Option<Uuid>, platform_user_id: Uuid, require_admin: bool, ) -> Result<(Org, Option<OrgProject>), ApiError> { // Get org by UUID let org = db.get_org_by_uuid(org_id) .map_err(|_| ApiError::NotFound)?; // Get project if ID provided let project = if let Some(project_id) = project_id { let project = db.get_org_project_by_uuid(project_id) .map_err(|_| ApiError::NotFound)?; // Ensure project belongs to org if project.org_id != org.id { error!("Project does not belong to organization"); return Err(ApiError::NotFound); } Some(project) } else { None }; // Verify user permissions let membership = db .get_org_membership_by_platform_user_and_org(platform_user_id, org.id) .map_err(|_| ApiError::Unauthorized)?; if require_admin { let role: OrgRole = membership.role.into(); if !matches!(role, OrgRole::Owner | OrgRole::Admin) { return Err(ApiError::Unauthorized); } } Ok((org, project)) }Usage example:
async fn create_secret( State(data): State<Arc<AppState>>, Extension(platform_user): Extension<PlatformUser>, Path((org_id, project_id)): Path<(Uuid, Uuid)>, Extension(create_request): Extension<CreateSecretRequest>, Extension(session_id): Extension<Uuid>, ) -> Result<Json<EncryptedResponse<SecretResponse>>, ApiError> { debug!("Creating project secret"); // Validate request if let Err(errors) = create_request.validate() { error!("Validation error: {:?}", errors); return Err(ApiError::BadRequest); } // Validate access with admin requirement let (org, project) = validate_project_access( &*data.db, org_id, Some(project_id), platform_user.uuid, true ).await?; // Rest of the function... }This refactor will:
- Reduce code duplication
- Centralize access control logic
- Make the code more maintainable
- Make it easier to modify the validation logic in one place
Also applies to: 1011-1033, 1063-1090, 1125-1147, 1185-1212, 1245-1267, 1295-1322, 1352-1374, 1399-1426
460-474:
⚠️ Potential issueFix redundant pattern matching.
Replace
if let Some(_)withis_some()to fix the pipeline failures and make the code more idiomatic.Apply this diff:
- if let Some(_) = data + if data .db .get_org_project_by_name_and_org(&create_request.name, org.id) .map_err(|e| { error!("Failed to check for existing project: {:?}", e); ApiError::InternalServerError - })? + })?.is_some() { error!( "Project with name '{}' already exists in this organization", &create_request.name ); return Err(ApiError::BadRequest); } - if let Some(_) = data + if data .db .get_org_project_by_name_and_org(&name, org.id) .map_err(|e| { error!("Failed to check for existing project: {:?}", e); ApiError::InternalServerError - })? + })?.is_some() { error!( "Project with name '{}' already exists in this organization", &name ); return Err(ApiError::BadRequest); }Also applies to: 580-595
🧰 Tools
🪛 GitHub Actions: Rust CI
[error] 460-460: redundant pattern matching, consider using
is_some()
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/web/platform/org_routes.rs (6)
319-425: Enhance error handling in organization routes.Consider adding more specific error messages in the error logs to help with debugging. For example:
- error!("Failed to create organization with owner: {:?}", e); + error!("Failed to create organization '{}' with owner {}: {:?}", create_request.name, platform_user.uuid, e);
459-475: Consider using a database constraint for project name uniqueness.The current implementation checks for project name uniqueness using a separate query. Consider adding a unique constraint in the database for
(org_id, name)to enforce this at the database level.
476-488: Enhance error logging in project creation.Add more context to error messages to help with debugging.
- error!("Failed to create project: {:?}", e); + error!("Failed to create project '{}' in organization {}: {:?}", new_project.name, org.id, e);
924-1001: Consider additional validation for secret key names.While
validate_alphanumeric_onlyis used, consider adding more specific validation rules for secret key names:
- Prevent common sensitive words in key names (e.g., "password", "token", "key").
- Add a regex pattern to enforce a specific format.
Example implementation:
fn validate_secret_key_name(key_name: &str) -> Result<(), validator::ValidationError> { let sensitive_words = ["password", "token", "key", "secret"]; if sensitive_words.iter().any(|word| key_name.to_lowercase().contains(word)) { return Err(validator::ValidationError::new("sensitive_key_name")); } // Only allow letters, numbers, and underscores, must start with a letter let key_pattern = regex::Regex::new(r"^[a-zA-Z][a-zA-Z0-9_]*$").unwrap(); if !key_pattern.is_match(key_name) { return Err(validator::ValidationError::new("invalid_key_name_format")); } Ok(()) }
1176-1235: Add rate limiting for settings updates.Consider implementing rate limiting for settings updates to prevent abuse.
You could use a rate limiting middleware or implement it directly in the handler:
use tokio::time::{Duration, Instant}; use std::collections::HashMap; use std::sync::Mutex; struct RateLimiter { last_update: HashMap<(Uuid, String), Instant>, min_interval: Duration, } impl RateLimiter { fn new(min_interval: Duration) -> Self { Self { last_update: HashMap::new(), min_interval, } } fn can_update(&mut self, project_id: Uuid, category: &str) -> bool { let now = Instant::now(); let key = (project_id, category.to_string()); if let Some(last) = self.last_update.get(&key) { if now.duration_since(*last) < self.min_interval { return false; } } self.last_update.insert(key, now); true } }
153-160: Enhance email verification URL validation.Add more specific validation for the email verification URL format and security.
fn validate_verification_url(url: &str) -> Result<(), validator::ValidationError> { // Parse URL let parsed_url = url::Url::parse(url).map_err(|_| { validator::ValidationError::new("invalid_url") })?; // Ensure HTTPS if parsed_url.scheme() != "https" { return Err(validator::ValidationError::new("non_https_url")); } // Validate URL pattern if !url.contains("{token}") { return Err(validator::ValidationError::new("missing_token_placeholder")); } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/web/platform/org_routes.rs(38 hunks)
🧰 Additional context used
🪛 GitHub Actions: Rust CI
src/web/platform/org_routes.rs
[error] 577-577: Formatting check failed. Please run 'cargo fmt' to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (1)
src/web/platform/org_routes.rs (1)
107-146: LGTM! Response structs successfully migrated to UUIDs.The changes to use
Uuidtype in response structs are consistent and align well with the broader transition from integer IDs to UUIDs.
AnthonyRonning
force-pushed
the
platform-apis
branch
from
ce42947 to
4126438
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/web/platform/org_routes.rs (1)
1018-1027: Enhance error messages for better debugging.While the error handling is thorough, consider making the error messages more descriptive by including the UUIDs in the messages. For example:
- error!("Project not found"); + error!("Project with UUID {} not found", project_id); - error!("Project does not belong to organization"); + error!("Project {} does not belong to organization {}", project_id, org_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/web/platform/org_routes.rs(38 hunks)
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (2)
src/web/platform/org_routes.rs (2)
108-126: Clean transition to UUIDs in response structs!The response structs have been updated to use UUIDs instead of integer IDs, which is a good practice for globally unique identifiers.
427-498: Good addition of duplicate project name validation!The create_project handler has been enhanced with:
- Proper UUID parameter handling
- New validation to prevent duplicate project names within an organization
- Clear error messages for better debugging
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/web/platform/org_routes.rs (3)
149-156: Enhance email provider validation.Consider these improvements to make the validation more robust and maintainable:
- Use case-insensitive comparison for better user experience
- Define a constant for the provider name to avoid magic strings
Apply this diff to implement the suggested improvements:
+const RESEND_PROVIDER: &str = "resend"; fn validate_email_provider(provider: &str) -> Result<(), validator::ValidationError> { - if provider != "resend" { + if provider.to_lowercase() != RESEND_PROVIDER { let mut error = validator::ValidationError::new("invalid_email_provider"); - error.message = Some("Only 'resend' is supported as an email provider".into()); + error.message = Some(format!("Only '{}' is supported as an email provider", RESEND_PROVIDER).into()); return Err(error); } Ok(()) }
1264-1273: Extract OAuth validation to a separate function.The OAuth settings validation logic could be moved to a separate function to improve readability and maintainability.
Apply this diff to extract the validation:
+fn validate_oauth_settings_consistency( + update_request: &UpdateOAuthSettingsRequest, +) -> Result<(), ApiError> { + if update_request.google_oauth_enabled && update_request.google_oauth_settings.is_none() { + error!("Google OAuth settings must be provided when Google OAuth is enabled"); + return Err(ApiError::BadRequest); + } + + if update_request.github_oauth_enabled && update_request.github_oauth_settings.is_none() { + error!("GitHub OAuth settings must be provided when GitHub OAuth is enabled"); + return Err(ApiError::BadRequest); + } + + Ok(()) +} async fn update_oauth_settings( State(data): State<Arc<AppState>>, Extension(platform_user): Extension<PlatformUser>, Path((org_id, project_id)): Path<(Uuid, Uuid)>, Extension(update_request): Extension<UpdateOAuthSettingsRequest>, Extension(session_id): Extension<Uuid>, ) -> Result<Json<EncryptedResponse<OAuthSettings>>, ApiError> { debug!("Updating project OAuth settings"); // Validate request using validator if let Err(errors) = update_request.validate() { error!("Validation error: {:?}", errors); return Err(ApiError::BadRequest); } - // Additional validation for the relationship between enabled flags and settings - if update_request.google_oauth_enabled && update_request.google_oauth_settings.is_none() { - error!("Google OAuth settings must be provided when Google OAuth is enabled"); - return Err(ApiError::BadRequest); - } - - if update_request.github_oauth_enabled && update_request.github_oauth_settings.is_none() { - error!("GitHub OAuth settings must be provided when GitHub OAuth is enabled"); - return Err(ApiError::BadRequest); - } + validate_oauth_settings_consistency(&update_request)?;
934-939: Extract project ownership validation to a helper function.The project ownership validation is duplicated across multiple handlers. Consider extracting it to a helper function to reduce code duplication.
Apply this diff to create a helper function:
+async fn validate_project_ownership( + data: &AppState, + org_id: Uuid, + project_id: Uuid, +) -> Result<(), ApiError> { + let org = data + .db + .get_org_by_uuid(org_id) + .map_err(|_| ApiError::NotFound)?; + + let project = data.db.get_org_project_by_uuid(project_id).map_err(|_| { + error!("Project not found"); + ApiError::NotFound + })?; + + if project.org_id != org.id { + error!("Project does not belong to organization"); + return Err(ApiError::NotFound); + } + + Ok(()) +}Then use it in the handlers:
- // Ensure project belongs to org - if project.org_id != org.id { - error!("Project does not belong to organization"); - return Err(ApiError::NotFound); - } + validate_project_ownership(&data, org_id, project_id).await?;Also applies to: 1059-1064, 1171-1176, 1286-1291
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/web/platform/org_routes.rs(35 hunks)
⏰ Context from checks skipped due to timeout of 100000ms (2)
- GitHub Check: Test Suite
- GitHub Check: Development Reproducible Build
🔇 Additional comments (1)
src/web/platform/org_routes.rs (1)
106-137: LGTM! Response structs updated to use UUIDs.The response structs have been correctly updated to use
Uuidinstead ofi32for identifiers, aligning with the transition to UUID-based identification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/models/org_memberships.rs (2)
157-166: Consider the performance implications of loading all records instead of using a count query.The previous implementation may have used a direct count query, while the updated code loads all owner memberships and then counts them. This approach could be less efficient for organizations with many owners, as it retrieves complete records instead of just the count.
Consider using
diesel::dsl::countor similar if performance for organizations with many owners is a concern:- // First, get all owner memberships with FOR UPDATE lock - let owner_memberships = org_memberships::table .filter(org_memberships::org_id.eq(membership.org_id)) .filter(org_memberships::role.eq(OrgRole::Owner.as_str())) .for_update() // This locks the rows .load::<OrgMembership>(conn)?; - // Then count them - let owner_count = owner_memberships.len() as i64; + // Get count of owner memberships with FOR UPDATE lock + let owner_count = org_memberships::table + .filter(org_memberships::org_id.eq(membership.org_id)) + .filter(org_memberships::role.eq(OrgRole::Owner.as_str())) + .for_update() // This locks the rows + .count() + .get_result::<i64>(conn)?;
196-205: Consider using consistent type comparison instead of conversion.The conversion from
usizetoi64might be unnecessary. Since we're comparing with a constant value (1), we could adjust the comparison value to match the type oflen()which isusize.// First, get all owner memberships with FOR UPDATE lock let owner_memberships = org_memberships::table .filter(org_memberships::org_id.eq(membership.org_id)) .filter(org_memberships::role.eq(OrgRole::Owner.as_str())) .for_update() // This locks the rows .load::<OrgMembership>(conn)?; // Then count them - let owner_count = owner_memberships.len() as i64; + let owner_count = owner_memberships.len(); - if owner_count <= 1 { + if owner_count <= 1_usize {Alternatively, the same issue exists in the
update_role_with_owner_checkmethod and should be addressed there as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pcrDev.jsonis excluded by!pcrDev.jsonpcrProd.jsonis excluded by!pcrProd.json
📒 Files selected for processing (1)
src/models/org_memberships.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (1)
src/models/org_memberships.rs (1)
157-166: Document why this approach was chosen over a direct count query.The change from what was likely a direct count query to loading all records and then counting them isn't accompanied by any comments explaining the rationale. Given that this pull request focuses on platform email verification features, it would be helpful to understand how this specific change relates to that objective.
Consider adding a comment to explain why this approach was chosen over a potentially more efficient count query:
+ // We load all owner memberships rather than using count() because...Also applies to: 196-205
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/web/platform/org_routes.rs (1)
1346-1416: Implementation of user profile endpoint with organization data.The
get_platform_userfunction provides a comprehensive user profile with email verification status and organizations.Note that this implementation makes multiple database calls (verification check, memberships, and one per organization), which could potentially be optimized if performance becomes a concern with a large number of organizations.
Consider optimizing the database queries to reduce the number of round trips, particularly for users with many organizations. This could be done by joining the relevant tables or implementing a more efficient query pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/jwt.rs(0 hunks)src/web/platform/login_routes.rs(4 hunks)src/web/platform/org_routes.rs(37 hunks)
💤 Files with no reviewable changes (1)
- src/jwt.rs
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (18)
src/web/platform/login_routes.rs (5)
4-7: Import path updated to use platform-specific modules.The import has been updated to use platform-specific modules, which aligns with the transition to platform-focused email verification.
74-94: New data structures for platform organization and project management.Good addition of
PlatformOrgandPlatformProjectstructs that use UUIDs and include comprehensive fields for platform entities. These structures support the broader transition from integer IDs to UUIDs and provide a consistent API surface.
219-221: Platform-specific email verification implementation.The code now uses
NewPlatformEmailVerificationinstead of the previous generic implementation, which is consistent with the platform APIs focus of this PR.
224-225: Enhanced error logging specificity.Error logging has been improved to specifically mention "platform email verification," which enhances debugging clarity.
277-278: More specific token validation context.The token validation now uses "platform_refresh" as the context, making the validation more specific and aligned with the platform-focused approach.
src/web/platform/org_routes.rs (13)
108-110: Transition from i32 to UUID for organization ID.The organization response structure now uses UUID instead of integer ID, which is consistent with the broader transition to UUIDs throughout the codebase.
114-119: Project response structure updated to use UUID.The project response structure has been updated to use UUID as the primary identifier, which aligns with the transition to UUIDs throughout the API.
139-147: New platform user response structure with email verification status.This new structure provides a comprehensive representation of a platform user, including email verification status, which is valuable for client applications.
149-153: New response structure for user profile with organizations.The
MeResponsestructure encapsulates both user information and their organizations, providing a convenient way to fetch this related data in a single request.
158-159: Email provider validation has been added.A new validation constraint has been added to ensure only compatible email providers are used.
165-172: Restrictive email provider validation logic.The validation function only allows "resend" as a valid provider. While this is likely intentional if "resend" is the only supported provider, it creates a hard constraint that might require code changes if additional providers need to be supported in the future.
Is there a strategic reason to only support "resend" as an email provider, or would it be better to design for future extensibility?
262-263: Comment clarification for project settings routes.The comment has been updated to clarify that only specialized endpoints are kept, which helps future developers understand the intent behind the API design.
316-320: New endpoint to retrieve platform user information.Great addition of a
/platform/meendpoint that provides a comprehensive view of the user's profile and associated organizations, which is a common requirement for platform applications.
403-408: Updated org retrieval to use UUID.The code now retrieves an organization by UUID instead of ID, which is consistent with the transition to UUIDs throughout the API.
447-452: Organization lookup now uses UUID.The code has been updated to use UUID-based lookup for organizations, which aligns with the broader transition to UUIDs.
464-480: Added project name uniqueness check within an organization.This is a good addition that prevents duplicate project names within the same organization, enhancing data integrity.
1354-1364: Email verification status check with explicit error handling.Good implementation of email verification status check with proper error handling. The code differentiates between the case where verification doesn't exist (returns false) and other database errors (returns an internal server error).
582-582: Code formatting.The indentation and formatting in this section appear correct now, consistent with Rust formatting standards.
AnthonyRonning
force-pushed
the
platform-apis
branch
from
493d51a to
846f2a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/web/platform/org_routes.rs (1)
1350-1420: New endpoint provides comprehensive user profile data.The implementation correctly retrieves the user's profile information, email verification status, and organization memberships in a single endpoint, which is a good API design pattern.
Consider optimizing the database access pattern:
- The code makes separate database calls for each organization in a loop (lines 1388-1394)
- For users with many organizations, this could become a performance bottleneck
- // Create the list of organizations with roles - let mut organizations = Vec::new(); - for membership in memberships { - let org_id = membership.org_id; - - // Fetch the organization details - let org = match data.db.get_org_by_id(org_id) { - Ok(org) => org, - Err(e) => { - error!("Error fetching organization {}: {:?}", org_id, e); - continue; // Skip this organization but continue processing others - } - }; - - // Add to our list of organizations - organizations.push(OrgResponse { - id: org.uuid, - name: org.name, - }); - } + // Extract all org IDs from memberships + let org_ids: Vec<i32> = memberships.iter().map(|m| m.org_id).collect(); + + // Fetch all organizations in bulk if possible + let orgs = match data.db.get_orgs_by_ids(&org_ids) { + Ok(orgs) => orgs, + Err(e) => { + error!("Error fetching organizations: {:?}", e); + return Err(ApiError::InternalServerError); + } + }; + + // Create response objects + let organizations: Vec<OrgResponse> = orgs + .into_iter() + .map(|org| OrgResponse { + id: org.uuid, + name: org.name, + }) + .collect();Note: This suggestion assumes you can implement a
get_orgs_by_idsmethod in your database layer that fetches multiple organizations in a single query.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pcrDev.jsonis excluded by!pcrDev.jsonpcrProd.jsonis excluded by!pcrProd.json
📒 Files selected for processing (3)
src/jwt.rs(0 hunks)src/web/platform/login_routes.rs(4 hunks)src/web/platform/org_routes.rs(36 hunks)
💤 Files with no reviewable changes (1)
- src/jwt.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/web/platform/login_routes.rs
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (4)
src/web/platform/org_routes.rs (4)
166-173: Good addition of email provider validation.This validator ensures only 'resend' is accepted as an email provider, which maintains system compatibility with supported services.
263-263: Clear documentation of route changes.The comment effectively communicates that only specialized endpoints are being maintained for project settings.
1362-1368: Explicit handling of email verification not found.The code appropriately handles the specific
PlatformEmailVerificationNotFounderror case by settingemail_verifiedtofalse, which is a logical default.
1404-1411: Comprehensive user response model.The
PlatformUserResponseincludes all necessary user information including the importantemail_verifiedflag that will be needed by client applications.
AnthonyRonning
force-pushed
the
platform-apis
branch
from
846f2a1 to
08f6aff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/web/platform/org_routes.rs (4)
166-173: Consider a more maintainable approach for email provider validation.Currently, the validation function only allows "resend" as an email provider with a hardcoded string comparison. If support for additional providers is planned in the future, consider using an enum or a configuration-based approach for better maintainability.
-fn validate_email_provider(provider: &str) -> Result<(), validator::ValidationError> { - if provider != "resend" { - let mut error = validator::ValidationError::new("invalid_email_provider"); - error.message = Some("Only 'resend' is supported as an email provider".into()); - return Err(error); - } - Ok(()) -} +fn validate_email_provider(provider: &str) -> Result<(), validator::ValidationError> { + const SUPPORTED_PROVIDERS: [&str; 1] = ["resend"]; + + if !SUPPORTED_PROVIDERS.contains(&provider) { + let mut error = validator::ValidationError::new("invalid_email_provider"); + error.message = Some(format!("Only {:?} are supported as email providers", SUPPORTED_PROVIDERS).into()); + return Err(error); + } + Ok(()) +}
1406-1416: Consider error handling refinement for email verification lookup.The error handling distinguishes between "not found" and other errors, which is good. However, consider adding more specific error logging for non-404 errors to aid in troubleshooting.
match data .db .get_platform_email_verification_by_platform_user_id(platform_user.uuid) { Ok(verification) => verification.is_verified, Err(crate::db::DBError::PlatformEmailVerificationNotFound) => false, Err(e) => { - error!("Error fetching platform email verification: {:?}", e); + error!("Error fetching platform email verification for user {}: {:?}", platform_user.uuid, e); return Err(ApiError::InternalServerError); } };
1437-1442: Add more context to organization lookup error log.When logging errors during organization lookup, include both organization ID and user ID to make troubleshooting easier.
Err(e) => { - error!("Error fetching organization {}: {:?}", org_id, e); + error!("Error fetching organization {} for user {}: {:?}", org_id, platform_user.uuid, e); continue; // Skip this organization but continue processing others }
1398-1468: Consider performance optimization for the user profile endpoint.The
get_platform_userfunction makes multiple sequential database queries - one to check email verification status and another to get memberships. Consider refactoring the database layer to combine these operations if this endpoint becomes a performance bottleneck.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pcrDev.jsonis excluded by!pcrDev.jsonpcrProd.jsonis excluded by!pcrProd.json
📒 Files selected for processing (1)
src/web/platform/org_routes.rs(37 hunks)
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (3)
src/web/platform/org_routes.rs (3)
242-245: LGTM! Added new endpoint is consistent with existing patterns.The new
get_projectendpoint follows the same pattern as other endpoints and is correctly secured with authentication and authorization.
321-325: LGTM! Well-designed user profile endpoint.This new endpoint for retrieving the current user's profile is a valuable addition and follows good REST patterns.
589-589: Fix the formatting issue to make CI pass.As indicated by the previous pipeline failure, this line still has a formatting issue. Run
cargo fmtto fix the code style.
AnthonyRonning
force-pushed
the
platform-apis
branch
from
6842e7e to
6070cb0
Compare
Summary by CodeRabbit
New Features
Refactor
Chores