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

[app-configuration] Fix issue with usages of 'crypto' package for browsers. #9648

Merged
merged 17 commits into from Jun 25, 2020

Conversation

@richardpark-msft
Copy link
Member

richardpark-msft commented Jun 22, 2020

Our usage of the crypto package in AppConfig causes an issue in browsers.

This adds functions to hash and HMAC that use the appropriate implementation for browser (WebCrypto) and the node.js crypto package for node.

Fixes #9585

…ers.

This adds functions to hash and hmac  that use the appropriate implementation for browser (WebCrypto) and the node.js crypto package for node.
@richardpark-msft richardpark-msft requested review from bterlson and xirzec Jun 22, 2020
@richardpark-msft richardpark-msft requested a review from chradek as a code owner Jun 22, 2020
@richardpark-msft
Copy link
Member Author

richardpark-msft commented Jun 22, 2020

@bterlson , @xirzec - this appears to be the only package impacted by this (using the 'crypto' package).

When talking with @chradek he mentioned that we should consider if this is something that we should make usable across other parts of the SDK.

BTW, this it the first time I've needed to make a browser-specific change in AppConfig so please let me know if I've missed anything here.

@richardpark-msft
Copy link
Member Author

richardpark-msft commented Jun 22, 2020

/azp run js - appconfiguration - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Jun 22, 2020

Azure Pipelines successfully started running 1 pipeline(s).
Copy link
Contributor

xirzec left a comment

A few minor thoughts; cool to see browser support lighting up!

- There was actually a "can't work in browser" function (util.isArray) that I replaced with the proper browser equivalent
- A user agent test that I used to have to force to be node or browser now just "works" since it uses the core-http method for figuring out the environment. Also, got to remove some code for that.
@richardpark-msft richardpark-msft requested a review from danieljurek as a code owner Jun 24, 2020
@richardpark-msft
Copy link
Member Author

richardpark-msft commented Jun 24, 2020

/azp run js - appconfiguration - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Jun 24, 2020

Azure Pipelines successfully started running 1 pipeline(s).
@richardpark-msft
Copy link
Member Author

richardpark-msft commented Jun 24, 2020

/azp run js - appconfiguration - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Jun 24, 2020

Azure Pipelines successfully started running 1 pipeline(s).
@chradek chradek dismissed their stale review Jun 24, 2020

I don't remember actually approving this.

@richardpark-msft
Copy link
Member Author

richardpark-msft commented Jun 24, 2020

/azp run js - appconfiguration - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Jun 24, 2020

Azure Pipelines successfully started running 1 pipeline(s).
…order and nock) into the test branch so we can ensure that our browser bundle is clean of node dependencies.
Copy link
Member

chradek left a comment

🚢🇮🇹

@richardpark-msft
Copy link
Member Author

richardpark-msft commented Jun 25, 2020

/azp run js - appconfiguration - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Jun 25, 2020

Azure Pipelines successfully started running 1 pipeline(s).
@richardpark-msft
Copy link
Member Author

richardpark-msft commented Jun 25, 2020

/check-enforcer override

@richardpark-msft richardpark-msft merged commit 1289b0c into Azure:master Jun 25, 2020
15 of 17 checks passed
15 of 17 checks passed
js - mgmt - pr Build #20200625.13 failed
Details
js - mgmt - pr (Build auto-generated projects) Build auto-generated projects failed
Details
check-enforcer
Details
js - appconfiguration - ci Build #20200625.2 succeeded
Details
js - appconfiguration - ci (Build Analyze) Build Analyze succeeded
Details
js - appconfiguration - ci (Build Build) Build Build succeeded
Details
js - appconfiguration - ci (Build UnitTest Browser_Linux_Node12) Build UnitTest Browser_Linux_Node12 succeeded
Details
js - appconfiguration - ci (Build UnitTest Linux_Node8) Build UnitTest Linux_Node8 succeeded
Details
js - appconfiguration - ci (Build UnitTest Windows_Node10) Build UnitTest Windows_Node10 succeeded
Details
js - appconfiguration - ci (Build UnitTest macOS_Node12) Build UnitTest macOS_Node12 succeeded
Details
js - appconfiguration - tests Build #20200625.1 succeeded
Details
js - appconfiguration - tests (IntegrationTest Linux Node 8) IntegrationTest Linux Node 8 succeeded
Details
js - appconfiguration - tests (IntegrationTest MacOS Node 12) IntegrationTest MacOS Node 12 succeeded
Details
js - appconfiguration - tests (IntegrationTest Windows Browser) IntegrationTest Windows Browser succeeded
Details
js - appconfiguration - tests (IntegrationTest Windows Node 10) IntegrationTest Windows Node 10 succeeded
Details
js - mgmt - pr (Check .only, .skip and version bump) Check .only, .skip and version bump succeeded
Details
license/cla All CLA requirements met.
Details
sadasant added a commit to sadasant/azure-sdk-for-js that referenced this pull request Jun 27, 2020
…wsers. (Azure#9648)

Our usage of the `crypto` package in appconfig causes an issue in browsers.

This adds functions to hash and hmac  that use the appropriate implementation for browser (WebCrypto) and the node.js crypto package for node.

Fixes Azure#9585
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.