Skip to content

Share SharedPreferencesInMemoryCache across cache instances Fixes AB#3428107 #2813

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.

Sign up for GitHub

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

Merged
siddhijain merged 8 commits into from
Nov 15, 2025
Merged

Share SharedPreferencesInMemoryCache across cache instances Fixes AB#3428107 #2813

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
56cdb78
Share SharedPreferencesInMemoryCache across cache instances
siddhijain Nov 14, 2025
9c24275
Incorporate copilot feedback comments.
siddhijain Nov 14, 2025
757b178
Update common4j/src/main/com/microsoft/identity/common/java/cache/Bro…
siddhijain Nov 14, 2025
9102d98
change the cache key
siddhijain Nov 14, 2025
90223bc
added tests + updated documentation
siddhijain Nov 14, 2025
280cffa
Merge branch 'dev' into sjain/static-in-memory-cache-layer
siddhijain Nov 14, 2025
5a67472
clear test flight after tests
siddhijain Nov 15, 2025
d439457
clear test flight after tests
siddhijain Nov 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
vNext
----------
- [MINOR] Share SharedPreferencesInMemoryCache across instances of BrokerOAuth2TokenCache
- [MINOR] Use SharedPreferencesInMemoryCache implementation in Broker (#2802)
- [MINOR] Add OpenTelemetry support for passkey operations (#2795)
- [MINOR] Add passkey registration support for WebView (#2769)
Expand Down
116 changes: 49 additions & 67 deletions common4j/src/main/com/microsoft/identity/common/java/cache/BrokerOAuth2TokenCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

import edu.umd.cs.findbugs.annotations.Nullable;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
Expand Down Expand Up @@ -1337,6 +1339,7 @@ public void clearAll() {

this.mFociCache.clearAll();
this.mApplicationMetadataCache.clear();
inMemoryCacheMapByStorage.clear();
}

/**
Expand Down Expand Up @@ -1641,24 +1644,7 @@ private static MicrosoftFamilyOAuth2TokenCache initializeFociCache(@NonNull fina
private static <T extends MsalOAuth2TokenCache> T getTokenCache(@NonNull final IPlatformComponents components,
@NonNull final INameValueStorage<String> spfm,
boolean isFoci) {
final ICacheKeyValueDelegate cacheKeyValueDelegate = new CacheKeyValueDelegate();
final boolean isFlightEnabled = CommonFlightsManager.INSTANCE
.getFlightsProvider()
.isFlightEnabled(CommonFlight.USE_IN_MEMORY_CACHE_FOR_ACCOUNTS_AND_CREDENTIALS);
final IAccountCredentialCache accountCredentialCache;
if (isFlightEnabled) {
accountCredentialCache = new SharedPreferencesAccountCredentialCacheWithMemoryCache(
cacheKeyValueDelegate,
spfm
);
SpanExtension.current().setAttribute(AttributeName.in_memory_cache_used_for_accounts_and_credentials.name(), true);
} else {
accountCredentialCache = new SharedPreferencesAccountCredentialCache(
cacheKeyValueDelegate,
spfm
);
SpanExtension.current().setAttribute(AttributeName.in_memory_cache_used_for_accounts_and_credentials.name(), false);
}
final IAccountCredentialCache accountCredentialCache = getCacheToBeUsed(spfm);
final MicrosoftStsAccountCredentialAdapter accountCredentialAdapter =
new MicrosoftStsAccountCredentialAdapter();

Expand All @@ -1678,6 +1664,51 @@ private static <T extends MsalOAuth2TokenCache> T getTokenCache(@NonNull final I
);
}

private static final Map<INameValueStorage<String>, SharedPreferencesAccountCredentialCacheWithMemoryCache>
inMemoryCacheMapByStorage = new ConcurrentHashMap<>();
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: Medium – Static cache map not cleared during clearAll() operations

Issue: The static inMemoryCacheMapByStorage map is never cleared, even when BrokerOAuth2TokenCache.clearAll() is called (line 1326). The clearAll() method clears individual token caches and application metadata, but the cached SharedPreferencesAccountCredentialCacheWithMemoryCache instances remain in the static map.

Impact:

  1. After clearAll(), subsequent cache operations may still use the old cached instances which have already been cleared, leading to inconsistent state
  2. The in-memory cache layer within SharedPreferencesAccountCredentialCacheWithMemoryCache will be out of sync with the underlying storage after a clear operation
  3. In testing scenarios, this can cause test pollution where one test's data affects another test even after explicit cache clearing

Recommendation:

  1. Add a static method to clear the inMemoryCacheMapByStorage map (e.g., clearCacheMap()) and call it from clearAll()
  2. Alternatively, call clear() on the map itself within clearAll() to ensure the in-memory cache references are released
  3. Consider whether clearing should trigger reloading of the in-memory cache or if the instances should be invalidated entirely

Example fix:

@Override
public void clearAll() {
    final List<BrokerApplicationMetadata> allClientsMetadata = mApplicationMetadataCache.getAll();
    for (final BrokerApplicationMetadata clientMetadata : allClientsMetadata) {
        final OAuth2TokenCache clientTokenCache = getTokenCacheForClient(
                clientMetadata.getClientId(),
                clientMetadata.getEnvironment(),
                clientMetadata.getUid()
        );

        if (clientTokenCache != null) {
            clientTokenCache.clearAll();
        }
    }

    this.mFociCache.clearAll();
    this.mApplicationMetadataCache.clear();
    inMemoryCacheMapByStorage.clear(); // Clear the static cache map
}

Copilot generated this review using guidance from repository custom instructions.
Copy link
Contributor Author

@siddhijain siddhijain Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in the next commit.

Copy link
Collaborator

@moumighosh moumighosh Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@siddhijain , let's discuss this offline, we should not be auto-resolving CoPilot comments when it is flagging high severity/Medium severity issues. All issues must be addressed in the same PR


/**
* Determines which cache implementation to use based on flighting.
* <p>
* When the flight {@code USE_IN_MEMORY_CACHE_FOR_ACCOUNTS_AND_CREDENTIALS} is enabled,
* returns a shared, cached instance of {@link SharedPreferencesAccountCredentialCacheWithMemoryCache}
* for the given storage, improving performance by reusing the in-memory cache layer across cache instances.
* When disabled, returns a new {@link SharedPreferencesAccountCredentialCache} instance.
* <p>
* Critical behavior: When flight is enabled, the same {@code SharedPreferencesAccountCredentialCacheWithMemoryCache}
* instance is returned for the same {@code spfm} reference, meaning the cache is shared across multiple
* {@code BrokerOAuth2TokenCache} instances.
* <p>
* Thread-safety: This method is thread-safe via {@code ConcurrentHashMap.computeIfAbsent}.
* <p>
* Lifecycle: Returned cached instances are never removed from the static map.
*
* @param spfm The shared preferences file manager / storage instance, used as the key
* for caching when the flight is enabled. The same storage reference will
* return the same cached instance.
* @return A cached shared in-memory cache instance (flight enabled) or a new
* non-cached instance (flight disabled).
*/
private static IAccountCredentialCache getCacheToBeUsed(@NonNull final INameValueStorage<String> spfm) {
final boolean isFlightEnabled = CommonFlightsManager.INSTANCE
.getFlightsProvider()
.isFlightEnabled(CommonFlight.USE_IN_MEMORY_CACHE_FOR_ACCOUNTS_AND_CREDENTIALS);
SpanExtension.current().setAttribute(AttributeName.in_memory_cache_used_for_accounts_and_credentials.name(), isFlightEnabled);
if (isFlightEnabled) {
return inMemoryCacheMapByStorage.computeIfAbsent(spfm, s ->
new SharedPreferencesAccountCredentialCacheWithMemoryCache(
new CacheKeyValueDelegate(),
spfm
)
);
} else {
return new SharedPreferencesAccountCredentialCache(
new CacheKeyValueDelegate(),
spfm
);
}
}

@Nullable
private MsalOAuth2TokenCache getTokenCacheForClient(@Nullable final BrokerApplicationMetadata metadata) {
final String methodName = ":getTokenCacheForClient(bam)";
Expand Down Expand Up @@ -1736,53 +1767,4 @@ private MsalOAuth2TokenCache getTokenCacheForClient(@NonNull final String client

return getTokenCacheForClient(metadata);
}

/**
* Sets the SSO state for the supplied Account, relative to the provided uid.
*
* @param uidStr The uid of the app whose SSO token is being inserted.
* @param account The account for which the supplied token is being inserted.
* @param refreshToken The token to insert.
*/
public void setSingleSignOnState(@NonNull final String uidStr,
@NonNull final GenericAccount account,
@NonNull final GenericRefreshToken refreshToken) {
final String methodName = ":setSingleSignOnState";

final boolean isFrt = refreshToken.getIsFamilyRefreshToken();

MsalOAuth2TokenCache targetCache;

final int uid = Integer.parseInt(uidStr);

if (isFrt) {
Logger.verbose(TAG + methodName, "Saving tokens to foci cache.");

targetCache = mFociCache;
} else {
// If there is an existing cache for this client id, use it. Otherwise, create a new
// one based on the supplied uid.
targetCache = initializeProcessUidCache(getComponents(), uid);
}
try {
targetCacheSetSingleSignOnState(account, refreshToken, targetCache);
updateApplicationMetadataCache(
refreshToken.getClientId(),
refreshToken.getEnvironment(),
refreshToken.getFamilyId(),
uid
);
} catch (ClientException e) {
Logger.warn(
TAG + methodName,
"Failed to save account/refresh token. Skipping."
);
}
}

// Suppressing unchecked warning as the generic type was not provided for targetCache
@SuppressWarnings(WarningType.unchecked_warning)
private void targetCacheSetSingleSignOnState(@NonNull GenericAccount account, @NonNull GenericRefreshToken refreshToken, MsalOAuth2TokenCache targetCache) throws ClientException {
targetCache.setSingleSignOnState(account, refreshToken);
}
}