Move passwords from tmc #1503
Move passwords from tmc #1503
Conversation
WalkthroughAdds local password storage, reset tokens, and single-use email codes; integrates password flows with a TMC client; exposes new auth endpoints and frontend reset/delete pages; implements SMTP email delivery, seeds generic email templates, updates DB schema/models/SQLx artifacts, and adds E2E tests. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant FE as Frontend
participant BE as Backend
participant DB as Database
participant TMC as TMC
participant SMTP as SMTP
rect rgb(200,220,255)
U->>FE: Request password reset (email)
FE->>BE: POST /send-reset-password-email {email, language}
BE->>DB: lookup user
alt user not found locally
BE->>TMC: get_user_from_tmc_with_email
TMC-->>BE: user info
BE->>DB: create local user
end
BE->>DB: insert_password_reset_token
BE->>SMTP: enqueue email with reset link
SMTP-->>U: sends reset link
end
rect rgb(220,255,220)
U->>FE: Click reset link (token)
FE->>BE: POST /reset-password-token-status {token}
BE->>DB: validate token (unused, not expired)
alt valid
FE->>BE: POST /reset-password {token, new_password}
BE->>DB: lock token FOR UPDATE, upsert password
BE->>TMC: set_user_password_managed_by_courses_mooc_fi (if upstream_id)
BE->>DB: mark token used
BE-->>FE: success
else invalid
BE-->>FE: invalid/expired
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
services/headless-lms/server/src/controllers/tmc_server/users.rs (1)
79-87: Same critical email matching issue in password change.This endpoint has the same security vulnerability with partial email matching.
Apply the same fix here to use exact email matching instead of substring search.
🧹 Nitpick comments (1)
services/headless-lms/server/src/controllers/auth.rs (1)
317-317: Fix typo in comment.- // If user is autenticated in tmc succesfully, hash password and save it to db + // If user is authenticated in tmc successfully, hash password and save it to db
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
services/headless-lms/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
services/headless-lms/migrations/20250709091252_add_user_passwords.down.sql(1 hunks)services/headless-lms/migrations/20250709091252_add_user_passwords.up.sql(1 hunks)services/headless-lms/models/.sqlx/query-111a140ce89194ff609ae91a7bbc7e205411c2f99ac741d8892178c24f862a8c.json(1 hunks)services/headless-lms/models/.sqlx/query-21a3a35797bdfd1983294768f228bf72a7732d07f81d365df4b90c94fb61f835.json(1 hunks)services/headless-lms/models/Cargo.toml(1 hunks)services/headless-lms/models/src/lib.rs(1 hunks)services/headless-lms/models/src/user_passwords.rs(1 hunks)services/headless-lms/server/Cargo.toml(1 hunks)services/headless-lms/server/src/controllers/auth.rs(6 hunks)services/headless-lms/server/src/controllers/tmc_server/mod.rs(1 hunks)services/headless-lms/server/src/controllers/tmc_server/users.rs(1 hunks)services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs(2 hunks)services/headless-lms/server/src/domain/authorization.rs(1 hunks)services/headless-lms/utils/src/tmc/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
services/headless-lms/models/src/lib.rs (1)
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs (1)
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
services/headless-lms/utils/src/tmc/mod.rs (1)
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
services/headless-lms/server/src/domain/authorization.rs (1)
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
services/headless-lms/server/src/controllers/tmc_server/mod.rs (1)
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
services/headless-lms/server/src/controllers/tmc_server/users.rs (1)
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
services/headless-lms/server/src/controllers/auth.rs (1)
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
🧬 Code Graph Analysis (2)
services/headless-lms/server/src/controllers/tmc_server/mod.rs (2)
services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs (1)
_add_routes(42-44)services/headless-lms/server/src/controllers/tmc_server/users.rs (1)
_add_routes(96-105)
services/headless-lms/server/src/controllers/tmc_server/users.rs (4)
services/headless-lms/server/src/domain/authorization.rs (1)
authorize_access_from_tmc_server_to_course_mooc_fi(318-352)services/headless-lms/models/src/user_details.rs (1)
search_for_user_details_by_email(97-114)services/headless-lms/models/src/user_passwords.rs (2)
verify_user_password(47-77)upsert_user_password(15-35)services/headless-lms/server/src/controllers/tmc_server/mod.rs (1)
_add_routes(13-16)
🪛 GitHub Actions: headless-lms docs
services/headless-lms/models/src/user_passwords.rs
[error] 9-9: Rust compiler error E0277: the trait ts_rs::TS is not implemented for SecretBox<str>. This causes failure to compile headless-lms-models crate.
🔇 Additional comments (16)
services/headless-lms/server/src/domain/authorization.rs (2)
316-318: Good improvement to function naming and documentation.The function rename from
authorize_access_to_tmc_servertoauthorize_access_from_tmc_server_to_course_mooc_fimakes the authorization direction much clearer. The expanded documentation also provides better context for the function's purpose.
318-352: No remaining references toauthorize_access_to_tmc_serverfound.All calls and imports of the old function name have been updated across the codebase.
services/headless-lms/migrations/20250709091252_add_user_passwords.down.sql (1)
1-1: LGTM - Clean and simple down migration.The down migration correctly drops the
user_passwordstable, properly reversing the corresponding up migration.services/headless-lms/models/src/lib.rs (1)
95-95: LGTM - Standard module declaration.The addition of
pub mod user_passwords;correctly exposes the new user password management module and follows the established pattern in the file.services/headless-lms/models/Cargo.toml (1)
59-62: Dependency versions verified as current and secure
secrecy = "0.10.3"is the latest release with no known vulnerabilities.argon2 = "0.5.3"is the latest stable release; a0.6.0-pre.1prerelease exists but isn’t required for security.- Both crates have no known security issues as of July 2025—approve and merge.
services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs (2)
11-12: LGTM - Consistent update to renamed function.The import correctly reflects the renamed authorization function from
authorize_access_to_tmc_servertoauthorize_access_from_tmc_server_to_course_mooc_fi.
30-30: LGTM - Function call updated consistently.The function call correctly uses the renamed authorization function, maintaining the same authorization logic.
services/headless-lms/server/Cargo.toml (1)
132-135: LGTM: Well-chosen dependencies for secure password management.The addition of
secrecyandargon2dependencies is appropriate for implementing secure password handling.secrecyhelps prevent accidental exposure of sensitive data in logs, andargon2provides industry-standard password hashing.services/headless-lms/utils/src/tmc/mod.rs (1)
180-192: LGTM: Clean implementation following established patterns.The method correctly implements the notification functionality to inform the TMC server that a user's password is managed locally. The implementation follows the established patterns in the codebase and properly handles authentication and error propagation.
services/headless-lms/models/.sqlx/query-111a140ce89194ff609ae91a7bbc7e205411c2f99ac741d8892178c24f862a8c.json (1)
1-18: LGTM: Well-structured query for password retrieval.The query correctly implements password hash retrieval with proper filtering for active records using the soft delete pattern (
deleted_at IS NULL). The parameterized query structure prevents SQL injection and the return type specification is appropriate.services/headless-lms/models/.sqlx/query-21a3a35797bdfd1983294768f228bf72a7732d07f81d365df4b90c94fb61f835.json (1)
1-12: LGTM: Proper upsert implementation for password management.The query correctly implements an atomic upsert operation using
INSERT ... ON CONFLICT. It properly handles both new password creation and updates, with appropriate timestamp management usingNOW(). The parameter types and structure follow PostgreSQL best practices.services/headless-lms/server/src/controllers/tmc_server/mod.rs (2)
7-7: LGTM: Proper module declaration.The addition of the
usersmodule follows Rust conventions and integrates well with the existing codebase structure.
14-15: LGTM: Consistent routing configuration.The new
/usersscope is properly configured following the same pattern as the existing/users-by-upstream-idscope. This will enable the new password authentication and management endpoints.services/headless-lms/migrations/20250709091252_add_user_passwords.up.sql (1)
1-18: Well-structured database migration!The table design follows best practices with proper foreign key constraints, automatic timestamp updates, soft deletion support, and comprehensive documentation.
services/headless-lms/models/src/user_passwords.rs (1)
37-45: Secure password hashing implementation!Good use of Argon2 with random salt generation and SecretString for secure handling.
services/headless-lms/server/src/controllers/auth.rs (1)
109-109: post_new_user_to_moocfi signature change verifiedVerified via ripgrep that the only callers of the wrapper live in
services/headless-lms/server/src/controllers/auth.rsand have been updated to pass references. The underlying util method inservices/headless-lms/utils/src/tmc/mod.rsremains unchanged. The reference-based signature is safe and performance-improving. LGTM.
services/headless-lms/server/src/controllers/tmc_server/users.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/headless-lms/server/src/controllers/auth.rs (1)
115-115: Consider minimizing password cloning for security.While the password cloning is functionally necessary, consider minimizing the number of clones to reduce memory exposure time. The current implementation clones passwords multiple times throughout the authentication flow.
You could potentially restructure to reduce cloning:
// Instead of multiple clones, use references where possible let password_secret = SecretString::new(password.into()); // Pass references to the SecretString instead of cloningAlso applies to: 220-221, 331-331, 357-357, 371-371, 377-377
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/headless-lms/models/.sqlx/query-352f3f2d38b17b63a7620cc7b4c8b657264fe751852d72b38262fa0421093abb.json(1 hunks)services/headless-lms/models/.sqlx/query-625f56ca4ce2b855c400327266a8791c8284a8f95614784034bad5d8e7a8bed1.json(1 hunks)services/headless-lms/models/.sqlx/query-886fbc61200b42e9508cbe49f225a97c49e4521dc7ba8189418e313effc13eb0.json(1 hunks)services/headless-lms/models/src/user_passwords.rs(1 hunks)services/headless-lms/server/src/controllers/auth.rs(8 hunks)
✅ Files skipped from review due to trivial changes (3)
- services/headless-lms/models/.sqlx/query-625f56ca4ce2b855c400327266a8791c8284a8f95614784034bad5d8e7a8bed1.json
- services/headless-lms/models/.sqlx/query-352f3f2d38b17b63a7620cc7b4c8b657264fe751852d72b38262fa0421093abb.json
- services/headless-lms/models/.sqlx/query-886fbc61200b42e9508cbe49f225a97c49e4521dc7ba8189418e313effc13eb0.json
🚧 Files skipped from review as they are similar to previous changes (1)
- services/headless-lms/models/src/user_passwords.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
services/headless-lms/server/src/controllers/auth.rs (1)
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
⏰ 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). (5)
- GitHub Check: build-and-system-tests
- GitHub Check: build-and-deploy
- GitHub Check: headless_lms
- GitHub Check: cms
- GitHub Check: code-style
🔇 Additional comments (10)
services/headless-lms/server/src/controllers/auth.rs (10)
19-19: Good security practice using SecretString.The import of
SecretStringfrom thesecrecycrate is excellent for secure password handling, preventing accidental exposure through debug output or logging.
109-109: Function signature change aligns with reference usage.Changing to pass a reference to
tmc_clientinstead of taking ownership is consistent with the function signature update.
115-115: Password cloning is necessary for multiple uses.Cloning the password here is required since it's used in multiple places (authentication and later hashing).
129-143: Secure password hashing implementation.The password hashing implementation follows security best practices:
- Uses
SecretStringfor secure password handling- Employs Argon2 for hashing (industry standard)
- Proper error handling without sensitive information leakage
- Secure storage via
upsert_user_password
145-160: TMC notification logic is well-implemented.The TMC notification logic properly handles the case where
upstream_idmight be missing, with appropriate warning logging. Error handling is comprehensive and doesn't leak sensitive information.
220-233: Consistent password handling in test mode.The test mode signup correctly mirrors the main signup function's password hashing and storage logic, maintaining consistency between test and production environments.
295-295: TMC client parameter addition is necessary.Adding the
tmc_clientparameter enables the new TMC notification functionality in the login flow.
312-345: Enhanced test mode authentication with dual verification.The test mode now properly supports both seeded test credentials and stored password verification, providing flexibility for testing scenarios while maintaining security.
347-410: Well-structured authentication flow with local-first approach.The authentication flow is well-designed:
- Attempts local password verification first (performance benefit)
- Falls back to TMC authentication for backward compatibility
- Stores password locally after successful TMC authentication
- Properly notifies TMC when password is managed locally
The implementation maintains security while providing a smooth migration path.
481-481: Function signature optimization for reference usage.Changing the parameter to take a reference instead of ownership is a reasonable optimization that aligns with the calling pattern in the signup function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/headless-lms/server/src/controllers/auth.rs (1)
109-161: Secure password storage implementation with minor optimization opportunity.The password hashing and storage implementation is well-structured:
- Proper use of Argon2 for password hashing
- Comprehensive error handling for database operations
- Appropriate TMC notification when password management is transferred
However, consider minimizing password cloning to reduce memory exposure:
- let auth_result = authorization::authenticate_moocfi_user( - &mut conn, - &client, - user_details.email, - user_details.password.clone(), - ) - .await?; - - if let Some((user, _token)) = auth_result { - // ... other code ... - let password_hash = models::user_passwords::hash_password(&SecretString::new( - user_details.password.into(), - )) + let password_secret = SecretString::new(user_details.password.into()); + let auth_result = authorization::authenticate_moocfi_user( + &mut conn, + &client, + user_details.email, + password_secret.expose_secret().clone(), + ) + .await?; + + if let Some((user, _token)) = auth_result { + // ... other code ... + let password_hash = models::user_passwords::hash_password(&password_secret)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/headless-lms/server/src/controllers/auth.rs(6 hunks)system-tests/src/tests/create-account-and-login.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
system-tests/**/*
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (2)
📓 Common learnings
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
services/headless-lms/server/src/controllers/auth.rs (1)
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
⏰ 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). (3)
- GitHub Check: build-and-deploy
- GitHub Check: headless_lms
- GitHub Check: build-and-system-tests
🔇 Additional comments (6)
system-tests/src/tests/create-account-and-login.spec.ts (1)
1-34: LGTM! Test properly waits for UI confirmations after backend mutations.The test correctly follows the coding guidelines by waiting for UI confirmation elements after each backend mutation:
- After account creation (line 15), it waits for the research consent heading (line 17)
- After saving research consent (line 19), it waits for the email confirmation heading (line 21)
- After login (line 30), it waits for the "Welcome!" heading (line 32)
This ensures the test doesn't proceed before backend operations complete.
services/headless-lms/server/src/controllers/auth.rs (5)
19-19: Good security practice using SecretString for password handling.The import of
SecretStringfrom thesecrecycrate is excellent for secure password handling, preventing accidental exposure in logs or memory dumps.
219-233: Consistent password handling in test mode.The test mode signup correctly implements the same secure password hashing and storage pattern as production mode, ensuring consistency across environments.
295-312: Excellent refactoring for better code organization.The login function has been well-refactored into separate helper functions for different authentication modes (UUID login, test mode, production). This improves maintainability and makes the authentication logic clearer.
314-457: Well-structured authentication helpers with robust fallback mechanism.The authentication helper functions are excellently implemented:
- UUID login: Simple and effective for development
- Test mode login: Properly handles both seeded test credentials and stored passwords
- Production login: Implements a secure fallback strategy:
- First attempts local password authentication
- Falls back to TMC authentication if local fails
- Stores password locally after successful TMC auth
- Notifies TMC about password management transfer
The fallback mechanism ensures smooth migration from TMC-managed to locally-managed passwords while maintaining security.
519-519: Good consistency improvement with web::Data wrapper.The function signature change to use
web::Data<TmcClient>is consistent with other handlers in the file and follows Actix Web's dependency injection patterns.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
services/headless-lms/models/src/user_passwords.rs (1)
6-12: TypeScript generation issue resolved.The previous compilation error with
SecretStringnot implementing theTStrait has been resolved by removing the TypeScript generation attributes from this struct. The struct design is now clean and secure.
🧹 Nitpick comments (3)
services/main-frontend/src/pages/reset-password.tsx (1)
35-41: Improve email validation for better user experience.The current email validation only checks for the presence of "@" character, which is quite basic and may allow invalid email formats.
Consider using a more robust email validation pattern:
- validate: { - isValidEmail: (value) => - value.split("").indexOf("@") !== -1 || t("enter-a-valid-email"), - }, + pattern: { + value: /^[^\s@]+@[^\s@]+\.[^\s@]+$/, + message: t("enter-a-valid-email"), + },services/headless-lms/server/src/controllers/main_frontend/users.rs (1)
189-193: Consider making email template subject configurable.The hardcoded subject "Reset password request" makes the system less flexible. Consider making this configurable through environment variables or database configuration.
services/headless-lms/models/src/user_passwords.rs (1)
88-105: *Minor optimization: Consider selecting specific column instead of .The function works correctly but could be slightly optimized by selecting a specific column instead of
SELECT *:-SELECT * +SELECT user_idThis reduces data transfer while maintaining the same functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
services/headless-lms/migrations/20250721091109_add-password-reset-tokens.down.sql(1 hunks)services/headless-lms/migrations/20250721091109_add-password-reset-tokens.up.sql(1 hunks)services/headless-lms/models/.sqlx/query-064a87b5bc647afbc6ad71649b451f69d150965ed896f9f7750d7ca90c360f46.json(1 hunks)services/headless-lms/models/.sqlx/query-12678b433a9c84513a0c100571b2d82db27fbc569e2b9c79b23e31906be0f559.json(0 hunks)services/headless-lms/models/.sqlx/query-247b57ac933ff6ed9d2a70daf2260c53c52f3fae1f1cb9d7212334837b909b1a.json(1 hunks)services/headless-lms/models/.sqlx/query-4db47a0bd26959404dbe4e273d07ccb74d9400d83f5c9d58c97fcef1f1191e40.json(1 hunks)services/headless-lms/models/.sqlx/query-8283c7aa316fe69e3ad158ddfb699a0955085460b4db39f7c5fbbfc352aba49d.json(1 hunks)services/headless-lms/models/.sqlx/query-8418000fb3d50f52e42766c3884416520d434ea9588248e26c3b4fbfb1abae90.json(1 hunks)services/headless-lms/models/.sqlx/query-c4db451cf0d08b2fca14976798e0883a3961533b35d7dd4d5057e1662d482592.json(1 hunks)services/headless-lms/models/.sqlx/query-dc6a94949ce20d11fa55e247dc706fdc88018bf1ba528487fcd0ef02f5c40ca7.json(1 hunks)services/headless-lms/models/.sqlx/query-e0da70d42e11a70fc5b3a4741a6f0781f81eec98fd05dcade978139c8a6bc762.json(1 hunks)services/headless-lms/models/.sqlx/query-efbc0faf9c6ac6336499543ea29b528cfcf60c9443f6495dfa13c38fe94a3dfd.json(1 hunks)services/headless-lms/models/.sqlx/query-f2beb11b5e6318b712b0af9bd4fde454472ce8eda3612295b788958148a376b5.json(1 hunks)services/headless-lms/models/.sqlx/query-f7841567b90206f7d7aba83b520e1cb7933c33d203d9a1df88e3db961ab83681.json(1 hunks)services/headless-lms/models/.sqlx/query-fd692e5f883ffec6910d75792c99c20dc1ea41c5912c6a8c1e2be6727069c6ff.json(1 hunks)services/headless-lms/models/src/email_deliveries.rs(1 hunks)services/headless-lms/models/src/email_templates.rs(2 hunks)services/headless-lms/models/src/user_passwords.rs(1 hunks)services/headless-lms/server/src/controllers/auth.rs(6 hunks)services/headless-lms/server/src/controllers/main_frontend/users.rs(2 hunks)services/headless-lms/server/src/controllers/tmc_server/users.rs(1 hunks)services/headless-lms/server/src/programs/email_deliver.rs(3 hunks)services/headless-lms/utils/src/email_processor.rs(18 hunks)services/headless-lms/utils/src/tmc/mod.rs(1 hunks)services/main-frontend/src/pages/login.tsx(1 hunks)services/main-frontend/src/pages/reset-password.tsx(1 hunks)services/main-frontend/src/services/backend/users.ts(2 hunks)shared-module/packages/common/src/bindings.guard.ts(1 hunks)shared-module/packages/common/src/bindings.ts(1 hunks)
💤 Files with no reviewable changes (1)
- services/headless-lms/models/.sqlx/query-12678b433a9c84513a0c100571b2d82db27fbc569e2b9c79b23e31906be0f559.json
✅ Files skipped from review due to trivial changes (9)
- services/headless-lms/models/.sqlx/query-e0da70d42e11a70fc5b3a4741a6f0781f81eec98fd05dcade978139c8a6bc762.json
- services/headless-lms/models/.sqlx/query-8418000fb3d50f52e42766c3884416520d434ea9588248e26c3b4fbfb1abae90.json
- services/main-frontend/src/pages/login.tsx
- services/headless-lms/models/.sqlx/query-064a87b5bc647afbc6ad71649b451f69d150965ed896f9f7750d7ca90c360f46.json
- services/headless-lms/models/.sqlx/query-fd692e5f883ffec6910d75792c99c20dc1ea41c5912c6a8c1e2be6727069c6ff.json
- services/headless-lms/models/.sqlx/query-8283c7aa316fe69e3ad158ddfb699a0955085460b4db39f7c5fbbfc352aba49d.json
- services/headless-lms/models/.sqlx/query-247b57ac933ff6ed9d2a70daf2260c53c52f3fae1f1cb9d7212334837b909b1a.json
- services/headless-lms/models/.sqlx/query-4db47a0bd26959404dbe4e273d07ccb74d9400d83f5c9d58c97fcef1f1191e40.json
- services/headless-lms/models/.sqlx/query-efbc0faf9c6ac6336499543ea29b528cfcf60c9443f6495dfa13c38fe94a3dfd.json
🚧 Files skipped from review as they are similar to previous changes (2)
- services/headless-lms/utils/src/tmc/mod.rs
- services/headless-lms/server/src/controllers/auth.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit Configuration File
**/*.tsx: Make sure to use theuseTranslationhook for all text that is visible to the user.
If a component uses colors, use colors from the theme if possible. Example: import { baseTheme } from "@/shared-module/common/styles" ... background: ${baseTheme.colors.clear[100]};
If a component uses media queries, use the respondToOrLarger function. Example: import { respondToOrLarger } from "@/shared-module/common/styles/respond" ... ${respondToOrLarger.sm} { ... }
If component sends requests to server, use either useQuery, or useToastMutation hooks, or use hooks wrapping those hooks.
If there is a form of any kind, make sure it uses useForm from react-hook-form, and make sure the form is integrated with the useToastMutation and useQuery hooks.
For loading and error states, use the Spinner and ErrorBanner components from the shared module.
Files:
services/main-frontend/src/pages/reset-password.tsx
🧠 Learnings (6)
services/headless-lms/models/src/email_templates.rs (1)
Learnt from: nygrenh
PR: #1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In services/headless-lms/server/src/controllers/course_material/courses.rs, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use authorize_access_to_course_material instead of authorize with Act::Teach for authorization.
services/main-frontend/src/pages/reset-password.tsx (2)
Learnt from: nygrenh
PR: #1335
File: services/main-frontend/src/components/page-specific/manage/courses/id/glossary/TermItem.tsx:88-94
Timestamp: 2024-11-14T14:07:21.540Z
Learning: In services/main-frontend/src/components/page-specific/manage/courses/id/glossary/TermItem.tsx, when validating form inputs using react-hook-form, prefer to keep using the regex pattern /\S+/ for required fields, and do not change it to a stricter validation that trims whitespace.
Learnt from: nygrenh
PR: #1335
File: services/main-frontend/src/components/page-specific/manage/courses/id/glossary/CreateTermForm.tsx:54-62
Timestamp: 2024-11-14T14:07:12.703Z
Learning: In the CreateTermForm component located at services/main-frontend/src/components/page-specific/manage/courses/id/glossary/CreateTermForm.tsx, the current validation using the regex pattern /\S+/ for input fields is preferred and enhancements to handle additional whitespace cases are not necessary.
services/headless-lms/server/src/controllers/tmc_server/users.rs (1)
Learnt from: nygrenh
PR: #1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In services/headless-lms/server/src/controllers/course_material/courses.rs, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use authorize_access_to_course_material instead of authorize with Act::Teach for authorization.
shared-module/packages/common/src/bindings.guard.ts (1)
Learnt from: nygrenh
PR: #1335
File: services/main-frontend/src/components/page-specific/manage/courses/id/glossary/TermItem.tsx:88-94
Timestamp: 2024-11-14T14:07:21.540Z
Learning: In services/main-frontend/src/components/page-specific/manage/courses/id/glossary/TermItem.tsx, when validating form inputs using react-hook-form, prefer to keep using the regex pattern /\S+/ for required fields, and do not change it to a stricter validation that trims whitespace.
services/headless-lms/models/src/user_passwords.rs (1)
Learnt from: nygrenh
PR: #1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In services/headless-lms/server/src/controllers/course_material/courses.rs, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use authorize_access_to_course_material instead of authorize with Act::Teach for authorization.
services/headless-lms/server/src/controllers/main_frontend/users.rs (1)
Learnt from: nygrenh
PR: #1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In services/headless-lms/server/src/controllers/course_material/courses.rs, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use authorize_access_to_course_material instead of authorize with Act::Teach for authorization.
🧬 Code Graph Analysis (3)
services/main-frontend/src/services/backend/users.ts (2)
services/main-frontend/src/services/mainFrontendClient.ts (1)
mainFrontendClient(3-7)shared-module/packages/common/src/utils/fetching.ts (2)
validateResponse(10-29)isBoolean(56-58)
services/headless-lms/models/src/email_templates.rs (1)
shared-module/packages/common/src/bindings.ts (1)
EmailTemplate(546-557)
services/main-frontend/src/pages/reset-password.tsx (1)
services/main-frontend/src/services/backend/users.ts (1)
sendResetPasswordLink(55-58)
⏰ 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). (5)
- GitHub Check: build-and-deploy
- GitHub Check: build-and-system-tests
- GitHub Check: cms
- GitHub Check: headless_lms
- GitHub Check: code-style
🔇 Additional comments (38)
shared-module/packages/common/src/bindings.ts (1)
556-556: LGTM - Appropriate nullability change for generic email templates.The change from
stringtostring | nullforcourse_instance_idcorrectly reflects the database schema update to support generic email templates that aren't tied to specific course instances. This is essential for password reset functionality where templates should be course-agnostic.services/headless-lms/models/.sqlx/query-f2beb11b5e6318b712b0af9bd4fde454472ce8eda3612295b788958148a376b5.json (1)
60-60: LGTM - Metadata correctly updated for nullable course_instance_id.The nullability metadata update properly reflects the database schema change where
course_instance_idcan now be NULL. This ensures SQLx query validation remains accurate after the schema modification.services/headless-lms/models/.sqlx/query-dc6a94949ce20d11fa55e247dc706fdc88018bf1ba528487fcd0ef02f5c40ca7.json (1)
60-60: LGTM - Consistent nullability metadata update.The nullable metadata correctly reflects the schema change for the soft delete query's RETURNING clause. This maintains consistency across all email_templates query metadata files.
services/headless-lms/models/.sqlx/query-c4db451cf0d08b2fca14976798e0883a3961533b35d7dd4d5057e1662d482592.json (1)
60-60: LGTM - Nullability metadata updated consistently.The metadata update correctly reflects the schema change. Note that this query specifically filters for course-instance-specific templates (
WHERE course_instance_id = $1), which maintains the existing behavior while the schema now also supports generic templates with NULL course_instance_id values.shared-module/packages/common/src/bindings.guard.ts (1)
1196-1197: LGTM! Type guard correctly updated for nullable field.The change properly validates
course_instance_idas nullable (string | null), which aligns with the database schema migration that made this column nullable in theemail_templatestable. The type guard logic correctly handles both null values and string values.services/headless-lms/models/.sqlx/query-f7841567b90206f7d7aba83b520e1cb7933c33d203d9a1df88e3db961ab83681.json (1)
60-60: LGTM! Schema change correctly reflected.The nullable flag change for
course_instance_id(index 9) fromfalsetotruecorrectly reflects the database migration that allows email templates to be generic (not tied to a specific course instance).services/main-frontend/src/services/backend/users.ts (2)
17-17: LGTM! Proper import addition.The
isBooleanimport is correctly added to support the new password reset function's response validation.
55-58: LGTM! Well-implemented password reset function.The
sendResetPasswordLinkfunction follows established patterns in the codebase:
- Proper TypeScript async function signature
- Correct use of
mainFrontendClient.post- Appropriate response validation with
isBooleantype guard- Clean and consistent with other service functions
services/headless-lms/migrations/20250721091109_add-password-reset-tokens.down.sql (1)
1-11: Fix the COMMENT syntax on line 10.The rollback logic is sound, but line 10 has incorrect SQL syntax. The
COMMENT ON COLUMNstatement should not useIS NULL.Apply this fix:
-COMMENT ON COLUMN email_templates.course_instance_id IS NULL; +COMMENT ON COLUMN email_templates.course_instance_id IS '';Or remove the comment entirely if not needed:
-COMMENT ON COLUMN email_templates.course_instance_id IS NULL;The rest of the rollback operations are correct:
- Dropping the
password_reset_tokenstable- Deleting generic templates before enforcing NOT NULL constraint
- Restoring the NOT NULL constraint on
course_instance_idLikely an incorrect or invalid review comment.
services/headless-lms/utils/src/email_processor.rs (3)
17-17: LGTM! Serde tag update is consistent.The change from
"name"to"type"for the serde tag aligns with common JSON schema conventions and likely improves compatibility with frontend processing.
53-53: LGTM! Type change is properly implemented.The
client_idfield type change fromUuidtoStringis consistently applied throughout the file, including all test cases. This change likely simplifies serialization/deserialization and frontend integration.
140-463: LGTM! Test updates are consistent.All test cases have been properly updated to convert
Uuid::new_v4()to string using.to_string(), maintaining the same test logic while adapting to the newStringtype forclient_id. The test coverage remains comprehensive and functional.services/headless-lms/models/src/email_templates.rs (2)
15-15: LGTM! Field type change aligns with schema migration.Changing
course_instance_idfromUuidtoOption<Uuid>correctly reflects the database schema change that allows email templates to be generic (not tied to a specific course instance).
51-70: LGTM! Well-implemented function for generic email templates.The
get_generic_email_templates_by_subjectfunction is properly implemented:
- Clear, descriptive function name
- Proper async/await pattern with
ModelResult<Vec<EmailTemplate>>- Correct SQL query filtering for generic templates (
course_instance_id IS NULL)- Consistent with existing code patterns in the file
- Supports the password reset functionality by enabling retrieval of subject-specific generic email templates
services/main-frontend/src/pages/reset-password.tsx (1)
14-51: Component implementation follows coding guidelines well.The component correctly uses
useForm,useToastMutation, and proper form integration as specified in the coding guidelines.services/headless-lms/server/src/controllers/main_frontend/users.rs (1)
180-180: Verify that skipping authorization is appropriate for password reset.Using
skip_authorize()allows anyone to trigger password reset emails. While this is typically correct for password reset functionality, ensure this aligns with your security requirements.services/headless-lms/models/src/email_deliveries.rs (3)
17-23: Good structural improvement to separate user ID and email address.The change from storing just a user ID to storing both
user_idand the actual email addresstoimproves the data model and makes email processing more efficient.
25-49: Well-implemented insert function follows standard patterns.The
insert_email_deliveryfunction correctly generates UUIDs and follows the established patterns in the codebase.
54-69: SQL query correctly handles the schema changes.The updated query properly joins the necessary tables to retrieve both user information and email addresses.
services/headless-lms/server/src/programs/email_deliver.rs (2)
121-147: Well-designed placeholder replacement function.The helper function properly handles only paragraph blocks and uses a clean replacement strategy with HashMap. The implementation is focused and safe.
56-80: Good implementation of conditional reset link insertion.The logic for detecting password reset emails and inserting tokens is well-structured with proper error handling and case-insensitive matching.
services/headless-lms/migrations/20250721091109_add-password-reset-tokens.up.sql (3)
1-25: Well-designed password reset token table with proper security measures.The table structure includes all necessary security features: expiration timestamps, usage tracking, and soft deletion. The 15-minute expiration window is a good security practice.
27-32: Good enhancement to support both generic and course-specific email templates.Making
course_instance_idnullable allows the system to support both general templates (like password reset) and course-specific templates, which aligns well with the password reset feature requirements.
16-24: Comprehensive documentation improves maintainability.The detailed comments on each column make the schema self-documenting and easier to understand for future developers.
services/headless-lms/server/src/controllers/tmc_server/users.rs (6)
1-13: LGTM: Well-documented module with secure imports.The module documentation clearly explains the purpose and security requirements. The imports properly use
SecretStringfrom the secrecy crate for secure password handling.
15-19: LGTM: Secure struct design using SecretString.The
LoginRequeststruct correctly usesSecretStringto protect the password field from accidental exposure in logs or debug output.
30-47: LGTM: Secure authentication implementation.This implementation correctly:
- Uses
user_idfor exact user identification, avoiding the email matching security issues- Properly authorizes TMC server access
- Uses secure error handling with
unwrap_or(false)- Leverages the
verify_user_passwordfunction for password verification
49-54: LGTM: Secure password change request structure.The struct properly uses
SecretStringfor both password fields to prevent accidental exposure.
63-98: LGTM: Secure password change implementation.This implementation follows security best practices:
- Verifies the old password before allowing changes
- Properly hashes the new password before storage
- Uses secure error handling that doesn't leak information
- Maintains authorization throughout the process
100-109: LGTM: Clean route configuration.The route setup is straightforward and correctly maps endpoints to handlers.
services/headless-lms/models/src/user_passwords.rs (8)
1-5: LGTM: Proper imports for secure password handling.The imports correctly include Argon2 for password hashing and the secrecy crate for secure string handling.
14-22: LGTM: Well-designed password reset token structure.The
PasswordResetTokenstruct includes all necessary fields for secure token management with proper timestamps and soft deletion support.
24-43: LGTM: Secure UPSERT implementation.The function correctly implements the UPSERT pattern for password storage. Using
expose_secret()is necessary for database insertion, and the function properly returns a boolean indicating success.
45-53: LGTM: Secure password hashing implementation.The function correctly uses Argon2 with:
- Random salt generation using
OsRng- Default Argon2 configuration (which provides good security defaults)
- Proper error handling and SecretString wrapping
55-86: LGTM: Secure password verification implementation.The function implements secure password verification:
- Uses exact user ID matching
- Checks for soft deletion
- Safely handles hash parsing errors
- Uses Argon2 for verification
- Returns
falseon any error, preventing information leakage
107-128: LGTM: Clean password reset token generation.The function correctly generates a UUID token and inserts it into the database. The database schema likely handles expiration timing through defaults or triggers.
130-158: LGTM: Comprehensive token validation logic.The function correctly validates reset tokens by checking:
- User ID match
- Not soft deleted
- Not already used
- Not expired (using current timestamp)
160-175: LGTM: Proper token invalidation implementation.The function correctly invalidates tokens by setting both
used_atanddeleted_attimestamps, ensuring tokens cannot be reused and are properly marked as deleted.
services/headless-lms/server/src/controllers/main_frontend/users.rs
Outdated
Show resolved
Hide resolved
services/headless-lms/server/src/controllers/main_frontend/users.rs
Outdated
Show resolved
Hide resolved
…ect-331 into move-passwords-from-tmc
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (6)
services/headless-lms/server/src/programs/email_deliver.rs (2)
22-31: Ensure infra sets new SMTP_ env vars everywhere.*These are required (expect panics). Confirm Helm/Kubernetes, skaffold, CI, and .env.example are updated; also include FRONTEND_BASE_URL. This was flagged earlier and still applies.
Run to spot missing wiring:
#!/bin/bash rg -n 'SMTP_(FROM|HOST|USER|PASS)|FRONTEND_BASE_URL' -S kubernetes helm deploy .env** docs** || true
55-60: Persist body-missing/invalid-JSON errors to DB to avoid infinite retries.Currently
?will bubble errors; the email stays eligible and gets retried forever.Apply:
- let mut email_block: Vec<EmailGutenbergBlock> = - serde_json::from_value(email.body.context("No body")?)?; + // Persist template body/JSON issues to avoid retry loops + let body = match email.body { + Some(v) => v, + None => { + save_err_to_email(email.id, "No body in email template", &mut conn).await?; + return Ok(()); + } + }; + let mut email_block: Vec<EmailGutenbergBlock> = match serde_json::from_value(body) { + Ok(blocks) => blocks, + Err(err) => { + save_err_to_email(email.id, format!("Invalid template JSON: {err}"), &mut conn) + .await?; + return Ok(()); + } + };services/main-frontend/src/components/forms/DeleteUserAccountForm.tsx (4)
29-34: Password state management needs React Hook Form integrationThe password is being managed with local state instead of being integrated with React Hook Form, which makes the form validation incomplete. Currently,
useFormis not being utilized at all since there are no registered fields and no form submission handler.
36-52: Pass form data to mutation instead of using component stateThe mutation closes over component state instead of accepting form data as parameters, which can lead to stale closure issues.
65-65: Use queryClient.clear() for logout flowsFor logout-like flows,
queryClient.clear()is more appropriate thanremoveQueries()as it clears both query and mutation caches completely.Apply this diff:
- queryClient.removeQueries() + queryClient.clear()
54-72: Add error handling to deleteAccountMutationThe mutation lacks an
onErrorhandler to properly surface failures to the user via the ErrorBanner.Apply this diff to add error handling:
const deleteAccountMutation = useToastMutation( async (code: string) => { const result = await deleteUserAccount(code) setError(null) setCredentialsError(!result) return result }, { notify: false }, { onSuccess: (result) => { if (result) { queryClient.clear() loginStateContext.refresh() // eslint-disable-next-line i18next/no-literal-string router.push("/login") } }, + onError: (err) => { + setError(err as Error) + }, }, )
🧹 Nitpick comments (20)
shared-module/packages/common/src/locales/en/main-frontend.json (2)
243-244: Minor copy polish: drop unnecessary comma.Reads more naturally in English.
- "enter-email-for-password-reset-link": "Enter your email address, and we will send you a link to reset your password.", + "enter-email-for-password-reset-link": "Enter your email address and we will send you a link to reset your password.",
719-722: Consistent punctuation with error messages.Most error strings end with a period; consider aligning.
- "reset-link-has-expired": "Password reset link has expired", + "reset-link-has-expired": "Password reset link has expired.",services/headless-lms/server/src/programs/email_deliver.rs (2)
154-180: Placeholders are replaced only in Paragraph blocks.If templates use placeholders in headings or lists, they won’t be substituted. Consider extending replacement to Heading.content and List.values.
Example refactor:
fn insert_placeholders( blocks: Vec<EmailGutenbergBlock>, replacements: &HashMap<String, String>, ) -> Vec<EmailGutenbergBlock> { - blocks + fn repl(s: String, reps: &HashMap<String, String>) -> String { + reps.iter().fold(s, |acc, (k, v)| acc.replace(&format!("{{{{{}}}}}", k), v)) + } + blocks .into_iter() .map(|mut block| { - if let BlockAttributes::Paragraph { - content, - drop_cap, - rest, - } = block.attributes - { - let replaced_content = replacements.iter().fold(content, |acc, (key, value)| { - acc.replace(&format!("{{{{{}}}}}", key), value) - }); - - block.attributes = BlockAttributes::Paragraph { - content: replaced_content, - drop_cap, - rest, - }; - } + match block.attributes { + BlockAttributes::Paragraph { content, drop_cap, rest } => { + block.attributes = BlockAttributes::Paragraph { content: repl(content, replacements), drop_cap, rest }; + } + BlockAttributes::Heading { content, level, rest } => { + block.attributes = BlockAttributes::Heading { content: repl(content, replacements), level, rest }; + } + BlockAttributes::List { values, ordered, rest } => { + block.attributes = BlockAttributes::List { values: repl(values, replacements), ordered, rest }; + } + BlockAttributes::Image { .. } => { /* no-op */ } + } block }) .collect() }
56-57: Nit: logging level/message.“Email send messages...” is unclear; prefer debug with email.id for traceability.
- tracing::info!("Email send messages..."); + tracing::debug!("Sending email {}...", email.id);shared-module/packages/common/src/components/InputFields/OneTimePasscodeField.tsx (4)
1-6: Add i18n and responsive helper importsImport useTranslation for accessible labels and the respondToOrLarger helper per guidelines.
import { css } from "@emotion/css" import React, { useCallback, useRef, useState } from "react" import { Input, TextField } from "react-aria-components" import { baseTheme } from "../../styles" +import { useTranslation } from "react-i18next" +import { respondToOrLarger } from "../../styles/respond"
11-15: Initialize i18n in the componentUse t() for aria-labels.
-export function OneTimePassCodeField({ onChange }: Props) { +export function OneTimePassCodeField({ onChange }: Props) { + const { t } = useTranslation()
92-129: Localize aria-labels, use theme color, and add OTP UX attributes
- Localize aria-labels (visible to SR users).
- Replace hard-coded #ccc with theme color.
- Add autocomplete and pattern for better OTP UX.
- aria-label={`Digit ${idx + 1}`} + aria-label={t("otp-digit", { index: idx + 1 })} type="text" // eslint-disable-next-line i18next/no-literal-string inputMode="numeric" + autoComplete="one-time-code" + pattern="[0-9]*" maxLength={1} value={val} onChange={(e) => handleChange(e.target.value, idx)} onKeyDown={(e) => handleKeyDown(e, idx)} onPaste={(e) => handlePaste(e, idx)} className={css` width: 40px; height: 40px; font-size: 20px; text-align: center; padding: 8px; border-radius: 6px; - border: 1px solid #ccc; + border: 1px solid ${baseTheme.colors.clear[500]}; &:focus { outline: none; border-color: ${baseTheme.colors.green[600]}; box-shadow: 0 0 0 1px ${baseTheme.colors.green[600]}; } - @media (max-width: 480px) { - width: 50px; - height: 50px; - font-size: 24px; - } + /* Consider switching to respondToOrLarger.* helpers per guidelines */ + ${respondToOrLarger.sm} { + /* Example: adjust sizes for >= sm breakpoints */ + width: 40px; + height: 40px; + font-size: 20px; + } `} />
11-11: Unify component name with filename
- In
shared-module/packages/common/src/components/InputFields/OneTimePasscodeField.tsx, rename the exported symbol, displayName, and default export fromOneTimePassCodeFieldtoOneTimePasscodeField.- Update all import sites (e.g.
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsxat the import on line 6 and usage on line 42) to import and useOneTimePasscodeFieldinstead ofOneTimePassCodeField.services/main-frontend/src/components/forms/VerifyPasswordForm.tsx (1)
39-48: Add autocomplete and optional loading Spinner
- Add autoComplete="current-password" to help browsers.
- Optional: show Spinner when isPending.
<TextField type="password" label={t("label-password")} + autoComplete="current-password" className={css` margin-bottom: 0px; `} aria-invalid={credentialsError} aria-describedby={credentialsError ? "password-error" : undefined} {...register("password", { required: true })} /> @@ - <Button type="submit" variant="primary" disabled={isPending} size={"medium"}> + <Button type="submit" variant="primary" disabled={isPending} size={"medium"}> {t("confirm")} </Button> + {isPending && <Spinner />}Add import:
+import Spinner from "@/shared-module/common/components/Spinner"Also applies to: 62-64
services/headless-lms/migrations/20250902111139_add_user_email_codes.down.sql (1)
1-1: Make down migration idempotentUse IF EXISTS to avoid failures when rolling back multiple times.
-DROP TABLE user_email_codes; +DROP TABLE IF EXISTS user_email_codes;services/headless-lms/models/.sqlx/query-d65c5db352c07e9b93914dedb530c0781f66e6660c9d74e38442f70b86aed9ad.json (1)
1-18: LGTM: insert and return code
Add aCHECKconstraint foruser_email_codes.codeinservices/headless-lms/migrations/20250902111139_add_user_email_codes.up.sqlto enforce the intended format (e.g.CHECK (code ~ '^[0-9]{6}$')).services/headless-lms/models/.sqlx/query-3c83c42e752bc8f58d93cc7f32adaac035d7a85d44900da6134a456f69f7e83f.json (1)
1-53: Consider explicit ordering/limiting for selectionIf callers only need the newest active code, add ORDER BY created_at DESC LIMIT 1 to avoid scanning/returning multiple rows.
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx (1)
80-81: Avoid using !important in CSSUsing
!importantshould be avoided as it makes styles harder to override and maintain. Consider increasing specificity or adjusting the component's CSS structure instead.Apply this diff to remove !important:
className={css` - padding-left: 4px !important; - color: ${baseTheme.colors.green[600]}!important; + padding-left: 4px; + color: ${baseTheme.colors.green[600]}; + &:hover { + color: ${baseTheme.colors.green[700]}; + } `}services/main-frontend/src/components/forms/DeleteUserAccountForm.tsx (1)
92-95: Consider using async/await consistentlyThe password submission handler mixes callback-style with async mutation. Consider awaiting the mutation for better error handling.
Apply this diff for consistency:
onSubmit={(pwd) => { setPassword(pwd) - sendEmailCodeMutation.mutateAsync() + void sendEmailCodeMutation.mutateAsync() }}Or better yet, make it async:
- onSubmit={(pwd) => { + onSubmit={async (pwd) => { setPassword(pwd) - sendEmailCodeMutation.mutateAsync() + await sendEmailCodeMutation.mutateAsync() }}services/headless-lms/migrations/20250902111139_add_user_email_codes.up.sql (2)
3-3: Consider increasing code length for better securityA 16-character code provides limited entropy. Consider using at least 32 characters for verification codes to ensure sufficient security against brute force attacks.
Apply this diff to increase code length:
- code VARCHAR(16) NOT NULL, + code VARCHAR(32) NOT NULL,
5-5: Consider making expiry time configurableThe 1-hour expiry is hardcoded. Consider making this configurable via application settings to allow flexibility for different use cases.
Consider storing the expiry interval in a configuration table or environment variable, then using a function or procedure to retrieve it when inserting new codes. This would allow adjusting the expiry time without schema changes.
services/headless-lms/models/src/user_email_codes.rs (4)
84-88: Change parameter type from&Stringto&str.Using
&Stringas a parameter type is unnecessarily restrictive and forces callers to have aString. Using&stris more idiomatic and allows bothStringand string literal inputs.pub async fn is_reset_user_email_code_valid( conn: &mut PgConnection, user_id: Uuid, - code: &String, + code: &str, ) -> ModelResult<bool> {
110-114: Change parameter type from&Stringto&str.Same issue as above - using
&stris more idiomatic and flexible than&String.pub async fn mark_user_email_code_used( conn: &mut PgConnection, user_id: Uuid, - code: &String, + code: &str, ) -> ModelResult<bool> {
20-33: Review transaction handling for code cleanup.The soft-delete operation uses
let _ = ...which ignores the result. While this might be intentional (to proceed even if no previous codes exist), consider logging any unexpected errors to aid debugging.- let _ = sqlx::query!( + let result = sqlx::query!( r#" UPDATE user_email_codes SET deleted_at = NOW() WHERE user_id = $1 AND deleted_at IS NULL "#, user_id ) .execute(&mut *tx) - .await?; + .await; + + if let Err(e) = result { + warn!("Failed to soft-delete previous email codes for user {}: {:?}", user_id, e); + return Err(e.into()); + }
57-57: Consider using a configurable expiration buffer.The
nowtimestamp is used directly for expiration checking. Consider adding a small buffer (e.g., a few seconds) to account for clock skew between application and database servers.- let now = Utc::now(); + // Add small buffer to account for potential clock skew + let now = Utc::now() + chrono::Duration::seconds(5);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
services/headless-lms/migrations/20250902111139_add_user_email_codes.down.sql(1 hunks)services/headless-lms/migrations/20250902111139_add_user_email_codes.up.sql(1 hunks)services/headless-lms/models/.sqlx/query-3c83c42e752bc8f58d93cc7f32adaac035d7a85d44900da6134a456f69f7e83f.json(1 hunks)services/headless-lms/models/.sqlx/query-4df53707e69ba703ee89d364b428b7b13d0287de2cda81404614d198857ff3df.json(1 hunks)services/headless-lms/models/.sqlx/query-7e5aee5c3efb4a45026448acfa6f88a811b40b4495cad020ff40191fb975202b.json(1 hunks)services/headless-lms/models/.sqlx/query-b1f5e7cb1a58fb9d07b75208a5fc5157a194e6f098ada9f8eb63d3eb4a5b1c9d.json(1 hunks)services/headless-lms/models/.sqlx/query-d65c5db352c07e9b93914dedb530c0781f66e6660c9d74e38442f70b86aed9ad.json(1 hunks)services/headless-lms/models/src/lib.rs(1 hunks)services/headless-lms/models/src/user_email_codes.rs(1 hunks)services/headless-lms/server/src/controllers/auth.rs(8 hunks)services/headless-lms/server/src/programs/email_deliver.rs(3 hunks)services/main-frontend/src/components/forms/DeleteUserAccountForm.tsx(1 hunks)services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx(1 hunks)services/main-frontend/src/components/forms/VerifyPasswordForm.tsx(1 hunks)shared-module/packages/common/src/components/InputFields/OneTimePasscodeField.tsx(1 hunks)shared-module/packages/common/src/locales/en/main-frontend.json(7 hunks)shared-module/packages/common/src/locales/fi/main-frontend.json(7 hunks)shared-module/packages/common/src/services/backend/auth.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/headless-lms/models/.sqlx/query-7e5aee5c3efb4a45026448acfa6f88a811b40b4495cad020ff40191fb975202b.json
🚧 Files skipped from review as they are similar to previous changes (1)
- services/headless-lms/models/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
**/*.tsx: Make sure to use theuseTranslationhook for all text that is visible to the user.
If a component uses colors, use colors from the theme if possible. Example: import { baseTheme } from "@/shared-module/common/styles" ... background: ${baseTheme.colors.clear[100]};
If a component uses media queries, use the respondToOrLarger function. Example: import { respondToOrLarger } from "@/shared-module/common/styles/respond" ... ${respondToOrLarger.sm} { ... }
If component sends requests to server, use either useQuery, or useToastMutation hooks, or use hooks wrapping those hooks.
If there is a form of any kind, make sure it uses useForm from react-hook-form, and make sure the form is integrated with the useToastMutation and useQuery hooks.
For loading and error states, use the Spinner and ErrorBanner components from the shared module.
We have better replacements for window.confirm, window.alert, and window.prompt, use the DialogProvider and the useDialog hook from the shared module instead.
Files:
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsxshared-module/packages/common/src/components/InputFields/OneTimePasscodeField.tsxservices/main-frontend/src/components/forms/VerifyPasswordForm.tsxservices/main-frontend/src/components/forms/DeleteUserAccountForm.tsx
🧠 Learnings (1)
📚 Learning: 2024-11-14T14:07:21.540Z
Learnt from: nygrenh
PR: rage/secret-project-331#1335
File: services/main-frontend/src/components/page-specific/manage/courses/id/glossary/TermItem.tsx:88-94
Timestamp: 2024-11-14T14:07:21.540Z
Learning: In `services/main-frontend/src/components/page-specific/manage/courses/id/glossary/TermItem.tsx`, when validating form inputs using `react-hook-form`, prefer to keep using the regex pattern `/\S+/` for required fields, and do not change it to a stricter validation that trims whitespace.
Applied to files:
services/main-frontend/src/components/forms/DeleteUserAccountForm.tsx
🧬 Code graph analysis (7)
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx (2)
shared-module/packages/common/src/components/InputFields/OneTimePasscodeField.tsx (1)
OneTimePassCodeField(11-133)shared-module/packages/common/src/styles/theme.ts (1)
baseTheme(11-96)
shared-module/packages/common/src/services/backend/auth.ts (1)
shared-module/packages/common/src/utils/fetching.ts (2)
validateResponse(10-29)isBoolean(56-58)
shared-module/packages/common/src/components/InputFields/OneTimePasscodeField.tsx (1)
shared-module/packages/common/src/styles/theme.ts (1)
baseTheme(11-96)
services/main-frontend/src/components/forms/VerifyPasswordForm.tsx (1)
shared-module/packages/common/src/styles/theme.ts (1)
baseTheme(11-96)
services/headless-lms/server/src/programs/email_deliver.rs (4)
services/headless-lms/models/src/user_passwords.rs (1)
get_unused_reset_password_token_with_user_id(168-196)services/headless-lms/models/src/email_deliveries.rs (2)
fetch_emails(51-76)save_err_to_email(93-112)services/headless-lms/utils/src/email_processor.rs (2)
process_content_to_plaintext(62-97)process_content_to_html(99-128)services/headless-lms/models/src/user_email_codes.rs (1)
get_unused_user_email_code_with_user_id(53-82)
services/headless-lms/server/src/controllers/auth.rs (7)
services/headless-lms/server/src/domain/authorization.rs (9)
session(75-75)session(117-117)session(158-158)upstream_id(60-62)skip_authorize(275-277)remember(142-154)authenticate_test_user(956-1011)authenticate_moocfi_user(719-734)forget(169-171)services/headless-lms/utils/src/tmc/mod.rs (1)
post_new_user_to_tmc(184-212)services/headless-lms/models/src/users.rs (4)
insert_with_upstream_id_and_moocfi_id(51-91)get_by_id(125-138)get_by_email(93-107)delete_user(256-271)services/headless-lms/models/src/user_passwords.rs (4)
hash_password(49-61)upsert_user_password(27-47)verify_user_password(63-109)check_if_users_password_is_stored(111-128)services/headless-lms/models/src/email_templates.rs (1)
get_generic_email_template_by_name_and_language(54-82)services/headless-lms/models/src/user_email_codes.rs (4)
get_unused_user_email_code_with_user_id(53-82)insert_user_email_code(15-51)is_reset_user_email_code_valid(84-108)mark_user_email_code_used(110-131)services/headless-lms/models/src/email_deliveries.rs (1)
insert_email_delivery(25-49)
services/main-frontend/src/components/forms/DeleteUserAccountForm.tsx (1)
shared-module/packages/common/src/services/backend/auth.ts (2)
sendEmailCode(43-54)deleteUserAccount(56-61)
🔇 Additional comments (16)
shared-module/packages/common/src/locales/en/main-frontend.json (3)
94-94: LGTM for new keys.New keys fit existing naming convention and flow.
Also applies to: 152-152
367-370: LGTM for error messages.Clear and consistent with existing style.
917-918: LGTM for title.Matches route/page purpose.
shared-module/packages/common/src/locales/fi/main-frontend.json (5)
231-233: LGTM (password reset copy).Wording is natural and matches EN intent.
94-94: LGTM for new keys.Consistent with existing terminology.
Also applies to: 144-146
356-359: LGTM (error messages).Short and clear.
704-706: LGTM (resend + expired).Correct and user-friendly.
895-896: LGTM (title).Appropriate Finnish for the page title.
services/headless-lms/server/src/programs/email_deliver.rs (2)
20-21: Good: reset URL now configurable (with sensible default).Using FRONTEND_BASE_URL with a sane default removes the hardcoded local URL risk.
121-126: No action needed: the backend email link uses/reset-user-password/{token}, matching the existingpages/reset-user-password/[id].tsxfrontend route.services/headless-lms/models/.sqlx/query-b1f5e7cb1a58fb9d07b75208a5fc5157a194e6f098ada9f8eb63d3eb4a5b1c9d.json (1)
1-12: LGTM: soft-delete active codes by user_idQuery is parameterized and matches the likely partial unique index on (user_id) where deleted_at IS NULL. No changes needed.
services/headless-lms/models/.sqlx/query-4df53707e69ba703ee89d364b428b7b13d0287de2cda81404614d198857ff3df.json (1)
1-12: LGTM: mark code used + soft-deleteParameterized update with correct guards. Rely on rows_affected for success checks in Rust.
shared-module/packages/common/src/services/backend/auth.ts (1)
43-61: LGTM: thin wrappers with runtime validationConsistent with existing patterns (validateResponse + isBoolean). Endpoints and payloads look correct.
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx (1)
6-6: Ignore incorrect import suggestion; the file path and default export (OneTimePassCodeField) are correct.Likely an incorrect or invalid review comment.
services/headless-lms/migrations/20250902111139_add_user_email_codes.up.sql (1)
15-17: Index strategy looks goodThe partial unique index on
(user_id, code)with conditionsWHERE deleted_at IS NULL AND used_at IS NULLis an excellent approach. It ensures uniqueness only for active codes while allowing historical records.services/headless-lms/server/src/controllers/auth.rs (1)
20-20: The existinguse rand::Rng;suffices; callingrand::rng()via its fully qualified path requires nouse rand::rng.Likely an incorrect or invalid review comment.
| #[allow(clippy::async_yields_async)] | ||
| pub async fn send_delete_user_email_code( | ||
| auth_user: Option<AuthUser>, | ||
| pool: web::Data<PgPool>, | ||
| payload: web::Json<SendEmailCodeData>, | ||
| ) -> ControllerResult<web::Json<bool>> { | ||
| let token = skip_authorize(); | ||
|
|
||
| // Check user credentials | ||
| if let Some(auth_user) = auth_user { | ||
| let mut conn = pool.acquire().await?; | ||
|
|
||
| let password_ok = user_passwords::verify_user_password( | ||
| &mut conn, | ||
| auth_user.id, | ||
| &SecretString::new(payload.password.clone().into()), | ||
| ) | ||
| .await?; | ||
|
|
||
| if !password_ok { | ||
| info!( | ||
| "User {} attempted account deletion with incorrect password", | ||
| auth_user.id | ||
| ); | ||
|
|
||
| return token.authorized_ok(web::Json(false)); | ||
| } | ||
|
|
||
| let language = &payload.language; | ||
|
|
||
| // Get user deletion email template | ||
| let delete_template = | ||
| match models::email_templates::get_generic_email_template_by_name_and_language( | ||
| &mut conn, | ||
| "delete-user-email", | ||
| language, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(t) => t, | ||
| Err(e) => { | ||
| warn!("No delete-user-email-code template available: {:?}", e); | ||
| return token.authorized_ok(web::Json(false)); | ||
| } | ||
| }; | ||
|
|
||
| let user = models::users::get_by_id(&mut conn, auth_user.id).await?; | ||
|
|
||
| let code = if let Some(existing) = | ||
| models::user_email_codes::get_unused_user_email_code_with_user_id( | ||
| &mut conn, | ||
| auth_user.id, | ||
| ) | ||
| .await? | ||
| { | ||
| existing.code | ||
| } else { | ||
| let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); | ||
| models::user_email_codes::insert_user_email_code( | ||
| &mut conn, | ||
| auth_user.id, | ||
| new_code.clone(), | ||
| ) | ||
| .await?; | ||
| new_code | ||
| }; | ||
|
|
||
| models::user_email_codes::insert_user_email_code(&mut conn, auth_user.id, code.clone()) | ||
| .await?; | ||
| let _ = | ||
| models::email_deliveries::insert_email_delivery(&mut conn, user.id, delete_template.id) | ||
| .await?; | ||
|
|
||
| return token.authorized_ok(web::Json(true)); | ||
| } | ||
| token.authorized_ok(web::Json(false)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add rate limiting to prevent email code spam.
The endpoint lacks rate limiting, which could allow users to spam email codes. Consider adding rate limiting middleware or implementing a cooldown period.
Add rate limiting to the route in the _add_routes function:
.service(
web::resource("/send-email-code")
.wrap(build_rate_limiting_middleware(Duration::from_secs(60), 3)) // Max 3 requests per minute
.wrap(build_rate_limiting_middleware(Duration::from_secs(300), 10)) // Max 10 requests per 5 minutes
.to(send_delete_user_email_code),
)🤖 Prompt for AI Agents
In services/headless-lms/server/src/controllers/auth.rs around lines 530 to 606,
the send_delete_user_email_code endpoint currently has no rate limiting and can
be abused to spam email codes; update the route registration in the _add_routes
function to wrap the /send-email-code resource with two rate-limiting middleware
instances (one enforcing a short cooldown, e.g. max 3 requests per 60 seconds,
and one enforcing a longer window, e.g. max 10 requests per 300 seconds), ensure
the build_rate_limiting_middleware helper is imported/available, and apply those
.wrap(...) middleware calls to the resource before attaching
.to(send_delete_user_email_code) so the endpoint enforces both short-term and
longer-term limits.
| { | ||
| existing.code | ||
| } else { | ||
| let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a cryptographically secure random number generator for email codes.
Using rand::rng().random_range() may not provide sufficient entropy for security-sensitive codes. Consider using a cryptographically secure random number generator or a more robust code generation approach.
- let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string();
+ use rand::{rngs::OsRng, Rng};
+ let new_code: String = OsRng.random_range(100_000..1_000_000).to_string();Or better yet, use a proper token generation library:
use rand::{distributions::Alphanumeric, rngs::OsRng, Rng};
let new_code: String = OsRng
.sample_iter(&Alphanumeric)
.take(8)
.map(char::from)
.collect();🤖 Prompt for AI Agents
In services/headless-lms/server/src/controllers/auth.rs around line 587, the
current generation of new_code uses rand::rng().random_range which is not
cryptographically secure; replace it with a cryptographically secure token
generator (e.g., use rand::rngs::OsRng with rand::distributions::Alphanumeric to
sample an appropriate length alphanumeric string or another secure token
library), update imports accordingly, and ensure the generated code length meets
security requirements and is returned/handled the same way as the original
value.
| let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); | ||
| models::user_email_codes::insert_user_email_code( | ||
| &mut conn, | ||
| auth_user.id, | ||
| new_code.clone(), | ||
| ) | ||
| .await?; | ||
| new_code | ||
| }; |
There was a problem hiding this comment.
Remove duplicate code insertion.
Lines 587-595 generate a new code and insert it, but then lines 597-599 insert the same code again. This creates duplicate entries and wastes database operations.
let code = if let Some(existing) =
models::user_email_codes::get_unused_user_email_code_with_user_id(
&mut conn,
auth_user.id,
)
.await?
{
existing.code
} else {
let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string();
models::user_email_codes::insert_user_email_code(
&mut conn,
auth_user.id,
new_code.clone(),
)
.await?;
new_code
};
- models::user_email_codes::insert_user_email_code(&mut conn, auth_user.id, code.clone())
- .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 new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); | |
| models::user_email_codes::insert_user_email_code( | |
| &mut conn, | |
| auth_user.id, | |
| new_code.clone(), | |
| ) | |
| .await?; | |
| new_code | |
| }; | |
| let code = if let Some(existing) = | |
| models::user_email_codes::get_unused_user_email_code_with_user_id( | |
| &mut conn, | |
| auth_user.id, | |
| ) | |
| .await? | |
| { | |
| existing.code | |
| } else { | |
| let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); | |
| models::user_email_codes::insert_user_email_code( | |
| &mut conn, | |
| auth_user.id, | |
| new_code.clone(), | |
| ) | |
| .await?; | |
| new_code | |
| }; | |
| // Removed duplicate insertion of `code` here |
🤖 Prompt for AI Agents
In services/headless-lms/server/src/controllers/auth.rs around lines 587-595
(and the redundant insertion at 597-599), a new_code is generated and inserted
into user_email_codes, then inserted again a few lines later, causing duplicate
DB entries; remove the second insertion (or make it conditional) and ensure the
code generated at 587-595 is the single value inserted and returned/reused
thereafter so only one insert_user_email_code call is executed.
| #[instrument(skip(pool, payload, auth_user, session))] | ||
| #[allow(clippy::async_yields_async)] | ||
| pub async fn delete_user_account( | ||
| auth_user: Option<AuthUser>, | ||
| pool: web::Data<PgPool>, | ||
| payload: web::Json<EmailCode>, | ||
| session: Session, | ||
| ) -> ControllerResult<web::Json<bool>> { | ||
| let token = skip_authorize(); | ||
| // Check users code is valid | ||
| if let Some(auth_user) = auth_user { | ||
| let mut conn = pool.acquire().await?; | ||
|
|
||
| let code_ok = user_email_codes::is_reset_user_email_code_valid( | ||
| &mut conn, | ||
| auth_user.id, | ||
| &payload.code, | ||
| ) | ||
| .await?; | ||
|
|
||
| if code_ok { | ||
| let _ = users::delete_user(&mut conn, auth_user.id).await; | ||
| let _ = | ||
| user_email_codes::mark_user_email_code_used(&mut conn, auth_user.id, &payload.code) | ||
| .await; | ||
| authorization::forget(&session); | ||
| return token.authorized_ok(web::Json(true)); | ||
| } else { | ||
| info!( | ||
| "User {} attempted account deletion with incorrect code", | ||
| auth_user.id | ||
| ); | ||
|
|
||
| return token.authorized_ok(web::Json(false)); | ||
| } | ||
| } else { | ||
| return token.authorized_ok(web::Json(false)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add rate limiting to account deletion endpoint.
Similar to the email code endpoint, this lacks rate limiting which could enable brute force attacks on the verification codes.
Add rate limiting to the route in the _add_routes function:
.service(
web::resource("/delete-user-account")
.wrap(build_rate_limiting_middleware(Duration::from_secs(60), 5)) // Max 5 attempts per minute
.to(delete_user_account),
)🤖 Prompt for AI Agents
In services/headless-lms/server/src/controllers/auth.rs around lines 617 to 655,
the delete_user_account endpoint lacks rate limiting which allows brute-force
attempts on verification codes; update the route registration in the _add_routes
function to wrap the /delete-user-account resource with the rate limiting
middleware (e.g., .wrap(build_rate_limiting_middleware(Duration::from_secs(60),
5))) so it caps attempts (5 per minute), and ensure any required imports for
build_rate_limiting_middleware and Duration are added at the top of the file or
module.
| {credentialsError && ( | ||
| <div | ||
| aria-live="assertive" | ||
| className={css` | ||
| padding: 10px; | ||
| border: 2px solid ${baseTheme.colors.red[500]}; | ||
| font-weight: bold; | ||
| color: ${baseTheme.colors.red[500]}; | ||
| `} | ||
| > | ||
| {t("incorrect-password")} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Fix missing aria-describedby target id and use shared ErrorBanner
Currently aria-describedby points to "password-error" but the element lacks id, breaking accessibility. Also prefer the shared ErrorBanner per guidelines.
- {credentialsError && (
- <div
- aria-live="assertive"
- className={css`
- padding: 10px;
- border: 2px solid ${baseTheme.colors.red[500]};
- font-weight: bold;
- color: ${baseTheme.colors.red[500]};
- `}
- >
- {t("incorrect-password")}
- </div>
- )}
+ {credentialsError && (
+ <ErrorBanner id="password-error" aria-live="assertive">
+ {t("incorrect-password")}
+ </ErrorBanner>
+ )}Add import:
import Button from "@/shared-module/common/components/Button"
import TextField from "@/shared-module/common/components/InputFields/TextField"
import { baseTheme } from "@/shared-module/common/styles"
+import ErrorBanner from "@/shared-module/common/components/ErrorBanner"📝 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.
| {credentialsError && ( | |
| <div | |
| aria-live="assertive" | |
| className={css` | |
| padding: 10px; | |
| border: 2px solid ${baseTheme.colors.red[500]}; | |
| font-weight: bold; | |
| color: ${baseTheme.colors.red[500]}; | |
| `} | |
| > | |
| {t("incorrect-password")} | |
| </div> | |
| )} | |
| // …other imports | |
| import Button from "@/shared-module/common/components/Button" | |
| import TextField from "@/shared-module/common/components/InputFields/TextField" | |
| import { baseTheme } from "@/shared-module/common/styles" | |
| import ErrorBanner from "@/shared-module/common/components/ErrorBanner" | |
| // …inside your JSX render: | |
| - {credentialsError && ( | |
| - <div | |
| - aria-live="assertive" | |
| - className={css` | |
| - padding: 10px; | |
| - border: 2px solid ${baseTheme.colors.red[500]}; | |
| - font-weight: bold; | |
| - color: ${baseTheme.colors.red[500]}; | |
| - `} | |
| - > | |
| - {t("incorrect-password")} | |
| - </div> | |
| {credentialsError && ( | |
| <ErrorBanner id="password-error" aria-live="assertive"> | |
| {t("incorrect-password")} | |
| </ErrorBanner> | |
| )} |
🤖 Prompt for AI Agents
In services/main-frontend/src/components/forms/VerifyPasswordForm.tsx around
lines 49-61, the error block is missing the id referenced by aria-describedby
and should use the shared ErrorBanner component; replace the current div with
the shared ErrorBanner and add the import for it at the top of the file (e.g.
import ErrorBanner from 'components/common/ErrorBanner' or the correct path in
this repo), ensure the ErrorBanner instance includes id="password-error" and
renders t("incorrect-password") so aria-describedby points to a real element and
the shared styling/component is used.
| "delete-account-did-not-recieve-email": "Didn't recieve code?", | ||
| "delete-account-info": "Deleting your account is permanent. All your data, progress, and personal information will be removed and cannot be recovered.", | ||
| "delete-account-info-2": "To delete your account, enter your password below and we will send a verification code to your email.", | ||
| "delete-chatbot-confirmation": "Are you sure you want to delete the chatbot \"{{name}}\"?", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix typos in key and value: "receive" (breaking i18n lookups).
Rename the key and fix the English copy. Also update all call sites and other locales accordingly.
- "delete-account-did-not-recieve-email": "Didn't recieve code?",
+ "delete-account-did-not-receive-email": "Didn't receive code?",Run to locate usages and ensure no stale keys remain:
🏁 Script executed:
#!/bin/bash
# Find old, misspelled keys anywhere in the repo
rg -nP 'delete-account-did-not-recieve-email|insert-single-use-code-accoutnt-deletion'
# Confirm corrected keys now exist in locales
rg -n 'delete-account-did-not-receive-email|insert-single-use-code-account-deletion' -- shared-module/packages/common/src/localesLength of output: 994
Rename misspelled i18n keys and update all usages
- In
shared-module/packages/common/src/locales/*/main-frontend.json, rename
•"delete-account-did-not-recieve-email"→"delete-account-did-not-receive-email"
•"insert-single-use-code-accoutnt-deletion"→"insert-single-use-code-account-deletion" - In
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx(and any other call sites), updatet("…")to use the corrected keys. - Run to confirm no stale keys remain and new keys exist:
rg -nP 'delete-account-did-not-recieve-email|insert-single-use-code-accoutnt-deletion'
rg -n 'delete-account-did-not-receive-email|insert-single-use-code-account-deletion' -- shared-module/packages/common/src/locales🤖 Prompt for AI Agents
In shared-module/packages/common/src/locales/en/main-frontend.json around lines
198-201, two i18n keys are misspelled and must be renamed and all usages
updated: change "delete-account-did-not-recieve-email" to
"delete-account-did-not-receive-email" and change
"insert-single-use-code-accoutnt-deletion" to
"insert-single-use-code-account-deletion" across every locale file under
shared-module/packages/common/src/locales and update all t("...") calls (e.g.,
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx and any
other call sites) to the corrected keys; after changes run the provided ripgrep
checks to ensure the old keys no longer appear and the new keys exist in the
locale files.
shared-module/packages/common/src/locales/en/main-frontend.json
Outdated
Show resolved
Hide resolved
| "delete-account-did-not-recieve-email": "Etkö saanut koodia?", | ||
| "delete-account-info": "Tilisi poistaminen on pysyvää. Kaikki tietosi, edistymisesi ja henkilökohtaiset tietosi poistetaan eikä niitä voida palauttaa.", | ||
| "delete-account-info-2": "Poistaaksesi tilisi, syötä salasanasi alle. Lähetämme vahvistuskoodin sähköpostiisi.", | ||
| "delete-chatbot-confirmation": "Haluatko varmasti poistaa keskustelurobotin nimeltä \"{{name}}\"?", |
There was a problem hiding this comment.
Fix key typo: "recieve" → "receive" (breaking i18n lookups).
Rename the key to keep parity with EN and avoid lookup misses. Value is fine.
- "delete-account-did-not-recieve-email": "Etkö saanut koodia?",
+ "delete-account-did-not-receive-email": "Etkö saanut koodia?",📝 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.
| "delete-account-did-not-recieve-email": "Etkö saanut koodia?", | |
| "delete-account-info": "Tilisi poistaminen on pysyvää. Kaikki tietosi, edistymisesi ja henkilökohtaiset tietosi poistetaan eikä niitä voida palauttaa.", | |
| "delete-account-info-2": "Poistaaksesi tilisi, syötä salasanasi alle. Lähetämme vahvistuskoodin sähköpostiisi.", | |
| "delete-chatbot-confirmation": "Haluatko varmasti poistaa keskustelurobotin nimeltä \"{{name}}\"?", | |
| "delete-account-did-not-receive-email": "Etkö saanut koodia?", | |
| "delete-account-info": "Tilisi poistaminen on pysyvää. Kaikki tietosi, edistymisesi ja henkilökohtaiset tietosi poistetaan eikä niitä voida palauttaa.", | |
| "delete-account-info-2": "Poistaaksesi tilisi, syötä salasanasi alle. Lähetämme vahvistuskoodin sähköpostiisi.", | |
| "delete-chatbot-confirmation": "Haluatko varmasti poistaa keskustelurobotin nimeltä \"{{name}}\"?", |
🤖 Prompt for AI Agents
In shared-module/packages/common/src/locales/fi/main-frontend.json around lines
187 to 190, the translation key "delete-account-did-not-recieve-email" contains
a typo ("recieve"); rename the key to "delete-account-did-not-receive-email"
(keep the Finnish value unchanged) and update any references to this key across
the codebase and other locale files to match the corrected key so i18n lookups
remain consistent with EN.
shared-module/packages/common/src/locales/fi/main-frontend.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/headless-lms/models/.sqlx/query-2fd273e366f4c948c3a665f218968ca42c344b59ad1abc2fa2993871d21ee4d0.json (2)
3-3: Indexing to support the new access pattern.Add a partial index to keep this LIMIT 1 lookup fast and avoid full scans:
Suggested migration:
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_email_templates_name_ci_lang_not_deleted ON email_templates (name, course_instance_id, language) WHERE deleted_at IS NULL;If duplicates must be prevented, prefer a partial UNIQUE index (covers both generic and CI-specific rows):
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS uq_email_templates_name_ci_lang_not_deleted ON email_templates (name, course_instance_id, language) WHERE deleted_at IS NULL;
3-3: Edge case when language = 'en'.When
$2 = 'en', the WHERE duplicates the same candidate twice conceptually. It works due to the ORDER BY, but you can simplify:Alternative ordering without CASE:
ORDER BY (language = $2) DESC, (language = 'en') DESC, (language IS NULL) DESC, updated_at DESC(Keep the current version if you value readability over micro-optimization.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
services/headless-lms/models/.sqlx/query-0e7f48cbb7ab7af479c25d9c41bd9c906f9bde414eaff07f8a4b9a1725e29928.json(1 hunks)services/headless-lms/models/.sqlx/query-247b57ac933ff6ed9d2a70daf2260c53c52f3fae1f1cb9d7212334837b909b1a.json(1 hunks)services/headless-lms/models/.sqlx/query-2fd273e366f4c948c3a665f218968ca42c344b59ad1abc2fa2993871d21ee4d0.json(3 hunks)services/headless-lms/models/.sqlx/query-352f3f2d38b17b63a7620cc7b4c8b657264fe751852d72b38262fa0421093abb.json(1 hunks)services/headless-lms/models/.sqlx/query-36fd69e14ef3a76c35af986ccfd142f6aa5f02fe191ef420f1246d4a5c217cc2.json(1 hunks)services/headless-lms/models/.sqlx/query-3c83c42e752bc8f58d93cc7f32adaac035d7a85d44900da6134a456f69f7e83f.json(1 hunks)services/headless-lms/models/.sqlx/query-4df53707e69ba703ee89d364b428b7b13d0287de2cda81404614d198857ff3df.json(1 hunks)services/headless-lms/models/.sqlx/query-57e727a18843dc84f55d422b69f9e5c32345b981879b2d639bdd0212e2120878.json(1 hunks)services/headless-lms/models/.sqlx/query-625f56ca4ce2b855c400327266a8791c8284a8f95614784034bad5d8e7a8bed1.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- services/headless-lms/models/.sqlx/query-3c83c42e752bc8f58d93cc7f32adaac035d7a85d44900da6134a456f69f7e83f.json
- services/headless-lms/models/.sqlx/query-4df53707e69ba703ee89d364b428b7b13d0287de2cda81404614d198857ff3df.json
- services/headless-lms/models/.sqlx/query-625f56ca4ce2b855c400327266a8791c8284a8f95614784034bad5d8e7a8bed1.json
- services/headless-lms/models/.sqlx/query-36fd69e14ef3a76c35af986ccfd142f6aa5f02fe191ef420f1246d4a5c217cc2.json
- services/headless-lms/models/.sqlx/query-352f3f2d38b17b63a7620cc7b4c8b657264fe751852d72b38262fa0421093abb.json
- services/headless-lms/models/.sqlx/query-247b57ac933ff6ed9d2a70daf2260c53c52f3fae1f1cb9d7212334837b909b1a.json
- services/headless-lms/models/.sqlx/query-57e727a18843dc84f55d422b69f9e5c32345b981879b2d639bdd0212e2120878.json
- services/headless-lms/models/.sqlx/query-0e7f48cbb7ab7af479c25d9c41bd9c906f9bde414eaff07f8a4b9a1725e29928.json
⏰ 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). (1)
- GitHub Check: code-style
🔇 Additional comments (4)
services/headless-lms/models/.sqlx/query-2fd273e366f4c948c3a665f218968ca42c344b59ad1abc2fa2993871d21ee4d0.json (4)
3-3: LGTM overall.The fallback logic (exact -> en -> NULL), soft-delete guard, and LIMIT 1 are correct.
56-60: No change required — nullability already mapped.
services/headless-lms/models/src/email_templates.rs containspub language: Option<String>andpub course_instance_id: Option<Uuid>.
78-79: Migrations verified — course_instance_id made nullable; language column added (nullable, no backfill).Found in services/headless-lms/migrations/20250721091109_add-password-reset-tokens.up.sql:
ALTER COLUMN email_templates.course_instance_id DROP NOT NULL; ADD COLUMN language VARCHAR(255); CREATE UNIQUE INDEX IF NOT EXISTS unique_email_templates_name_language_general ON email_templates(name, language) WHERE course_instance_id IS NULL. The down migration deletes rows WHERE course_instance_id IS NULL before SET NOT NULL and drops language.
- No default or data backfill for language detected; add a backfill/default or confirm nullable/no-backfill is intentional.
64-66: Param type change to Text is OK — call sites verified.Function accepts &str for both params; callers pass a string literal and &payload.language (coerces to &str).
- services/headless-lms/models/src/email_templates.rs:54-57
- services/headless-lms/server/src/controllers/auth.rs:563
- services/headless-lms/server/src/controllers/main_frontend/users.rs:188-191
| { | ||
| "db_name": "PostgreSQL", | ||
| "query": "\nINSERT INTO email_templates (name, course_instance_id, subject)\nVALUES ($1, $2, $3)\nRETURNING *\n", | ||
| "query": "\nSELECT *\nFROM email_templates\nWHERE name = $1\n AND course_instance_id IS NULL\n AND deleted_at IS NULL\n AND (language = $2 OR language = 'en' OR language IS NULL)\nORDER BY CASE\n WHEN language = $2 THEN 0\n WHEN language = 'en' THEN 1\n WHEN language IS NULL THEN 2\n ELSE 3\n END\nLIMIT 1\n ", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good language-aware fallback; tighten selection and stability.
- Recommend explicit column list instead of
SELECT *to stabilize the SQLx contract against schema drift. - Add a deterministic tie-breaker (e.g., newest template first) to avoid non-deterministic picks when duplicates exist at the same priority.
Apply:
- "query": "\nSELECT *\nFROM email_templates\nWHERE name = $1\n AND course_instance_id IS NULL\n AND deleted_at IS NULL\n AND (language = $2 OR language = 'en' OR language IS NULL)\nORDER BY CASE\n WHEN language = $2 THEN 0\n WHEN language = 'en' THEN 1\n WHEN language IS NULL THEN 2\n ELSE 3\n END\nLIMIT 1\n ",
+ "query": "\nSELECT id, created_at, updated_at, deleted_at, content, name, subject, exercise_completions_threshold, points_threshold, course_instance_id, language\nFROM email_templates\nWHERE name = $1\n AND course_instance_id IS NULL\n AND deleted_at IS NULL\n AND (language = $2 OR language = 'en' OR language IS NULL)\nORDER BY CASE\n WHEN language = $2 THEN 0\n WHEN language = 'en' THEN 1\n WHEN language IS NULL THEN 2\n ELSE 3\n END,\n updated_at DESC\nLIMIT 1\n ",📝 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.
| "query": "\nSELECT *\nFROM email_templates\nWHERE name = $1\n AND course_instance_id IS NULL\n AND deleted_at IS NULL\n AND (language = $2 OR language = 'en' OR language IS NULL)\nORDER BY CASE\n WHEN language = $2 THEN 0\n WHEN language = 'en' THEN 1\n WHEN language IS NULL THEN 2\n ELSE 3\n END\nLIMIT 1\n ", | |
| "query": "\nSELECT id, created_at, updated_at, deleted_at, content, name, subject, exercise_completions_threshold, points_threshold, course_instance_id, language\nFROM email_templates\nWHERE name = $1\n AND course_instance_id IS NULL\n AND deleted_at IS NULL\n AND (language = $2 OR language = 'en' OR language IS NULL)\nORDER BY CASE\n WHEN language = $2 THEN 0\n WHEN language = 'en' THEN 1\n WHEN language IS NULL THEN 2\n ELSE 3\n END,\n updated_at DESC\nLIMIT 1\n ", |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
shared-module/packages/common/src/bindings.ts (1)
648-650: EmailTemplateNew — server accepts omitted/null; align TS type/guard
- Verified: services/headless-lms/models/src/email_templates.rs defines EmailTemplateNew.content as Option<serde_json::Value>, so serde treats both missing and explicit null as None — server will not 400 on create.
- Action: align TypeScript runtime and types — change shared-module/packages/common/src/bindings.ts to content?: unknown | null or update shared-module/packages/common/src/bindings.guard.ts (isEmailTemplateNew) to accept undefined for content so the generated TS accurately reflects the server contract.
shared-module/packages/common/src/bindings.guard.ts (1)
1338-1341: Guard doesn’t assert presence of content for EmailTemplateNewTyping requires content: unknown | null, but the guard doesn’t check it at all. If you rely on this guard to validate untyped inputs, objects missing content will pass. If intentional (generator skips unknown), ignore; otherwise, add an existence/null check.
Apply if you want the guard to require the property (still allowing any value or null):
export function isEmailTemplateNew(obj: unknown): obj is EmailTemplateNew { const typedObj = obj as EmailTemplateNew return ( (typedObj !== null && typeof typedObj === "object" || typeof typedObj === "function") && typeof typedObj["name"] === "string" && (typedObj["language"] === null || - typeof typedObj["language"] === "string") + typeof typedObj["language"] === "string") && + // Require presence of the property; allow any value or null. + ("content" in typedObj) && + (typedObj["content"] === null || true) ) }Note: if this file is fully generated, adjust the generator instead of hand-editing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
system-tests/src/__screenshots__/proposed-edits/proposed-edits.spec.ts/preview-desktop-regular.pngis excluded by!**/*.png
📒 Files selected for processing (4)
shared-module/packages/common/src/bindings.guard.ts(2 hunks)shared-module/packages/common/src/bindings.ts(1 hunks)shared-module/packages/common/src/locales/en/main-frontend.json(7 hunks)shared-module/packages/common/src/locales/fi/main-frontend.json(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- shared-module/packages/common/src/locales/en/main-frontend.json
- shared-module/packages/common/src/locales/fi/main-frontend.json
⏰ 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). (7)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build-and-system-tests
- GitHub Check: build-and-deploy
- GitHub Check: course-material
- GitHub Check: headless_lms
- GitHub Check: cms
- GitHub Check: code-style
🔇 Additional comments (2)
shared-module/packages/common/src/bindings.ts (1)
642-644: EmailTemplate: nullability and language field verifiedbindings.guard.ts now accepts null for course_instance_id and language; cms edit page guards course_instance_id (templateQuery.data?.course_instance_id + assertNotNullOrUndefined) before fetching; manage course-instances page supplies a non-null route id for creates and passes language: null for new templates — no callers found that assume course_instance_id is always a string.
shared-module/packages/common/src/bindings.guard.ts (1)
1325-1329: Guard updated to accept nullable course_instance_id and languageThis aligns with the typings change and will prevent false negatives at runtime. Good.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
services/headless-lms/server/src/programs/seed/seed_generic_emails.rs (4)
112-155: Add Finnish variant for delete-account email for i18n parity.You seed FI for reset but not for deletion. Consider adding "fi" subject/body to keep UX consistent.
11-17: Prefer taking &Pool instead of Pool.Passing the pool by reference avoids unnecessary clones and aligns with common SQLx patterns. Requires updating call sites.
Suggested change here:
-pub async fn seed_generic_emails( - db_pool: Pool<Postgres>, +pub async fn seed_generic_emails( + db_pool: &Pool<Postgres>, seed_users_result: SeedUsersResult, ) -> anyhow::Result<()> { - let mut conn = db_pool.acquire().await?; + let mut conn = db_pool.acquire().await?;
17-101: Consider atomic seeding with a transaction.If one insert fails, earlier templates remain written. Wrapping these inserts in a single transaction improves seed reliability. This likely requires refactoring model fns to accept a generic Executor (e.g., E: sqlx::Executor<Database = Postgres>) instead of &mut PgConnection.
Also applies to: 114-155
53-55: De‑dup template name literals with constants.Reduces typo risk and keeps names consistent across seeds.
Apply this diff:
use uuid::Uuid; +const RESET_PASSWORD_TEMPLATE: &str = "reset-password-email"; +const DELETE_USER_TEMPLATE: &str = "delete-user-email"; + ... - let english_template = EmailTemplateNew { - name: "reset-password-email".to_string(), + let english_template = EmailTemplateNew { + name: RESET_PASSWORD_TEMPLATE.to_string(), ... - let finnish_template = EmailTemplateNew { - name: "reset-password-email".to_string(), + let finnish_template = EmailTemplateNew { + name: RESET_PASSWORD_TEMPLATE.to_string(), ... - let delete_template = EmailTemplateNew { - name: "delete-user-email".to_string(), + let delete_template = EmailTemplateNew { + name: DELETE_USER_TEMPLATE.to_string(),Also applies to: 96-97, 149-150
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/headless-lms/server/src/programs/seed/mod.rs(2 hunks)services/headless-lms/server/src/programs/seed/seed_generic_emails.rs(1 hunks)services/main-frontend/src/components/forms/VerifyPasswordForm.tsx(1 hunks)shared-module/packages/common/src/locales/en/main-frontend.json(7 hunks)shared-module/packages/common/src/locales/fi/main-frontend.json(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- services/headless-lms/server/src/programs/seed/mod.rs
- services/main-frontend/src/components/forms/VerifyPasswordForm.tsx
- shared-module/packages/common/src/locales/fi/main-frontend.json
- shared-module/packages/common/src/locales/en/main-frontend.json
🧰 Additional context used
🧬 Code graph analysis (1)
services/headless-lms/server/src/programs/seed/seed_generic_emails.rs (3)
services/headless-lms/models/src/email_templates.rs (1)
insert_email_template(84-118)services/headless-lms/models/src/user_passwords.rs (1)
insert_password_reset_token(130-166)services/headless-lms/server/src/programs/seed/seed_users.rs (1)
seed_users(28-269)
⏰ 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). (5)
- GitHub Check: build-and-deploy
- GitHub Check: build-and-system-tests
- GitHub Check: headless_lms
- GitHub Check: cms
- GitHub Check: code-style
🔇 Additional comments (1)
services/headless-lms/server/src/programs/seed/seed_generic_emails.rs (1)
53-57: OK — template names and placeholders match.
Both "reset-password-email" and "delete-user-email" are used as lookup keys (email_deliver.rs/controllers) and the seeded templates use matching placeholders ({{RESET_LINK}} for reset, {{CODE}} for delete).
services/headless-lms/server/src/programs/seed/seed_generic_emails.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
services/headless-lms/server/src/controllers/auth.rs (4)
588-588: Use cryptographically secure random generator for codesUsing
rand::rng().random_range()may not provide sufficient entropy for security-sensitive verification codes.Apply this fix:
- let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); + use rand::{rngs::OsRng, Rng}; + let new_code: String = OsRng.random_range(100_000..1_000_000).to_string();
588-599: Remove duplicate code insertionLines 588-595 insert a new code, but lines 598-599 insert the same code again, creating duplicate database entries.
Apply this fix:
let code = if let Some(existing) = models::user_email_codes::get_unused_user_email_code_with_user_id( &mut conn, auth_user.id, ) .await? { existing.code } else { let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); models::user_email_codes::insert_user_email_code( &mut conn, auth_user.id, new_code.clone(), ) .await?; new_code }; - models::user_email_codes::insert_user_email_code(&mut conn, auth_user.id, code.clone()) - .await?;
531-607: Add rate limiting to prevent email code spamThe
/send-email-codeendpoint lacks rate limiting, allowing potential abuse.Update the route registration in
_add_routesaround line 762:.route( "/send-email-code", - web::post().to(send_delete_user_email_code), + web::post() + .wrap(build_rate_limiting_middleware(Duration::from_secs(60), 3)) // 3 per minute + .wrap(build_rate_limiting_middleware(Duration::from_secs(300), 10)) // 10 per 5 minutes + .to(send_delete_user_email_code), );
620-675: Add rate limiting to account deletion endpointThe
/delete-user-accountendpoint lacks rate limiting, enabling brute force attacks on verification codes.Update the route registration in
_add_routesaround line 760:- .route("/delete-user-account", web::post().to(delete_user_account)) + .route("/delete-user-account", web::post() + .wrap(build_rate_limiting_middleware(Duration::from_secs(60), 5)) // 5 attempts per minute + .to(delete_user_account))services/headless-lms/utils/src/tmc/mod.rs (1)
26-33: Fix TmcUserInfo struct to match TMC API responseThe TMC API returns a numeric
idfield, not a UUID. The struct also incorrectly defines bothid: Uuidandupstream_id: i32, which will cause JSON deserialization failures.Apply this fix:
-#[derive(Debug, Deserialize)] -pub struct TmcUserInfo { - pub id: Uuid, - pub email: String, - pub first_name: Option<String>, - pub last_name: Option<String>, - pub upstream_id: i32, -} +#[derive(Debug, Deserialize)] +pub struct TmcUserInfo { + // TMC's numeric user id mapped to upstream_id + #[serde(rename = "id")] + pub upstream_id: i64, + pub email: String, + pub first_name: Option<String>, + pub last_name: Option<String>, +}
🧹 Nitpick comments (1)
services/headless-lms/server/src/controllers/auth.rs (1)
664-669: Consider soft-deleting password data before user deletionThe password hash remains in
user_passwordstable after user deletion, which may be a PII retention concern.Consider soft-deleting the password before deleting the user:
// Delete user locally and mark email code as used + // Soft-delete stored password to avoid PII retention + sqlx::query!( + r#"UPDATE user_passwords SET deleted_at = NOW() WHERE user_id = $1"#, + auth_user.id + ) + .execute(&mut *tx) + .await?; + users::delete_user(&mut tx, auth_user.id).await?; user_email_codes::mark_user_email_code_used(&mut tx, auth_user.id, &payload.code).await?;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/headless-lms/server/src/controllers/auth.rs(8 hunks)services/headless-lms/utils/src/tmc/mod.rs(6 hunks)shared-module/packages/common/src/locales/en/main-frontend.json(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shared-module/packages/common/src/locales/en/main-frontend.json
🧰 Additional context used
🧬 Code graph analysis (2)
services/headless-lms/utils/src/tmc/mod.rs (1)
services/headless-lms/server/src/controllers/auth.rs (1)
post_new_user_to_tmc(680-699)
services/headless-lms/server/src/controllers/auth.rs (7)
services/headless-lms/server/src/domain/authorization.rs (8)
session(75-75)session(117-117)session(158-158)upstream_id(60-62)skip_authorize(276-278)remember(142-154)authenticate_moocfi_user(720-735)forget(169-171)services/headless-lms/utils/src/tmc/mod.rs (1)
post_new_user_to_tmc(189-217)services/headless-lms/models/src/users.rs (4)
insert_with_upstream_id_and_moocfi_id(51-91)get_by_id(125-138)get_by_email(93-107)delete_user(256-271)services/headless-lms/models/src/user_passwords.rs (4)
hash_password(49-61)upsert_user_password(27-47)verify_user_password(63-109)check_if_users_password_is_stored(111-128)services/headless-lms/models/src/email_templates.rs (1)
get_generic_email_template_by_name_and_language(54-82)services/headless-lms/models/src/user_email_codes.rs (4)
get_unused_user_email_code_with_user_id(53-82)insert_user_email_code(15-51)is_reset_user_email_code_valid(84-108)mark_user_email_code_used(110-131)services/headless-lms/models/src/email_deliveries.rs (1)
insert_email_delivery(25-49)
⏰ 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). (5)
- GitHub Check: build-and-deploy
- GitHub Check: headless_lms
- GitHub Check: cms
- GitHub Check: build-and-system-tests
- GitHub Check: code-style
🔇 Additional comments (4)
services/headless-lms/utils/src/tmc/mod.rs (2)
126-136: Good PII protection in loggingThe URL redaction logic properly prevents email addresses and other query parameters from appearing in logs at the warn level, with full details relegated to debug level.
238-253: LGTM! Proper URL encoding for email parameterThe implementation correctly uses
Url::parseandquery_pairs_mut().append_pair()to ensure the email parameter is properly percent-encoded, avoiding issues with special characters like+or@.services/headless-lms/server/src/controllers/auth.rs (2)
139-165: Well-structured password management flowThe implementation properly:
- Hashes passwords using secure Argon2id algorithm
- Stores them locally
- Notifies TMC about password management handover
- Handles errors appropriately
378-462: Smart authentication fallback strategyThe production login flow intelligently:
- Checks local password first for efficiency
- Falls back to TMC authentication if needed
- Migrates passwords to local storage on successful TMC auth
- Properly handles upstream_id absence with a warning
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/headless-lms/server/src/programs/email_deliver.rs (2)
76-98: Replaceexpecton message build with persisted error; avoid panics.
expect("Failed to build email")can crash the worker. Persist and return Ok(()) instead.Apply:
- let msg = Message::builder() + let msg = match Message::builder() .from(SMTP_FROM.parse()?) .to(email .to .parse() .with_context(|| format!("Invalid address: {}", email_to))?) .subject(email.subject.context("No subject")?) .multipart( MultiPart::alternative() .singlepart( SinglePart::builder() .header(header::ContentType::TEXT_PLAIN) .body(msg_as_plaintext), ) .singlepart( SinglePart::builder() .header(header::ContentType::TEXT_HTML) .body(msg_as_html), ), - ) - // should never fail - .expect("Failed to build email"); + ) { + Ok(m) => m, + Err(err) => { + save_err_to_email(email.id, format!("Failed to build email: {err}"), &mut conn) + .await + .context("Couldn't save build-message err to db")?; + return Ok(()); + } + };
99-106: Blocking SMTP send on Tokio runtime — convert to async transport or offload.The code uses synchronous SmtpTransport::send inside async tasks (buffered with BATCH_SIZE) which will block the Tokio runtime at scale; lettre 0.11.18 is present but no tokio1 feature is enabled in Cargo.toml.
- Preferred: enable lettre's tokio1 feature and switch to AsyncSmtpTransport — update imports (AsyncSmtpTransport, AsyncTransport, Tokio1Executor), change function signatures to accept AsyncSmtpTransport, call mailer.send(msg).await, and build with AsyncSmtpTransport::::relay(...). Also add/adjust Cargo.toml (services/headless-lms/server/Cargo.toml) to enable features, e.g. lettre = { version = "0.11.18", features = ["tokio1"] }.
- Alternative (minimum): offload the blocking send with tokio::task::spawn_blocking (clone mailer and msg) and await the JoinHandle; handle join errors and propagate/save failures as now.
Actionable locations: services/headless-lms/server/src/programs/email_deliver.rs — imports (line ~10), mail_sender signature (line ~33), send_message signature (line ~54), blocking send at line ~99, mailer construction at line ~196; services/headless-lms/server/Cargo.toml lists lettre = "0.11.18" (no tokio1 feature found).
♻️ Duplicate comments (4)
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx (2)
5-7: Use shared ErrorBanner and add an id to match aria-describedby.Fixes a11y linkage and keeps UI consistent with shared components.
import Button from "@/shared-module/common/components/Button" import OneTimePassCodeField from "@/shared-module/common/components/InputFields/OneTimePasscodeField" import { baseTheme } from "@/shared-module/common/styles" +import ErrorBanner from "@/shared-module/common/components/ErrorBanner" @@ - {credentialsError && ( - <div - aria-live="assertive" - className={css` - padding: 10px; - border: 2px solid ${baseTheme.colors.red[500]}; - font-weight: bold; - color: ${baseTheme.colors.red[500]}; - `} - > - {t("incorrect-code")} - </div> - )} + {credentialsError && ( + <ErrorBanner id="code-error" aria-live="assertive" error={new Error(t("incorrect-code"))} /> + )}Also applies to: 44-55
40-41: LGTM: fixed translation key typo noted earlier.The i18n key now uses “account” correctly.
services/headless-lms/server/src/programs/email_deliver.rs (2)
121-156: Don’t send when critical template data is missing; bail after saving error.After
save_err_to_email(...)youreturn Ok(blocks), so the email still gets sent with placeholders. Bail to stop sending. (This is a follow‑up to the earlier comment; not fully addressed.)Apply:
- } else { - let msg = anyhow::anyhow!("No reset token found for user {}", user_id); - save_err_to_email(email_id, msg, conn).await?; - return Ok(blocks); - } + } else { + let msg = format!("No reset token found for user {}", user_id); + save_err_to_email(email_id, &msg, conn).await?; + anyhow::bail!(msg); + } @@ - } else { - let msg = anyhow::anyhow!("No deletion code found for user {}", user_id); - save_err_to_email(email_id, msg, conn).await?; - return Ok(blocks); - } + } else { + let msg = format!("No deletion code found for user {}", user_id); + save_err_to_email(email_id, &msg, conn).await?; + anyhow::bail!(msg); + }
56-60: Persist body/JSON errors to avoid infinite retries.
?onemail.bodyandserde_json::from_value(...)bubbles errors, leaving the email unmarked; the worker will pick it again forever. Save the error to the email and return Ok(()).Apply:
- tracing::info!("Email send messages..."); - - let mut email_block: Vec<EmailGutenbergBlock> = - serde_json::from_value(email.body.context("No body")?)?; + tracing::debug!("Sending email {}...", email.id); + + // Persist missing/invalid template body errors to avoid retry loops + let body = match email.body { + Some(v) => v, + None => { + save_err_to_email(email.id, "No body in email template", &mut conn) + .await + .context("Couldn't save missing-body err to db")?; + return Ok(()); + } + }; + let mut email_block: Vec<EmailGutenbergBlock> = match serde_json::from_value(body) { + Ok(blocks) => blocks, + Err(err) => { + save_err_to_email(email.id, format!("Invalid template JSON: {err}"), &mut conn) + .await + .context("Couldn't save invalid-json err to db")?; + return Ok(()); + } + };
🧹 Nitpick comments (8)
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx (2)
85-87: Avoid !important in component styles.Prefer variant/prop or specificity over !important to keep styles maintainable.
1-1: File name typo: From → Form.Rename file to VerifyOneTimeCodeForm.tsx to match the component and avoid confusion in imports.
services/headless-lms/server/src/programs/email_deliver.rs (6)
190-203: Keep the worker alive on tick errors.
mail_sender(...).await?will terminate the loop on transient DB/SMTP issues. Log and continue.Apply:
- loop { - interval.tick().await; - mail_sender(&pool, &mailer).await?; - } + loop { + interval.tick().await; + if let Err(err) = mail_sender(&pool, &mailer).await { + tracing::error!("mail_sender tick failed: {err:#}"); + } + }
22-31: Make SMTP port configurable; default to 587.Support environments where the relay runs on a non‑default port.
Apply:
static DB_URL: Lazy<String> = Lazy::new(|| env::var("DATABASE_URL").expect("No db url found in the env variables.")); +static SMTP_PORT: Lazy<u16> = Lazy::new(|| { + env::var("SMTP_PORT").ok().and_then(|s| s.parse().ok()).unwrap_or(587) +}); @@ - let mailer = SmtpTransport::relay(&SMTP_HOST)?.credentials(creds).build(); + let mailer = SmtpTransport::relay(SMTP_HOST.as_str())? + .port(*SMTP_PORT) + .credentials(creds) + .build();(Adjust accordingly if you adopt AsyncSmtpTransport.)
Also applies to: 194-197
75-83: Persist invalid recipient/subject errors.Invalid
toor missingsubjectcurrently bubble and cause retries. Capture, save to DB, and return Ok(()).Sketch:
let to_addr = match email.to.parse() { Ok(m) => m, Err(e) => { save_err_to_email(email.id, format!("Invalid to address: {e}"), &mut conn).await?; return Ok(()); } }; let subject = match email.subject { Some(s) => s, None => { save_err_to_email(email.id, "No subject", &mut conn).await?; return Ok(()); } };Then plug
to_addrandsubjectinto the builder.
161-187: Placeholder replacement only covers Paragraphs.If templates need replacements in headings or list items, extend handling to those variants.
Would you like a patch to generalize replacements across
HeadingandListtoo?
38-49: Make concurrency configurable.
BATCH_SIZEis fixed at 100. Expose via env (e.g., EMAIL_SENDER_CONCURRENCY) to tune per environment and pool size.Example:
static EMAIL_SENDER_CONCURRENCY: Lazy<usize> = Lazy::new(|| { env::var("EMAIL_SENDER_CONCURRENCY").ok().and_then(|s| s.parse().ok()).unwrap_or(100) }); ... .buffer_unordered(*EMAIL_SENDER_CONCURRENCY);
56-56: Nit: clarify log message.“Email send messages...” is confusing. Use “Sending email {id}…”.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
services/headless-lms/models/.sqlx/query-7f07d3938ecf7124b78c72763c13e1268a3cb64d8e4963ca366c5bfc960ae7b9.json(0 hunks)services/headless-lms/server/src/programs/email_deliver.rs(3 hunks)services/headless-lms/server/src/programs/seed/seed_generic_emails.rs(1 hunks)services/main-frontend/src/components/forms/DeleteUserAccountForm.tsx(1 hunks)services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx(1 hunks)services/main-frontend/src/pages/user-settings.tsx(2 hunks)shared-module/packages/common/src/components/LoginControls.tsx(1 hunks)shared-module/packages/common/src/locales/en/main-frontend.json(7 hunks)shared-module/packages/common/src/locales/fi/main-frontend.json(7 hunks)
💤 Files with no reviewable changes (1)
- services/headless-lms/models/.sqlx/query-7f07d3938ecf7124b78c72763c13e1268a3cb64d8e4963ca366c5bfc960ae7b9.json
🚧 Files skipped from review as they are similar to previous changes (6)
- services/main-frontend/src/pages/user-settings.tsx
- shared-module/packages/common/src/components/LoginControls.tsx
- services/main-frontend/src/components/forms/DeleteUserAccountForm.tsx
- shared-module/packages/common/src/locales/en/main-frontend.json
- shared-module/packages/common/src/locales/fi/main-frontend.json
- services/headless-lms/server/src/programs/seed/seed_generic_emails.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
**/*.tsx: Make sure to use theuseTranslationhook for all text that is visible to the user.
If a component uses colors, use colors from the theme if possible. Example: import { baseTheme } from "@/shared-module/common/styles" ... background: ${baseTheme.colors.clear[100]};
If a component uses media queries, use the respondToOrLarger function. Example: import { respondToOrLarger } from "@/shared-module/common/styles/respond" ... ${respondToOrLarger.sm} { ... }
If component sends requests to server, use either useQuery, or useToastMutation hooks, or use hooks wrapping those hooks.
If there is a form of any kind, make sure it uses useForm from react-hook-form, and make sure the form is integrated with the useToastMutation and useQuery hooks.
For loading and error states, use the Spinner and ErrorBanner components from the shared module.
We have better replacements for window.confirm, window.alert, and window.prompt, use the DialogProvider and the useDialog hook from the shared module instead.
Files:
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx
🧬 Code graph analysis (2)
services/headless-lms/server/src/programs/email_deliver.rs (4)
services/headless-lms/models/src/user_passwords.rs (1)
get_unused_reset_password_token_with_user_id(168-196)services/headless-lms/models/src/email_deliveries.rs (2)
fetch_emails(51-76)save_err_to_email(93-112)services/headless-lms/utils/src/email_processor.rs (2)
process_content_to_plaintext(62-97)process_content_to_html(99-128)services/headless-lms/models/src/user_email_codes.rs (1)
get_unused_user_email_code_with_user_id(53-82)
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx (2)
shared-module/packages/common/src/components/InputFields/OneTimePasscodeField.tsx (1)
OneTimePassCodeField(11-133)shared-module/packages/common/src/styles/theme.ts (1)
baseTheme(11-96)
⏰ 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). (7)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build-and-deploy
- GitHub Check: course-material
- GitHub Check: build-and-system-tests
- GitHub Check: headless_lms
- GitHub Check: cms
- GitHub Check: code-style
🔇 Additional comments (2)
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx (2)
6-6: No change required — default export present.
shared-module/packages/common/src/components/InputFields/OneTimePasscodeField.tsx declares a named export and alsoexport default OneTimePassCodeField(lines ~11 and 136), so the current default import is valid.
75-83: Set Resend Button to type="button"Avoid accidental form submission when clicking Resend.
- <Button + <Button + type="button" variant="icon" size={"small"} transform="none" disabled={resendCooldown > 0} onClick={() => { onResend() startCooldown(60) }}
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx
Outdated
Show resolved
Hide resolved
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
shared-module/packages/common/src/locales/en/main-frontend.json (1)
389-395: Key name fixed; normalize quotes around Verify.
The earlier “accoutnt” typo is resolved—nice. Replace curly quotes with straight quotes for consistency across locales.- "insert-single-use-code-account-deletion": "A verification code has been sent to your email. Enter the code below and press “Verify” to confirm account deletion.", + "insert-single-use-code-account-deletion": "A verification code has been sent to your email. Enter the code below and press \"Verify\" to confirm account deletion.",#!/bin/bash set -euo pipefail # 1) Ensure stale, misspelled keys are gone rg -nP 'delete-account-did-not-recieve-email|insert-single-use-code-accoutnt-deletion' || echo "OK: no stale keys found" # 2) Ensure corrected keys exist in all locales fd -a 'main-frontend.json' shared-module/packages/common/src/locales | while read -r f; do echo "Checking $f" rg -n '"delete-account-did-not-receive-email"' "$f" rg -n '"insert-single-use-code-account-deletion"' "$f" done
🧹 Nitpick comments (1)
shared-module/packages/common/src/locales/en/main-frontend.json (1)
214-216: Fix typo and improve phrasing ("recieve" → "receive").
Suggest “a code” for clarity.- "delete-account-did-not-receive-email": "Didn't recieve code?", + "delete-account-did-not-receive-email": "Didn't receive a code?",Please ensure other locales mirror the corrected key (value localized as appropriate).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx(1 hunks)shared-module/packages/common/src/locales/en/main-frontend.json(7 hunks)shared-module/packages/common/src/locales/fi/main-frontend.json(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/main-frontend/src/components/forms/VerifyOneTimeCodeFrom.tsx
- shared-module/packages/common/src/locales/fi/main-frontend.json
🔇 Additional comments (5)
shared-module/packages/common/src/locales/en/main-frontend.json (5)
98-98: LGTM: new verify button key.
Key and copy look consistent with existing button-text-* entries.
162-162: LGTM: confirm-your-new-password added.
Matches password reset flow.
263-263: LGTM: password‑reset helper text.
Copy is clear; no issues.
757-758: LGTM: resend + reset-link-has-expired.
Reads well; aligns with reset flow.
958-958: LGTM: title-delete-account.
Title casing consistent with nearby keys.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
services/headless-lms/models/src/user_passwords.rs (2)
168-196: Add ORDER BY...LIMIT 1 for deterministic token selection.If multiple valid tokens exist (e.g., due to race conditions before index enforcement or during migration), the query returns an arbitrary row. This creates non-deterministic behavior.
Apply this diff:
SELECT token, user_id, created_at, updated_at, used_at, deleted_at, expires_at FROM password_reset_tokens WHERE user_id = $1 AND deleted_at IS NULL AND used_at IS NULL AND expires_at > $2 +ORDER BY created_at DESC +LIMIT 1Based on past review comments.
276-290: Fix type mismatch in TMC client call.Line 280 passes
user_iddirectly asUuid, but based on the method signature pattern (takingupstream_id.to_string()), the second parameter likely expects aString.Apply this diff:
if let Err(e) = tmc_client - .set_user_password_managed_by_courses_mooc_fi(upstream_id.to_string(), user_id) + .set_user_password_managed_by_courses_mooc_fi(upstream_id.to_string(), user_id.to_string()) .awaitVerify the method signature to confirm the expected type:
#!/bin/bash # Check the signature of set_user_password_managed_by_courses_mooc_fi rg -nP "fn\s+set_user_password_managed_by_courses_mooc_fi" -A5Based on past review comments.
services/headless-lms/server/src/controllers/main_frontend/users.rs (2)
274-298: Enforce server-side password validation before hashing.The endpoint hashes
new_password(lines 284-286) without any validation. This leaves the system vulnerable to weak passwords if frontend validation is bypassed.Apply a minimal password policy before hashing:
pub async fn reset_user_password( pool: web::Data<PgPool>, payload: web::Json<ResetPasswordData>, tmc_client: web::Data<TmcClient>, ) -> ControllerResult<web::Json<bool>> { let mut conn = pool.acquire().await?; let token = skip_authorize(); + // Enforce server-side password policy + let trimmed_password = payload.new_password.trim(); + if trimmed_password.len() < 12 { + return Err(ControllerError::BadRequest("Password must be at least 12 characters".into())); + } + if trimmed_password.len() > 128 { + return Err(ControllerError::BadRequest("Password too long".into())); + } + let token_uuid = Uuid::parse_str(&payload.token)?; let password_hash = models::user_passwords::hash_password(&SecretString::new( - payload.new_password.clone().into(), + trimmed_password.to_string().into(), )) .map_err(|e| anyhow!("Failed to hash password: {:?}", e))?;Based on past review comments.
307-330: Enforce server-side password validation before hashing.The endpoint hashes
new_password(lines 315-317) without validation, similar toreset_user_password.Apply the same password policy as suggested for
reset_user_password:pub async fn change_user_password( pool: web::Data<PgPool>, payload: web::Json<ChangePasswordData>, user: AuthUser, ) -> ControllerResult<web::Json<bool>> { let mut conn = pool.acquire().await?; let token = skip_authorize(); + + // Enforce server-side password policy + let trimmed_password = payload.new_password.trim(); + if trimmed_password.len() < 12 { + return Err(ControllerError::BadRequest("Password must be at least 12 characters".into())); + } + if trimmed_password.len() > 128 { + return Err(ControllerError::BadRequest("Password too long".into())); + } + let password_hash = models::user_passwords::hash_password(&SecretString::new( - payload.new_password.clone().into(), + trimmed_password.to_string().into(), )) .map_err(|e| anyhow!("Failed to hash password: {:?}", e))?;Based on past review comments.
🧹 Nitpick comments (3)
services/main-frontend/src/components/forms/ChangeUserPasswordForm.tsx (2)
27-29: Consider using a less aggressive validation mode.The
onChangemode triggers validation on every keystroke, which can be distracting for users, especially for password fields. Consider usingonBluroronTouchedmode for a better user experience.Apply this diff to use
onBlurmode:} = useForm<ChangePasswordFormFields>({ - // eslint-disable-next-line i18next/no-literal-string - mode: "onChange", + mode: "onBlur", defaultValues: {
64-72: Remove unnecessary wrapper div.The wrapping
<div>around the button serves no purpose and can be removed for cleaner code.Apply this diff:
if (!isOpen) { return ( - <div> - <Button variant="primary" size="small" onClick={() => setIsOpen(true)}> - {t("change-password")} - </Button> - </div> + <Button variant="primary" size="small" onClick={() => setIsOpen(true)}> + {t("change-password")} + </Button> ) }services/headless-lms/models/src/user_passwords.rs (1)
27-47: Consider explicitly setting updated_at in the upsert.While
deleted_at = NULLcorrectly restores soft-deleted rows (line 38),updated_atis not being updated on conflict. Unless there's a database trigger handling this, the timestamp won't reflect the password change.Apply this diff if no database trigger exists:
INSERT INTO user_passwords (user_id, password_hash) VALUES ($1, $2) ON CONFLICT (user_id) DO UPDATE SET password_hash = EXCLUDED.password_hash, + updated_at = NOW(), deleted_at = NULL
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
services/headless-lms/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
kubernetes/dev/headless-lms/env.yml(1 hunks)kubernetes/test/headless-lms/env.yml(1 hunks)services/headless-lms/models/.sqlx/query-5b672734108bf9ba51050c7e1ce8cb2753e5a8ab8fe3d9ddf1bc2b29b03c9307.json(1 hunks)services/headless-lms/models/Cargo.toml(1 hunks)services/headless-lms/models/src/error.rs(1 hunks)services/headless-lms/models/src/lib.rs(1 hunks)services/headless-lms/models/src/user_passwords.rs(1 hunks)services/headless-lms/server/Cargo.toml(1 hunks)services/headless-lms/server/src/controllers/main_frontend/users.rs(3 hunks)services/main-frontend/src/components/forms/ChangeUserPasswordForm.tsx(1 hunks)services/main-frontend/src/components/forms/ResetUserPasswordForm.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/headless-lms/models/.sqlx/query-5b672734108bf9ba51050c7e1ce8cb2753e5a8ab8fe3d9ddf1bc2b29b03c9307.json
🚧 Files skipped from review as they are similar to previous changes (4)
- kubernetes/test/headless-lms/env.yml
- kubernetes/dev/headless-lms/env.yml
- services/headless-lms/server/Cargo.toml
- services/main-frontend/src/components/forms/ResetUserPasswordForm.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
**/*.tsx: Make sure to use theuseTranslationhook for all text that is visible to the user.
If a component uses colors, use colors from the theme if possible. Example: import { baseTheme } from "@/shared-module/common/styles" ... background: ${baseTheme.colors.clear[100]};
If a component uses media queries, use the respondToOrLarger function. Example: import { respondToOrLarger } from "@/shared-module/common/styles/respond" ... ${respondToOrLarger.sm} { ... }
If component sends requests to server, use either useQuery, or useToastMutation hooks, or use hooks wrapping those hooks.
If there is a form of any kind, make sure it uses useForm from react-hook-form, and make sure the form is integrated with the useToastMutation and useQuery hooks.
For loading and error states, use the Spinner and ErrorBanner components from the shared module.
We have better replacements for window.confirm, window.alert, and window.prompt, use the DialogProvider and the useDialog hook from the shared module instead.
For calling the React hooks useMemo and useEffect, don't use the React. prefix.
If a component uses fonts, use fonts from the theme if possible. Example: import { primaryFont } from "@/shared-module/common/styles" ... font-family: ${primaryFont};
Files:
services/main-frontend/src/components/forms/ChangeUserPasswordForm.tsx
🧬 Code graph analysis (3)
services/headless-lms/models/src/error.rs (1)
shared-module/packages/common/src/bindings.ts (1)
EmailTemplateNew(656-660)
services/main-frontend/src/components/forms/ChangeUserPasswordForm.tsx (2)
shared-module/packages/common/src/hooks/useToastMutation.tsx (1)
useToastMutation(36-160)services/main-frontend/src/services/backend/users.ts (1)
postPasswordChange(77-87)
services/headless-lms/server/src/controllers/main_frontend/users.rs (5)
services/headless-lms/server/src/domain/authorization.rs (1)
skip_authorize(276-278)services/headless-lms/models/src/email_templates.rs (1)
get_generic_email_template_by_name_and_language(54-82)services/headless-lms/models/src/users.rs (3)
get_by_email(93-107)insert_with_upstream_id_and_moocfi_id(51-91)res(214-214)services/headless-lms/models/src/user_passwords.rs (5)
insert_password_reset_token(130-166)is_reset_password_token_valid(198-219)hash_password(49-61)change_user_password_with_password_reset_token(221-294)change_user_password_with_old_password(296-316)services/headless-lms/models/src/email_deliveries.rs (1)
insert_email_delivery(25-49)
⏰ 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: headless_lms
- GitHub Check: build-and-system-tests
- GitHub Check: code-style
- GitHub Check: build-and-deploy
🔇 Additional comments (6)
services/main-frontend/src/components/forms/ChangeUserPasswordForm.tsx (1)
74-134: Form implementation looks good.The form correctly:
- Uses proper password field types for security
- Implements appropriate validation rules (required fields, 8-char minimum, confirmation matching)
- Shows error messages for each field
- Disables submit button during mutation to prevent duplicate submissions
- Uses translations for all user-facing text
The validation logic for password confirmation correctly watches the
new_passwordfield and compares it in real-time.services/headless-lms/models/src/error.rs (1)
356-378: LGTM! Test correctly updated to match API changes.The test changes properly reflect the updated
insert_email_templatesignature (now acceptingOption<Uuid>) and the new optionallanguageandcontentfields inEmailTemplateNew.services/headless-lms/server/src/controllers/main_frontend/users.rs (1)
178-241: LGTM! Email enumeration attacks properly mitigated.The implementation correctly handles all edge cases:
- Returns success regardless of user existence (lines 202, 240) to prevent email enumeration
- Gracefully handles missing email templates with controlled error handling (lines 197-204)
- Integrates with upstream TMC to fetch and create users not present locally (lines 211-222)
This addresses the previously flagged security concerns.
services/headless-lms/models/Cargo.toml (1)
61-64: LGTM! Dependencies appropriate for secure password handling.The additions of
secrecyfor secret management andargon2for Argon2id password hashing are well-chosen and correctly versioned for the password management functionality.services/headless-lms/models/src/lib.rs (1)
93-93: LGTM! Module declarations correctly expose new functionality.The public module declarations for
user_email_codesanduser_passwordsappropriately expand the public API surface.services/headless-lms/models/src/user_passwords.rs (1)
63-109: LGTM! Timing attack mitigation properly implemented.The use of
LazyLockfor the dummy hash (lines 85-91) and consistent verification path for non-existent users effectively prevents timing attacks while avoiding runtime panics. This addresses previous review concerns.
| const changePasswordMutation = useToastMutation<boolean, unknown, ChangePasswordFormFields>( | ||
| async (data) => { | ||
| const response = await postPasswordChange(data.current_password, data.new_password) | ||
| return response | ||
| }, | ||
| { | ||
| method: "POST", | ||
| notify: true, | ||
| }, | ||
| { | ||
| onSuccess: () => { | ||
| setIsOpen(false) | ||
| reset() | ||
| }, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Missing notification messages for toast notifications.
The useToastMutation is configured with notify: true but does not provide the required notification text options (loadingText, successHeader, successMessage, errorHeader, errorMessage). This will result in toast notifications displaying without proper user-facing messages.
Apply this diff to add proper notification messages:
const changePasswordMutation = useToastMutation<boolean, unknown, ChangePasswordFormFields>(
async (data) => {
const response = await postPasswordChange(data.current_password, data.new_password)
return response
},
{
method: "POST",
notify: true,
+ loadingText: t("changing-password"),
+ successHeader: t("password-changed"),
+ successMessage: t("your-password-has-been-changed-successfully"),
+ errorHeader: t("password-change-failed"),
+ errorMessage: t("failed-to-change-password"),
},
{📝 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.
| const changePasswordMutation = useToastMutation<boolean, unknown, ChangePasswordFormFields>( | |
| async (data) => { | |
| const response = await postPasswordChange(data.current_password, data.new_password) | |
| return response | |
| }, | |
| { | |
| method: "POST", | |
| notify: true, | |
| }, | |
| { | |
| onSuccess: () => { | |
| setIsOpen(false) | |
| reset() | |
| }, | |
| }, | |
| ) | |
| const changePasswordMutation = useToastMutation<boolean, unknown, ChangePasswordFormFields>( | |
| async (data) => { | |
| const response = await postPasswordChange(data.current_password, data.new_password) | |
| return response | |
| }, | |
| { | |
| method: "POST", | |
| notify: true, | |
| loadingText: t("changing-password"), | |
| successHeader: t("password-changed"), | |
| successMessage: t("your-password-has-been-changed-successfully"), | |
| errorHeader: t("password-change-failed"), | |
| errorMessage: t("failed-to-change-password"), | |
| }, | |
| { | |
| onSuccess: () => { | |
| setIsOpen(false) | |
| reset() | |
| }, | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In services/main-frontend/src/components/forms/ChangeUserPasswordForm.tsx around
lines 38 to 53, the useToastMutation call sets notify: true but omits the
required notification text options; update the mutation options to include
loadingText, successHeader, successMessage, errorHeader, and errorMessage
strings that describe the in-progress state, successful password change, and
failure (e.g., "Changing password...", "Password changed", "Your password was
updated successfully", "Password change failed", and a brief error message),
leaving the existing onSuccess behaviour unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/headless-lms/utils/src/http.rs (1)
7-16: Critical: Unconditional HTTP allowance creates credential leak vulnerability.The security concern is valid. Endpoints are configured from environment variables without HTTPS validation:
AZURE_CHATBOT_API_ENDPOINTandAZURE_SEARCH_ENDPOINTare parsed viaUrl::parse()which accepts both HTTP and HTTPS- No validation enforces the HTTPS protocol
- If these environment variables are set to HTTP URLs, the client will use them unencrypted due to
.https_only(false)- API keys and credentials are passed in request headers, creating a credential leakage risk
Additionally,
http_test_mode(lines 7–12) is dead code—it only controls a warning message, not the actual HTTPS behavior, making the warning misleading.Recommended fix:
Either enforce HTTPS protocol validation on endpoints or revert to conditional HTTPS enforcement:
reqwest::Client::builder() .use_rustls_tls() - .https_only(false) + .https_only(!http_test_mode) .build()Alternatively, validate that configured endpoints use HTTPS protocol before allowing the client to connect.
♻️ Duplicate comments (3)
services/headless-lms/utils/src/tmc/mod.rs (1)
26-33: TmcUserInfo will fail to deserialize; map JSON "id" to numeric upstream_id (drop Uuid id).TMC returns a numeric id; modeling it as Uuid plus a separate upstream_id causes runtime JSON errors.
Apply:
-#[derive(Debug, Deserialize)] -pub struct TmcUserInfo { - pub id: Uuid, - pub email: String, - pub first_name: Option<String>, - pub last_name: Option<String>, - pub upstream_id: i32, -} +#[derive(Debug, Deserialize)] +pub struct TmcUserInfo { + #[serde(rename = "id")] + pub upstream_id: i64, + pub email: String, + #[serde(default)] + pub first_name: Option<String>, + #[serde(default)] + pub last_name: Option<String>, +}Update call sites to use
upstream_idinstead ofid. Based on learnings.services/headless-lms/server/src/controllers/auth.rs (2)
589-589: Use cryptographically secure RNG for email codes.The 6-digit deletion code is security-sensitive but uses
rand::rng().random_range(), which may not provide cryptographic-strength randomness.Use
OsRngfor cryptographically secure random generation:- let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); + use rand::rngs::OsRng; + let new_code: String = OsRng.random_range(100_000..1_000_000).to_string();
580-600: Remove duplicate code insertion.Lines 590-595 insert the newly generated code, but lines 599-600 insert it again, creating duplicate database entries.
Apply this diff to remove the duplicate insertion:
} else { let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); models::user_email_codes::insert_user_email_code( &mut conn, auth_user.id, new_code.clone(), ) .await?; new_code }; - models::user_email_codes::insert_user_email_code(&mut conn, auth_user.id, code.clone()) - .await?; let _ = models::email_deliveries::insert_email_delivery(&mut conn, user.id, delete_template.id) .await?;
🧹 Nitpick comments (7)
services/headless-lms/utils/src/tmc/mod.rs (3)
55-61: Be robust to missing fields from TMC user_field.Default/optional fields avoid brittle parsing when TMC omits keys.
-#[derive(Debug, Serialize, Deserialize)] -pub struct TMCUserField { - pub first_name: String, - pub last_name: String, - pub organizational_id: String, - pub course_announcements: bool, -} +#[derive(Debug, Serialize, Deserialize)] +pub struct TMCUserField { + #[serde(default)] + pub first_name: String, + #[serde(default)] + pub last_name: String, + #[serde(default)] + pub organizational_id: String, + #[serde(default)] + pub course_announcements: bool, +}
166-173: Avoid propagating full error bodies (possible PII) in returned errors; log truncated body at debug.Keep redacted path in warn; move body to truncated debug; return status-only error.
- tracing::debug!("Response body: {}", error_text); - - Err(anyhow::anyhow!( - "Request failed with status {}: {}", - status, - error_text - )) + let snippet: String = error_text.chars().take(512).collect(); + tracing::debug!("Response body (truncated): {}", snippet); + Err(anyhow::anyhow!(format!("Request failed with status {}", status)))
341-345: Redundant status checks; unreachable after request_with_headers already enforces success.Remove dead branches; rely on
request_with_headerserrors.- if !res.status().is_success() { - error!("Failed to get user from TMC with status {}", res.status()); - return Err(anyhow::anyhow!("Failed to get current user from TMC")); - } + // request_with_headers already returns Err for non-success- if !res.status().is_success() { - error!("Failed to get user from TMC with status {}", res.status()); - return Err(anyhow::anyhow!("Failed to get user from TMC")); - } + // request_with_headers already returns Err for non-successAlso applies to: 377-380
services/headless-lms/server/src/config.rs (1)
23-35: token_url wiring looks correct; consider env override for flexibility.Using a dedicated token_url and passing it to OAuth client is good. Optionally allow overriding via env for non‑tmc deployments.
Also applies to: 44-49, 82-86
services/headless-lms/server/src/domain/langs/token.rs (1)
120-127: Remove unused local TmcUser struct.Duplicate of utils::TMCUser; safe to delete.
-#[derive(Debug, Deserialize)] -#[allow(unused)] -struct TmcUser { - id: i32, - username: String, - email: String, - administrator: bool, -}services/headless-lms/server/src/domain/authorization.rs (1)
316-352: Consider timing-safe comparison for secret validation.The current string comparison on line 344 (
auth_header == tmc_server_secret_for_communicating_to_secret_project) is vulnerable to timing attacks. While this is a server-to-server authentication mechanism, using a constant-time comparison would improve security posture.Consider using a constant-time comparison:
+use subtle::ConstantTimeEq; // If auth header correct one, grant access - if auth_header == tmc_server_secret_for_communicating_to_secret_project { + if auth_header.as_bytes().ct_eq(tmc_server_secret_for_communicating_to_secret_project.as_bytes()).into() { return Ok(skip_authorize()); }Note: This would require adding the
subtlecrate dependency.services/headless-lms/server/src/controllers/auth.rs (1)
616-676: Consider adding rate limiting to prevent code brute-force.Both
delete_user_accountandsend_delete_user_email_codelack rate limiting, which could allow brute-force attempts on 6-digit codes. The code space is only 900,000 possibilities.Update the route registration in
_add_routesto add rate limiting:.route("/user-info", web::get().to(user_info)) - .route("/delete-user-account", web::post().to(delete_user_account)) - .route( - "/send-email-code", - web::post().to(send_delete_user_email_code), - ); + .service( + web::resource("/delete-user-account") + .wrap(build_rate_limiting_middleware(Duration::from_secs(60), 5)) + .to(delete_user_account), + ) + .service( + web::resource("/send-email-code") + .wrap(build_rate_limiting_middleware(Duration::from_secs(60), 3)) + .wrap(build_rate_limiting_middleware(Duration::from_secs(300), 10)) + .to(send_delete_user_email_code), + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
services/headless-lms/Dockerfile(1 hunks)services/headless-lms/server/src/config.rs(3 hunks)services/headless-lms/server/src/controllers/auth.rs(8 hunks)services/headless-lms/server/src/controllers/langs.rs(7 hunks)services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs(2 hunks)services/headless-lms/server/src/domain/authorization.rs(5 hunks)services/headless-lms/server/src/domain/langs/token.rs(3 hunks)services/headless-lms/server/src/test_helper.rs(1 hunks)services/headless-lms/server/tests/integration_test.rs(1 hunks)services/headless-lms/utils/src/http.rs(1 hunks)services/headless-lms/utils/src/tmc/mod.rs(8 hunks)services/tmc/Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-03T09:10:44.098Z
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
Applied to files:
services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rsservices/headless-lms/server/src/controllers/langs.rsservices/headless-lms/server/src/domain/authorization.rs
🧬 Code graph analysis (5)
services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs (1)
services/headless-lms/server/src/domain/authorization.rs (3)
authorize_access_from_tmc_server_to_course_mooc_fi(318-352)get_or_create_user_from_tmc_mooc_fi_response(811-850)upstream_id(59-61)
services/headless-lms/server/src/domain/langs/token.rs (1)
services/headless-lms/server/src/domain/authorization.rs (1)
get_or_create_user_from_tmc_mooc_fi_response(811-850)
services/headless-lms/server/src/domain/authorization.rs (1)
shared-module/packages/common/src/bindings.ts (1)
User(2186-2193)
services/headless-lms/utils/src/tmc/mod.rs (1)
services/headless-lms/server/src/controllers/auth.rs (1)
post_new_user_to_tmc(681-700)
services/headless-lms/server/src/controllers/auth.rs (7)
services/headless-lms/server/src/domain/authorization.rs (9)
session(74-74)session(116-116)session(157-157)upstream_id(59-61)skip_authorize(275-277)remember(141-153)authenticate_test_user(855-910)authenticate_moocfi_user(719-751)forget(168-170)services/headless-lms/utils/src/tmc/mod.rs (1)
post_new_user_to_tmc(224-257)services/headless-lms/models/src/users.rs (1)
insert_with_upstream_id_and_moocfi_id(51-91)services/headless-lms/models/src/user_passwords.rs (4)
hash_password(49-61)upsert_user_password(27-47)verify_user_password(63-109)check_if_users_password_is_stored(111-128)services/headless-lms/models/src/email_templates.rs (1)
get_generic_email_template_by_name_and_language(54-82)services/headless-lms/models/src/user_email_codes.rs (4)
get_unused_user_email_code_with_user_id(53-82)insert_user_email_code(15-51)is_reset_user_email_code_valid(84-108)mark_user_email_code_used(110-131)services/headless-lms/models/src/email_deliveries.rs (1)
insert_email_delivery(25-49)
⏰ 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: headless_lms
- GitHub Check: code-style
- GitHub Check: build-and-system-tests
- GitHub Check: build-and-deploy
🔇 Additional comments (7)
services/tmc/Dockerfile (1)
17-17: LGTM!Adding the
-fflag tormis the correct approach here. This makes the build more resilient by allowing the cleanup to succeed even if.sha256files are absent, preventing spurious build failures during the Docker image creation process.services/headless-lms/Dockerfile (1)
4-8: LGTM! Proper permission setup for non-root user.The change correctly switches to root temporarily to create and set up
/appwith appropriate ownership, then reverts to the non-root user. This pattern is a best practice in multi-stage Docker builds and ensures the/appdirectory is writable by the user during subsequent build steps and at runtime.The idempotent use of
mkdir -pand explicitchown user:userare appropriate.services/headless-lms/utils/src/tmc/mod.rs (2)
283-295: Nice: GET email lookup now uses Url builder and no body.This fixes URL encoding and avoids GET bodies. Good improvement.
229-244: I'll search for TMC API documentation regarding the authentication requirements for user creation.Refactor payload construction to conditionally set "origin".
The
originparameter is conditionally used in the tmc-server endpoint for redirect logic. Sendingnullmay not cause a failure, but the payload should only includeoriginwhen configured to avoid unnecessary null values.- let payload = json!({ - "user": { - "email": user_info.email, - "first_name": user_info.first_name, - "last_name": user_info.last_name, - "password": user_info.password, - "password_confirmation": user_info.password_confirmation - }, - "user_field": { - "first_name": user_info.first_name, - "last_name": user_info.last_name - }, - "origin": app_conf.tmc_account_creation_origin, - "language": user_info.language - }); + let mut payload = json!({ + "user": { + "email": user_info.email, + "first_name": user_info.first_name, + "last_name": user_info.last_name, + "password": user_info.password, + "password_confirmation": user_info.password_confirmation + }, + "user_field": { + "first_name": user_info.first_name, + "last_name": user_info.last_name + }, + "language": user_info.language + }); + if let Some(origin) = &app_conf.tmc_account_creation_origin { + payload + .as_object_mut() + .expect("payload is object") + .insert("origin".to_string(), serde_json::Value::String(origin.clone())); + }Regarding authentication: verify whether
TMCRequestAuth::NoAuthis correct by checking if the/api/v8/usersendpoint requires admin token or if public registration is permitted.services/headless-lms/server/tests/integration_test.rs (1)
65-72: Tests updated to supply token_url.Consistent with config changes; keeps tests self‑contained.
services/headless-lms/server/src/test_helper.rs (1)
13-22: Test helper passes token_url as well.Keeps helper aligned with ServerConfigBuilder.
services/headless-lms/server/src/controllers/auth.rs (1)
110-128: LGTM! Upstream ID persistence implemented correctly.The signup flow now correctly:
- Retrieves
upstream_idfrom TMC (line 110)- Persists it to the local user via
insert_with_upstream_id_and_moocfi_id(lines 113-128)- Uses it for TMC notifications (line 155)
This addresses the previous review concern about missing upstream_id persistence.
| let tmc_user = tmc_client | ||
| .get_user_from_tmc_mooc_fi_by_tmc_access_token( | ||
| token.access_token().secret(), | ||
| ) | ||
| .await?; | ||
|
|
There was a problem hiding this comment.
? on anyhow::Result doesn’t convert to ControllerError here; compile-time type mismatch.
Map the error to ControllerError explicitly.
- let tmc_user = tmc_client
- .get_user_from_tmc_mooc_fi_by_tmc_access_token(
- token.access_token().secret(),
- )
- .await?;
+ let tmc_user = tmc_client
+ .get_user_from_tmc_mooc_fi_by_tmc_access_token(
+ token.access_token().secret(),
+ )
+ .await
+ .map_err(|err| {
+ ControllerError::new(
+ ControllerErrorType::Unauthorized,
+ "Failed to get current user from TMC".to_string(),
+ Some(err),
+ )
+ })?;🤖 Prompt for AI Agents
In services/headless-lms/server/src/domain/langs/token.rs around lines 87–92,
the call to tmc_client.get_user_from_tmc_mooc_fi_by_tmc_access_token(...).await
currently uses the `?` operator on an anyhow::Result which doesn't auto-convert
to ControllerError; change the call to explicitly map the error into a
ControllerError (e.g. replace `...await?` with `...await.map_err(|e|
ControllerError::Internal(e.to_string()))?` or another appropriate
ControllerError variant) so the error type matches the function's return type
and include context in the mapped message.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
services/headless-lms/server/src/domain/langs/token.rs (1)
83-101: Map anyhow errors to ControllerError in extractor (as noted earlier)The
?ontmc_client...awaitandget_or_create_user_from_tmc_mooc_fi_response(...).awaitwon’t compile here. Map errors.- let tmc_user = tmc_client - .get_user_from_tmc_mooc_fi_by_tmc_access_token(token.clone()) - .await?; + let tmc_user = tmc_client + .get_user_from_tmc_mooc_fi_by_tmc_access_token(token.clone()) + .await + .map_err(|err| { + ControllerError::new( + ControllerErrorType::Unauthorized, + "Failed to get current user from TMC".to_string(), + Some(err.into()), + ) + })?; @@ - let user = - get_or_create_user_from_tmc_mooc_fi_response(&mut conn, tmc_user) - .await?; + let user = get_or_create_user_from_tmc_mooc_fi_response(&mut conn, tmc_user) + .await + .map_err(|err| { + ControllerError::new( + ControllerErrorType::InternalServerError, + "Failed to create or fetch local user".to_string(), + Some(err.into()), + ) + })?;services/headless-lms/utils/src/tmc/mod.rs (1)
27-34: Critical: TmcUserInfo will fail JSON deserializationThis struct defines both
id: Uuidandupstream_id: i32, but TMC's API returns a numericidfield (not a UUID). When the client receives a response from TMC with{"id": 12345, ...}, serde will attempt to deserialize12345into theid: Uuidfield and fail at runtime.This issue was previously flagged but remains unresolved.
Apply this fix to match TMC's actual response schema:
#[derive(Debug, Deserialize)] pub struct TmcUserInfo { - pub id: Uuid, + #[serde(rename = "id")] + pub upstream_id: i64, pub email: String, pub first_name: Option<String>, pub last_name: Option<String>, - pub upstream_id: i32, }Then update any call sites that expected
id: Uuidto useupstream_id: i64instead.
🧹 Nitpick comments (8)
services/headless-lms/utils/src/lib.rs (2)
63-74: Don't panic on missing TMC_ACCESS_TOKEN; return an errorPanicking inside configuration assembly can take the service down unexpectedly. Propagate a structured error instead.
- let tmc_admin_access_token = SecretString::new( - std::env::var("TMC_ACCESS_TOKEN") - .unwrap_or_else(|_| { - if test_mode { - "mock-access-token".to_string() - } else { - panic!("TMC_ACCESS_TOKEN must be defined in production") - } - }) - .into(), - ); + let tmc_admin_access_token = if test_mode { + SecretString::new( + env::var("TMC_ACCESS_TOKEN") + .unwrap_or_else(|_| "mock-access-token".to_string()) + .into(), + ) + } else { + SecretString::new( + env::var("TMC_ACCESS_TOKEN") + .context("TMC_ACCESS_TOKEN must be defined in production")? + .into(), + ) + };
58-61: Optional env should not be forcedField type is Option but code now errors unless TMC_ACCOUNT_CREATION_ORIGIN is set. If this origin is not strictly required in all environments, make it optional.
- let tmc_account_creation_origin = Some( - env::var("TMC_ACCOUNT_CREATION_ORIGIN") - .context("TMC_ACCOUNT_CREATION_ORIGIN must be defined")?, - ); + let tmc_account_creation_origin = env::var("TMC_ACCOUNT_CREATION_ORIGIN").ok();services/headless-lms/server/tests/integration_test.rs (1)
72-72: Test wiring looks good; tiny nittoken_url addition and mock tmc_admin_access_token are fine. Nit: avoid redundant to_string().into().
- tmc_admin_access_token: SecretString::new("mock-access-token".to_string().into()), + tmc_admin_access_token: SecretString::new("mock-access-token".into()),Also applies to: 85-86
services/headless-lms/server/src/domain/authorization.rs (2)
317-353: Harden TMC server auth: avoid expect, avoid per-request env, and compare in constant time
- Using expect() will panic on misconfiguration; return Unauthorized/ISE instead.
- Read and store the secret at startup (ApplicationConfiguration) rather than per request.
- Use constant‑time comparison to reduce timing side‑channel risk.
Would you like a follow-up PR to move this secret into ApplicationConfiguration and switch to a constant‑time compare (e.g., via subtle::ConstantTimeEq)?
814-853: User creation from TMC data: confirm UUID fallback behaviorWhen courses_mooc_fi_user_id is absent, a new UUID is generated. Confirm this aligns with downstream expectations and won’t desync identities across systems.
services/headless-lms/server/src/domain/langs/token.rs (2)
47-53: Stricter header parsing (avoid lossy decode; allow case-insensitive 'Bearer')Use to_str() and case-insensitive match; trim extra spaces.
- let auth_header = req - .headers() - .get(header::AUTHORIZATION) - .map(|hv| String::from_utf8_lossy(hv.as_bytes())) - .and_then(|h| h.strip_prefix("Bearer ").map(str::to_string)) - .map(|o| SecretString::new(o.into())); + let auth_header = req + .headers() + .get(header::AUTHORIZATION) + .and_then(|hv| hv.to_str().ok()) + .and_then(|h| { + let h = h.trim(); + h.strip_prefix("Bearer ") + .or_else(|| h.strip_prefix("bearer ")) + }) + .map(|tok| SecretString::new(tok.to_string().into()));
114-121: Remove unused local TmcUser structDead code; drop it to reduce noise.
-#[derive(Debug, Deserialize)] -#[allow(unused)] -struct TmcUser { - id: i32, - username: String, - email: String, - administrator: bool, -}services/headless-lms/utils/src/tmc/mod.rs (1)
364-389: Misleading method nameThe method is named
get_user_from_tmc_mooc_fi_by_tmc_access_token_and_upstream_id, but it only takesupstream_id: &i32and uses the admin token for authentication (line 374), not a user access token. This naming mismatch can confuse callers.Consider renaming to better reflect what it does:
- pub async fn get_user_from_tmc_mooc_fi_by_tmc_access_token_and_upstream_id( + pub async fn get_user_from_tmc_mooc_fi_by_upstream_id( &self, upstream_id: &i32, ) -> anyhow::Result<TMCUser> {Also note: This method uses the bare
info!,error!, anddebug!macros—see the earlier comment about missing imports.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
services/headless-lms/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
services/headless-lms/Cargo.toml(1 hunks)services/headless-lms/models/Cargo.toml(1 hunks)services/headless-lms/server/Cargo.toml(2 hunks)services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs(2 hunks)services/headless-lms/server/src/domain/authorization.rs(9 hunks)services/headless-lms/server/src/domain/langs/token.rs(4 hunks)services/headless-lms/server/src/test_helper.rs(3 hunks)services/headless-lms/server/tests/integration_test.rs(3 hunks)services/headless-lms/utils/Cargo.toml(1 hunks)services/headless-lms/utils/src/lib.rs(2 hunks)services/headless-lms/utils/src/tmc/mod.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/headless-lms/server/src/test_helper.rs
- services/headless-lms/server/Cargo.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-03T09:10:44.098Z
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
Applied to files:
services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rsservices/headless-lms/server/src/domain/authorization.rs
🧬 Code graph analysis (5)
services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs (1)
services/headless-lms/server/src/domain/authorization.rs (3)
authorize_access_from_tmc_server_to_course_mooc_fi(319-353)get_or_create_user_from_tmc_mooc_fi_response(814-853)upstream_id(60-62)
services/headless-lms/utils/src/lib.rs (2)
services/headless-lms/server/src/controllers/main_frontend/exercise_repositories.rs (1)
new(25-71)services/headless-lms/chatbot/src/azure_blob_storage.rs (1)
new(19-45)
services/headless-lms/server/src/domain/authorization.rs (1)
services/headless-lms/models/src/users.rs (1)
get_by_email(93-107)
services/headless-lms/server/src/domain/langs/token.rs (2)
services/headless-lms/server/src/domain/authorization.rs (1)
get_or_create_user_from_tmc_mooc_fi_response(814-853)services/headless-lms/utils/src/cache.rs (3)
cache(334-334)cache(361-361)new(47-116)
services/headless-lms/utils/src/tmc/mod.rs (1)
services/headless-lms/server/src/controllers/auth.rs (1)
post_new_user_to_tmc(681-700)
⏰ 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: build-and-system-tests
- GitHub Check: code-style
- GitHub Check: headless_lms
- GitHub Check: build-and-deploy
🔇 Additional comments (18)
services/headless-lms/utils/Cargo.toml (1)
91-92: Appropriate addition for secret handling in utilities.The workspace dependency reference is correct, and the placement in utils makes secrecy available for foundational components.
services/headless-lms/Cargo.toml (1)
58-59: Workspace dependency addition is correct and secure.Version 0.10.3 is the latest published version (October 2024), and no advisories mark it as vulnerable; it actually includes a security fix from RUSTSEC-2024-0346. The serde feature is appropriate for serialization workflows, and the workspace dependency structure is properly configured. No changes needed.
services/headless-lms/models/Cargo.toml (1)
61-64: Argon2 0.5.3 is current and secure—no changes needed.The latest stable version of the argon2 crate is 0.5.3 (released Jan 20, 2024), and no RustSec or OSV advisories report a vulnerability in the argon2 crate v0.5.3 itself. The dependency additions are appropriate for password hashing in the models layer.
services/headless-lms/utils/src/lib.rs (1)
31-40: Good: secret encapsulation and derive changeUsing SecretString for tmc_admin_access_token and removing PartialEq on ApplicationConfiguration reduce accidental secret exposure/comparison. LGTM.
services/headless-lms/server/src/domain/authorization.rs (1)
720-752: Good shift to TmcClient + SecretStringClear separation of token exchange and user resolution. No concerns here.
services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs (1)
7-15: Good: inject TmcClient via app dataCleaner boundary; avoids global env reads in the handler. LGTM.
Also applies to: 23-29
services/headless-lms/server/src/domain/langs/token.rs (2)
123-127: Cache key derivation and TTL look goodHashing the token for the key avoids storing raw tokens; 1h TTL is reasonable baseline.
If TMC tokens have a known expiry, consider aligning TTL to it.
Also applies to: 129-141
68-79: Heads‑up: test_mode path depends on broken authenticate_test_tokenUntil authenticate_test_token is fixed (see separate comment), this branch will error in tests using bearer tokens.
services/headless-lms/utils/src/tmc/mod.rs (10)
36-68: LGTM!The
TMCUserResponse,TmcDeleteAccountResponse,TMCUser,TMCUserField, andTMCRequestAuthtypes are well-structured and appropriate for their use cases. TheTMCRequestAuthenum provides a clean way to handle different authentication scenarios.
73-113: LGTM!The environment setup correctly handles dev/prod differences, validates token presence in production, and configures a reasonable 15-second timeout for HTTP requests.
115-178: LGTM!The authentication logic correctly handles admin tokens, user tokens, and no-auth scenarios. The URL redaction at lines 160-169 properly prevents PII from being logged in query parameters, addressing the earlier concern about privacy.
180-226: LGTM!The user update logic correctly constructs the nested payload structure and appropriately uses admin authentication.
228-261: LGTM!The refactored method correctly returns the upstream ID and appropriately uses
NoAuthfor new user registration since the user doesn't have credentials yet.
263-285: LGTM!The method signature and implementation are correct. Previous concerns about call sites passing the wrong types were addressed in commit 10f0fa6.
287-307: Verify after fixing TmcUserInfoThe GET request implementation correctly URL-encodes the email and sends no body (addressing previous concerns from commit 2771ba7). However, this method returns
TmcUserInfo, which has a critical deserialization bug. Once that struct is fixed (see earlier comment), verify that this method correctly deserializes TMC's response.
309-327: LGTM!The delete operation correctly uses admin authentication and properly parses the response to return the success status.
329-362: Method logic is correct; depends on fixing missing importsThe method correctly uses the user's access token for authentication and has good logging for debugging. However, the logging macros (
info!,error!,debug!) are not imported—see the earlier comment about missing imports.
391-397: LGTM!The test mock correctly initializes all required fields with mock values.
| let tmc_user = tmc_client | ||
| .get_user_from_tmc_mooc_fi_by_tmc_access_token_and_upstream_id(&upstream_id) | ||
| .await?; | ||
|
|
||
| debug!( | ||
| "Creating or fetching user with TMC id {} and mooc.fi UUID {}", | ||
| tmc_user.id, | ||
| tmc_user | ||
| .courses_mooc_fi_user_id | ||
| .map(|uuid| uuid.to_string()) | ||
| .unwrap_or_else(|| "None (will generate new UUID)".to_string()) | ||
| ); | ||
| let user = get_or_create_user_from_tmc_mooc_fi_response(&mut conn, tmc_user).await?; | ||
| info!( |
There was a problem hiding this comment.
Map external errors into ControllerError (fix compile-time mismatch)
Both calls return anyhow::Result; this handler returns ControllerResult. Map errors explicitly.
- let tmc_user = tmc_client
- .get_user_from_tmc_mooc_fi_by_tmc_access_token_and_upstream_id(&upstream_id)
- .await?;
+ let tmc_user = tmc_client
+ .get_user_from_tmc_mooc_fi_by_tmc_access_token_and_upstream_id(&upstream_id)
+ .await
+ .map_err(|err| {
+ ControllerError::new(
+ ControllerErrorType::Unauthorized,
+ "Failed to fetch user from TMC".to_string(),
+ Some(err.into()),
+ )
+ })?;
@@
- let user = get_or_create_user_from_tmc_mooc_fi_response(&mut conn, tmc_user).await?;
+ let user = get_or_create_user_from_tmc_mooc_fi_response(&mut conn, tmc_user)
+ .await
+ .map_err(|err| {
+ ControllerError::new(
+ ControllerErrorType::InternalServerError,
+ "Failed to create or fetch local user".to_string(),
+ Some(err.into()),
+ )
+ })?;📝 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 tmc_user = tmc_client | |
| .get_user_from_tmc_mooc_fi_by_tmc_access_token_and_upstream_id(&upstream_id) | |
| .await?; | |
| debug!( | |
| "Creating or fetching user with TMC id {} and mooc.fi UUID {}", | |
| tmc_user.id, | |
| tmc_user | |
| .courses_mooc_fi_user_id | |
| .map(|uuid| uuid.to_string()) | |
| .unwrap_or_else(|| "None (will generate new UUID)".to_string()) | |
| ); | |
| let user = get_or_create_user_from_tmc_mooc_fi_response(&mut conn, tmc_user).await?; | |
| info!( | |
| let tmc_user = tmc_client | |
| .get_user_from_tmc_mooc_fi_by_tmc_access_token_and_upstream_id(&upstream_id) | |
| .await | |
| .map_err(|err| { | |
| ControllerError::new( | |
| ControllerErrorType::Unauthorized, | |
| "Failed to fetch user from TMC".to_string(), | |
| Some(err.into()), | |
| ) | |
| })?; | |
| debug!( | |
| "Creating or fetching user with TMC id {} and mooc.fi UUID {}", | |
| tmc_user.id, | |
| tmc_user | |
| .courses_mooc_fi_user_id | |
| .map(|uuid| uuid.to_string()) | |
| .unwrap_or_else(|| "None (will generate new UUID)".to_string()) | |
| ); | |
| let user = get_or_create_user_from_tmc_mooc_fi_response(&mut conn, tmc_user) | |
| .await | |
| .map_err(|err| { | |
| ControllerError::new( | |
| ControllerErrorType::InternalServerError, | |
| "Failed to create or fetch local user".to_string(), | |
| Some(err.into()), | |
| ) | |
| })?; | |
| info!( |
🤖 Prompt for AI Agents
In
services/headless-lms/server/src/controllers/tmc_server/users_by_upstream_id.rs
around lines 31-44, both await calls currently use the ? operator on functions
that return anyhow::Result while this handler returns ControllerResult; replace
the trailing ? on each .await with .await.map_err(|e|
ControllerError::InternalError(e.into())) (or the appropriate ControllerError
constructor used in this crate) so the anyhow::Error is explicitly converted
into a ControllerError before returning; ensure ControllerError is in scope or
import it if necessary.
| // Only used for testing, not to use in production. | ||
| pub async fn authenticate_test_token( | ||
| conn: &mut PgConnection, | ||
| token: &str, | ||
| _token: &SecretString, | ||
| application_configuration: &ApplicationConfiguration, | ||
| ) -> anyhow::Result<User> { | ||
| // Sanity check to ensure this is not called outside of test mode. The whole application configuration is passed to this function instead of just the boolean to make mistakes harder. | ||
| assert!(application_configuration.test_mode); | ||
| let user = models::users::get_by_email(conn, token).await?; | ||
| // TODO: this has never worked | ||
| let user = models::users::get_by_email(conn, "TODO").await?; | ||
| Ok(user) | ||
| } |
There was a problem hiding this comment.
Broken test-token auth path; implement minimal working logic
This function currently can’t succeed (“TODO”). Any test using bearer tokens in test_mode will fail.
pub async fn authenticate_test_token(
conn: &mut PgConnection,
- _token: &SecretString,
+ token: &SecretString,
application_configuration: &ApplicationConfiguration,
) -> anyhow::Result<User> {
// Sanity check to ensure this is not called outside of test mode. The whole application configuration is passed to this function instead of just the boolean to make mistakes harder.
assert!(application_configuration.test_mode);
- // TODO: this has never worked
- let user = models::users::get_by_email(conn, "TODO").await?;
- Ok(user)
+ // Minimal mapping for tests; extend as needed.
+ let email = match token.expose_secret().as_str() {
+ "admin" => "[email protected]",
+ "teacher" => "[email protected]",
+ "user" => "[email protected]",
+ other if other.ends_with("@example.com") => other,
+ _ => anyhow::bail!("Unknown test token"),
+ };
+ models::users::get_by_email(conn, email).await.map_err(Into::into)
}📝 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.
| // Only used for testing, not to use in production. | |
| pub async fn authenticate_test_token( | |
| conn: &mut PgConnection, | |
| token: &str, | |
| _token: &SecretString, | |
| application_configuration: &ApplicationConfiguration, | |
| ) -> anyhow::Result<User> { | |
| // Sanity check to ensure this is not called outside of test mode. The whole application configuration is passed to this function instead of just the boolean to make mistakes harder. | |
| assert!(application_configuration.test_mode); | |
| let user = models::users::get_by_email(conn, token).await?; | |
| // TODO: this has never worked | |
| let user = models::users::get_by_email(conn, "TODO").await?; | |
| Ok(user) | |
| } | |
| // Only used for testing, not to use in production. | |
| pub async fn authenticate_test_token( | |
| conn: &mut PgConnection, | |
| token: &SecretString, | |
| application_configuration: &ApplicationConfiguration, | |
| ) -> anyhow::Result<User> { | |
| // Sanity check to ensure this is not called outside of test mode. The whole application configuration is passed to this function instead of just the boolean to make mistakes harder. | |
| assert!(application_configuration.test_mode); | |
| // Minimal mapping for tests; extend as needed. | |
| let email = match token.expose_secret().as_str() { | |
| "admin" => "[email protected]", | |
| "teacher" => "[email protected]", | |
| "user" => "[email protected]", | |
| other if other.ends_with("@example.com") => other, | |
| _ => anyhow::bail!("Unknown test token"), | |
| }; | |
| models::users::get_by_email(conn, email).await.map_err(Into::into) | |
| } |
🤖 Prompt for AI Agents
In services/headless-lms/server/src/domain/authorization.rs around lines 915 to
926, the test-token auth path is a TODO and currently always fails; replace the
placeholder with minimal working logic that interprets the SecretString token as
the user's email address: extract the inner string from the SecretString,
validate it is non-empty (return an error if empty), call
models::users::get_by_email(conn, &email).await? to load the user, and return
it; keep the existing assert!(application_configuration.test_mode) and ensure
errors propagate via anyhow::Result rather than panicking.
| use anyhow::{Context, Result}; | ||
| use reqwest::Client; | ||
| use secrecy::{ExposeSecret, SecretString}; | ||
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::json; | ||
| use url::Url; | ||
| use uuid::Uuid; | ||
|
|
||
| use crate::ApplicationConfiguration; | ||
|
|
There was a problem hiding this comment.
Missing imports for logging macros
The code uses info!, error!, and debug! macros at lines 333, 346, 350, 353, 368, 381, and 385, but these macros are not imported. This will cause compilation errors.
Add the tracing macros to your imports:
use anyhow::{Context, Result};
use reqwest::Client;
use secrecy::{ExposeSecret, SecretString};
use serde::{Deserialize, Serialize};
use serde_json::json;
+use tracing::{debug, error, info};
use url::Url;
use uuid::Uuid;🤖 Prompt for AI Agents
In services/headless-lms/utils/src/tmc/mod.rs around lines 1 to 10, the tracing
macros info!, error!, and debug! are used later but not imported; add a use
statement to import those macros (e.g., use tracing::{info, error, debug};) at
the top of the file alongside the other imports so the macros resolve and the
crate compiles.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
services/headless-lms/server/src/controllers/auth.rs (3)
760-765: Rate‑limit sensitive endpoints to prevent spam/bruteforce.Add throttling for email code send and account deletion endpoints.
Apply:
- .route("/user-info", web::get().to(user_info)) - .route("/delete-user-account", web::post().to(delete_user_account)) - .route( - "/send-email-code", - web::post().to(send_delete_user_email_code), - ); + .route("/user-info", web::get().to(user_info)) + .service( + web::resource("/delete-user-account") + .wrap(build_rate_limiting_middleware(Duration::from_secs(60), 5)) + .to(delete_user_account), + ) + .service( + web::resource("/send-email-code") + .wrap(build_rate_limiting_middleware(Duration::from_secs(60), 3)) + .wrap(build_rate_limiting_middleware(Duration::from_secs(300), 10)) + .to(send_delete_user_email_code), + );
589-597: Secure RNG and remove duplicate insert for email codes.Use OsRng for codes and avoid inserting the same code twice.
Apply:
- let new_code: String = rand::rng().random_range(100_000..1_000_000).to_string(); + use rand::{rngs::OsRng, Rng}; + let new_code: String = OsRng.random_range(100_000..1_000_000).to_string(); @@ - models::user_email_codes::insert_user_email_code(&mut conn, auth_user.id, code.clone()) - .await?; + // Code already ensured/inserted above; no need to insert again.Also add the OsRng import at the top if not present:
-use rand::Rng; +use rand::{rngs::OsRng, Rng};Also applies to: 599-601
665-668: Also soft‑delete stored password hashes when deleting accounts.Avoid retaining credential material after account deletion.
Apply in this function:
- // Delete user locally and mark email code as used + // Soft-delete stored password, then delete user and mark code as used + user_passwords::soft_delete_by_user_id(&mut tx, auth_user.id).await?; users::delete_user(&mut tx, auth_user.id).await?; user_email_codes::mark_user_email_code_used(&mut tx, auth_user.id, &payload.code).await?;And add model helper (services/headless-lms/models/src/user_passwords.rs):
pub async fn soft_delete_by_user_id(conn: &mut PgConnection, user_id: Uuid) -> ModelResult<()> { sqlx::query!(r#"UPDATE user_passwords SET deleted_at = now() WHERE user_id = $1"#, user_id) .execute(conn) .await?; Ok(()) }services/headless-lms/server/src/domain/langs/token.rs (1)
83-86: Map TMC client errors into ControllerError;?won’t convert here.FromRequest returns ControllerError; anyhow::Error from TmcClient must be mapped.
Apply:
- let tmc_user = tmc_client - .get_user_from_tmc_mooc_fi_by_tmc_access_token(&token.clone()) - .await?; + let tmc_user = tmc_client + .get_user_from_tmc_mooc_fi_by_tmc_access_token(&token) + .await + .map_err(|err| { + ControllerError::new( + ControllerErrorType::Unauthorized, + "Failed to get current user from TMC".to_string(), + Some(err), + ) + })?;services/headless-lms/server/src/domain/authorization.rs (1)
915-926: Fix test‑token auth path; current TODO breaks test mode.Implement minimal working logic that maps token → test user email and loads the user.
Apply:
pub async fn authenticate_test_token( conn: &mut PgConnection, - _token: &SecretString, + token: &SecretString, application_configuration: &ApplicationConfiguration, ) -> anyhow::Result<User> { assert!(application_configuration.test_mode); - // TODO: this has never worked - let user = models::users::get_by_email(conn, "TODO").await?; - Ok(user) + let email = match token.expose_secret().as_str() { + "admin" => "[email protected]", + "teacher" => "[email protected]", + "user" => "[email protected]", + other if other.ends_with("@example.com") => other, + _ => anyhow::bail!("Unknown test token"), + }; + models::users::get_by_email(conn, email).await.map_err(Into::into) }services/headless-lms/utils/src/tmc/mod.rs (2)
1-10: Import tracing macros; otherwise info!/debug!/error! won’t resolve.Current file uses these macros unqualified.
Apply:
use anyhow::{Context, Result}; use reqwest::Client; use secrecy::{ExposeSecret, SecretString}; use serde::{Deserialize, Serialize}; use serde_json::json; +use tracing::{debug, error, info}; use url::Url; use uuid::Uuid;
27-34: Fix TmcUserInfo schema to match TMC JSON (idis numeric).Current struct will fail to deserialize and carries a redundant field.
Apply:
-#[derive(Debug, Deserialize)] -pub struct TmcUserInfo { - pub id: Uuid, - pub email: String, - pub first_name: Option<String>, - pub last_name: Option<String>, - pub upstream_id: i32, -} +#[derive(Debug, Deserialize)] +pub struct TmcUserInfo { + #[serde(rename = "id")] + pub upstream_id: i64, + pub email: String, + pub first_name: Option<String>, + pub last_name: Option<String>, +}Update call sites to use upstream_id.
🧹 Nitpick comments (6)
services/headless-lms/server/src/domain/langs/token.rs (2)
54-57: Avoid panic on missing TMC client. Return 500 instead.expect(...) will crash the worker if misconfigured. Prefer a controlled ControllerError.
Example:
let tmc_client = req .app_data::<web::Data<TmcClient>>() .cloned() .ok_or_else(|| ControllerError::new( ControllerErrorType::InternalServerError, "TMC client not configured".to_string(), None, ))?;
47-53: Parse “Bearer” case‑insensitively and robustly.Current strip_prefix("Bearer ") fails on lowercase and extra spaces.
Example:
let auth_header = req .headers() .get(header::AUTHORIZATION) .and_then(|hv| hv.to_str().ok()) .and_then(|h| h.split_once(' ')) .and_then(|(scheme, token)| (scheme.eq_ignore_ascii_case("bearer")).then(|| SecretString::new(token.to_string().into())));services/headless-lms/server/src/domain/authorization.rs (1)
317-353: Harden secret comparison against timing attacks (optional).Use constant‑time compare for Authorization header vs env secret.
Example:
use subtle::ConstantTimeEq; if auth_header.as_bytes().ct_eq(tmc_server_secret_for_communicating_to_secret_project.as_bytes()).into() { return Ok(skip_authorize()); }services/headless-lms/utils/src/tmc/mod.rs (1)
287-290: Avoid unwrap() on URL parsing.Prefer propagating a descriptive error to prevent panics.
Apply:
- let mut url = Url::parse(TMC_API_URL).unwrap(); + let mut url = Url::parse(TMC_API_URL).context("Invalid TMC_API_URL")?; url.path_segments_mut().unwrap().push("get_user_with_email");Also replace the second unwrap with a proper error:
url.path_segments_mut() .map_err(|_| anyhow::anyhow!("TMC_API_URL cannot be base for path segments"))? .push("get_user_with_email");services/headless-lms/server/src/controllers/auth.rs (2)
520-526: Remove unused email field from SendEmailCodeData.email isn’t used; drop to reduce PII surface.
Apply:
#[derive(Debug, Serialize, Deserialize)] pub struct SendEmailCodeData { - pub email: String, pub password: String, pub language: String, }
601-604: Minor: no need forlet _ = … .await?;Just await and propagate the error.
Apply:
- let _ = - models::email_deliveries::insert_email_delivery(&mut conn, user.id, delete_template.id) - .await?; + models::email_deliveries::insert_email_delivery(&mut conn, user.id, delete_template.id) + .await?;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
services/headless-lms/models/src/user_passwords.rs(1 hunks)services/headless-lms/server/src/controllers/auth.rs(8 hunks)services/headless-lms/server/src/controllers/main_frontend/users.rs(3 hunks)services/headless-lms/server/src/controllers/tmc_server/users.rs(1 hunks)services/headless-lms/server/src/domain/authorization.rs(9 hunks)services/headless-lms/server/src/domain/langs/token.rs(4 hunks)services/headless-lms/utils/src/tmc/mod.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/headless-lms/server/src/controllers/main_frontend/users.rs
- services/headless-lms/models/src/user_passwords.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-03T09:10:44.098Z
Learnt from: nygrenh
PR: rage/secret-project-331#1342
File: services/headless-lms/server/src/controllers/course_material/courses.rs:922-938
Timestamp: 2024-12-03T09:10:44.098Z
Learning: In `services/headless-lms/server/src/controllers/course_material/courses.rs`, when implementing endpoint functions that should be accessible to all users with access to the course material (e.g., students, assistants), use `authorize_access_to_course_material` instead of `authorize` with `Act::Teach` for authorization.
Applied to files:
services/headless-lms/server/src/domain/authorization.rs
🧬 Code graph analysis (5)
services/headless-lms/server/src/domain/langs/token.rs (1)
services/headless-lms/server/src/domain/authorization.rs (1)
get_or_create_user_from_tmc_mooc_fi_response(814-853)
services/headless-lms/utils/src/tmc/mod.rs (1)
services/headless-lms/server/src/controllers/auth.rs (1)
post_new_user_to_tmc(681-700)
services/headless-lms/server/src/controllers/tmc_server/users.rs (3)
services/headless-lms/server/src/domain/authorization.rs (1)
authorize_access_from_tmc_server_to_course_mooc_fi(319-353)services/headless-lms/models/src/user_passwords.rs (3)
verify_user_password(63-109)hash_password(49-61)upsert_user_password(27-47)services/headless-lms/server/src/controllers/tmc_server/mod.rs (1)
_add_routes(13-16)
services/headless-lms/server/src/controllers/auth.rs (7)
services/headless-lms/server/src/domain/authorization.rs (9)
session(75-75)session(117-117)session(158-158)upstream_id(60-62)skip_authorize(276-278)remember(142-154)authenticate_test_user(858-913)authenticate_moocfi_user(720-752)forget(169-171)services/headless-lms/utils/src/tmc/mod.rs (1)
post_new_user_to_tmc(228-261)services/headless-lms/models/src/users.rs (4)
insert_with_upstream_id_and_moocfi_id(51-91)get_by_id(125-138)get_by_email(93-107)delete_user(256-271)services/headless-lms/models/src/user_passwords.rs (4)
hash_password(49-61)upsert_user_password(27-47)verify_user_password(63-109)check_if_users_password_is_stored(111-128)services/headless-lms/models/src/email_templates.rs (1)
get_generic_email_template_by_name_and_language(54-82)services/headless-lms/models/src/user_email_codes.rs (4)
get_unused_user_email_code_with_user_id(53-82)insert_user_email_code(15-51)is_reset_user_email_code_valid(84-108)mark_user_email_code_used(110-131)services/headless-lms/models/src/email_deliveries.rs (1)
insert_email_delivery(25-49)
services/headless-lms/server/src/domain/authorization.rs (1)
services/headless-lms/models/src/users.rs (1)
get_by_email(93-107)
⏰ 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: code-style
- GitHub Check: build-and-system-tests
- GitHub Check: headless_lms
- GitHub Check: build-and-deploy
| #[instrument(skip(pool))] | ||
| pub async fn courses_moocfi_password_login( | ||
| request: HttpRequest, | ||
| pool: web::Data<PgPool>, | ||
| payload: web::Json<LoginRequest>, | ||
| ) -> ControllerResult<web::Json<bool>> { | ||
| let token = authorize_access_from_tmc_server_to_course_mooc_fi(&request).await?; |
There was a problem hiding this comment.
Do not record secrets in tracing spans; skip request and payload.
HttpRequest may include the Authorization header (shared TMC secret) and payload contains passwords. instrument currently captures them.
Apply:
-#[instrument(skip(pool))]
+#[instrument(skip(request, pool, payload))]
pub async fn courses_moocfi_password_login(
@@
-#[instrument(skip(pool))]
+#[instrument(skip(request, pool, payload))]
pub async fn courses_moocfi_password_change(Also applies to: 63-69
🤖 Prompt for AI Agents
In services/headless-lms/server/src/controllers/tmc_server/users.rs around lines
30-36, the #[instrument] macro currently records HttpRequest and payload (which
may contain Authorization headers and passwords); update the attribute to
#[instrument(skip(request, payload))] to prevent secrets from being captured,
and make the analogous change for the other handler noted at lines 63-69 (add
skip(request, payload) to that #[instrument] as well).
| let is_valid = models::user_passwords::verify_user_password(&mut conn, user_id, &password) | ||
| .await | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Don’t swallow DB errors during login. Return 500 on internal failures.
unwrap_or(false) conflates infra errors with invalid credentials.
Apply:
- let is_valid = models::user_passwords::verify_user_password(&mut conn, user_id, &password)
- .await
- .unwrap_or(false);
+ let is_valid = models::user_passwords::verify_user_password(&mut conn, user_id, &password)
+ .await
+ .map_err(|e| {
+ ControllerError::new(
+ ControllerErrorType::InternalServerError,
+ "Password verification failed".to_string(),
+ Some(e.into()),
+ )
+ })?;📝 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 is_valid = models::user_passwords::verify_user_password(&mut conn, user_id, &password) | |
| .await | |
| .unwrap_or(false); | |
| let is_valid = models::user_passwords::verify_user_password(&mut conn, user_id, &password) | |
| .await | |
| .map_err(|e| { | |
| ControllerError::new( | |
| ControllerErrorType::InternalServerError, | |
| "Password verification failed".to_string(), | |
| Some(e.into()), | |
| ) | |
| })?; |
🤖 Prompt for AI Agents
In services/headless-lms/server/src/controllers/tmc_server/users.rs around lines
42 to 44, the call to verify_user_password currently uses unwrap_or(false) which
hides database/infra errors and treats them as invalid credentials; change this
to handle the Result explicitly: match or ? the Result from
verify_user_password, and if it returns an Err, log the error and return an
internal server error response (HTTP 500) to the client instead of treating it
as a failed login; only treat Ok(false) as invalid credentials and proceed
accordingly.
| // Verify old password if it is not None | ||
| if let Some(old) = old_password { | ||
| let is_user_valid = models::user_passwords::verify_user_password(&mut conn, user_id, &old) | ||
| .await | ||
| .unwrap_or(false); | ||
| if !is_user_valid { | ||
| return token.authorized_ok(web::Json(false)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Change‑password: distinguish invalid old password from server errors.
Return false only for invalid old_password; propagate hashing/upsert/DB errors as 5xx.
Apply:
- if let Some(old) = old_password {
- let is_user_valid = models::user_passwords::verify_user_password(&mut conn, user_id, &old)
- .await
- .unwrap_or(false);
+ if let Some(old) = old_password {
+ let is_user_valid = models::user_passwords::verify_user_password(&mut conn, user_id, &old)
+ .await
+ .map_err(|e| {
+ ControllerError::new(
+ ControllerErrorType::InternalServerError,
+ "Old password verification failed".to_string(),
+ Some(e.into()),
+ )
+ })?;
if !is_user_valid {
return token.authorized_ok(web::Json(false));
}
}
- let new_password_hash = match models::user_passwords::hash_password(&new_password) {
- Ok(hash) => hash,
- Err(_) => return token.authorized_ok(web::Json(false)),
- };
+ let new_password_hash = models::user_passwords::hash_password(&new_password).map_err(|e| {
+ ControllerError::new(
+ ControllerErrorType::InternalServerError,
+ "Failed to hash new password".to_string(),
+ Some(e.into()),
+ )
+ })?;
- let update_ok =
- models::user_passwords::upsert_user_password(&mut conn, user_id, &new_password_hash)
- .await
- .unwrap_or(false);
+ let update_ok = models::user_passwords::upsert_user_password(
+ &mut conn,
+ user_id,
+ &new_password_hash,
+ )
+ .await
+ .map_err(|e| {
+ ControllerError::new(
+ ControllerErrorType::InternalServerError,
+ "Failed to store new password".to_string(),
+ Some(e.into()),
+ )
+ })?;Also applies to: 89-92, 94-98
🤖 Prompt for AI Agents
services/headless-lms/server/src/controllers/tmc_server/users.rs lines 79-87
(and similarly 89-92, 94-98): the code currently uses unwrap_or(false) when
verifying the old password which treats any DB/hashing error as “invalid
password” and returns false; instead match the Result from verify_user_password
and only return false on Ok(false), but on Err propagate the error as an
internal server error (5xx) response to the client; modify each occurrence to
handle Result properly (match or ?/map_err) so that Ok(true) continues,
Ok(false) returns token.authorized_ok(web::Json(false)), and Err(_) returns a
5xx response (e.g., early return with an Internal Server Error response) rather
than returning false.
Summary by CodeRabbit
New Features
UI & UX
Chores
Tests