Skip to content

Commit 5aed8d5

Browse files
Avery-DunnAvery-Dunn
committed
Address PR feedback
1 parent 9881d5d commit 5aed8d5

File tree

3 files changed

+123
-12
lines changed

3 files changed

+123
-12
lines changed

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

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -575,14 +575,8 @@ public T correlationId(String val) {
575575
aadAadInstanceDiscoveryResponse);
576576
}
577577

578-
if (authenticationAuthority.authorityType == AuthorityType.OIDC) {
579-
((OidcAuthority) authenticationAuthority).setAuthorityProperties(
580-
OidcDiscoveryProvider.performOidcDiscovery(
581-
(OidcAuthority) authenticationAuthority, this));
582-
583-
if (!((OidcAuthority) authenticationAuthority).isIssuerValid()) {
584-
throw new MsalClientException(String.format("Invalid issuer from OIDC discovery: %s", ((OidcAuthority) authenticationAuthority).issuerFromOidcDiscovery), "issuer_validation");
585-
}
586-
}
578+
((OidcAuthority) authenticationAuthority).setAuthorityProperties(
579+
OidcDiscoveryProvider.performOidcDiscovery(
580+
(OidcAuthority) authenticationAuthority, this));
587581
}
588582
}

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

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,11 @@
88

99
public class OidcAuthority extends Authority {
1010
//Part of the OpenIdConnect standard, this is appended to the authority to create the endpoint that has OIDC metadata
11-
static final String WELL_KNOWN_OPENID_CONFIGURATION = ".well-known/openid-configuration";
11+
private static final String WELL_KNOWN_OPENID_CONFIGURATION = ".well-known/openid-configuration";
1212
private static final String AUTHORITY_FORMAT = "https://%s/%s/";
1313
private static final String CIAM_AUTHORITY_FORMAT = "https://%s.ciamlogin.com/%s";
1414

15-
String issuerFromOidcDiscovery;
15+
private String issuerFromOidcDiscovery;
1616

1717
OidcAuthority(URL authorityUrl) throws MalformedURLException {
1818
super(createOidcDiscoveryUrl(authorityUrl), AuthorityType.OIDC);
@@ -33,6 +33,16 @@ void setAuthorityProperties(OidcDiscoveryResponse instanceDiscoveryResponse) {
3333
this.deviceCodeEndpoint = instanceDiscoveryResponse.deviceCodeEndpoint();
3434
this.selfSignedJwtAudience = this.tokenEndpoint;
3535
this.issuerFromOidcDiscovery = instanceDiscoveryResponse.issuer();
36+
37+
validateIssuer();
38+
}
39+
40+
private void validateIssuer() {
41+
if (!isIssuerValid()) {
42+
throw new MsalClientException(
43+
String.format("Invalid issuer from OIDC discovery. Issuer %s does not match authority %s, or is in an unexpected format", issuerFromOidcDiscovery, canonicalAuthorityUrl),
44+
"issuer_validation");
45+
}
3646
}
3747

3848
/**
@@ -42,7 +52,7 @@ void setAuthorityProperties(OidcDiscoveryResponse instanceDiscoveryResponse) {
4252
*
4353
* @return true if the issuer is valid, false otherwise
4454
*/
45-
boolean isIssuerValid() {
55+
private boolean isIssuerValid() {
4656
if (issuerFromOidcDiscovery == null) {
4757
return false;
4858
}

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

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
package com.microsoft.aad.msal4j;
2+
3+
import org.junit.jupiter.api.Test;
4+
import org.junit.jupiter.api.BeforeEach;
5+
import org.mockito.Mockito;
6+
7+
import java.net.MalformedURLException;
8+
import java.net.URL;
9+
10+
import static org.junit.jupiter.api.Assertions.*;
11+
import static org.mockito.Mockito.*;
12+
13+
class OidcAuthorityTest {
14+
15+
private OidcDiscoveryResponse mockDiscoveryResponse;
16+
17+
@BeforeEach
18+
void setup() {
19+
mockDiscoveryResponse = Mockito.mock(OidcDiscoveryResponse.class);
20+
when(mockDiscoveryResponse.authorizationEndpoint()).thenReturn("https://login.example.com/authorize");
21+
when(mockDiscoveryResponse.tokenEndpoint()).thenReturn("https://login.example.com/token");
22+
when(mockDiscoveryResponse.deviceCodeEndpoint()).thenReturn("https://login.example.com/devicecode");
23+
}
24+
25+
@Test
26+
void testSetAuthorityProperties_IssuerMatchesAuthority() throws MalformedURLException {
27+
// Arrange
28+
URL authorityUrl = new URL("https://login.example.com/tenant1/");
29+
OidcAuthority authority = new OidcAuthority(authorityUrl);
30+
31+
// Match the issuer to the authority URL (without the well-known segment)
32+
when(mockDiscoveryResponse.issuer()).thenReturn("https://login.example.com/tenant1/");
33+
34+
// Act & Assert - Should not throw exception
35+
assertDoesNotThrow(() -> authority.setAuthorityProperties(mockDiscoveryResponse));
36+
37+
// Verify properties were set
38+
assertEquals("https://login.example.com/authorize", authority.authorizationEndpoint());
39+
assertEquals("https://login.example.com/token", authority.tokenEndpoint());
40+
assertEquals("https://login.example.com/devicecode", authority.deviceCodeEndpoint());
41+
assertEquals("https://login.example.com/token", authority.selfSignedJwtAudience);
42+
}
43+
44+
@Test
45+
void testSetAuthorityProperties_IssuerFollowsCiamPattern() throws MalformedURLException {
46+
// Arrange
47+
String tenant = "contoso";
48+
URL authorityUrl = new URL("https://" + tenant + ".ciamlogin.com/" + tenant + "/");
49+
OidcAuthority authority = new OidcAuthority(authorityUrl);
50+
51+
// Set an issuer that follows CIAM pattern but doesn't exactly match the authority
52+
String ciamIssuer = "https://" + tenant + ".ciamlogin.com/" + tenant + "/v2.0";
53+
when(mockDiscoveryResponse.issuer()).thenReturn(ciamIssuer);
54+
55+
// Act & Assert - Should not throw exception
56+
assertDoesNotThrow(() -> authority.setAuthorityProperties(mockDiscoveryResponse));
57+
}
58+
59+
@Test
60+
void testSetAuthorityProperties_IssuerInvalid() throws MalformedURLException {
61+
// Arrange
62+
URL authorityUrl = new URL("https://login.example.com/tenant1/");
63+
OidcAuthority authority = new OidcAuthority(authorityUrl);
64+
65+
// Set an issuer that doesn't match the authority and doesn't follow CIAM pattern
66+
when(mockDiscoveryResponse.issuer()).thenReturn("https://login.different.com/tenant1/");
67+
68+
// Act & Assert - Should throw MsalClientException
69+
MsalClientException exception = assertThrows(MsalClientException.class,
70+
() -> authority.setAuthorityProperties(mockDiscoveryResponse));
71+
72+
// Verify exception details
73+
assertEquals("issuer_validation", exception.errorCode());
74+
assertTrue(exception.getMessage().contains("Invalid issuer from OIDC discovery"));
75+
}
76+
77+
@Test
78+
void testSetAuthorityProperties_IssuerIsNull() throws MalformedURLException {
79+
// Arrange
80+
URL authorityUrl = new URL("https://login.example.com/tenant1/");
81+
OidcAuthority authority = new OidcAuthority(authorityUrl);
82+
83+
// Set null issuer
84+
when(mockDiscoveryResponse.issuer()).thenReturn(null);
85+
86+
// Act & Assert - Should throw MsalClientException
87+
MsalClientException exception = assertThrows(MsalClientException.class,
88+
() -> authority.setAuthorityProperties(mockDiscoveryResponse));
89+
90+
// Verify exception details
91+
assertEquals("issuer_validation", exception.errorCode());
92+
assertTrue(exception.getMessage().contains("Invalid issuer from OIDC discovery"));
93+
}
94+
95+
@Test
96+
void testSetAuthorityProperties_TrailingSlashNormalization() throws MalformedURLException {
97+
// Arrange
98+
URL authorityUrl = new URL("https://login.example.com/tenant1/");
99+
OidcAuthority authority = new OidcAuthority(authorityUrl);
100+
101+
// Match the issuer to the authority but without trailing slash
102+
when(mockDiscoveryResponse.issuer()).thenReturn("https://login.example.com/tenant1");
103+
104+
// Act & Assert - Should not throw exception because normalization happens
105+
assertDoesNotThrow(() -> authority.setAuthorityProperties(mockDiscoveryResponse));
106+
}
107+
}

0 commit comments

Comments
 (0)