Feat/api token scoping#33
Conversation
Add company_id FK (restrict) to api_tokens table. Data migration associates existing tokens with their creator's owner membership company, deactivating orphaned tokens. Update ApiToken schema with belongs_to :company. Set FK IDs on struct (never cast) per project conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add create_api_token/3 (with owner authorization check), list_api_tokens/2, and revoke_api_token/3 — all scoped to user + company. Update validate_api_token/1 to preload company. Keep 2-arity versions for backward compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ApiAuth plug now assigns :current_company to conn from the token's preloaded company. API consumers no longer need to pass company_id as a query parameter. Tests updated to use company-scoped tokens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
InvoiceController and TokenController now derive company_id from conn.assigns.current_company (set by ApiAuth plug from token). No more company_id query parameter. OpenAPI specs updated to remove the parameter. Add controller-level integration tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TokenLive now shows only tokens for the current company, creates tokens scoped to that company, and revokes with company scope. Non-owner users are redirected with a flash message. Tests verify company isolation and access control. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughCompany-scoped API tokens were introduced: ApiToken now references Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Plug as API Auth Plug
participant Accounts as Accounts
participant Companies as Companies Context
participant DB as Database
Note over Client,DB: Company-scoped token creation & auth flow
Client->>Plug: POST /api/tokens (Bearer token / attrs)
Plug->>Accounts: validate_api_token(plain_token)
Accounts->>DB: query api_tokens |> preload(:company)
DB-->>Accounts: api_token (with company)
Accounts-->>Plug: {:ok, api_token}
Plug->>Plug: assign(:api_token), assign(:current_company)
Plug->>Accounts: create_api_token(user_id, company_id, attrs)
Accounts->>Companies: has_role?(user_id, company_id, "owner")
Companies-->>Accounts: true/false
alt owner
Accounts->>DB: insert api_token with company_id
DB-->>Accounts: {:ok, token}
Accounts-->>Client: 201 Created (token plaintext)
else not owner
Accounts-->>Client: {:error, :unauthorized} -> 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/controllers/api/token_controller.ex (1)
51-55:⚠️ Potential issue | 🟡 MinorAdd 403 response to OpenAPI spec.
The
createaction handles{:error, :unauthorized}with a 403 response (lines 77-80), but this is not documented in the operation spec.📝 Proposed fix
responses: %{ 201 => {"Created token", "application/json", Schemas.TokenCreatedResponse}, 401 => {"Unauthorized", "application/json", Schemas.ErrorResponse}, + 403 => {"Forbidden", "application/json", Schemas.ErrorResponse}, 422 => {"Validation error", "application/json", Schemas.ErrorResponse} }Based on learnings: "Every REST API controller action must have an
open_api_spexoperation spec" - the spec should accurately reflect all possible responses.
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/accounts.ex`:
- Around line 428-442: The API controller's delete action must enforce
owner-only access like the LiveView; before calling Accounts.revoke_api_token/3,
check that conn.assigns.current_role (or conn.assigns[:current_role]) equals
"owner" and return an appropriate unauthorized/forbidden response if not; mirror
the ownership check pattern used for create_api_token/3 (and the LiveView mount:
socket.assigns.current_role != "owner") so only owners can revoke tokens, then
proceed to call revoke_api_token(user_id, company_id, token_id) when the role
check passes.
In `@priv/repo/migrations/20260211000001_add_company_id_to_api_tokens.exs`:
- Around line 14-21: The UPDATE using a FROM join can pick any matching
membership for users who are owners of multiple companies; replace the
JOIN-based UPDATE with a deterministic subquery that selects a single membership
per api_token. Change the SET to use (SELECT company_id FROM memberships m WHERE
m.user_id = api_tokens.created_by_id AND m.role = 'owner' ORDER BY
m.inserted_at, m.id LIMIT 1) and keep the WHERE api_tokens.company_id IS NULL so
each api_tokens row gets a single, well-ordered company_id; reference the UPDATE
statement, table api_tokens, and memberships alias m when making this change.
🧹 Nitpick comments (7)
test/ksef_hub_web/controllers/api/invoice_controller_test.exs (4)
8-17: Consider extracting shared test helpers to a common module.This helper is duplicated across test files (
api_auth_test.exs,token_controller_test.exs). Consider creating a shared helper module intest/support/to reduce duplication.
19-23: Minor inconsistency with other test helpers.This helper omits the
"content-type"header that's included in the similar helper intoken_controller_test.exs. Since the POST endpoints here don't send request bodies, this works fine, but consider aligning for consistency.
73-83: Consider adding authorization boundary test for approve action.The
showtests include a "returns 404 for invoice from different company" case. For consistency and to ensure company scoping is enforced on mutations, consider adding a similar test for theapproveaction.💡 Example test case
test "returns 404 when approving invoice from different company", %{conn: conn} do %{token: token} = create_owner_with_token() other_company = insert(:company) invoice = insert(:invoice, company: other_company, type: "expense", status: "pending") assert_error_sent 404, fn -> conn |> api_conn(token) |> post("/api/invoices/#{invoice.id}/approve") end end
85-95: Same recommendation: consider adding authorization boundary test for reject action.Similar to the approve action, adding a test that verifies 404 is returned when attempting to reject an invoice from a different company would strengthen the test coverage for company scoping.
priv/repo/migrations/20260211000001_add_company_id_to_api_tokens.exs (1)
34-36: Use simpler column removal syntax.In
down/0, theremove/3withreferencesis unusual for removal. Simply useremove :company_idsince the foreign key constraint is automatically dropped with the column.Simplified removal
alter table(:api_tokens) do - remove :company_id, references(:companies, type: :binary_id), null: true + remove :company_id endlib/ksef_hub_web/controllers/api/invoice_controller.ex (1)
1-16: Missing@moduledocand function documentation.Per coding guidelines, every module must have a
@moduledocand every public function must have@docand@spec. Controller actions are public functions and should be documented.Add module documentation
defmodule KsefHubWeb.Api.InvoiceController do + `@moduledoc` """ + REST API controller for invoice operations. + + All endpoints derive the company context from the authenticated API token. + """ + use KsefHubWeb, :controller use OpenApiSpex.ControllerSpecsAs per coding guidelines: "Every module must have a
@moduledocdescribing the module's purpose" and "Every public function must have@docdocumentation".test/ksef_hub_web/controllers/api/token_controller_test.exs (1)
50-73: Consider adding test for 403 unauthorized response.The controller handles
{:error, :unauthorized}with a 403 response, but there's no test coverage for this case. Since the API auth derives company from the token, triggering this would require a scenario where the token's user loses owner role after token creation.📝 Example test for 403 case
test "returns 403 when user is no longer an owner", %{conn: conn} do %{user: user, company: company, token: token} = create_owner_with_token() # Demote user from owner to accountant membership = Repo.get_by!(Membership, user_id: user.id, company_id: company.id) membership |> Ecto.Changeset.change(%{role: "accountant"}) |> Repo.update!() conn = conn |> api_conn(token) |> post("/api/tokens", %{name: "New Token"}) assert conn.status == 403 assert Jason.decode!(conn.resp_body)["error"] =~ "owners" end
- Add owner check to revoke_api_token/3 mirroring create_api_token/3;
non-owners get {:error, :unauthorized}. TokenController delete
action returns 403 for non-owners.
- Replace JOIN-based UPDATE in data migration with a deterministic
subquery (ORDER BY inserted_at, id LIMIT 1) so users who own
multiple companies get a consistent single result per token.
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 (2)
lib/ksef_hub_web/controllers/api/token_controller.ex (2)
31-55:⚠️ Potential issue | 🟡 MinorAdd the 403 response to the create operation spec.
The code returns
:forbidden, but the OpenAPI responses omit it.Suggested update
responses: %{ 201 => {"Created token", "application/json", Schemas.TokenCreatedResponse}, 401 => {"Unauthorized", "application/json", Schemas.ErrorResponse}, + 403 => {"Forbidden", "application/json", Schemas.ErrorResponse}, 422 => {"Validation error", "application/json", Schemas.ErrorResponse} }
24-125:⚠️ Potential issue | 🟡 MinorAdd
@moduledocand@doc/@SPEC for controller actions.Index/create/delete are public but undocumented and untyped.
As per coding guidelines: "Every module must have a
@moduledocdescribing the module's purpose", "Every public function must have@docdocumentation", and "Every function (public and private) must have@spectype specification".
- Extract shared test helpers to ApiTestHelpers (DRY across 3 test files) - Add @moduledoc to InvoiceController and TokenController - Add 403 to create operation OpenAPI spec in TokenController - Add boundary tests for approve/reject from different company - Add 403 test for non-owner token creation - Make migration down/0 irreversible (data migration deactivates tokens) - Add data-testid attributes to token names, use has_element? in tests - Use api_conn/2 helper consistently across all API tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex`:
- Around line 119-136: Add `@doc` and `@spec` for the public controller actions
approve/2 and reject/2: write a short `@doc` string describing the action (e.g.,
"Approve an invoice for the current company" and "Reject an invoice for the
current company", mentioning key outcomes like success and possible error
responses) and add `@spec` declarations such as approve(conn :: Plug.Conn.t(),
params :: map()) :: Plug.Conn.t() and similarly for reject/2; place these
annotations immediately above the existing function definitions (approve/2 and
reject/2 referenced in the diff) and ensure the specs reflect the returned
Plug.Conn.t() after JSON responses and error statuses.
- Around line 71-76: The public controller action index/2 is missing `@doc` and
`@spec` declarations; add a concise `@doc` above def index/2 describing its purpose
(e.g., "List invoices for the current company, applying query filters") and add
a `@spec` line with types for conn and params and the Plug.Conn.t return (e.g.,
`@spec` index(Plug.Conn.t(), map()) :: Plug.Conn.t()); place these annotations
immediately above the index function that uses conn.assigns.current_company.id,
build_filters(params), Invoices.list_invoices/2 and invoice_json/1.
- Around line 196-214: The html/2 action calls
pdf_mod.generate_html(invoice.xml_content) without checking for nil; add a nil
check on invoice.xml_content (after fetching with Invoices.get_invoice!) and
short-circuit to return an appropriate JSON error/status (matching
invoice_pdf_controller behavior, e.g., put_status(:not_found) |> json(%{error:
"No XML content"})) when xml_content is nil, otherwise call
pdf_mod.generate_html/1 and keep the existing error logging using
sanitize_error(reason).
- Around line 235-256: Check for nil invoice.xml_content in the pdf/2 action and
return a proper error before calling pdf_mod.generate_html; update the pdf/2
function to explicitly handle the nil case (e.g., log sanitized error with
sanitize_error and return a 400/404 JSON response) and only call
pdf_mod.generate_html and pdf_mod.generate_pdf when invoice.xml_content is
present, referencing invoice.xml_content, pdf_mod.generate_html,
pdf_mod.generate_pdf, sanitize_filename and sanitize_error to locate code; also
add a concise `@doc` and `@spec` for pdf/2 describing parameters and return type.
- Around line 95-99: The show, approve, reject, html and pdf actions call
Invoices.get_invoice!/2 which raises Ecto.NoResultsError and yields a 500
instead of the documented 404; update the controller to use action_fallback
KsefHubWeb.FallbackController and switch each action to use the non-bang
Invoices.get_invoice(company_id, id) with pattern matching (nil -> {:error,
:not_found} ; invoice -> ...), or alternatively rescue the exception in each
action, and also add brief `@doc` and `@spec` annotations for each of show/2,
approve/2, reject/2, html/2 and pdf/2 to match the OpenAPI contract.
In `@priv/repo/migrations/20260211000001_add_company_id_to_api_tokens.exs`:
- Around line 5-33: Add a DB-level CHECK constraint to the api_tokens table to
prevent active tokens from having company_id = NULL: in the migration that
alters table(:api_tokens) (the same migration that adds company_id and runs the
data-migration UPDATEs), add an ALTER TABLE ... ADD CONSTRAINT (e.g.
api_tokens_active_requires_company_check) with condition "is_active = false OR
company_id IS NOT NULL" (or equivalently "NOT is_active OR company_id IS NOT
NULL") so any active token must have a non-null company_id; implement this using
an execute/ALTER TABLE statement after the data migrations so the constraint
will not fail during migration.
🧹 Nitpick comments (3)
lib/ksef_hub_web/controllers/api/invoice_controller.ex (1)
277-286: Consider extractingmaybe_put_date/3to a shared helper module.This function is duplicated from
lib/ksef_hub_web/live/invoice_live/index.ex(lines 73-78). Consider extracting it to a shared module likeKsefHubWeb.FilterHelpersto avoid duplication.lib/ksef_hub_web/live/token_live.ex (1)
85-101: Consider handling:unauthorizedin the revoke event.The
createevent handles{:error, :unauthorized}(lines 68-69), but therevokeevent only handles:not_foundand a generic error case. IfAccounts.revoke_api_token/3can return{:error, :unauthorized}, it would be caught by the generic{:error, _}clause with a less informative message.This may be intentional if owner-only access is already enforced by the mount guard, but for consistency and clarity, consider adding explicit handling:
♻️ Suggested improvement
case Accounts.revoke_api_token(user_id, company_id, id) do {:ok, revoked_token} -> {:noreply, socket |> stream_insert(:tokens, revoked_token) |> put_flash(:info, "Token revoked.")} + {:error, :unauthorized} -> + {:noreply, put_flash(socket, :error, "Only company owners can revoke tokens.")} + {:error, :not_found} -> {:noreply, put_flash(socket, :error, "Token not found.")} {:error, _} -> {:noreply, put_flash(socket, :error, "Failed to revoke token.")} endpriv/repo/migrations/20260211000001_add_company_id_to_api_tokens.exs (1)
4-34: Consider extracting SQL blocks to keepup/0short.
up/0is much longer than the 15‑line guideline; pulling the SQL into helpers will keep the migration more focused.♻️ Suggested refactor
def up do alter table(:api_tokens) do add :company_id, references(:companies, type: :binary_id, on_delete: :restrict) end create index(:api_tokens, [:company_id]) - execute """ - UPDATE api_tokens - SET company_id = ( - SELECT m.company_id - FROM memberships m - WHERE m.user_id = api_tokens.created_by_id - AND m.role = 'owner' - ORDER BY m.inserted_at, m.id - LIMIT 1 - ) - WHERE api_tokens.company_id IS NULL - """ - - execute """ - UPDATE api_tokens - SET is_active = false - WHERE company_id IS NULL - AND is_active = true - """ + execute backfill_company_id_sql() + execute deactivate_orphaned_tokens_sql() end + + defp backfill_company_id_sql do + """ + UPDATE api_tokens + SET company_id = ( + SELECT m.company_id + FROM memberships m + WHERE m.user_id = api_tokens.created_by_id + AND m.role = 'owner' + ORDER BY m.inserted_at, m.id + LIMIT 1 + ) + WHERE api_tokens.company_id IS NULL + """ + end + + defp deactivate_orphaned_tokens_sql do + """ + UPDATE api_tokens + SET is_active = false + WHERE company_id IS NULL + AND is_active = true + """ + endAs per coding guidelines, keep functions small and focused (< 15 lines ideally) in Elixir.
| def index(conn, params) do | ||
| with {:ok, company_id} <- require_company_id(conn, params) do | ||
| filters = build_filters(params) | ||
| invoices = Invoices.list_invoices(company_id, filters) | ||
| json(conn, %{data: Enum.map(invoices, &invoice_json/1)}) | ||
| end | ||
| company_id = conn.assigns.current_company.id | ||
| filters = build_filters(params) | ||
| invoices = Invoices.list_invoices(company_id, filters) | ||
| json(conn, %{data: Enum.map(invoices, &invoice_json/1)}) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing @doc and @spec for public function index/2.
Per coding guidelines, every public function must have @doc documentation and @spec type specification.
📝 Add documentation and type spec
+ `@doc` """
+ Lists invoices for the company associated with the authenticated API token.
+ """
+ `@spec` index(Plug.Conn.t(), map()) :: Plug.Conn.t()
def index(conn, params) doAs per coding guidelines: "Every public function must have @doc documentation" and "Every function (public and private) must have @spec type specification".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def index(conn, params) do | |
| with {:ok, company_id} <- require_company_id(conn, params) do | |
| filters = build_filters(params) | |
| invoices = Invoices.list_invoices(company_id, filters) | |
| json(conn, %{data: Enum.map(invoices, &invoice_json/1)}) | |
| end | |
| company_id = conn.assigns.current_company.id | |
| filters = build_filters(params) | |
| invoices = Invoices.list_invoices(company_id, filters) | |
| json(conn, %{data: Enum.map(invoices, &invoice_json/1)}) | |
| end | |
| `@doc` """ | |
| Lists invoices for the company associated with the authenticated API token. | |
| """ | |
| `@spec` index(Plug.Conn.t(), map()) :: Plug.Conn.t() | |
| def index(conn, params) do | |
| company_id = conn.assigns.current_company.id | |
| filters = build_filters(params) | |
| invoices = Invoices.list_invoices(company_id, filters) | |
| json(conn, %{data: Enum.map(invoices, &invoice_json/1)}) | |
| end |
🤖 Prompt for AI Agents
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex` around lines 71 - 76,
The public controller action index/2 is missing `@doc` and `@spec` declarations; add
a concise `@doc` above def index/2 describing its purpose (e.g., "List invoices
for the current company, applying query filters") and add a `@spec` line with
types for conn and params and the Plug.Conn.t return (e.g., `@spec`
index(Plug.Conn.t(), map()) :: Plug.Conn.t()); place these annotations
immediately above the index function that uses conn.assigns.current_company.id,
build_filters(params), Invoices.list_invoices/2 and invoice_json/1.
| def show(conn, %{"id" => id}) do | ||
| company_id = conn.assigns.current_company.id | ||
| invoice = Invoices.get_invoice!(company_id, id) | ||
| json(conn, %{data: invoice_json(invoice)}) | ||
| end |
There was a problem hiding this comment.
Unhandled exception when invoice not found; also missing @doc/@spec.
Invoices.get_invoice!/2 raises Ecto.NoResultsError when no record is found, but the OpenAPI spec promises a 404 JSON response. Without an action fallback or rescue, this will return a 500 error instead of the documented 404.
The same issue applies to approve/2, reject/2, html/2, and pdf/2.
🛠️ Proposed fix using action fallback or rescue
Option 1 - Add a fallback controller (recommended for consistency):
# In this controller, add:
action_fallback KsefHubWeb.FallbackController
# Then use non-bang version:
def show(conn, %{"id" => id}) do
company_id = conn.assigns.current_company.id
case Invoices.get_invoice(company_id, id) do
nil -> {:error, :not_found}
invoice -> json(conn, %{data: invoice_json(invoice)})
end
endOption 2 - Rescue the exception inline:
+ `@doc` """
+ Returns a single invoice by ID from the token's company.
+ """
+ `@spec` show(Plug.Conn.t(), map()) :: Plug.Conn.t()
def show(conn, %{"id" => id}) do
company_id = conn.assigns.current_company.id
- invoice = Invoices.get_invoice!(company_id, id)
- json(conn, %{data: invoice_json(invoice)})
+
+ try do
+ invoice = Invoices.get_invoice!(company_id, id)
+ json(conn, %{data: invoice_json(invoice)})
+ rescue
+ Ecto.NoResultsError ->
+ conn
+ |> put_status(:not_found)
+ |> json(%{error: "Invoice not found"})
+ end
end🤖 Prompt for AI Agents
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex` around lines 95 - 99,
The show, approve, reject, html and pdf actions call Invoices.get_invoice!/2
which raises Ecto.NoResultsError and yields a 500 instead of the documented 404;
update the controller to use action_fallback KsefHubWeb.FallbackController and
switch each action to use the non-bang Invoices.get_invoice(company_id, id) with
pattern matching (nil -> {:error, :not_found} ; invoice -> ...), or
alternatively rescue the exception in each action, and also add brief `@doc` and
`@spec` annotations for each of show/2, approve/2, reject/2, html/2 and pdf/2 to
match the OpenAPI contract.
| def approve(conn, %{"id" => id}) do | ||
| company_id = conn.assigns.current_company.id | ||
| invoice = Invoices.get_invoice!(company_id, id) | ||
|
|
||
| case Invoices.approve_invoice(invoice) do | ||
| {:ok, updated} -> | ||
| json(conn, %{data: invoice_json(updated)}) | ||
| case Invoices.approve_invoice(invoice) do | ||
| {:ok, updated} -> | ||
| json(conn, %{data: invoice_json(updated)}) | ||
|
|
||
| {:error, {:invalid_type, _type}} -> | ||
| conn | ||
| |> put_status(:unprocessable_entity) | ||
| |> json(%{error: "Only expense invoices can be approved"}) | ||
| {:error, {:invalid_type, _type}} -> | ||
| conn | ||
| |> put_status(:unprocessable_entity) | ||
| |> json(%{error: "Only expense invoices can be approved"}) | ||
|
|
||
| {:error, changeset} -> | ||
| conn | ||
| |> put_status(:unprocessable_entity) | ||
| |> json(%{error: changeset_errors(changeset)}) | ||
| end | ||
| {:error, changeset} -> | ||
| conn | ||
| |> put_status(:unprocessable_entity) | ||
| |> json(%{error: changeset_errors(changeset)}) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing @doc and @spec for approve/2 and reject/2.
Both public functions need documentation and type specs per coding guidelines.
📝 Add documentation and type specs
+ `@doc` """
+ Marks an expense invoice as approved.
+ """
+ `@spec` approve(Plug.Conn.t(), map()) :: Plug.Conn.t()
def approve(conn, %{"id" => id}) do+ `@doc` """
+ Marks an expense invoice as rejected.
+ """
+ `@spec` reject(Plug.Conn.t(), map()) :: Plug.Conn.t()
def reject(conn, %{"id" => id}) doAs per coding guidelines: "Every public function must have @doc documentation" and "Every function (public and private) must have @spec type specification".
Also applies to: 157-175
🤖 Prompt for AI Agents
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex` around lines 119 -
136, Add `@doc` and `@spec` for the public controller actions approve/2 and
reject/2: write a short `@doc` string describing the action (e.g., "Approve an
invoice for the current company" and "Reject an invoice for the current
company", mentioning key outcomes like success and possible error responses) and
add `@spec` declarations such as approve(conn :: Plug.Conn.t(), params :: map())
:: Plug.Conn.t() and similarly for reject/2; place these annotations immediately
above the existing function definitions (approve/2 and reject/2 referenced in
the diff) and ensure the specs reflect the returned Plug.Conn.t() after JSON
responses and error statuses.
| def html(conn, %{"id" => id}) do | ||
| company_id = conn.assigns.current_company.id | ||
| invoice = Invoices.get_invoice!(company_id, id) | ||
| pdf_mod = Application.get_env(:ksef_hub, :pdf_generator, KsefHub.Pdf) | ||
|
|
||
| case pdf_mod.generate_html(invoice.xml_content) do | ||
| {:ok, html_content} -> | ||
| conn | ||
| |> put_resp_content_type("text/html") | ||
| |> send_resp(200, html_content) | ||
| case pdf_mod.generate_html(invoice.xml_content) do | ||
| {:ok, html_content} -> | ||
| conn | ||
| |> put_resp_content_type("text/html") | ||
| |> send_resp(200, html_content) | ||
|
|
||
| {:error, reason} -> | ||
| Logger.error("HTML generation failed for invoice #{id}: #{sanitize_error(reason)}") | ||
| {:error, reason} -> | ||
| Logger.error("HTML generation failed for invoice #{id}: #{sanitize_error(reason)}") | ||
|
|
||
| conn | ||
| |> put_status(:internal_server_error) | ||
| |> json(%{error: "HTML generation failed"}) | ||
| end | ||
| conn | ||
| |> put_status(:internal_server_error) | ||
| |> json(%{error: "HTML generation failed"}) | ||
| end | ||
| end |
There was a problem hiding this comment.
Missing nil check for xml_content before HTML generation.
If an invoice has nil for xml_content, passing it to pdf_mod.generate_html/1 may cause unexpected behavior. The web controller (invoice_pdf_controller.ex) explicitly handles this case with a guard clause.
🛡️ Proposed fix to handle nil xml_content
def html(conn, %{"id" => id}) do
company_id = conn.assigns.current_company.id
invoice = Invoices.get_invoice!(company_id, id)
+
+ if is_nil(invoice.xml_content) do
+ conn
+ |> put_status(:unprocessable_entity)
+ |> json(%{error: "Invoice has no XML content"})
+ else
pdf_mod = Application.get_env(:ksef_hub, :pdf_generator, KsefHub.Pdf)
case pdf_mod.generate_html(invoice.xml_content) do
# ... existing code
end
+ end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def html(conn, %{"id" => id}) do | |
| company_id = conn.assigns.current_company.id | |
| invoice = Invoices.get_invoice!(company_id, id) | |
| pdf_mod = Application.get_env(:ksef_hub, :pdf_generator, KsefHub.Pdf) | |
| case pdf_mod.generate_html(invoice.xml_content) do | |
| {:ok, html_content} -> | |
| conn | |
| |> put_resp_content_type("text/html") | |
| |> send_resp(200, html_content) | |
| case pdf_mod.generate_html(invoice.xml_content) do | |
| {:ok, html_content} -> | |
| conn | |
| |> put_resp_content_type("text/html") | |
| |> send_resp(200, html_content) | |
| {:error, reason} -> | |
| Logger.error("HTML generation failed for invoice #{id}: #{sanitize_error(reason)}") | |
| {:error, reason} -> | |
| Logger.error("HTML generation failed for invoice #{id}: #{sanitize_error(reason)}") | |
| conn | |
| |> put_status(:internal_server_error) | |
| |> json(%{error: "HTML generation failed"}) | |
| end | |
| conn | |
| |> put_status(:internal_server_error) | |
| |> json(%{error: "HTML generation failed"}) | |
| end | |
| end | |
| def html(conn, %{"id" => id}) do | |
| company_id = conn.assigns.current_company.id | |
| invoice = Invoices.get_invoice!(company_id, id) | |
| if is_nil(invoice.xml_content) do | |
| conn | |
| |> put_status(:unprocessable_entity) | |
| |> json(%{error: "Invoice has no XML content"}) | |
| else | |
| pdf_mod = Application.get_env(:ksef_hub, :pdf_generator, KsefHub.Pdf) | |
| case pdf_mod.generate_html(invoice.xml_content) do | |
| {:ok, html_content} -> | |
| conn | |
| |> put_resp_content_type("text/html") | |
| |> send_resp(200, html_content) | |
| {:error, reason} -> | |
| Logger.error("HTML generation failed for invoice #{id}: #{sanitize_error(reason)}") | |
| conn | |
| |> put_status(:internal_server_error) | |
| |> json(%{error: "HTML generation failed"}) | |
| end | |
| end | |
| end |
🤖 Prompt for AI Agents
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex` around lines 196 -
214, The html/2 action calls pdf_mod.generate_html(invoice.xml_content) without
checking for nil; add a nil check on invoice.xml_content (after fetching with
Invoices.get_invoice!) and short-circuit to return an appropriate JSON
error/status (matching invoice_pdf_controller behavior, e.g.,
put_status(:not_found) |> json(%{error: "No XML content"})) when xml_content is
nil, otherwise call pdf_mod.generate_html/1 and keep the existing error logging
using sanitize_error(reason).
| def pdf(conn, %{"id" => id}) do | ||
| company_id = conn.assigns.current_company.id | ||
| invoice = Invoices.get_invoice!(company_id, id) | ||
| pdf_mod = Application.get_env(:ksef_hub, :pdf_generator, KsefHub.Pdf) | ||
|
|
||
| with {:ok, html_content} <- pdf_mod.generate_html(invoice.xml_content), | ||
| {:ok, pdf_binary} <- pdf_mod.generate_pdf(html_content) do | ||
| filename = sanitize_filename("#{invoice.invoice_number}.pdf") | ||
| with {:ok, html_content} <- pdf_mod.generate_html(invoice.xml_content), | ||
| {:ok, pdf_binary} <- pdf_mod.generate_pdf(html_content) do | ||
| filename = sanitize_filename("#{invoice.invoice_number}.pdf") | ||
|
|
||
| conn | ||
| |> put_resp_content_type("application/pdf") | ||
| |> put_resp_header("content-disposition", ~s(attachment; filename="#{filename}")) | ||
| |> send_resp(200, pdf_binary) | ||
| else | ||
| {:error, reason} -> | ||
| Logger.error("PDF generation failed for invoice #{id}: #{sanitize_error(reason)}") | ||
|
|
||
| conn | ||
| |> put_resp_content_type("application/pdf") | ||
| |> put_resp_header("content-disposition", ~s(attachment; filename="#{filename}")) | ||
| |> send_resp(200, pdf_binary) | ||
| else | ||
| {:error, reason} -> | ||
| Logger.error("PDF generation failed for invoice #{id}: #{sanitize_error(reason)}") | ||
|
|
||
| conn | ||
| |> put_status(:internal_server_error) | ||
| |> json(%{error: "PDF generation failed"}) | ||
| end | ||
| |> put_status(:internal_server_error) | ||
| |> json(%{error: "PDF generation failed"}) | ||
| end | ||
| end |
There was a problem hiding this comment.
Same nil xml_content issue in PDF generation; also missing @doc/@spec.
Same concern as the html/2 action—invoice.xml_content could be nil, which should be handled explicitly before attempting PDF generation.
🛡️ Proposed fix
+ `@doc` """
+ Generates a PDF rendering of the invoice from its FA(3) XML.
+ """
+ `@spec` pdf(Plug.Conn.t(), map()) :: Plug.Conn.t()
def pdf(conn, %{"id" => id}) do
company_id = conn.assigns.current_company.id
invoice = Invoices.get_invoice!(company_id, id)
+
+ if is_nil(invoice.xml_content) do
+ conn
+ |> put_status(:unprocessable_entity)
+ |> json(%{error: "Invoice has no XML content"})
+ else
pdf_mod = Application.get_env(:ksef_hub, :pdf_generator, KsefHub.Pdf)
# ... rest of function
+ end
end🤖 Prompt for AI Agents
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex` around lines 235 -
256, Check for nil invoice.xml_content in the pdf/2 action and return a proper
error before calling pdf_mod.generate_html; update the pdf/2 function to
explicitly handle the nil case (e.g., log sanitized error with sanitize_error
and return a 400/404 JSON response) and only call pdf_mod.generate_html and
pdf_mod.generate_pdf when invoice.xml_content is present, referencing
invoice.xml_content, pdf_mod.generate_html, pdf_mod.generate_pdf,
sanitize_filename and sanitize_error to locate code; also add a concise `@doc` and
`@spec` for pdf/2 describing parameters and return type.
| alter table(:api_tokens) do | ||
| add :company_id, references(:companies, type: :binary_id, on_delete: :restrict) | ||
| end | ||
|
|
||
| create index(:api_tokens, [:company_id]) | ||
|
|
||
| # Data migration: associate existing tokens with their creator's earliest owner | ||
| # membership company. Uses a deterministic subquery (ORDER BY + LIMIT 1) so | ||
| # users who own multiple companies get a consistent, single result per token. | ||
| # Tokens without a valid owner membership are deactivated below. | ||
| execute """ | ||
| UPDATE api_tokens | ||
| SET company_id = ( | ||
| SELECT m.company_id | ||
| FROM memberships m | ||
| WHERE m.user_id = api_tokens.created_by_id | ||
| AND m.role = 'owner' | ||
| ORDER BY m.inserted_at, m.id | ||
| LIMIT 1 | ||
| ) | ||
| WHERE api_tokens.company_id IS NULL | ||
| """ | ||
|
|
||
| execute """ | ||
| UPDATE api_tokens | ||
| SET is_active = false | ||
| WHERE company_id IS NULL | ||
| AND is_active = true | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the migration file
find . -name "*add_company_id_to_api_tokens*" -type fRepository: appunite/appunite-ksef-ex
Length of output: 139
🏁 Script executed:
# Check for mix.exs to determine Ecto/Phoenix version
fd "mix.exs" --type f | head -1 | xargs catRepository: appunite/appunite-ksef-ex
Length of output: 3415
🏁 Script executed:
# Search for other constraint usage patterns in migrations
fd "migrations" -type dRepository: appunite/appunite-ksef-ex
Length of output: 239
🏁 Script executed:
# List migrations directory and check other migrations for constraint patterns
ls -la priv/repo/migrations/ | head -20Repository: appunite/appunite-ksef-ex
Length of output: 1699
🏁 Script executed:
# Search for constraint usage in migrations
rg "create constraint" priv/repo/migrations/ -A 2Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Read the actual migration file to verify content
cat -n priv/repo/migrations/20260211000001_add_company_id_to_api_tokens.exsRepository: appunite/appunite-ksef-ex
Length of output: 1630
🏁 Script executed:
# Check for api_tokens schema definition
fd "api_tokens" --extension exRepository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Look for examples of constraints in schema files or any constraint documentation
rg "constraint\(" lib/ --type ex -A 2Repository: appunite/appunite-ksef-ex
Length of output: 95
🏁 Script executed:
# Find schema files
fd "schema" --type f | head -20Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Look for api_tokens in lib directory
rg "api_tokens" lib/ -A 3Repository: appunite/appunite-ksef-ex
Length of output: 1846
🏁 Script executed:
# Check Ecto.Migration documentation by looking at examples in the codebase
rg "create table\|alter table" priv/repo/migrations/ --max-count=5Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Get the full api_token schema
cat -n lib/ksef_hub/accounts/api_token.exRepository: appunite/appunite-ksef-ex
Length of output: 1575
🏁 Script executed:
# Check other migration files for constraint patterns
cat priv/repo/migrations/20260208000002_add_company_id_to_credentials.exsRepository: appunite/appunite-ksef-ex
Length of output: 694
🌐 Web query:
Ecto 3.13 create constraint check syntax PostgreSQL
💡 Result:
Ecto 3.13 (ecto_sql 3.13) migration syntax (PostgreSQL CHECK constraint)
Use create constraint/3 with the :check option:
def change do
create constraint("products", :price_must_be_positive,
check: "price > 0"
)
endYou can also pass options like prefix: ..., comment: ..., and (Postgres) validate: false. [1]
If you want Ecto to turn a DB CHECK violation into a changeset error, annotate the changeset with the same constraint name:
changeset
|> check_constraint(:price, name: :price_must_be_positive)[2]
Underlying PostgreSQL SQL form (for reference)
PostgreSQL CHECK constraints are of the form:
ALTER TABLE products
ADD CONSTRAINT price_must_be_positive CHECK (price > 0);(or inline in CREATE TABLE as price numeric CHECK (price > 0)). [3]
Sources: [1] [2] [3]
🏁 Script executed:
# Check how api_tokens are created in the codebase to verify if constraint would break anything
rg "ApiToken" lib/ -B 2 -A 5 | grep -E "changeset|create" -A 5 | head -40Repository: appunite/appunite-ksef-ex
Length of output: 2391
🏁 Script executed:
# Look for any tests that might create api_tokens without company_id
rg "api_token" test/ -i -B 2 -A 3 | head -60Repository: appunite/appunite-ksef-ex
Length of output: 3753
🏁 Script executed:
# Check how the single-arity create_api_token is being used - does it set is_active?
rg "create_api_token\(" lib/ -B 1 -A 3Repository: appunite/appunite-ksef-ex
Length of output: 2544
🏁 Script executed:
# Verify the full do_create_api_token implementation
sed -n '52,85p' lib/ksef_hub/accounts.exRepository: appunite/appunite-ksef-ex
Length of output: 1176
🏁 Script executed:
# Check the context around the 1-arity create_api_token - is it documented?
sed -n '30,52p' lib/ksef_hub/accounts.exRepository: appunite/appunite-ksef-ex
Length of output: 595
🏁 Script executed:
# Search for any future potential uses of the 1-arity create_api_token
rg "create_api_token\(.*,\s*[^,]+\s*\)" lib/ --type-listRepository: appunite/appunite-ksef-ex
Length of output: 458
🏁 Script executed:
# Check if there are any tests for the 1-arity create_api_token
rg "create_api_token.*\)" test/ | grep -v "create_api_token.*,.*," | head -20Repository: appunite/appunite-ksef-ex
Length of output: 1309
🏁 Script executed:
# Get the full accounts.ex to understand create_api_token implementation better
sed -n '44,80p' lib/ksef_hub/accounts.exRepository: appunite/appunite-ksef-ex
Length of output: 1231
🏁 Script executed:
# Check if the 1-arity create_api_token is actively used in production code (not tests)
rg "create_api_token\(" lib/ --type-list -vRepository: appunite/appunite-ksef-ex
Length of output: 458
🏁 Script executed:
# Verify what the changeset does with is_active
cat -n lib/ksef_hub/accounts/api_token.ex | sed -n '34,42p'Repository: appunite/appunite-ksef-ex
Length of output: 438
🏁 Script executed:
# Check the full create_api_token documentation and implementation
sed -n '44,75p' lib/ksef_hub/accounts.exRepository: appunite/appunite-ksef-ex
Length of output: 1118
🏁 Script executed:
# Find the line numbers for create_api_token functions
rg "def create_api_token" lib/ksef_hub/accounts.ex -nRepository: appunite/appunite-ksef-ex
Length of output: 172
🏁 Script executed:
# Get the lines around the create_api_token definitions
sed -n '80,130p' lib/ksef_hub/accounts.exRepository: appunite/appunite-ksef-ex
Length of output: 1791
🏁 Script executed:
# Get the actual create_api_token function definitions
sed -n '310,365p' lib/ksef_hub/accounts.exRepository: appunite/appunite-ksef-ex
Length of output: 2207
Add a DB constraint to prevent active tokens without company_id.
The migration currently allows the 2-arity create_api_token/2 function to persist tokens with company_id = NULL and is_active = true (the field defaults to true). A check constraint will enforce company scoping at the database layer.
🔒 Suggested constraint
execute """
UPDATE api_tokens
SET is_active = false
WHERE company_id IS NULL
AND is_active = true
"""
+
+ create constraint(:api_tokens, :api_tokens_active_company_id_required,
+ check: "company_id IS NOT NULL OR is_active = false"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| alter table(:api_tokens) do | |
| add :company_id, references(:companies, type: :binary_id, on_delete: :restrict) | |
| end | |
| create index(:api_tokens, [:company_id]) | |
| # Data migration: associate existing tokens with their creator's earliest owner | |
| # membership company. Uses a deterministic subquery (ORDER BY + LIMIT 1) so | |
| # users who own multiple companies get a consistent, single result per token. | |
| # Tokens without a valid owner membership are deactivated below. | |
| execute """ | |
| UPDATE api_tokens | |
| SET company_id = ( | |
| SELECT m.company_id | |
| FROM memberships m | |
| WHERE m.user_id = api_tokens.created_by_id | |
| AND m.role = 'owner' | |
| ORDER BY m.inserted_at, m.id | |
| LIMIT 1 | |
| ) | |
| WHERE api_tokens.company_id IS NULL | |
| """ | |
| execute """ | |
| UPDATE api_tokens | |
| SET is_active = false | |
| WHERE company_id IS NULL | |
| AND is_active = true | |
| """ | |
| alter table(:api_tokens) do | |
| add :company_id, references(:companies, type: :binary_id, on_delete: :restrict) | |
| end | |
| create index(:api_tokens, [:company_id]) | |
| # Data migration: associate existing tokens with their creator's earliest owner | |
| # membership company. Uses a deterministic subquery (ORDER BY + LIMIT 1) so | |
| # users who own multiple companies get a consistent, single result per token. | |
| # Tokens without a valid owner membership are deactivated below. | |
| execute """ | |
| UPDATE api_tokens | |
| SET company_id = ( | |
| SELECT m.company_id | |
| FROM memberships m | |
| WHERE m.user_id = api_tokens.created_by_id | |
| AND m.role = 'owner' | |
| ORDER BY m.inserted_at, m.id | |
| LIMIT 1 | |
| ) | |
| WHERE api_tokens.company_id IS NULL | |
| """ | |
| execute """ | |
| UPDATE api_tokens | |
| SET is_active = false | |
| WHERE company_id IS NULL | |
| AND is_active = true | |
| """ | |
| create constraint(:api_tokens, :api_tokens_active_company_id_required, | |
| check: "company_id IS NOT NULL OR is_active = false" | |
| ) |
🤖 Prompt for AI Agents
In `@priv/repo/migrations/20260211000001_add_company_id_to_api_tokens.exs` around
lines 5 - 33, Add a DB-level CHECK constraint to the api_tokens table to prevent
active tokens from having company_id = NULL: in the migration that alters
table(:api_tokens) (the same migration that adds company_id and runs the
data-migration UPDATEs), add an ALTER TABLE ... ADD CONSTRAINT (e.g.
api_tokens_active_requires_company_check) with condition "is_active = false OR
company_id IS NOT NULL" (or equivalently "NOT is_active OR company_id IS NOT
NULL") so any active token must have a non-null company_id; implement this using
an execute/ALTER TABLE statement after the data migrations so the constraint
will not fail during migration.
1 participant
Summary by CodeRabbit