-
Notifications
You must be signed in to change notification settings - Fork 340
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
Device code flow fixes when broker is enabled #4846
Device code flow fixes when broker is enabled #4846
Conversation
src/client/Microsoft.Identity.Client/Cache/Items/MsalAccountCacheItem.cs
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.
Approved with comments and questions
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 was not in the conversation of how to fix the issue. The code is aligned with the description "WAM does not support device code flow so mark the account with account source to indicate device code flow constructed this account and never go to WAM for subsequent silent token calls", LGTM.
/// The initial flow that established the account. For example, device code flow. | ||
/// </summary> | ||
/// <remarks>Can be null. Currently only device code flow updates this property with a valid string</remarks> | ||
string AccountSource { get; } |
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.
This is a breaking change. You can't add properties to an interface. Please do not ship this.
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.
IMO this should not be added to the public API.
Fixes #4786
Changes proposed in this request
Testing
Integration test added.
Also tested manually using the device code flow functionality added to the test app.
Performance impact
Negligible. Just an extra account property to read from and write to the cache.