8000 fix(adyen): fix missing hmac key when pod is restarted by paul-nicolas · Pull Request #446 · formancehq/payments · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 1 commit into from
May 14, 2025

Conversation

paul-nicolas
Copy link
Contributor
@paul-nicolas paul-nicolas commented May 14, 2025

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

Copy link
Contributor
coderabbitai bot commented May 14, 2025

Walkthrough

The 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 CreateWebhook method, stored in webhook configuration metadata, and explicitly passed to the verification method, ensuring it is not lost across worker restarts.

Changes

File(s) Change Summary
.../adyen/client/client.go
.../adyen/client/webhooks.go
Changed CreateWebhook to return a response struct with HMAC key; VerifyWebhookHMAC now takes HMAC key param.
.../adyen/client/client_generated.go Updated mock methods to match new signatures for CreateWebhook and VerifyWebhookHMAC.
.../adyen/webhooks.go Added constant for HMAC metadata key; stores HMAC key in webhook metadata; passes HMAC key to verification.
.../adyen/webhooks_test.go Updated tests to expect/store HMAC key in metadata and to pass key to verification.

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
Loading

Assessment against linked issues

Objective (Issue) Addressed Explanation
Persist HMAC key for Adyen webhook across worker pod restarts (PMNT-107)

Poem

A hop and a skip, with HMAC in tow,
Now webhooks are safe, wherever they go!
The key’s in the metadata, tucked in tight—
No more lost secrets in the deep of night.
Pods may restart, but all will be well,
For Adyen’s webhooks, this rabbit can tell!
🐇🔑

Tip

⚡️ Faster reviews with caching
  • CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.

Enjoy the performance boost—your workflow just got faster.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
codecov bot commented May 14, 2025

Codecov Report

Attention: Patch coverage is 26.31579% with 14 lines in your changes missing coverage. Please review.

Project coverage is 68.42%. Comparing base (9dbfa28) to head (6f7d29b).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...connectors/plugins/public/adyen/client/webhooks.go 0.00% 11 Missing ⚠️
...ternal/connectors/plugins/public/adyen/webhooks.go 62.50% 2 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@paul-nicolas paul-nicolas marked this pull request as ready for review May 14, 2025 16:02
@paul-nicolas paul-nicolas requested a review from a team as a code owner May 14, 2025 16:02
Copy link
Contributor
@coderabbitai coderabbitai bot left a 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 via c.wrapSDKError)
  • Error from GenerateHmacKey API call (wrapping via c.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 code

This 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9dbfa28 and 6f7d29b.

📒 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 value

The mock expectation for CreateWebhook is properly updated to return a CreateWebhookResponse containing the HMAC key "test", matching the updated method signature.


73-73: Verifies HMAC key is stored in metadata

Good 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 metadata

The 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 metadata

The test correctly includes the HMAC key in the webhook configuration metadata for the invalid HMAC test case.


152-152: Mock updated for new VerifyWebhookHMAC parameter

The 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 case

The test correctly sets up the webhook configuration metadata for the valid HMAC verification test case.


180-180: Mock updated for successful verification case

The 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 explicit

The 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 method

The 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 key

Creating 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 key

The 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 response

The 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 key

The 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 tests

internal/connectors/plugins/public/adyen/webhooks.go (5)

19-21: Good use of a constant for metadata key name

Using 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 response

The 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 metadata

This 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 verification

The 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 storage

The 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 tests

internal/connectors/plugins/public/adyen/client/client_generated.go (3)

47-53: Implementation aligns with the HMAC persistence requirement

The 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 persistence

Adding the hmacKey parameter to the VerifyWebhookHMAC 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 signature

The mock recorder has been properly updated to handle the additional hmacKey parameter, ensuring tests using this mock will work correctly with the new interface.

@paul-nicolas paul-nicolas added this pull request to the merge queue May 14, 2025
Merged via the queue into main with commit 89ca816 May 14, 2025
7 of 9 checks passed
@paul-nicolas paul-nicolas deleted the fix/adyen-webhooks branch May 14, 2025 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0