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

Ensure cache interoperability with other MSALs #367

Closed
chlowell opened this issue Dec 27, 2022 · 3 comments · Fixed by #425
Closed

Ensure cache interoperability with other MSALs #367

chlowell opened this issue Dec 27, 2022 · 3 comments · Fixed by #425
Labels

Comments

@chlowell
Copy link
Collaborator

Inconsistencies in cache data written by MSAL Go and MSAL Python could prevent interop between them. I've noticed these differences (there may be others):

  • Casing of data types in keys e.g., in Go we have "AccessToken", "AppMetadata", "IDToken" where Python has the same but lowercased
    • note that changing key format could make caches written by a prior version unreadable
  • MSAL Python writes more fields than MSAL Go
    • AccessToken token_type
    • RefreshToken last_modification_time and target

The work here is writing unit tests to ensure MSAL Go writes correct cache entries and fixing any bugs causing it not to do so.

Related: #58

@bgavrilMS
Copy link
Member

  1. MSAL.NET also has CamelCase naming. It should be expected that MSALs perform all string operations in a case insensitive way (although note that as per OIDC spec, some strings are case sensitive, like scopes, but we've never came across an issue in this area).

  2. token_type is an optional field. It's absence means that the token is Bearer. Once MSAL Go will support other token types (e.g. PoP), then this field becomes more important

  3. target is an optional field for RT. target is represents the scopes for which the token is defined. And AAD refresh tokens are what we call "Multi-Resource RT", i.e. the same RT can be used to get a token for several resources and scopes. Since MSAL Go only supports AAD authorites, this is fine (same applies for B2C and ADFS).

  4. last_modification_time - optional field. Not used and not set by MSAL.NET either. Not sure what MSAL Py uses it for. @rayluo ?

For full reference, please see https://identitydivision.visualstudio.com/DevEx/_git/AuthLibrariesApiReview?path=/SSO/Schema.md&_a=contents&version=GBdev

I think this issue can be closed.

@element-of-surprise
Copy link
Contributor

If all SDKs are required to handle these fields regardless of case, then we should be good. Go will read in the value regardless of the case.

As the others are optional fields we don't need, can I close this @chlowell ?

@rayluo
Copy link
Contributor

rayluo commented Apr 12, 2023

Casing of data types in keys e.g., in Go we have "AccessToken", "AppMetadata", "IDToken" where Python has the same but lowercased

MSAL.NET also has CamelCase naming. It should be expected that MSALs perform all string operations in a case insensitive way (although note that as per OIDC spec, some strings are case sensitive, like scopes, but we've never came across an issue in this area).

MSAL Python's "key makers" deliberately converts all token keys into lowercase (such as this one). This convertion is required by the internal token cache schema, specifically in the "key case sensitivity" section: "Each key component should be normalized before talking to the system persistence layer. Normalization includes: Lowercasing ...".

Therefore, I would suggest MSAL Go to change the CamelCase in persistence layer to lowercase, to comply with the schema. This move is expected to be smooth, because "it should be expected that MSALs perform all string operations in a case insensitive way". Same applies to MSAL .Net, too. CC: @bgavrilMS

MSAL Python writes more fields than MSAL Go

  • RefreshToken last_modification_time

last_modification_time - optional field. Not used and not set by MSAL.NET either. Not sure what MSAL Py uses it for. @rayluo ?

It is optional, so, by definition it is OK to not utilize it.

MSAL Python uses it in this situation (surprisingly, the github search result happens to also contain the inline comment, quoted below): "Since unfit RTs would not be aggressively removed (from token cache), we start from newer RTs which are more likely fit".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants