Feat/centralized authorization roles#81
Merged
Merged
Conversation
Single source of truth for role-permission checks. Defines permissions for owner, admin, reviewer, and accountant roles with table-driven tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
No migration needed since role is stored as string (Ecto.Enum at app level). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reusable plug checking Authorization.can?/2, returns 403 on failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add :require_admin hook (owner + admin) - Move certificates, categories, tags, team into :admin_only session - Keep tokens, syncs, exports in :authenticated (mount-level checks) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace inline role checks with centralized permission checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Token create/revoke: owner, admin, accountant - Invitation create/cancel: owner, admin Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- InvoiceController: block accountant from mutations - CategoryController/TagController: block reviewer + accountant from CUD - TokenController: require :manage_tokens, update error messages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- TokenLive: check :manage_tokens (owner/admin/accountant) - ExportLive: check :view_exports via Authorization.can?/2 - SyncLive: check :view_syncs (blocks accountant) - InvoiceLive.Upload: check :create_invoice (blocks accountant) - InvoiceLive.Show: hide mutation UI (edit/approve/reject/tags/categories) for accountant - CompanyLive.Index: check :manage_company for :new/:edit actions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add create_admin_with_token/1 and create_accountant_with_token/1 helpers - InvoiceController: accountant blocked from mutations, admin allowed - CategoryController/TagController: accountant/reviewer blocked from CUD, admin allowed - TokenController: admin + accountant can create, reviewer blocked Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Membership: add :admin to valid roles, use :superadmin as invalid - Accounts: use :reviewer for unauthorized token tests (accountant now allowed) - LiveAuth: expect redirect for admin-only pages - Nav: categories/tags hidden for non-admin, tokens visible for accountant - Team: update error message assertion - TokenLive: use :reviewer for blocked access test - Upload: use :accountant for blocked access test (reviewer now allowed) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix line length issues in plug declarations - Add ADR 0029 for centralized authorization refactor - Add docs/roles.md with full permission matrix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ation.can?/2 - Add nil role handling to Authorization.can?/2 (returns false) - Add :view_all_invoice_types permission for reviewer vs accountant invoice visibility - Replace inline role checks in ExportController, LiveAuth, DashboardLive, InvoiceLive.Index, Accounts context - Add :admin to invitation role enum so admins can be invited - Add RequirePermission plug unit test - Add nil role and view_all_invoice_types tests to authorization_test Co-Authored-By: Claude Opus 4.6 <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:
📝 WalkthroughWalkthroughCentralizes authorization into KsefHub.Authorization (can?/2 and can?/3), introduces an :admin role, replaces scattered role checks with permission-based plugs and LiveAuth hooks, updates router live_sessions, controllers, LiveViews, layout/nav, docs, helpers, and tests to use the new permission model. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Plug as RequirePermission
participant Auth as KsefHub.Authorization
participant Controller
Client->>Router: HTTP/Live request (e.g., POST /api/categories or LiveView route)
Router->>Plug: invoke RequirePermission(:manage_categories)
Plug->>Auth: can?(conn.assigns.current_role, :manage_categories)
Auth-->>Plug: true / false
alt allowed
Plug->>Controller: proceed to action / mount
Controller->>Controller: business logic / DB / LiveView assign
Controller-->>Client: 200/201 or LiveView render
else denied
Plug-->>Client: 403 {"error":"Forbidden — insufficient permissions"}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/ksef_hub_web/live/export_live/index.ex (1)
17-23:⚠️ Potential issue | 🟠 MajorRe-check permission inside
"export"as well.
mount/3only gates the initial connection. A LiveView that stays open after the user's membership is downgraded can still call"export"and create a batch, becausehandle_event("export", ...)has no authorization guard. Please enforce the permission again in the event handler before callingdo_export/2.Based on learnings: "Check authorization in controllers and LiveViews" and "Validate all state changes in LiveView events".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/export_live/index.ex` around lines 17 - 23, Add an authorization check inside the LiveView event handler that processes "export" so it doesn't rely solely on mount/3; in the handle_event("export", params, socket) path, call Authorization.can?(socket.assigns[:current_role], :view_exports) and if it returns false return {:noreply, socket |> put_flash(:error, "You don't have permission to access exports.") |> redirect(to: ~p"/c/#{socket.assigns.current_company.id}/invoices")} otherwise proceed to call do_export/2 with the socket and params; ensure you reference socket.assigns[:current_role], socket.assigns.current_company.id, the "export" event handler, and do_export/2 so the guard is enforced at runtime.lib/ksef_hub_web/live/invoice_live/upload.ex (1)
36-42:⚠️ Potential issue | 🟠 MajorRe-check
:create_invoiceauthorization in the"upload"event handler, not only inmount/3.A user who mounts this LiveView while authorized can continue uploading after their role is downgraded or revoked—the existing socket remains valid until reconnect, and
handle_event("upload", ...)performs no re-authorization check. The downstreamdo_upload/3function receives no user or role context, so the write proceeds unguarded. Add the authorization check at event time:🔒 Minimal fix
def handle_event("upload", _params, socket) do - case consume_pdf(socket) do - {:ok, {binary, filename}} -> - start_upload_task(socket, binary, filename) - - {:error, :no_file} -> - {:noreply, put_flash(socket, :error, "Please select a PDF file.")} - - {:error, {:read_failed, _reason}} -> - {:noreply, - put_flash(socket, :error, "Failed to read the uploaded file. Please try again.")} + if Authorization.can?(socket.assigns[:current_role], :create_invoice) do + case consume_pdf(socket) do + {:ok, {binary, filename}} -> + start_upload_task(socket, binary, filename) + + {:error, :no_file} -> + {:noreply, put_flash(socket, :error, "Please select a PDF file.")} + + {:error, {:read_failed, _reason}} -> + {:noreply, + put_flash(socket, :error, "Failed to read the uploaded file. Please try again.")} + end + else + {:noreply, + socket + |> put_flash(:error, "You do not have permission to upload invoices.") + |> push_navigate(to: ~p"/c/#{socket.assigns.current_company.id}/invoices")} end end🤖 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/upload.ex` around lines 36 - 42, The "upload" event handler must re-check permissions at event time: inside the handle_event clause for "upload" (where it currently calls do_upload/3), call Authorization.can?(socket.assigns[:current_role], :create_invoice) and if it returns false, respond like in mount/3 (put_flash :error "You do not have permission to upload invoices." and redirect) instead of proceeding; alternatively ensure do_upload/3 accepts and validates the acting user/role (pass socket.assigns[:current_role] or current_user into do_upload/3 and perform the same Authorization.can? check there) so uploads cannot proceed after a role change.lib/ksef_hub_web/live/invoice_live/show.ex (1)
286-292:⚠️ Potential issue | 🟠 MajorMissing server-side authorization check in mutation events.
The
toggle_editevent (and other mutation events likesave_edit,set_category,toggle_tag,approve,reject) set state or call context functions without verifyingcan_mutateserver-side. While the UI disables these controls, a user could send events directly via WebSocket, bypassing the UI guards.Consider adding a guard at the start of each mutation handler:
🛡️ Proposed fix for toggle_edit
def handle_event("toggle_edit", _params, socket) do + unless socket.assigns.can_mutate do + {:noreply, socket} + else {:noreply, assign(socket, editing: true, edit_form: build_edit_form(socket.assigns.invoice) )} + end endSimilar guards should be added to
save_edit,set_category,toggle_tag,create_and_add_tag,approve,reject,dismiss_duplicate,confirm_duplicate, andre_extracthandlers. Based on learnings: "Check authorization in controllers and LiveViews".🤖 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 286 - 292, Add a server-side authorization guard to all mutation LiveView handlers (e.g., toggle_edit, save_edit, set_category, toggle_tag, create_and_add_tag, approve, reject, dismiss_duplicate, confirm_duplicate, re_extract) by checking the existing authorization flag or function (e.g., socket.assigns.can_mutate or a can_mutate?/1 helper) at the top of each handler; if the check fails, return early (e.g., {:noreply, socket} or redirect/put_flash as appropriate) so no state changes or context calls (like build_edit_form, invoice updates, approve/reject actions) are performed without authorization. Ensure the guard is applied before any assignments, calls to context functions, or side effects in those handler functions.
🧹 Nitpick comments (6)
test/ksef_hub_web/live/invoice_live/upload_test.exs (1)
168-192: Consider adding test coverage for admin role upload access.The tests correctly verify that accountants cannot access upload functionality. Given the PR introduces the
:adminrole to the hierarchy, it would strengthen coverage to add a test verifying that admins can access the upload page and see the upload button, similar to how owners are tested.💡 Suggested additional test case
test "shows Upload PDF button for admin" do {:ok, admin} = Accounts.get_or_create_google_user(%{ uid: "g-upload-idx-admin", email: "admin-idx@example.com", name: "Admin" }) company = insert(:company) insert(:membership, user: admin, company: company, role: :admin) conn = build_conn() |> log_in_user(admin, %{current_company_id: company.id}) {:ok, view, _html} = live(conn, ~p"/c/#{company.id}/invoices") upload_path = ~p"/c/#{company.id}/invoices/upload" assert has_element?(view, ~s(a[href="#{upload_path}"]), "Upload PDF") end🤖 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/upload_test.exs` around lines 168 - 192, Add a new test mirroring the owner test to ensure admins see the Upload PDF button: create an admin user via Accounts.get_or_create_google_user, create a company and membership with role: :admin using insert(:membership), sign in with build_conn() |> log_in_user(admin, %{current_company_id: company.id}), call live(conn, ~p"/c/#{company.id}/invoices") and assert has_element?(view, ~s(a[href="#{upload_path}"]), "Upload PDF"); place this alongside the existing owner/accountant tests so the Upload PDF visibility is covered for owners, admins, and accountants.lib/ksef_hub_web/controllers/api/category_controller.ex (1)
18-19: Good use of theRequirePermissionplug for centralizing write operation authorization.The OpenAPI specs for
create,update, anddeleteactions should include a403response to document the permission enforcement:📝 Suggested addition to operation specs
Add this response to the
:create,:update, and:deleteoperation specs:403 => {"Forbidden — insufficient permissions", "application/json", Schemas.ErrorResponse}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/api/category_controller.ex` around lines 18 - 19, Update the OpenAPI operation specs for the controller actions create/2, update/2, and delete/2 to include a 403 response documenting permission enforcement; specifically add a response entry like {"Forbidden — insufficient permissions", "application/json", Schemas.ErrorResponse} to each of the operation specs associated with the create, update, and delete actions in category_controller (the actions that are protected by the RequirePermission plug).test/ksef_hub_web/controllers/api/tag_controller_test.exs (1)
147-187: LGTM!Good test coverage for the new permission enforcement on tag operations. The tests correctly verify that:
accountantandreviewercan read tags (index)accountantandreviewercannot create tags (403)admincan create tags (201)Consider adding permission tests for
updateanddeleteactions to ensure complete coverage of the:manage_tagspermission across all write operations. This could be deferred if theRequirePermissionplug is thoroughly tested elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/tag_controller_test.exs` around lines 147 - 187, Add tests for update and delete to fully exercise the :manage_tags permission: for each action insert a tag (using insert(:tag, company: company, name: "test") or similar), then call create_accountant_with_token, create_reviewer_with_token, and create_admin_with_token and exercise the endpoints (PATCH or PUT to "/api/tags/:id" for update and DELETE to "/api/tags/:id" for delete) using api_conn(token) with appropriate JSON bodies for update; assert 403 for accountant and reviewer and a success status (e.g., 200/204 for delete, 200/201 for update depending on controller behavior) for admin, and add these tests alongside the existing "permission enforcement" describe block referencing the existing helper functions create_accountant_with_token, create_reviewer_with_token, create_admin_with_token and the "/api/tags" routes.test/ksef_hub_web/controllers/api/category_controller_test.exs (1)
168-216: Add one permission case forupdate.This block covers
index,create, anddelete, butupdateis the remaining mutating endpoint behind the new permission gate. One 403/200 case there would close the controller-level gap.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/category_controller_test.exs` around lines 168 - 216, Add tests covering the update permission: mirror existing patterns by adding one test that a non-privileged role (e.g., create_accountant_with_token()) receives 403 when calling the update endpoint and one test that an admin (create_admin_with_token()) can update and gets a success status. Locate the category setup using insert(:category, company: company) and send a JSON body (e.g., Jason.encode!(%{name: "ops:updated"})) via patch("/api/categories/#{category.id}", body) or put(...) through api_conn(token); assert conn.status == 403 for accountant and conn.status in the expected success code (200/204) for admin to close the controller-level gap.lib/ksef_hub_web/live/company_live/index.ex (1)
28-38: Consider hiding the "New Company" link for unauthorized users.The authorization check correctly prevents unauthorized users from creating companies, but the "New Company" link (line 249) is still visible to all users. This creates a confusing UX where users click a link only to be immediately redirected with an error.
🔧 Proposed fix to conditionally show the link
Add
can_manage_companyassign inmount/3:def mount(_params, _session, socket) do {:ok, socket |> assign(page_title: "Companies") + |> assign(can_manage_company: Authorization.can?(socket.assigns[:current_role], :manage_company)) |> load_companies()} endThen in the template:
<:actions> - <.link navigate={~p"/companies/new"} class="btn btn-primary btn-sm"> + <.link :if={`@can_manage_company`} navigate={~p"/companies/new"} class="btn btn-primary btn-sm"> <.icon name="hero-plus" class="size-4" /> New Company </.link> </:actions>Similarly, hide the Edit action for unauthorized users at line 423.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/company_live/index.ex` around lines 28 - 38, Add a boolean assign (e.g., :can_manage_company) in mount/3 using Authorization.can?(socket.assigns[:current_role], :manage_company) and ensure it is assigned alongside :company and related assigns; then update the LiveView template to conditionally render the "New Company" link and the Edit action based on that assign (use the :can_manage_company assign to hide those links for unauthorized users). This ensures mount/3 (and any helper functions that build assigns) exposes can_manage_company and the template checks that assign before showing the New Company link and the Edit action.lib/ksef_hub_web/plugs/require_permission.ex (1)
1-34: LGTM with minor documentation suggestion.The plug correctly implements the authorization pattern with proper halting. The
role && Authorization.can?(role, permission)guard handles nil roles safely.Consider clarifying in the
@moduledocthat this plug is intended for API controllers (since it returns JSON), to prevent accidental use in browser pipelines where an HTML response or redirect would be more appropriate.📝 Suggested documentation update
`@moduledoc` """ - Plug that checks `Authorization.can?/2` against the current role. + Plug that checks `Authorization.can?/2` against the current role in API controllers. Returns 403 Forbidden if the user's role does not have the required permission. + Note: This plug returns a JSON response and is intended for use in `:api` pipelines. + For browser routes, authorization should be handled in LiveView mounts or with redirects. + ## Usage plug KsefHubWeb.Plugs.RequirePermission, :create_invoice when action in [:create, :upload] """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/plugs/require_permission.ex` around lines 1 - 34, Update the module documentation for KsefHubWeb.Plugs.RequirePermission to explicitly state that this plug returns a JSON 403 response and is intended for API controllers/pipelines (not browser pipelines that expect HTML or redirects); mention that it uses conn.assigns[:current_role] and Authorization.can?/2 and that callers should ensure current_role is set or handle nil roles appropriately so it isn't accidentally used in HTML/browser contexts.
🤖 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 23-30: The OpenAPI operation specs for this controller (see
operation/2) are missing 403 responses introduced by the new RequirePermission
plugs; update the operation/2 specs for actions create, upload, update,
confirm_duplicate, dismiss_duplicate, approve, reject, set_category, and
set_tags to include a 403 response (forbidden) with the appropriate error schema
so generated docs/clients reflect permission-denied branches; ensure each
operation/2 clause that currently lists only 401/404/422 is amended to include
403 and matches the existing error response structure used elsewhere in the
project.
In `@lib/ksef_hub_web/controllers/api/tag_controller.ex`:
- Line 18: The controller now uses KsefHubWeb.Plugs.RequirePermission with
:manage_tags for actions :create, :update, and :delete, so update the OpenAPI
operation/2 specs for those actions to include a 403 Forbidden response; modify
the operation/2 clauses handling :create, :update, and :delete to add a 403
response entry (e.g. description "Forbidden" and the existing error schema used
for other errors) so the open_api_spex contract matches the controller behavior.
In `@lib/ksef_hub_web/live/sync_live.ex`:
- Around line 17-24: Add an authorization check in the "trigger_sync" event
handler: before calling History.trigger_manual_sync/1 (in handle_event/3 for the
"trigger_sync" event), revalidate
Authorization.can?(socket.assigns[:current_role], :trigger_sync) and if it fails
return an appropriate {:noreply, socket} response (put_flash error and redirect
or halt further work) instead of proceeding to call
History.trigger_manual_sync/1; update the handler to use
socket.assigns[:current_role] for the check and to avoid side effects when
unauthorized.
---
Outside diff comments:
In `@lib/ksef_hub_web/live/export_live/index.ex`:
- Around line 17-23: Add an authorization check inside the LiveView event
handler that processes "export" so it doesn't rely solely on mount/3; in the
handle_event("export", params, socket) path, call
Authorization.can?(socket.assigns[:current_role], :view_exports) and if it
returns false return {:noreply, socket |> put_flash(:error, "You don't have
permission to access exports.") |> redirect(to:
~p"/c/#{socket.assigns.current_company.id}/invoices")} otherwise proceed to call
do_export/2 with the socket and params; ensure you reference
socket.assigns[:current_role], socket.assigns.current_company.id, the "export"
event handler, and do_export/2 so the guard is enforced at runtime.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 286-292: Add a server-side authorization guard to all mutation
LiveView handlers (e.g., toggle_edit, save_edit, set_category, toggle_tag,
create_and_add_tag, approve, reject, dismiss_duplicate, confirm_duplicate,
re_extract) by checking the existing authorization flag or function (e.g.,
socket.assigns.can_mutate or a can_mutate?/1 helper) at the top of each handler;
if the check fails, return early (e.g., {:noreply, socket} or redirect/put_flash
as appropriate) so no state changes or context calls (like build_edit_form,
invoice updates, approve/reject actions) are performed without authorization.
Ensure the guard is applied before any assignments, calls to context functions,
or side effects in those handler functions.
In `@lib/ksef_hub_web/live/invoice_live/upload.ex`:
- Around line 36-42: The "upload" event handler must re-check permissions at
event time: inside the handle_event clause for "upload" (where it currently
calls do_upload/3), call Authorization.can?(socket.assigns[:current_role],
:create_invoice) and if it returns false, respond like in mount/3 (put_flash
:error "You do not have permission to upload invoices." and redirect) instead of
proceeding; alternatively ensure do_upload/3 accepts and validates the acting
user/role (pass socket.assigns[:current_role] or current_user into do_upload/3
and perform the same Authorization.can? check there) so uploads cannot proceed
after a role change.
---
Nitpick comments:
In `@lib/ksef_hub_web/controllers/api/category_controller.ex`:
- Around line 18-19: Update the OpenAPI operation specs for the controller
actions create/2, update/2, and delete/2 to include a 403 response documenting
permission enforcement; specifically add a response entry like {"Forbidden —
insufficient permissions", "application/json", Schemas.ErrorResponse} to each of
the operation specs associated with the create, update, and delete actions in
category_controller (the actions that are protected by the RequirePermission
plug).
In `@lib/ksef_hub_web/live/company_live/index.ex`:
- Around line 28-38: Add a boolean assign (e.g., :can_manage_company) in mount/3
using Authorization.can?(socket.assigns[:current_role], :manage_company) and
ensure it is assigned alongside :company and related assigns; then update the
LiveView template to conditionally render the "New Company" link and the Edit
action based on that assign (use the :can_manage_company assign to hide those
links for unauthorized users). This ensures mount/3 (and any helper functions
that build assigns) exposes can_manage_company and the template checks that
assign before showing the New Company link and the Edit action.
In `@lib/ksef_hub_web/plugs/require_permission.ex`:
- Around line 1-34: Update the module documentation for
KsefHubWeb.Plugs.RequirePermission to explicitly state that this plug returns a
JSON 403 response and is intended for API controllers/pipelines (not browser
pipelines that expect HTML or redirects); mention that it uses
conn.assigns[:current_role] and Authorization.can?/2 and that callers should
ensure current_role is set or handle nil roles appropriately so it isn't
accidentally used in HTML/browser contexts.
In `@test/ksef_hub_web/controllers/api/category_controller_test.exs`:
- Around line 168-216: Add tests covering the update permission: mirror existing
patterns by adding one test that a non-privileged role (e.g.,
create_accountant_with_token()) receives 403 when calling the update endpoint
and one test that an admin (create_admin_with_token()) can update and gets a
success status. Locate the category setup using insert(:category, company:
company) and send a JSON body (e.g., Jason.encode!(%{name: "ops:updated"})) via
patch("/api/categories/#{category.id}", body) or put(...) through
api_conn(token); assert conn.status == 403 for accountant and conn.status in the
expected success code (200/204) for admin to close the controller-level gap.
In `@test/ksef_hub_web/controllers/api/tag_controller_test.exs`:
- Around line 147-187: Add tests for update and delete to fully exercise the
:manage_tags permission: for each action insert a tag (using insert(:tag,
company: company, name: "test") or similar), then call
create_accountant_with_token, create_reviewer_with_token, and
create_admin_with_token and exercise the endpoints (PATCH or PUT to
"/api/tags/:id" for update and DELETE to "/api/tags/:id" for delete) using
api_conn(token) with appropriate JSON bodies for update; assert 403 for
accountant and reviewer and a success status (e.g., 200/204 for delete, 200/201
for update depending on controller behavior) for admin, and add these tests
alongside the existing "permission enforcement" describe block referencing the
existing helper functions create_accountant_with_token,
create_reviewer_with_token, create_admin_with_token and the "/api/tags" routes.
In `@test/ksef_hub_web/live/invoice_live/upload_test.exs`:
- Around line 168-192: Add a new test mirroring the owner test to ensure admins
see the Upload PDF button: create an admin user via
Accounts.get_or_create_google_user, create a company and membership with role:
:admin using insert(:membership), sign in with build_conn() |>
log_in_user(admin, %{current_company_id: company.id}), call live(conn,
~p"/c/#{company.id}/invoices") and assert has_element?(view,
~s(a[href="#{upload_path}"]), "Upload PDF"); place this alongside the existing
owner/accountant tests so the Upload PDF visibility is covered for owners,
admins, and accountants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5a8f7fd-0334-4a7f-bcb1-3e3051731920
📒 Files selected for processing (38)
docs/adr/0029-centralized-authorization-and-role-refactor.mddocs/roles.mdlib/ksef_hub/accounts.exlib/ksef_hub/authorization.exlib/ksef_hub/companies/membership.exlib/ksef_hub/invitations.exlib/ksef_hub/invitations/invitation.exlib/ksef_hub_web/components/layouts.exlib/ksef_hub_web/controllers/api/category_controller.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/controllers/api/tag_controller.exlib/ksef_hub_web/controllers/api/token_controller.exlib/ksef_hub_web/controllers/export_controller.exlib/ksef_hub_web/live/company_live/index.exlib/ksef_hub_web/live/dashboard_live.exlib/ksef_hub_web/live/export_live/index.exlib/ksef_hub_web/live/invoice_live/index.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/invoice_live/upload.exlib/ksef_hub_web/live/live_auth.exlib/ksef_hub_web/live/sync_live.exlib/ksef_hub_web/live/token_live.exlib/ksef_hub_web/plugs/require_permission.exlib/ksef_hub_web/router.extest/ksef_hub/accounts_test.exstest/ksef_hub/authorization_test.exstest/ksef_hub/companies/membership_test.exstest/ksef_hub_web/controllers/api/category_controller_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/ksef_hub_web/controllers/api/tag_controller_test.exstest/ksef_hub_web/controllers/api/token_controller_test.exstest/ksef_hub_web/live/invoice_live/upload_test.exstest/ksef_hub_web/live/live_auth_test.exstest/ksef_hub_web/live/role_based_nav_test.exstest/ksef_hub_web/live/team_live_test.exstest/ksef_hub_web/live/token_live_test.exstest/ksef_hub_web/plugs/require_permission_test.exstest/support/api_test_helpers.ex
- Add 403 response to OpenAPI specs for all permission-protected endpoints (invoice, tag, category controllers) - Add server-side authorization guards to LiveView event handlers (sync, export, invoice show/upload, company index) - Use pattern-matching guard clause for invoice show mutation events - Add tests for admin/accountant/reviewer permission enforcement - Update RequirePermission plug documentation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub_web/live/export_live/index.ex (1)
17-23:⚠️ Potential issue | 🟡 MinorHandle
current_company == nilbefore these redirects.
do_mount/1and"preview"already treat a missing company as possible, but these new forbidden branches callsocket.assigns.current_company.idunconditionally. In that edge case the auth check will crash instead of redirecting cleanly.Also applies to: 98-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/export_live/index.ex` around lines 17 - 23, The forbidden branches call socket.assigns.current_company.id unconditionally and will crash when current_company is nil; update the Authorization.can? guard branches (the mount block using Authorization.can? and the similar "preview" branch) to first fetch current_company = socket.assigns[:current_company] and handle the nil case (e.g., put_flash/redirect to a safe fallback such as the companies index or root) before attempting to access .id, otherwise proceed with the existing redirect that uses current_company.id or call do_mount(socket) as appropriate; ensure you update both the Authorization.can? else branch and the preview else branch to use the same nil-safe logic.
🧹 Nitpick comments (3)
lib/ksef_hub_web/plugs/require_permission.ex (1)
22-27: Document the exported Plug callbacks.
init/1andcall/2are public functions inlib/, but both are marked@doc false.As per coding guidelines, "Every public function must have
@docdocumentation in Elixir".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/plugs/require_permission.ex` around lines 22 - 27, Add public documentation for the Plug callbacks init/1 and call/2 in the RequirePermission plug module: replace `@doc` false with short `@doc` strings describing the purpose and expected arguments/return values (e.g., init(permission) — initializes and returns the permission option; call(conn, permission) — enforces the permission on the connection and returns the conn or halts). Ensure the docs mention that these implement the Plug behaviour and briefly state error/halting behavior so readers know what to expect from init/1 and call/2.test/ksef_hub_web/live/invoice_live/upload_test.exs (2)
176-192: Exercise the admin route, not just the button.This proves the index renders the link, but it doesn’t prove admin can actually open
/invoices/uploador submit there. Since the PR centralizes permissions, I’d add at least onelive/2assertion for the upload page as admin so UI and route checks can’t drift. 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/live/invoice_live/upload_test.exs` around lines 176 - 192, Add a live/2 assertion that actually navigates to the admin upload route (use the existing upload_path or call live(conn, ~p"/c/#{company.id}/invoices/upload") after the index view) and assert the upload page renders expected elements (e.g., the upload form or a file input) so the test verifies the route and page render, not just the presence of the link; update the "shows Upload PDF button for admin" test (or add a new test) to call live(conn, upload_path) and assert on the returned view/html for the upload page.
47-64: Please cover the new submit-time permission check too.This only exercises the mount redirect. The new guard in
handle_event("upload")is still untested, so a drift between mount-time and submit-time permissions would slip through. I’d add a case that mounts as an allowed role, downgrades/revokes access, and then asserts the submit is rejected. 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/live/invoice_live/upload_test.exs` around lines 47 - 64, Add an integration test that exercises the submit-time permission guard in handle_event("upload"): mount the Upload live view as an allowed user (reuse Accounts.get_or_create_google_user and live(conn, ~p"/c/#{company.id}/invoices/upload")), then change the user's membership/role (e.g., update or delete the inserted :membership to revoke accountant access) and invoke the upload submit (use render_submit or send the "upload" event to the live view) and assert the submit is rejected/redirected the same way as mount (expect {:error, {:redirect, %{to: ^expected_path}}} or the appropriate response). Ensure the test references the same company, accountant, and membership setup so it specifically verifies the handle_event("upload") guard path.
🤖 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/company_live/index.ex`:
- Around line 16-19: The LiveView currently only checks manage_company in
apply_action/3, but mutating event handlers (e.g., save_company/3 and any
inbound-email handlers) still call the context layer and can be invoked by
crafted events; update each mutating handler (save_company/3, handle_event
callbacks that create/update/delete companies, and any inbound-email handling
functions) to re-check Authorization.can?(socket.assigns[:current_role],
:manage_company) at the start and return an unauthorized response
(halt/redirect/put_flash as appropriate) when the check fails, so all
state-changing operations are guarded server-side rather than relying solely on
apply_action/3.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 84-95: The authorization guard misses note-editing events: add
"edit_note" and "save_note" to the `@mutation_events` list so handle_event/3's
clause that checks %{assigns: %{can_mutate: false}} will also block those
mutations; update the `@mutation_events` definition (and any related
pattern-matched clauses using that list) so note edits cannot be handled when
can_mutate is false, ensuring all state-changing LiveView events like
edit_note/save_note are validated by the existing guard.
- Around line 55-69: The LiveView currently computes a single can_mutate =
Authorization.can?(role, :update_invoice); replace that with distinct permission
assigns—e.g. can_approve = Authorization.can?(role, :approve_invoice),
can_set_category = Authorization.can?(role, :set_invoice_category), can_set_tags
= Authorization.can?(role, :set_invoice_tags) and (if needed) can_manage_tags =
Authorization.can?(role, :manage_tags)—by adding those to the socket assign
block where can_mutate is set and removing can_mutate; then update any event
handlers and UI gates that previously checked can_mutate (handlers that perform
approvals, category changes, tag updates, and tag management) to check the
corresponding new flag (can_approve, can_set_category, can_set_tags,
can_manage_tags) so the LiveView permission checks mirror the API authorization
model.
---
Outside diff comments:
In `@lib/ksef_hub_web/live/export_live/index.ex`:
- Around line 17-23: The forbidden branches call
socket.assigns.current_company.id unconditionally and will crash when
current_company is nil; update the Authorization.can? guard branches (the mount
block using Authorization.can? and the similar "preview" branch) to first fetch
current_company = socket.assigns[:current_company] and handle the nil case
(e.g., put_flash/redirect to a safe fallback such as the companies index or
root) before attempting to access .id, otherwise proceed with the existing
redirect that uses current_company.id or call do_mount(socket) as appropriate;
ensure you update both the Authorization.can? else branch and the preview else
branch to use the same nil-safe logic.
---
Nitpick comments:
In `@lib/ksef_hub_web/plugs/require_permission.ex`:
- Around line 22-27: Add public documentation for the Plug callbacks init/1 and
call/2 in the RequirePermission plug module: replace `@doc` false with short `@doc`
strings describing the purpose and expected arguments/return values (e.g.,
init(permission) — initializes and returns the permission option; call(conn,
permission) — enforces the permission on the connection and returns the conn or
halts). Ensure the docs mention that these implement the Plug behaviour and
briefly state error/halting behavior so readers know what to expect from init/1
and call/2.
In `@test/ksef_hub_web/live/invoice_live/upload_test.exs`:
- Around line 176-192: Add a live/2 assertion that actually navigates to the
admin upload route (use the existing upload_path or call live(conn,
~p"/c/#{company.id}/invoices/upload") after the index view) and assert the
upload page renders expected elements (e.g., the upload form or a file input) so
the test verifies the route and page render, not just the presence of the link;
update the "shows Upload PDF button for admin" test (or add a new test) to call
live(conn, upload_path) and assert on the returned view/html for the upload
page.
- Around line 47-64: Add an integration test that exercises the submit-time
permission guard in handle_event("upload"): mount the Upload live view as an
allowed user (reuse Accounts.get_or_create_google_user and live(conn,
~p"/c/#{company.id}/invoices/upload")), then change the user's membership/role
(e.g., update or delete the inserted :membership to revoke accountant access)
and invoke the upload submit (use render_submit or send the "upload" event to
the live view) and assert the submit is rejected/redirected the same way as
mount (expect {:error, {:redirect, %{to: ^expected_path}}} or the appropriate
response). Ensure the test references the same company, accountant, and
membership setup so it specifically verifies the handle_event("upload") guard
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea41e45a-9e9d-40c5-bc00-4afbfe0c2684
📒 Files selected for processing (12)
lib/ksef_hub_web/controllers/api/category_controller.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/controllers/api/tag_controller.exlib/ksef_hub_web/live/company_live/index.exlib/ksef_hub_web/live/export_live/index.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/invoice_live/upload.exlib/ksef_hub_web/live/sync_live.exlib/ksef_hub_web/plugs/require_permission.extest/ksef_hub_web/controllers/api/category_controller_test.exstest/ksef_hub_web/controllers/api/tag_controller_test.exstest/ksef_hub_web/live/invoice_live/upload_test.exs
🚧 Files skipped from review as they are similar to previous changes (2)
- test/ksef_hub_web/controllers/api/category_controller_test.exs
- lib/ksef_hub_web/live/sync_live.ex
Comment on lines
+84
to
+95
| # --- Authorization guard --- | ||
| # Catch-all for mutation events when the user lacks permission. | ||
|
|
||
| @mutation_events ~w(re_extract approve reject dismiss_duplicate confirm_duplicate | ||
| set_category toggle_tag create_and_add_tag toggle_edit save_edit) | ||
|
|
||
| @impl true | ||
| @spec handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: | ||
| {:noreply, Phoenix.LiveView.Socket.t()} | ||
| def handle_event(event, _params, %{assigns: %{can_mutate: false}} = socket) | ||
| when event in @mutation_events do | ||
| {:noreply, put_flash(socket, :error, "You don't have permission to modify this invoice.")} |
There was a problem hiding this comment.
Add note edits to the new mutation guard.
edit_note and save_note still mutate invoice.note, but they are outside @mutation_events and the note controls remain available when @can_mutate is false. That leaves a write path open for read-only roles.
Based on learnings, "Check authorization in controllers and LiveViews" and "Validate all state changes in LiveView events".
🤖 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 84 - 95, The
authorization guard misses note-editing events: add "edit_note" and "save_note"
to the `@mutation_events` list so handle_event/3's clause that checks %{assigns:
%{can_mutate: false}} will also block those mutations; update the
`@mutation_events` definition (and any related pattern-matched clauses using that
list) so note edits cannot be handled when can_mutate is false, ensuring all
state-changing LiveView events like edit_note/save_note are validated by the
existing guard.
- Authorization: set-based permissions (MapSet) replace per-clause functions;
add can?/3 (user_id, company_id, permission) to centralize membership lookup
- LiveAuth: replace :require_owner/:require_admin with generic
{:require_permission, perm} on_mount hook
- Router: move upload, tokens, syncs, exports into permission-gated live_sessions
so auth is enforced at the routing layer, not inside each LiveView mount
- LiveViews: strip redundant inline Authorization.can? checks from mount/3 in
SyncLive, ExportLive, TokenLive, InvoiceLive.Upload (now router-enforced)
- Layouts: replace 12 repeated nav <li> blocks with data-driven nav_items/2
- Accounts: remove private authorized_for?/3, use Authorization.can?/3 directly
- ApiTestHelpers: unify 4 near-identical create_*_with_token fns into single
create_user_with_token/2 dispatching on role atom (~60 lines removed)
Net: -144 lines, fewer copy-paste patterns, single source of truth for auth.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CompanyLive.Index: add @company_mutation_events guard that checks can_manage_company before all mutating handlers (save, inbound email ops) - InvoiceLive.Show: add edit_note/save_note to @mutation_events so note edits are blocked when can_mutate is false; add fine-grained permission assigns (can_approve, can_set_category, can_set_tags) with separate handler guards matching API-level authorization model - ExportLive.Index: fix nil crash — check current_company before accessing .id in the export handler's authorization branch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub_web/live/token_live.ex (1)
61-62:⚠️ Potential issue | 🟡 MinorInconsistent error message after permission model change.
The error message says "Only company owners can create tokens" but according to the updated
@moduledocand the centralizedAuthorization.can?(:manage_tokens)check, owners, admins, and accountants can all create tokens. This message should reflect the new permission model.📝 Suggested fix
{:error, :unauthorized} -> - {:noreply, put_flash(socket, :error, "Only company owners can create tokens.")} + {:noreply, put_flash(socket, :error, "You don't have permission to create tokens.")}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/token_live.ex` around lines 61 - 62, Update the unauthorized flash in the handle_info/handle_event clause that matches {:error, :unauthorized} (in lib/ksef_hub_web/live/token_live.ex) so it reflects the current permission model used by Authorization.can?(:manage_tokens); replace "Only company owners can create tokens." with a neutral/inclusive message such as "Only company owners, admins, or accountants can create tokens." to match the `@moduledoc` and centralized authorization check.
♻️ Duplicate comments (1)
lib/ksef_hub_web/live/invoice_live/show.ex (1)
55-58:⚠️ Potential issue | 🟠 Major
create_and_add_tagis still authorized by the wrong permission.
create_and_add_tagcreates a company tag, but it is grouped under@tag_eventsand guarded only by@can_set_tags.lib/ksef_hub/authorization.exLines 43-53 grants reviewers:set_invoice_tagsbut not:manage_tags, so reviewers can still create new tags from this page.Based on learnings, "Check authorization in controllers and LiveViews" and "Validate all state changes in LiveView events".Possible split
- can_set_tags = Authorization.can?(role, :set_invoice_tags) + can_set_tags = Authorization.can?(role, :set_invoice_tags) + can_manage_tags = Authorization.can?(role, :manage_tags) {:ok, socket |> assign( page_title: "Invoice #{invoice.invoice_number}", invoice: invoice, can_mutate: can_mutate, can_approve: can_approve, can_set_category: can_set_category, can_set_tags: can_set_tags, + can_manage_tags: can_manage_tags, html_preview: generate_preview(invoice), ... )} - `@tag_events` ~w(toggle_tag create_and_add_tag) + `@tag_events` ~w(toggle_tag) + `@manage_tag_events` ~w(create_and_add_tag) def handle_event(event, _params, %{assigns: %{can_set_tags: false}} = socket) when event in `@tag_events` do {:noreply, put_flash(socket, :error, "You don't have permission to manage invoice tags.")} end + def handle_event(event, _params, %{assigns: %{can_manage_tags: false}} = socket) + when event in `@manage_tag_events` do + {:noreply, put_flash(socket, :error, "You don't have permission to create tags.")} + end + - <.form - :if={`@can_mutate`} + <.form + :if={`@can_manage_tags`} for={`@new_tag_form`} phx-submit="create_and_add_tag"Also applies to: 97-98, 119-121, 987-1003
🤖 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 55 - 58, The create_and_add_tag event is currently protected only by `@can_set_tags` (via `@tag_events`) but must require the :manage_tags permission; update the authorization so that the LiveView checks Authorization.can?(role, :manage_tags) for create_and_add_tag (both where `@tag_events` are defined and in the handle_event clause that processes "create_and_add_tag"), and add server-side gate logic inside the handle_event/"create_and_add_tag" handler to reject unauthorized attempts. Also apply the same change to the other tag-creation spots you flagged (the other create/add tag handlers referenced) so all create-and-add-tag flows require :manage_tags rather than :set_invoice_tags.
🧹 Nitpick comments (2)
test/ksef_hub_web/controllers/api/category_controller_test.exs (1)
168-234: Missing reviewer update/delete tests for consistency.The permission enforcement tests cover accountant and reviewer for read access, but only accountant for update/delete operations. For consistency with
tag_controller_test.exs(which tests both accountant and reviewer for all operations), consider adding:
- "reviewer cannot update categories"
- "reviewer cannot delete categories"
📝 Suggested additional tests
test "reviewer cannot update categories", %{conn: conn} do {:ok, %{company: company, token: token}} = create_user_with_token(:reviewer) category = insert(:category, company: company) body = Jason.encode!(%{name: "ops:updated"}) conn = conn |> api_conn(token) |> patch("/api/categories/#{category.id}", body) assert conn.status == 403 end test "reviewer cannot delete categories", %{conn: conn} do {:ok, %{company: company, token: token}} = create_user_with_token(:reviewer) category = insert(:category, company: company) conn = conn |> api_conn(token) |> delete("/api/categories/#{category.id}") assert conn.status == 403 end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/category_controller_test.exs` around lines 168 - 234, Add two tests mirroring the accountant update/delete cases for reviewer: add "reviewer cannot update categories" and "reviewer cannot delete categories" that call create_user_with_token(:reviewer), insert(:category, company: company), build the JSON body with Jason.encode!(%{name: "ops:updated"}) for the update case, and use api_conn(token) |> patch("/api/categories/#{category.id}", body) asserting status 403; and use api_conn(token) |> delete("/api/categories/#{category.id}") asserting status 403 for the delete case, matching the style of the existing tests that use create_user_with_token, insert(:category), api_conn, patch/delete and status assertions.lib/ksef_hub_web/router.ex (1)
137-145:admin_onlycollapses three distinct permissions back into:manage_team.
lib/ksef_hub/authorization.exLines 30-39andlib/ksef_hub_web/components/layouts.exLines 152-160 already treat certificates, categories, tags, and team as separate capabilities. Requiring:manage_team` for all four routes reintroduces a coarse gate and will drift from the centralized matrix the next time the role policy changes. Split these routes by their matching permission.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/router.ex` around lines 137 - 145, The admin_only live_session currently applies a single permission {:require_permission, :manage_team} to CertificateLive, CategoryLive, TagLive, and TeamLive; split the routes so each live view uses its specific permission instead of reusing :manage_team—either create separate live_session blocks or change the on_mount entry per route to call KsefHubWeb.LiveAuth with {:require_permission, :manage_certificates}, {:require_permission, :manage_categories}, {:require_permission, :manage_tags}, and {:require_permission, :manage_team} respectively (references: live_session :admin_only, KsefHubWeb.LiveAuth, {:require_permission, ...}, CertificateLive, CategoryLive, TagLive, TeamLive).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/ksef_hub_web/live/token_live.ex`:
- Around line 61-62: Update the unauthorized flash in the
handle_info/handle_event clause that matches {:error, :unauthorized} (in
lib/ksef_hub_web/live/token_live.ex) so it reflects the current permission model
used by Authorization.can?(:manage_tokens); replace "Only company owners can
create tokens." with a neutral/inclusive message such as "Only company owners,
admins, or accountants can create tokens." to match the `@moduledoc` and
centralized authorization check.
---
Duplicate comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 55-58: The create_and_add_tag event is currently protected only by
`@can_set_tags` (via `@tag_events`) but must require the :manage_tags permission;
update the authorization so that the LiveView checks Authorization.can?(role,
:manage_tags) for create_and_add_tag (both where `@tag_events` are defined and in
the handle_event clause that processes "create_and_add_tag"), and add
server-side gate logic inside the handle_event/"create_and_add_tag" handler to
reject unauthorized attempts. Also apply the same change to the other
tag-creation spots you flagged (the other create/add tag handlers referenced) so
all create-and-add-tag flows require :manage_tags rather than :set_invoice_tags.
---
Nitpick comments:
In `@lib/ksef_hub_web/router.ex`:
- Around line 137-145: The admin_only live_session currently applies a single
permission {:require_permission, :manage_team} to CertificateLive, CategoryLive,
TagLive, and TeamLive; split the routes so each live view uses its specific
permission instead of reusing :manage_team—either create separate live_session
blocks or change the on_mount entry per route to call KsefHubWeb.LiveAuth with
{:require_permission, :manage_certificates}, {:require_permission,
:manage_categories}, {:require_permission, :manage_tags}, and
{:require_permission, :manage_team} respectively (references: live_session
:admin_only, KsefHubWeb.LiveAuth, {:require_permission, ...}, CertificateLive,
CategoryLive, TagLive, TeamLive).
In `@test/ksef_hub_web/controllers/api/category_controller_test.exs`:
- Around line 168-234: Add two tests mirroring the accountant update/delete
cases for reviewer: add "reviewer cannot update categories" and "reviewer cannot
delete categories" that call create_user_with_token(:reviewer),
insert(:category, company: company), build the JSON body with
Jason.encode!(%{name: "ops:updated"}) for the update case, and use
api_conn(token) |> patch("/api/categories/#{category.id}", body) asserting
status 403; and use api_conn(token) |> delete("/api/categories/#{category.id}")
asserting status 403 for the delete case, matching the style of the existing
tests that use create_user_with_token, insert(:category), api_conn, patch/delete
and status assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31ddc61b-0383-4bcf-a725-cca14cdb0b9e
📒 Files selected for processing (19)
lib/ksef_hub/accounts.exlib/ksef_hub/authorization.exlib/ksef_hub_web/components/layouts.exlib/ksef_hub_web/live/company_live/index.exlib/ksef_hub_web/live/export_live/index.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/invoice_live/upload.exlib/ksef_hub_web/live/live_auth.exlib/ksef_hub_web/live/sync_live.exlib/ksef_hub_web/live/team_live.exlib/ksef_hub_web/live/token_live.exlib/ksef_hub_web/router.extest/ksef_hub_web/controllers/api/category_controller_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/ksef_hub_web/controllers/api/tag_controller_test.exstest/ksef_hub_web/controllers/api/token_controller_test.exstest/ksef_hub_web/live/token_live_test.exstest/ksef_hub_web/plugs/api_auth_test.exstest/support/api_test_helpers.ex
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/ksef_hub_web/live/invoice_live/upload.ex
- lib/ksef_hub_web/live/sync_live.ex
- test/ksef_hub_web/live/token_live_test.exs
- Update token_live flash to use neutral message matching permission model - Require :manage_tags (not just :set_invoice_tags) for create_and_add_tag - Split admin_only live_session into per-route permission checks - Add missing reviewer authorization tests for category update/delete Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/ksef_hub_web/live/invoice_live/show.ex (1)
732-748:⚠️ Potential issue | 🟠 MajorUI permission gates still misaligned with backend authorization guards.
The backend guards correctly check individual permissions (
can_approve,can_set_category,can_set_tags,can_manage_tags), but the UI still gates these actions with@can_mutate:
UI Element Current Gate Should Use Approve/Reject buttons (lines 732, 742) @can_mutate@can_approveCategory select (lines 954, 963) @can_mutate@can_set_categoryTag checkboxes (lines 987, 989) @can_mutate@can_set_tagsNew tag form (line 996) @can_mutate@can_manage_tags🔧 Suggested fixes
# Lines 732-733, 742-743: Approve/Reject buttons - :if={ - `@can_mutate` && `@invoice.type` == :expense && `@invoice.status` == :pending && + :if={ + `@can_approve` && `@invoice.type` == :expense && `@invoice.status` == :pending && `@invoice.duplicate_status` != :confirmed }# Line 954: Category form - phx-change={if(`@can_mutate`, do: "set_category")} + phx-change={if(`@can_set_category`, do: "set_category")}# Line 963: Category select disabled - disabled={not `@can_mutate`} + disabled={not `@can_set_category`}# Lines 987, 989: Tag checkboxes - phx-click={if(`@can_mutate`, do: "toggle_tag")} + phx-click={if(`@can_set_tags`, do: "toggle_tag")} phx-value-tag-id={tag.id} - disabled={not `@can_mutate`} + disabled={not `@can_set_tags`}# Line 996: New tag form - :if={`@can_mutate`} + :if={`@can_manage_tags`},
Also applies to: 954-996
🤖 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 732 - 748, The UI permission gates use the generic `@can_mutate` but must match backend guards; update the :if conditions for each control to replace `@can_mutate` with the specific assigns while preserving the rest of the predicates (invoice.type, invoice.status, duplicate_status): use `@can_approve` for the Approve/Reject buttons, `@can_set_category` for the category select, `@can_set_tags` for tag checkboxes, and `@can_manage_tags` for the new-tag form so the template's visibility matches the backend guards.
🧹 Nitpick comments (1)
test/ksef_hub_web/controllers/api/category_controller_test.exs (1)
201-249: Add the missing admin delete case.The new matrix proves admin can create and update categories, but it never exercises delete. That leaves one of the newly centralized write permissions uncovered, so a missing admin delete grant could slip through.
Suggested test
+ test "admin can delete categories", %{conn: conn} do + {:ok, %{company: company, token: token}} = create_user_with_token(:admin) + category = insert(:category, company: company) + + conn = conn |> api_conn(token) |> delete("/api/categories/#{category.id}") + assert conn.status == 200 + endBased on learnings: "Test controller actions and context interactions using integration tests" and "Check authorization in controllers and LiveViews".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/category_controller_test.exs` around lines 201 - 249, Add an integration test "admin can delete categories" that mirrors the other role tests: call create_user_with_token(:admin) to get company and token, insert a category with insert(:category, company: company), perform delete using api_conn(token) |> delete("/api/categories/#{category.id}"), and assert the response status is 204 (successfully deleted). Ensure the test follows the same setup style as "admin can create categories" and "admin can update categories" so the controller delete authorization is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 732-748: The UI permission gates use the generic `@can_mutate` but
must match backend guards; update the :if conditions for each control to replace
`@can_mutate` with the specific assigns while preserving the rest of the
predicates (invoice.type, invoice.status, duplicate_status): use `@can_approve`
for the Approve/Reject buttons, `@can_set_category` for the category select,
`@can_set_tags` for tag checkboxes, and `@can_manage_tags` for the new-tag form so
the template's visibility matches the backend guards.
---
Nitpick comments:
In `@test/ksef_hub_web/controllers/api/category_controller_test.exs`:
- Around line 201-249: Add an integration test "admin can delete categories"
that mirrors the other role tests: call create_user_with_token(:admin) to get
company and token, insert a category with insert(:category, company: company),
perform delete using api_conn(token) |>
delete("/api/categories/#{category.id}"), and assert the response status is 204
(successfully deleted). Ensure the test follows the same setup style as "admin
can create categories" and "admin can update categories" so the controller
delete authorization is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18368b66-5b12-47fd-88a9-8471d27204eb
📒 Files selected for processing (4)
lib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/token_live.exlib/ksef_hub_web/router.extest/ksef_hub_web/controllers/api/category_controller_test.exs
- Replace generic @can_mutate with specific permission assigns in template (approve/reject → @can_approve, category → @can_set_category, tags → @can_set_tags, new tag form → @can_manage_tags) - Add missing "admin can delete categories" test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub_web/live/invoice_live/show.ex (1)
1016-1059:⚠️ Potential issue | 🟡 MinorNote edit UI controls should be gated by
@can_mutatefor consistency.The edit button (line 1021) and clickable note div (line 1052) remain interactive regardless of permission. While the server-side guards block the mutation, users will see controls they can't use and receive error flashes when clicking.
For consistency with the category/tag UI (which uses
disabledand conditionalphx-click), gate these note edit controls.🛠️ Suggested fix
<button - :if={!@editing_note} + :if={`@can_mutate` && !@editing_note} phx-click="edit_note" class="btn btn-xs btn-ghost" aria-label="Edit note" >And for the clickable note div:
<div class={[ "text-sm cursor-pointer hover:bg-base-200 rounded p-1 -m-1", !@invoice.note && "text-base-content/40 italic" ]} - phx-click="edit_note" + phx-click={if(`@can_mutate`, do: "edit_note")} >Alternatively, remove
cursor-pointerandhover:bg-base-200classes when@can_mutateis false.Based on learnings: "Check authorization in controllers and LiveViews" and "Validate all state changes in LiveView events".
🤖 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 1016 - 1059, The note edit controls are currently interactive regardless of permissions; gate them with `@can_mutate` by only rendering the edit button and the phx-click handlers when `@can_mutate` is true and by removing cursor-pointer/hover classes when `@can_mutate` is false: conditionally render the edit button (the element with phx-click="edit_note"), only attach phx-click="edit_note" to the note display div when `@can_mutate` is true, and keep the textarea/form submit (phx-submit="save_note") and cancel (phx-click="cancel_note") accessible only inside the `@editing_note` branch when `@can_mutate` is true; alternatively toggle CSS (remove "cursor-pointer" and "hover:bg-base-200") when `@can_mutate` is false so controls appear non-interactive.
🧹 Nitpick comments (1)
test/ksef_hub_web/controllers/api/category_controller_test.exs (1)
168-259: Addshowto the permission matrix.The new block exercises
index/create/update/delete, butshowis the other read path affected by the centralized role checks. Adding one allowed and one deniedshowcase would close the obvious authorization gap here.Based on learnings: "Test controller actions and context interactions using integration tests" and "Check authorization in controllers and LiveViews".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/category_controller_test.exs` around lines 168 - 259, Add two "show" tests in the same "permission enforcement" describe block: a positive case "accountant can read category (show)" and a negative case "reviewer cannot read category (show)". For each, use create_user_with_token(:accountant) / create_user_with_token(:reviewer), insert(:category, company: company), call conn |> api_conn(token) |> get("/api/categories/#{category.id}") and assert conn.status == 200 for the allowed accountant test and assert conn.status == 403 for the denied reviewer test; place the new tests near the existing index/create/update/delete tests so they exercise the same controller authorization paths (show action).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 1016-1059: The note edit controls are currently interactive
regardless of permissions; gate them with `@can_mutate` by only rendering the edit
button and the phx-click handlers when `@can_mutate` is true and by removing
cursor-pointer/hover classes when `@can_mutate` is false: conditionally render the
edit button (the element with phx-click="edit_note"), only attach
phx-click="edit_note" to the note display div when `@can_mutate` is true, and keep
the textarea/form submit (phx-submit="save_note") and cancel
(phx-click="cancel_note") accessible only inside the `@editing_note` branch when
`@can_mutate` is true; alternatively toggle CSS (remove "cursor-pointer" and
"hover:bg-base-200") when `@can_mutate` is false so controls appear
non-interactive.
---
Nitpick comments:
In `@test/ksef_hub_web/controllers/api/category_controller_test.exs`:
- Around line 168-259: Add two "show" tests in the same "permission enforcement"
describe block: a positive case "accountant can read category (show)" and a
negative case "reviewer cannot read category (show)". For each, use
create_user_with_token(:accountant) / create_user_with_token(:reviewer),
insert(:category, company: company), call conn |> api_conn(token) |>
get("/api/categories/#{category.id}") and assert conn.status == 200 for the
allowed accountant test and assert conn.status == 403 for the denied reviewer
test; place the new tests near the existing index/create/update/delete tests so
they exercise the same controller authorization paths (show action).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14127ff9-7415-4342-8490-a9ea2c458851
📒 Files selected for processing (2)
lib/ksef_hub_web/live/invoice_live/show.extest/ksef_hub_web/controllers/api/category_controller_test.exs
…ests - Hide note edit button and remove clickable styling when user lacks update_invoice permission - Add accountant/reviewer positive tests for category show endpoint Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 9, 2026
Merged
Merged
This was referenced Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests