Fix AcquireTokenSilent for ADFS users #446
Conversation
|
No, this is not the correct approach. The id of an ADFS account it based on the string homeAccountId = clientInfo?.ToAccountIdentifier() ?? idToken?.Subject; |
chlowell
added
bug
|
Kudos, SonarCloud Quality Gate passed!
|
| // a home account ID is required to find user tokens in the cache | ||
| if o.account.HomeAccountID == "" { | ||
| // an account is required to find user tokens in the cache | ||
| if reflect.ValueOf(o.account).IsZero() { | ||
| return AuthResult{}, errNoAccount |
There was a problem hiding this comment.
@chlowell would you guys consider exposing this errNoAccount as ErrNoAccount so us devs can use errors.Is()? I'm doing a string comparison currently and it feels messy.
There was a problem hiding this comment.
We should. Please open a bug.
But would like to understand your scenario better. In a public client (desktop app) scenario, if the silent auth fails for whatever reason, you should go to interactive auth. Why do you need this level of detail programatically?
Aside: sub error codes like AADSTS700082 can be useful for logging / metrics, but should not be part of app logic, as they are not guarateed to not change (or at least they can be expanded)
#426 broke silent auth for ADFS users by requiring a home account ID, which ADFS accounts don't have. This PR restores the v1.0.0 behavior for ADFS by relaxing that requirement, allowing any nonzero account for silent auth. However, I'm uncertain the prior behavior was correct. Because the home account ID for ADFS accounts and tokens is always
"", the cache matches access tokens on realm, client ID and scope alone, effectively ignoring the user's identity. If the cache has tokens for more than one ADFS user, it will return the first token it considers regardless of the owner. @rayluo @bgavrilMS should the cache match something else in this case such as the local account ID?