feat: add invoice access control for reviewers#100
Merged
Conversation
Allow admins/owners to restrict invoice visibility so only explicitly granted reviewers can see them. Adds access_restricted boolean flag on invoices and invoice_access_grants join table. Refactors role checks to use Authorization.can? instead of hardcoded pattern matching. - Migration: access_restricted column + invoice_access_grants table - Schema: InvoiceAccessGrant with belongs_to invoice/user/granted_by - Context: grant_access, revoke_access, set_access_restricted, list_access_grants - Query filtering: maybe_filter_by_access applied to all invoice queries - LiveView: Notion-style access control card on invoice show page - Threads user_id through all invoice query callers (LiveView + API) - 27 new tests (context CRUD, filtering by role, LiveView card) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds invoice access control: new InvoiceAccessGrant schema and migrations, invoice Changes
Sequence DiagramsequenceDiagram
actor User as User/API
participant Ctrl as Controller / LiveView
participant Inv as Invoices Context
participant Auth as Authorization
participant DB as Database
User->>Ctrl: Request invoice(s)/action (role, api token/current_user)
Ctrl->>Inv: list/get invoice(s) with opts {role, user_id}
Inv->>Auth: can?(role, :view_all_invoice_types)
Auth-->>Inv: permission result
alt full visibility
Inv->>DB: Query invoices (no access filter)
else limited visibility
Inv->>DB: Query WHERE access_restricted = false
Inv->>DB: OR invoice.id IN (SELECT invoice_id FROM invoice_access_grants WHERE user_id = ?)
end
DB-->>Inv: Filtered results
Inv-->>Ctrl: Invoice(s) (+ grants when requested)
Ctrl-->>User: Render JSON / HTML / LiveView update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
- GET /api/invoices/:id/access — view access status and grants - PUT /api/invoices/:id/access — toggle access_restricted flag - POST /api/invoices/:id/access/grants — grant user access - DELETE /api/invoices/:id/access/grants/:user_id — revoke access - All endpoints require :manage_team permission (owner/admin only) - Add access_restricted field to Invoice OpenAPI schema and JSON response - Add AccessGrant and AccessGrantListResponse OpenAPI schemas - 6 new API tests including reviewer filtering verification 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/ksef_hub/invoices/invoice_access_grant.ex (1)
1-21: Consider adding a changeset for constraint handling.The schema is correctly defined with appropriate associations. However, adding a changeset function would allow proper handling of the unique constraint violation (duplicate grant attempts) and provide a more idiomatic Ecto pattern.
♻️ Optional: Add changeset for validation
defmodule KsefHub.Invoices.InvoiceAccessGrant do `@moduledoc` "Schema for access grants on restricted invoices. Links a user to an invoice they are allowed to see." use Ecto.Schema + import Ecto.Changeset alias KsefHub.Accounts.User alias KsefHub.Invoices.Invoice `@type` t :: %__MODULE__{} `@primary_key` {:id, :binary_id, autogenerate: true} `@foreign_key_type` :binary_id schema "invoice_access_grants" do belongs_to :invoice, Invoice belongs_to :user, User belongs_to :granted_by, User timestamps() end + + `@doc` "Builds a changeset for creating an access grant." + `@spec` changeset(t(), map()) :: Ecto.Changeset.t() + def changeset(grant, attrs) do + grant + |> cast(attrs, [:invoice_id, :user_id, :granted_by_id]) + |> validate_required([:invoice_id, :user_id]) + |> foreign_key_constraint(:invoice_id) + |> foreign_key_constraint(:user_id) + |> unique_constraint([:invoice_id, :user_id]) + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/invoice_access_grant.ex` around lines 1 - 21, Add a changeset function to KsefHub.Invoices.InvoiceAccessGrant that casts and validates the association ids (:invoice_id, :user_id, :granted_by_id), enforces required fields and adds unique_constraint for the duplicate-grant case (e.g., unique index on [:invoice_id, :user_id]) and foreign_key_constraint calls for :invoice_id, :user_id and :granted_by_id so constraint violations are translated into changeset errors instead of crashing.lib/ksef_hub_web/live/invoice_live/show.ex (1)
88-90: Avoid loading access data when the card is hidden.
access_grantsandcompany_reviewersare fetched on every invoice view, even whencan_manage_accessis false and the section never renders. Guarding those assigns keeps the read-only path from paying two unnecessary queries.♻️ Suggested change
can_manage_payment_requests: can_manage_payment_requests, can_view_payment_requests: can_view_payment_requests, can_manage_access: can_manage_access, - access_grants: Invoices.list_access_grants(invoice.id), - company_reviewers: list_company_reviewers(company.id), + access_grants: if(can_manage_access, do: Invoices.list_access_grants(invoice.id), else: []), + company_reviewers: + if(can_manage_access, do: list_company_reviewers(company.id), else: []), payment_status: payment_status,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/show.ex` around lines 88 - 90, The view currently always calls Invoices.list_access_grants(invoice.id) and list_company_reviewers(company.id) even when can_manage_access is false; change the assigns so access_grants and company_reviewers are only fetched and assigned when can_manage_access is true (keep them nil or omit the keys otherwise). Locate the block building the socket assigns (where can_manage_access is set) in invoice_live/show.ex and wrap or branch the calls to Invoices.list_access_grants and list_company_reviewers behind that boolean so the queries run only when can_manage_access is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 1442-1447: The revoke button (phx-click="revoke_access" with
phx-value-user_id={grant.user_id}) is only visible on hover; add focus-driven
visibility classes so keyboard users can see and use it—update the button's
class to include focus/focus-visible (e.g., add focus:opacity-100 and/or
focus-visible:opacity-100, matching the pattern used by the comment actions) so
it becomes fully opaque on keyboard focus while keeping the existing hover/group
behavior and aria-label intact.
In `@lib/ksef_hub/invoices.ex`:
- Around line 1805-1807: The helper full_invoice_visibility?/1 currently treats
a nil role as full access, which allows callers that omit :role or :user_id to
bypass scoping; change full_invoice_visibility?(nil) to return false (deny by
default) and ensure any logic in maybe_filter_by_access/3 (and its catch-all
clause) relies on explicit presence of opts[:role] or opts[:user_id] before
returning the unfiltered query; update checks so missing role/user_id triggers
access_restricted behavior (or the filtering path) rather than granting full
visibility.
- Around line 1731-1742: grant_access/3 currently inserts unvalidated grants;
first load the invoice and the target user (e.g., via Repo.get/Repo.get_by or
your domain fetchers) and verify the user is a reviewer and user.company_id ==
invoice.company_id before calling Repo.insert; if the checks fail, return
{:error, changeset} (build a changeset from %InvoiceAccessGrant{} and add an
appropriate error via Ecto.Changeset.add_error) so the function still returns
the declared {:ok, ...} | {:error, Ecto.Changeset.t()} shape and prevents
cross-company or non-reviewer grants.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 88-90: The view currently always calls
Invoices.list_access_grants(invoice.id) and list_company_reviewers(company.id)
even when can_manage_access is false; change the assigns so access_grants and
company_reviewers are only fetched and assigned when can_manage_access is true
(keep them nil or omit the keys otherwise). Locate the block building the socket
assigns (where can_manage_access is set) in invoice_live/show.ex and wrap or
branch the calls to Invoices.list_access_grants and list_company_reviewers
behind that boolean so the queries run only when can_manage_access is true.
In `@lib/ksef_hub/invoices/invoice_access_grant.ex`:
- Around line 1-21: Add a changeset function to
KsefHub.Invoices.InvoiceAccessGrant that casts and validates the association ids
(:invoice_id, :user_id, :granted_by_id), enforces required fields and adds
unique_constraint for the duplicate-grant case (e.g., unique index on
[:invoice_id, :user_id]) and foreign_key_constraint calls for :invoice_id,
:user_id and :granted_by_id so constraint violations are translated into
changeset errors instead of crashing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ded0fc3-3af8-4815-88bb-182a4c35d8cf
📒 Files selected for processing (11)
lib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/invoices/invoice_access_grant.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/live/invoice_live/classify.exlib/ksef_hub_web/live/invoice_live/index.exlib/ksef_hub_web/live/invoice_live/show.expriv/repo/migrations/20260324000000_add_invoice_access_control.exstest/ksef_hub/invoices_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/support/factory.ex
Comment on lines
+1805
to
+1807
| @spec full_invoice_visibility?(Membership.role() | nil) :: boolean() | ||
| defp full_invoice_visibility?(nil), do: true | ||
| defp full_invoice_visibility?(role), do: Authorization.can?(role, :view_all_invoice_types) |
There was a problem hiding this comment.
Don't make omitted auth context equivalent to full access.
full_invoice_visibility?(nil) plus the catch-all maybe_filter_by_access/3 clause turn missing :role/:user_id into the unfiltered query. Any caller that forgets to thread one of those opts bypasses both reviewer scoping and access_restricted, so this authorization boundary is currently fail-open.
Also applies to: 1823-1840
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices.ex` around lines 1805 - 1807, The helper
full_invoice_visibility?/1 currently treats a nil role as full access, which
allows callers that omit :role or :user_id to bypass scoping; change
full_invoice_visibility?(nil) to return false (deny by default) and ensure any
logic in maybe_filter_by_access/3 (and its catch-all clause) relies on explicit
presence of opts[:role] or opts[:user_id] before returning the unfiltered query;
update checks so missing role/user_id triggers access_restricted behavior (or
the filtering path) rather than granting full visibility.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ksef_hub_web/controllers/api/invoice_controller_test.exs (1)
1492-1518: Add malformeduser_idtests for grant/revoke.Please add negative tests for invalid UUID inputs (e.g.,
"not-a-uuid") so API error semantics are pinned for both endpoints.🧪 Suggested test additions
+ test "grant_access returns 422 for invalid user_id", %{conn: conn} do + %{company: company, token: token} = create_user_with_token(:owner) + invoice = insert(:invoice, company: company, access_restricted: true) + + conn = + conn + |> api_conn(token) + |> post("/api/invoices/#{invoice.id}/access/grants", %{user_id: "not-a-uuid"}) + + assert conn.status == 422 + end + + test "revoke_access returns 422 for invalid user_id", %{conn: conn} do + %{company: company, token: token} = create_user_with_token(:owner) + invoice = insert(:invoice, company: company, access_restricted: true) + + conn = + conn + |> api_conn(token) + |> delete("/api/invoices/#{invoice.id}/access/grants/not-a-uuid") + + assert conn.status == 422 + endAlso applies to: 1520-1526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs` around lines 1492 - 1518, Add negative tests for malformed UUIDs covering both grant and revoke endpoints: create a test (e.g., "grant_access and revoke_access reject malformed user_id") that sets up company/token and an access_restricted invoice like the existing "grant_access and revoke_access work" test, then call POST "/api/invoices/#{invoice.id}/access/grants" with %{user_id: "not-a-uuid"} and assert the response is a 400 (or the API's validation status) and contains an error about invalid UUID, and similarly call DELETE "/api/invoices/#{invoice.id}/access/grants/not-a-uuid" and assert the same 400 + validation error; use the same helpers (api_conn, create_user_with_token, insert) to locate where to add the tests near the existing test for grants/revokes so the behavior is pinned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex`:
- Around line 926-933: The OpenAPI responses map for the set_access operation
(in lib/ksef_hub_web/controllers/api/invoice_controller.ex) is missing the 422
entry that the controller actually returns; update the responses map used by the
set_access operation to include a 422 response (e.g., {"Unprocessable Entity —
validation failed", "application/json", Schemas.ErrorResponse}) so the API
contract matches the actual returns. Locate the responses map near the
set_access operation and add the 422 key with a clear message and the
appropriate schema (Schemas.ErrorResponse) to cover the validation/error cases
referenced on the other lines.
---
Nitpick comments:
In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs`:
- Around line 1492-1518: Add negative tests for malformed UUIDs covering both
grant and revoke endpoints: create a test (e.g., "grant_access and revoke_access
reject malformed user_id") that sets up company/token and an access_restricted
invoice like the existing "grant_access and revoke_access work" test, then call
POST "/api/invoices/#{invoice.id}/access/grants" with %{user_id: "not-a-uuid"}
and assert the response is a 400 (or the API's validation status) and contains
an error about invalid UUID, and similarly call DELETE
"/api/invoices/#{invoice.id}/access/grants/not-a-uuid" and assert the same 400 +
validation error; use the same helpers (api_conn, create_user_with_token,
insert) to locate where to add the tests near the existing test for
grants/revokes so the behavior is pinned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c4a0f43-1ceb-4e96-9325-df10cbea0192
📒 Files selected for processing (6)
lib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/router.exlib/ksef_hub_web/schemas/access_grant.exlib/ksef_hub_web/schemas/access_grant_list_response.exlib/ksef_hub_web/schemas/invoice.extest/ksef_hub_web/controllers/api/invoice_controller_test.exs
✅ Files skipped from review due to trivial changes (3)
- lib/ksef_hub_web/schemas/invoice.ex
- lib/ksef_hub_web/schemas/access_grant_list_response.ex
- lib/ksef_hub_web/schemas/access_grant.ex
Review fixes: - Revoke button: add focus:opacity-100 focus-visible:opacity-100 for keyboard accessibility (matching comment actions pattern) - maybe_filter_by_access catch-all: deny restricted invoices when role is present but user_id is missing (safety net for forgotten opts) - grant_access: validate target user is a company member with a non-privileged role before inserting; reject cross-company and admin/owner/accountant grants with changeset errors - InvoiceAccessGrant: add changeset/2 with cast, validate_required, unique_constraint and foreign_key_constraint - Lazy-load access_grants/company_reviewers only when can_manage_access - set_access OpenAPI: add missing 422 response - API grant/revoke: validate UUID format before passing to context - Tests: add grant validation tests (non-member, full-visibility role, malformed UUID), update existing CRUD tests to use proper memberships Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/ksef_hub/invoices.ex (1)
1847-1886: Consider documenting the authorization bypass fornilrole.The current design treats
nilrole as a system/internal caller with full access (line 1848, 1883). While a safety net exists for cases where role is present butuser_idis missing (line 1885-1886), callers that omit both parameters bypass access filtering entirely.This appears intentional for background jobs and internal operations, but the implicit trust model should be documented. Consider adding a comment explaining when
nilrole is expected and warning that omitting both opts bypasses authorization.+ # NOTE: nil role indicates an internal/system call (e.g., background jobs, sync) + # and bypasses access filtering. External callers (API, LiveView) must always + # provide role and user_id to enforce access restrictions. `@spec` full_invoice_visibility?(Membership.role() | nil) :: boolean() defp full_invoice_visibility?(nil), do: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices.ex` around lines 1847 - 1886, Add an explicit comment documenting the authorization bypass for a nil role: explain that full_invoice_visibility?/1 treats nil as an internal/system caller with full access and that maybe_filter_by_access/3, scope_by_role/2, and maybe_scope_type_by_role/2 therefore do no filtering for nil; state the intended callers (e.g., background jobs, system processes), and warn that omitting both role and user_id will bypass access checks so callers should pass a non-nil role/user_id for external requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub/invoices/invoice_access_grant.ex`:
- Around line 23-33: Remove the unused changeset/2 function from the
InvoiceAccessGrant module: delete the entire def changeset(grant, attrs) block
so programmatically-set fields (invoice_id, user_id, granted_by_id) are not
exposed via cast; search for any callers of changeset/2 and replace creation
code to explicitly set those fields when building the struct or use a safe
changeset that only casts permitted public attributes if you actually need a
changeset helper in the future.
---
Nitpick comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 1847-1886: Add an explicit comment documenting the authorization
bypass for a nil role: explain that full_invoice_visibility?/1 treats nil as an
internal/system caller with full access and that maybe_filter_by_access/3,
scope_by_role/2, and maybe_scope_type_by_role/2 therefore do no filtering for
nil; state the intended callers (e.g., background jobs, system processes), and
warn that omitting both role and user_id will bypass access checks so callers
should pass a non-nil role/user_id for external requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 068d772c-4cc1-42e3-87de-5b40b5031c53
📒 Files selected for processing (6)
lib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice_access_grant.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/live/invoice_live/show.extest/ksef_hub/invoices_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exs
✅ Files skipped from review due to trivial changes (2)
- test/ksef_hub_web/controllers/api/invoice_controller_test.exs
- test/ksef_hub/invoices_test.exs
Comment on lines
+23
to
+33
| @doc "Builds a changeset for creating an access grant." | ||
| @spec changeset(t() | Ecto.Changeset.t(), map()) :: Ecto.Changeset.t() | ||
| def changeset(grant, attrs) do | ||
| grant | ||
| |> cast(attrs, [:invoice_id, :user_id, :granted_by_id]) | ||
| |> validate_required([:invoice_id, :user_id]) | ||
| |> unique_constraint([:invoice_id, :user_id]) | ||
| |> foreign_key_constraint(:invoice_id) | ||
| |> foreign_key_constraint(:user_id) | ||
| |> foreign_key_constraint(:granted_by_id) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if InvoiceAccessGrant.changeset/2 is actually called anywhere
rg -n "InvoiceAccessGrant\.changeset" --type=elixir
rg -n "InvoiceAccessGrant\)\.changeset" --type=elixirRepository: appunite/appunite-ksef-ex
Length of output: 51
Remove the unused changeset/2 function.
The function is not called anywhere in the codebase. Additionally, if it were used, it would violate the coding guideline that programmatically-set fields like user_id, invoice_id, and granted_by_id must not be listed in cast calls — these should be explicitly set when creating the struct instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices/invoice_access_grant.ex` around lines 23 - 33, Remove
the unused changeset/2 function from the InvoiceAccessGrant module: delete the
entire def changeset(grant, attrs) block so programmatically-set fields
(invoice_id, user_id, granted_by_id) are not exposed via cast; search for any
callers of changeset/2 and replace creation code to explicitly set those fields
when building the struct or use a safe changeset that only casts permitted
public attributes if you actually need a changeset helper in the future.
Replace hardcoded type-based scoping (reviewers forced to expense only) with the access control system. Income invoices are now automatically created with access_restricted: true, so reviewers can't see them unless explicitly granted access — same mechanism as restricted expense invoices. - Remove scope_by_role and maybe_scope_type_by_role from Invoices context - Auto-set access_restricted: true on income invoice creation/upsert - Pass access_restricted through trusted_fields in do_insert/do_upsert - Migration: set access_restricted=true on all existing income invoices - Reviewers now see both Expense/Income tabs (empty income is expected) - Remove sanitize_type from LiveView index (access control handles it) - Update all tests to use access_restricted: true on income test fixtures - Fix credo issues: convert with-single-clause to case in API controller Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/ksef_hub/invoices.ex (1)
1861-1886:⚠️ Potential issue | 🟠 MajorAuthorization defaults remain fail-open for missing role.
full_invoice_visibility?(nil)returningtrueand themaybe_filter_by_access(query, nil, _user_id)clause returning an unfiltered query means that any caller omitting:rolebypasses access filtering entirely.While the comment indicates this is intentional for "internal/system calls," this design creates risk if an external entry point (controller/LiveView) inadvertently omits the
roleoption. The safety net on lines 1885-1886 only catches the case where role is present butuser_idis missing.Consider adding compile-time or runtime validation in controller/LiveView entry points to ensure
roleis always passed, or inverting the default to deny access when role isnil.#!/bin/bash # Verify all callers of get_invoice*/list_invoices* pass role: option echo "=== Checking API controller calls ===" rg -n "Invoices\.(get_invoice|list_invoices)" lib/ksef_hub_web/controllers/api/ -A 3 | head -60 echo "" echo "=== Checking LiveView calls ===" rg -n "Invoices\.(get_invoice|list_invoices)" lib/ksef_hub_web/live/ -A 3 | head -60 echo "" echo "=== Checking other callers that might omit role ===" rg -n "Invoices\.(get_invoice|list_invoices)" lib/ksef_hub/ --glob '!lib/ksef_hub/invoices.ex' -A 3 | head -60🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices.ex` around lines 1861 - 1886, full_invoice_visibility?(nil) and the current maybe_filter_by_access/3 clause for nil role are leaving access open when callers omit role; change the default to deny rather than allow by updating full_invoice_visibility?(nil) to return false (or remove the nil clause) and make the maybe_filter_by_access(query, nil, _user_id) clause apply the restricted-invoice filter (same as the _role clause), ensuring callers that omit role cannot bypass checks; alternatively, if you want to keep a special internal/system path, add an explicit internal API (e.g., maybe_filter_by_access_internal/3) and require callers to use it so public functions Invoices.get_invoice*/list_invoices* must always pass role.
🧹 Nitpick comments (3)
lib/ksef_hub_web/live/invoice_live/index.ex (1)
92-92: Remove the leftover role-based wrapper.
can_view_all_typesis hard-coded now andnormalize_filters/2no longer transforms anything, so the old fallback path is dead code. Inliningfiltershere and dropping the unused wrapper will make the access-control flow easier to follow.Also applies to: 100-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/index.ex` at line 92, The role-based wrapper around setting filters is now dead: remove the wrapper that calls normalize_filters/2 and directly inline the hard-coded filters assignment where can_view_all_types: true is currently set; delete the unused normalize_filters/2 fallback/conditional path and any variables only used for that branch so the access-control flow uses the direct filters value (update occurrences referenced around the same block and the related lines noted at 100-101).test/ksef_hub_web/live/invoice_live/show_test.exs (1)
1009-1015: Assert the access card by selector, not by raw HTML text.These checks will break on copy or markup churn and don't prove that the specific card/menu is present or absent. Prefer
has_element?/2against the card/menu selector you added for this UI.Based on learnings "Never test against raw HTML - always use
element/2,has_element?/2, and similar functions with selectors likeassert has_element?(view, \"#my-form\")."Also applies to: 1017-1035
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/invoice_live/show_test.exs` around lines 1009 - 1015, Replace brittle raw-HTML assertions in the test "access control card is visible for owner" by using the LiveView helpers: capture the returned view from live/2 (use the second element from {:ok, view, _html}) and assert presence of the access card via has_element?/2 or element/2 with the specific selector you added (e.g. "#access-card" or ".access-control-card"); do the same replacement for the related assertions in the block covering lines 1017-1035 so tests assert has_element?(view, "YOUR_SELECTOR") instead of html =~ "Access" / "All reviewers".test/ksef_hub_web/live/invoice_live/index_test.exs (1)
347-366: This case still only proves the default expense tab.The view defaults to
type=expense, so the restricted income invoice never reaches the query/render path here. If the intent is to cover the new access-control behavior, drive the test with?type=incomeor add an unrestricted income control and assert only the restricted record is filtered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/invoice_live/index_test.exs` around lines 347 - 366, The test currently relies on the default expense tab so the access-restricted income never reaches the view; update the test to drive the income view explicitly by calling live with the income query param (replace live(conn, ~p"/c/#{company.id}/invoices") with live(conn, ~p"/c/#{company.id}/invoices?type=income")) and insert an additional unrestricted income invoice (e.g. insert(:invoice, type: :income, seller_name: "Visible Income Seller", company: company, access_restricted: false)) so you can assert the restricted income ("Hidden Income Seller") is filtered out while the unrestricted income is shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex`:
- Around line 1011-1045: The grant_access/2 clause only matches when user_id is
a binary and will raise FunctionClauseError if missing or non-string; add a
fallback clause named grant_access/2 that matches conn and params (or pattern
with missing or non-binary "user_id") similar to set_access, and return a
guarded JSON error response (e.g., put_status(:unprocessable_entity) or
:bad_request and json(%{error: "Invalid or missing user_id"})) to safely handle
invalid inputs instead of crashing; update the grant_access function set to
include the existing binary user_id clause plus this new fallback clause so all
request shapes are handled.
In `@test/ksef_hub/invoices_test.exs`:
- Around line 600-605: The test currently uses role: nil which creates a hidden
backdoor — change the test "role: nil returns all invoices (backward compat)" so
it supplies an explicit auth context instead of nil; call
Invoices.list_invoices_paginated(company.id, %{}, role: :admin) (or an
appropriate explicit role/user_id) and assert total_count == 2, or alternatively
update the test to expect restricted results when no role is provided; this
ensures the code path in Invoices.list_invoices_paginated is exercised with an
explicit auth context rather than relying on a nil bypass.
---
Duplicate comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 1861-1886: full_invoice_visibility?(nil) and the current
maybe_filter_by_access/3 clause for nil role are leaving access open when
callers omit role; change the default to deny rather than allow by updating
full_invoice_visibility?(nil) to return false (or remove the nil clause) and
make the maybe_filter_by_access(query, nil, _user_id) clause apply the
restricted-invoice filter (same as the _role clause), ensuring callers that omit
role cannot bypass checks; alternatively, if you want to keep a special
internal/system path, add an explicit internal API (e.g.,
maybe_filter_by_access_internal/3) and require callers to use it so public
functions Invoices.get_invoice*/list_invoices* must always pass role.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/index.ex`:
- Line 92: The role-based wrapper around setting filters is now dead: remove the
wrapper that calls normalize_filters/2 and directly inline the hard-coded
filters assignment where can_view_all_types: true is currently set; delete the
unused normalize_filters/2 fallback/conditional path and any variables only used
for that branch so the access-control flow uses the direct filters value (update
occurrences referenced around the same block and the related lines noted at
100-101).
In `@test/ksef_hub_web/live/invoice_live/index_test.exs`:
- Around line 347-366: The test currently relies on the default expense tab so
the access-restricted income never reaches the view; update the test to drive
the income view explicitly by calling live with the income query param (replace
live(conn, ~p"/c/#{company.id}/invoices") with live(conn,
~p"/c/#{company.id}/invoices?type=income")) and insert an additional
unrestricted income invoice (e.g. insert(:invoice, type: :income, seller_name:
"Visible Income Seller", company: company, access_restricted: false)) so you can
assert the restricted income ("Hidden Income Seller") is filtered out while the
unrestricted income is shown.
In `@test/ksef_hub_web/live/invoice_live/show_test.exs`:
- Around line 1009-1015: Replace brittle raw-HTML assertions in the test "access
control card is visible for owner" by using the LiveView helpers: capture the
returned view from live/2 (use the second element from {:ok, view, _html}) and
assert presence of the access card via has_element?/2 or element/2 with the
specific selector you added (e.g. "#access-card" or ".access-control-card"); do
the same replacement for the related assertions in the block covering lines
1017-1035 so tests assert has_element?(view, "YOUR_SELECTOR") instead of html =~
"Access" / "All reviewers".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da32dc49-776c-445e-9ecb-6c40b9e6b772
📒 Files selected for processing (8)
lib/ksef_hub/invoices.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/live/invoice_live/index.expriv/repo/migrations/20260324064116_restrict_income_invoices.exstest/ksef_hub/invoices_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/ksef_hub_web/live/invoice_live/index_test.exstest/ksef_hub_web/live/invoice_live/show_test.exs
✅ Files skipped from review due to trivial changes (1)
- priv/repo/migrations/20260324064116_restrict_income_invoices.exs
Comment on lines
600
to
605
| test "role: nil returns all invoices (backward compat)", %{company: company} do | ||
| insert(:invoice, type: :income, company: company) | ||
| insert(:invoice, type: :expense, company: company) | ||
|
|
||
| result = Invoices.list_invoices_paginated(company.id, %{}, role: nil) | ||
|
|
||
| assert result.total_count == 2 |
There was a problem hiding this comment.
Don't keep role: nil as an unrestricted backdoor.
This test bakes in “missing auth context == full visibility”. The context APIs still default opts to [], so any caller that forgets to pass role/user_id will silently bypass the new restriction model and expose restricted invoices. Safer behavior is to require explicit auth context or default the nil-role path to the restricted view instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ksef_hub/invoices_test.exs` around lines 600 - 605, The test currently
uses role: nil which creates a hidden backdoor — change the test "role: nil
returns all invoices (backward compat)" so it supplies an explicit auth context
instead of nil; call Invoices.list_invoices_paginated(company.id, %{}, role:
:admin) (or an appropriate explicit role/user_id) and assert total_count == 2,
or alternatively update the test to expect restricted results when no role is
provided; this ensures the code path in Invoices.list_invoices_paginated is
exercised with an explicit auth context rather than relying on a nil bypass.
Security fixes: - invoice_pdf_controller: add missing user_id to access filtering (reviewer could download restricted invoice PDFs) - payment_request_live/form: add role/user_id to invoice lookup (reviewer could prefill payment from restricted invoice) - Prevent unrestricting income invoices (set_access_restricted rejects income + false to maintain invariant that income is always restricted) New tests: - Income invoice cannot be unrestricted - Expense invoice can be toggled freely - Grant to non-member returns error - Grant to admin returns error (already has full access) - Upsert auto-restricts income invoices - API: cannot unrestrict income invoice (422) Documentation: - ADR 0035: Invoice Access Control architecture decision record Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/ksef_hub_web/controllers/api/invoice_controller.ex (1)
1016-1050:⚠️ Potential issue | 🟠 MajorAdd the missing fallback for invalid
grant_accesspayloads.This action still only matches when
user_idis present and binary. A missing or non-string payload will raiseFunctionClauseErrorinstead of returning the documented422.🛡️ Proposed fallback
def grant_access(conn, %{"id" => id, "user_id" => user_id}) when is_binary(user_id) do case Ecto.UUID.cast(user_id) do {:ok, _} -> company_id = conn.assigns.current_company.id granted_by_id = conn.assigns.api_token.created_by_id invoice = Invoices.get_invoice!(company_id, id, role: conn.assigns[:current_role], user_id: granted_by_id ) case Invoices.grant_access(invoice.id, user_id, granted_by_id) do {:ok, _grant} -> grants = Invoices.list_access_grants(invoice.id) json(conn, %{ data: %{ access_restricted: invoice.access_restricted, grants: Enum.map(grants, &access_grant_json/1) } }) {:error, _} -> conn |> put_status(:unprocessable_entity) |> json(%{error: "Failed to grant access"}) end :error -> conn |> put_status(:unprocessable_entity) |> json(%{error: "Invalid user_id format"}) end end + + def grant_access(conn, _params) do + conn + |> put_status(:unprocessable_entity) + |> json(%{error: "user_id (UUID string) is required"}) + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex` around lines 1016 - 1050, The grant_access action currently only has a clause def grant_access(conn, %{"id" => id, "user_id" => user_id}) when is_binary(user_id) and will raise FunctionClauseError for missing or non-string payloads; add a fallback clause (e.g., def grant_access(conn, _params) or def grant_access(conn, %{"id" => _id} = _params) to match requests without a binary "user_id") that returns put_status(:unprocessable_entity) and json(%{error: "Invalid or missing user_id"}) so invalid payloads consistently respond with 422 instead of crashing; update any tests or docs referencing grant_access behavior as needed.
🧹 Nitpick comments (1)
test/ksef_hub_web/controllers/api/invoice_controller_test.exs (1)
1465-1583: Add one API allow-path test for explicit grants.The new suite proves deny-paths and grant CRUD, but it never verifies that a reviewer with an explicit grant can actually
GET /api/invoices/:id(and ideally see it inindex). That end-to-end assertion is the controller wiring this PR changes, so it's worth covering here. Based on learnings: Test controller actions and context interactions using integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs` around lines 1465 - 1583, Add an integration test that verifies a reviewer with an explicit grant can view a restricted invoice via GET /api/invoices/:id and see it in GET /api/invoices: create owner and reviewer users with create_user_with_token, insert an access_restricted invoice (insert(:invoice, access_restricted: true)), add the reviewer membership (insert(:membership, user: reviewer, company: company, role: :reviewer)), grant access by POSTing to "/api/invoices/#{invoice.id}/access/grants" using api_conn(token) for the owner and then call GET "/api/invoices/#{invoice.id}" with the reviewer token to assert status 200 and returned invoice id matches; also call GET "/api/invoices" with reviewer token and assert the invoice appears in the returned data and meta.total_count > 0. Ensure you use the same helper names (create_user_with_token, api_conn, insert) and the existing grants endpoints used in tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex`:
- Around line 1068-1074: Update the OpenApi operation spec for revoke_access/2
in InvoiceController to include a 422 response entry: locate the operation
definition (the responses map for revoke_access/2) and add a 422 =>
{"Unprocessable Entity — invalid request parameters (e.g. invalid UUID)",
"application/json", Schemas.ErrorResponse} so generated clients/docs match the
runtime behavior; apply the same 422 addition to the other revoke_access spec
occurrence noted around the 1107-1110 region and ensure the controller still
uses OpenApiSpex.ControllerSpecs.
---
Duplicate comments:
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex`:
- Around line 1016-1050: The grant_access action currently only has a clause def
grant_access(conn, %{"id" => id, "user_id" => user_id}) when is_binary(user_id)
and will raise FunctionClauseError for missing or non-string payloads; add a
fallback clause (e.g., def grant_access(conn, _params) or def grant_access(conn,
%{"id" => _id} = _params) to match requests without a binary "user_id") that
returns put_status(:unprocessable_entity) and json(%{error: "Invalid or missing
user_id"}) so invalid payloads consistently respond with 422 instead of
crashing; update any tests or docs referencing grant_access behavior as needed.
---
Nitpick comments:
In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs`:
- Around line 1465-1583: Add an integration test that verifies a reviewer with
an explicit grant can view a restricted invoice via GET /api/invoices/:id and
see it in GET /api/invoices: create owner and reviewer users with
create_user_with_token, insert an access_restricted invoice (insert(:invoice,
access_restricted: true)), add the reviewer membership (insert(:membership,
user: reviewer, company: company, role: :reviewer)), grant access by POSTing to
"/api/invoices/#{invoice.id}/access/grants" using api_conn(token) for the owner
and then call GET "/api/invoices/#{invoice.id}" with the reviewer token to
assert status 200 and returned invoice id matches; also call GET "/api/invoices"
with reviewer token and assert the invoice appears in the returned data and
meta.total_count > 0. Ensure you use the same helper names
(create_user_with_token, api_conn, insert) and the existing grants endpoints
used in tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85057935-7992-4d86-9c07-a65c31d8a0cc
📒 Files selected for processing (8)
docs/adr/0035-invoice-access-control.mdlib/ksef_hub/invoices.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/controllers/invoice_pdf_controller.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/payment_request_live/form.extest/ksef_hub/invoices_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exs
✅ Files skipped from review due to trivial changes (2)
- docs/adr/0035-invoice-access-control.md
- test/ksef_hub/invoices_test.exs
Comment on lines
+612
to
+623
| def handle_event("grant_access", %{"user_id" => user_id}, socket) do | ||
| invoice = socket.assigns.invoice | ||
| granted_by_id = socket.assigns.current_user.id | ||
|
|
||
| case Invoices.grant_access(invoice.id, user_id, granted_by_id) do | ||
| {:ok, _grant} -> | ||
| {:noreply, assign(socket, access_grants: Invoices.list_access_grants(invoice.id))} | ||
|
|
||
| {:error, _} -> | ||
| {:noreply, put_flash(socket, :error, "Failed to grant access.")} | ||
| end | ||
| end |
There was a problem hiding this comment.
Guard user_id in the access events.
Both handlers trust a client-supplied user_id. A tampered event with a missing or malformed value can miss the function head or raise before you ever reach the flash path, which kills the LiveView instead of returning a recoverable error.
🛡️ Proposed hardening
- def handle_event("grant_access", %{"user_id" => user_id}, socket) do
+ def handle_event("grant_access", %{"user_id" => user_id}, socket) when is_binary(user_id) do
invoice = socket.assigns.invoice
granted_by_id = socket.assigns.current_user.id
- case Invoices.grant_access(invoice.id, user_id, granted_by_id) do
- {:ok, _grant} ->
- {:noreply, assign(socket, access_grants: Invoices.list_access_grants(invoice.id))}
-
- {:error, _} ->
- {:noreply, put_flash(socket, :error, "Failed to grant access.")}
+ with {:ok, _} <- Ecto.UUID.cast(user_id),
+ {:ok, _grant} <- Invoices.grant_access(invoice.id, user_id, granted_by_id) do
+ {:noreply, assign(socket, access_grants: Invoices.list_access_grants(invoice.id))}
+ else
+ :error ->
+ {:noreply, put_flash(socket, :error, "Invalid reviewer id.")}
+
+ {:error, _} ->
+ {:noreply, put_flash(socket, :error, "Failed to grant access.")}
end
end
+
+ def handle_event("grant_access", _params, socket),
+ do: {:noreply, put_flash(socket, :error, "Invalid reviewer id.")}
- def handle_event("revoke_access", %{"user_id" => user_id}, socket) do
+ def handle_event("revoke_access", %{"user_id" => user_id}, socket) when is_binary(user_id) do
invoice = socket.assigns.invoice
- case Invoices.revoke_access(invoice.id, user_id) do
- {:ok, _} ->
- {:noreply, assign(socket, access_grants: Invoices.list_access_grants(invoice.id))}
-
- {:error, _} ->
- {:noreply, put_flash(socket, :error, "Failed to revoke access.")}
+ with {:ok, _} <- Ecto.UUID.cast(user_id),
+ {:ok, _} <- Invoices.revoke_access(invoice.id, user_id) do
+ {:noreply, assign(socket, access_grants: Invoices.list_access_grants(invoice.id))}
+ else
+ :error ->
+ {:noreply, put_flash(socket, :error, "Invalid reviewer id.")}
+
+ {:error, _} ->
+ {:noreply, put_flash(socket, :error, "Failed to revoke access.")}
end
end
+
+ def handle_event("revoke_access", _params, socket),
+ do: {:noreply, put_flash(socket, :error, "Invalid reviewer id.")}Also applies to: 630-639
- Remove dead `can_view_all_types` assign (always true) and template conditionals from invoice index — both tabs now render unconditionally - Simplify `maybe_filter_by_access` to accept opts keyword list directly with safety-net fallback for missing user_id (deny restricted) - Change `full_invoice_visibility?(nil)` to return false (fail-closed) - Add grant_access fallback clause for missing user_id in API controller - Improve test coverage: reviewer-with-grant e2e API test, reviewer access filtering in index LiveView test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.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
1 participant
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.
Allow admins/owners to restrict invoice visibility so only explicitly granted reviewers can see them. Adds access_restricted boolean flag on invoices and invoice_access_grants join table. Refactors role checks to use Authorization.can? instead of hardcoded pattern matching.
Summary by CodeRabbit