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

fix(ClientRequest): support the "auth" option #440

Merged
merged 4 commits into from
Sep 22, 2023

Conversation

mikicho
Copy link
Contributor

@mikicho mikicho commented Sep 21, 2023

Options: https://nodejs.org/api/http.html#httprequesturl-options-callback

@kettanaito

  • Not sure if this is where the test should be
  • Also, I'm not sure if this is what you mean by "Do not propagate username and password onto the NodeClientRequest.url", I just override the username/password. Please let me know if you have something else in mind.

closes #438

@mikicho mikicho marked this pull request as ready for review September 21, 2023 20:55
@kettanaito kettanaito changed the title Support NodeClientRequest auth option fix(ClientRequest): support the "auth" option Sep 22, 2023
@kettanaito kettanaito force-pushed the Michael/node-support-credentials branch from cc62ba3 to 301b3bf Compare September 22, 2023 10:54
@@ -159,7 +166,10 @@ export function getUrlByRequestOptions(options: ResolvedRequestOptions): URL {
: ''
logger.info('auth string:', authString)

const url = new URL(`${protocol}//${authString}${hostname}${path}`)
const url = new URL(`${protocol}//${hostname}${path}`)
url.username = credentials?.username || ''
Copy link
Member

Choose a reason for hiding this comment

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

TIL you can set username and password on the existing URL instance since href is a getter. This makes the URL construct a bit more readable.

const password = 'secret123'
const req = http.request(
// The request URL can include the basic auth directly.
new URL(
Copy link
Member

@kettanaito kettanaito Sep 22, 2023

Choose a reason for hiding this comment

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

I've also added a test case for when the auth is included in the URL itself.

This was also where I mistakingly assumed username and password are ClientRequest options. They are not. They are the options of the URL instance if you wish to create it object-first.

Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

I did a few minor adjustments but it looks great! Thank you for opening this, @mikicho 👏

@kettanaito
Copy link
Member

Please let me know if this sufficiently addresses the use case. If it does, we can proceed with releasing this.

@mikicho
Copy link
Contributor Author

mikicho commented Sep 22, 2023

I patched it locally and Nock's tests pass!
image

@kettanaito kettanaito merged commit 8d04c08 into mswjs:main Sep 22, 2023
1 check passed
@mikicho mikicho deleted the Michael/node-support-credentials branch September 22, 2023 12:33
@kettanaito
Copy link
Member

Released: v0.25.4 🎉

This has been released in v0.25.4!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

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.

Support username:password in NodeClientRequest
2 participants