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 1 commit
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
99 changes: 32 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 @@ -1641,24 +1643,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 +1663,35 @@ 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.
* @param spfm
* @return
*/
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);
final ICacheKeyValueDelegate cacheKeyValueDelegate = new CacheKeyValueDelegate();
SpanExtension.current().setAttribute(AttributeName.in_memory_cache_used_for_accounts_and_credentials.name(), isFlightEnabled);
if (isFlightEnabled) {
return inMemoryCacheMapByStorage.computeIfAbsent(spfm, s ->
new SharedPreferencesAccountCredentialCacheWithMemoryCache(
cacheKeyValueDelegate,
spfm
)
);
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 – CacheKeyValueDelegate instantiated per call instead of being reused

Issue: A new CacheKeyValueDelegate instance is created on every call to getCacheToBeUsed(), even when returning a cached SharedPreferencesAccountCredentialCacheWithMemoryCache instance. However, the delegate is only used during the initial construction (inside computeIfAbsent) when the cache is first created.

Impact:

  1. Unnecessary object allocation on every cache lookup (minor performance overhead)
  2. Inconsistency: when the flight is disabled, each call creates a new SharedPreferencesAccountCredentialCache with a new delegate (expected), but when enabled, the returned cached instance uses the delegate from the first creation, not the current one
  3. This could cause confusion during debugging if delegate behavior needs to be analyzed

Recommendation: Move the cacheKeyValueDelegate instantiation inside the lambda passed to computeIfAbsent so it's only created when a new cache instance is actually constructed. This makes the code more efficient and clearer:

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
        );
    }
}

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

@siddhijain siddhijain Nov 14, 2025 •
edited
Loading

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 not do this in the future, all comments must be addressed in the same PR

Copy link
Contributor Author

@siddhijain siddhijain Nov 16, 2025

Choose a reason for hiding this comment

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

Yes - I did take care of all the comments in this PR. I commented changes in the next 'commit' not the next 'PR'. This specific comment however got outdated after made key related changes in the PR.
image

} else {
return new SharedPreferencesAccountCredentialCache(
cacheKeyValueDelegate,
spfm
);
}
}

@Nullable
private MsalOAuth2TokenCache getTokenCacheForClient(@Nullable final BrokerApplicationMetadata metadata) {
final String methodName = ":getTokenCacheForClient(bam)";
Expand Down Expand Up @@ -1736,53 +1750,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);
}
}
Loading