Refactored public surface area for device code flow #68

jhendrixMSFT · 2020-10-29T19:11:24Z

Renamed PublicClientApplication constructor function and updated its sig
to take an options struct.
Updated AcquireTokenSilent() and AcquireTokenByDeviceCode() APIs to take
a context.Context and options structs.
Removed "Get" prefix from getter-type methods.
Replaced some interface return types with their underlying type.

Renamed PublicClientApplication constructor function and updated its sig to take an options struct. Updated AcquireTokenSilent() and AcquireTokenByDeviceCode() APIs to take a context.Context and options structs. Removed "Get" prefix from getter-type methods. Replaced some interface return types with their underlying type.

msal/public_client_application.go

keegan-caruso · 2020-10-30T22:43:44Z

test/devapps/username_password_sample.go

+	var err error
+	if userAccount != nil {
+		// found a cached account, now see if an applicable token has been cached
+		// NOTE: this API conflates error states, i.e. err is non-nil if an applicable token isn't


Any suggestions on this?

I've been thinking about this one a bit. It's somewhat similar to calling an io.Reader returning io.EOF at the end of a stream.

I'm not familiar enough with how customers use this functionality. Does it seem valuable to return a distinct error, e.g. msal.ErrNoToken (something like that)? Are there use-cases where distinguishing this specific case is valuable?

MSAL errors codes are mostly well known. e.g. it may be a MsalClientException, an MsalServiceException. And there are specific error codes that lets a developer write the code in a specific way. we should do the same here. Java, Python, Node and .NET show cases this.

However error handling in general is not taken care of across the library

In reply to: 515436829 [](ancestors = 515436829)

I took a cursory look at the C# implementation and it doesn't appear to throw an exception if the account isn't found (it's not doc'ed that way, and I dug a bit into the implementation and didn't see anyplace where it obviously throws).

GetAccounts doesn't throw. My comment was arround the API in general (sorry for not being clear about that). My main comment on errors was that we should take that as a separate work item (already tracked elsewhere). We should simply follow the std. msal patterns for user flows and that will lead to specific expected errors that guide the developer and the user experience.

jhendrixMSFT · 2020-11-02T22:12:15Z

msal/public_client_application.go

+}
+
+// AcquireTokenByDeviceCodeOptions contains the optional parameters used to acquire an access token using the device code flow.
+type AcquireTokenByDeviceCodeOptions struct {


We use this in our track 2 SDKs to reduce the chance of breaking changes. It works well for generated code where there tends to be more churn. For hand-written APIs where we can be tactical it might not make as much sense. Any thoughts on this one?

I think it's fine to have an option object for all flows, though it there is nothing today then likelihood of anything in the future is relatively small. I would though actually have expected some option

In reply to: 516285138 [](ancestors = 516285138)

element-of-surprise · 2020-11-02T23:39:17Z

msal/confidential_client_application.go

 )

+// ConfidentialClientApplicationOptions configures the PublicClientApplication's behavior.
+type ConfidentialClientApplicationOptions struct {


A few things I'd note here:

We really need to do something about these names (though probably not in this PR)

We should not use the options struct. This is non-go standard. I know the official SDK track 2 guidelines say to do this, but since we aren't in the SDK, we should take advantage of variardic options. This struct would become private and modified by those args. It handles better defaults.

HTTPClient, which we are renaming somewhere above, should just be eliminated in the end. If we really have a use case, we can add it back later (but I doubt we will).

The Go blog details both variadic config and optional args struct as cromulent ways for handling configuration. There's a nice write-up as part of the module compatibility blog post.

https://blog.golang.org/module-compatibility

We (the Go SDK team) went round and round on this, and in the end we felt that the self-documenting properties of the optional params struct is the better customer experience. Note that we haven't had any usability studies as of yet, so our position may change.

Having a configurable HTTP client allows the use of something like our pipeline middleware for controlling request/response lifetime. Most common case is the ability to specify a retry policy for handling transient failures. At any rate we can defer it for now.

And to be clear, I'm not saying it has to be optional args struct, just providing my perspective (while I do think there's benefit of having consistency across SDKs, this isn't the hill I'm gonna die on).

We have bring your own HttpClient in all our libraries. Has been a wish so many times that we are providing it out of the box with new libraries we write.

henrik-me · 2020-11-03T00:20:22Z

While there is more work to be done, I think think change makes the MSAL api more clear and is an improvement.

jhendrixMSFT · 2020-11-03T00:29:49Z

Addresses #60

jmprieur

LGTM
I left a few comments and questions, mainly about the consistency with the other MSAL libraries

jmprieur · 2020-11-03T10:49:45Z

msal/confidential_client_application.go


-// GetAccounts gets all the accounts in the token cache.
-func (cca *ConfidentialClientApplication) GetAccounts() []AccountProvider {
+// Accounts gets all the accounts in the token cache.


Does it make sense to returns the accounts in a confidential client application given that there will be one cache per account, and therefore Accounts will always be empty?
We have a GitHub issue to remove it from the ConfidentialClientApplication public API in MSAL.NET. See AzureAD/microsoft-authentication-library-for-dotnet#2141

Opened #73 to track this.

msal/public_client_application.go

msal/public_client_application_test.go

jmprieur · 2020-11-03T10:55:06Z

test/devapps/authorization_code_sample.go

-	authCodeParams.Code = code
-	authCodeParams.CodeChallenge = config.CodeChallenge
-	result, err := publicClientApp.AcquireTokenByAuthCode(authCodeParams)
+	result, err := publicClientApp.AcquireTokenByAuthCode(context.Background(), config.Scopes, config.RedirectURI, &msal.AcquireTokenByAuthCodeOptions{


For Auth Code flow there is a need for a reply URI, but it should be https://login.microsoftonline.com/common/oauth2/nativeclient. Do we need to pass it as a parameter?

Is it always the same URL? What about for Azure Stack and/or sovereign clouds?

yes, it's always the same URL. Just there is a need for one, but MSAL stops the redirect (to that URLà, and grabs the auth code on the fly, so it's never reached.

I see. So just to confirm, we always specify this same redirect URL but since we never follow it there's no need to make it configurable, is that correct?

The redirectURI param has been removed.

@jmprieur I've removed the param, anything else or is this good to merge?

test/devapps/device_code_flow_sample.go

jmprieur · 2020-11-03T11:01:18Z

test/devapps/device_code_flow_sample.go

-		result, err := publicClientApp.AcquireTokenSilent(silentParams)
+	var result *msalbase.AuthenticationResult
+	var err error
+	if userAccount != nil {


Do we need to test if the userAccount is not nil? couldn't just AcquireTokenSilent return an error if the userAccount is nil? (Other MSALs do that)

With the current implementation, a non-nil error is ambiguous; it means that either the user account wasn't found or that something else failed (e.g. perhaps there was a transient IO failure making the network request).

I think we need to decide on the semantics we want here, and also look at what kind of error(s) other implementations expose here. I've opened #70 to track standardizing on error types.

keegan-caruso

LGTM. Definitely a step in the right direction.

sonarqubecloud · 2020-11-05T19:14:16Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
4.0% Duplication

jhendrixMSFT · 2020-11-12T00:06:56Z

Fixes #56

jennyf19

keegan-caruso reviewed Oct 29, 2020

View reviewed changes

msal/public_client_application.go Outdated Show resolved Hide resolved

keegan-caruso reviewed Oct 29, 2020

View reviewed changes

msal/public_client_application.go Outdated Show resolved Hide resolved

fix up remaining APIs on public and confidential clients

5a0a6e7

keegan-caruso reviewed Oct 30, 2020

View reviewed changes

jhendrixMSFT requested a review from element-of-surprise November 2, 2020 21:34

rename Client -> HTTPClient

d031ee4

jhendrixMSFT commented Nov 2, 2020

View reviewed changes

element-of-surprise reviewed Nov 2, 2020

View reviewed changes

jhendrixMSFT marked this pull request as ready for review November 3, 2020 00:21

jhendrixMSFT mentioned this pull request Nov 3, 2020

[Cleanup] Return standardized error types #70

Closed

jmprieur reviewed Nov 3, 2020

View reviewed changes

set default value for AAD authority

6613df4

This was referenced Nov 3, 2020

[Bug] Include openid claim by default #72

Closed

[Cleanup] Return single account for ConfidentialClientApplication #73

Closed

keegan-caruso approved these changes Nov 4, 2020

View reviewed changes

remove redirectURI param from device code APIs

d9e22a9

jennyf19 approved these changes Nov 12, 2020

View reviewed changes

jhendrixMSFT merged commit 2a54de3 into AzureAD:dev Nov 12, 2020

jhendrixMSFT deleted the device_code_apis branch November 12, 2020 17:28

jhendrixMSFT mentioned this pull request Nov 16, 2020

Remove RedirectURL parameter from auth code flow Azure/azure-sdk-for-go#13565

Merged

5 tasks

jhendrixMSFT mentioned this pull request Dec 7, 2020

Support for setting redirect-url in token-fetch portion of confidential-app authentication code flow #96

Closed

Refactored public surface area for device code flow #68

Refactored public surface area for device code flow #68

Uh oh!

Conversation

jhendrixMSFT commented Oct 29, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrik-me Oct 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhendrixMSFT Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhendrixMSFT Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrik-me commented Nov 3, 2020

Uh oh!

jhendrixMSFT commented Nov 3, 2020

Uh oh!

jmprieur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmprieur Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keegan-caruso left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 5, 2020

Uh oh!

jhendrixMSFT commented Nov 12, 2020

Uh oh!

jennyf19 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

henrik-me Oct 31, 2020 •

edited

Loading

jhendrixMSFT Nov 2, 2020 •

edited

Loading

jhendrixMSFT Nov 2, 2020 •

edited

Loading

jmprieur Nov 4, 2020 •

edited

Loading