Skip to content
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

[Enhancement] GetAccountsAsync should no longer be exposed in confidential client applications (via deprecation) #1967

Closed
3 tasks
jmprieur opened this issue Jul 31, 2020 · 1 comment
Assignees
Milestone

Comments

@jmprieur
Copy link
Contributor

jmprieur commented Jul 31, 2020

Which Version of MSAL are you using ?
4.17.0

Platform
net45, netcore

What authentication flow has the issue?

  • Web App
    • Authorization code
    • OBO
  • Web API
    • OBO

Repro

var accounts = await confidentialClientApplications.GetAccountsAsync();

Expected behavior
GetAccountsAsync() does not make sense in confidential client applications as there should be one cache per user and GetAccountsAsync() does not know which cache key to use.

We'd want to have a warning at build time: Use GetAccountAsync in web apps and web APIs, and use a token cache serializer for better security and performance. See https://aka.ms/msal-net-cca-token-cache-serialization.

Proposed spec
In IConfidentialClientApplication, add a new method (with the same signature as in the base interface):

        /// <inheritdoc/>
        [Obsolete("Use GetAccountAsync in web apps and web APIs, and use a token cache serializer for better security and performance. See https://aka.ms/msal-net-cca-token-cache-serialization.")]
        new Task<IEnumerable<IAccount>> GetAccountsAsync()

Given the signature is the same as in the base interface this is not really adding a new method to the interface, and therefore, not a breaking change.

Actual behavior
It's possible to call GetAccountsAsync(), but it always returns 0 accounts (unless developers don't override serialization, but this should not be done in confidential client applications).

@henrik-me henrik-me changed the title [Bug] GetAccountsAsync should no longer be exposed in confidential client applications [Enhancement] GetAccountsAsync should no longer be exposed in confidential client applications Aug 5, 2020
@jmprieur jmprieur added this to the Future Major Version Update milestone Oct 15, 2020
@jmprieur jmprieur modified the milestones: 5.0.0, 4.28.0 Mar 4, 2021
@jmprieur jmprieur added this to Todo in MSAL.NET (legacy) via automation Mar 4, 2021
@jmprieur
Copy link
Contributor Author

jmprieur commented Mar 4, 2021

Proposing, in 4.28.0 to just do the following (non-breaking)

  • add a new method GetAccountsAsync on IConfidentialClientApplication (at the moment it's only on ApplicationBase)
  • add an obsolete attribute, with warning? The message should be: Use GetAccountAsync in web apps and web apis, and use a token cache serializer for better security and performance. See https://aka.ms/msal-net-cca-token-cache-serialization
    @bgavrilMS

@jmprieur jmprieur moved this from Todo to Estimated/Committed in MSAL.NET (legacy) Mar 4, 2021
@bgavrilMS bgavrilMS changed the title [Enhancement] GetAccountsAsync should no longer be exposed in confidential client applications [Enhancement] GetAccountsAsync should no longer be exposed in confidential client applications (via deprecation) Mar 8, 2021
@pmaytak pmaytak self-assigned this Mar 9, 2021
@pmaytak pmaytak moved this from Estimated/Committed to In Progress in MSAL.NET (legacy) Mar 9, 2021
@pmaytak pmaytak moved this from In Progress to Fixed in MSAL.NET (legacy) Mar 9, 2021
@pmaytak pmaytak added Fixed and removed In Progress labels Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

4 participants