-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[v5] Changes to Configuration - CacheOptions (Config #2) #7697
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
base: msal-v5
Are you sure you want to change the base?
Changes from all commits
0cb8305
becbd55
9ecf558
279d602
91f9f57
b581414
a0187a4
6bf2b16
d6931e4
bac042d
385b4d5
e0f8803
2bc9137
cd106bc
88e033a
4539d7d
a3fb3fc
9e89fb9
431bced
e13705c
cdf34c1
79e0a34
00a3f90
5864f6c
35cb314
48373e6
4ccb22b
8ac7249
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,7 +109,6 @@ body: | |
}, | ||
cache: { | ||
cacheLocation: "sessionStorage" | ||
storeAuthStateInCookie: false | ||
} | ||
} | ||
validations: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "major", | ||
"comment": "Configuration changes to CacheOptions #7697", | ||
"packageName": "@azure/msal-browser", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "minor", | ||
"comment": "Deprecate claimsBasedCachingEnabled as part of Configuration change #7697", | ||
"packageName": "@azure/msal-common", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,10 +22,6 @@ const msalConfig = { | |
}, | ||
cache: { | ||
cacheLocation: "sessionStorage", | ||
temporaryCacheLocation: "sessionStorage", | ||
storeAuthStateInCookie: false, | ||
secureCookies: false, | ||
claimsBasedCachingEnabled: true, | ||
}, | ||
system: { | ||
loggerOptions: { | ||
|
@@ -96,16 +92,6 @@ const msalInstance = new PublicClientApplication(msalConfig); | |
| --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------- | --------------------------------------------------- | | ||
| `cacheLocation` | Location of token cache in browser. | String value that must be one of the following: `"sessionStorage"`, `"localStorage"`, `"memoryStorage"` | `sessionStorage` | | ||
|
||
**Note: The following cache config options are deprecated and will be removed in the next major version** | ||
|
||
| Option | Description | Format | Default Value | | ||
| --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------- | --------------------------------------------------- | | ||
| `temporaryCacheLocation` | Location of temporary cache in browser. This option should only be changed for specific edge cases. Please refer to [caching](https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-browser/docs/caching.md#cached-artifacts) for more. | String value that must be one of the following: `"sessionStorage"`, `"localStorage"`, `"memoryStorage"` | `sessionStorage` | | ||
| `storeAuthStateInCookie` | If true, stores cache items in cookies as well as browser cache. Should be set to true for use cases using IE. | boolean | `false` | | ||
| `secureCookies` | If true and `storeAuthStateInCookies` is also enabled, MSAL adds the `Secure` flag to the browser cookie so it can only be sent over HTTPS. | boolean | `false` | | ||
| `cacheMigrationEnabled` | If true, cache entries from older versions of MSAL will be updated to conform to the latest cache schema on startup. If your application has not been recently updated to a new version of MSAL.js you can safely turn this off. In the event old cache entries are not migrated it may result in a cache miss when attempting to retrieve accounts or tokens and affected users may need to re-authenticate to get up to date. | boolean | `true` when using `localStorage`, `false` otherwise | | ||
| `claimsBasedCachingEnabled` | If `true`, access tokens will be cached under a key containing the hash of the requested claims string, resulting in a cache miss and new network token request when the same token request is made with different or missing claims. If set to `false`, tokens will be cached without claims, but all requests containing claims will go to the network and overwrite any previously cached token with the same scopes. | boolean | `false` | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let us ensure we test this, that we never cache claims. I want to make sure we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sameerag See tests added below to ensure no claims cached. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's get claims removed from the cache key as well if we're removing the functionality. Doesn't need to be in this PR, or even a requisite before GA, but let's note it somewhere so we can clean it up at some point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend this before we GA v5. Otherwise, it sounds like we could have some cache behavior changes after removing claims from the cache key. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defining a model for changes like that is what I'm working on now, we need to be able to make cache changes anytime so I don't feel strongly that it needs to happen right away but if we want to plan that in as part of the GA that's totally cool :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to make this a gating feature for GA. @tnorling so this means you will also consider folks transitioning from claims cached to none in your work too? |
||
|
||
See [Caching in MSAL](./caching.md) for more. | ||
|
||
### System Config Options | ||
|
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.
Off the topic, but related. Can we make sure the
one
cookie we set for KMSI is the only one we manage and add it to the docs undercookies MSAL JS manages..
etc. Also link to the migration guide on the discontinuation of existing cookies.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.
Manually tested to ensure that KMSI is the only cookie set and managed. Migration guide will come in a different PR but will make a note this is mentioned.
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.
There was some registration work that Thomas had to do for the KMSI cookie. Did we not have any 1p users using this functionality? Or did they just not reach out to us that they were using it?
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 thought they did, atleast OWA who tested KMSI.