Repository navigation
Email notifications: account security (A4 + A5) - #127
Merged
Merged
Conversation
Hooks an after_action change onto the existing :update_role action so a role change kicks off an Oban-delivered Activity-category email to the affected user. Skips delivery when the role didn't actually change and when the actor is updating their own role (per docs/notifications.md recipient rule #1). The new sender mirrors the password-changed shape: HTML + plain-text bodies, escaped display name, settings link, and an unsubscribe footer keyed to :account_role_changed. Covers issue #118 (A5).
Adds an :update action to User that lets a signed-in user change their own email after re-authenticating with their current password. Exposed via the Accounts code interface and as a GraphQL mutation. Mirrors the shape of :change_password (same PasswordValidation, same self-only policy). Confirmed_at is left in place because the confirmation strategy is configured with confirm_on_update? false. Sets up the action that A4's email-changed security notice will hook into.
Hooks an after_action change onto :change_email that captures the old address via changeset context (since user.email is the new value post-update) and enqueues two Oban jobs: one notice to the old address with a password-reset link for recovery, and one confirmation to the new address showing the previous value for context. Both messages share the :email_changed transactional trigger; the sender branches on payload audience. Covers issue #118 (A4).
- AccountRoleChanged sender: pass a context map to the body helpers instead of five positional args; pattern-match on the previous_role to dedupe the "from X to Y" / "to Y" branching; add a `safe/1` helper to match the EmailChanged shape - Extract `find_user!/2` to Huddlz.Test.Helpers.FeatureUsers so the two new feature step files share one implementation instead of redefining it side-by-side - Trim the email-change step's case-on-update wrapper to a plain Ash.update call (the same-email failure is the test's expected outcome; no branching needed)
Reach for the public Ash API instead of `changeset.context[:private][:actor]`. The 3-arity `change after_action(fn cs, r, ctx -> ...)` form receives an `Ash.Resource.Change.Context` struct with `:actor` already extracted.
…email `changeset.data.email` already holds the pre-update value during after_action, so storing it on context first added indirection without value. Also remove the dead `previous_email != ""` guard — `changeset.data.email` is always present for an existing user.
…senders Three different inline forms of `Phoenix.HTML.html_escape |> safe_to_string` had grown across the senders. Lift it into one helper and converge.
The earlier comment claimed the same-email scenario failed at the unique identity; it doesn't. The after_action's previous-vs-new guard is what skips the notification.
Previously a missing or typo'd `audience` key produced a cryptic FunctionClauseError at delivery time. A catch-all clause now raises ArgumentError with the offending payload so the failure is debuggable without grovelling through Oban logs.
Match `%Ash.Error.Invalid{}` for the malformed-email and email-in-use
cases instead of a bare `{:error, _}`. Catches the regression where the
path flips to `Forbidden` (which would otherwise pass silently).
Modeled on notification_preferences_test.exs. Covers the success path (verifying the email is persisted and both old/new audience jobs are enqueued), wrong-password rejection, and unauthenticated rejection (no enqueue, no mutation).
The old-address audience used to point users at /reset if the change wasn't them. But the password-reset email goes to the *new* address, which a hijacker would control — so /reset is not actually a recovery path in this scenario. Lead with "contact support" and explicitly note that /reset won't help. The /reset link is gone from this body. Codifies the rule that recovery links only belong in security notices when the recovery channel itself hasn't moved.
Adds a "Sender conventions" section to docs/notifications.md covering: - HTML escape: every user-controlled string in html_body MUST go through Senders.HtmlEscape.escape/1 - Footer: transactional none, activity unsubscribe-token footer - Recovery advice: /reset link only when the recipient's address is the current account email — table makes the rule explicit - Test floor: minimum assertions every sender test should make Also points the Sender behaviour moduledoc at the new section so the file you're editing tells you where the rules live.
- assert errors == [] in the change_email GraphQL success path; the prior `errors in [nil, []]` would mask a regression where the response shape flipped between [] and nil - drop "action is exposed in the resource" sanity test (and the unused aliases) — Ash validates action presence at compile time and the rest of the file already exercises the code interface, so a missing or renamed action could not survive
- update_role after_action: explain why a nil actor (system call with
authorize?: false) intentionally falls through to notify, rather
than being silently suppressed by the short-circuit equality
- email_changed sender: flag that recipient header-injection safety
depends on the User.email regex constraint rejecting CR/LF, since
Swoosh does not sanitize recipient strings
- account_role_changed_notification_steps: justify the 50ms timeout
in the refute helper — the queue is already drained, so the wait
only flushes in-flight {:email, _} mailbox messages
The setup used the default group generator, which seeds `description` with `Faker.Lorem.paragraph(2..3)` — variable-length text that occasionally exceeds MetaHelpers' 200-char truncation. When that happens, the rendered <meta name="description"> picks up an ellipsis and the equality assertion against the un-truncated `group.description` fails (e.g. CI seed produced a 213-char paragraph). Set an explicit short description in setup so the meta assertions are deterministic regardless of Faker output.
Faker.Company.name() can return non-ASCII (e.g. CJK) names that slugify into single-codepoint slugs. Phoenix's verified routes URL-encode those in the rendered og:url, but the assertion compared against the raw `group.slug`, so `=~` failed (e.g. "http://localhost:4002/groups/%E4%85%A7" =~ "/groups/䅧"). Pin `name: "Test Group"` in setup alongside the previously pinned description so every meta-tag assertion is deterministic regardless of Faker locale roulette.
The capacity-validation setup didn't pin `max_attendees`. The huddl generator's changeset_generator auto-fills any accepted attribute it isn't given, and for an `allow_nil? true` integer it picks `nil` or a random integer roughly half the time each. When it picked an integer (e.g. 1_105_234_120 in CI), the test's post-validation assert `reloaded.max_attendees == nil` failed even though the validation itself worked correctly. Pin `max_attendees: nil` so the "start unlimited, then try to reduce below the current RSVP count" scenario is the one we're actually testing every run.
The previous fix (be5d6f6) pinned `name` to make the slug deterministic, on the theory that slug is derived from name. That was wrong: the :create_group action exposes a public `:slug` argument with allow_nil?: true, and Ash.Generator.action_input/3 includes public arguments in its optional-input bucket. StreamData.one_of randomly picks the :utf8 string generator a third of the time and emits CJK or private-use codepoints (e.g. U+F3134 → "%F3%8D%84%94" in og:url). That random slug overrides the name-derived one in GenerateSlug.resolve_slug/1 because the first non-blank-binary clause matches before the name fallback. Pinning slug: nil in the generator defaults means every test going through generate(group(...)) gets a name-derived slug. Tests that need a specific slug (group_test, group_unicode_test, edit_test, group_edit_link_test, group_live_image_test) already pin it explicitly via overrides and are unaffected.
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.
Closes #118.
Hooks the two remaining account-security email triggers from
docs/notifications.md. A3 (password changed) was already shipped on main; this branch adds A4 and A5.Summary
User.update_role. Activity-category email with an unsubscribe footer. Skips delivery when the role didn't actually change and when an admin updates their own role (per docs rule Component standardization (cards, buttons, forms) #1).:change_emailAsh action gated by current password (exposed via the Accounts code interface and as a GraphQL mutation), then sends two tailored transactional emails: a security notice + reset link to the old address, and a confirmation to the new one.defp safe/1, dedupes the role-change phrase via pattern matching, and extracts a sharedfind_user!/2for the new feature step files.Test plan
mix test test/huddlz/notifications/senders/account_role_changed_test.exsmix test test/huddlz/notifications/senders/email_changed_test.exsmix test test/huddlz/accounts/user/change_email_test.exsaccount_role_changed_notification.feature(3 scenarios) andemail_change_notification.feature(2 scenarios)mix precommit— 860 tests, 0 failures, credo cleanmix phx.server+/dev/mailbox: admin promotes a user (verify role-change email + working unsubscribe link), admin promotes themselves (verify no email), user changes their own email (verify two distinct messages),/profile/notificationsshows A3/A4 disabled-but-on under Security and A5 toggle under Activity