Feat/user scoped certificates

[emilwojtaszek]

Summary by CodeRabbit

• New Features

  • Certificates are now managed per-user; UI surfaces show user-level certificate data and actions.

• Bug Fixes & Improvements

  • Authentication and sync check for owner certificates and skip gracefully if none exist; dashboard and company list labels clarified.

• Migrations

  • Added migrations to create user_certificates, migrate existing data, and remove legacy certificate columns.

• Tests

  • Added/updated tests for user certificate lifecycle and related flows.

• Documentation

  • Implementation plan updated to mark numerous checklist items complete.

[coderabbitai]

📝 Walkthrough

Walkthrough

Moves certificate storage from company-level credentials to per-user UserCertificate records: adds schema + migrations, migrates data, removes certificate columns from credentials, updates credentials API, LiveViews, auth/sync workers, certificate parsing, factories, and tests to operate on user-scoped certificates.

Changes

Cohort / File(s)	Summary
Documentation docs/implementation-plan.md	Marked many checklist items done; editorial updates only.
Credentials context lib/ksef_hub/credentials.ex	Added user-certificate APIs: get_active_user_certificate/1, create_user_certificate/2, replace_active_user_certificate/2, get_certificate_for_company/1, deactivate_user_certificate/1 and transactional replacement logic.
Credential schema lib/ksef_hub/credentials/credential.ex	Removed certificate fields (certificate_data_encrypted, certificate_password_encrypted, certificate_expires_at, certificate_subject) and consolidated cast fields via @cast_fields.
New UserCertificate schema lib/ksef_hub/credentials/user_certificate.ex	Adds UserCertificate Ecto schema (encrypted data, password, subject, not_before/not_after, fingerprint, is_active, belongs_to :user) with changeset and unique active-per-user constraint.
Auth & Sync workers lib/ksef_hub/ksef_client/auth_worker.ex, lib/ksef_hub/sync/sync_worker.ex	Refactored to load owner user certificate via Credentials.get_certificate_for_company/1; added load_certificate/1, verify_owner_certificate/1; handle {:error, :no_certificate} short-circuit and logging.
LiveViews — certificate UI lib/ksef_hub_web/live/certificate_live.ex, lib/ksef_hub_web/live/dashboard_live.ex, lib/ksef_hub_web/live/company_live/index.ex	Switched UI and flows to user-scoped certificates: renamed helpers, updated upload/save/remove to create/deactivate UserCertificate, adapted metadata (expires_at → not_after) and display labels/status.
Migrations priv/repo/migrations/20260210100001_create_user_certificates.exs, ...100002_migrate_cert_data_to_user_certificates.exs, ...100003_remove_cert_columns_from_credentials.exs	Create user_certificates table, migrate existing certificate data (joined via owner membership), and remove certificate columns from ksef_credentials.
Certificate parsing behaviour lib/ksef_hub/credentials/certificate_info/behaviour.ex, lib/ksef_hub/credentials/certificate_info/openssl.ex	Behaviour and OpenSSL parser updated to include not_before (validity start) in returned metadata; parser returns {:error, :parse_failed} on parse errors.
Tests & Factory updates test/support/factory.ex, test/ksef_hub/credentials/user_certificate_test.exs, test/ksef_hub/credentials_test.exs, test/ksef_hub/ksef_client/auth_worker_test.exs, test/ksef_hub/sync/sync_worker_test.exs, test/ksef_hub_web/live/certificate_live_test.exs	Added user_certificate_factory/0, new unit tests for UserCertificate changeset and Credentials API, and updated tests to exercise user-scoped certificate flows and new error paths.
Misc / UI wording lib/ksef_hub_web/live/company_live/index.ex , mix.exs, mix.lock	UI label/status text tweaks and test-related dependency/lockfile updates.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant LiveView as Certificate LiveView
    participant Credentials as Credentials Module
    participant DB as Database
    participant AuthWorker as AuthWorker

    rect rgba(150,200,100,0.5)
    Note over User,AuthWorker: Per-user certificate flow
    User->>LiveView: Upload certificate (p12 / key+crt)
    LiveView->>Credentials: create_user_certificate(user, attrs)
    Credentials->>DB: INSERT user_certificates (user_id, encrypted data, is_active=true)
    DB-->>Credentials: OK
    LiveView->>LiveView: ensure placeholder exists for company
    AuthWorker->>Credentials: get_certificate_for_company(company_id)
    Credentials->>DB: join memberships -> user_certificates where is_active=true
    DB-->>Credentials: user_certificate
    Credentials-->>AuthWorker: user_certificate (encrypted)
    AuthWorker->>AuthWorker: decrypt certificate and obtain token from KSeF
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• Feat/key crt upload #27 — Overlaps LiveView certificate upload/save/refactor changes and tests.
• fix: apply review fixes from stacked PR review (#1-#19) #20 — Similar refactor of certificate storage/fields and credentials surface.
• Feat/certificate page redesign #28 — Related changes to CertificateInfo behaviour and OpenSSL parsing (adds not_before).

Poem

🐰 I hopped through tables, fields in tow,

Lifted certs from companies to who they know.
Migrations stitched the old to new,
LiveViews bloom with clearer view.
A rabbit cheers — per-user we go!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Feat/user scoped certificates' clearly and directly summarizes the main change: migrating certificate storage from company-level credentials to user-level certificates, which is the primary focus across all modified files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/user-scoped-certificates

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

lib/ksef_hub_web/live/certificate_live.ex (2)

251-263: Inconsistent field naming between openssl.ex and UserCertificate schema.

The extract_certificate_info/2 function maps info.expires_at to :not_after, but openssl.ex now returns not_before alongside expires_at. Consider using consistent naming throughout the pipeline to reduce confusion:

   defp extract_certificate_info(cert_data, password) do
     case certificate_info().extract(cert_data, password) do
       {:ok, info} ->
         %{certificate_subject: info.subject}
         |> put_if_present(:not_before, Map.get(info, :not_before))
-        |> put_if_present(:not_after, Map.get(info, :expires_at))
+        |> put_if_present(:not_after, info.expires_at)

       {:error, _reason} ->
         Logger.warning("Failed to extract certificate info")
         %{}
     end
   end

The current code works correctly but uses Map.get/2 inconsistently (pattern matching for not_before would also work since it's now part of the returned map).

---

214-233: Consider clarifying intent of ensure_credential_exists.

This helper silently succeeds even when credential creation fails (returns :ok after logging a warning). This is intentional for non-blocking behavior, but consider documenting this design decision or returning the result for the caller to decide:

-  `@spec` ensure_credential_exists(KsefHub.Companies.Company.t()) :: :ok
+  `@spec` ensure_credential_exists(KsefHub.Companies.Company.t()) :: :ok | {:error, term()}
   defp ensure_credential_exists(company) do
     case Credentials.get_active_credential(company.id) do
       nil ->
         case Credentials.replace_active_credential(company.id, %{}) do
           {:ok, _credential} ->
             :ok

           {:error, changeset} ->
             Logger.warning(
               "Failed to create credential for company #{company.id}: #{inspect(changeset.errors)}"
             )
-
-            :ok
+            {:error, changeset}
         end

       _credential ->
         :ok
     end
   end

However, if the current behavior (always proceed, just log warnings) is intentional, adding a brief comment would help future maintainers understand this choice.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🤖 Fix all issues with AI agents

In `@lib/ksef_hub/credentials.ex`:
- Around line 198-205: The get_certificate_for_company/1 query can raise
Ecto.MultipleResultsError when multiple owner memberships exist; modify the
query in get_certificate_for_company to defensively limit results (e.g., add
limit(1)) and add a deterministic order_by (for example order_by: [desc:
uc.inserted_at] or uc.id) before calling Repo.one(), so it always returns a
single, predictable UserCertificate even if multiple owner memberships exist.

In
`@priv/repo/migrations/20260210100002_migrate_cert_data_to_user_certificates.exs`:
- Around line 10-31: The migration's up/execute SQL inserts
certificate_password_encrypted (non-nullable) from ksef_credentials which is
nullable; add a NULL-guard by extending the WHERE clause in the execute block
(inside the up function) to include "AND kc.certificate_password_encrypted IS
NOT NULL" so only rows with both certificate_data_encrypted and
certificate_password_encrypted are inserted and the INSERT into
user_certificates won't fail.

In
`@priv/repo/migrations/20260210100003_remove_cert_columns_from_credentials.exs`:
- Around line 10-16: The migration's change/0 is irreversible because the remove
calls inside alter table(:ksef_credentials) don't provide full schema info for
rollback; update each remove invocation to the three-arity form that includes
the column type and options so Ecto can recreate them on rollback (e.g. replace
remove :certificate_data_encrypted, :binary with
remove(:certificate_data_encrypted, :binary, null: true) and do the same for
:certificate_password_encrypted, :certificate_subject, and
:certificate_expires_at), leaving the change/0 and alter block intact.

In `@test/ksef_hub/ksef_client/auth_worker_test.exs`:
- Around line 22-39: Replace inline maps passed to Credentials.create_credential
in this test with factory-generated params via params_for(:credential, ...) so
credential attributes follow factory defaults; specifically, build params using
params_for(:credential, nip: company.nip, company_id: company.id, is_active:
true) and pass that to Credentials.create_credential where the current inline
map is used (this applies to the three create_credential calls in the file),
leaving the insert(:user_certificate, ...) usage intact.

🧹 Nitpick comments (5)

docs/implementation-plan.md (1)

152-157: Refine LiveView test plan into smaller, staged files.

The plan calls out a single LiveView test file; consider splitting into smaller, isolated files and sequencing content-only checks before interaction tests to match the project’s LiveView testing strategy.

Based on learnings: "Come up with a step-by-step test plan for LiveView tests that splits major test cases into small, isolated files, starting with simpler tests that verify content exists before adding interaction tests".

lib/ksef_hub_web/live/dashboard_live.ex (1)

53-72: Consider aligning cert_active with certificate existence.

Currently cert_active is based solely on credential.is_active, but certificate data now comes from user_cert. If a credential exists without an owner certificate uploaded, the dashboard will show "Active" but no expiration date, which may confuse users.

Should cert_active also verify that user_cert exists?

cert_active: credential != nil && credential.is_active && user_cert != nil

test/ksef_hub/credentials_test.exs (1)

151-312: Use factory-driven test data for user certificate attributes.

Several tests build inline attribute maps; using params_for(:user_certificate) keeps test data consistent with the factory definition and reduces maintenance burden when defaults change.

Example refactor

-      attrs = %{
-        certificate_data_encrypted: "enc-data",
-        certificate_password_encrypted: "enc-pass",
-        certificate_subject: "CN=Test",
-        not_before: ~D[2026-01-01],
-        not_after: ~D[2028-01-01],
-        fingerprint: "AA:BB",
-        is_active: true
-      }
+      attrs =
+        :user_certificate
+        |> params_for()
+        |> Map.drop([:user_id])
+        |> Map.merge(%{
+          certificate_subject: "CN=Test",
+          not_before: ~D[2026-01-01],
+          not_after: ~D[2028-01-01],
+          fingerprint: "AA:BB",
+          is_active: true
+        })

test/ksef_hub/credentials/user_certificate_test.exs (1)

1-135: Use factory params to reduce inline test data duplication.

The user_certificate_factory exists and provides all needed fields. Replace inline maps with params_for(:user_certificate) to align with testing standards and keep test data centralized. Note: These tests validate changeset behavior with minimal required fields, so inline maps are understandable for clarity—refactoring is optional but recommended.

♻️ Example refactor

-      changeset =
-        %UserCertificate{user_id: user.id}
-        |> UserCertificate.changeset(%{
-          certificate_data_encrypted: "encrypted-data",
-          certificate_password_encrypted: "encrypted-pass",
-          is_active: true
-        })
+      changeset =
+        %UserCertificate{user_id: user.id}
+        |> UserCertificate.changeset(
+          params_for(:user_certificate)
+          |> Map.drop([:user_id])
+        )

lib/ksef_hub_web/live/certificate_live.ex (1)

214-224: Consider handling failures in ensure_credential_exists.

The result of Credentials.replace_active_credential/2 is discarded. If credential creation fails, this will silently return :ok, potentially leaving the system in an inconsistent state where a user certificate exists but no company credential exists for sync operations.

♻️ Suggested improvement: log on failure

 `@spec` ensure_credential_exists(KsefHub.Companies.Company.t()) :: :ok
 defp ensure_credential_exists(company) do
   case Credentials.get_active_credential(company.id) do
     nil ->
-      Credentials.replace_active_credential(company.id, %{})
-      :ok
+      case Credentials.replace_active_credential(company.id, %{}) do
+        {:ok, _credential} ->
+          :ok
+
+        {:error, changeset} ->
+          Logger.warning("Failed to create credential for company #{company.id}: #{inspect(changeset.errors)}")
+          :ok
+      end

     _credential ->
       :ok
   end
 end

[coderabbitai]

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/credentials/certificate_info/openssl.ex (1)

99-117: ⚠️ Potential issue | 🟡 Minor

Update extract/2 docs/spec to include not_before.
parse_certificate/1 now returns not_before (Line 99-113), but extract/2 still documents/specs only expires_at, which can mislead callers.

🛠️ Suggested update for `extract/2` `@doc/`@spec

-  `@doc` """
-  Extracts the subject and expiry date from a PKCS12 binary.
+  `@doc` """
+  Extracts certificate metadata (subject, validity start, expiry) from a PKCS12 binary.
@@
-  Returns `{:ok, %{subject: String.t(), expires_at: Date.t()}}` on success,
+  Returns `{:ok, %{subject: String.t(), not_before: Date.t(), expires_at: Date.t()}}` on success,
@@
-  `@spec` extract(binary(), String.t()) ::
-          {:ok, %{subject: String.t(), expires_at: Date.t()}} | {:error, term()}
+  `@spec` extract(binary(), String.t()) ::
+          {:ok, %{subject: String.t(), not_before: Date.t(), expires_at: Date.t()}} | {:error, term()}

🤖 Fix all issues with AI agents

In
`@priv/repo/migrations/20260210100002_migrate_cert_data_to_user_certificates.exs`:
- Around line 36-38: The current down/0 performs a destructive "DELETE FROM
user_certificates" which will remove any post-migration user data; fix by either
making the migration irreversible (change down/0 in this migration to raise an
Ecto.MigrationError with a clear message stating the migration cannot be rolled
back) or by implementing safe rollbacks: add a tracking column (e.g.,
migrated_from_credential_id) during up/0, populate it for migrated rows, and
change down/0 to delete only rows where migrated_from_credential_id is NOT NULL
(or matches the source IDs), ensuring only originally migrated records are
removed; reference the migration's down/0 function and the user_certificates
table and migrated_from_credential_id when making the change.

🧹 Nitpick comments (2)

lib/ksef_hub/credentials.ex (1)

144-150: Consider adding defensive query hardening for consistency.

Unlike get_active_credential/1 (lines 27-31) and get_certificate_for_company/1 (lines 205-206), this function relies solely on the unique constraint without defensive order_by + limit(1). While the constraint should prevent duplicates, adding defensive hardening would provide consistency and guard against constraint violations or race conditions.

♻️ Optional defensive hardening

   def get_active_user_certificate(user_id) do
     UserCertificate
     |> where([uc], uc.user_id == ^user_id and uc.is_active == true)
+    |> order_by([uc], desc: uc.inserted_at)
+    |> limit(1)
     |> Repo.one()
   end

test/ksef_hub_web/live/certificate_live_test.exs (1)

80-81: Consider using element selectors instead of raw HTML matching.

These assertions test text content via render(view) =~, which works but is less resilient to markup changes. If possible, add data-testid or ID attributes to the heading and subject elements, then use has_element?/3 with selectors.

However, this is acceptable for verifying displayed text when specific element IDs aren't available.