Add an api endpoint for TMC to post a new user and migrate password#1653
Add an api endpoint for TMC to post a new user and migrate password#1653
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a POST endpoint Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as Handler
participant Auth as Authentication
participant TMC as TmcClient
participant DB as Database
participant Crypto as PasswordHasher
Client->>Handler: POST /api/v0/tmc-server/users/create {upstream_id, password}
Handler->>Auth: verify credentials
Auth-->>Handler: auth result
Handler->>TMC: fetch TMC MoocFi user (upstream_id)
TMC-->>Handler: TMC user data
Handler->>DB: get_or_create local user from TMC data
DB-->>Handler: local user
Handler->>Crypto: hash provided password
Crypto-->>Handler: password hash
Handler->>DB: store password hash (transactional) and commit
DB-->>Handler: commit result
Handler->>TMC: notify password managed by courses.mooc.fi (inline retries)
alt inline notify fails
Handler->>Handler: schedule background notification fallback
end
TMC-->>Handler: acknowledgment
Handler-->>Client: CreateUserResponse {user, password_set}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@services/headless-lms/server/src/controllers/tmc_server/users.rs`:
- Around line 113-128: The current code maps any failure from
tmc_client.set_user_password_managed_by_courses_mooc_fi into a ControllerError
after tx.commit(), which returns an InternalServerError despite the DB commit
succeeding; instead, change the handling in the block that calls
tmc_client.set_user_password_managed_by_courses_mooc_fi(upstream_id.to_string(),
user.id).await so failures are caught and logged as a warning (include context:
upstream_id and user.id and the error message) and do not propagate as
ControllerError — i.e., remove the map_err->ControllerError path and replace
with a match or if let Err(e) => { warn!(...) } so the handler returns
token.authorized_ok(web::Json(CreateUserResponse { user, password_set })) on
success regardless of TMC notification outcome; optionally mention adding a
response flag (tmc_notified: bool) if you want to signal the partial success.
🧹 Nitpick comments (2)
services/headless-lms/server/src/controllers/tmc_server/users.rs (2)
66-113: Several inline comments restate what the code already expresses.Lines 83, 88, and 110 largely repeat the function/method names they precede (e.g.,
// Set passwordbeforehash_password/upsert_user_password). Per coding guidelines, comments should add information not obvious from the code. Consider trimming these; Line 80's comment about atomicity is the most valuable of the group and worth keeping. As per coding guidelines, "Do not add comments that simply restate what the code already says… Comments should… communicate information that is not immediately obvious from the code itself."
96-108:password_setis alwaystruewhen the response is returned.Since Lines 99-108 return an error when
password_setisfalse, the field inCreateUserResponsewill always betrueon a successful response. Consider whether this field adds value to the API contract, or if the error return already communicates failure sufficiently. If the intent is forward-compatibility, it's fine to keep.
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: 1
🧹 Nitpick comments (2)
services/headless-lms/server/src/controllers/tmc_server/users.rs (2)
66-110: A few inline comments restate the code rather than adding context.Per coding guidelines, comments should communicate information not immediately obvious from the code itself.
- Line 83:
// Create user in headless-lms (or fetch if already exists)just paraphrases the function nameget_or_create_user_from_tmc_mooc_fi_response.- Line 88:
// Set passwordis a pure restatement of the lines that follow.- Line 110:
// Commit the transaction only if everything succeededimplies a conditional commit, but thetx.commit()call is unconditional at that point (the only conditions are the early returns above it).♻️ Suggested removals
- // Create user in headless-lms (or fetch if already exists) let user = get_or_create_user_from_tmc_mooc_fi_response(&mut tx, tmc_user).await?; info!("User {} created or fetched successfully", user.id); - // Set password let password_hash = models::user_passwords::hash_password(&password).map_err(|e| {- // Commit the transaction only if everything succeeded tx.commit().await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/controllers/tmc_server/users.rs` around lines 66 - 110, Remove or replace the redundant inline comments that merely restate nearby code: delete the comment above the call to get_or_create_user_from_tmc_mooc_fi_response(...) ("Create user in headless-lms..."), remove the "// Set password" comment before calling models::user_passwords::hash_password and models::user_passwords::upsert_user_password, and either delete or correct the misleading comment about committing the transaction near tx.commit() so it accurately reflects whether commit is conditional; if additional context is needed, add a short note explaining why a transaction is started or why upsert_user_password returning false is treated as an error rather than restating function names.
27-30:password_setinCreateUserResponseis alwaystrue.The guard on Lines 99–108 returns an error when
upsert_user_passwordreturnsfalse, so by the timeCreateUserResponseis constructed on Line 147,password_setis guaranteed to betrue. The field is misleading to callers and the log on Line 145 (info!("Password set: {}", password_set)) is always trivially true.Consider removing the field (and the early-return guard, treating a
falseresult as the error it already signals through?), or changing the guard to allowfalseto propagate as a meaningful response value rather than an error.♻️ Simplification if the field is dropped
-#[derive(Debug, Serialize)] -pub struct CreateUserResponse { - pub user: User, - pub password_set: bool, -} +#[derive(Debug, Serialize)] +pub struct CreateUserResponse { + pub user: User, +}- if !password_set { - return Err(ControllerError::new( - ControllerErrorType::InternalServerError, - "Failed to set password", - Some(anyhow::Error::msg(format!( - "upsert_user_password returned false for user {}", - user.id - ))), - )); - } - // Commit the transaction only if everything succeeded tx.commit().await?;- info!("Password set: {}", password_set); - - token.authorized_ok(web::Json(CreateUserResponse { user, password_set })) + token.authorized_ok(web::Json(CreateUserResponse { user }))Also applies to: 99-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/controllers/tmc_server/users.rs` around lines 27 - 30, The CreateUserResponse struct includes password_set but upsert_user_password is currently guarded such that a false return is turned into an early error, making password_set always true; update the code by either (A) dropping password_set from CreateUserResponse and remove the guard that converts a false from upsert_user_password into an early return (allow upsert_user_password? to return Result<bool, _> and propagate errors via ?), and remove the always-true info!("Password set: {}", password_set) log, or (B) change the guard around upsert_user_password so it does not treat a false result as an error (let it return Ok(false) and use that value when constructing CreateUserResponse and logging); reference CreateUserResponse, upsert_user_password, and the info! log to locate and update the struct, the password upsert call/guard, and the response construction accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/headless-lms/server/src/controllers/tmc_server/users.rs`:
- Around line 115-143: Reduce the inline retry budget and move long retries to a
detached background task: change the inline loop around
tmc_client.set_user_password_managed_by_courses_mooc_fi to use only ~3 attempts
with an exponential backoff capped at ~2000ms (e.g., MAX_ATTEMPTS_INLINE = 3,
MAX_DELAY_MS_INLINE = 2_000) so the HTTP response isn't blocked; if those inline
attempts still fail, spawn a tokio::spawn async task that runs the original
longer retry loop (the previous MAX_ATTEMPTS and MAX_DELAY_MS logic) using
cloned tmc_client, upstream_id and user.id, logging failures but not returning
ControllerError from the request path; update the error/return path that
currently calls ControllerError::new so the handler returns promptly while long
retries proceed in the background and only background logs/report on ultimate
failure.
---
Duplicate comments:
In `@services/headless-lms/server/src/controllers/tmc_server/users.rs`:
- Around line 135-141: The Err(e) branch currently returns a
ControllerError::new InternalServerError after tx.commit() has already
succeeded, causing a false failure to be reported to TMC; change this to log the
error (include context and the error e) instead of returning an error and then
return the successful CreateUserResponse so TMC sees the creation succeeded;
keep the ControllerError construction removed from this path, and if needed emit
a warning to trigger background reconciliation or enqueue a retry job for
notifying TMC asynchronously.
---
Nitpick comments:
In `@services/headless-lms/server/src/controllers/tmc_server/users.rs`:
- Around line 66-110: Remove or replace the redundant inline comments that
merely restate nearby code: delete the comment above the call to
get_or_create_user_from_tmc_mooc_fi_response(...) ("Create user in
headless-lms..."), remove the "// Set password" comment before calling
models::user_passwords::hash_password and
models::user_passwords::upsert_user_password, and either delete or correct the
misleading comment about committing the transaction near tx.commit() so it
accurately reflects whether commit is conditional; if additional context is
needed, add a short note explaining why a transaction is started or why
upsert_user_password returning false is treated as an error rather than
restating function names.
- Around line 27-30: The CreateUserResponse struct includes password_set but
upsert_user_password is currently guarded such that a false return is turned
into an early error, making password_set always true; update the code by either
(A) dropping password_set from CreateUserResponse and remove the guard that
converts a false from upsert_user_password into an early return (allow
upsert_user_password? to return Result<bool, _> and propagate errors via ?), and
remove the always-true info!("Password set: {}", password_set) log, or (B)
change the guard around upsert_user_password so it does not treat a false result
as an error (let it return Ok(false) and use that value when constructing
CreateUserResponse and logging); reference CreateUserResponse,
upsert_user_password, and the info! log to locate and update the struct, the
password upsert call/guard, and the response construction accordingly.
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: 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/server/src/controllers/tmc_server/users.rs (1)
1-9:⚠️ Potential issue | 🟡 MinorModule doc comment is stale after adding the
/createendpoint.The current description says the endpoints are "used by the TMC server to verify whether a user's email and password match what is stored in this system," which no longer covers the new user-creation endpoint. Update it to reflect the full scope of the module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/controllers/tmc_server/users.rs` around lines 1 - 9, Update the module-level doc comment at the top of users.rs to reflect that this module exposes multiple handlers under /api/v0/tmc-server/users/, including both user verification (email/password checks) and a new /create endpoint for creating users; state which endpoints require a valid authorization header if different from creation, and ensure the description covers the full scope (verification, creation, and access requirements) so the comment is no longer stale.
🧹 Nitpick comments (1)
services/headless-lms/server/src/controllers/tmc_server/users.rs (1)
66-66: Low-value comment restates the code.
// Fetch user details from tmc.mooc.fisays exactly what the next line already says. As per coding guidelines, comments should convey information not immediately obvious from the code itself — rationale, constraints, or non-obvious behavior. Remove it or replace it with something meaningful (e.g. why this fetch happens before the transaction, or what happens if the TMC call fails here).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/controllers/tmc_server/users.rs` at line 66, The inline comment "// Fetch user details from tmc.mooc.fi" is redundant; either remove it or replace it with a brief rationale that isn't obvious from the code (for example explain why the TMC fetch occurs before the DB transaction, or document how failures from the TMC call are handled/retried). Locate the exact comment string in users.rs and update it to convey non-obvious behavior or error handling expectations, or delete it if no additional context is necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/headless-lms/server/src/controllers/tmc_server/users.rs`:
- Line 59: The code currently calls pool.acquire().await? to get conn and then
begins a transaction, holding the pooled connection (conn) across later retry
sleeps; change these patterns to call pool.begin().await? (using the
Acquire::begin implementation on PgPool) so you get a Transaction (tx) directly
and the underlying pool slot is returned to the pool as soon as tx.commit()
completes; replace the two-step acquire + conn.begin() usage (the conn variable
and its begin() calls) with direct pool.begin() and ensure you drop/commit the
Transaction (tx) before any sleep/retry loops—apply the same change for other
occurrences (the acquire at the other location referenced around the conn/tx
usage).
---
Outside diff comments:
In `@services/headless-lms/server/src/controllers/tmc_server/users.rs`:
- Around line 1-9: Update the module-level doc comment at the top of users.rs to
reflect that this module exposes multiple handlers under
/api/v0/tmc-server/users/, including both user verification (email/password
checks) and a new /create endpoint for creating users; state which endpoints
require a valid authorization header if different from creation, and ensure the
description covers the full scope (verification, creation, and access
requirements) so the comment is no longer stale.
---
Nitpick comments:
In `@services/headless-lms/server/src/controllers/tmc_server/users.rs`:
- Line 66: The inline comment "// Fetch user details from tmc.mooc.fi" is
redundant; either remove it or replace it with a brief rationale that isn't
obvious from the code (for example explain why the TMC fetch occurs before the
DB transaction, or document how failures from the TMC call are handled/retried).
Locate the exact comment string in users.rs and update it to convey non-obvious
behavior or error handling expectations, or delete it if no additional context
is necessary.
services/headless-lms/server/src/controllers/tmc_server/users.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/headless-lms/server/src/controllers/tmc_server/users.rs (2)
139-139: Prefertokio::spawn(...);overdrop(tokio::spawn(...)).Wrapping the
JoinHandleindrop()is functionally identical to a bare expression statement, but it's an unusual pattern that adds noise. The idiomatic way to intentionally detach a spawned task is simply:♻️ Proposed refactor
- drop(tokio::spawn(async move { + tokio::spawn(async move { // ... - })); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/controllers/tmc_server/users.rs` at line 139, The code currently calls drop(tokio::spawn(async move { ... })) which is noisy; replace that pattern with a detached spawn by simply calling tokio::spawn(async move { ... }); so the JoinHandle is discarded idiomatically—locate the spawn call in users.rs (the drop(tokio::spawn(...)) expression) and remove the surrounding drop(...) wrapper leaving tokio::spawn(...); as the statement.
136-174: Background task has no shutdown coordination — retries will be silently abandoned on process exit.
tokio::spawndetaches the task from the request lifecycle, which is intentional. However, if the Tokio runtime begins shutdown (e.g., a SIGTERM is received) while the background retry loop is sleeping or executing, the task is dropped without completing or logging a final warning. For a notification that has already failed the inline path, silent abandonment at shutdown means the TMC inconsistency will never be surfaced.Consider one of:
- Tracking outstanding background tasks via a
JoinSetor a tokioCancellationTokenwith a shutdown hook, so the application can drain them gracefully on exit.- Persisting the pending notification to a database outbox table so it survives a restart, which also makes it resilient to crashes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/controllers/tmc_server/users.rs` around lines 136 - 174, The detached tokio::spawn background retry loop started when inline_succeeded is false (wrapping the call to tmc_client.set_user_password_managed_by_courses_mooc_fi) can be dropped silently on process shutdown; replace the fire-and-forget spawn with coordinated shutdown handling by either (a) registering the task with a shared JoinSet (or storing JoinHandle and awaiting/aborting them in your shutdown hook) or (b) wiring a CancellationToken that the runtime shutdown signal triggers so the retry loop can exit cleanly and log final state; alternatively persist the pending notification to an outbox table and have a resilient background worker read/ack it on restart—apply this change around the tokio::spawn/drop block and the retry loop logic (MAX_ATTEMPTS_BG, delays, and logging) so tasks are tracked and gracefully drained on shutdown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/headless-lms/server/src/controllers/tmc_server/users.rs`:
- Line 139: The code currently calls drop(tokio::spawn(async move { ... }))
which is noisy; replace that pattern with a detached spawn by simply calling
tokio::spawn(async move { ... }); so the JoinHandle is discarded
idiomatically—locate the spawn call in users.rs (the drop(tokio::spawn(...))
expression) and remove the surrounding drop(...) wrapper leaving
tokio::spawn(...); as the statement.
- Around line 136-174: The detached tokio::spawn background retry loop started
when inline_succeeded is false (wrapping the call to
tmc_client.set_user_password_managed_by_courses_mooc_fi) can be dropped silently
on process shutdown; replace the fire-and-forget spawn with coordinated shutdown
handling by either (a) registering the task with a shared JoinSet (or storing
JoinHandle and awaiting/aborting them in your shutdown hook) or (b) wiring a
CancellationToken that the runtime shutdown signal triggers so the retry loop
can exit cleanly and log final state; alternatively persist the pending
notification to an outbox table and have a resilient background worker read/ack
it on restart—apply this change around the tokio::spawn/drop block and the retry
loop logic (MAX_ATTEMPTS_BG, delays, and logging) so tasks are tracked and
gracefully drained on shutdown.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/headless-lms/server/src/controllers/tmc_server/users.rs (1)
136-175: Background task is silently dropped on runtime shutdown — inconsistent state may go unrecovered.Shutting down a Tokio runtime immediately cancels all tasks, stopping them at whichever
.awaitthey have yielded at. Since theJoinHandleis discarded here, there is no way to wait for the background task to finish during graceful shutdown. If the process restarts while the task is sleeping between retries, the DB already records the password as managed by courses.mooc.fi but TMC has never been notified — and nothing will retry again after restart.Two options to improve recoverability:
- Operator alerting: if the task exhausts all 10 background retries and still fails, emit a structured
error!log containing enough context (upstream_id, user_id, timestamp) for an on-call engineer to trigger a manual reconciliation.- Persistent job queue / outbox: use a DB-backed job table (an "outbox") so that pending notifications survive process restarts. This is the robust solution for this class of eventual-consistency problem.
Option 1 is low-effort and already partially present (line 167–170 emits an
error!); verify that it includes sufficient context for reconciliation and that your alerting stack will fire on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/controllers/tmc_server/users.rs` around lines 136 - 175, The background tokio::spawn in users.rs (the block calling tmc_client.set_user_password_managed_by_courses_mooc_fi with MAX_ATTEMPTS_BG) can be dropped on shutdown leaving TMC uninformed; update the final Err branch (where attempt == MAX_ATTEMPTS_BG) to emit a structured error log containing upstream_id, user_id, MAX_ATTEMPTS_BG, and a timestamp (e.g. Utc::now()) so alerting can act, and additionally persist a durable outbox/job record (call a new helper like persist_tmc_notification_outbox(upstream_id, user_id) or enqueue_tmc_retry) so the notification survives restarts; modify or add those calls near the existing error! in the tokio::spawn closure and keep tokio::spawn usage but ensure the outbox persistence is synchronous/awaited before the task finishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/headless-lms/server/src/controllers/tmc_server/users.rs`:
- Around line 136-175: The background tokio::spawn in users.rs (the block
calling tmc_client.set_user_password_managed_by_courses_mooc_fi with
MAX_ATTEMPTS_BG) can be dropped on shutdown leaving TMC uninformed; update the
final Err branch (where attempt == MAX_ATTEMPTS_BG) to emit a structured error
log containing upstream_id, user_id, MAX_ATTEMPTS_BG, and a timestamp (e.g.
Utc::now()) so alerting can act, and additionally persist a durable outbox/job
record (call a new helper like persist_tmc_notification_outbox(upstream_id,
user_id) or enqueue_tmc_retry) so the notification survives restarts; modify or
add those calls near the existing error! in the tokio::spawn closure and keep
tokio::spawn usage but ensure the outbox persistence is synchronous/awaited
before the task finishes.
1 participant
Add an api endpoint where TMC posts new users immediately after they're created in TMC.
The endpoint also moves password management for the user to courses's side.
Summary by CodeRabbit