feat: viewer role and per-user public invoice tokens with 30-day expiry#155
Merged
Merged
Conversation
…expiry Viewer role: - New :viewer membership role with read-only access to invoices and invoice details only (view_invoices + view_all_invoice_types permissions) - Dashboard route gated behind :view_dashboard — viewer is redirected to /invoices instead - Owner/admin can assign :viewer when inviting team members Per-user public tokens: - Replace single invoice.public_token (no expiry, no ownership) with a dedicated invoice_public_tokens table scoped to (invoice, user) - Tokens expire after 30 days; expired tokens are replaced atomically via upsert with on_conflict, preventing race conditions - ensure_public_token/2 returns :created | :existing so activity log events are only emitted on first generation, not on every "Copy link" click - Blocking a team member immediately invalidates all their shared links via delete_public_tokens_for_user/2 Security: - IDOR web-layer tests: URL company_id must match user's membership (live_auth_test) - Viewer and IDOR context-layer tests for cross-company access isolation - Public token expiry and per-user isolation tested end-to-end Co-Authored-By: Claude Sonnet 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:
📝 WalkthroughWalkthroughReplaced the Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
actor User
participant LiveView
participant PublicTokens
participant Repo
end
User->>LiveView: Click "Copy public link"
LiveView->>PublicTokens: ensure_public_token(invoice, user_id)
PublicTokens->>Repo: get_by(invoice_id: i.id, user_id: u, expires_at > now)
alt existing valid token
Repo-->>PublicTokens: existing token record
PublicTokens-->>LiveView: {:ok, token_record, :existing}
else missing or expired
PublicTokens->>PublicTokens: generate URL-safe token, set expires_at (30d)
PublicTokens->>Repo: upsert (conflict: invoice_id,user_id) replace token+expires_at
Repo-->>PublicTokens: canonical persisted record
PublicTokens-->>LiveView: {:ok, token_record, :created}
end
LiveView->>User: Provide public URL with token_record.token
sequenceDiagram
rect rgba(200,255,200,0.5)
actor PublicViewer
participant PublicLiveView
participant PublicTokens
participant Repo
end
PublicViewer->>PublicLiveView: Visit /public?token=XYZ
PublicLiveView->>PublicTokens: get_invoice_by_public_token("XYZ")
PublicTokens->>PublicTokens: validate token format/size
PublicTokens->>Repo: query invoice_public_tokens by token (digest) and expires_at > now
alt token found
Repo-->>PublicTokens: InvoicePublicToken w/ invoice_id
PublicTokens->>Repo: preload invoice (company, xml_file, pdf_file, category)
Repo-->>PublicTokens: populated Invoice
PublicTokens-->>PublicLiveView: Invoice
PublicLiveView-->>PublicViewer: Render invoice / PDF
else not found/invalid/expired
PublicTokens-->>PublicLiveView: nil
PublicLiveView-->>PublicViewer: 404 / access denied
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ksef_hub/authorization.ex (1)
71-93:⚠️ Potential issue | 🟠 MajorAdd
:view_all_invoice_typesto the viewer permission set.The PR objective says
:viewergets both:view_invoicesand:view_all_invoice_types, but the new@viewer_permissionsonly includes:view_invoices. As written, viewer users will be stricter than intended and can get blocked from invoice types they were supposed to see.Minimal fix
- `@viewer_permissions` MapSet.new([:view_invoices]) + `@viewer_permissions` MapSet.new([:view_invoices, :view_all_invoice_types])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/authorization.ex` around lines 71 - 93, The `@viewer_permissions` MapSet currently only contains :view_invoices; update the declaration of `@viewer_permissions` to include :view_all_invoice_types so that can?(:viewer, permission) returns true for both :view_invoices and :view_all_invoice_types (update the MapSet in the module where `@viewer_permissions` is defined, which is used by the can?/2 clause for :viewer).lib/ksef_hub_web/live/team_member_live/show.ex (1)
115-122:⚠️ Potential issue | 🟠 MajorMake member blocking and token revocation atomic.
delete_public_tokens_for_user/2runs afterCompanies.block_member/1, and its result is ignored. If that second write fails, the member is blocked but their public links remain valid, which breaks the “block immediately invalidates shared links” guarantee.Suggested direction
- case Companies.block_member(membership) do + case Companies.block_member_and_revoke_public_tokens(membership, actor_opts(socket)) do {:ok, updated} -> - Invoices.delete_public_tokens_for_user(membership.user_id, membership.company_id) - {:noreply, socket |> assign(membership: %{updated | user: membership.user}) |> put_flash(:info, "Member blocked.")}As per coding guidelines, "Use Ecto transactions and unique constraints for atomicity".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/team_member_live/show.ex` around lines 115 - 122, Wrap the member-block + token-deletion in an atomic Ecto transaction (e.g., Ecto.Multi or Repo.transaction) so Companies.block_member(membership) and Invoices.delete_public_tokens_for_user(membership.user_id, membership.company_id) are executed as a single unit and any failure in token deletion rolls back the block; replace the current sequential calls in the case clause with a transaction that returns the updated membership (preserving membership.user) only on success and surface the error to the socket (put_flash :error) or handle {:error, _} accordingly to avoid leaving the member blocked while tokens remain valid.
🧹 Nitpick comments (1)
lib/ksef_hub/invoices.ex (1)
173-175: Add@docand@specfor the delegated public-token APIs.These are new public context functions, but they currently have no documentation or typespecs. Please annotate them here so the
KsefHub.InvoicesAPI stays consistent with the rest of the context.As per coding guidelines "Every public function must have
@docdocumentation" and "Every function (public and private) must have@spectypespec".🤖 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 173 - 175, Add `@doc` and `@spec` annotations for each delegated public function so the KsefHub.Invoices API matches project guidelines: add a short `@doc` describing purpose for get_invoice_by_public_token/1, ensure_public_token/2 and delete_public_tokens_for_user/2, and add `@specs` that mirror the signatures and return types of the delegated functions in PublicTokens (e.g. `@spec` get_invoice_by_public_token(String.t()) :: <appropriate_invoice_type() | nil>, `@spec` ensure_public_token(invoice :: <invoice_type()>, user_id :: integer()) :: <return_type()> and `@spec` delete_public_tokens_for_user(user_id :: integer(), company_id :: integer()) :: <return_type()>), referencing the exact PublicTokens functions (PublicTokens.get_invoice_by_public_token/1, PublicTokens.ensure_public_token/2, PublicTokens.delete_public_tokens_for_user/2) to pick the correct types so docs and specs are present and consistent.
🤖 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 387-396: The handle_event "copy_public_link" currently
pattern-matches {:ok, pt, created?} = Invoices.ensure_public_token(invoice,
user_id) which will crash the LiveView on {:error, _}; change the call to a safe
case/with branch: call Invoices.ensure_public_token(invoice, user_id) and handle
{:ok, pt, created?} by building url(~p"..."), calling
Events.invoice_public_link_generated/2 when created? == :created, and returning
{:noreply, put_flash(socket, :info, "Public link copied") |> assign(...)};
handle {:error, reason} by logging or put_flash(socket, :error, "Could not
create public link") and return {:noreply, socket} so the socket doesn’t crash.
Ensure you reference handle_event("copy_public_link", ...),
Invoices.ensure_public_token/2 and Events.invoice_public_link_generated/2 when
making the changes.
In `@lib/ksef_hub_web/router.ex`:
- Around line 120-128: The routes "/invoices/:id/classify" and "/settings" are
still inside the plain :authenticated live_session so users with the :viewer
role can access more than the intended "invoice list + invoice details" surface;
move those routes into a permission-gated live_session (the same pattern used
for :require_view_dashboard) by placing them under a live_session that uses
on_mount: {KsefHubWeb.LiveAuth, {:require_permission, :view_dashboard}} or, if
they require a different permission, create a new live_session using the
appropriate {:require_permission, :permission_name} and mount
"/invoices/:id/classify" and "/settings" there so :authenticated without that
permission cannot reach them.
In `@lib/ksef_hub/companies/membership.ex`:
- Around line 64-65: The new role_description(:viewer) clause is not formatted
by mix format; run mix format (or manually wrap the long string) so the do: body
is moved to a new indented line instead of keeping the long string inline—for
example update the role_description(:viewer) clause in the Membership module
(role_description(:viewer)) so the string is wrapped across a new line per
formatter conventions and then run mix format to apply consistent formatting.
In `@lib/ksef_hub/invoices.ex`:
- Line 175: The delete_public_tokens_for_user/2 path currently performs a silent
Repo.delete_all in PublicTokens (lib/ksef_hub/invoices/public_tokens.ex) which
removes user-shared links without producing an audit trail; change the
implementation so that instead of a bare Repo.delete_all you either (a) use
TrackedRepo to perform the deletion so it emits a tracked activity event, or (b)
explicitly emit a domain event (e.g. Events.PublicLinksRevoked or similar) from
PublicTokens.delete_public_tokens_for_user/2 (or from the member-block flow that
calls it) containing user_id, company_id and count of revoked tokens; ensure the
event is recorded consistently with other Events.* usages per guidelines.
In `@lib/ksef_hub/invoices/invoice_public_token.ex`:
- Around line 20-38: Persisting the raw bearer token is unsafe; modify the
schema and changeset to store only a digest and keep the raw token transient and
redacted: add a persisted field like token_digest (field :token_digest, :string,
redact: true) and make the raw token a virtual/redacted field (field :token,
:string, virtual: true, redact: true); update the changeset function changeset/2
to cast the virtual :token and other attrs, compute and
put_change(:token_digest, secure_hash(token)) when a token is present,
validate_required against :token_digest (not :token), and use
unique_constraint(:token_digest) and
foreign_key_constraint(:invoice_id)/(:user_id) as before so lookups use the
digest while the raw token is never persisted or logged.
In `@lib/ksef_hub/invoices/public_tokens.ex`:
- Around line 103-129: The formatter failed in rotate_public_token; run mix
format to fix whitespace/formatting around the token/expires_at generation and
the Repo.insert pipeline in rotate_public_token, and then decide whether to stop
replacing inserted_at on conflict: locate the Repo.insert call in
rotate_public_token (InvoicePublicToken.changeset |> Repo.insert) and remove
:inserted_at from the on_conflict replace list (or replace it with an
appropriate mutable timestamp like :updated_at) if you want created timestamps
to remain immutable for audit; keep the conflict_target [:invoice_id, :user_id]
and ensure the upsert behavior still returns the DB-canonical token via
Repo.get_by! afterward.
- Around line 67-83: The delete_public_tokens_for_user function uses
Repo.delete_all which bypasses activity events; change the delete call to
TrackedRepo.delete_all so InvoicePublicToken deletions emit activity (keep the
same query using invoice_ids/subquery and where on pt.user_id). Also consider
ensuring the caller that invokes block_member and delete_public_tokens_for_user
runs them inside a single TrackedRepo.transaction (or have block_member call
delete_public_tokens_for_user within a TrackedRepo.transaction) so both blocking
and token deletion succeed or roll back together.
In `@test/ksef_hub_web/live/invoice_live/show_test.exs`:
- Around line 239-252: The test currently only counts InvoicePublicToken rows by
invoice_id; change the query to assert idempotency on the (invoice_id, user_id)
tuple by adding a where clause for the test's user (e.g. where: pt.user_id ==
^user.id) and ensure the test uses the existing `user` from the test context (or
fetch current user from `conn.assigns`) when building the query against
InvoicePublicToken and Repo.one; this both verifies the same user's token was
reused and resolves the formatter complaint in this block.
In `@test/ksef_hub/invoices/access_control_test.exs`:
- Line 720: Reformat the failing assertion so it conforms to mix format: run
`mix format` (or `mix format --check-formatted` to verify) and update the
`assert is_nil(...)` call that wraps `Invoices.get_invoice(company_a.id,
invoice_b.id, role: :owner, user_id: user.id)` so it is formatted across lines
per Elixir formatter rules (break arguments/macro call into multiple lines if
needed) until `mix format` passes.
---
Outside diff comments:
In `@lib/ksef_hub_web/live/team_member_live/show.ex`:
- Around line 115-122: Wrap the member-block + token-deletion in an atomic Ecto
transaction (e.g., Ecto.Multi or Repo.transaction) so
Companies.block_member(membership) and
Invoices.delete_public_tokens_for_user(membership.user_id,
membership.company_id) are executed as a single unit and any failure in token
deletion rolls back the block; replace the current sequential calls in the case
clause with a transaction that returns the updated membership (preserving
membership.user) only on success and surface the error to the socket (put_flash
:error) or handle {:error, _} accordingly to avoid leaving the member blocked
while tokens remain valid.
In `@lib/ksef_hub/authorization.ex`:
- Around line 71-93: The `@viewer_permissions` MapSet currently only contains
:view_invoices; update the declaration of `@viewer_permissions` to include
:view_all_invoice_types so that can?(:viewer, permission) returns true for both
:view_invoices and :view_all_invoice_types (update the MapSet in the module
where `@viewer_permissions` is defined, which is used by the can?/2 clause for
:viewer).
---
Nitpick comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 173-175: Add `@doc` and `@spec` annotations for each delegated public
function so the KsefHub.Invoices API matches project guidelines: add a short
`@doc` describing purpose for get_invoice_by_public_token/1, ensure_public_token/2
and delete_public_tokens_for_user/2, and add `@specs` that mirror the signatures
and return types of the delegated functions in PublicTokens (e.g. `@spec`
get_invoice_by_public_token(String.t()) :: <appropriate_invoice_type() | nil>,
`@spec` ensure_public_token(invoice :: <invoice_type()>, user_id :: integer()) ::
<return_type()> and `@spec` delete_public_tokens_for_user(user_id :: integer(),
company_id :: integer()) :: <return_type()>), referencing the exact PublicTokens
functions (PublicTokens.get_invoice_by_public_token/1,
PublicTokens.ensure_public_token/2,
PublicTokens.delete_public_tokens_for_user/2) to pick the correct types so docs
and specs are present and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5c26031-11d8-475e-bb12-702b7c1a5460
📒 Files selected for processing (21)
lib/ksef_hub/authorization.exlib/ksef_hub/companies/membership.exlib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/invoices/invoice_public_token.exlib/ksef_hub/invoices/public_tokens.exlib/ksef_hub_web/live/invoice_live/public_show.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/team_member_live/show.exlib/ksef_hub_web/router.expriv/repo/migrations/20260418200001_create_invoice_public_tokens.exspriv/repo/migrations/20260418200002_remove_public_token_from_invoices.exspriv/repo/migrations/20260418200003_add_unique_index_to_invoice_public_tokens.exstest/ksef_hub/authorization_test.exstest/ksef_hub/invoices/access_control_test.exstest/ksef_hub/invoices/public_token_test.exstest/ksef_hub_web/controllers/public_invoice_pdf_controller_test.exstest/ksef_hub_web/live/invoice_live/public_show_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/ksef_hub_web/live/live_auth_test.exstest/ksef_hub_web/live/role_based_nav_test.exs
💤 Files with no reviewable changes (1)
- lib/ksef_hub/invoices/invoice.ex
Comment on lines
+120
to
+128
| live_session :require_view_dashboard, | ||
| on_mount: [ | ||
| {KsefHubWeb.LiveAuth, :default}, | ||
| {KsefHubWeb.LiveAuth, {:require_permission, :view_dashboard}} | ||
| ] do | ||
| live "/dashboard", DashboardLive | ||
| end | ||
|
|
||
| live_session :authenticated, on_mount: {KsefHubWeb.LiveAuth, :default} do |
There was a problem hiding this comment.
Viewer access is still broader than the PR contract.
This split only gates "/dashboard". "/invoices/:id/classify" is still mounted in the plain :authenticated session, and "/settings" remains ungated below, so :viewer can still reach screens outside the promised “invoice list + invoice details only” surface. Please move those routes behind permission-gated sessions before release.
🤖 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 120 - 128, The routes
"/invoices/:id/classify" and "/settings" are still inside the plain
:authenticated live_session so users with the :viewer role can access more than
the intended "invoice list + invoice details" surface; move those routes into a
permission-gated live_session (the same pattern used for
:require_view_dashboard) by placing them under a live_session that uses
on_mount: {KsefHubWeb.LiveAuth, {:require_permission, :view_dashboard}} or, if
they require a different permission, create a new live_session using the
appropriate {:require_permission, :permission_name} and mount
"/invoices/:id/classify" and "/settings" there so :authenticated without that
permission cannot reach them.
| end | ||
| defdelegate get_invoice_by_public_token(token), to: PublicTokens | ||
| defdelegate ensure_public_token(invoice, user_id), to: PublicTokens | ||
| defdelegate delete_public_tokens_for_user(user_id, company_id), to: PublicTokens |
There was a problem hiding this comment.
Public-link invalidation needs its own audit event.
The new delete_public_tokens_for_user/2 API invalidates user-shared links, but the implementation in lib/ksef_hub/invoices/public_tokens.ex:67-83 does a bare Repo.delete_all/0. Blocking a member will silently revoke links with no activity trail. Please emit an Events.* record from that path, or from the member-block flow that calls it.
As per coding guidelines "Every context mutation that affects user-visible state must emit an activity event using TrackedRepo instead of Repo" and "Use Events.* directly instead of TrackedRepo when dealing with login/logout, sync triggers (Oban jobs), multi transactions, or cross-entity events that don't have a changeset".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices.ex` at line 175, The delete_public_tokens_for_user/2
path currently performs a silent Repo.delete_all in PublicTokens
(lib/ksef_hub/invoices/public_tokens.ex) which removes user-shared links without
producing an audit trail; change the implementation so that instead of a bare
Repo.delete_all you either (a) use TrackedRepo to perform the deletion so it
emits a tracked activity event, or (b) explicitly emit a domain event (e.g.
Events.PublicLinksRevoked or similar) from
PublicTokens.delete_public_tokens_for_user/2 (or from the member-block flow that
calls it) containing user_id, company_id and count of revoked tokens; ensure the
event is recorded consistently with other Events.* usages per guidelines.
Comment on lines
+20
to
+38
| schema "invoice_public_tokens" do | ||
| field :token, :string | ||
| field :expires_at, :utc_datetime | ||
|
|
||
| belongs_to :invoice, Invoice | ||
| belongs_to :user, User | ||
|
|
||
| timestamps(updated_at: false) | ||
| end | ||
|
|
||
| @doc "Builds a changeset for inserting a new public token." | ||
| @spec changeset(t(), map()) :: Ecto.Changeset.t() | ||
| def changeset(token, attrs) do | ||
| token | ||
| |> cast(attrs, [:token, :expires_at, :invoice_id, :user_id]) | ||
| |> validate_required([:token, :expires_at, :invoice_id, :user_id]) | ||
| |> foreign_key_constraint(:invoice_id) | ||
| |> foreign_key_constraint(:user_id) | ||
| |> unique_constraint(:token) |
There was a problem hiding this comment.
Don't persist the share token in plaintext.
token is a bearer secret. Keeping the raw value in this table means a DB read, crash dump, or inspected changeset is enough to open a shared invoice. Please store only a digest and look up by digest; if the raw value remains transiently in memory, mark the persisted field redacted as well.
As per coding guidelines "Never log certificates, passwords, or token values".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices/invoice_public_token.ex` around lines 20 - 38,
Persisting the raw bearer token is unsafe; modify the schema and changeset to
store only a digest and keep the raw token transient and redacted: add a
persisted field like token_digest (field :token_digest, :string, redact: true)
and make the raw token a virtual/redacted field (field :token, :string, virtual:
true, redact: true); update the changeset function changeset/2 to cast the
virtual :token and other attrs, compute and put_change(:token_digest,
secure_hash(token)) when a token is present, validate_required against
:token_digest (not :token), and use unique_constraint(:token_digest) and
foreign_key_constraint(:invoice_id)/(:user_id) as before so lookups use the
digest while the raw token is never persisted or logged.
Comment on lines
+67
to
+83
| @doc """ | ||
| Deletes all public sharing tokens created by a user for invoices belonging | ||
| to a given company. Called when a member is blocked to immediately invalidate | ||
| any links they may have shared. | ||
| """ | ||
| @spec delete_public_tokens_for_user(Ecto.UUID.t(), Ecto.UUID.t()) :: non_neg_integer() | ||
| def delete_public_tokens_for_user(user_id, company_id) | ||
| when is_binary(user_id) and is_binary(company_id) do | ||
| invoice_ids = from(i in Invoice, where: i.company_id == ^company_id, select: i.id) | ||
|
|
||
| {count, _} = | ||
| InvoicePublicToken | ||
| |> where([pt], pt.user_id == ^user_id and pt.invoice_id in subquery(invoice_ids)) | ||
| |> Repo.delete_all() | ||
|
|
||
| count | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if block_member logs an activity event when blocking
rg -n 'TrackedRepo|Events\.' lib/ksef_hub/companies.ex -A 3 -B 3
# Check how other bulk deletions handle activity logging
rg -n 'delete_all' lib/ksef_hub/ --type elixir -A 2Repository: appunite/appunite-ksef-ex
Length of output: 3388
🏁 Script executed:
# Find where delete_public_tokens_for_user is called
rg -n 'delete_public_tokens_for_user' lib/ksef_hub/ --type elixir -B 3 -A 3
# Check the full block_member flow to see if tokens are deleted
rg -n 'block_member' lib/ksef_hub/ --type elixir -B 2 -A 5Repository: appunite/appunite-ksef-ex
Length of output: 2672
🏁 Script executed:
# Search for calls to delete_public_tokens_for_user
rg -n 'delete_public_tokens_for_user' lib/ksef_hub/ --type elixir
# Also check if there's a context/api layer that chains block_member with token deletion
rg -n 'Invoices\.delete_public_tokens_for_user|PublicTokens\.delete_public_tokens_for_user' . --type elixir -B 5 -A 5Repository: appunite/appunite-ksef-ex
Length of output: 4251
Use TrackedRepo to emit activity events for token deletion.
This function mutates database state (deletes tokens) affecting user-visible access. Per coding guidelines, context mutations in lib/ksef_hub/**/*.ex that affect user-visible state must emit activity events via TrackedRepo instead of Repo.
Token deletion falls under this requirement: users lose access to shared invoice links, which is observable state change. Currently using Repo.delete_all() directly skips activity logging. Replace with TrackedRepo.delete_all() or emit an explicit activity event.
Additionally, this function is called as a separate step after block_member succeeds (in team_member_live/show.ex:117) with no transaction wrapping—if deletion fails, the member is already blocked. Consider wrapping both operations in a transaction for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices/public_tokens.ex` around lines 67 - 83, The
delete_public_tokens_for_user function uses Repo.delete_all which bypasses
activity events; change the delete call to TrackedRepo.delete_all so
InvoicePublicToken deletions emit activity (keep the same query using
invoice_ids/subquery and where on pt.user_id). Also consider ensuring the caller
that invokes block_member and delete_public_tokens_for_user runs them inside a
single TrackedRepo.transaction (or have block_member call
delete_public_tokens_for_user within a TrackedRepo.transaction) so both blocking
and token deletion succeed or roll back together.
…expiry Implement ADR-0046 (per-user public invoice tokens) and ADR-0047 (approver/analyst role rename): - Per-user public sharing tokens scoped to (invoice, user) pairs with 30-day TTL - Unique constraint ensures one active token per user/invoice pair - Concurrent callers converge on same DB-canonical token via upsert - Token revocation when member is blocked (atomic transaction) - Activity events: invoice.public_link_generated and member.public_tokens_revoked Rename reviewer → approver and viewer → analyst: - Clarifies role intent: approver is action-oriented, analyst is read-only - Analyst retains same data scope as approver (access grants required for restricted) - Analyst is non-invitable; only admin, accountant, approver are invitable roles - Route gating: /classify moved to :require_set_category, /settings to :require_view_dashboard - Analyst is now redirected from admin-only pages (dashboard, settings) Fixes: - Fix @SPEC for ensure_public_token to include :existing in return type - Remove :inserted_at from on_conflict list (prevents token rotation timestamp overwrite) - Fix test: idempotency now correctly asserts :existing + same token on second call - Update stale comments and test names from 'viewer' to 'analyst' - Add test: analyst is redirected from /settings - Add migration to revert token_digest column back to token Co-Authored-By: Claude Sonnet 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)
test/ksef_hub/invitations/invitation_notifier_test.exs (1)
24-33:⚠️ Potential issue | 🟡 MinorRename the test case to match
:approver.The assertions now validate
approver, but the test title still says “reviewer”, which is stale and confusing.Proposed rename
- test "includes correct role description for reviewer" do + test "includes correct role description for approver" do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/invitations/invitation_notifier_test.exs` around lines 24 - 33, Rename the test description string to reflect the role being asserted: change the test title "includes correct role description for reviewer" to something like "includes correct role description for approver" in the test that calls InvitationNotifier.deliver_invitation and asserts email.text_body contains "approver" so the test name matches the asserted role.
♻️ Duplicate comments (2)
lib/ksef_hub/invoices/invoice_public_token.ex (1)
21-21:⚠️ Potential issue | 🟠 MajorHarden token changeset against secret leakage and mass-assignment.
Line 21 should redact token values, and Line 34 should not cast
:invoice_id/:user_idfrom attrs. These IDs should be set on the struct by trusted code paths before callingchangeset/2.🔒 Suggested patch
schema "invoice_public_tokens" do - field :token, :string + field :token, :string, redact: true field :expires_at, :utc_datetime belongs_to :invoice, Invoice belongs_to :user, User @@ def changeset(token_record, attrs) do token_record - |> cast(attrs, [:token, :expires_at, :invoice_id, :user_id]) + |> cast(attrs, [:token, :expires_at]) |> validate_required([:token, :expires_at, :invoice_id, :user_id]) |> foreign_key_constraint(:invoice_id) |> foreign_key_constraint(:user_id) |> unique_constraint(:token) endAs per coding guidelines "Fields which are set programmatically, such as
user_id, must not be listed incastcalls for security purposes" and "Never log certificates, passwords, or token values".Also applies to: 34-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/invoice_public_token.ex` at line 21, The token field must be redacted from logs and IDs must not be mass-assignable: add a module-level Inspect derivation to exclude :token (e.g. `@derive` {Inspect, except: [:token]}) so token values are never shown by Inspect/logs, and update the changeset/2 function to remove :invoice_id and :user_id from the cast(...) list (leave only public attrs like :token or other safe fields) and rely on trusted code paths to set invoice_id/user_id on the struct before calling changeset/2 (or set them via put_change/put_assoc inside trusted code), keeping validations (validate_required/unique_constraint) intact for those IDs if needed.lib/ksef_hub/invoices/public_tokens.ex (1)
103-108:⚠️ Potential issue | 🟡 MinorFix formatting to pass CI.
The pipeline reports a formatting failure. The
expires_atassignment should be wrapped across multiple lines permix format.🔧 Proposed formatting fix
defp rotate_public_token(invoice_id, user_id) do token = :crypto.strong_rand_bytes(32) |> Base.url_encode64(padding: false) - expires_at = DateTime.utc_now() |> DateTime.add(`@token_ttl_days`, :day) |> DateTime.truncate(:second) + + expires_at = + DateTime.utc_now() + |> DateTime.add(`@token_ttl_days`, :day) + |> DateTime.truncate(:second)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/public_tokens.ex` around lines 103 - 108, The formatting error is from the multi-assignment block in rotate_public_token — break the long expires_at expression across multiple lines so it matches mix format expectations; locate the expires_at binding inside the rotate_public_token/2 function and reflow the DateTime.utc_now() |> DateTime.add(`@token_ttl_days`, :day) |> DateTime.truncate(:second) call over multiple lines and then continue building the %InvoicePublicToken{} struct as separate lines to satisfy mix format.
🧹 Nitpick comments (3)
.claude/skills/ksef-hub-design/SKILL.md (1)
1-5: Consider defining invocation parameters in YAML frontmatter.Based on learnings, SKILL.md should use YAML frontmatter with consistent formatting to define invocation parameters. If this skill accepts arguments (e.g., surface type, language, theme), define them here for clarity and validation.
Additionally, the description field is quite verbose for metadata. Consider moving detailed usage instructions to the body and keeping the description concise.
Example parameter definition
--- name: ksef-hub-design -description: Use this skill to generate well-branded interfaces and assets for KSeF Hub (a microservice for Poland's Krajowy System e-Faktur), either for production or throwaway prototypes/mocks/slides. Contains essential design guidelines, colors, type, fonts, assets, and UI kit components for prototyping the Phoenix/LiveView admin and its marketing surfaces. +description: Generate well-branded interfaces and assets for KSeF Hub using the project's design system. user-invocable: true +arguments: + surface: + type: string + description: Target surface (admin UI, marketing landing, docs, slides, mock) + required: false + language: + type: string + description: Content language (EN or PL) + required: false + theme: + type: string + description: Theme mode (light, dark, both) + required: false ---Based on learnings: SKILL.md must contain Claude instructions with YAML frontmatter for argument definitions, using consistent formatting to define invocation parameters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/ksef-hub-design/SKILL.md around lines 1 - 5, The SKILL.md currently lacks YAML frontmatter defining invocation parameters and has an overly verbose description; update SKILL.md to include proper YAML frontmatter at the top that declares allowed arguments (e.g., surface, language, theme, mode) and their types/defaults for the ksef-hub-design skill, and move long usage instructions out of the description into the document body so the top-level description is concise; ensure the frontmatter follows the same formatting pattern used by other skills and include clear parameter names that map to the skill’s invocation (e.g., surface, language, theme) so the runtime can validate inputs.test/ksef_hub_web/controllers/api/category_controller_test.exs (1)
205-290: Consider renaming “reviewer” test labels to “approver”.The role fixtures are correctly updated to
:approver, but several test titles still reference “reviewer”, which can cause confusion in failure output.Suggested test-title cleanup
- test "reviewer can read category (show)", %{conn: conn} do + test "approver can read category (show)", %{conn: conn} do - test "reviewer can read categories (index)", %{conn: conn} do + test "approver can read categories (index)", %{conn: conn} do - test "reviewer cannot create categories", %{conn: conn} do + test "approver cannot create categories", %{conn: conn} do - test "reviewer cannot update categories", %{conn: conn} do + test "approver cannot update categories", %{conn: conn} do - test "reviewer cannot delete categories", %{conn: conn} do + test "approver cannot delete categories", %{conn: conn} do🤖 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 205 - 290, Update the test titles that still say "reviewer" to "approver" in test/ksef_hub_web/controllers/api/category_controller_test.exs so they match the role fixture :approver used in the setup; specifically rename the tests with descriptions "reviewer can read category (show)", "reviewer can read categories (index)", "reviewer cannot create categories", "reviewer cannot update categories", and "reviewer cannot delete categories" to use "approver" instead.test/ksef_hub_web/plugs/api_auth_test.exs (1)
94-103: Rename this test description toapproverfor clarity.The body now validates
:approver, but the test name still says “reviewer”, which is misleading during triage.Suggested rename
- test "assigns reviewer role for reviewer-created token", %{conn: conn} do + test "assigns approver role for approver-created token", %{conn: conn} do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/plugs/api_auth_test.exs` around lines 94 - 103, The test description string is misleading—update the test's description (the first argument to the test macro in the test that calls create_user_with_token(:approver) and asserts conn.assigns.current_role == :approver) from the current "reviewer" wording to "approver" so the name matches the body (e.g., replace the test description text with "approver").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/ksef-hub-design/SKILL.md:
- Around line 1-28: The skill directory is missing the required metadata and
user docs; add a skill.json describing the skill (include keys: name, version,
description, invocation command, categories, keywords, and dependencies) and add
a README.md with user-facing documentation and quick usage instructions that
reference SKILL.md for full guidance; ensure skill.json uses the same name
"ksef-hub-design" and that README.md summarizes intent, available assets
(colors_and_type.css, brand/, preview/, ui_kits/admin/, assets/), typical
invocation questions (surface, language, theme, screens) and how to request HTML
or production code.
- Line 11: The SKILL.md references to the non-existent docs/design_system/ paths
(notably `docs/design_system/colors_and_type.css`) must be removed or corrected:
update the section around lines referencing design tokens to point only to the
actual file `assets/css/app.css` (or delete the block if documentation was
removed), and ensure any mention of "docs/design_system" is replaced with the
correct path or omitted; edit the SKILL.md entry that mentions
`docs/design_system/colors_and_type.css` so it either references
`assets/css/app.css` as the source of design tokens or removes the
nonexistent-path instructions entirely.
In `@test/ksef_hub/companies/membership_test.exs`:
- Line 55: The test's valid-role loop in membership_test.exs currently iterates
over [:owner, :admin, :accountant, :approver] and omits the :analyst role;
update the for loop that defines role (for role <- [:owner, :admin, :accountant,
:approver]) to include :analyst so the test covers analyst membership as well,
then run the test suite to ensure no other assertions need adjusting.
---
Outside diff comments:
In `@test/ksef_hub/invitations/invitation_notifier_test.exs`:
- Around line 24-33: Rename the test description string to reflect the role
being asserted: change the test title "includes correct role description for
reviewer" to something like "includes correct role description for approver" in
the test that calls InvitationNotifier.deliver_invitation and asserts
email.text_body contains "approver" so the test name matches the asserted role.
---
Duplicate comments:
In `@lib/ksef_hub/invoices/invoice_public_token.ex`:
- Line 21: The token field must be redacted from logs and IDs must not be
mass-assignable: add a module-level Inspect derivation to exclude :token (e.g.
`@derive` {Inspect, except: [:token]}) so token values are never shown by
Inspect/logs, and update the changeset/2 function to remove :invoice_id and
:user_id from the cast(...) list (leave only public attrs like :token or other
safe fields) and rely on trusted code paths to set invoice_id/user_id on the
struct before calling changeset/2 (or set them via put_change/put_assoc inside
trusted code), keeping validations (validate_required/unique_constraint) intact
for those IDs if needed.
In `@lib/ksef_hub/invoices/public_tokens.ex`:
- Around line 103-108: The formatting error is from the multi-assignment block
in rotate_public_token — break the long expires_at expression across multiple
lines so it matches mix format expectations; locate the expires_at binding
inside the rotate_public_token/2 function and reflow the DateTime.utc_now() |>
DateTime.add(`@token_ttl_days`, :day) |> DateTime.truncate(:second) call over
multiple lines and then continue building the %InvoicePublicToken{} struct as
separate lines to satisfy mix format.
---
Nitpick comments:
In @.claude/skills/ksef-hub-design/SKILL.md:
- Around line 1-5: The SKILL.md currently lacks YAML frontmatter defining
invocation parameters and has an overly verbose description; update SKILL.md to
include proper YAML frontmatter at the top that declares allowed arguments
(e.g., surface, language, theme, mode) and their types/defaults for the
ksef-hub-design skill, and move long usage instructions out of the description
into the document body so the top-level description is concise; ensure the
frontmatter follows the same formatting pattern used by other skills and include
clear parameter names that map to the skill’s invocation (e.g., surface,
language, theme) so the runtime can validate inputs.
In `@test/ksef_hub_web/controllers/api/category_controller_test.exs`:
- Around line 205-290: Update the test titles that still say "reviewer" to
"approver" in test/ksef_hub_web/controllers/api/category_controller_test.exs so
they match the role fixture :approver used in the setup; specifically rename the
tests with descriptions "reviewer can read category (show)", "reviewer can read
categories (index)", "reviewer cannot create categories", "reviewer cannot
update categories", and "reviewer cannot delete categories" to use "approver"
instead.
In `@test/ksef_hub_web/plugs/api_auth_test.exs`:
- Around line 94-103: The test description string is misleading—update the
test's description (the first argument to the test macro in the test that calls
create_user_with_token(:approver) and asserts conn.assigns.current_role ==
:approver) from the current "reviewer" wording to "approver" so the name matches
the body (e.g., replace the test description text with "approver").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc3edce8-736f-4dd0-b259-4c839ee57986
📒 Files selected for processing (50)
.claude/skills/frontend/SKILL.md.claude/skills/ksef-hub-design/SKILL.mddocs/adr/0046-per-user-public-invoice-tokens.mddocs/adr/0047-approver-analyst-roles.mddocs/architecture.mdlib/ksef_hub/activity_log/events.exlib/ksef_hub/authorization.exlib/ksef_hub/companies/membership.exlib/ksef_hub/invitations/invitation.exlib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice_public_token.exlib/ksef_hub/invoices/public_tokens.exlib/ksef_hub_web/components/invoice_components.exlib/ksef_hub_web/helpers/auth_helpers.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/team_live/invite.exlib/ksef_hub_web/live/team_member_live/show.exlib/ksef_hub_web/router.expriv/repo/migrations/20260419110000_rename_reviewer_viewer_roles.exspriv/repo/migrations/20260419120000_revert_token_digest_to_token.exstest/ksef_hub/accounts/api_tokens_test.exstest/ksef_hub/activity_log/events_test.exstest/ksef_hub/activity_log/integration_test.exstest/ksef_hub/activity_log/tracked_repo_test.exstest/ksef_hub/authorization_test.exstest/ksef_hub/companies/membership_test.exstest/ksef_hub/companies_test.exstest/ksef_hub/invitations/invitation_notifier_test.exstest/ksef_hub/invitations_test.exstest/ksef_hub/invoices/access_control_test.exstest/ksef_hub/invoices/public_token_test.exstest/ksef_hub/invoices_test.exstest/ksef_hub_web/controllers/api/category_controller_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/ksef_hub_web/controllers/export_controller_test.exstest/ksef_hub_web/live/bank_account_live_test.exstest/ksef_hub_web/live/invitation_accept_live_test.exstest/ksef_hub_web/live/invoice_live/classify_test.exstest/ksef_hub_web/live/invoice_live/index_test.exstest/ksef_hub_web/live/invoice_live/public_show_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/ksef_hub_web/live/live_auth_test.exstest/ksef_hub_web/live/role_based_nav_test.exstest/ksef_hub_web/live/settings_live/activity_log_test.exstest/ksef_hub_web/live/settings_live/general_test.exstest/ksef_hub_web/live/team_live_test.exstest/ksef_hub_web/live/team_member_live/show_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 due to trivial changes (12)
- lib/ksef_hub_web/components/invoice_components.ex
- test/support/api_test_helpers.ex
- lib/ksef_hub_web/helpers/auth_helpers.ex
- .claude/skills/frontend/SKILL.md
- test/ksef_hub_web/live/settings_live/activity_log_test.exs
- test/ksef_hub_web/live/team_member_live/show_test.exs
- test/ksef_hub/activity_log/tracked_repo_test.exs
- lib/ksef_hub_web/live/team_live/invite.ex
- priv/repo/migrations/20260419110000_rename_reviewer_viewer_roles.exs
- docs/architecture.md
- docs/adr/0046-per-user-public-invoice-tokens.md
- test/ksef_hub_web/controllers/api/invoice_controller_test.exs
🚧 Files skipped from review as they are similar to previous changes (8)
- test/ksef_hub_web/live/invoice_live/public_show_test.exs
- test/ksef_hub_web/live/role_based_nav_test.exs
- lib/ksef_hub_web/live/team_member_live/show.ex
- test/ksef_hub_web/live/live_auth_test.exs
- test/ksef_hub/invoices/access_control_test.exs
- test/ksef_hub/authorization_test.exs
- lib/ksef_hub/companies/membership.ex
- lib/ksef_hub/invoices.ex
Comment on lines
+1
to
+28
| --- | ||
| name: ksef-hub-design | ||
| description: Use this skill to generate well-branded interfaces and assets for KSeF Hub (a microservice for Poland's Krajowy System e-Faktur), either for production or throwaway prototypes/mocks/slides. Contains essential design guidelines, colors, type, fonts, assets, and UI kit components for prototyping the Phoenix/LiveView admin and its marketing surfaces. | ||
| user-invocable: true | ||
| --- | ||
|
|
||
| # KSeF Hub design skill | ||
|
|
||
| Read the `docs/design_system/README.md` file within this project, and explore the other available files. | ||
|
|
||
| - `docs/design_system/colors_and_type.css` has every design token (shadcn-style custom properties + brand accent, light/dark). | ||
| - `docs/design_system/brand/` has the working logo (mark + wordmark) and naming alternatives. | ||
| - `docs/design_system/preview/` has small specimen cards -- good references when building new components. | ||
| - `docs/design_system/ui_kits/admin/` is a fully-wired LiveView admin recreation: app shell, invoices list, invoice detail, dashboard, settings, login. Read its JSX components (`Primitives.jsx`, `AppShell.jsx`, etc.) -- they are the source of truth for component visuals and API. | ||
| - `docs/design_system/assets/` holds patterns/icons referenced by the kits (Heroicons is CDN-linked). | ||
|
|
||
| If creating visual artifacts (slides, mocks, throwaway prototypes, etc.), copy assets out and create static HTML files for the user to view. Load `docs/design_system/colors_and_type.css` from any HTML you produce and everything else will fall into place visually. | ||
|
|
||
| If working on production code, read `docs/design_system/README.md` to internalize the rules (voice, iconography, status vocabulary, layout) and use the tokens already shipped in `assets/css/app.css` of the real codebase. | ||
|
|
||
| If the user invokes this skill without any other guidance, ask them what they want to build or design. Then ask clarifying questions: | ||
|
|
||
| - Which surface? (admin UI, marketing landing, docs, slides, one-off mock) | ||
| - EN or PL copy? | ||
| - Light/dark/both? | ||
| - Any specific screens or flows? | ||
|
|
||
| Then act as an expert designer who outputs HTML artifacts _or_ production code, depending on the need. Match the existing voice: dry, technical, infrastructure-grade (Stripe/Linear). Never invent new color ramps; always reach for the tokens. Never use emoji except on category badges. Keep the admin flat -- no gradients, no shadows on content cards, borders at 1px. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for required skill structure files.
echo "Checking .claude/skills/ksef-hub-design/ structure:"
fd . .claude/skills/ksef-hub-design --type f --max-depth 1
echo ""
echo "Required files:"
for file in skill.json SKILL.md README.md; do
if [ -f ".claude/skills/ksef-hub-design/$file" ]; then
echo "✓ $file exists"
else
echo "✗ $file MISSING"
fi
done
echo ""
echo "Optional directories:"
for dir in references scripts; do
if [ -d ".claude/skills/ksef-hub-design/$dir" ]; then
echo "✓ $dir/ exists"
else
echo " $dir/ not present (optional)"
fi
doneRepository: appunite/appunite-ksef-ex
Length of output: 330
Add required skill.json and README.md files to complete the skill directory structure.
This skill is missing two mandatory files per the standardized skill structure:
skill.json— Must define metadata: name, version, description, invocation command, categories, keywords, and dependencies.README.md— Must provide user-facing documentation.
Only SKILL.md is currently present. Add both files to .claude/skills/ksef-hub-design/ before this skill can be properly distributed.
🧰 Tools
🪛 LanguageTool
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...atives. - docs/design_system/preview/ has small specimen cards -- good references...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/ksef-hub-design/SKILL.md around lines 1 - 28, The skill
directory is missing the required metadata and user docs; add a skill.json
describing the skill (include keys: name, version, description, invocation
command, categories, keywords, and dependencies) and add a README.md with
user-facing documentation and quick usage instructions that reference SKILL.md
for full guidance; ensure skill.json uses the same name "ksef-hub-design" and
that README.md summarizes intent, available assets (colors_and_type.css, brand/,
preview/, ui_kits/admin/, assets/), typical invocation questions (surface,
language, theme, screens) and how to request HTML or production code.
|
|
||
| Read the `docs/design_system/README.md` file within this project, and explore the other available files. | ||
|
|
||
| - `docs/design_system/colors_and_type.css` has every design token (shadcn-style custom properties + brand accent, light/dark). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for the existence of the design token CSS file and compare paths.
# Check if docs design token file exists
if [ -f "docs/design_system/colors_and_type.css" ]; then
echo "✓ docs/design_system/colors_and_type.css exists"
echo "File size: $(wc -l < docs/design_system/colors_and_type.css) lines"
else
echo "✗ docs/design_system/colors_and_type.css NOT FOUND"
fi
# Check production CSS file
if [ -f "assets/css/app.css" ]; then
echo "✓ assets/css/app.css exists"
echo "Contains design tokens: $(rg -c 'oklch|--color-' assets/css/app.css || echo '0') lines"
else
echo "✗ assets/css/app.css NOT FOUND"
fi
# List actual docs/design_system structure
echo ""
echo "Actual docs/design_system/ structure:"
fd . docs/design_system --type f --max-depth 3Repository: appunite/appunite-ksef-ex
Length of output: 343
🏁 Script executed:
# List the actual directory structure to understand what exists
echo "=== Repository root structure ==="
ls -la | head -20
echo ""
echo "=== docs/ directory structure ==="
if [ -d "docs" ]; then
find docs -type f -name "*.md" -o -name "*.css" | head -20
else
echo "docs/ directory does not exist"
fi
echo ""
echo "=== assets/ directory structure ==="
if [ -d "assets" ]; then
find assets -type f -name "*.css" | head -20
else
echo "assets/ directory does not exist"
fi
echo ""
echo "=== Check actual SKILL.md content around lines 9-19 ==="
if [ -f ".claude/skills/ksef-hub-design/SKILL.md" ]; then
sed -n '9,19p' ".claude/skills/ksef-hub-design/SKILL.md"
else
echo "SKILL.md not found"
fiRepository: appunite/appunite-ksef-ex
Length of output: 3507
Remove references to non-existent docs/design_system/ paths (lines 11–17).
The entire docs/design_system/ directory referenced in lines 11–15 does not exist in the repository. Only assets/css/app.css exists and contains the design tokens. Lines 9–17 should be rewritten to correctly reference only paths that exist, or these instructions should be removed if the design system documentation has been moved or removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/ksef-hub-design/SKILL.md at line 11, The SKILL.md references
to the non-existent docs/design_system/ paths (notably
`docs/design_system/colors_and_type.css`) must be removed or corrected: update
the section around lines referencing design tokens to point only to the actual
file `assets/css/app.css` (or delete the block if documentation was removed),
and ensure any mention of "docs/design_system" is replaced with the correct path
or omitted; edit the SKILL.md entry that mentions
`docs/design_system/colors_and_type.css` so it either references
`assets/css/app.css` as the source of design tokens or removes the
nonexistent-path instructions entirely.
- Redact :token from Inspect and drop :invoice_id/:user_id from InvoicePublicToken.changeset cast to prevent FK mass-assignment (matches the Membership.changeset convention). - Reflow expires_at to satisfy mix format. - Cover :analyst role in the Membership valid-roles test. - Rename "reviewer" test descriptions to "approver" so names match the :approver role used in the bodies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extract transaction and event emission into helpers to satisfy Credo max nesting depth. Reorder aliases in public token test alphabetically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
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/team_member_live/show.ex (1)
111-119:⚠️ Potential issue | 🟠 MajorMove the block+revoke invariant into a context API and pass
actor_opts/1through it.This new flow only revokes tokens from this LiveView, so any other caller of
Companies.block_member/1can still leave public links active. It also callsCompanies.block_member/1withoutactor_opts(socket), so the block mutation loses actor attribution. Please make this a context-level operation and invoke it withactor_opts(socket)from here.Suggested direction
- case block_member_and_revoke_tokens(membership) do + case Companies.block_member_and_revoke_tokens(membership, actor_opts(socket)) do {:ok, {updated, count}} -> maybe_emit_tokens_revoked(membership, count, socket)Then move the transaction out of this LiveView and into the
Companies/domain layer so every block path enforces the same revocation rule.Based on learnings: LiveView handlers should pass
actor_opts(socket)with[user_id: ..., actor_label: ...]to context functions.Also applies to: 234-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/team_member_live/show.ex` around lines 111 - 119, Refactor the LiveView to call a new context API that performs the block+revoke transaction and accepts actor options: replace direct calls to the local block_member_and_revoke_tokens(membership) flow with a domain function like Companies.block_member_and_revoke_tokens(membership, actor_opts(socket)) that runs the transaction and returns {:ok, {updated_member, revoked_count}}; invoke that from handle_event("block_member", ...) and also update the other occurrence (lines referenced 234-248) to call the same Companies function, then use maybe_emit_tokens_revoked(membership, revoked_count, socket) as before to emit UI events — ensure you pass actor_opts(socket) so the block mutation is attributed to the actor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ksef_hub/invoices/public_token_test.exs`:
- Around line 182-194: The test currently calls
Companies.block_member(membership) but then manually calls
Invoices.delete_public_tokens_for_user(user.id, company.id), so it doesn't
validate that the block path itself revokes tokens; update the test to exercise
the real block hook by removing the explicit
Invoices.delete_public_tokens_for_user call and asserting that
Companies.block_member triggers token invalidation (keep using
Invoices.ensure_public_token and Invoices.get_invoice_by_public_token to assert
nil), or if the intent is to test explicit cleanup, rename the test to indicate
it asserts manual cleanup after blocking and keep the current calls; refer to
Companies.block_member, Invoices.ensure_public_token,
Invoices.delete_public_tokens_for_user, and Invoices.get_invoice_by_public_token
when making the change.
---
Outside diff comments:
In `@lib/ksef_hub_web/live/team_member_live/show.ex`:
- Around line 111-119: Refactor the LiveView to call a new context API that
performs the block+revoke transaction and accepts actor options: replace direct
calls to the local block_member_and_revoke_tokens(membership) flow with a domain
function like Companies.block_member_and_revoke_tokens(membership,
actor_opts(socket)) that runs the transaction and returns {:ok, {updated_member,
revoked_count}}; invoke that from handle_event("block_member", ...) and also
update the other occurrence (lines referenced 234-248) to call the same
Companies function, then use maybe_emit_tokens_revoked(membership,
revoked_count, socket) as before to emit UI events — ensure you pass
actor_opts(socket) so the block mutation is attributed to the actor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a5d5036-e647-412f-ab5f-54415b409331
📒 Files selected for processing (2)
lib/ksef_hub_web/live/team_member_live/show.extest/ksef_hub/invoices/public_token_test.exs
Keep the web layer thin: LiveView now delegates to Companies.block_member_and_revoke_tokens/2 instead of orchestrating the transaction locally. The public token test exercises the real block path and asserts the revoked count, no longer requiring a manual cleanup call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/team_member_live/show.ex (1)
202-214:⚠️ Potential issue | 🟠 MajorAdd
:viewerto the role allowlist here.The PR introduces a
:viewermembership role, butparse_role/1will currently reject"viewer"as invalid. That means this screen cannot save the new role.💡 Minimal fix
- `@valid_roles` ~w(owner admin accountant approver analyst)a + `@valid_roles` ~w(owner admin accountant viewer approver analyst)a🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/team_member_live/show.ex` around lines 202 - 214, The parse_role/1 function rejects the new "viewer" role because `@valid_roles` currently omits :viewer; update the `@valid_roles` declaration (the module attribute used by parse_role/1) to include :viewer (e.g., add :viewer to the ~w(...)a list) so that String.to_existing_atom(role) -> role_atom and the membership check role_atom in `@valid_roles` will accept the viewer role; no other changes to parse_role/1 are required.
🤖 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/companies.ex`:
- Around line 381-383: The expression formatting around the call to
Invoices.delete_public_tokens_for_user is not formatted per mix format; run the
formatter and break the call across lines so the assignment to count and the
tuple return {updated, count} follow project formatting rules — locate the block
using Invoices.delete_public_tokens_for_user, the local variable count, and the
returned tuple {updated, count}, then run mix format (or adjust line breaks) so
CI's mix format --check-formatted passes.
- Around line 378-381: block_member/2 currently uses TrackedRepo.update/2 inside
Repo.transaction/1 which can create activity entries that persist even if later
DB work (like Invoices.delete_public_tokens_for_user) rolls back; change the
flow so the DB writes inside the transaction use plain Repo or an Ecto.Multi (do
the membership update and token deletions together via Repo or Ecto.Multi in the
transaction) and do not call TrackedRepo.update/2 there, then after the
transaction succeeds emit the block + revocation events via Events.* (or call
the existing tracked/event helpers outside the transaction). Ensure references:
replace TrackedRepo.update in block_member/2 or add a non-tracked path used by
the Repo.transaction block, perform Invoices.delete_public_tokens_for_user
inside the same Repo/Ecto.Multi transaction, and move any Events emission to
after the transaction commit.
---
Outside diff comments:
In `@lib/ksef_hub_web/live/team_member_live/show.ex`:
- Around line 202-214: The parse_role/1 function rejects the new "viewer" role
because `@valid_roles` currently omits :viewer; update the `@valid_roles`
declaration (the module attribute used by parse_role/1) to include :viewer
(e.g., add :viewer to the ~w(...)a list) so that String.to_existing_atom(role)
-> role_atom and the membership check role_atom in `@valid_roles` will accept the
viewer role; no other changes to parse_role/1 are required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6044f41-d105-484c-9262-fc078d4ff1c6
📒 Files selected for processing (3)
lib/ksef_hub/companies.exlib/ksef_hub_web/live/team_member_live/show.extest/ksef_hub/invoices/public_token_test.exs
✅ Files skipped from review due to trivial changes (1)
- test/ksef_hub/invoices/public_token_test.exs
Comment on lines
+378
to
+381
| Repo.transaction(fn -> | ||
| case block_member(membership, opts) do | ||
| {:ok, updated} -> | ||
| count = Invoices.delete_public_tokens_for_user(membership.user_id, membership.company_id) |
There was a problem hiding this comment.
Avoid routing the membership update through TrackedRepo inside this transaction.
Line 379 calls block_member/2, which uses TrackedRepo.update/2, from inside a cross-entity Repo.transaction/1. If the token revocation later rolls back, you can end up with an activity entry for a block that never committed. Keep the DB writes in plain Repo/Ecto.Multi here, then emit the block + revocation events after the transaction succeeds.
As per coding guidelines, "Use Events.* directly instead of TrackedRepo when dealing with login/logout, sync triggers (Oban jobs), multi transactions, or cross-entity events that don't have a changeset".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/companies.ex` around lines 378 - 381, block_member/2 currently
uses TrackedRepo.update/2 inside Repo.transaction/1 which can create activity
entries that persist even if later DB work (like
Invoices.delete_public_tokens_for_user) rolls back; change the flow so the DB
writes inside the transaction use plain Repo or an Ecto.Multi (do the membership
update and token deletions together via Repo or Ecto.Multi in the transaction)
and do not call TrackedRepo.update/2 there, then after the transaction succeeds
emit the block + revocation events via Events.* (or call the existing
tracked/event helpers outside the transaction). Ensure references: replace
TrackedRepo.update in block_member/2 or add a non-tracked path used by the
Repo.transaction block, perform Invoices.delete_public_tokens_for_user inside
the same Repo/Ecto.Multi transaction, and move any Events emission to after the
transaction commit.
TrackedRepo.update inside Repo.transaction broadcasts via PubSub immediately; if a later step rolls back the transaction, the event still persists. Use plain Repo.update inside the transaction and emit team.member_blocked via Events only after commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The create migration already defines :token, so on fresh DBs (CI) the rename fails with "column token_digest does not exist". Wrap the rename in a DO block that only runs when the old column is still present. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
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
:viewermembership with read-only access to invoices and invoice details only; no dashboard, exports, payments, or team management. Dashboard route gated at the router level; viewer is redirected to/invoices.invoice.public_tokencolumn (no expiry, no ownership) with aninvoice_public_tokenstable. Tokens are scoped to(invoice, user), expire after 30 days, and are created atomically via upsert to prevent race conditions. Blocking a team member immediately invalidates all their shared links.company_idmust match the authenticated user's membership; viewer access isolation; public token expiry coverage.Migrations
20260418200001— createinvoice_public_tokenstable20260418200002— removepublic_tokencolumn frominvoices(old links stop working)20260418200003— add unique index on(invoice_id, user_id)to enforce one active token per pairKnown gaps (out of scope for this PR)
settings_generalroute has no permission gate (viewer can access/settings)Test plan
mix test— 2010 tests, 0 failures:viewerrole, confirm they can open invoices but are redirected from/dashboard,/settings/tokens,/settings/exports, etc.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Behavior