Skip to content
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

feat(op): allow custom interceptor for authenticated client #422

Merged
merged 10 commits into from
Feb 20, 2024

Conversation

raducristianpopa
Copy link
Member

@raducristianpopa raducristianpopa commented Feb 9, 2024

Changes proposed in this pull request

Allow a custom request interceptor to be passed rather than the private key and key ID. This custom interceptor will then be registered and replace the built-in interceptor for generating signatures.

Context

Closes #412.

Copy link

changeset-bot bot commented Feb 9, 2024

🦋 Changeset detected

Latest commit: 07546fa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@interledger/open-payments Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Feb 9, 2024

Deploy Preview for openpayments-preview canceled.

Name Link
🔨 Latest commit 07546fa
🔍 Latest deploy log https://app.netlify.com/sites/openpayments-preview/deploys/65d488990ce3c70008bfcb78

packages/open-payments/src/client/index.ts Outdated Show resolved Hide resolved
Comment on lines 237 to 239
runWhen: (config: InternalAxiosRequestConfig) =>
config.method?.toLowerCase() === 'post' ||
!!(config.headers && config.headers['Authorization'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think because we are specifying POST with Authorization runWhen here (and not leaving it up to the caller), we should probably make the requestInterceptor name more explicit? like authenticatedRequestInterceptor or something along those lines.

@@ -0,0 +1,5 @@
---
'@interledger/open-payments': minor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im trying to think of a better naming when it comes to the "experimental" nature of the change. Maybe I'm over-complicating this though

Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final comment

Comment on lines +236 to +244
axiosInstance.interceptors.request.use(
args.authenticatedRequestInterceptor,
undefined,
{
runWhen: (config: InternalAxiosRequestConfig) =>
config.method?.toLowerCase() === 'post' ||
!!(config.headers && config.headers['Authorization'])
}
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only thing that would be nice is a small test for this to check that the interceptor was added. I think you can use

 createCustomAxiosInstance({
          requestTimeoutMs: 0,
          authenticatedRequestInterceptor: interceptor
        }).interceptors.request['handlers'][0]

to get a reference to the interceptor

@raducristianpopa raducristianpopa merged commit 8d4ccc3 into main Feb 20, 2024
10 checks passed
@raducristianpopa raducristianpopa deleted the rp--update-authenticated-client branch February 20, 2024 12:05
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.

Enable the option to provide a custom request interceptor for the authenticated client
2 participants