Skip to content

Commit eac0d8d

Browse files
Avery-DunnAvery-Dunn
authored
Merge pull request #894 from AzureAD/avdunn/cache-npe-fix
Fix potential null pointer exception during cache lookup
2 parents bbdc93d + e355883 commit eac0d8d

4 files changed

Lines changed: 114 additions & 7 deletions

File tree

‎msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenCache.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -477,7 +477,8 @@ private Optional<AccessTokenCacheEntity> getAccessTokenCacheEntity(
477477

478478
return accessTokens.values().stream().filter(
479479
accessToken ->
480-
accessToken.homeAccountId.equals(account.homeAccountId()) &&
480+
accessToken.homeAccountId != null &&
481+
accessToken.homeAccountId.equals(account.homeAccountId()) &&
481482
environmentAliases.contains(accessToken.environment) &&
482483
accessToken.realm.equals(authority.tenant()) &&
483484
accessToken.clientId.equals(clientId) &&
@@ -552,6 +553,7 @@ private Optional<RefreshTokenCacheEntity> getRefreshTokenCacheEntity(
552553

553554
return refreshTokens.values().stream().filter(
554555
refreshToken ->
556+
refreshToken.homeAccountId != null &&
555557
refreshToken.homeAccountId.equals(account.homeAccountId()) &&
556558
environmentAliases.contains(refreshToken.environment) &&
557559
refreshToken.clientId.equals(clientId)

‎msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenRequestExecutor.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import java.io.IOException;
1818
import java.net.MalformedURLException;
19+
import java.nio.charset.StandardCharsets;
1920
import java.util.*;
2021

2122
@Getter(AccessLevel.PACKAGE)
@@ -118,9 +119,15 @@ private AuthenticationResult createAuthenticationResultFromOauthHttpResponse(
118119
}
119120

120121
AccountCacheEntity accountCacheEntity = null;
122+
if (!StringHelper.isNullOrBlank(tokens.getIDTokenString())) {
123+
String idTokenJson;
124+
try {
125+
idTokenJson = new String(Base64.getDecoder().decode(tokens.getIDTokenString().split("\\.")[1]), StandardCharsets.UTF_8);
126+
} catch (ArrayIndexOutOfBoundsException e) {
127+
throw new MsalServiceException("Error parsing ID token, missing payload section. Ensure that the ID token is following the JWT format.",
128+
AuthenticationErrorCode.INVALID_JWT);
129+
}
121130

122-
if (tokens.getIDToken() != null) {
123-
String idTokenJson = tokens.getIDToken().getParsedParts()[1].decodeToString();
124131
IdToken idToken = JsonHelper.convertJsonToObject(idTokenJson, IdToken.class);
125132

126133
AuthorityType type = msalRequest.application().authenticationAuthority.authorityType;

‎msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/CacheTests.java‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
package com.microsoft.aad.msal4j;
5+
6+
import org.junit.jupiter.api.Test;
7+
8+
import java.util.Collections;
9+
import java.util.HashMap;
10+
11+
import static org.junit.jupiter.api.Assertions.assertEquals;
12+
import static org.junit.jupiter.api.Assertions.assertNull;
13+
import static org.mockito.ArgumentMatchers.any;
14+
import static org.mockito.Mockito.mock;
15+
import static org.mockito.Mockito.times;
16+
import static org.mockito.Mockito.verify;
17+
import static org.mockito.Mockito.when;
18+
19+
class CacheTests {
20+
21+
@Test
22+
void cacheLookup_MixAccountBasedAndAssertionBasedSilentFlows() throws Exception {
23+
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
24+
25+
ConfidentialClientApplication cca =
26+
ConfidentialClientApplication.builder("clientId", ClientCredentialFactory.createFromSecret("password"))
27+
.authority("https://login.microsoftonline.com/tenant/")
28+
.instanceDiscovery(false)
29+
.validateAuthority(false)
30+
.httpClient(httpClientMock)
31+
.build();
32+
33+
HashMap<String, String> responseParameters = new HashMap<>();
34+
//Acquire a token with no ID token/account associated with it
35+
responseParameters.put("access_token", "accessTokenNoAccount");
36+
37+
ClientCredentialParameters clientCredentialParameters = ClientCredentialParameters.builder(Collections.singleton("someScopes")).build();
38+
when(httpClientMock.send(any(HttpRequest.class))).thenReturn(TestHelper.expectedResponse(200, TestHelper.getSuccessfulTokenResponse(responseParameters)));
39+
IAuthenticationResult resultNoAccount = cca.acquireToken(clientCredentialParameters).get();
40+
41+
//Ensure there is one token in the cache, and the result had no account
42+
assertEquals(1, cca.tokenCache.accessTokens.size());
43+
assertNull(resultNoAccount.account());
44+
verify(httpClientMock, times(1)).send(any());
45+
46+
//Acquire a second token, this time with an ID token/account
47+
responseParameters.put("access_token", "accessTokenWithAccount");
48+
responseParameters.put("id_token", TestHelper.createIdToken(new HashMap<>()));
49+
50+
when(httpClientMock.send(any(HttpRequest.class))).thenReturn(TestHelper.expectedResponse(200, TestHelper.getSuccessfulTokenResponse(responseParameters)));
51+
OnBehalfOfParameters onBehalfOfParametersarameters = OnBehalfOfParameters.builder(Collections.singleton("someOtherScopes"), new UserAssertion(TestHelper.signedAssertion)).build();
52+
IAuthenticationResult resultWithAccount = cca.acquireToken(onBehalfOfParametersarameters).get();
53+
54+
//Ensure there are now two tokens in the cache, and the result has an account
55+
assertEquals(2, cca.tokenCache.accessTokens.size());
56+
assertNull(resultNoAccount.account());
57+
verify(httpClientMock, times(2)).send(any());
58+
59+
//Make two silent calls, one with the account and one without
60+
SilentParameters silentParametersNoAccount = SilentParameters.builder(Collections.singleton("someScopes")).build();
61+
SilentParameters silentParametersWithAccount = SilentParameters.builder(Collections.singleton("someOtherScopes"), resultWithAccount.account()).build();
62+
63+
resultNoAccount = cca.acquireTokenSilently(silentParametersNoAccount).get();
64+
resultWithAccount = cca.acquireTokenSilently(silentParametersWithAccount).get();
65+
66+
//Ensure the correct access tokens were returned from each silent call
67+
assertEquals("accessTokenNoAccount", resultNoAccount.accessToken());
68+
assertEquals("accessTokenWithAccount", resultWithAccount.accessToken());
69+
}
70+
}

‎msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/TestHelper.java‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,13 @@
77
import com.nimbusds.jose.crypto.RSASSASigner;
88
import com.nimbusds.jose.jwk.RSAKey;
99
import com.nimbusds.jose.jwk.gen.RSAKeyGenerator;
10-
1110
import java.io.File;
1211
import java.io.FileWriter;
1312
import java.io.IOException;
1413
import java.net.URISyntaxException;
1514
import java.nio.file.Files;
1615
import java.nio.file.Paths;
16+
import java.util.Base64;
1717
import java.util.Collections;
1818
import java.util.HashMap;
1919
import java.util.List;
@@ -29,6 +29,17 @@ class TestHelper {
2929
"\"expires_on\": %d ,\"expires_in\": %d," +
3030
"\"token_type\":\"Bearer\"}";
3131

32+
static final String idTokenFormat = "{\"aud\": \"%s\"," +
33+
"\"iss\": \"%s\"," +
34+
"\"iat\": 1455833828," + "\"nbf\": 1455833828," + "\"exp\": 1455837728," +
35+
"\"ipaddr\": \"131.107.159.117\"," +
36+
"\"name\": \"%s\"," +
37+
"\"oid\": \"%s\"," +
38+
"\"preferred_username\": \"%s\"," +
39+
"\"sub\": \"%s\"," +
40+
"\"tid\": \"%s\"," +
41+
"\"ver\": \"2.0\"}";
42+
3243
static String readResource(Class<?> classInstance, String resource) {
3344
try {
3445
return new String(Files.readAllBytes(Paths.get(classInstance.getResource(resource).toURI())));
@@ -76,10 +87,10 @@ static String getSuccessfulTokenResponse(HashMap<String, String> responseValues)
7687

7788
return String.format(successfulResponseFormat,
7889
responseValues.getOrDefault("access_token", "access_token"),
79-
responseValues.getOrDefault("id_token", "id_token"),
90+
responseValues.getOrDefault("id_token", ""),
8091
responseValues.getOrDefault("refresh_token", "refresh_token"),
8192
responseValues.getOrDefault("client_id", "client_id"),
82-
responseValues.getOrDefault("client_info", "client_info"),
93+
responseValues.getOrDefault("client_info", "eyJ1aWQiOiI1OTdmODZjZC0xM2YzLTQ0YzAtYmVjZS1hMWU3N2JhNDMyMjgiLCJ1dGlkIjoiZjY0NWFkOTItZTM4ZC00ZDFhLWI1MTAtZDFiMDlhNzRhOGNhIn0"),
8394
expiresOn,
8495
expiresIn
8596
);
@@ -97,4 +108,21 @@ static HttpResponse expectedResponse(int statusCode, String response) {
97108

98109
return httpResponse;
99110
}
100-
}
111+
112+
//Maps various values to the idTokenFormat string
113+
static String createIdToken(HashMap<String, String> idTokenValues) {
114+
String tokenValues = String.format(idTokenFormat,
115+
idTokenValues.getOrDefault("aud", "e854a4a7-6c34-449c-b237-fc7a28093d84"),
116+
idTokenValues.getOrDefault("iss", "https://login.microsoftonline.com/6c3d51dd-f0e5-4959-b4ea-a80c4e36fe5e/v2.0/"),
117+
idTokenValues.getOrDefault("name", "name"),
118+
idTokenValues.getOrDefault("oid", "oid"),
119+
idTokenValues.getOrDefault("preferred_username", "preferred_username"),
120+
idTokenValues.getOrDefault("sub", "K4_SGGxKqW1SxUAmhg6C1F6VPiFzcx-Qd80ehIEdFus"),
121+
idTokenValues.getOrDefault("client_info", "eyJ1aWQiOiI1OTdmODZjZC0xM2YzLTQ0YzAtYmVjZS1hMWU3N2JhNDMyMjgiLCJ1dGlkIjoiZjY0NWFkOTItZTM4ZC00ZDFhLWI1MTAtZDFiMDlhNzRhOGNhIn0")
122+
);
123+
124+
String encodedTokenValues = Base64.getUrlEncoder().encodeToString(tokenValues.getBytes());
125+
126+
return String.format("someheader.%s.somesignature", encodedTokenValues);
127+
}
128+
}

0 commit comments

Comments
 (0)