fix: SyncWorker auto-reauthenticates instead of cancelling on expired…#40
Merged
Conversation
… tokens
SyncWorker permanently cancelled ({:cancel, :reauth_required}) when
TokenManager had no valid access token, stopping syncs entirely until
the next manual certificate upload.
Extract shared XADES auth logic into KsefClient.Authenticator so both
AuthWorker and SyncWorker can perform authentication. SyncWorker now
attempts re-auth on :reauth_required and returns retryable errors
instead of permanent cancellations.
- Add KsefClient.Authenticator with authenticate_and_store/1
- Refactor AuthWorker to delegate to Authenticator
- SyncWorker.get_access_token attempts reauth via Authenticator
- Permanent errors (no_credential, no_certificate) still cancel
- Transient errors (KSeF API failures) are retried by Oban
- TokenManager.store_tokens return value is now checked
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracts XAdES authentication into a new KsefHub.KsefClient.Authenticator, updates AuthWorker to delegate to it, expands SyncWorker re-auth orchestration and session termination, and adds tests covering Authenticator and re-auth scenarios. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
Replace case expressions in load_credential/1 and load_certificate/1 with pipeline + function-head pattern matching via normalize_* helpers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/ksef_client/authenticator.ex`:
- Around line 16-38: In authenticate_and_store/1, after successfully calling
TokenManager.store_tokens (and before returning {:ok, tokens.access_token}),
call terminate_session(tokens.access_token) to explicitly end the KSeF session;
invoke the existing terminate_session/1 function (or add it if missing), and
handle its result locally (log errors but do not change the overall success
return) so session termination won’t break the happy path; apply the same
pattern to any other authentication flows in this module that redeem tokens
(i.e., after Auth.authenticate and TokenManager.store_tokens).
Comment on lines
+16
to
+38
| @doc """ | ||
| Loads the certificate for a company, decrypts it, performs full XADES | ||
| authentication against KSeF, and stores the resulting tokens. | ||
|
|
||
| Returns `{:ok, access_token}` on success, or `{:error, reason}` on failure. | ||
| """ | ||
| @spec authenticate_and_store(Ecto.UUID.t()) :: {:ok, String.t()} | {:error, term()} | ||
| def authenticate_and_store(company_id) do | ||
| with {:ok, credential} <- load_credential(company_id), | ||
| {:ok, user_cert} <- load_certificate(company_id), | ||
| {:ok, cert_data} <- Encryption.decrypt(user_cert.certificate_data_encrypted), | ||
| {:ok, password} <- Encryption.decrypt(user_cert.certificate_password_encrypted), | ||
| {:ok, tokens} <- Auth.authenticate(credential.nip, cert_data, password), | ||
| :ok <- | ||
| TokenManager.store_tokens( | ||
| company_id, | ||
| tokens.access_token, | ||
| tokens.refresh_token, | ||
| tokens.access_valid_until, | ||
| tokens.refresh_valid_until | ||
| ) do | ||
| Logger.info("Authenticator: XADES authentication successful for company #{company_id}") | ||
| {:ok, tokens.access_token} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find lib/ksef_hub/ksef_client -name "*.ex" | head -20Repository: appunite/appunite-ksef-ex
Length of output: 296
🏁 Script executed:
# Search for Auth.authenticate implementation
rg -n --type elixir "def authenticate" lib/ksef_hub/ksef_client/Repository: appunite/appunite-ksef-ex
Length of output: 336
🏁 Script executed:
# Search for session termination patterns
rg -n --type elixir "(terminate|logout|close_session|end_session|session)" lib/ksef_hub/ksef_client/ -iRepository: appunite/appunite-ksef-ex
Length of output: 338
🏁 Script executed:
# Read the Auth.authenticate implementation
cat -n lib/ksef_hub/ksef_client/auth.ex | head -60Repository: appunite/appunite-ksef-ex
Length of output: 2564
🏁 Script executed:
# Read the entire authenticator.ex to see full function and if terminate_session is used
cat -n lib/ksef_hub/ksef_client/authenticator.exRepository: appunite/appunite-ksef-ex
Length of output: 3194
🏁 Script executed:
# Check the terminate_session callback and implementation
cat -n lib/ksef_hub/ksef_client/behaviour.ex | sed -n '40,50p'Repository: appunite/appunite-ksef-ex
Length of output: 371
🏁 Script executed:
# Check terminate_session in live.ex
cat -n lib/ksef_hub/ksef_client/live.ex | sed -n '240,260p'Repository: appunite/appunite-ksef-ex
Length of output: 803
Explicitly terminate the KSeF session after successful authentication.
The authenticate_and_store/1 function does not terminate the KSeF session after tokens are redeemed. Add a call to terminate_session(tokens.access_token) in the success path (after TokenManager.store_tokens succeeds but before returning) to prevent session leakage. This applies to all authentication flows in this module.
🤖 Prompt for AI Agents
In `@lib/ksef_hub/ksef_client/authenticator.ex` around lines 16 - 38, In
authenticate_and_store/1, after successfully calling TokenManager.store_tokens
(and before returning {:ok, tokens.access_token}), call
terminate_session(tokens.access_token) to explicitly end the KSeF session;
invoke the existing terminate_session/1 function (or add it if missing), and
handle its result locally (log errors but do not change the overall success
return) so session termination won’t break the happy path; apply the same
pattern to any other authentication flows in this module that redeem tokens
(i.e., after Auth.authenticate and TokenManager.store_tokens).
Prevents session leakage by explicitly calling terminate_session after sync finishes (success or partial). On sync failure, the session is kept alive so Oban retries can reuse the token. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
emilwojtaszek
force-pushed
the
fix/sync-worker-auto-reauth
branch
from
89591ce to
e09a2f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/sync/sync_worker.ex`:
- Around line 15-16: Add a `@doc` above the public perform/1 function in module
KsefHub.Sync.SyncWorker (the def perform(%Oban.Job{args: %{"company_id" =>
company_id}} = job) definition) describing what the job does, the expected args
(company_id), and the return value/behaviour (success/failure tuple), placing
the `@doc` immediately above the existing `@impl` Oban.Worker line so the public
function is documented per guidelines.
- Around line 20-28: The code currently calls
terminate_session_safely(access_token, company_id) before
handle_sync_result(sync_result, credential), which can terminate the session
even if handle_sync_result (and downstream update_last_sync/1) fails; move the
session termination to after handle_sync_result returns successfully and only
terminate when the final result is :ok (e.g., call
handle_sync_result(sync_result, credential) first, check that it returned :ok,
and then call terminate_session_safely(access_token, company_id)); ensure you
still preserve the original behavior of keeping the session alive on any error
by pattern matching on the final outcome rather than on sync_all_types alone.
In `@test/ksef_hub/sync/sync_worker_test.exs`:
- Around line 111-155: The test block's formatting is out of sync with mix
format; run mix format and fix whitespace/indentation in the
test/ksef_hub/sync/sync_worker_test.exs block that sets expectations for
KsefHub.KsefClient.Mock and KsefHub.XadesSigner.Mock (the sequence around
expect(:redeem_tokens, ...), expect(:query_invoice_metadata, ...),
expect(:terminate_session, ...) and the final assert calling
SyncWorker.perform/1). Reformat the multiline map and function clauses to match
project formatting (use mix format) so the CI formatting check passes.
- Around line 198-242: Reformat the failing test block with mix format: run `mix
format` (or your editor's Elixir formatter) on
test/ksef_hub/sync/sync_worker_test.exs to fix the formatting around the mocked
expectations (the chain of KsefHub.KsefClient.Mock |> expect... |> expect...)
and the assert call to SyncWorker.perform; ensure indentation, pipe alignment
and spacing match mix format rules so CI formatting check passes.
Comment on lines
+111
to
+155
| # Expect XADES re-authentication flow | ||
| KsefHub.KsefClient.Mock | ||
| |> expect(:get_challenge, fn -> | ||
| {:ok, %{challenge: "reauth-challenge", timestamp: "2025-01-15T12:00:00Z"}} | ||
| end) | ||
|
|
||
| KsefHub.XadesSigner.Mock | ||
| |> expect(:sign_challenge, fn "reauth-challenge", "1234567890", "cert-data", "cert-pass" -> | ||
| {:ok, "<ReauthSignedXML/>"} | ||
| end) | ||
|
|
||
| KsefHub.KsefClient.Mock | ||
| |> expect(:authenticate_xades, fn "<ReauthSignedXML/>" -> | ||
| {:ok, | ||
| %{ | ||
| reference_number: "ref-reauth", | ||
| auth_token: "auth-tok-reauth", | ||
| auth_token_valid_until: DateTime.add(DateTime.utc_now(), 300) | ||
| }} | ||
| end) | ||
|
|
||
| KsefHub.KsefClient.Mock | ||
| |> expect(:poll_auth_status, fn "ref-reauth", "auth-tok-reauth" -> | ||
| {:ok, :success} | ||
| end) | ||
|
|
||
| KsefHub.KsefClient.Mock | ||
| |> expect(:redeem_tokens, fn "auth-tok-reauth" -> | ||
| {:ok, | ||
| %{ | ||
| access_token: "reauth-access", | ||
| refresh_token: "reauth-refresh", | ||
| access_valid_until: access_until, | ||
| refresh_valid_until: refresh_until | ||
| }} | ||
| end) | ||
| # After re-auth, expect sync queries with the new token | ||
| KsefHub.KsefClient.Mock | ||
| |> expect(:query_invoice_metadata, 2, fn "reauth-access", _filters, _opts -> | ||
| {:ok, %{invoices: [], has_more: false, is_truncated: false}} | ||
| end) | ||
| |> expect(:terminate_session, fn "reauth-access" -> :ok end) | ||
|
|
||
| assert :ok = SyncWorker.perform(%Oban.Job{args: %{"company_id" => company.id}}) | ||
| end |
There was a problem hiding this comment.
Run mix format — formatting check failed in this block.
CI reports a mix format failure touching this region (Lines 145-148). Please reformat the file.
🧰 Tools
🪛 GitHub Actions: CI
[error] 145-148: mix format check failed. The following files are not formatted. Run 'mix format' to fix. (Lines 145-148 show formatting changes required.)
🤖 Prompt for AI Agents
In `@test/ksef_hub/sync/sync_worker_test.exs` around lines 111 - 155, The test
block's formatting is out of sync with mix format; run mix format and fix
whitespace/indentation in the test/ksef_hub/sync/sync_worker_test.exs block that
sets expectations for KsefHub.KsefClient.Mock and KsefHub.XadesSigner.Mock (the
sequence around expect(:redeem_tokens, ...), expect(:query_invoice_metadata,
...), expect(:terminate_session, ...) and the final assert calling
SyncWorker.perform/1). Reformat the multiline map and function clauses to match
project formatting (use mix format) so the CI formatting check passes.
Comment on lines
+198
to
+242
| # Expect XADES re-authentication flow (triggered by expired tokens) | ||
| KsefHub.KsefClient.Mock | ||
| |> expect(:get_challenge, fn -> | ||
| {:ok, %{challenge: "expired-challenge", timestamp: "2025-01-15T12:00:00Z"}} | ||
| end) | ||
|
|
||
| KsefHub.XadesSigner.Mock | ||
| |> expect(:sign_challenge, fn "expired-challenge", "1234567890", "cert-data", "cert-pass" -> | ||
| {:ok, "<ExpiredSignedXML/>"} | ||
| end) | ||
|
|
||
| KsefHub.KsefClient.Mock | ||
| |> expect(:authenticate_xades, fn "<ExpiredSignedXML/>" -> | ||
| {:ok, | ||
| %{ | ||
| reference_number: "ref-expired", | ||
| auth_token: "auth-tok-expired", | ||
| auth_token_valid_until: DateTime.add(DateTime.utc_now(), 300) | ||
| }} | ||
| end) | ||
|
|
||
| KsefHub.KsefClient.Mock | ||
| |> expect(:poll_auth_status, fn "ref-expired", "auth-tok-expired" -> | ||
| {:ok, :success} | ||
| end) | ||
|
|
||
| KsefHub.KsefClient.Mock | ||
| |> expect(:redeem_tokens, fn "auth-tok-expired" -> | ||
| {:ok, | ||
| %{ | ||
| access_token: "fresh-access", | ||
| refresh_token: "fresh-refresh", | ||
| access_valid_until: access_until, | ||
| refresh_valid_until: refresh_until | ||
| }} | ||
| end) | ||
| # After re-auth, expect sync queries with the fresh token | ||
| KsefHub.KsefClient.Mock | ||
| |> expect(:query_invoice_metadata, 2, fn "fresh-access", _filters, _opts -> | ||
| {:ok, %{invoices: [], has_more: false, is_truncated: false}} | ||
| end) | ||
| |> expect(:terminate_session, fn "fresh-access" -> :ok end) | ||
|
|
||
| assert :ok = SyncWorker.perform(%Oban.Job{args: %{"company_id" => company.id}}) | ||
| end |
There was a problem hiding this comment.
Run mix format — formatting check failed in this block.
CI reports a mix format failure touching this region (Lines 232-235). Please reformat the file.
🧰 Tools
🪛 GitHub Actions: CI
[error] 232-235: mix format check failed. The following files are not formatted. Run 'mix format' to fix. (Lines 232-235 show formatting changes required.)
🤖 Prompt for AI Agents
In `@test/ksef_hub/sync/sync_worker_test.exs` around lines 198 - 242, Reformat the
failing test block with mix format: run `mix format` (or your editor's Elixir
formatter) on test/ksef_hub/sync/sync_worker_test.exs to fix the formatting
around the mocked expectations (the chain of KsefHub.KsefClient.Mock |>
expect... |> expect...) and the assert call to SyncWorker.perform; ensure
indentation, pipe alignment and spacing match mix format rules so CI formatting
check passes.
Move terminate_session_safely after handle_sync_result so the KSeF session is only terminated when the entire workflow (including update_last_sync) succeeds. This keeps the session alive for Oban retries on any failure path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
… tokens
SyncWorker permanently cancelled ({:cancel, :reauth_required}) when TokenManager had no valid access token, stopping syncs entirely until the next manual certificate upload.
Extract shared XADES auth logic into KsefClient.Authenticator so both AuthWorker and SyncWorker can perform authentication. SyncWorker now attempts re-auth on :reauth_required and returns retryable errors instead of permanent cancellations.
Summary by CodeRabbit
New Features
Refactor
Tests