search

Azure / ms-rest-nodeauth

node.js based authentication library for Azure with type definitions
MIT License
33 stars 33 forks source link

PR https://github.com/Azure/ms-rest-nodeauth/pull/126 introduces a broken change #130

Closed AlphaWong closed 3 years ago

AlphaWong commented 3 years ago

related PR

https://github.com/Azure/ms-rest-nodeauth/pull/126

issue code

https://github.com/AzureAD/azure-activedirectory-library-for-nodejs/commit/362a00e5d34186e052728932d8f87a527b788bd8#diff-7a7a2457283d142ea9659e12006bbc85204c3a6c89e9990b8375b35f6cb27e91

background

upgrade the adal from 0.18.x to 0.22.x will break all the proxy settings.

workaround

const HttpsProxyAgent = require('https-proxy-agent');
const axios = require('axios')
axios.defaults.proxy = false
axios.defaults.httpsAgent = new HttpsProxyAgent('http://MY_PROXY:8888')

good to have

  1. update the readme file to state the proxy issue
  2. check the PR carefully next time
  3. rollback the adal upgrade

proxy issue with axios

  1. https://github.com/axios/axios/issues/2072#issuecomment-567473812
ramya-rao-a commented 3 years ago

Thanks for reporting @AlphaWong!

Do you have any pointers to any official recommendation from the adal team?

@sadasant, Please consult with the adal team to confirm the right next steps here

AlphaWong commented 3 years ago

Thanks for reporting @AlphaWong!

Do you have any pointers to any official recommendation from the adal team?

@sadasant, Please consult with the adal team to confirm the right next steps here

emm, as my working environment is behind proxy. And we are enterprise user of Azure.

The change from request to axios is really surprise our devops team.

Also, the version number marks at 0.22.x that we expect the promotion from 0.18.x will not introduce a broken change according to the x.y.z release rule.

sadasant commented 3 years ago

Hello @AlphaWong, we’re still working on this. I’m planning to write back this week with more information.

sadasant commented 3 years ago

@AlphaWong Hello! I need to be able to narrow this down for the Adal team. What error are you getting exactly? do you have a minimal example that reproduces the issue?

sameerag commented 3 years ago

@AlphaWong Thank you for reporting this. I am from the adal-node team and. @sadasant and @ramya-rao-a raised this issue with us. Apologies for the inconvenience.

This is a side effect we did not see coming when we moved from npm request to axios as the package of choice for httpRequests for adal-node. To give you a brief background, we are currently making only security fixes for all of our adal libraries and were asked to upgrade from request to any other network package for adal-node early this year. Since we fully expected axios to support all the use cases request does including the proxies we have changed it under the hood and did not expect to break it.

We only discovered later that axios has a problem. supporting proxies by default and the same issue is tracked in our new library msal-node. Since we are not pushing any changes to adal-node and soon archiving the repo, we decided we will fix this in msal-node going forward.

I will make sure we add this info in the adal-node repo for anyone using it. We are soon moving all our older repos to a single mono repo and I will upgrade the docs once we conclude that activity.

Thank you for root causing this helping us catch this use case.

sameerag commented 3 years ago

Added a PR in adal-node repo here

ramya-rao-a commented 3 years ago

Thanks @sameerag!

@sadasant Let's add a similar note to the readme in this repo as well

ramya-rao-a commented 3 years ago

On the other hand, I pushed some changes to the PR @AlphaWong had already created. See https://github.com/Azure/ms-rest-nodeauth/pull/131

ramya-rao-a commented 3 years ago

Version 3.1.0 of the @azure/ms-rest-nodeauth package will include a note on the known proxy issue in the readme