[Identity] Improvements to MSAL support #11407

sophiajt · 2020-09-22T20:10:20Z

This is another batch of improvements on our MSAL interop. In this PR:

Will will do silent flow authentication if the user provides a cache and authentication record for lookup
The HTTP pipeline is configurable, and we'll adapt calls for the MSAL pipeline
We now parse out and use the port from the redirectUri

We'll continue improving, I just wanted to land updates more incrementally.

xirzec

Left some thoughts / code cleanup suggestions

xirzec · 2020-09-23T22:49:55Z

sdk/identity/identity/package.json

@@ -82,14 +82,14 @@
    "@azure/core-http": "^1.1.6",
    "@azure/core-tracing": "1.0.0-preview.9",
    "@azure/logger": "^1.0.0",
-    "@azure/msal-node": "^1.0.0-alpha.5",
+    "@azure/msal-node": "^1.0.0-alpha.6",


should we pin this? or are they not making breaking changes? ;)

They're still making changes, yeah.

sdk/identity/identity/review/identity.api.md

sdk/identity/identity/src/client/identityClient.ts

xirzec · 2020-09-23T22:57:16Z

sdk/identity/identity/src/credentials/interactiveBrowserCredential.ts

+      try {
+        return this.acquireTokenFromCache();
+      } catch (e) {
+        if (e instanceof AuthenticationRequired) {


this is a bit brittle, since errors from different constructors/contexts are not instanceof, hence a name property check is sometimes better.

Do you have a link to a preferred pattern, by chance?

If you set this.name = "YourErrorType" in your custom error constructor, you can check if e.name === "YourErrorType". It's not necessarily better, but duck typing on a discriminant seems a little better than relying on two packages using the same constructor function for an error.

It'll also print your custom error's class name when toString is called, along with message.

Luckily, we have full control here, since we both define and are the only ones throwing this error. I think I'll leave it as-is for now, but something we might want to throw into the next JS/TS team discussion around preferred coding patterns.

sdk/identity/identity/src/credentials/interactiveBrowserCredential.ts

xirzec · 2020-09-23T23:00:12Z

sdk/identity/identity/review/identity.api.md

+    cacheOptions?: {
+        cachePlugin?: {
+            readFromStorage: () => Promise<string>;
+            writeToStorage: (getMergedState: (oldState: string) => string) => Promise<void>;


feels kinda bad that write isn't the same shape as read... what does getMergedState() do?

This comes from having to merge our changes into the cache rather than just writing out (other applications and/or other languages may have written to the shared cache)

Right, so you always hand back oldState being the last string you read in with readFromStorage and then write that? Or something else?

Generally, the user won't even fulfill this contract. We'll soon be providing helper functions that will create the storage/persistence plugin for you.

I'm tempted to chalk this up as some of the underlying implementation leaking through a little bit, though I'm not sure how to resolve as we still currently allow users to implement their own persistence plugins.

@schaabs - any thoughts?

We've been back and forth on whether we'll be adding APIs to allow people to write the cache themselves. If we can hide these for now it would be ideal. But given it's a preview if we can't easily hide these in the short term we could include them, but we should be clear it's subject to change.

sadasant · 2020-09-24T15:28:26Z

sdk/identity/identity/src/client/identityClient.ts

+        status: response.status
+      };
+    });
+  }


Sorry to ask, but I'd like to know: Don't we need tracing here?

sadasant · 2020-09-24T15:42:31Z

sdk/identity/identity/src/credentials/interactiveBrowserCredential.ts

+    this.port = parseInt(url.port);
+    if (isNaN(this.port)) {
+      this.port = 80;
+    }


In the odd scenario where somebody has the processor when this method is called, could it be possible for something actively reading this property to read it in a NaN state?

If that isn't plausible, then ignore this! 🌞

Not sure what you're asking here. Though maybe this will help clarify the above, from parseInt docs:

Return value An integer parsed from the given string. Or NaN when the radix is smaller than 2 or bigger than 36, or the first non-whitespace character cannot be converted to a number.

There may be clearer ways to do the above. I'm just detecting the NaN case and defaulting the port.

schaabs · 2020-09-28T18:18:56Z

sdk/identity/identity/review/identity.api.md

@@ -31,6 +31,15 @@ export class AuthenticationError extends Error {
 // @public
 export const AuthenticationErrorName = "AuthenticationError";

+// @public
+export interface AuthenticationRecord {


AuthenticationRecord should have a clientId property as well as well.

It comes from https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-common/src/account/AccountInfo.ts#L14-L20

Should we take in clientId and repackage it for MSAL as one of their fields?

This data should be returned in the response from acquireToken***.

Actually, I take that back. In .NET we take that data into the constructor of the InteractiveBrowserCredential and store it as a property, ClientId, on the instance:

https://github.com/Azure/azure-sdk-for-net/blob/a8a0e8cdf5528017fa3dfd22cae81a879e7f0d39/sdk/identity/Azure.Identity/src/InteractiveBrowserCredential.cs#L75-L77

Then when we interactively authenticate we update the AuthenticationRecord also passing in the stored ClientId:

https://github.com/Azure/azure-sdk-for-net/blob/a8a0e8cdf5528017fa3dfd22cae81a879e7f0d39/sdk/identity/Azure.Identity/src/InteractiveBrowserCredential.cs#L207-L214

Not quite sure I follow. Are you saying you do repack the value for MSAL and rename the field? Or just that .NET works differently than msal-node

schaabs · 2020-09-28T18:50:40Z

sdk/identity/identity/src/credentials/interactiveBrowserCredential.ts

+        expiresOnTimestamp: response.expiresOn.getTime(),
+        token: response.accessToken
+      };
+    } catch (e) {


I'm not sure that handling ALL exceptions thrown from acquireTokenSilent is the right thing to do here. Does MSAL document an particular error they will raise in the case that interaction is needed?

From the MSAL docs:

"In case the refresh_token is expired or not found, an error is thrown and the guidance is for the user to call any interactive token acquisition API (eg: acquireTokenByCode())."

We could ask them for more details, and see if they're comfortable locking down the exception side of the API or exposing more information.

Yes, we should follow up with them on this. It would be nice to make the error contract for these cases a bit tighter.

This sounds like it's out of scope for the time being. But they can look into it for a follow-up. I think we can take their change and modify what we do. It doesn't feel like necessarily a breaking change for us.

schaabs

I think the current design is ok for a preview so we can get feedback here. We're still finalizing the design across languages so I don't think we should block getting this into this preview. Looks good from my perspective.

xirzec

I think this is fine, with the caveat that as-is this package will eventually break due to MSAL breaking changes.

Left some comments around async functions and custom errors. We really need a better custom error story for our SDKs.

xirzec · 2020-09-29T20:02:46Z

sdk/identity/identity/src/client/identityClient.ts

@@ -184,6 +190,36 @@ export class IdentityClient extends ServiceClient {
    }
  }

+  sendGetRequestAsync<T>(


why isn't this an async function so we can use await inside instead?

xirzec · 2020-09-29T20:03:30Z

sdk/identity/identity/src/client/identityClient.ts

+    });
+  }
+
+  sendPostRequestAsync<T>(


this can also be an async function. The Async suffix on these methods feels very .NET though, we don't follow that convention anywhere else.

xirzec · 2020-09-29T20:05:21Z

sdk/identity/identity/src/credentials/interactiveBrowserCredential.ts

-  tenantId: string;
-  username: string;
-}
+class AuthenticationRequired extends CredentialUnavailable {}


might be worth setting a custom name on the error class. It also feels a little bad to not have the suffix Error here, as all the built-in JS errors are suffixed with Error.

Oh and if you're going to make a custom error type, shouldn't you export it somewhere?

You can see how we set the prototype and name in RestError here: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-http/src/restError.ts

xirzec · 2020-09-29T20:06:57Z

sdk/identity/identity/src/credentials/interactiveBrowserCredential.ts

@@ -66,6 +65,12 @@ export class InteractiveBrowserCredential implements TokenCredential {
      this.redirectUri = "http://localhost";
    }

+    const url = new URL(this.redirectUri);


I believe this won't work on older Nodes. You can see how I do this in core-https: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-https/src/util/url.ts

xirzec · 2020-09-29T20:09:25Z

sdk/identity/identity/src/credentials/interactiveBrowserCredential.ts

@@ -104,7 +109,37 @@ export class InteractiveBrowserCredential implements TokenCredential {
  ): Promise<AccessToken | null> {
    const scopeArray = typeof scopes === "object" ? scopes : [scopes];

-    return this.acquireTokenFromBrowser(scopeArray);
+    if (this.authenticationRecord && this.persistenceEnabled) {
+      return this.acquireTokenFromCache().catch((e) => {


not sure why this method isn't async so you can try/catch here instead of using .catch

sophiajt · 2020-09-29T20:17:46Z

Caveats noted. I'm working on a follow up PR that should address many of these, but it may not quite make it by the deadline for beta 2. Landing this just in case, but I hope to improve upon the shortcomings of this PR soon.

[Hub Generated] Review request for Microsoft.StorageCache to add version stable/2020-10-01 (Azure#11407) * Init the next version so diffs can work better. * Updates readme * Updates API version in new specs and examples * Updates to StorageTargetProperties and added examples of cmk, mtu Add 202 to the storage cache and storage target create/update and update examples Fix ST no junction example's 202 response. add properties for nfs extended groups fix issues with extended groups properties found with autorest add properties and objects for Active Directory username download undo unintended change changes from comments in pull request AccessPolicy support. Example fixes for Access Policies. Fix attribute names to match latest RP. update to credential properties for LDAP and Active Directory marking password properties with x-ms-secret tag minor changes on extended groups and add examples Added blob NFS and some other validation fixes. Update required property for domainName from dnsName Updated blobNfs examples and some kpi fixes. Correct validation errors in examples. Added systemdata to resources. Remove x-ms-secret in keyvault reference due to linter error and common types not using it. Remove blobNfs from this version. Remove blobNfs from spec file. Remove x-ms-secret due to linter errors. Fix certificate spelling. Updating prettier and spell check errors. Used prettier on main spec file. Readded x-ms-secret that open api hub failed on but the PR pipeline allows. * Add prettier fix after rebase * Remove 202 bodies and add systemData to examples. * Update spec with prettier. * Address comments on spec for descriptions, readmes, and extendedGroupsEnabled. * Updating to address addition ldap and pattern comments. * Update version tag to include 01 * Restore changes that would be considered breaking changes matching 2020-03-01 * Address a few other comments. * Update netbios field names and some descriptions. * Fix s360 for missing debugInfo operation. * Fix credscan error in example file. * Update required fields, new validator failure on debugInfo return codes, and bindPassword example value. * Update debug info example with new return codes. * Update other credscan password errors.

Jonathan Turner added 6 commits September 21, 2020 12:01

Add in configurable http pipeline and cache

719e143

Add in configurable http pipeline and cache

7b3136f

Fix port, cache failure, console.logs

b822b66

Fix port, cache failure, console.logs

b96f61b

Merge branch 'master' into msal_wip4

47d4390

Merge master, remove unused dep

ff652ab

sophiajt requested review from sadasant and schaabs September 22, 2020 20:10

sophiajt requested review from bterlson, daviwil and xirzec as code owners September 22, 2020 20:10

ghost added the Azure.Identity label Sep 22, 2020

xirzec reviewed Sep 23, 2020

View reviewed changes

Jonathan Turner added 6 commits September 24, 2020 11:33

Update to latest msal-node

d8bd361

Update to latest msal-node

c25ade8

Address feedback

8aec185

Export AuthenticationRecord

f351060

Export AuthenticationRecord

e3af557

Don't expose the access token

05eeb4f

sadasant reviewed Sep 24, 2020

View reviewed changes

schaabs reviewed Sep 28, 2020

View reviewed changes

schaabs self-requested a review September 29, 2020 19:46

schaabs approved these changes Sep 29, 2020

View reviewed changes

xirzec approved these changes Sep 29, 2020

View reviewed changes

sophiajt merged commit b9eb1c5 into Azure:master Sep 29, 2020

sophiajt deleted the msal_wip4 branch September 29, 2020 20:17

sadasant mentioned this pull request Oct 5, 2020

[Identity] Changelog updates before releasing #11646

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Identity] Improvements to MSAL support #11407

[Identity] Improvements to MSAL support #11407

sophiajt commented Sep 22, 2020

xirzec left a comment

xirzec Sep 23, 2020

sophiajt Sep 23, 2020

xirzec Sep 23, 2020

sophiajt Sep 23, 2020

xirzec Sep 23, 2020

xirzec Sep 23, 2020

sophiajt Sep 24, 2020

xirzec Sep 23, 2020

sophiajt Sep 23, 2020

xirzec Sep 23, 2020

sophiajt Sep 23, 2020

schaabs Sep 28, 2020

sadasant Sep 24, 2020 •

edited

Loading

sadasant Sep 24, 2020

sophiajt Sep 27, 2020

schaabs Sep 28, 2020

sophiajt Sep 28, 2020

schaabs Sep 28, 2020

schaabs Sep 28, 2020 •

edited

Loading

sophiajt Sep 28, 2020

schaabs Sep 28, 2020

sophiajt Sep 28, 2020

schaabs Sep 28, 2020

sophiajt Sep 29, 2020

schaabs left a comment

xirzec left a comment

xirzec Sep 29, 2020

xirzec Sep 29, 2020

xirzec Sep 29, 2020

xirzec Sep 29, 2020

xirzec Sep 29, 2020

xirzec Sep 29, 2020

sophiajt commented Sep 29, 2020

[Identity] Improvements to MSAL support #11407

[Identity] Improvements to MSAL support #11407

Conversation

sophiajt commented Sep 22, 2020

xirzec left a 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

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

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 Sep 24, 2020 • 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

Choose a reason for hiding this comment

schaabs Sep 28, 2020 • 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

Choose a reason for hiding this comment

schaabs left a comment

Choose a reason for hiding this comment

xirzec left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiajt commented Sep 29, 2020

sadasant Sep 24, 2020 •

edited

Loading

schaabs Sep 28, 2020 •

edited

Loading