Performance: Account class didn't implement equals() and hashCode() => wrong results in getAccounts().join().contains()

[gterminator]

Hi, the class Account didnt implement the equals() and hashCode() methods. Therefore the Set cannot determine that an object is in or not.

Problem boolean isAccountInCache = app.getAccounts().join().contains(myAccount); This is not working because of the missing methods. As a Workaround, we have to iterate trough every element in this cache. Which make this library very inefficient and latency expensive.

Problematic Workaround boolean isAccountInCache = app.getAccounts().join().stream().anymatch( acc -> acc.username().equals(myusername)); This is working well for ~100 accounts in the cache, but in larger applications with more than 10-100k users in cache this will be a problem. Imagine we iterate for every request 10k times through this accounts set.

Maybe I find time to open a PR and/or provide a PoC of this problem.

Best regards

Max

[gterminator]

Here the PoC

public class Main
{

    public static void main(String[] args) throws InterruptedException
    {
        Set<Account> accountsBuggy = getExampleCacheOfAccounts(1000);
        Set<AccountFixed> accountsFixed = getExampleCacheOfAccountsFixed( 1000);

        /**
         * JDK normally hashcodes the object reference. In a distributed environment we didn't have the
         * object reference on another server...
         * Creating here a new Object with the same name but another reference.
         */
        Account account = Account.builder()
                                 .name("Max")
                                 .build();

        System.out.println("--Running bug Testcase--");
        boolean isAccountInSet = accountsBuggy.contains( account );
        System.out.println("Is account in Set= " + isAccountInSet + " expected=true");

        System.out.println("--Running fixed Testcase--");
        AccountFixed accountFixed = AccountFixed.builder()
                                 .name("Max")
                                 .build();

        boolean isAccountFixedInSet = accountsFixed.contains( accountFixed );
        System.out.println("Is accountFixed in Set= " + isAccountFixedInSet + " expected=true");

    }

    public static Set<Account> getExampleCacheOfAccounts(int amount)
    {
        Random random = new Random();
        HashSet<Account> accountHashSet = new HashSet<>();

        // example account for fetching
        Account account = Account.builder()
                                 .name("Max")
                                 .build();
        accountHashSet.add(account);

        for (int i = 0; i < amount; i++)
        {
            Account dummyAccount = Account.builder()
                                          .name("dummy" + random.nextInt())
                                          .build();
            accountHashSet.add(dummyAccount);
        }
        return accountHashSet;
    }

    public static Set<AccountFixed> getExampleCacheOfAccountsFixed(int amount)
    {
        Random random = new Random();
        HashSet<AccountFixed> accountHashSet = new HashSet<>();

        // example account for fetching
        AccountFixed account = AccountFixed.builder()
                                 .name("Max")
                                 .build();
        accountHashSet.add(account);

        for (int i = 0; i < amount; i++)
        {
            AccountFixed dummyAccount = AccountFixed.builder()
                                          .name("dummy" + random.nextInt())
                                          .build();
            accountHashSet.add(dummyAccount);
        }
        return accountHashSet;
    }

}

@Getter
@Setter
@Builder
@AllArgsConstructor
public class Account
{

    private String name;
}

@Getter
@Setter
@Builder
@EqualsAndHashCode
@AllArgsConstructor
public class AccountFixed
{

    private String name;
}

Output

--Running bug Testcase--
Is account in Set= false expected=true
--Running fixed Testcase--
Is accountFixed in Set= true expected=true

[bgavrilMS]

Hi @gterminator - your token cache should never so large.

In public client applications (e.g. desktop apps), a user can't have more than a handful of accounts. The most we ever saw is 30.

In confidential client applications - in web apps, it's an antipattern to have a singleton confidentialclientapp (CCA) with all the tokens of all the users. Instead, organize your code so that each user session creates it's own CCA object / serialize the cache as part of the session. Web APIs are a bit more complex, but the concept of 1 CCA per session still holds.

[gterminator]

Hi @bgavrilMS, ok. Thank you. There was a misunderstanding. That I understand this correct: a) The MSAL Token Cache should be per UserSession. b) The MSAL Token Cache interface is not application-wide, it should be user-session wide. c) In a distributed environment this user-session-cache should be fetched from the distributed cache and set on request.

Maybe I overseen this in the documentation (and the servlet example) but the interface leads to this misunderstanding very easy.

Anyway, I think the PR is still valid but not that big problem I was thinking.

Thanks for the clearification @bgavrilMS!

Max

[bgavrilMS]

Yes, the simplest is to create a new CCA object on each request and to assign it the token cache from the session level.

[Avery-Dunn]

Thanks for the fix in https://github.com/AzureAD/microsoft-authentication-library-for-java/pull/681, which is now in the latest 1.13.10 release.