fix: clean acquireToken caching so it doesn't accrue through states #544
Conversation
|
|
||
| this._cacheStorage.setItem(acquireTokenUserKey, JSON.stringify(user)); | ||
| this._cacheStorage.setItem(authorityKey, authenticationRequest.authority, this.storeAuthStateInCookie); | ||
| this._cacheStorage.setItem(Constants.nonceIdToken, authenticationRequest.nonce); |
There was a problem hiding this comment.
The storeAuthStateInCookie flag should be passed in here as well, correct?
There was a problem hiding this comment.
storeAuthStateInCookie was only provided for the authority key in all the previous code sections that this function is replacing. This change is only about consolidating code / eliminating duplicate code while keeping the same functionality. Changes to the functionality, such as including storeAuthStateInCookie for other storage values, is a discussion outside the scope of this change.
There was a problem hiding this comment.
We are using the storeAuthStateInCookie for the nonceIdToken parameter in line 1076 of the file before your changes. We are most likely going to refactor the usage for this functionality in the near future, but for now we will most likely need to pass this here as well.
There was a problem hiding this comment.
@manoj-rath @pkanher617 Added a commit that uses storeAuthStateInCookie for the nonce.
lib/msal-core/src/Storage.ts
Outdated
| @@ -107,18 +107,37 @@ export class Storage {// Singleton | |||
| return results; | |||
| } | |||
|
|
|||
| removeAcquireTokenEntries(authorityKey: string, acquireTokenUserKey: string): void { | |||
| removeAcquireTokenEntries(clearCookies?: boolean): void { | |||
There was a problem hiding this comment.
I would prefer to rename the argument clearCookies to storeAuthStateInCookie to be consistent and clear on the intention.
There was a problem hiding this comment.
The intention for the argument clearCookies is to determine whether or not it should remove related cookies in addition to removing entries from storage. It'd be strange to have an argument that describes storing data in a function that is not storing data. Maybe it can be renamed to removeCookies or removeAuthStateCookies.
It might also be considered that this shouldn't be an optional parameter to begin with, and the function should always attempt to remove the related cookies (if they exist) when it's called.
There was a problem hiding this comment.
@manoj-rath Added a commit to make removeAcquireTokenEntries always remove cookies, rather than conditionally based on an input argument.
…m/zamzowd/microsoft-authentication-library-for-js into zamzowd/clean_acquire_token_cache
|
Any updates on this? |
|
@JulioPablo The main team started on some significant redesigns after this pull request was created. While they're doing that, it looks like they aren't accepting any pull requests on the current version. I'm not sure what the targeted release date of the new preview version is. They recently added this page to their wiki about the redesign - https://github.com/AzureAD/microsoft-authentication-library-for-js/wiki/MSAL.js-1.0.0-preview-changes |
|
@zamzowd Thanks for the update. @JulioPablo this is one of the PRs we want to integrate soon after our redesign changes are in. I have labeled it accordingly to track it. |
|
Closing this as #675 and other related PRs do the same cleanup. |
Fix for #527
The cache entries for the Acquire Token process aren't properly removed, resulting in a build-up of entries (with different state values) over time.
Update to
Storage.removeAcquireTokenEntriesso it removes all entries that are not In Progress, rather than only specified entries.Update to
Storage.resetCacheItemsso it removes all the cache entries for the Acquire Token process.Create private function in
UserAgentApplication.updateAcquireTokenCacheto set the Acquire Token cache values from the user and the authenticationRequest objects. Update to Acquire Token processes to use that function, eliminating duplicate code.Minor cleanup to
UserAgentApplication.saveTokenFromHash.Update tests to reflect cache-clearing responsibility getting moved to
Storage.