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

Feature: Allow HTTP Digest Auth #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

subzero10
Copy link

Thank you for this Github Action!

I am using it to call an API that requires HTTP Digest. This PR replaces node-fetch with urllib, which supports HTTP Digest out-of-the-box.

Tests pass without any other changes 👌

Note:

  • When I do npm run build, ncc generates more than one file, however it works fine.

@mydea
Copy link
Owner

mydea commented Apr 26, 2022

Thanks for the PR!

Generally speaking, I'd rather like to not replace node-fetch with something else, especially as fetch is finally supported in Node 18, so eventually I'd like to actually remove it, and use native fetch instead.

I could not really find that much information about digest auth, to be honest, and don't really have any experience with it.

So in order to rather keep this a bit separate, I'd prefer to handle this a bit differently:

This way, we can enapsulate this a bit. So simplified speaking, doFetch could look something like this:

New options:

  • auth-user
  • auth-password
  • auth-use-digest (default: false)
function makeFetchRequest() {
  if (authUser && authPassword) {
    let client = new DigestFetch(authUser, authPassword, , { basic: !useDigestAuth });
    return client.fetch(url, options);
  }

  return fetch(url, options);
}

async function doFetch() {
  let response = await makeFetchRequest();
  // etc.
}

Also please make sure to not change the formatting, and I use let everywhere inside of methods ;)

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