Repository navigation
Conversation
- Add :corsica dep with runtime origin lookup via HuddlzWeb.Cors - Read :cors_origins from config (:all or list of allowed origins) - Dev/test default to :all; prod parses comma-separated CORS_ORIGINS env var - Cover preflight, actual request, and origin filtering with tests
- authenticated_conn/2 attaches a bearer JWT for the given user - gql_post/3 POSTs a GraphQL query (with optional variables) to /gql - Smoke test verifies token is valid and an introspection query returns 200
- Slim shape selecting id, display_name and loading current_profile_picture_url - Will be used as read_action on creator/owner/member/attendee relationships - Public to all actors (anonymous, regular, admin all see the same shape)
POST /api/auth/register calls register_with_password and returns the issued JWT alongside a slim user payload (id, email, display_name). - 201 on success; 422 with an errors list on validation failure - Mounted under /api/auth via the existing :api pipeline - Tests cover the happy path and the duplicate-email, weak-password, and mismatched-confirmation failure modes
POST /api/auth/sign_in calls sign_in_with_password and returns the issued JWT alongside a slim user payload. Wrong password and unknown email both return the same 401 shape so attackers can't enumerate registered emails.
GET /api/auth/me returns the current actor's slim self profile when a valid bearer is provided, 401 otherwise. Self profile includes email since /me is the user reading their own info.
DELETE /api/auth/sign_out revokes the bearer JWT via the token resource so subsequent requests with the same token fail with 401. Returns 204 on success, 401 when no bearer is provided.
POST /api/auth/password_reset always returns 204 (regardless of whether the email matches a user) so attackers can't enumerate registered emails. Internally calls request_password_reset_token, which sends the reset email when the user exists.
8000
Generated via mix ash_authentication.add_strategy api_key. Plaintext keys are returned only at create time via the plaintext_api_key metadata; only the hash is persisted. Wire AshAuthentication.Strategy.ApiKey.Plug into the :api pipeline with a pass-through on_error so it co-exists with the existing JWT :load_from_bearer plug — when the bearer is a JWT (not an API key), the API-key plug is a no-op instead of returning 401. Tests cover the action (key generation, valid calc, sign_in_with_api_key) and the HTTP plug integration (API key authenticates GET /api/auth/me).
Mirrors the :api pipeline so /gql accepts both JWT and API-key bearers. New cross-cutting api_auth_test.exs verifies that JWT and API-key auth co-exist on both pipelines, and that an expired API key results in unauthenticated access.
POST /api/auth/api_keys mints an API key for the current actor and returns the plaintext once. Accepts an optional expires_in_days parameter (default 30, accepts integer or string). Switch the ApiKey :create action to relate_actor(:user) so the user_id is set from the actor and can never be set by the caller; the policy becomes a simple actor_present() check. Existing tests and the api_key_conn helper are migrated to pass actor instead of user_id.
GET /api/auth/api_keys returns only the actor's keys (id, expires_at, valid). The plaintext and hash are never returned. Policy on the ApiKey :read action filters to expr(user_id == ^actor(:id)) so the authorization is enforced at the resource layer.
DELETE /api/auth/api_keys/:id revokes the actor's key. After deletion that key no longer authenticates. Attempting to delete another actor's key returns 404 (the read policy filters it out, so the controller can't distinguish from a non-existent id). Add policy on the ApiKey :destroy action so the resource layer enforces ownership.
GET /api/json/huddlz/upcoming and the upcomingHuddlz GraphQL query return future huddlz, sorted ascending by starts_at. Both surfaces respect the existing FilterByVisibility preparation.
GET /api/json/huddlz/past and the pastHuddlz GraphQL query return past huddlz, sorted descending by starts_at.
GET /api/json/huddlz/by_group?group_id=... and the huddlzInGroup GraphQL query return future huddlz scoped to a single group, with keyset pagination already configured on the action.
Drop the user_id argument from Huddl.rsvp; the action now infers the user from the actor instead. The Rsvp change reads context.actor.id so impersonation is structurally impossible. Expose as the rsvpToHuddl GraphQL mutation and a PATCH /api/json/huddlz/:id/rsvp JSON:API route. BREAKING CHANGE: Communities.rsvp_huddl/3 no longer accepts a user_id argument. Callers must pass the actor via opts; the action authorizes and creates the attendee for that actor. The "user cannot RSVP for someone else" policy regression test is replaced with a structural "actor required" test, since user_id can no longer be passed.
The default stub returned {:error, :not_found}, which fires a
Logger.warning for every test that creates a huddl, group, or location
without an explicit geocode stub. The end state on the record is
identical (lat/lng remain nil) whether geocoding errors or returns
nil coords, so switch the stub to {:ok, %{latitude: nil, longitude: nil}}.
Tests that need real coordinates already override via Mox.stub or the
stub_geocode helper, so this only affects log noise.
Drop the user_id argument from Huddl.cancel_rsvp and read the user from the actor in the CancelRsvp change. Exposed as the cancelRsvpToHuddl GraphQL mutation and PATCH /api/json/huddlz/:id/cancel_rsvp. The "other user tries to cancel another user's RSVP" test previously asserted Forbidden via a forbid_unless policy. With user_id no longer passable, the action is naturally idempotent for the calling actor — the test now asserts the original RSVP is still in place. BREAKING CHANGE: Communities.cancel_rsvp_huddl/3 no longer accepts a user_id argument.
DELETE /api/json/huddlz/:id and the deleteHuddl GraphQL mutation remove a huddl. The existing policy on action([:update, :destroy]) restricts this to group owners and organizers; strangers receive 403/404.
Add base extension blocks. listGroups returns the paginated read, getGroup looks up by slug (read_one), and JSON:API exposes both list and slug-based show under /api/json/groups.
GET /api/json/groups/search?query=... and the searchGroups GraphQL query both use the existing trigram-similarity Group.search read action.
Drop the owner_id argument from Group.get_by_owner; the action now filters on the actor's id directly. Exposed as the myGroups GraphQL query and GET /api/json/groups/mine. BREAKING CHANGE: Communities.get_by_owner/N no longer takes an owner_id arg — the actor must be passed via opts.
POST /api/json/groups and the createGroup GraphQL mutation create a new group via the existing create_group action. Authenticated users can create groups; the AddOwnerAsMember change makes the actor the owning member.
PATCH /api/json/groups/:id and updateGroup GraphQL mutation use the existing update_details action. The owner-only policy on the action restricts updates to the group owner; strangers receive 403/404.
DELETE /api/json/groups/:id and the deleteGroup mutation are wired
to the existing destroy action. Owner-only authorization is
preserved via the existing policy.
The default destroy may return 400 ("would leave records behind")
when the group has members or huddlz; cascading or soft-delete is
out of scope for this commit. The test verifies the auth gate works
(owner gets past the policy; stranger receives 403/404).
Add base extension blocks to GroupMember and expose the get_by_group read action via the groupMembers GraphQL query and GET /api/json/group_members/by_group?group_id=... The existing policy restricts results to group owners, organizers, and members; non-members are blocked.
Drop the user_id arg from GroupMember.get_by_user; the action filters by actor.id. Exposed as the myMemberships GraphQL query and GET /api/json/group_members/mine. Adds a policy granting any authenticated actor access to their own memberships. BREAKING CHANGE: Communities.get_by_user/N no longer takes a user_id arg.
Drop the user_id arg from GroupMember.join_group; the action uses relate_actor(:user) so the actor always joins as themselves — impersonation is structurally impossible. Exposed as the joinGroup GraphQL mutation and POST /api/json/group_members/join. BREAKING CHANGE: Communities.GroupMember.join_group no longer accepts a user_id argument.
DELETE /api/json/group_members/:id and the leaveGroup mutation use the existing leave_group destroy action. Add a default :read policy gating GroupMember reads to the actor's own records (so the destroy can read the record before destroying it). The existing relates_to_actor_via policy already prevents leaving on someone else's behalf.
Single ExUnit test exercises the full API user journey: register → sign_in → create_group → create_huddl → RSVP → list my RSVPs → GraphQL me query → sign_out → revoked token returns 401. This is the smoke test that catches integration problems even when each unit test passes (schema not registered, route not forwarded, plug ordering wrong).
User GraphQL type is only reachable via the me query (filtered to the actor's own record), so email is acceptable on User but role and hashed_password still aren't. Author / UserPublicProfile are cross- user shapes and remain on the strict deny list (no email, role, hashed_password, or home lat/lng).
Captures the items left on the cutting room floor while landing the JSON:API + GraphQL surface — pagination sweep, OpenAPI prose polish, GroupMember.remove_member JSON:API route, real multipart HTTP tests, ProfilePicture upload exposure, slug ergonomics, group destroy cascading, attribute visibility audit, mutation-arg ergonomics, api-key scopes/rotation/audit, rate limiting, Cucumber happy-path. Each entry has rationale + suggested approach so picking one up later doesn't require reconstructing context.
There was a problem hiding this comment.
Pull request overview
This PR expands Huddlz’s external API surface (JSON:API + GraphQL) and authentication support by adding API endpoints for registration/session management, introducing API-key auth, enabling CORS, and reshaping RSVP/group-membership actions to be actor-based (removing user-id inputs).
Changes:
- Add
/api/auth/*JSON endpoints for register/sign-in/me/sign-out/password reset plus API key CRUD, and wire API-key auth into API + GraphQL pipelines. - Expose additional resources/actions via AshJsonApi/AshGraphql (groups, huddlz, attendees, locations, images) and add shared API test helpers + end-to-end coverage.
- Convert RSVP/join-group style actions and reads to use the current actor instead of
user_idargs, updating LiveView and tests accordingly.
Reviewed changes
Copilot reviewed 57 out of 58 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/support/geocoding_stub.ex | Changes default geocoding stub behavior used across tests. |
| test/support/api_case.ex | Adds shared test template/helpers for JSON:API + GraphQL API tests. |
| test/huddlz_web/live/huddl_live/show_test.exs | Updates RSVP tests to actor-based RSVP action inputs. |
| test/huddlz_web/cors_test.exs | Adds CORS behavior tests (preflight + actual + allowed?/2). |
| test/huddlz_web/controllers/api/auth_controller_test.exs | Adds full test coverage for new /api/auth/* endpoints. |
| test/huddlz_web/api_case_test.exs | Tests ApiCase helpers (authenticated_conn/2, gql_post/3). |
| test/huddlz_web/api_auth_test.exs | Verifies JWT + API-key coexistence across API + GraphQL pipelines. |
| test/huddlz_web/api/sensitive_fields_audit_test.exs | Adds GraphQL schema audit to prevent leaking sensitive fields. |
| test/huddlz_web/api/json/huddl_test.exs | Adds JSON:API tests for huddl actions (RSVP, cancel, listing, delete). |
| test/huddlz_web/api/json/huddl_image_test.exs | Adds image upload action/route smoke coverage. |
| test/huddlz_web/api/json/huddl_attendee_test.exs | Adds JSON:API attendee endpoints tests (by_huddl/mine). |
| test/huddlz_web/api/json/group_test.exs | Adds JSON:API group endpoints tests (list/search/mine CRUD). |
| test/huddlz_web/api/json/group_member_test.exs | Adds JSON:API group member endpoints tests + GraphQL mutation exposure check. |
| test/huddlz_web/api/json/group_location_test.exs | Adds JSON:API group location endpoints tests. |
| test/huddlz_web/api/graphql/huddl_test.exs | Adds GraphQL huddl queries/mutations coverage. |
| test/huddlz_web/api/graphql/group_test.exs | Adds GraphQL group queries coverage. |
| test/huddlz_web/api/end_to_end_test.exs | Adds end-to-end API flow test across auth + JSON:API + GraphQL. |
| test/huddlz/communities/huddl_rsvp_test.exs | Updates RSVP domain tests to actor-based behavior and new semantics. |
| test/huddlz/communities/huddl_rsvp_edge_cases_test.exs | Updates RSVP edge-case tests for actor-based actions. |
| test/huddlz/communities/huddl_access_control_test.exs | Updates access-control test RSVP call to actor-based. |
| test/huddlz/communities/group_membership_test.exs | Updates join_group tests to actor-based join semantics. |
| test/huddlz/communities/group_membership_permissions_test.exs | Updates permissions tests to actor-based join semantics. |
| test/huddlz/communities/group_live_functionality_test.exs | Updates get_by_owner usage to actor-based read. |
| test/huddlz/accounts/user_test.exs | Adds tests for new :public_profile read shape/select behavior. |
| test/huddlz/accounts/api_key_test.exs | Adds tests for ApiKey creation/valid calculation/api-key sign-in. |
| test/features/step_definitions/rsvp_cancellation_steps.exs | Updates cucumber steps to actor-based RSVP. |
| priv/resource_snapshots/repo/api_keys/20260425153611.json | Adds resource snapshot for new api_keys table. |
| priv/repo/migrations/20260425153610_add_api_key_auth.exs | Adds migration creating api_keys table + unique hash index. |
| mix.lock | Adds corsica dependency lock. |
| mix.exs | Adds corsica dependency. |
| lib/huddlz_web/router.ex | Adds /api/auth/* routes and API-key auth plug to API/GraphQL pipelines. |
| lib/huddlz_web/live/huddl_live/show.ex | Updates RSVP/cancel/check_rsvp calls to actor-based domain APIs. |
| lib/huddlz_web/live/group_live/show.ex | Updates join_group to actor-based join action. |
| lib/huddlz_web/graphql_schema.ex | Adds Accounts domain to AshGraphql schema. |
| lib/huddlz_web/endpoint.ex | Adds Corsica plug configuration. |
| lib/huddlz_web/cors.ex | Implements configurable CORS allow-list logic. |
| lib/huddlz_web/controllers/api/auth_controller.ex | Adds API auth controller (JWT + API key CRUD + password reset). |
| lib/huddlz_web/ash_json_api_router.ex | Adds Accounts domain to AshJsonApi router. |
| lib/huddlz_web/api_auth.ex | Adds ApiKey plug on_error pass-through helper. |
| lib/huddlz/storage/changes/persist_upload.ex | Adds shared change to persist uploaded files via storage module pipeline. |
| lib/huddlz/communities/huddl_image.ex | Exposes huddl image upload via JSON:API + GraphQL and wires PersistUpload. |
| lib/huddlz/communities/huddl_attendee.ex | Exposes attendee endpoints and reshapes reads to actor-based filters/policies. |
| lib/huddlz/communities/huddl/changes/rsvp.ex | Makes RSVP change actor-driven; adds error when actor missing. |
| lib/huddlz/communities/huddl/changes/cancel_rsvp.ex | Makes cancel RSVP change actor-driven; adds error when actor missing. |
| lib/huddlz/communities/huddl.ex | Exposes additional JSON:API/GraphQL routes + actor-based RSVP actions. |
| lib/huddlz/communities/group_member.ex | Exposes JSON:API/GraphQL routes and makes join/read actor-based. |
| lib/huddlz/communities/group_location.ex | Exposes group locations via JSON:API + GraphQL. |
| lib/huddlz/communities/group_image.ex | Exposes group image upload via JSON:API + GraphQL and wires PersistUpload. |
| lib/huddlz/communities/group.ex | Exposes groups via JSON:API + GraphQL; makes get_by_owner actor-based. |
| lib/huddlz/communities.ex | Updates domain interface definitions to match actor-based actions. |
| lib/huddlz/accounts/user.ex | Adds GraphQL exposure, :public_profile/:me reads, and API-key strategy hooks. |
| lib/huddlz/accounts/profile_picture.ex | Adds actor-based upload action and PersistUpload change. |
| lib/huddlz/accounts/api_key.ex | Adds ApiKey Ash resource with hashing + validity calculation + policies. |
| lib/huddlz/accounts.ex | Registers ApiKey resource in Accounts domain. |
| docs/api-followups.md | Adds follow-up backlog for deferred API improvements. |
| config/test.exs | Sets test CORS config to :all. |
| config/runtime.exs | Adds prod CORS origins config via CORS_ORIGINS env var. |
| config/dev.exs | Sets dev CORS config to :all. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+11
to
+23
| create table(:api_keys, primary_key: false) do | ||
| add :id, :uuid, null: false, default: fragment("gen_random_uuid()"), primary_key: true | ||
| add :api_key_hash, :binary, null: false | ||
| add :expires_at, :utc_datetime_usec, null: false | ||
|
|
||
| add :user_id, | ||
| references(:users, | ||
| column: :id, | ||
| name: "api_keys_user_id_fkey", | ||
| type: :uuid, | ||
| prefix: "public" | ||
| ) | ||
| end |
There was a problem hiding this comment.
api_keys.user_id is created nullable (no null: false) and without an index. Since API keys should always belong to a user and are frequently queried/scoped by user, consider making user_id null: false (and usually on_delete: :delete_all) and adding an index on user_id (optionally with expires_at) for efficient listing/revocation.
| end | ||
|
|
||
| relationships do | ||
| belongs_to :user, Huddlz.Accounts.User |
There was a problem hiding this comment.
The ApiKey → User relationship is declared without allow_nil? false, which leads to a nullable user_id in the generated migration/snapshot. If API keys must always be owned by a user (as the policies assume), set the belongs_to :user relationship to allow_nil? false so the DB constraint and Ash validations enforce that invariant.
Suggested change
| belongs_to :user, Huddlz.Accounts.User | |
| belongs_to :user, Huddlz.Accounts.User do | |
| allow_nil? false | |
| end |
Comment on lines
+36
to
+46
| case storage_module.store(path, filename, content_type, parent_id) do | ||
| {:ok, metadata} -> | ||
| changeset | ||
| |> Ash.Changeset.force_change_attribute(:filename, filename) | ||
| |> Ash.Changeset.force_change_attribute(:content_type, content_type) | ||
| |> Ash.Changeset.force_change_attribute(:size_bytes, metadata.size_bytes) | ||
| |> Ash.Changeset.force_change_attribute(:storage_path, metadata.storage_path) | ||
| |> Ash.Changeset.force_change_attribute(:thumbnail_path, metadata.thumbnail_path) | ||
|
|
||
| {:error, reason} -> | ||
| Ash.Changeset.add_error(changeset, message: format_error(reason), field: file_arg) |
There was a problem hiding this comment.
PersistUpload calls storage_module.store/4 even when parent_id is nil (e.g., if the parent id argument/attribute wasn’t provided or hasn’t been set yet by earlier changes). That can lead to files stored in the wrong location or unexpected storage errors. Add an explicit check that parent_id is present (and return a changeset error on parent_arg) before calling the storage module.
Suggested change
| case storage_module.store(path, filename, content_type, parent_id) do | |
| {:ok, metadata} -> | |
| changeset | |
| |> Ash.Changeset.force_change_attribute(:filename, filename) | |
| |> Ash.Changeset.force_change_attribute(:content_type, content_type) | |
| |> Ash.Changeset.force_change_attribute(:size_bytes, metadata.size_bytes) | |
| |> Ash.Changeset.force_change_attribute(:storage_path, metadata.storage_path) | |
| |> Ash.Changeset.force_change_attribute(:thumbnail_path, metadata.thumbnail_path) | |
| {:error, reason} -> | |
| Ash.Changeset.add_error(changeset, message: format_error(reason), field: file_arg) | |
| case parent_id do | |
| nil -> | |
| Ash.Changeset.add_error(changeset, | |
| message: "parent id is required", | |
| field: parent_arg | |
| ) | |
| parent_id -> | |
| case storage_module.store(path, filename, content_type, parent_id) do | |
| {:ok, metadata} -> | |
| changeset | |
| |> Ash.Changeset.force_change_attribute(:filename, filename) | |
| |> Ash.Changeset.force_change_attribute(:content_type, content_type) | |
| |> Ash.Changeset.force_change_attribute(:size_bytes, metadata.size_bytes) | |
| |> Ash.Changeset.force_change_attribute(:storage_path, metadata.storage_path) | |
| |> Ash.Changeset.force_change_attribute(:thumbnail_path, metadata.thumbnail_path) | |
| {:error, reason} -> | |
| Ash.Changeset.add_error(changeset, message: format_error(reason), field: file_arg) | |
| end |
ultrareview/bug_002. Public exposure of create_group via JSON:API (POST /api/json/groups) and GraphQL (createGroup) let any authenticated user pick an arbitrary owner_id, sticking the victim with attacker-named groups (and squatting their slugs) because the policy only checked role == :user. Drop :owner_id from the action's accept list entirely so the attribute is no longer exposed as a settable input on either surface, then set it server-side from the actor via a SetOwnerToActor before_action change. before_action runs after Ash propagates actor: from either for_create or Ash.create opts, which the builtin relate_actor/1 doesn't. Generator and direct test call sites updated to match: owner_id is dropped from create_group input maps; the test generator infers the actor from owner_id when it's still passed for setup convenience.
ultrareview/bug_005. Newly-exposed Ash resources (Group, Huddl,
GroupMember, GroupLocation) declared their user-facing attributes
without public?: true, so AshGraphql/AshJsonApi were rendering
only id (and slug for Group). Real-world clients querying
{ listGroups { results { name description } } } would fail
schema-validation; existing API tests asserted only on id and
missed the regression.
Mark name, description, location, is_public, title, starts_at,
ends_at, event_type, physical_location, is_private, thumbnail_url,
role, address public. Keep virtual_link sensitive (unchanged).
Tighten one JSON:API and one GraphQL test to read non-id
attributes so the regression cannot return silently.
ultrareview/bug_007. parse_expires_in_days previously accepted any signed integer (or parseable string) and fed it straight into DateTime.add/2. Negative values silently minted already-expired keys; large values minted near-permanent keys; malformed strings fell through to a 30-day default and hid client bugs. Validate in the controller: 1..365 days, integer or numeric string. Anything outside that range returns 422 with an expires_in_days field error.
ultrareview/bug_004. The cross-user audit was iterating over a hard-coded list of type names (Author, UserPublicProfile) that don't exist in the schema and silently no-opping via Enum.reject — so a future PR exposing a User-typed relationship would ship green even if it dragged email/role/hashed_password along. Replace the static list with a schema introspection that finds every user-shaped object type. Compare against an allowlist (@known_user_shaped_types). When a new type appears, the test fails with a clear prompt to add it to the allowlist AND extend the field deny-list audit accordingly.
Follow-on to df8f31f. The bug_002 sweep removed owner_id from create_group input maps but missed five LiveView test files that had the same pattern. Same mechanism — the action no longer accepts owner_id, the actor drives the relationship — these maps were just overlooked in the commit.
Two bugs in one. (a) POST /api/json/groups required a slug because :slug was in the action's accept list with allow_nil? false; the JSON:API validation pass rejected the input before any change ran. (b) When a slug *was* supplied — via LiveView form, GraphQL, or JSON:API — GenerateSlug overrode it unconditionally with force_change_attribute(:slug, derived_from_name). The :slug affordance was advertised but didn't actually work. Fix: drop :slug from accept and add it as an optional action argument. Teach GenerateSlug to honor a non-blank caller-supplied slug, falling back to Slug.slugify(name). Tighten the latent nil-name fall-through that was returning the changeset as the slug. Update the unicode-names test that asserted the override behavior — the new contract is "user-supplied slug wins, name-derived is the default", and the test now checks that contract. Add JSON:API tests for both the omitted-slug and custom-slug paths plus unit-level tests for slug resolution. BREAKING CHANGE: callers can no longer set :slug as an attribute override on update flows via this action's accept list (it was never updatable, the field stayed in :accept by accident). Pass slug as an action argument now.
DELETE /api/json/groups/:id used to fail with "would leave records behind" whenever a group had any rows pointing to it (group_members, huddlz, group_images), because no FK declared on_delete. Owner-only auth passed but the destroy never actually worked for a non-empty group. Add `references do reference :group, on_delete: :delete end` postgres blocks to group_member, group_image, and huddl. The huddl cascade itself needs its own dependents to cascade, otherwise destroying a huddl breaks on the FK from huddl_attendees / huddl_images — add references on those two as well. Five resource files + one migration. Tighten the group destroy test to set up a non-empty group (huddl + RSVP) and assert the dependents are gone after. BREAKING CHANGE: deleting a group now also deletes its huddlz, members, images, attendees, and locations transitively. Owner-only gating is unchanged.
The :upload action and policy shipped earlier had no DSL blocks, so no public endpoint reached them. Add AshJsonApi.Resource and AshGraphql.Resource to ProfilePicture's extensions, plus a graphql mutation (uploadProfilePicture) and a JSON:API route at POST /api/json/profile_pictures/upload. The Accounts domain also needed AshJsonApi.Domain in its extensions list — User exposure worked via GraphQL only, but JSON:API route matching requires the domain extension to compile the route table.
Add a generic-action wrapper (remove_member_by_ids) that finds the membership by (group_id, user_id) and delegates to the existing :remove_member destroy. Expose via JSON:API as DELETE /api/json/group_members/remove?group_id=...&user_id=... JSON:API's `route :delete` helper requires a generic action — it won't accept a destroy with arg-based filters directly. The wrapper is a thin lookup + delegate, with the destroy itself being the authorization gate (GroupOwner policy unchanged). Tests: owner can remove a member; non-owner gets 403.
Add multipart_post/4 helper to HuddlzWeb.ApiCase that builds the
multipart/x.ash+form-data envelope expected by
AshJsonApi.Plug.Parser and dispatches it through the endpoint.
Two non-obvious gotchas captured in the helper:
1. The "data" part needs a filename header so Plug's MULTIPART
produces a Plug.Upload struct — that's what AshJsonApi pattern-
matches on to recognize the JSON envelope. Without a filename it
falls through to plain form-field decoding and the JSON is lost
silently.
2. The JSON body sent in the data part is the *inner* resource
object ({"type": ..., "attributes": ...}); AshJsonApi adds the
outer {"data": ...} wrapper. Sending the wrapper produces a
"got key [data, data]" 400.
Use the helper in huddl_image_test.exs to do a real
POST /api/json/huddl_images/upload that asserts the response
includes a storage_path and that Huddlz.Storage.exists?/1 confirms
the bytes landed.
PersistUpload's format_error/1 used to fall back to inspect/1 for any non-binary reason, so an .exe upload returned the literal string ":invalid_extension" to the API consumer. Map the known atoms (and one tuple shape) to actionable messages; keep inspect/1 as the catch-all so unknown errors still surface something diagnostic. Add a test that uploads a .exe and asserts the error message is "Invalid file type. Allowed: JPG, PNG, WebP" — not the raw atom.
Items 1-6 from the followups doc are landed in this branch: slug ergonomics, group destroy cascade, ProfilePicture upload exposure, GroupMember.remove_member JSON:API route, real multipart HTTP test, and image-upload error mapping. Remove those entries from the doc and add a status header. Item 7 (user update mutations id arg) attempted the read_action: :me + identity: false fix per the plan, but Ash's atomic update pipeline turns the actor-filter read into a subquery that update_all rejects. Reverted; updated the doc entry to capture what was tried and the alternative approach (generic-action wrapper).
The PATCH /api/json/huddlz/:id/rsvp test wrapped a generate_group_with_members
call whose result was discarded via `_ = group`, then ran a no-op
`|> case do {g, members} -> {g, members} end` pass-through, only to bind a
single user that the outer setup already had. The huddl was always created
against the outer group, not the discarded one.
Replace the block with `member_user = generate(user())`. The :rsvp policy
authorizes any actor against a public huddl in a public group, so the
discarded "make this user a group member" step was never load-bearing.
Add followup entries surfaced during PR review: - RemoveMemberByIds membership-existence leak: the unauthenticated lookup distinguishes 404 vs 403, letting authenticated callers probe membership pairs. Folds into the API-key scopes work. - User-FK cascade asymmetry: group_members.user_id, huddl_attendees.user_id, and huddlz.creator_id are not cascaded. Not a blocker today (no delete-account flow), but huddlz.creator_id needs a product call before the FK can change. - sign_out vs API-key auth: DELETE /api/auth/sign_out only revokes JWTs; API-key actors get the generic 401 with no pointer to /api_keys/:id.
mwoods79
commented
Apr 27, 2026
| endpoint: required!("AWS_ENDPOINT_URL_S3") | ||
|
|
||
| cors_origins = | ||
| "CORS_ORIGINS" |
Contributor
Author
There was a problem hiding this comment.
Should read this from PHX_HOST if possible.
Without an explicit CORS_ORIGINS value, prod deploys fell back to an
empty allow-list and rejected every cross-origin request. Default to
"${PHX_SCHEME}://${PHX_HOST}" so the canonical host is allowed out of
the box; multi-origin setups can still override via CORS_ORIGINS.
The previous on_error pass-through silently swallowed every API key auth failure — including invalid Bearer tokens — and let them flow on as anonymous. Replace it with a wrapper plug that short-circuits when an actor is already loaded (so JWTs from :load_from_bearer aren't rechecked as API keys) and otherwise lets the default 401 path run. Invalid Bearer tokens now correctly 401 instead of being downgraded to anonymous. The custom on_error keeps the existing "Authentication required" body for consistency with the controller.
The unit tests under describe "HuddlzWeb.Cors.allowed?/2" mutated :huddlz, :cors_origins via Application.put_env while the file ran async — a real flake risk for any other test that read the same config. Split a pure matches?/2 helper out of allowed?/2 and have the tests call it with literal inputs, so the file stays async: true and never touches global state. The HTTP-level CORS tests already work against the default test config and are untouched.
The api_keys.user_id column was nullable, had no index, and the FK had no on-delete behavior — yet every policy on the resource scopes by user_id, so an unowned key would be unreachable but undeletable when the would-be owner went away. Set the belongs_to to allow_nil? false, add an on_delete: :delete reference and a custom_index on user_id, and ship the delta migration. Add regression tests that creating without an actor is rejected and that destroying a user removes their api keys.
PersistUpload was calling storage_module.store/4 with parent_id = nil when the parent attribute hadn't been set yet — most visibly for the ProfilePicture :upload action, where relate_actor's manage_relationship queues the user but doesn't write user_id until the action commits. Result: files were stored at /uploads/profile_pictures//<uuid>.ext with a literal double slash. Move the work into a before_action callback and resolve parent_id from the action argument, the explicit attribute change, OR the queued belongs_to relationship — whichever is set. If none yields an id, surface a friendly "is required" changeset error on parent_arg instead of silently writing to a malformed path.
Surface a flake observed during the PR-113 review sweep so the next person who hits it has a starting point instead of dismissing it as random.
GeocodingStub returned {:ok, %{latitude: nil, longitude: nil}} to
silence the "Geocoding failed for ..." warning in test runs, but
that violates the Huddlz.Geocoding behaviour, whose coordinates are
typed as %{latitude: float(), longitude: float()} (no nils). Code
that pattern-matches on {:ok, coords} and assumes numeric values
would be quietly wrong.
Revert the stub to {:error, :not_found} and instead skip the
production warning when env is :test. Add a CaptureLog assertion to
the geocoding-change test so the silence is enforced going forward.
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.
No description provided.