[Identity] Reverting ClientSecretCredential and UsernamePasswordCredential to v1 only for browsers, and fixing Karma browser bundles #14910

sadasant · 2021-04-16T20:41:22Z

This PR makes it so ClientSecretCredential behaves like how it did on v1 only for browsers. It also ensures browser bundles can be used when built with Karma.

ClientSecretCredential (and also UsernamePasswordCredential)

Since the inception of Identity, we've been relying on ClientSecretCredential to be available in browsers to make it easier for us to automate bowser tests.

Whether ClientSecretCredential should continue being available on browsers in the future, is something yet to be decided. We started a conversation on the matter here: #14879

Our current plan is to switch master back to v1, as you can see here: #14909

Since the v2 changes to ClientSecretCredential won't be reverted, that means that as soon as we move master back to v1, all of our current clients will immediately start working with the latest ClientSecretCredential.

ClientSecretCredential was rewritten to use MSAL, specifically @azure/msal-node, which is not intended to work on browsers.

This PR makes it so ClientSecretCredential is kept as it was on v1 only for browsers. This will:

Allow us to keep our current browser recordings.
Allow us to continue using this credential for browser tests.
On v1, dropping this credential on browsers would be a breaking change, so this technically reduces the differences with v1.

Important:
Keep in mind that while we've always supported this credential in browsers, it can only work if browser security is disabled. This isn't recommended outside of our tests, and it's not something we should be advertising.

(Later we also did the same to UsernamePasswordCredential).

Browser bundles

While my initial change was scoped to one credential, I asked Harsha to test this PR and he found that the browser bundles would not work with Identity v2. We decided to fix it right away since this PR wouldn't be very useful with broken browser bundles.

ramya-rao-a

Thanks for the detailed PR description @sadasant
This makes sense to me

Reviewed that the file added for ClientSecretCredential for browser has the exact same code as what we had for this credential in v1

sdk/identity/identity/src/credentials/clientSecretCredential.browser.ts

HarshaNalluru · 2021-04-16T22:11:22Z

There are a couple other node-related things present in the browser bundle, we are working on removing them.

HarshaNalluru

Waiting until we resolve all the errors with browser bundles.

HarshaNalluru · 2021-04-16T22:22:21Z

While investigating with @sadasant...
I had to do b2ad072 on top of this PR to get identity v2 working with keyvault-secrets and keyvault-keys browser tests.

sdk/identity/identity/src/index.ts

Dismissing review as the scope of the PR has changed

ramya-rao-a · 2021-04-16T23:13:46Z

sdk/identity/identity/test/internal/identityClient.spec.ts

+      if (isNode) {
+        assert.equal(error.name, "CredentialUnavailableError");
+      } else {
+        assert.equal(error.name, "AuthenticationError");


This is the same behavior in v1 also right?

i.e. the fact that error that user would get is different in node vs browser for the same test case?

Let me check

Yes: https://github.com/Azure/azure-sdk-for-js/blob/hotfix/identity_1.3.0/sdk/identity/identity/test/internal/identityClient.spec.ts#L58

The link suggests that previously we had AuthenticationError thrown for node as well. The reason for difference in node?

This test uses ClientSecretCredential, which now has two different implementations. In this case the Node one is correct (on Node) because the provided tenant is not a valid tenant, so this credential can't be used. MSAL does validations like this that we don't do in v1.

ramya-rao-a · 2021-04-16T23:15:16Z

sdk/identity/identity/CHANGELOG.md

@@ -2,6 +2,7 @@

 ## 2.0.0-beta.3 (Unreleased)

+- Breaking change: Disabled `UsernamePasswordCredential` in browser bundles. This credential was never supported in browsers, but now that it uses MSAL, having it in browsers would break the browser bundles.


What was the exact behavior in v1 when someone used UsernamePasswordCredential in the browser?

It would work if CORS was disabled too: https://github.com/Azure/azure-sdk-for-js/blob/hotfix/identity_1.3.0/sdk/identity/identity/src/credentials/usernamePasswordCredential.ts should I bring this credential back as well? (only for the browser, of course)

If we are making ClientSecretCredential behavior in browser to be the same as in v1, shouldnt the same be applied to other credentials (other than of course InteractiveBrowserCredential)?

We can touch the other credentials in a different PR, but since UsernamePasswordCredential is anyway being touched here, we should consider making the browser experience to be same as v1

I can add UsernamePasswordCredential too. I feel like we should drop the ones we're not using to test stuff, but adding it would align us a little better with v1, so... I'll add it back.

Pushed the v1 version of UsernamePasswordCredential on browsers 👍

sdk/identity/identity/test/internal/identityClient.spec.ts

ghost · 2021-04-17T00:07:50Z

Hello @sadasant!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

…ntial to v1 only for browsers, and fixing Karma browser bundles (Azure#14910) This PR makes it so `ClientSecretCredential` behaves like how it did on v1 only for browsers. It also ensures browser bundles can be used when built with Karma. --- ## ClientSecretCredential (and also UsernamePasswordCredential) Since the inception of Identity, we've been relying on `ClientSecretCredential` to be available in browsers to make it easier for us to automate bowser tests. Whether `ClientSecretCredential` should continue being available on browsers in the future, is something yet to be decided. We started a conversation on the matter here: Azure#14879 Our current plan is to switch master back to v1, as you can see here: Azure#14909 Since the v2 changes to `ClientSecretCredential` won't be reverted, that means that as soon as we move master back to v1, all of our current clients will immediately start working with the latest `ClientSecretCredential`. `ClientSecretCredential` was rewritten to use MSAL, specifically `@azure/msal-node`, which is not intended to work on browsers. This PR makes it so `ClientSecretCredential` is kept as it was on v1 only for browsers. This will: - Allow us to keep our current browser recordings. - Allow us to continue using this credential for browser tests. - On v1, dropping this credential on browsers would be a breaking change, so this technically reduces the differences with v1. **Important:** Keep in mind that while we've always supported this credential in browsers, it can only work if browser security is disabled. This isn't recommended outside of our tests, and it's not something we should be advertising. (Later we also did the same to `UsernamePasswordCredential`). --- ## Browser bundles While my initial change was scoped to one credential, I asked Harsha to test this PR and he found that the browser bundles would not work with Identity v2. We decided to fix it right away since this PR wouldn't be very useful with broken browser bundles.

reverting client-secret in the browser only

43f5130

sadasant requested review from KarishmaGhiya, HarshaNalluru, ramya-rao-a and witemple-msft April 16, 2021 20:41

sadasant requested review from daviwil, maorleger and schaabs as code owners April 16, 2021 20:41

sadasant self-assigned this Apr 16, 2021

ghost added the Azure.Identity label Apr 16, 2021

sadasant changed the title ~~[Identity] Reverting client-secret to v1 in the browser only~~ [Identity] Reverting ClientSecretCredential to v1 in the browser only Apr 16, 2021

sadasant changed the title ~~[Identity] Reverting ClientSecretCredential to v1 in the browser only~~ [Identity] Reverting ClientSecretCredential to v1 only for browsers Apr 16, 2021

ramya-rao-a previously approved these changes Apr 16, 2021

View reviewed changes

HarshaNalluru reviewed Apr 16, 2021

View reviewed changes

sdk/identity/identity/src/credentials/clientSecretCredential.browser.ts Show resolved Hide resolved

HarshaNalluru approved these changes Apr 16, 2021

View reviewed changes

HarshaNalluru requested changes Apr 16, 2021

View reviewed changes

sadasant added 2 commits April 16, 2021 22:26

Disabled UsernamePasswordCredential in browsers and other browser fixes

dec0b6a

comment on ClientSecretCredential

ff94d88

sadasant requested a review from HarshaNalluru April 16, 2021 22:36

HarshaNalluru mentioned this pull request Apr 16, 2021

[Identity] Browser tests #14913

Closed

sadasant commented Apr 16, 2021

View reviewed changes

sdk/identity/identity/src/index.ts Outdated Show resolved Hide resolved

removing unnecessary changes

cc359a8

sadasant changed the title ~~[Identity] Reverting ClientSecretCredential to v1 only for browsers~~ [Identity] Reverting ClientSecretCredential to v1 only for browsers, and fixing Karma browser bundles Apr 16, 2021

re-enabled karma tests on Identity

d14639d

sadasant requested a review from ramya-rao-a April 16, 2021 23:04

brought back the old client secret credential test

c4eef45

ramya-rao-a reviewed Apr 16, 2021

View reviewed changes

HarshaNalluru reviewed Apr 16, 2021

View reviewed changes

sdk/identity/identity/test/internal/identityClient.spec.ts Show resolved Hide resolved

HarshaNalluru approved these changes Apr 16, 2021

View reviewed changes

comment, suggestion by Harsha

12fe58f

HarshaNalluru reviewed Apr 16, 2021

View reviewed changes

sdk/identity/identity/test/internal/identityClient.spec.ts Outdated Show resolved Hide resolved

HarshaNalluru requested changes Apr 16, 2021

View reviewed changes

sdk/identity/identity/test/internal/identityClient.spec.ts Outdated Show resolved Hide resolved

typos...

1a585ae

HarshaNalluru approved these changes Apr 16, 2021

View reviewed changes

sadasant changed the title ~~[Identity] Reverting ClientSecretCredential to v1 only for browsers, and fixing Karma browser bundles~~ [Identity] Reverting ClientSecretCredential and UsernamePasswordCredential to v1 only for browsers, and fixing Karma browser bundles Apr 16, 2021

Reinstated UsernamePasswordCredential on the browser

04adc1f

ramya-rao-a approved these changes Apr 17, 2021

View reviewed changes

sadasant added the auto-merge label Apr 17, 2021

ghost merged commit 424ad49 into Azure:master Apr 17, 2021

sadasant deleted the identity/clientSecretBrowserRevert branch April 17, 2021 14:35

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Identity] Reverting ClientSecretCredential and UsernamePasswordCredential to v1 only for browsers, and fixing Karma browser bundles #14910

[Identity] Reverting ClientSecretCredential and UsernamePasswordCredential to v1 only for browsers, and fixing Karma browser bundles #14910

sadasant commented Apr 16, 2021 •

edited

Loading

ramya-rao-a left a comment

HarshaNalluru commented Apr 16, 2021

HarshaNalluru left a comment

HarshaNalluru commented Apr 16, 2021 •

edited

Loading

ramya-rao-a Apr 16, 2021

ramya-rao-a Apr 16, 2021

sadasant Apr 16, 2021

sadasant Apr 16, 2021

ramya-rao-a Apr 16, 2021

sadasant Apr 16, 2021 •

edited

Loading

ramya-rao-a Apr 16, 2021

sadasant Apr 16, 2021 •

edited

Loading

ramya-rao-a Apr 16, 2021

ramya-rao-a Apr 16, 2021

sadasant Apr 16, 2021 •

edited

Loading

sadasant Apr 17, 2021

ghost commented Apr 17, 2021

		@@ -2,6 +2,7 @@

		## 2.0.0-beta.3 (Unreleased)

		- Breaking change: Disabled `UsernamePasswordCredential` in browser bundles. This credential was never supported in browsers, but now that it uses MSAL, having it in browsers would break the browser bundles.

[Identity] Reverting ClientSecretCredential and UsernamePasswordCredential to v1 only for browsers, and fixing Karma browser bundles #14910

[Identity] Reverting ClientSecretCredential and UsernamePasswordCredential to v1 only for browsers, and fixing Karma browser bundles #14910

Conversation

sadasant commented Apr 16, 2021 • edited Loading

ClientSecretCredential (and also UsernamePasswordCredential)

Browser bundles

ramya-rao-a left a comment

Choose a reason for hiding this comment

HarshaNalluru commented Apr 16, 2021

HarshaNalluru left a comment

Choose a reason for hiding this comment

HarshaNalluru commented Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 17, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

sadasant commented Apr 16, 2021 •

edited

Loading

HarshaNalluru commented Apr 16, 2021 •

edited

Loading

sadasant Apr 16, 2021 •

edited

Loading

sadasant Apr 16, 2021 •

edited

Loading

sadasant Apr 16, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.