-
Notifications
You must be signed in to change notification settings - Fork 7
fix(adyen): fix missing hmac key when pod is restarted #446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes update the Adyen webhook creation and verification logic to explicitly handle and persist the HMAC key. The HMAC key is now returned by the Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin
participant Client
participant ConfigMetadata
Plugin->>Client: CreateWebhook(ctx, url, connectorID)
Client-->>Plugin: CreateWebhookResponse{HMACKey}, error
Plugin->>ConfigMetadata: Store HMACKey in Metadata
Note over Plugin,ConfigMetadata: On webhook verification
Plugin->>ConfigMetadata: Retrieve HMACKey from Metadata
Plugin->>Client: VerifyWebhookHMAC(item, HMACKey)
Client-->>Plugin: bool
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
==========================================
- Coverage 68.43% 68.42% -0.01%
==========================================
Files 586 586
Lines 29805 29812 +7
==========================================
+ Hits 20397 20400 +3
- Misses 8326 8329 +3
- Partials 1082 1083 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
internal/connectors/plugins/public/adyen/client/webhooks.go (1)
54-110
: 💡 Verification agent🧩 Analysis chain
Consider adding test coverage for error paths
The static analysis indicates that several error handling paths in the updated code aren't covered by tests. Consider adding tests for these paths to ensure errors are properly handled.
🏁 Script executed:
#!/bin/bash # Check test coverage for the error paths in CreateWebhook # Look for tests that verify error handling in CreateWebhook echo "Searching for tests that cover error paths in CreateWebhook..." grep -r "CreateWebhook.*error" --include="*_test.go" .Length of output: 733
Add comprehensive tests for CreateWebhook error paths
Tests for the happy path exist in the workflow integration, but there are currently no unit tests covering the error branches in
internal/connectors/plugins/public/adyen/client/webhooks.go::CreateWebhook
. Please add tests that simulate and assert correct handling for:
- Early return when
c.standardWebhook
is already set- Failure in
c.searchWebhook
(returning its error)- Error from
SetUpWebhook
API call (wrapping viac.wrapSDKError
)- Error from
GenerateHmacKey
API call (wrapping viac.wrapSDKError
)Affected file:
- internal/connectors/plugins/public/adyen/client/webhooks.go
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-58: internal/connectors/plugins/public/adyen/client/webhooks.go#L58
Added line #L58 was not covered by tests
[warning] 60-60: internal/connectors/plugins/public/adyen/client/webhooks.go#L60
Added line #L60 was not covered by tests
[warning] 64-64: internal/connectors/plugins/public/adyen/client/webhooks.go#L64
Added line #L64 was not covered by tests
[warning] 68-68: internal/connectors/plugins/public/adyen/client/webhooks.go#L68
Added line #L68 was not covered by tests
[warning] 95-95: internal/connectors/plugins/public/adyen/client/webhooks.go#L95
Added line #L95 was not covered by tests
[warning] 103-103: internal/connectors/plugins/public/adyen/client/webhooks.go#L103
Added line #L103 was not covered by tests
[warning] 108-110: internal/connectors/plugins/public/adyen/client/webhooks.go#L108-L110
Added lines #L108 - L110 were not covered by tests
🧹 Nitpick comments (1)
internal/connectors/plugins/public/adyen/client/client_generated.go (1)
1-7
: Note on generated codeThis is an auto-generated mock file. When making interface changes, ensure you're updating the source interface and regenerating this file using the mockgen command shown in the header, rather than manually editing it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/connectors/plugins/public/adyen/client/client.go
(1 hunks)internal/connectors/plugins/public/adyen/client/client_generated.go
(2 hunks)internal/connectors/plugins/public/adyen/client/webhooks.go
(3 hunks)internal/connectors/plugins/public/adyen/webhooks.go
(3 hunks)internal/connectors/plugins/public/adyen/webhooks_test.go
(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
internal/connectors/plugins/public/adyen/webhooks.go (1)
internal/models/webhooks.go (1)
PSPWebhookConfig
(3-9)
internal/connectors/plugins/public/adyen/webhooks_test.go (1)
internal/connectors/plugins/public/adyen/client/webhooks.go (1)
CreateWebhookResponse
(54-56)
internal/connectors/plugins/public/adyen/client/client.go (2)
internal/connectors/plugins/public/adyen/client/webhooks.go (1)
CreateWebhookResponse
(54-56)internal/models/webhooks.go (1)
BasicAuth
(20-23)
internal/connectors/plugins/public/adyen/client/webhooks.go (1)
internal/connectors/metrics/transport.go (1)
OperationContext
(16-18)
🪛 GitHub Check: codecov/patch
internal/connectors/plugins/public/adyen/webhooks.go
[warning] 53-54: internal/connectors/plugins/public/adyen/webhooks.go#L53-L54
Added lines #L53 - L54 were not covered by tests
internal/connectors/plugins/public/adyen/client/webhooks.go
[warning] 58-58: internal/connectors/plugins/public/adyen/client/webhooks.go#L58
Added line #L58 was not covered by tests
[warning] 60-60: internal/connectors/plugins/public/adyen/client/webhooks.go#L60
Added line #L60 was not covered by tests
[warning] 64-64: internal/connectors/plugins/public/adyen/client/webhooks.go#L64
Added line #L64 was not covered by tests
[warning] 68-68: internal/connectors/plugins/public/adyen/client/webhooks.go#L68
Added line #L68 was not covered by tests
[warning] 95-95: internal/connectors/plugins/public/adyen/client/webhooks.go#L95
Added line #L95 was not covered by tests
[warning] 103-103: internal/connectors/plugins/public/adyen/client/webhooks.go#L103
Added line #L103 was not covered by tests
[warning] 108-110: internal/connectors/plugins/public/adyen/client/webhooks.go#L108-L110
Added lines #L108 - L110 were not covered by tests
[warning] 126-127: internal/connectors/plugins/public/adyen/client/webhooks.go#L126-L127
Added lines #L126 - L127 were not covered by tests
🔇 Additional comments (21)
internal/connectors/plugins/public/adyen/webhooks_test.go (7)
68-68
: Test case correctly updated for new CreateWebhook return valueThe mock expectation for
CreateWebhook
is properly updated to return aCreateWebhookResponse
containing the HMAC key "test", matching the updated method signature.
73-73
: Verifies HMAC key is stored in metadataGood addition of an assertion that checks if the HMAC key is properly stored in the webhook configuration's metadata under the expected key.
109-111
: Test correctly sets up webhook metadataThe test now includes the HMAC key in the webhook configuration metadata, properly setting up the test case for the webhook verification flow.
139-141
: Test case properly includes HMAC key in metadataThe test correctly includes the HMAC key in the webhook configuration metadata for the invalid HMAC test case.
152-152
: Mock updated for new VerifyWebhookHMAC parameterThe mock expectation for
VerifyWebhookHMAC
correctly includes the HMAC key "test" as a parameter, matching the updated method signature.
167-169
: Test properly includes HMAC key in metadata for valid HMAC caseThe test correctly sets up the webhook configuration metadata for the valid HMAC verification test case.
180-180
: Mock updated for successful verification caseThe mock expectation for the successful HMAC verification correctly passes the HMAC key and returns true.
internal/connectors/plugins/public/adyen/client/client.go (2)
21-21
: Good change to make HMAC key handling more explicitThe signature change for
CreateWebhook
to return a structured response containing the HMAC key is a good design choice. This makes the HMAC key handling more explicit and avoids storing it only in memory.
23-23
: HMAC key now passed explicitly to verification methodThe method signature update for
VerifyWebhookHMAC
to accept the HMAC key as a parameter is a good design choice. This eliminates reliance on internal state and allows for using the key stored in persistent metadata.internal/connectors/plugins/public/adyen/client/webhooks.go (4)
54-56
: Good structured response with HMAC keyCreating a well-defined response structure for
CreateWebhook
makes the API more self-documenting and provides a clean way to return the HMAC key.
58-61
: Method signature correctly updated to return HMAC keyThe implementation correctly updates the return type to match the interface change, returnin 8000 g the CreateWebhookResponse struct.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-58: internal/connectors/plugins/public/adyen/client/webhooks.go#L58
Added line #L58 was not covered by tests
[warning] 60-60: internal/connectors/plugins/public/adyen/client/webhooks.go#L60
Added line #L60 was not covered by tests
108-110
: HMAC key correctly returned in responseThe implementation now properly returns the HMAC key in the response instead of storing it internally, allowing it to be persisted elsewhere.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-110: internal/connectors/plugins/public/adyen/client/webhooks.go#L108-L110
Added lines #L108 - L110 were not covered by tests
126-127
: Verification method now accepts explicit HMAC keyThe HMAC verification is now correctly implemented to accept and use the provided HMAC key rather than relying on internal state.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 126-127: internal/connectors/plugins/public/adyen/client/webhooks.go#L126-L127
Added lines #L126 - L127 were not covered by testsinternal/connectors/plugins/public/adyen/webhooks.go (5)
19-21
: Good use of a constant for metadata key nameUsing a constant for the metadata key name is good practice, making the code more maintainable and reducing the risk of typos when referring to this key.
51-54
: Properly captures HMAC key from responseThe implementation now correctly captures the HMAC key returned from the
CreateWebhook
method, which is essential for the fix.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-54: internal/connectors/plugins/public/adyen/webhooks.go#L53-L54
Added lines #L53 - L54 were not covered by tests
59-61
: HMAC key correctly stored in metadataThis is the core of the fix - storing the HMAC key in webhook configuration metadata ensures it persists across pod restarts and can be retrieved later.
78-78
: HMAC key correctly retrieved from metadata for verificationThe implementation now properly retrieves the HMAC key from the webhook configuration metadata and passes it to the verification method, completing the fix.
51-78
: Strong solution for persistent HMAC key storageThe overall implementation provides a robust solution to the issue of the HMAC key being lost on pod restarts. By storing the key in webhook metadata and retrieving it during verification, the system can now correctly verify webhooks even after the pod has been restarted. This is an essential fix for webhook reliability in production environments.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-54: internal/connectors/plugins/public/adyen/webhooks.go#L53-L54
Added lines #L53 - L54 were not covered by testsinternal/connectors/plugins/public/adyen/client/client_generated.go (3)
47-53
: Implementation aligns with the HMAC persistence requirementThe change to return
CreateWebhookResponse
instead of just an error allows for returning the HMAC key generated during webhook creation. This supports the PR objective of persisting the HMAC key in webhook metadata, enabling retrieval after pod restarts.
120-125
: Correct approach for HMAC key persistenceAdding the
hmacKey
parameter to theVerifyWebhookHMAC
method is the right approach for passing the HMAC key retrieved from webhook metadata. This ensures the verification can succeed even after pod restarts since the key is now passed explicitly rather than being stored in memory.
128-131
: Mock recorder updated correctly for the new signatureThe mock recorder has been properly updated to handle the additional
hmacKey
parameter, ensuring tests using this mock will work correctly with the new interface.
Description
Context
When we first install Adyen, the CreateWebhooks workflow is called and calls the right function inside adyen plugin. Inside the client, the webhooks are created on the adyen psp and an hmac key is generated and store in memory. When rebooting the worker pod, the hmac key is then empty and cannot be retrieved anymore, hence all the webhooks coming after the reboot are failing the hmac verification
Solution
I'm adding the hmac key inside the metadata of the webhooks creation in order to get it when a webhook is coming inside the config
Fixes PMNT-107