-
Notifications
You must be signed in to change notification settings - Fork 87
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
Support client capabilities and claims #355
Conversation
3ca81a9
to
5aaf796
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Leaving a couple minor comments below.
case "password": | ||
mockClient.AppendResponse(mock.WithBody([]byte(`{"account_type":"Managed","cloud_audience_urn":".","cloud_instance_name":".","domain_name":"."}`))) | ||
case "passwordFederated": | ||
mockClient.AppendResponse(mock.WithBody([]byte(`{"account_type":"Federated","cloud_audience_urn":".","cloud_instance_name":".","domain_name":".","federation_protocol":".","federation_metadata_url":"."}`))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of the scope of this PR, but a FYI: MSALs are allowing confidential clients to use password flow (internal work item).
claims: `{"access_token":{"nbf":{"essential":true, "value":"42"}}}`, | ||
expected: `{"access_token":{"nbf":{"essential":true, "value":"42"}, "xms_cc":{"values":["cp1","cp2"]}}}`, | ||
}, | ||
} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a great unit test, very easy to read and understand.
func (cca Client) AcquireTokenSilent(ctx context.Context, scopes []string, opts ...AcquireSilentOption) (AuthResult, error) { | ||
o := AcquireTokenSilentOptions{} | ||
if err := options.ApplyOptions(&o, opts); err != nil { | ||
return AuthResult{}, err | ||
} | ||
|
||
if o.claims != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it WithClaims() that adds this in? If so, maybe we shouldn't mention it as one of the Options since it will lead immediately into an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You raise a good point. It will one day be possible for this method to succeed when given claims; we can add the option then. The value of taking the option today is in maintaining the calling pattern used in all other cases (see e.g. azidentity):
- call
AcquireTokenSilent
(check the cache) - if that returned an error, call
AcquireTokenBy*
(request a new token)
Applications expect errors from AcquireTokenSilent
because it commonly returns them, which is okay because that's just a cache miss. Having an option that guarantees it will return an error therefore doesn't make users write more, different, or tricky code. On the other hand, if this method doesn't take claims, the probably miniscule population of users who use this feature will have to write conditional code around it, then rewrite that code when this method does take claims. But sure, it's weird to have an anti-useful option.
Edit: whoops, I buried the lede there: having this option steers users away from an authorization failure loop. When a user presents an access token to some RP and receives a claims challenge in response, that means the token isn't valid, even if it hasn't yet expired; the user must acquire a new token having the requested claims. The MSAL client doesn't know this because it doesn't observe requests to the RP. So, a user who receives a claims challenge and proceeds to call AcquireTokenSilent
will probably get an invalid token. This is what I was getting at above when I wrote that not having this option compels users to write conditional code: they would have to know not to call AcquireTokenSilent
when handling a claims challenge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I get what you mean - it's definitely not a straightforward case and it sounds like it's critical to consider that it's running as part of an overall pattern of calls, not just on its own (if I'm following your logic).
Resolving for now, explanation makes sense to me.
Closes #263 by adding a
WithClientCapabilities
option for client constructors and aWithClaims
option for token acquisition methods. Usage looks like this:(confidential and public clients have the same options)
I believe I've covered all the bases here (see the e2e tests) but please look closely; many code paths are impacted. Let me highlight a few subtleties:
AcquireTokenSilent(..., WithClaims("..."))
won't return a cached access token. This means it always fails for confidential clients. It can succeed for public clients that have a refresh token for the account. Similarly,AcquireTokenOnBehalfOf
can succeed silently for confidential clients.