-
Notifications
You must be signed in to change notification settings - Fork 88
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
Multitenant silent authentication fixes #366
Conversation
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.
Thanks for fixing this @chlowell. I verified that it fixes my larger end to end scenario and thanks for taking my repo and turning it into a test.
FWIW - It looks like some of the option types have been renamed (and the signature of some public methods changed) which meant I had to do a little surgery both in the azidentity
package and my code (which uses MSAL directly) to adopt the breaking changes. Not sure if you intend to roll the back on not.
Thanks again for your help here!
@@ -84,6 +84,7 @@ func isMatchingScopes(scopesOne []string, scopesTwo string) bool { | |||
|
|||
// Read reads a storage token from the cache if it exists. | |||
func (m *Manager) Read(ctx context.Context, authParameters authority.AuthParams, account shared.Account) (TokenResponse, error) { | |||
tr := TokenResponse{} |
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.
Do you think there is value in still returning an error in the case where we fail to read some parts of TokenResponse
to communicate to our caller that we were only able to do a partial read? In general, I know you shouldn't touch any of the other returned values when error is non nil, but I've seen some go APIs that document what values are correct when an error is returned - I imagine that we could say we could trust an non-zero part of the structure.
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'm just worried that some consumer is going to call Read()
in this state, fail to find an ID token, but since an error is not returned, they go ahead and party on the zero value of the ID Token and expect it to be something sensible.
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.
Sure, that's possible in principle, though I think zero values here are obviously invalid. Because this is an internal API, I don't mind trusting callers to continue following the convention of validating cache data before using it (see AuthResultFromStorage()
). If that proves unworkable, I would rather decompose Read()
into ReadAccessToken()
, ReadIDToken()
, etc. than ask callers to ignore irrelevant errors.
|
Whenever you silently authenticate a user in a new tenant e.g., authenticate a user in tenant A then silently authenticate that user in tenant B. The cache doesn't return an ID token in this scenario because it searches for one whose realm equals the requested tenant (here; |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
This fixes a couple bugs: