-
Notifications
You must be signed in to change notification settings - Fork 328
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
Pass account to Auth result #3199
Conversation
src/client/Microsoft.Identity.Client/Internal/Requests/Silent/CacheSilentStrategy.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Internal/Requests/Silent/CacheSilentStrategy.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Internal/Requests/Silent/CacheSilentStrategy.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
Outdated
Show resolved
Hide resolved
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.
- No Account for ADFS / B2C shows that we are missing some tests, or some assertion on existing tests
- We now access the CacheManager 3 times - once for Id token, once for tenant profiles and once for account. This is not needed - a single method can do them all.
The tests do not fail since the account object was used only to populate tenant profiles and wam id. Updating the code to use the account passed. |
@@ -26,7 +26,7 @@ internal interface ICacheSessionManager | |||
|
|||
Task<IEnumerable<IAccount>> GetAccountsAsync(); | |||
Task<IDictionary<string, TenantProfile>> GetTenantProfilesAsync(string homeAccountId); | |||
|
|||
Task<Account> GetAccountAssociatedWithAccessTokenAsync(MsalAccessTokenCacheItem msalAccessTokenCacheItem); |
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.
Can we delete GetIdTokenCacheItemAsync
and GetTenantProfilesAsync
from here and make them private methods in TokenCache class?
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.
Deleting GetTenantProfilesAsync
making it private in TokenCache. Leaving GetIdTokenCacheItemAsync
since I did not combine it with tenant profiles as mentioned in #3199 (comment)
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.
Some minor comments remaining (cleanup)
|
||
// Assert | ||
AssertWamIds(response.Account as Account, 1, "wamId2"); | ||
Assert.AreEqual(tenant2, response.TenantId); |
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.
Assert tenant profiles for second account too?
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.
The second account will only have 1 tenant profile since only 1 id token is present for the home account id. That is why it is not verified for multiple tenant profiles. This is same behavior as existing tests on the cache for tenant profiles.
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.
Tested with B2C and SIlentAuthentication. That works too
…library-for-dotnet * 'master' of github.com:AzureAD/microsoft-authentication-library-for-dotnet: (39 commits) OBO for SP with RT support (AzureAD#3266) (AzureAD#3280) Update some projects to net48 (AzureAD#3269) Bump Source Link version (tool change) (AzureAD#3275) Update changelog.txt (AzureAD#3282) Default to WebView1 instead of WebView2 when using AAD or ADFS authorities (AzureAD#3276) Revert "OBO for SP with RT support (AzureAD#3266)" OBO for SP with RT support (AzureAD#3266) Add perf tests for Acquire Token calls with serialization cache and Builders (AzureAD#3250) Minor fix to SuggestedCacheKey comment and update NuGet icon in Readme. (AzureAD#3268) MSAL changelog 4.43.0 (AzureAD#3263) Bogavril/3251 (AzureAD#3255) Fix for AzureAD#3248 - use the correct plugin to sing out (AzureAD#3253) Performance project improvements (AzureAD#3064) Reuse lists in token cache filtering logic. (AzureAD#3233) Update andorid install script to use andorid 30 (AzureAD#3243) Fix for UWP packaging (AzureAD#3239) Pass account to Auth result (AzureAD#3199) App ported to Lab4 (AzureAD#3229) Trwalke/ruleset updates (AzureAD#3189) Conditional Access for Android (AzureAD#3210) ...
Fixes #
#3121
Changes proposed in this request
Pass account to auth result
Testing
Added unit tests
Performance impact