feat(auth): mobile welcome email endpoint#2608
Merged
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbitRelease NotesNew Features
WalkthroughAdds a POST /mobile-welcome-email API with OpenAPI schema, HTML email template, handler that validates/normalizes emails, DB claiming logic, SES send, DB migration and client crate, IP rate-limiting middleware and cache client feature, and workspace/Cargo updates. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rs`:
- Around line 98-119: There’s a TOCTOU race between get_mobile_welcome_email and
insert_mobile_welcome_email that can cause duplicate sends; make the operation
atomic by changing insert_mobile_welcome_email to return a bool (true if
rows_affected() > 0) when performing the INSERT ... ON CONFLICT DO NOTHING, then
in mod.rs replace the pre-check using
macro_db_client::mobile_welcome_email::get_mobile_welcome_email with a single
call to
macro_db_client::mobile_welcome_email::insert_mobile_welcome_email(&ctx.db,
&lowercase_email).await? and only call ctx.ses_client.send_email(...) if that
insert returned true, otherwise return SendMobileWelcomeEmailError::AlreadySent.
In `@rust/cloud-storage/macro_db_client/src/mobile_welcome_email.rs`:
- Around line 2-6: The tracing attribute on get_mobile_welcome_email should
include error capture for Result-returning functions; update
#[tracing::instrument(skip(db))] to #[tracing::instrument(skip(db), err)] on
get_mobile_welcome_email and likewise add err to the tracing::instrument
attribute for the other Result-returning function(s) in the same file (the one
around lines 24–28) so errors are recorded in traces while still skipping the db
parameter.
- Around line 45-68: Move the inline #[cfg(test)] mod tests { ... } block into a
separate tests.rs file in the same module directory: remove the inline block
from mobile_welcome_email.rs, create tests.rs containing the test function
test_insert_and_get_mobile_welcome_email and its imports (use super::*; use
sqlx::{Pool, Postgres}; #[sqlx::test(migrations = "migrations")] async fn ...),
and keep calls to get_mobile_welcome_email and insert_mobile_welcome_email
unchanged; ensure the new tests.rs is compiled as a module (it will be picked up
as a sibling module file) and run cargo test to verify everything still passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcd0900d-a936-40f0-a240-24fa4d85137f
⛔ Files ignored due to path filters (32)
js/app/packages/service-clients/service-auth/generated/client.tsis excluded by!**/generated/**js/app/packages/service-clients/service-auth/generated/schemas/index.tsis excluded by!**/generated/**js/app/packages/service-clients/service-auth/generated/schemas/sendMobileWelcomeEmailRequest.tsis excluded by!**/generated/**rust/cloud-storage/macro_db_client/.sqlx/query-06a72bfcfbcc5f5bbed6cd093436309607f9375d2ba7b151e9dc1a8493f88e44.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-270cd8ae6b47508cb7e4e920bd76cb0f6177d685bddedb06f1892d0014af6f01.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-317f05707fdc73269f3017a3bb96d55b7327338bf5e52dfaad106b4b492cbb8e.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-3293eab63193f1b3822a59e9a6a9621520f26e6ec8248abed103985281ef7ff1.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-3357165799ff1f7bf1de3891174668e0ab16c64bd9dfc905213681e6021474fa.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-36dfcf982834f117d97d1ba332be6784c946f05120c7c228ff258389ceba97a1.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-48697f17a7121986cc770d6698fbca7bf107ae20dbadbf64a7799ab162b2feb4.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-4e096eb7d3c507e3647685073fba7b3f36ef0e70c887db31f5cc5a8eb8ee1844.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-621186dcec072b603a5f3d7b9e987a1638589d539c60d9dc5c2ab0865ff062d6.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-625ee33d48768e369ee1fe3abe27811ad04313b1aa31f4b51abe5fd3bf569674.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-73c67fbf678e1479d528adcc5bea7da3261d0a04e24cc16f96ec9f2dea3b2e98.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-74d33764e454fb9ab4f2775974462d6fdb092bffc720d929de5e7b50ecef5139.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-765650cab2b1a1d1abe6ca406438bf48968cebebc7316b87b192d649e096cd67.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-78f1cf9da464937604c1a2d5115c014cab9de72bd88605ff47a316baa2c7acc2.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-7c0b9d645a85e30577377e4d8c8a8f976e86c6251e12517a2b1e14d525e09198.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-7ccdaab5873129437c98581ac37eb4148255b83e18fad15349d06c54040fd6cc.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-7eaa606441665fa4dca3077c6b3c02e8c4a0d136a40149829b96dff878897108.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-86eee52e30e4168f4a7f319d1dcaa59144a5b3269120df892f161c74b5fe7436.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-92edc80110af7b96f7f908989461a4abb11652671a95454969369a0484ed9239.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-94c019227741f6c4fe8411454cb7c0a3a97e5594c29f11fa893238656e161ff1.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-a0046a263c572915a8e14a8ac70479f1ba89549f1930f571bf278fcfe259cfd1.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-a5d9f358b5382c0eaed918f903bc6aa328f527e6e47d649c37dfcf1c9f3a7139.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-b3f9f3696438bd2ad8754650d87d7bbf32e70d3d173a748a07eacdb4e42841eb.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-b6705d3675950d0bbbd4a0ba10f7716a24694cfd98741fc0508d607684b1c8a9.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-c27077a4053c8a35ca011c5edaf08e3bf1c954b1773fcd2ecab33c2ba518ba34.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-d20bc5b5dfecf0a2ef4b7d77b9090ad28584cd0a1fd24460096b6a5097a870ac.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-db297e24e3ed1e94c27121977d81813bbe4cde5e5b7bec42e424c72db0e27e73.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-efc740cd935fb67ef5c2a75b3c37cf16bf09d6e52e5f6e5c5b107ef8244a141d.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-f3e53dd531be738b881df199783a2beded3bd378038cbd6f012080ac77c5291b.jsonis excluded by!**/.sqlx/**
📒 Files selected for processing (8)
js/app/packages/service-clients/service-auth/openapi.jsonrust/cloud-storage/authentication_service/src/api/mobile_welcome_email/_welcome_email_template.htmlrust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rsrust/cloud-storage/authentication_service/src/api/mod.rsrust/cloud-storage/authentication_service/src/api/swagger.rsrust/cloud-storage/macro_db_client/migrations/20260415121644_create_mobile_welcome_email.sqlrust/cloud-storage/macro_db_client/src/lib.rsrust/cloud-storage/macro_db_client/src/mobile_welcome_email.rs
- Fix TOCTOU race: insert atomically then send, instead of check-then-send - Add err to tracing::instrument on Result-returning DB functions - Move tests to separate test.rs file per project conventions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
whutchinson98
requested changes
Apr 16, 2026
Member
There was a problem hiding this comment.
nit: we should really avoid adding new calls to macro_db_client
instead we should make a new crate just for this table which in theory could become a hex crate in the future. for now it could just contain sqlx dependency and use macrodb_migrator crate for tests.
| #[tracing::instrument(skip(ctx), fields(email=%req.email), err)] | ||
| pub async fn handler( | ||
| State(ctx): State<ApiContext>, | ||
| extract::Json(req): extract::Json<SendMobileWelcomeEmailRequest>, |
Member
There was a problem hiding this comment.
This should have ip rate limit extractor and rate limit based on IP to avoid potential spammers
| }; | ||
|
|
||
| // Check if the email is blocked | ||
| let blocked_emails = |
Member
There was a problem hiding this comment.
I believe the blocked_email table will also store non-normalized emails as well such as x+y@email.com rather than just x@email.com
I could be mistaken though
| .await?; | ||
|
|
||
| if !inserted { | ||
| return Err(SendMobileWelcomeEmailError::AlreadySent); |
Member
There was a problem hiding this comment.
imo this shouldn't be an error it should just be a 204 NO_CONTENT
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rs`:
- Around line 98-118: The insert_mobile_welcome_email call currently claims the
slot before calling ctx.ses_client.send_email, so if send_email fails the DB
record remains and blocks retries; update the flow in the function containing
mobile_welcome_email_db_client::mobile_welcome_email::insert_mobile_welcome_email
and ctx.ses_client.send_email to either wrap the insert+send in a DB transaction
and rollback on SES error, or catch a send_email failure and delete/undo the
inserted record before returning Err(SendMobileWelcomeEmailError::...);
alternatively, if you intend the current behavior, add a clear code comment next
to the insert_mobile_welcome_email call explaining the tradeoff and why no
rollback is performed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9118f502-9b85-48e7-8e90-6a7b56712ea5
⛔ Files ignored due to path filters (15)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lockrust/cloud-storage/macro_db_client/.sqlx/query-03a323aefdba22727c6789b28d90d7200e795a4591812dab4c4e7718c5512055.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-1435acbba365796ddcb6ddbef86f4ebf664cffc44082996f6b60a60373c28f75.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-37b5168f739c490a3373c65eab35f247a34af0a8e659cbad61cb99e3094e5a60.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-54e3755d63a676a7fa70da77321e618032f0130c73eb6b0ae985f07c5c6c448b.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-63492b018803ce26db5891964a0f52f37d2f54bd8243265c6561f4423af8617c.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-7617f808df94ffe77c8195a911ee6b230a21fe9935e099d1310d244248ca253a.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-79d3bc72e6f424765706f7deea426379ae49cbd70dd122daedd95343618b9ac6.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-8d21bf0aa4c20589a55cf5777aa9c658fdacea4d0d0c7070aded12cccd373911.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-96d853b887f2e6d7aac6b15ddd2b71a8d8900d2ef30047da7ca1a5940503b6d0.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-9cd40fbb1ade51e9e0d49792839d1a363cada017252a6f653c2272608189bcc4.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-d536db3ccfb32aee9a48227e2b15d2404023ea24b62c159466c95899d76e29ed.jsonis excluded by!**/.sqlx/**rust/cloud-storage/macro_db_client/.sqlx/query-f2d5d7aea9918978cd69c14f8afe24156c2fa387e001ee8eaf57e4a70a106069.jsonis excluded by!**/.sqlx/**rust/cloud-storage/mobile_welcome_email_db_client/.sqlx/query-7eaa606441665fa4dca3077c6b3c02e8c4a0d136a40149829b96dff878897108.jsonis excluded by!**/.sqlx/**rust/cloud-storage/mobile_welcome_email_db_client/.sqlx/query-c27077a4053c8a35ca011c5edaf08e3bf1c954b1773fcd2ecab33c2ba518ba34.jsonis excluded by!**/.sqlx/**
📒 Files selected for processing (8)
rust/cloud-storage/Cargo.tomlrust/cloud-storage/authentication_service/Cargo.tomlrust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rsrust/cloud-storage/mobile_welcome_email_db_client/Cargo.tomlrust/cloud-storage/mobile_welcome_email_db_client/justfilerust/cloud-storage/mobile_welcome_email_db_client/src/lib.rsrust/cloud-storage/mobile_welcome_email_db_client/src/mobile_welcome_email.rsrust/cloud-storage/mobile_welcome_email_db_client/src/mobile_welcome_email/test.rs
Comment on lines
+98
to
+118
| // Atomically claim the slot — returns false if the email was already sent | ||
| let inserted = | ||
| mobile_welcome_email_db_client::mobile_welcome_email::insert_mobile_welcome_email( | ||
| &ctx.db, | ||
| &lowercase_email, | ||
| ) | ||
| .await?; | ||
|
|
||
| if !inserted { | ||
| return Err(SendMobileWelcomeEmailError::AlreadySent); | ||
| } | ||
|
|
||
| let welcome_email_content = WELCOME_EMAIL_TEMPLATE.to_string(); | ||
| ctx.ses_client | ||
| .send_email( | ||
| "noreply@macro.com", | ||
| &lowercase_email, | ||
| "Welcome to Macro", | ||
| &welcome_email_content, | ||
| ) | ||
| .await?; |
Contributor
There was a problem hiding this comment.
Consider rollback strategy if email sending fails.
The current flow atomically claims the slot (insert), then sends the email. If send_email fails (lines 111-118), the database record persists, marking the email as "already sent" even though no email was delivered. This could permanently block retries for that user.
Consider either:
- Wrapping in a transaction and rolling back on SES failure
- Deleting the record on SES failure
- Accepting this as a tradeoff (user can contact support for manual re-send)
If option 3 is intentional, a code comment explaining this tradeoff would be helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rs`
around lines 98 - 118, The insert_mobile_welcome_email call currently claims the
slot before calling ctx.ses_client.send_email, so if send_email fails the DB
record remains and blocks retries; update the flow in the function containing
mobile_welcome_email_db_client::mobile_welcome_email::insert_mobile_welcome_email
and ctx.ses_client.send_email to either wrap the insert+send in a DB transaction
and rollback on SES error, or catch a send_email failure and delete/undo the
inserted record before returning Err(SendMobileWelcomeEmailError::...);
alternatively, if you intend the current behavior, add a clear code comment next
to the insert_mobile_welcome_email call explaining the tradeoff and why no
rollback is performed.
evanhutnik
force-pushed
the
evan/mobile-web-email-backend
branch
from
a3c9811 to
fc36125
Compare
evanhutnik
force-pushed
the
evan/mobile-web-email-backend
branch
from
092b5ba to
4714508
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
rust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rs (1)
104-124:⚠️ Potential issue | 🟠 MajorA failed SES call still burns the send slot.
Once
insertedistrue, any later SES error leaves the address permanently marked as sent, so retries return204even though no welcome email was delivered. Please either compensate on send failure or document that this loss of retryability is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rs` around lines 104 - 124, The code currently inserts the "sent" marker via mobile_welcome_email_db_client::mobile_welcome_email::insert_mobile_welcome_email (result stored in inserted) before calling ctx.ses_client.send_email, which means any SES error will leave the address marked as sent; change the flow so failure doesn't burn the slot: either (A) perform the SES send first and only call insert_mobile_welcome_email after send_email succeeds, or (B) keep the insert but make it a transactional/stateful insert (e.g. insert a pending status) and on SES error call the DB update/rollback to clear or mark as failed using the same mobile_welcome_email module; update error handling around ctx.ses_client.send_email to ensure the DB is compensated on failure and return appropriate error responses instead of leaving inserted=true with no retryability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/service-clients/service-auth/openapi.json`:
- Around line 913-957: Add an HTTP 409 response to the operation's "responses"
block to document the conflict case where the email is already present in
mobile_welcome_email: add a "409" entry with a brief description and an
application/json content schema referencing the existing ErrorResponse (same
shape used for 400/429/500) so clients can recognize and handle the "email
already recorded" conflict; update the OpenAPI "responses" object that currently
lists 200/204/400/429/500 to include this 409 error response.
In
`@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/mobile_welcome_email.rs`:
- Around line 43-49: The rate-limit rejection is logged with tracing::error!
which should be reduced to tracing::warn! because this is expected user
behavior; update the logging call in the mobile_welcome_email middleware that
references RATE_LIMIT_CONFIG.mobile_welcome_email, ip, count (the block starting
with if count >= RATE_LIMIT_CONFIG.mobile_welcome_email.0) to use tracing::warn!
instead of tracing::error! (preserving the same fields and message
"rate_limit_exceeded").
In
`@rust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rs`:
- Line 79: The tracing span is recording raw PII via fields(email=%req.email);
compute a non-reversible representation (e.g., SHA-256 hash or redacted form) of
req.email and record that instead. Concretely, remove or replace
fields(email=%req.email) in the #[tracing::instrument(...)] attribute and use a
field like email_hash or email_redacted; compute the hash/redaction early in the
handler (before the span or at the start of the function that has the traced
attribute) and reference that variable in the attribute (or add it to the span
via tracing::Span::current().record if you must compute after entry). Ensure you
only log the hashed/redacted variable and never the raw req.email in tracing
fields.
In `@rust/cloud-storage/authentication_service/src/rate_limit_config.rs`:
- Line 33: The rate limit tuple for mobile_welcome_email currently is (5, 3600)
but the comment states "10 attempts per hour per IP"; update the configuration
so the tuple matches the comment by changing mobile_welcome_email to (10, 3600)
and keep or adjust the inline comment to "10 attempts per hour per IP" (or
alternatively, if 5 is intended, update the comment to reflect 5 attempts);
locate the mobile_welcome_email entry in rate_limit_config.rs to make this
change.
---
Duplicate comments:
In
`@rust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rs`:
- Around line 104-124: The code currently inserts the "sent" marker via
mobile_welcome_email_db_client::mobile_welcome_email::insert_mobile_welcome_email
(result stored in inserted) before calling ctx.ses_client.send_email, which
means any SES error will leave the address marked as sent; change the flow so
failure doesn't burn the slot: either (A) perform the SES send first and only
call insert_mobile_welcome_email after send_email succeeds, or (B) keep the
insert but make it a transactional/stateful insert (e.g. insert a pending
status) and on SES error call the DB update/rollback to clear or mark as failed
using the same mobile_welcome_email module; update error handling around
ctx.ses_client.send_email to ensure the DB is compensated on failure and return
appropriate error responses instead of leaving inserted=true with no
retryability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61bc74c8-2500-493e-b830-cc3a4b2aed7d
⛔ Files ignored due to path filters (4)
js/app/packages/service-clients/service-auth/generated/client.tsis excluded by!**/generated/**rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lockrust/cloud-storage/mobile_welcome_email_db_client/.sqlx/query-7eaa606441665fa4dca3077c6b3c02e8c4a0d136a40149829b96dff878897108.jsonis excluded by!**/.sqlx/**rust/cloud-storage/mobile_welcome_email_db_client/.sqlx/query-c27077a4053c8a35ca011c5edaf08e3bf1c954b1773fcd2ecab33c2ba518ba34.jsonis excluded by!**/.sqlx/**
📒 Files selected for processing (17)
js/app/packages/service-clients/service-auth/openapi.jsonrust/cloud-storage/Cargo.tomlrust/cloud-storage/authentication_service/Cargo.tomlrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/mobile_welcome_email.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/mod.rsrust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rsrust/cloud-storage/authentication_service/src/api/mod.rsrust/cloud-storage/authentication_service/src/rate_limit_config.rsrust/cloud-storage/justfilerust/cloud-storage/macro_cache_client/Cargo.tomlrust/cloud-storage/macro_cache_client/src/lib.rsrust/cloud-storage/macro_cache_client/src/mobile_welcome_email_rate_limit.rsrust/cloud-storage/mobile_welcome_email_db_client/Cargo.tomlrust/cloud-storage/mobile_welcome_email_db_client/justfilerust/cloud-storage/mobile_welcome_email_db_client/src/lib.rsrust/cloud-storage/mobile_welcome_email_db_client/src/mobile_welcome_email.rsrust/cloud-storage/mobile_welcome_email_db_client/src/mobile_welcome_email/test.rs
Comment on lines
+913
to
+957
| "responses": { | ||
| "200": { | ||
| "description": "", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/EmptyResponse" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "204": { | ||
| "description": "" | ||
| }, | ||
| "400": { | ||
| "description": "", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "429": { | ||
| "description": "", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing 409 (Conflict) response documentation.
Per the PR objectives, the endpoint should return HTTP 409 when the email is already recorded in mobile_welcome_email. This response is not documented in the OpenAPI spec.
📝 Proposed fix - add 409 response
"429": {
"description": "",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
}
},
+ "409": {
+ "description": "Email already sent",
+ "content": {
+ "application/json": {
+ "schema": {
+ "$ref": "#/components/schemas/ErrorResponse"
+ }
+ }
+ }
+ },
"500": {📝 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.
Suggested change
| "responses": { | |
| "200": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/EmptyResponse" | |
| } | |
| } | |
| } | |
| }, | |
| "204": { | |
| "description": "" | |
| }, | |
| "400": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ErrorResponse" | |
| } | |
| } | |
| } | |
| }, | |
| "429": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ErrorResponse" | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ErrorResponse" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| "responses": { | |
| "200": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/EmptyResponse" | |
| } | |
| } | |
| } | |
| }, | |
| "204": { | |
| "description": "" | |
| }, | |
| "400": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ErrorResponse" | |
| } | |
| } | |
| } | |
| }, | |
| "429": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ErrorResponse" | |
| } | |
| } | |
| } | |
| }, | |
| "409": { | |
| "description": "Email already sent", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ErrorResponse" | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ErrorResponse" | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/service-clients/service-auth/openapi.json` around lines 913 -
957, Add an HTTP 409 response to the operation's "responses" block to document
the conflict case where the email is already present in mobile_welcome_email:
add a "409" entry with a brief description and an application/json content
schema referencing the existing ErrorResponse (same shape used for 400/429/500)
so clients can recognize and handle the "email already recorded" conflict;
update the OpenAPI "responses" object that currently lists 200/204/400/429/500
to include this 409 error response.
Comment on lines
+43
to
+49
| if count >= RATE_LIMIT_CONFIG.mobile_welcome_email.0 { | ||
| tracing::error!( | ||
| ip = ip, | ||
| rate_limit = RATE_LIMIT_CONFIG.mobile_welcome_email.0, | ||
| count = count, | ||
| "rate_limit_exceeded" | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reducing log level for expected rate-limit rejections.
Using tracing::error! for rate limit exceeded is appropriate for alerting, but this is expected user behavior rather than a system error. Consider using tracing::warn! to avoid polluting error logs with expected throttling events.
📝 Suggested change
if count >= RATE_LIMIT_CONFIG.mobile_welcome_email.0 {
- tracing::error!(
+ tracing::warn!(
ip = ip,
rate_limit = RATE_LIMIT_CONFIG.mobile_welcome_email.0,
count = count,
"rate_limit_exceeded"
);📝 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.
Suggested change
| if count >= RATE_LIMIT_CONFIG.mobile_welcome_email.0 { | |
| tracing::error!( | |
| ip = ip, | |
| rate_limit = RATE_LIMIT_CONFIG.mobile_welcome_email.0, | |
| count = count, | |
| "rate_limit_exceeded" | |
| ); | |
| if count >= RATE_LIMIT_CONFIG.mobile_welcome_email.0 { | |
| tracing::warn!( | |
| ip = ip, | |
| rate_limit = RATE_LIMIT_CONFIG.mobile_welcome_email.0, | |
| count = count, | |
| "rate_limit_exceeded" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/mobile_welcome_email.rs`
around lines 43 - 49, The rate-limit rejection is logged with tracing::error!
which should be reduced to tracing::warn! because this is expected user
behavior; update the logging call in the mobile_welcome_email middleware that
references RATE_LIMIT_CONFIG.mobile_welcome_email, ip, count (the block starting
with if count >= RATE_LIMIT_CONFIG.mobile_welcome_email.0) to use tracing::warn!
instead of tracing::error! (preserving the same fields and message
"rate_limit_exceeded").
| (status = 500, body = ErrorResponse), | ||
| ) | ||
| )] | ||
| #[tracing::instrument(skip(ctx), fields(email=%req.email), err)] |
Contributor
There was a problem hiding this comment.
Stop recording the raw email address in tracing spans.
req.email is PII, and this span is created for every request, including invalid input. Please log a redacted or hashed form instead so traces do not retain full user email addresses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/authentication_service/src/api/mobile_welcome_email/mod.rs`
at line 79, The tracing span is recording raw PII via fields(email=%req.email);
compute a non-reversible representation (e.g., SHA-256 hash or redacted form) of
req.email and record that instead. Concretely, remove or replace
fields(email=%req.email) in the #[tracing::instrument(...)] attribute and use a
field like email_hash or email_redacted; compute the hash/redaction early in the
handler (before the span or at the start of the function that has the traced
attribute) and reference that variable in the attribute (or add it to the span
via tracing::Span::current().record if you must compute after entry). Ensure you
only log the hashed/redacted variable and never the raw req.email in tracing
fields.
| merge_email_daily: (5, 86400), | ||
|
|
||
| create_user_hourly: (50, 3600), | ||
| mobile_welcome_email: (5, 3600), // 10 attempts per hour per IP |
Contributor
There was a problem hiding this comment.
Comment does not match the actual configuration value.
The config is set to (5, 3600) which means 5 attempts per hour, but the comment states "10 attempts per hour per IP".
📝 Proposed fix
- mobile_welcome_email: (5, 3600), // 10 attempts per hour per IP
+ mobile_welcome_email: (5, 3600), // 5 attempts per hour per IP📝 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.
Suggested change
| mobile_welcome_email: (5, 3600), // 10 attempts per hour per IP | |
| mobile_welcome_email: (5, 3600), // 5 attempts per hour per IP |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/authentication_service/src/rate_limit_config.rs` at line
33, The rate limit tuple for mobile_welcome_email currently is (5, 3600) but the
comment states "10 attempts per hour per IP"; update the configuration so the
tuple matches the comment by changing mobile_welcome_email to (10, 3600) and
keep or adjust the inline comment to "10 attempts per hour per IP" (or
alternatively, if 5 is intended, update the comment to reflect 5 attempts);
locate the mobile_welcome_email entry in rate_limit_config.rs to make this
change.
|
|
||
| //! Database client for the mobile welcome email table. | ||
|
|
||
| pub mod mobile_welcome_email; |
Member
There was a problem hiding this comment.
nit: do you need a module for this when the crate is called modile_welcome_email_db_client?
whutchinson98
approved these changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new
POST /mobile-welcome-emailendpoint to the authentication service that sends a welcome email to users who sign up via the mobile web flow.Endpoint behavior:
+alias suffixes (e.g.evan+test@macro.com→evan@macro.com)BlockedEmailtable and rejects blocked addresses (400)mobile_welcome_emailtable and rejects duplicates (409)mobile_welcome_emailto prevent re-sendsChanges:
mobile_welcome_emailPostgres table (migration) withemail(PK) andcreated_atmacro_db_client::mobile_welcome_emailmodule withget/insertqueriesauthentication_service::api::mobile_welcome_emailhandler withthiserror-based error type