fix(customers): close PR #149 review findings + tracking issue #173#184
Merged
mostlyvirtual merged 8 commits intomasterfrom May 6, 2026
Merged
fix(customers): close PR #149 review findings + tracking issue #173#184mostlyvirtual merged 8 commits intomasterfrom
mostlyvirtual merged 8 commits intomasterfrom
Conversation
… cross-tenant enumeration guard PR #149 introduced a customer-user invite flow that called the private SecureCustomerUserService._send_welcome_email_secure() directly from two views, bypassing project guardrails. Hardening: - Add public SecureCustomerUserService.send_welcome_invite() wrapper with a per-user cache guard (3 sends/hour, configurable via security.welcome_invite_limit_per_user_per_hour). Mirrors the pattern in _notify_owners_of_join_request_secure. - Switch both call sites to the public wrapper (resolves M1). - Add @throttle_classes([BurstAPIThrottle]) to customer_users_create matching the pattern used by update_customer_billing_address — limits the email-bomb surface even before the per-user guard kicks in. - Replace django.core.validators.validate_email with SecureInputValidator.validate_email_secure (timing-safe, normalizes to lowercase, length-bounded). - Differentiate in-org vs cross-tenant existence: * If user is already a member of THIS customer -> 409 "already a member of your organization" (allowed info — staff can see their own org). * If user exists in another tenant -> 400 "Cannot create a user with this email" (no enumeration leak). Mirrors the IntegrityError race path so concurrent inserts do not reveal whether the row landed in another tenant. Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
…emails PR #149's invite-email fix is non-blocking on email failure: if SMTP times out or the address has a typo, the user row is committed with an unusable password and no invite. Until now there was no recovery path — staff had to manually trigger a Django password reset. Add POST /customers/<customer_id>/user/<user_id>/resend-invite/ which: - Requires staff_required + POST. - Verifies the user is a member of THIS customer (no cross-tenant ops). - Verifies the user still has an unusable password — once they have set one, they should use the standard password-reset flow rather than have staff potentially overwrite their session via a re-issued reset token. - Calls the public SecureCustomerUserService.send_welcome_invite() so the per-user rate-limit guard applies. - Surface the URL in the messages.warning() emitted by customer_create_user so staff get a one-click recovery link inline with the failure notice. Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
Three small template hygiene issues from the PR #149 review: - HTML heading bound an unused customer_name variable to a string that did not reference it, producing a noisy .po entry. Restructure as one blocktrans block "You have been invited to {customer_name} on PRAHO" that leads with company context (better UX for invitees who do not yet recognise the PRAHO brand). - All multi-line blocktrans blocks now use the trimmed modifier so .po msgids do not pick up template-indentation whitespace. - The .txt template's first line had {% load i18n %}{% blocktrans %} on a single line; split into separate tags with a blank line so makemessages extracts a clean msgid. Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
… rate limit, resend New tests for the PR #149 follow-up hardening. Each one fails if the matching production fix is reverted: API view (test_customer_api.py): - test_create_user_existing_in_other_customer_returns_generic_400: asserts the generic message ("Cannot create"), no "already" wording — guards against cross-tenant enumeration regression. - test_create_user_already_in_this_customer_returns_409: 409 + specific in-org message — preserves useful staff feedback for same-tenant dups. - test_create_user_invalid_email_returns_400_before_db_write: malformed address rejected with no User row created (validate_email_secure path). - test_create_user_membership_failure_rolls_back_user: simulated IntegrityError on CustomerMembership.create asserts User row count unchanged — proves the transaction.atomic() block actually rolls back. - test_create_user_propagates_request_ip: the IP from get_safe_client_ip() reaches _send_welcome_email_secure unchanged. Staff view (test_user_management_views.py): - CustomerCreateUserSecurityTests covers the same in-org vs cross-tenant message split and invalid-email rejection. - SendWelcomeInviteRateLimitTests verifies the per-user cache guard blocks the 4th send within an hour and that failed sends do not consume the quota. Uses @override_settings(CACHES=LOCMEM_TEST_CACHE) because the default test cache is DummyCache. - CustomerResendInviteTests covers the H2 recovery endpoint: success case, blocked when the user already has a usable password, and blocked when the user is not a member of the customer. Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
…173 item 5) Issue #173 item 5 noted that all PR #149 invite-flow tests mock the email helper, so nothing in the suite catches a regression in the email templates themselves (broken {% url %} tag, misspelled context var, template syntax error, etc). Add CustomerCreateUserEmailRenderingIntegrationTests with one focused end-to-end test that: - Drives customer_create_user via the Django test Client (no mocks of send_welcome_invite or _send_welcome_email_secure). - Asserts mail.outbox grows by exactly one message. - Asserts the To/Subject/body/HTML alternative match the customer. - Extracts the password-reset URL from the body and verifies the token validates against default_token_generator for the new user — this is the strict check that fails if the template renders {{ token }} as a literal or generates a token for the wrong user. Mutation-tested before commit by changing the {% url %} tag in welcome_email.html to a non-existent route name and confirming the test failed with "0 != 1 : Exactly one email should have been sent" — proving the test catches the regression class it claims to catch. Template was then reverted; only the test is committed. Uses @override_settings(CACHES=LOCMEM_TEST_CACHE) so the per-user send_welcome_invite cache guard is exercised end-to-end (not bypassed via DummyCache). Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
…, display-name fallback (closes #173 items 2/3/6) Three remaining items from issue #173: Item 2 — Duplicate _send_welcome_email_secure on two service classes (byte-for-byte identical at lines 364 and 854 of users/services.py). Extract to a module-level _render_and_send_welcome_email helper; both class methods become 1-line delegates so a future bug fix cannot drift between copies. Item 3 — Empty company_name produces broken email text. Individual-type customers have a blank company_name, which previously rendered as "Account Created for " (subject, trailing space) and "invited to on PRAHO" (body, double space). Switch to customer.get_display_name(), which already returns company_name for company-type customers when set and falls back to .name otherwise. Item 6 — Hardcoded "2 hours" in templates. Compute expiry_hours from settings.PASSWORD_RESET_TIMEOUT // 3600 in the helper, pass as context, and render with {% blocktrans count counter=expiry_hours %} so the singular form ("1 hour") is used when the setting is exactly one hour. Test coverage (3 new integration tests; mutation-tested before commit): - test_individual_customer_renders_name_when_company_name_empty: reverting to customer.company_name produced exactly the #173 subject artifact "Account Created for " — test caught it. - test_expiry_hours_derives_from_password_reset_timeout: hardcoding expiry_hours = 999 produced "999 hours" in body — test caught it. - test_expiry_hours_singular_pluralization: verifies the {% plural %} branch when PASSWORD_RESET_TIMEOUT == 3600. - The pre-existing test_create_user_renders_welcome_email_with_valid_reset_token (commit 2cb5858) failed against a "return False" mutation of the delegate, so the refactor itself is regression-protected. All 59 tests in tests.customers.test_user_management_views and tests.api.test_customer_api still pass; ruff and mypy clean. Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27e7d2fb0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| messages.warning( | ||
| request, | ||
| format_html( | ||
| '{} <a href="{}" class="underline">{}</a>', |
There was a problem hiding this comment.
When invite delivery fails, this message renders a plain <a href> to customers:resend_invite, but that endpoint is @require_POST (customer_resend_invite), so clicking the link issues a GET and returns 405 instead of resending. This breaks the recovery flow exactly in the failure scenario it is meant to handle; use a POST form/button (with CSRF token) or make the endpoint accept GET safely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens the “create customer user + send welcome invite” flow across both the staff UI and the customer API to reduce enumeration risk, add throttling/rate limiting, and improve email rendering correctness and test coverage.
Changes:
- Adds API throttling, timing-safe email validation/normalization, and cross-tenant-safe error messaging to close enumeration paths.
- Consolidates duplicate welcome-email sending logic and introduces a public, rate-limited
send_welcome_invitewrapper. - Adds a staff “resend invite” recovery endpoint and expands behavioral + integration tests (including real template rendering via
mail.outbox).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/platform/apps/api/customers/views.py | Adds throttling, secure email validation, and cross-tenant-safe responses; uses rate-limited invite sender. |
| services/platform/apps/customers/user_management_views.py | Secures staff create-user flow, adds resend-invite endpoint, and uses public invite wrapper. |
| services/platform/apps/customers/urls.py | Registers the resend-invite staff route. |
| services/platform/apps/customers/views.py | Re-exports the new resend-invite view for backward-compatible imports. |
| services/platform/apps/users/services.py | Deduplicates email send helper, derives expiry hours dynamically, and adds per-user cache guard for invite sending. |
| services/platform/templates/customers/emails/welcome_email.html | Updates copy/i18n blocks and uses dynamic expiry + display-name fallback. |
| services/platform/templates/customers/emails/welcome_email.txt | Same as HTML: i18n hygiene, display-name fallback, and dynamic expiry pluralization. |
| services/platform/tests/api/test_customer_api.py | Adds/updates tests for enumeration behavior, invalid email rejection, rollback on failure, and request_ip propagation. |
| services/platform/tests/customers/test_user_management_views.py | Adds staff-view security tests, resend-invite tests, cache-guard tests, and template-rendering integration tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+147
to
+156
| resend_url = reverse("customers:resend_invite", kwargs={"customer_id": customer.id, "user_id": user.id}) | ||
| messages.warning( | ||
| request, | ||
| format_html( | ||
| '{} <a href="{}" class="underline">{}</a>', | ||
| _("User created but invite email could not be sent."), | ||
| resend_url, | ||
| _("Resend invite"), | ||
| ), | ||
| ) |
Comment on lines
+98
to
+101
| # Derive the human-readable expiry from the actual setting so the email | ||
| # body cannot drift from PASSWORD_RESET_TIMEOUT. | ||
| expiry_hours = max(1, settings.PASSWORD_RESET_TIMEOUT // SECONDS_PER_HOUR) | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Thanks for the catch — we chose floor deliberately. The asymmetry: floor under-promises ("1 hour" when 1.5h remain) and at worst makes the user act faster than they need to (no harm); ceil would over-promise ("2 hours" when 1.5h remain) and let users think they have time after the token has actually expired, hitting a confusing "link expired" error. Since PASSWORD_RESET_TIMEOUT = 7200 (exactly 2h) ships in config/settings/base.py:162, this is hypothetical anyway, but keeping floor for the safer-direction property. If a non-hour-multiple value is ever configured we can revisit — likely rendering "1h 30m" rather than rounding either direction. Closing as wontfix.
Comment on lines
+892
to
898
| guard_key = f"welcome_invite:{user.pk}" | ||
| invite_limit = SettingsService.get_integer_setting("security.welcome_invite_limit_per_user_per_hour", 3) | ||
| sent_count = cache.get(guard_key, 0) | ||
| if sent_count >= invite_limit: | ||
| logger.warning( | ||
| f"🚦 [Welcome Invite] Rate limit hit for user {user.pk} ({sent_count}/{invite_limit} per hour)" | ||
| ) |
Comment on lines
+892
to
+894
| guard_key = f"welcome_invite:{user.pk}" | ||
| invite_limit = SettingsService.get_integer_setting("security.welcome_invite_limit_per_user_per_hour", 3) | ||
| sent_count = cache.get(guard_key, 0) |
Contributor
Author
There was a problem hiding this comment.
This is intentional — the rate-limit's threat model is "don't email-bomb the target's inbox", not "limit per (inviter, customer) pair". From the target's perspective, getting 3 invitations per hour is enough regardless of which customer they came from; if we keyed by (user, customer), an attacker with credentials in N customers could email-bomb a single target with up to 3×N invites per hour, defeating the cap. Keying by user.pk (1:1 with email globally in PRAHO's user model) is the correct boundary for the inbox-bombing concern. Per-credential / per-(inviter, customer) throttling is handled at a different layer by @throttle_classes([BurstAPIThrottle]) on the API view (commit de56d83). Closing as wontfix.
…184) Codex caught that the "Resend invite" link rendered in the failure warning was a bare <a href>, but the resend endpoint is @require_POST (the action has side effects: it generates a fresh password-reset token that invalidates the prior one, so it must be CSRF-protected). Clicking the link issued a GET and 405'd — breaking the recovery flow exactly when staff need it. Fix: render the warning with an inline <form method="post"> + a CSRF token via django.middleware.csrf.get_token(request). The button is styled to look like a link. Test coverage (mutation-tested before commit): - test_failure_warning_renders_post_form_not_get_link asserts the warning HTML contains method="post" and csrfmiddlewaretoken, and does NOT contain a bare <a href ... resend-invite>. - test_clicking_resend_form_actually_resends extracts the form's action URL from the rendered warning, POSTs to it, and asserts the resend succeeds end-to-end (mock_send.call_count == 2 — once for the create, once for the resend). Reverted to the broken <a href> for verification: both new tests failed with the exact bug Codex described ("'method=\"post\"' not found in '... <a href=...resend-invite/...'"). Restored the fix; all 5 CustomerResendInviteTests pass; full module green at 61 tests. Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
on PR #184) Copilot caught that the per-user rate-limit guard in send_welcome_invite() was non-atomic: sent_count = cache.get(guard_key, 0) # T0 if sent_count >= invite_limit: return False # T1 sent = cls._send_welcome_email_secure(...) # T2 if sent: cache.set(guard_key, sent_count + 1) # T3 Two concurrent invite sends could both read sent_count=2 at T0, both pass the threshold check at T1, both increment to 3 at T3 — allowing 4 sends through a 3/hour cap. Fix: switch to the cache.add + cache.incr pattern, which is atomic on memcached/Redis backends. cache.add(guard_key, 0, timeout=3600) # idempotent seed sent_count = cache.incr(guard_key) # atomic if sent_count > invite_limit: cache.decr(guard_key) # release the claim return False sent = cls._send_welcome_email_secure(...) if not sent: cache.decr(guard_key) # transient failure: don't penalise Compatibility: cache.incr raises ValueError on backends that lack atomic increment (e.g. DummyCache used in some test settings). Catch and fall back to direct send — the view-layer @throttle_classes still bounds traffic, so this is defense-in-depth, not the only line. New test test_atomic_counter_uses_cache_incr_not_set asserts the cache key value matches the call count at each step, pinning the implementation to the atomic primitive (mutation-equivalent: any non-atomic implementation that "works on average" would still pass the existing count-based tests, but this new test fails if the counter diverges from the expected value at any point). All 62 tests pass; ruff and mypy clean. Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
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
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
Follow-up to PR #149 (invite-email on customer-user creation). Addresses every finding from the dual review (internal pr-reviewer + codex) and every item from tracking issue #173.
Changes (6 commits)
de56d835HIGH/MEDIUM hardening:@throttle_classes([BurstAPIThrottle]), switch toSecureInputValidator.validate_email_secure, publicsend_welcome_invitewrapper with per-user cache guard (3/hour), in-org (409) vs cross-tenant (400 generic) differentiation to close the enumeration oracle.1d2edc4eRecovery: new staffPOST /customers/<id>/user/<uid>/resend-invite/endpoint, surfaced inline in the failure warning message.dc25e007i18n hygiene:{% blocktrans trimmed %}everywhere, lead heading with company context.69c5d3838 new behavioral tests for the hardening above.2cb5858cIntegration test throughmail.outbox(closes chore: invite email hardening — throttle, dedup, validation gaps #173 item 5). Mutation-tested.27e7d2fbCloses chore: invite email hardening — throttle, dedup, validation gaps #173 items 2/3/6: consolidate duplicate helper, fix empty-company-name rendering, compute expiry fromPASSWORD_RESET_TIMEOUT. 3 more integration tests, mutation-tested.Closes
Closes #173
Test plan
Issue #173 closure map
Built with Claude Code (/systematic-debugging + /test-driven-development).