Feat/certificate page redesign#28
Conversation
Extract certificate subject (CN, O, etc.) and expiry date from PKCS12 files using openssl CLI + Erlang :public_key module. Follows the same secure temp file pattern as Pkcs12Converter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Call CertificateInfo.extract/2 after receiving PKCS12 data to populate certificate_subject and certificate_expires_at fields. Extraction is best-effort — failures are logged but don't block the upload. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace cryptic openssl exit codes with user-friendly messages. Specific message for exit code 1 (wrong passphrase / mismatched files). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace accept: :any with specific extensions (.key, .pem for private keys and .crt, .pem, .cer for certificates). Register MIME types for these extensions in config. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change default upload_mode from :p12 to :key_crt and swap button order so .key + .crt appears first. Most users have separate key and certificate files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the dual Active Certificate / All Certificates sections with a single "Current Certificate" card showing NIP, Subject, Expires, Last Sync, and Uploaded date. Add empty state when no certificate exists. The upload form is now toggleable via "Replace Certificate" / "Cancel" buttons and auto-hides after successful upload. The "Remove" button replaces the old "Deactivate" action. Remove: stream(:credentials), has_credentials assign, active_badge component, All Certificates table. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace fragile elem/2 tuple access with proper Record.defrecord macros for OTPCertificate and OTPTBSCertificate. Simplify dead branch in extract_pem retry logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds certificate metadata extraction (behaviour + OpenSSL implementation), registers additional certificate MIME types, introduces an Oban Auth worker to perform post-upload XADES authentication, refactors the certificate LiveView for upload/remove/metadata display, and adds tests and mocks for these flows. Changes
Sequence Diagram(s)sequenceDiagram
participant LiveView as "LiveView\n(Certificate UI)"
participant DB as "Credential Store"
participant AuthWorker as "AuthWorker (Oban)"
participant Decrypt as "Decryption (local)"
participant KsefAuth as "KsefClient.Auth"
participant TokenMgr as "TokenManager"
LiveView->>DB: save credential (encrypted p12/password) + metadata
LiveView->>AuthWorker: enqueue job(company_id)
AuthWorker->>DB: load_credential(company_id)
DB-->>AuthWorker: credential (encrypted)
AuthWorker->>Decrypt: decrypt p12_data
AuthWorker->>Decrypt: decrypt password
Decrypt-->>AuthWorker: p12_data, password
AuthWorker->>KsefAuth: authenticate(p12_data, password)
KsefAuth-->>AuthWorker: challenge/sign/poll -> tokens
AuthWorker->>TokenMgr: store_tokens(tokens, validity)
TokenMgr-->>AuthWorker: :ok
AuthWorker-->>AuthWorker: return :ok
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/certificate_live.ex (1)
261-266:⚠️ Potential issue | 🔴 CriticalWrap template content with
<Layouts.app>to render sidebar, navigation, and flash messages.The template must begin with
<Layouts.app flash={@flash} current_user={@current_user} current_company={@current_company} companies={@companies} current_path={@current_path}>instead of starting directly with<.header>. The root layout only provides the HTML document structure; the application layout component provides the sidebar navigation, flash message display, and main content container. All required assigns are available from theLiveAuthon_mount hook.
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/credentials/certificate_info/openssl.ex`:
- Around line 95-97: The Logger.warning call currently logs the full OpenSSL
output in the clause matching {:ok, {output, exit_code}}; change it to avoid
logging any certificate or error output by removing `output` from the log and
only logging the exit_code (or a sanitized/static message) in the Logger.warning
invocation (i.e., update the clause with {:ok, {output, exit_code}} -> ... so
Logger.warning only includes exit_code and not the `output` variable).
- Around line 15-16: The formatting in the OpenSSL credential file is off
causing CI to fail; run `mix format` for the module (file containing
Record.defrecord(:otp_certificate, ...) and
Record.defrecord(:otp_tbscertificate, ...)) or manually reformat so the two
Record.defrecord lines conform to Elixir formatter style (proper
spacing/indentation and line breaks) and then commit the formatted file so CI
passes.
🧹 Nitpick comments (1)
test/ksef_hub/credentials/certificate_info_test.exs (1)
1-7: Consider renaming test file to mirror source path.The test file is at
test/ksef_hub/credentials/certificate_info_test.exsbut testsKsefHub.Credentials.CertificateInfo.Openssllocated atlib/ksef_hub/credentials/certificate_info/openssl.ex. Per coding guidelines, test files should mirror source paths.Consider renaming to
test/ksef_hub/credentials/certificate_info/openssl_test.exsfor consistency, or update the module name to reflect it tests multiple implementations.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/credentials/certificate_info/openssl.ex`:
- Around line 134-137: The rescue block currently logs exception details via
Logger.warning("Failed to parse certificate: #{inspect(e)}") which can leak
certificate data; change the log to a static message (e.g.
Logger.warning("Failed to parse certificate")) and preserve the existing return
{:error, :parse_failed} so no exception/binary contents are logged; update the
rescue in the same function where Logger.warning and {:error, :parse_failed}
appear (the certificate parse/rescue block) to remove inspect(e) from the log.
🧹 Nitpick comments (1)
lib/ksef_hub/credentials/certificate_info/openssl.ex (1)
55-100: Reduce duplication in OpenSSL invocation helpers.
extract_pem/2andretry_without_legacy/2duplicate the same Task/timeout boilerplate, inflating both functions beyond the small-function guideline. Consider extracting a shared helper to keep each function focused.♻️ Suggested refactor
+ defp run_openssl(args) do + task = Task.async(fn -> System.cmd("openssl", args, stderr_to_stdout: true) end) + + case Task.yield(task, 30_000) || Task.shutdown(task, :brutal_kill) do + {:ok, {output, 0}} -> {:ok, output} + {:ok, {output, exit_code}} -> {:error, {:openssl_failed, output, exit_code}} + nil -> {:error, :timeout} + end + end + defp extract_pem(p12_path, pass_path) do args = [ "pkcs12", "-in", p12_path, "-passin", "file:#{pass_path}", "-clcerts", "-nokeys", "-legacy" ] - task = Task.async(fn -> System.cmd("openssl", args, stderr_to_stdout: true) end) - - case Task.yield(task, 30_000) || Task.shutdown(task, :brutal_kill) do - {:ok, {output, 0}} -> - {:ok, output} - - {:ok, {_output, _exit_code}} -> - # Retry without -legacy flag for older OpenSSL versions - retry_without_legacy(p12_path, pass_path) - - nil -> - Logger.warning("openssl pkcs12 info extraction timed out") - {:error, :timeout} - end + case run_openssl(args) do + {:ok, output} -> + {:ok, output} + + {:error, {:openssl_failed, _output, _exit_code}} -> + retry_without_legacy(p12_path, pass_path) + + {:error, :timeout} -> + Logger.warning("openssl pkcs12 info extraction timed out") + {:error, :timeout} + end end defp retry_without_legacy(p12_path, pass_path) do args = [ "pkcs12", "-in", p12_path, "-passin", "file:#{pass_path}", "-clcerts", "-nokeys" ] - task = Task.async(fn -> System.cmd("openssl", args, stderr_to_stdout: true) end) - - case Task.yield(task, 30_000) || Task.shutdown(task, :brutal_kill) do - {:ok, {output, 0}} -> - {:ok, output} - - {:ok, {output, exit_code}} -> - Logger.warning("openssl pkcs12 info extraction failed (exit #{exit_code}): #{output}") - {:error, {:openssl_failed, exit_code}} - - nil -> - Logger.warning("openssl pkcs12 info extraction timed out") - {:error, :timeout} - end + case run_openssl(args) do + {:ok, output} -> + {:ok, output} + + {:error, {:openssl_failed, output, exit_code}} -> + Logger.warning("openssl pkcs12 info extraction failed (exit #{exit_code}): #{output}") + {:error, {:openssl_failed, exit_code}} + + {:error, :timeout} -> + Logger.warning("openssl pkcs12 info extraction timed out") + {:error, :timeout} + end endAs per coding guidelines, "Keep functions small and focused (< 15 lines ideally) in Elixir."
After a certificate upload, the SyncWorker would fail with reauth_required because no initial XADES authentication was triggered. Add AuthWorker (Oban job) that runs the full auth flow after upload and stores tokens via TokenManager. Also: stop logging openssl output in cert info extraction, rename certificate_info test to mirror source path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub_web/live/certificate_live.ex`:
- Around line 225-235: In extract_certificate_info, avoid logging the
potentially sensitive reason returned from certificate_info().extract; replace
Logger.warning("Failed to extract certificate info: #{inspect(reason)}") with a
static, non-sensitive message such as Logger.warning("Failed to extract
certificate info") so extract_certificate_info and any callers of
certificate_info().extract no longer emit certificate or parsing details to
logs.
🧹 Nitpick comments (2)
lib/ksef_hub_web/live/certificate_live.ex (1)
247-252: Consider handlingenqueue_authfailures.The return value of
enqueue_auth/1is ignored. If Oban insertion fails, the user receives a success flash but authentication won't be attempted. Consider logging or notifying on failure.💡 Suggested improvement
- `@spec` enqueue_auth(Ecto.UUID.t()) :: {:ok, Oban.Job.t()} | {:error, term()} - defp enqueue_auth(company_id) do - %{company_id: company_id} - |> AuthWorker.new() - |> Oban.insert() + `@spec` enqueue_auth(Ecto.UUID.t()) :: :ok + defp enqueue_auth(company_id) do + case %{company_id: company_id} |> AuthWorker.new() |> Oban.insert() do + {:ok, _job} -> :ok + {:error, reason} -> + Logger.error("Failed to enqueue auth job for company #{company_id}") + :ok + end endtest/ksef_hub/credentials/certificate_info/openssl_test.exs (1)
1-8: Considerasync: falsefor integration tests using external processes.This test spawns OpenSSL processes via
Pkcs12ConverterandOpenssl.extract. WhileSecureTempgenerates unique paths, running multiple integration tests concurrently could cause resource contention or flaky behavior.💡 Suggested change
- use ExUnit.Case, async: true + use ExUnit.Case, async: false
Remove potentially sensitive data from log messages in certificate parsing and extraction. Extract shared run_openssl/1 helper to eliminate Task/timeout boilerplate duplication. Handle enqueue_auth failures with explicit error logging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub_web/live/certificate_live.ex (1)
85-101:⚠️ Potential issue | 🟠 MajorVerify authorization before removing credential.
The
remove_certificatehandler validates the UUID and fetches the credential, but doesn't verify the credential belongs tocurrent_company. A user could potentially remove credentials from other companies by manipulating theidparameter.🛡️ Proposed fix to add authorization check
def handle_event("remove_certificate", %{"id" => id}, socket) do + company_id = socket.assigns.current_company && socket.assigns.current_company.id + with {:ok, uuid} <- Ecto.UUID.cast(id), - %{} = credential <- Credentials.get_credential(uuid) do + %{} = credential <- Credentials.get_credential(uuid), + true <- credential.company_id == company_id do case Credentials.deactivate_credential(credential) do {:ok, _} -> {:noreply, socket |> put_flash(:info, "Certificate removed.") |> load_credentials()} {:error, _} -> {:noreply, put_flash(socket, :error, "Failed to remove certificate.")} end else _ -> {:noreply, put_flash(socket, :error, "Certificate not found.")} end endBased on learnings: "Validate all state changes in LiveView events" and "Validate all user input in event handlers."
🧹 Nitpick comments (2)
lib/ksef_hub/credentials/certificate_info/openssl.ex (1)
99-116: Consider returning more specific error information.The
rescueblock catches all exceptions and returns a generic:parse_failederror. While this is safe from a logging perspective, it may make debugging difficult. Consider matching specific exception types if there are known failure modes that would benefit from distinct error atoms.lib/ksef_hub_web/live/certificate_live.ex (1)
247-257: Silent failure inenqueue_authmay hide issues.The function logs an error but always returns
:ok, making the failure invisible to the caller. Consider whether the caller should be informed of enqueueing failures, or at minimum ensure monitoring/alerting catches these logged errors.♻️ Alternative: Return the error tuple for caller awareness
`@spec` enqueue_auth(Ecto.UUID.t()) :: :ok | {:error, term()} defp enqueue_auth(company_id) do case %{company_id: company_id} |> AuthWorker.new() |> Oban.insert() do {:ok, _job} -> :ok {:error, reason} -> Logger.error("Failed to enqueue auth job for company #{company_id}") - :ok + {:error, reason} end endThen handle in
save_credential:case Credentials.replace_active_credential(company.id, attrs) do {:ok, _credential} -> case enqueue_auth(company.id) do :ok -> :ok {:error, _} -> :ok # Log already happened, upload succeeded end # ... rest of success handling
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / UX