From 450eb70edaaba76b6a0cb1902f6a2a14ff0295d6 Mon Sep 17 00:00:00 2001 From: avdunn Date: Tue, 9 Jun 2026 12:38:21 -0700 Subject: [PATCH 1/3] Fix JSON serialization and minor validation issues. --- .../aad/msal4j/DeviceCodeFlowParameters.java | 2 +- .../microsoft/aad/msal4j/ErrorResponse.java | 2 - .../aad/msal4j/OidcDiscoveryResponse.java | 1 + .../aad/msal4j/RefreshTokenParameters.java | 2 +- .../microsoft/aad/msal4j/RequestedClaim.java | 2 +- .../aad/msal4j/ParameterBuilderTest.java | 34 ++++------- .../aad/msal4j/ResponseParsingTest.java | 61 +++++++++++++------ 7 files changed, 57 insertions(+), 47 deletions(-) diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DeviceCodeFlowParameters.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DeviceCodeFlowParameters.java index bfd3ab54..cd3feaa9 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DeviceCodeFlowParameters.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DeviceCodeFlowParameters.java @@ -115,7 +115,7 @@ public DeviceCodeFlowParametersBuilder scopes(Set scopes) { * Cannot be null. */ public DeviceCodeFlowParametersBuilder deviceCodeConsumer(Consumer deviceCodeConsumer) { - validateNotNull("deviceCodeConsumer", scopes); + validateNotNull("deviceCodeConsumer", deviceCodeConsumer); this.deviceCodeConsumer = deviceCodeConsumer; return this; diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ErrorResponse.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ErrorResponse.java index bc55dcba..69e2a300 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ErrorResponse.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ErrorResponse.java @@ -67,8 +67,6 @@ static ErrorResponse fromJson(JsonReader jsonReader) throws IOException { public JsonWriter toJson(JsonWriter jsonWriter) throws IOException { jsonWriter.writeStartObject(); - jsonWriter.writeStartObject(); - jsonWriter.writeNumberField("statusCode", statusCode); jsonWriter.writeStringField("statusMessage", statusMessage); jsonWriter.writeStringField("error", error); diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/OidcDiscoveryResponse.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/OidcDiscoveryResponse.java index d0e961b4..eeedd715 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/OidcDiscoveryResponse.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/OidcDiscoveryResponse.java @@ -47,6 +47,7 @@ public static OidcDiscoveryResponse fromJson(JsonReader jsonReader) throws IOExc public JsonWriter toJson(JsonWriter jsonWriter) throws IOException { jsonWriter.writeStartObject(); + jsonWriter.writeStringField("issuer", issuer); jsonWriter.writeStringField("authorization_endpoint", authorizationEndpoint); jsonWriter.writeStringField("token_endpoint", tokenEndpoint); jsonWriter.writeStringField("device_authorization_endpoint", deviceCodeEndpoint); diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/RefreshTokenParameters.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/RefreshTokenParameters.java index 35a3e661..cee46ffb 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/RefreshTokenParameters.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/RefreshTokenParameters.java @@ -113,7 +113,7 @@ public RefreshTokenParametersBuilder scopes(Set scopes) { * Cannot be null. */ public RefreshTokenParametersBuilder refreshToken(String refreshToken) { - validateNotNull("refreshToken", scopes); + validateNotNull("refreshToken", refreshToken); this.refreshToken = refreshToken; return this; diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/RequestedClaim.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/RequestedClaim.java index e9e779da..574822cc 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/RequestedClaim.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/RequestedClaim.java @@ -50,7 +50,7 @@ public JsonWriter toJson(JsonWriter jsonWriter) throws IOException { jsonWriter.writeStartObject(); if (name != null && requestedClaimAdditionalInfo != null) { - jsonWriter.writeString(name); + jsonWriter.writeFieldName(name); requestedClaimAdditionalInfo.toJson(jsonWriter); } diff --git a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ParameterBuilderTest.java b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ParameterBuilderTest.java index b5ac5533..f1ad04d6 100644 --- a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ParameterBuilderTest.java +++ b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ParameterBuilderTest.java @@ -72,17 +72,12 @@ void deviceCodeFlow_NullScopes_Throws() { } @Test - void deviceCodeFlow_DeviceCodeConsumerValidation_ValidatesWrongField() { - // BUG: DeviceCodeFlowParameters.Builder.deviceCodeConsumer() validates scopes - // instead of the deviceCodeConsumer parameter (copy-paste bug on line 118). - // Since scopes was already set by builder(scopes, consumer), passing null consumer - // does NOT throw — it silently accepts null. - DeviceCodeFlowParameters params = DeviceCodeFlowParameters - .builder(SCOPES, dc -> {}) - .deviceCodeConsumer(null) - .build(); - - assertNull(params.deviceCodeConsumer()); + void deviceCodeFlow_DeviceCodeConsumerValidation_RejectsNull() { + assertThrows(IllegalArgumentException.class, () -> + DeviceCodeFlowParameters + .builder(SCOPES, dc -> {}) + .deviceCodeConsumer(null) + .build()); } // ========== IntegratedWindowsAuthenticationParameters ========== @@ -505,17 +500,12 @@ void refreshToken_BlankToken_Throws() { } @Test - void refreshToken_BuilderRefreshTokenValidation_ValidatesWrongField() { - // BUG: RefreshTokenParameters.Builder.refreshToken() validates scopes - // instead of the refreshToken parameter (copy-paste bug on line 116). - // Since scopes was already set by builder(scopes, token), passing null - // to the builder setter does NOT throw. - RefreshTokenParameters params = RefreshTokenParameters - .builder(SCOPES, "initial-token") - .refreshToken(null) - .build(); - - assertNull(params.refreshToken()); + void refreshToken_BuilderRefreshTokenValidation_RejectsNull() { + assertThrows(IllegalArgumentException.class, () -> + RefreshTokenParameters + .builder(SCOPES, "initial-token") + .refreshToken(null) + .build()); } // ========== AuthorizationCodeParameters ========== diff --git a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ResponseParsingTest.java b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ResponseParsingTest.java index 52ce2730..d7943fcd 100644 --- a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ResponseParsingTest.java +++ b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ResponseParsingTest.java @@ -108,25 +108,47 @@ void errorResponse_settersAndGetters_fullCoverage() { } @Test - void errorResponse_toJson_throwsDueToDoubleStartObject() { - // Bug: ErrorResponse.toJson() calls writeStartObject() twice (lines 68, 70) - // which causes an IllegalStateException from the JSON writer + void errorResponse_toJson_allFields() throws IOException { ErrorResponse response = new ErrorResponse(); response.statusCode(400); response.error("invalid_grant"); + response.errorDescription("Token expired"); + response.subError("token_expired"); + response.correlation_id("corr-123"); + response.claims("{\"access_token\":{}}"); + + String output = TestHelper.writeToJson(response); - assertThrows(IllegalStateException.class, () -> TestHelper.writeToJson(response), - "toJson has a double writeStartObject bug that causes IllegalStateException"); + assertTrue(output.contains("\"statusCode\":400")); + assertTrue(output.contains("\"error\":\"invalid_grant\"")); + assertTrue(output.contains("\"error_description\":\"Token expired\"")); + assertTrue(output.contains("\"suberror\":\"token_expired\"")); + assertTrue(output.contains("\"correlation_id\":\"corr-123\"")); + assertTrue(output.contains("\"claims\":\"{\\\"access_token\\\":{}}\"")); } @Test - void errorResponse_toJson_nullErrorCodes_throwsDueToDoubleStartObject() { - // Same double writeStartObject bug affects all toJson calls + void errorResponse_toJson_nullErrorCodes_writesNull() throws IOException { ErrorResponse response = new ErrorResponse(); response.statusCode(500); response.error("server_error"); - assertThrows(IllegalStateException.class, () -> TestHelper.writeToJson(response)); + String output = TestHelper.writeToJson(response); + + assertTrue(output.contains("\"error\":\"server_error\"")); + assertTrue(output.contains("\"error_codes\":null")); + } + + @Test + void errorResponse_toJson_withErrorCodes() throws IOException { + ErrorResponse response = new ErrorResponse(); + response.statusCode(400); + response.error("invalid_request"); + response.errorCodes(new long[]{50076, 700082}); + + String output = TestHelper.writeToJson(response); + + assertTrue(output.contains("\"error_codes\":[50076,700082]")); } // ========== UserDiscoveryResponse ========== @@ -272,8 +294,7 @@ void oidcDiscoveryResponse_fromJson_unknownFieldsSkipped() throws IOException { } @Test - void oidcDiscoveryResponse_toJson_doesNotIncludeIssuer() throws IOException { - // Note: toJson() intentionally omits the issuer field — this test documents that behavior + void oidcDiscoveryResponse_toJson_includesIssuer() throws IOException { String json = "{\"authorization_endpoint\":\"https://example.com/auth\"," + "\"token_endpoint\":\"https://example.com/token\"," + "\"device_authorization_endpoint\":\"https://example.com/device\"," @@ -283,11 +304,10 @@ void oidcDiscoveryResponse_toJson_doesNotIncludeIssuer() throws IOException { String output = TestHelper.writeToJson(response); - assertTrue(output.contains("authorization_endpoint")); - assertTrue(output.contains("token_endpoint")); - assertTrue(output.contains("device_authorization_endpoint")); - // toJson does not write the issuer field - assertFalse(output.contains("\"issuer\"")); + assertTrue(output.contains("\"issuer\":\"https://example.com/issuer\"")); + assertTrue(output.contains("\"authorization_endpoint\":\"https://example.com/auth\"")); + assertTrue(output.contains("\"token_endpoint\":\"https://example.com/token\"")); + assertTrue(output.contains("\"device_authorization_endpoint\":\"https://example.com/device\"")); } // ========== RequestedClaimAdditionalInfo ========== @@ -437,14 +457,15 @@ void requestedClaim_any_nullName() { } @Test - void requestedClaim_toJson_withNameAndInfo_throwsDueToBadStringWrite() { - // Bug: RequestedClaim.toJson() calls writeString(name) inside an object context, - // which is invalid JSON (a raw string value where a field name is expected) + void requestedClaim_toJson_withNameAndInfo_writesFieldWithObject() throws IOException { RequestedClaimAdditionalInfo info = new RequestedClaimAdditionalInfo(true, null, null); RequestedClaim claim = new RequestedClaim("sub", info); - assertThrows(IllegalStateException.class, () -> TestHelper.writeToJson(claim), - "toJson writes a raw string in object context, causing IllegalStateException"); + String output = TestHelper.writeToJson(claim); + + // Should produce: {"sub":{"essential":true}} + assertTrue(output.contains("\"sub\"")); + assertTrue(output.contains("\"essential\":true")); } @Test From 69313eff1f9e8411b5c88e82395dc3ad0c5a4dfb Mon Sep 17 00:00:00 2001 From: avdunn Date: Tue, 9 Jun 2026 13:43:56 -0700 Subject: [PATCH 2/3] Fix AuthenticationResult equals issues and NPE --- .../aad/msal4j/AuthenticationResult.java | 28 +++- .../msal4j/AuthenticationResultMetadata.java | 21 +++ .../com/microsoft/aad/msal4j/IdToken.java | 29 ++++ .../microsoft/aad/msal4j/TenantProfile.java | 17 ++ .../aad/msal4j/AuthenticationResultTest.java | 153 +++++++++++------- 5 files changed, 188 insertions(+), 60 deletions(-) diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResult.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResult.java index d87dfc4b..294f909d 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResult.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResult.java @@ -16,10 +16,10 @@ final class AuthenticationResult implements IAuthenticationResult { private final Long refreshOn; private final String familyId; private final String idToken; - private final IdToken idTokenObject = getIdTokenObj(); + private final IdToken idTokenObject; private final AccountCacheEntity accountCacheEntity; - private final IAccount account = getAccount(); - private final ITenantProfile tenantProfile = getTenantProfile(); + private final IAccount account; + private final ITenantProfile tenantProfile; private String environment; private final Date expiresOnDate; private final String scopes; @@ -40,6 +40,9 @@ final class AuthenticationResult implements IAuthenticationResult { this.metadata = metadata == null ? AuthenticationResultMetadata.builder().build() : metadata; this.isPopAuthorization = isPopAuthorization; this.expiresOnDate = new Date(expiresOn * 1000); + this.idTokenObject = getIdTokenObj(); + this.account = getAccount(); + this.tenantProfile = getTenantProfile(); } private IdToken getIdTokenObj() { @@ -47,7 +50,11 @@ private IdToken getIdTokenObj() { return null; } - return JsonHelper.createIdTokenFromEncodedTokenString(idToken); + try { + return JsonHelper.createIdTokenFromEncodedTokenString(idToken); + } catch (Exception e) { + return null; + } } private IAccount getAccount() { @@ -62,7 +69,16 @@ private ITenantProfile getTenantProfile() { return null; } - return new TenantProfile(JsonHelper.parseJsonToMap(JsonHelper.getTokenPayloadClaims(idToken)), getAccount().environment()); + IAccount acct = getAccount(); + if (acct == null) { + return null; + } + + try { + return new TenantProfile(JsonHelper.parseJsonToMap(JsonHelper.getTokenPayloadClaims(idToken)), acct.environment()); + } catch (Exception e) { + return null; + } } public String accessToken() { @@ -114,7 +130,7 @@ AccountCacheEntity accountCacheEntity() { } public IAccount account() { - return getAccount(); + return this.account; } public ITenantProfile tenantProfile() { diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResultMetadata.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResultMetadata.java index 35cded71..bf18071e 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResultMetadata.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResultMetadata.java @@ -4,6 +4,7 @@ package com.microsoft.aad.msal4j; import java.io.Serializable; +import java.util.Objects; /** * Contains metadata and additional context for the contents of an AuthenticationResult @@ -59,6 +60,26 @@ void cacheRefreshReason(CacheRefreshReason cacheRefreshReason) { this.cacheRefreshReason = cacheRefreshReason; } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof AuthenticationResultMetadata)) return false; + + AuthenticationResultMetadata other = (AuthenticationResultMetadata) o; + + return Objects.equals(tokenSource, other.tokenSource) + && Objects.equals(refreshOn, other.refreshOn) + && Objects.equals(cacheRefreshReason, other.cacheRefreshReason); + } + + @Override + public int hashCode() { + int result = tokenSource == null ? 0 : tokenSource.hashCode(); + result = 31 * result + (refreshOn == null ? 0 : refreshOn.hashCode()); + result = 31 * result + (cacheRefreshReason == null ? 0 : cacheRefreshReason.hashCode()); + return result; + } + public static class AuthenticationResultMetadataBuilder { private TokenSource tokenSource; private Long refreshOn; diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IdToken.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IdToken.java index f9992a71..c1166883 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IdToken.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IdToken.java @@ -10,6 +10,7 @@ import java.io.IOException; import java.io.Serializable; +import java.util.Objects; class IdToken implements Serializable, JsonSerializable { @@ -101,4 +102,32 @@ public JsonWriter toJson(JsonWriter jsonWriter) throws IOException { return jsonWriter; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof IdToken)) return false; + + IdToken other = (IdToken) o; + + return Objects.equals(issuer, other.issuer) + && Objects.equals(subject, other.subject) + && Objects.equals(audience, other.audience) + && Objects.equals(expirationTime, other.expirationTime) + && Objects.equals(issuedAt, other.issuedAt) + && Objects.equals(notBefore, other.notBefore) + && Objects.equals(name, other.name) + && Objects.equals(preferredUsername, other.preferredUsername) + && Objects.equals(objectIdentifier, other.objectIdentifier) + && Objects.equals(tenantIdentifier, other.tenantIdentifier) + && Objects.equals(upn, other.upn) + && Objects.equals(uniqueName, other.uniqueName); + } + + @Override + public int hashCode() { + return Objects.hash(issuer, subject, audience, expirationTime, issuedAt, + notBefore, name, preferredUsername, objectIdentifier, + tenantIdentifier, upn, uniqueName); + } } \ No newline at end of file diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TenantProfile.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TenantProfile.java index ad14707a..e64a2a1c 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TenantProfile.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TenantProfile.java @@ -4,6 +4,7 @@ package com.microsoft.aad.msal4j; import java.util.Map; +import java.util.Objects; /** * Representation of a single tenant profile @@ -40,4 +41,20 @@ public TenantProfile environment(String environment) { this.environment = environment; return this; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof TenantProfile)) return false; + + TenantProfile other = (TenantProfile) o; + + return Objects.equals(idTokenClaims, other.idTokenClaims) + && Objects.equals(environment, other.environment); + } + + @Override + public int hashCode() { + return Objects.hash(idTokenClaims, environment); + } } diff --git a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthenticationResultTest.java b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthenticationResultTest.java index 22ab1680..1882fe6d 100644 --- a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthenticationResultTest.java +++ b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthenticationResultTest.java @@ -25,16 +25,6 @@ class AuthenticationResultTest { private static final String ENVIRONMENT = "login.microsoftonline.com"; private static final String SCOPES = "user.read openid"; - // Metadata is shared across equals/hashCode tests because AuthenticationResultMetadata - // does not implement equals(), so two separately-constructed instances will fail - // reference equality even if all fields match. - private static final AuthenticationResultMetadata SHARED_METADATA = - AuthenticationResultMetadata.builder() - .tokenSource(TokenSource.IDENTITY_PROVIDER) - .refreshOn(REFRESH_ON) - .cacheRefreshReason(CacheRefreshReason.NOT_APPLICABLE) - .build(); - private static AccountCacheEntity buildAccountCacheEntity() { AccountCacheEntity entity = new AccountCacheEntity(); entity.homeAccountId = "uid.utid"; @@ -46,11 +36,14 @@ private static AccountCacheEntity buildAccountCacheEntity() { return entity; } - /** - * Builds a result with no idToken (and thus no idTokenObject or tenantProfile), which - * is needed for equals/hashCode tests because IdToken and TenantProfile do not implement - * equals(). This ensures equals() comparisons are based on value equality of all fields. - */ + private static AuthenticationResultMetadata buildMetadata() { + return AuthenticationResultMetadata.builder() + .tokenSource(TokenSource.IDENTITY_PROVIDER) + .refreshOn(REFRESH_ON) + .cacheRefreshReason(CacheRefreshReason.NOT_APPLICABLE) + .build(); + } + private static AuthenticationResult buildBaselineResult() { return AuthenticationResult.builder() .accessToken(ACCESS_TOKEN) @@ -62,7 +55,7 @@ private static AuthenticationResult buildBaselineResult() { .accountCacheEntity(buildAccountCacheEntity()) .environment(ENVIRONMENT) .scopes(SCOPES) - .metadata(SHARED_METADATA) + .metadata(buildMetadata()) .isPopAuthorization(false) .build(); } @@ -141,17 +134,13 @@ void build_NullMetadata_DefaultsToEmptyMetadata() { // ========== Derived Field Tests ========== @Test - void build_WithIdToken_IdTokenObjectIsNull_DueToInitOrder() { - // Documents a known issue: field initializers run before the constructor body, - // so idTokenObject = getIdTokenObj() executes when this.idToken is still null. - // This means idTokenObject() always returns null regardless of the idToken value. + void build_WithIdToken_IdTokenObjectIsParsed() { AuthenticationResult result = AuthenticationResult.builder() .idToken(TestHelper.ENCODED_JWT) .build(); - assertNull(result.idTokenObject(), - "idTokenObject is always null due to field initialization order"); - // The idToken string itself is stored correctly + assertNotNull(result.idTokenObject(), + "idTokenObject should be parsed from the idToken string"); assertEquals(TestHelper.ENCODED_JWT, result.idToken()); } @@ -165,9 +154,7 @@ void build_WithBlankIdToken_IdTokenObjectIsNull() { } @Test - void build_WithAccountCacheEntity_PublicGetterRecomputesAccount() { - // The public account() getter calls getAccount() each time, so it works correctly - // despite the field initialization order bug (the private 'account' field is always null). + void build_WithAccountCacheEntity_AccountIsSet() { AccountCacheEntity entity = buildAccountCacheEntity(); AuthenticationResult result = AuthenticationResult.builder() @@ -191,10 +178,7 @@ void build_WithNullAccountCacheEntity_AccountIsNull() { } @Test - void build_WithIdTokenAndAccount_TenantProfileIsNull_DueToInitOrder() { - // Documents same initialization order issue: tenantProfile = getTenantProfile() - // runs before this.idToken is set, so StringHelper.isBlank(idToken) returns true - // and tenantProfile is always null. + void build_WithIdTokenAndAccount_TenantProfileIsSet() { AccountCacheEntity entity = buildAccountCacheEntity(); AuthenticationResult result = AuthenticationResult.builder() @@ -202,8 +186,24 @@ void build_WithIdTokenAndAccount_TenantProfileIsNull_DueToInitOrder() { .accountCacheEntity(entity) .build(); + assertNotNull(result.tenantProfile(), + "tenantProfile should be computed from idToken and account"); + assertEquals(ENVIRONMENT, result.tenantProfile().environment()); + assertNotNull(result.tenantProfile().getClaims()); + } + + @Test + void build_WithIdTokenButNullAccount_TenantProfileIsNull() { + // When idToken is present but accountCacheEntity is null, getTenantProfile() + // gracefully returns null instead of throwing NPE. + AuthenticationResult result = AuthenticationResult.builder() + .idToken(TestHelper.ENCODED_JWT) + .accountCacheEntity(null) + .build(); + assertNull(result.tenantProfile(), - "tenantProfile is always null due to field initialization order"); + "tenantProfile should be null when no account is available"); + assertEquals(TestHelper.ENCODED_JWT, result.idToken()); } @Test @@ -253,10 +253,6 @@ void equals_Null_ReturnsFalse() { @Test void equals_IdenticalFields_NoIdToken_ReturnsTrue() { - // Two independently-constructed results with identical fields and shared metadata - // should be equal when idToken is blank (avoiding IdToken/TenantProfile reference - // equality issues). This is the only scenario where equals() works for separately - // constructed instances, because AuthenticationResultMetadata also lacks equals(). AuthenticationResult result1 = buildBaselineResult(); AuthenticationResult result2 = buildBaselineResult(); assertEquals(result1, result2); @@ -264,10 +260,7 @@ void equals_IdenticalFields_NoIdToken_ReturnsTrue() { /** * Parameterized test that verifies each field in equals() by varying one field at a time - * from the baseline. Uses blank idToken and shared metadata to ensure reference equality - * issues in IdToken/TenantProfile/Metadata don't interfere. - * - * Each argument set contains: field name (for display), and a result with that field changed. + * from the baseline. */ @ParameterizedTest(name = "equals returns false when {0} differs") @MethodSource("differentFieldProvider") @@ -290,7 +283,9 @@ private static Stream differentFieldProvider() { Arguments.of("environment", buildWithOverride().environment("different.env").build()), Arguments.of("scopes", buildWithOverride().scopes("different.scope").build()), Arguments.of("isPopAuthorization", buildWithOverride().isPopAuthorization(true).build()), - Arguments.of("accountCacheEntity", buildWithOverride().accountCacheEntity(differentAccount).build()) + Arguments.of("accountCacheEntity", buildWithOverride().accountCacheEntity(differentAccount).build()), + Arguments.of("metadata", buildWithOverride().metadata( + AuthenticationResultMetadata.builder().tokenSource(TokenSource.CACHE).build()).build()) ); } @@ -308,36 +303,30 @@ private static AuthenticationResult.AuthenticationResultBuilder buildWithOverrid .accountCacheEntity(buildAccountCacheEntity()) .environment(ENVIRONMENT) .scopes(SCOPES) - .metadata(SHARED_METADATA) + .metadata(buildMetadata()) .isPopAuthorization(false); } @Test - void equals_IdTokenSetOnBoth_ReturnsTrue_BecauseIdTokenObjectAlwaysNull() { - // Despite IdToken lacking equals(), this comparison passes because the field - // initialization order bug means idTokenObject is always null for both results. - // The idToken string comparison (line 238) passes, and the idTokenObject comparison - // (line 239) passes because both are null. + void equals_IdTokenSetOnBoth_ReturnsTrue() { AccountCacheEntity entity = buildAccountCacheEntity(); AuthenticationResult result1 = AuthenticationResult.builder() .idToken(TestHelper.ENCODED_JWT) .accountCacheEntity(entity) - .metadata(SHARED_METADATA) + .metadata(buildMetadata()) .build(); AuthenticationResult result2 = AuthenticationResult.builder() .idToken(TestHelper.ENCODED_JWT) .accountCacheEntity(entity) - .metadata(SHARED_METADATA) + .metadata(buildMetadata()) .build(); assertEquals(result1, result2, - "Results with same idToken are equal because idTokenObject is always null"); + "Results with same idToken should be equal"); } @Test - void equals_DifferentMetadataInstances_ReturnsFalse() { - // Documents that AuthenticationResultMetadata also lacks equals(), so two results - // with equivalent but separately-constructed metadata instances will not be equal. + void equals_EquivalentMetadataInstances_ReturnsTrue() { AuthenticationResultMetadata meta1 = AuthenticationResultMetadata.builder() .tokenSource(TokenSource.CACHE).build(); AuthenticationResultMetadata meta2 = AuthenticationResultMetadata.builder() @@ -352,8 +341,28 @@ void equals_DifferentMetadataInstances_ReturnsFalse() { .metadata(meta2) .build(); + assertEquals(result1, result2, + "Results with equivalent metadata instances should be equal"); + } + + @Test + void equals_DifferentMetadataValues_ReturnsFalse() { + AuthenticationResultMetadata meta1 = AuthenticationResultMetadata.builder() + .tokenSource(TokenSource.CACHE).build(); + AuthenticationResultMetadata meta2 = AuthenticationResultMetadata.builder() + .tokenSource(TokenSource.IDENTITY_PROVIDER).build(); + + AuthenticationResult result1 = AuthenticationResult.builder() + .expiresOn(EXPIRES_ON) + .metadata(meta1) + .build(); + AuthenticationResult result2 = AuthenticationResult.builder() + .expiresOn(EXPIRES_ON) + .metadata(meta2) + .build(); + assertNotEquals(result1, result2, - "Two results with equivalent metadata instances are not equal because AuthenticationResultMetadata lacks equals()"); + "Results with different metadata values should not be equal"); } @Test @@ -398,7 +407,7 @@ void hashCode_AllFieldsSet_DoesNotThrow() { .accountCacheEntity(buildAccountCacheEntity()) .environment(ENVIRONMENT) .scopes(SCOPES) - .metadata(SHARED_METADATA) + .metadata(buildMetadata()) .isPopAuthorization(true) .build(); @@ -465,4 +474,40 @@ void metadata_ToString_ContainsFieldValues() { assertTrue(toString.contains("100")); assertTrue(toString.contains("EXPIRED")); } + + @Test + void metadata_Equals_SameValues_ReturnsTrue() { + AuthenticationResultMetadata meta1 = AuthenticationResultMetadata.builder() + .tokenSource(TokenSource.CACHE) + .refreshOn(100L) + .cacheRefreshReason(CacheRefreshReason.EXPIRED) + .build(); + AuthenticationResultMetadata meta2 = AuthenticationResultMetadata.builder() + .tokenSource(TokenSource.CACHE) + .refreshOn(100L) + .cacheRefreshReason(CacheRefreshReason.EXPIRED) + .build(); + + assertEquals(meta1, meta2); + assertEquals(meta1.hashCode(), meta2.hashCode()); + } + + @Test + void metadata_Equals_DifferentTokenSource_ReturnsFalse() { + AuthenticationResultMetadata meta1 = AuthenticationResultMetadata.builder() + .tokenSource(TokenSource.CACHE).build(); + AuthenticationResultMetadata meta2 = AuthenticationResultMetadata.builder() + .tokenSource(TokenSource.IDENTITY_PROVIDER).build(); + + assertNotEquals(meta1, meta2); + } + + @Test + void metadata_Equals_NullAndWrongType() { + AuthenticationResultMetadata meta = AuthenticationResultMetadata.builder().build(); + + assertFalse(meta.equals(null)); + assertFalse(meta.equals("not metadata")); + assertEquals(meta, meta); + } } From a9e08b9933b694a86deff1bc235aa40a7e4d8664 Mon Sep 17 00:00:00 2001 From: avdunn Date: Tue, 9 Jun 2026 13:50:00 -0700 Subject: [PATCH 3/3] Remove unneeded test --- .../aad/msal4j/AuthenticationResultTest.java | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthenticationResultTest.java b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthenticationResultTest.java index 1882fe6d..44a502e0 100644 --- a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthenticationResultTest.java +++ b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthenticationResultTest.java @@ -216,21 +216,6 @@ void build_ExpiresOn_ExpiresOnDateConvertedCorrectly() { assertEquals(expectedDate, result.expiresOnDate()); } - @Test - void build_WithIdTokenButNullAccount_DoesNotThrowNPE_DueToInitOrder() { - // If the field initialization order bug were fixed, this scenario would throw NPE: - // getTenantProfile() calls getAccount().environment(), and getAccount() returns null - // when accountCacheEntity is null. However, since idToken is null at init time, - // getTenantProfile() returns null before reaching the getAccount() call. - AuthenticationResult result = AuthenticationResult.builder() - .idToken(TestHelper.ENCODED_JWT) - .accountCacheEntity(null) - .build(); - - assertNull(result.tenantProfile()); - assertEquals(TestHelper.ENCODED_JWT, result.idToken()); - } - // ========== equals() Tests ========== @Test