Skip to content
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

Pass optional tenant override to internal silent call #886

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Pass optional tenant override to internal silent call #886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0dcdc8b
Fix tenant override API not being properly used in certain confidenti…
Avery-Dunn Dec 9, 2024
f042744
Replace integration test with unit test
Avery-Dunn Dec 10, 2024
0874c4c
Improve unit test
Avery-Dunn Dec 10, 2024
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
2 changes: 1 addition & 1 deletion msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/AcquireTokenInteractiveIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ void acquireTokenInteractive_ManagedUser_InstanceAware() {

@Test
void acquireTokenInteractive_Ciam() {
User user = labUserProvider.getCiamUser();
User user = labUserProvider.getCiamCudUser();

Map<String, String> extraQueryParameters = new HashMap<>();

Expand Down
3 changes: 1 addition & 2 deletions msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/ClientCredentialsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ void acquireTokenClientCredentials_ClientCertificate() throws Exception {
void acquireTokenClientCredentials_ClientSecret() throws Exception {
AppCredentialProvider appProvider = new AppCredentialProvider(AzureEnvironment.AZURE);
final String clientId = appProvider.getLabVaultAppId();
final String password = appProvider.getLabVaultPassword();
IClientCredential credential = CertificateHelper.getClientCertificate();

assertAcquireTokenCommon(clientId, credential, TestConstants.MICROSOFT_AUTHORITY);
Expand All @@ -68,7 +67,7 @@ void acquireTokenClientCredentials_ClientAssertion() throws Exception {
@Test
void acquireTokenClientCredentials_ClientSecret_Ciam() throws Exception {

User user = labUserProvider.getCiamUser();
User user = labUserProvider.getCiamCudUser();
String clientId = user.getAppId();

AppCredentialProvider appProvider = new AppCredentialProvider(AzureEnvironment.CIAM);
Expand Down
20 changes: 10 additions & 10 deletions msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/OnBehalfOfIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ void acquireTokenWithOBO_Managed(String environment) throws Exception {
new UserAssertion(accessToken)).build()).
get();

assertNotNull(result);
assertNotNull(result.accessToken());
assertResultNotNull(result);
}

@ParameterizedTest
Expand All @@ -63,8 +62,7 @@ void acquireTokenWithOBO_testCache(String environment) throws Exception {
new UserAssertion(accessToken)).build()).
get();

assertNotNull(result1);
assertNotNull(result1.accessToken());
assertResultNotNull(result1);

// Same scope and userAssertion, should return cached tokens
IAuthenticationResult result2 =
Expand All @@ -82,8 +80,7 @@ void acquireTokenWithOBO_testCache(String environment) throws Exception {
new UserAssertion(accessToken)).build()).
get();

assertNotNull(result3);
assertNotNull(result3.accessToken());
assertResultNotNull(result3);
assertNotEquals(result2.accessToken(), result3.accessToken());

// Scope 2, should return cached token
Expand All @@ -105,8 +102,7 @@ void acquireTokenWithOBO_testCache(String environment) throws Exception {
.build()).
get();

assertNotNull(result5);
assertNotNull(result5.accessToken());
assertResultNotNull(result5);
assertNotEquals(result5.accessToken(), result4.accessToken());
assertNotEquals(result5.accessToken(), result2.accessToken());

Expand All @@ -121,13 +117,17 @@ void acquireTokenWithOBO_testCache(String environment) throws Exception {
.build()).
get();

assertNotNull(result6);
assertNotNull(result6.accessToken());
assertResultNotNull(result6);
assertNotEquals(result6.accessToken(), result5.accessToken());
assertNotEquals(result6.accessToken(), result4.accessToken());
assertNotEquals(result6.accessToken(), result2.accessToken());
}

private void assertResultNotNull(IAuthenticationResult result) {
assertNotNull(result);
assertNotNull(result.accessToken());
}

private String getAccessToken() throws Exception {

LabUserProvider labUserProvider = LabUserProvider.getInstance();
Expand Down
2 changes: 1 addition & 1 deletion msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/UsernamePasswordIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ void acquireTokenWithUsernamePassword_Ciam() throws Exception {

Map<String, String> extraQueryParameters = new HashMap<>();

User user = labUserProvider.getCiamUser();
User user = labUserProvider.getCiamCudUser();
PublicClientApplication pca = PublicClientApplication.builder(user.getAppId())
.authority("https://" + user.getLabName() + ".ciamlogin.com/")
.build();
Expand Down
6 changes: 0 additions & 6 deletions msal4j-sdk/src/integrationtest/java/labapi/AppCredentialProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ public class AppCredentialProvider {
private KeyVaultSecretsProvider keyVaultSecretsProvider;

private String labVaultClientId;
private String labVaultPassword;

private String clientId;

Expand All @@ -19,7 +18,6 @@ public AppCredentialProvider(String azureEnvironment) {
keyVaultSecretsProvider = new KeyVaultSecretsProvider();

labVaultClientId = keyVaultSecretsProvider.getSecret(LabConstants.APP_ID_KEY_VAULT_SECRET);
labVaultPassword = keyVaultSecretsProvider.getSecret(LabConstants.APP_PASSWORD_KEY_VAULT_SECRET);

switch (azureEnvironment) {
case AzureEnvironment.AZURE:
Expand Down Expand Up @@ -65,8 +63,4 @@ public String getOboAppPassword() {
public String getLabVaultAppId() {
return labVaultClientId;
}

public String getLabVaultPassword() {
return labVaultPassword;
}
}
2 changes: 1 addition & 1 deletion msal4j-sdk/src/integrationtest/java/labapi/LabConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public class LabConstants {
public final static String USER_MSA_USERNAME_URL = "https://msidlabs.vault.azure.net/secrets/MSA-MSIDLAB4-UserName";
public final static String USER_MSA_PASSWORD_URL = "https://msidlabs.vault.azure.net/secrets/MSA-MSIDLAB4-Password";
public final static String OBO_APP_PASSWORD_URL = "https://msidlabs.vault.azure.net/secrets/TodoListServiceV2-OBO";
public final static String CIAM_KEY_VAULT_SECRET_KEY = "https://msidlabs.vault.azure.net/secrets/MSIDLABCIAM2-cc";
public final static String CIAM_KEY_VAULT_SECRET_KEY = "https://msidlabs.vault.azure.net/secrets/MSIDLABCIAM6-cc";

public final static String ARLINGTON_APP_ID = "cb7faed4-b8c0-49ee-b421-f5ed16894c83";
public final static String ARLINGTON_OBO_APP_ID = "c0555d2d-02f2-4838-802e-3463422e571d";
Expand Down
8 changes: 0 additions & 8 deletions msal4j-sdk/src/integrationtest/java/labapi/LabUserProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,14 +105,6 @@ public User getUserByGuestHomeAzureEnvironments(String guestEnvironment, String
return getLabUser(query);
}

public User getCiamUser() {

UserQueryParameters query = new UserQueryParameters();
query.parameters.put(UserQueryParameters.FEDERATION_PROVIDER, FederationProvider.CIAM);

return getLabUser(query);
}

public User getCiamCudUser() {
UserQueryParameters query = new UserQueryParameters();
query.parameters.put(UserQueryParameters.FEDERATION_PROVIDER, FederationProvider.CIAMCUD);
Expand Down
1 change: 1 addition & 0 deletions ...4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenByClientCredentialSupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ AuthenticationResult execute() throws Exception {
SilentParameters parameters = SilentParameters
.builder(this.clientCredentialRequest.parameters.scopes())
.claims(this.clientCredentialRequest.parameters.claims())
.tenant(this.clientCredentialRequest.parameters.tenant())
Copy link
Member

Choose a reason for hiding this comment

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

This is not tested.

.build();

RequestContext context = new RequestContext(
Expand Down
1 change: 1 addition & 0 deletions msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenByOnBehalfOfSupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ AuthenticationResult execute() throws Exception {
SilentParameters parameters = SilentParameters
.builder(this.onBehalfOfRequest.parameters.scopes())
.claims(this.onBehalfOfRequest.parameters.claims())
.tenant(this.onBehalfOfRequest.parameters.tenant())
.build();

RequestContext context = new RequestContext(
Expand Down
95 changes: 95 additions & 0 deletions msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/OnBehalfOfTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.microsoft.aad.msal4j;

import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.*;
import static org.mockito.Mockito.times;

@ExtendWith(MockitoExtension.class)
class OnBehalfOfTests {

private String getSuccessfulResponse(String accessToken) {
return "{\"access_token\":\""+accessToken+"\",\"expires_in\": \""+ 60*60*1000 +"\",\"token_type\":" +
Copy link
Member

Choose a reason for hiding this comment

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

You also need to add a client_info, that is part of all user token protocols.

"\"Bearer\",\"client_id\":\"client_id\",\"Content-Type\":\"text/html; charset=utf-8\"}";
Copy link
Member

Choose a reason for hiding this comment

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

  1. Consider adding this to a common file. It's the standard /token response for S2S
  2. OBO receives id tokens, refresh tokens etc. The response should reflect that.
  3. I am not aware of the COntent-Type being part of the response content. It may be a header?

}

private HttpResponse expectedResponse(int statusCode, String response) {
Map<String, List<String>> headers = new HashMap<String, List<String>>();
headers.put("Content-Type", Collections.singletonList("application/json"));

HttpResponse httpResponse = new HttpResponse();
httpResponse.statusCode(statusCode);
httpResponse.body(response);
httpResponse.addHeaders(headers);

return httpResponse;
}

@Test
void OnBehalfOf_InternalCacheLookup_Success() throws Exception {
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);

when(httpClientMock.send(any(HttpRequest.class))).thenReturn(expectedResponse(200, getSuccessfulResponse("token")));

ConfidentialClientApplication cca =
ConfidentialClientApplication.builder("clientId", ClientCredentialFactory.createFromSecret("password"))
Copy link
Member

Choose a reason for hiding this comment

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

Consider using some constants for ClientID.

.authority("https://login.microsoftonline.com/tenant/")
.instanceDiscovery(false)
.validateAuthority(false)
.httpClient(httpClientMock)
.build();

OnBehalfOfParameters parameters = OnBehalfOfParameters.builder(Collections.singleton("scopes"), new UserAssertion(TestHelper.signedToken)).build();

IAuthenticationResult result = cca.acquireToken(parameters).get();
IAuthenticationResult result2 = cca.acquireToken(parameters).get();

//OBO flow should perform an internal cache lookup, so similar parameters should only cause one HTTP client call
Copy link
Member

Choose a reason for hiding this comment

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

So do you confirm that the lookup is done by assertion_hash in this case?

assertEquals(result.accessToken(), result2.accessToken());
verify(httpClientMock, times(1)).send(any());
}

@Test
void OnBehalfOf_TenantOverride() throws Exception {
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);

ConfidentialClientApplication cca =
ConfidentialClientApplication.builder("clientId", ClientCredentialFactory.createFromSecret("password"))
.authority("https://login.microsoftonline.com/tenant")
.instanceDiscovery(false)
Copy link
Member

Choose a reason for hiding this comment

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

ok for now, but the http mock can easily handle this as well.

.validateAuthority(false)
.httpClient(httpClientMock)
.build();

when(httpClientMock.send(any(HttpRequest.class))).thenReturn(expectedResponse(200, getSuccessfulResponse("appTenantToken")));
Copy link
Member

Choose a reason for hiding this comment

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

THis is a great path forward for tests. Consider making a separate class with all this logic. It should the standard way of doing tests and little by little all tests should be re-written to use this.

Copy link
Member

Choose a reason for hiding this comment

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

You should only return that response once, not all the time, i.e. assert that only 1 call to the token endpoint is made.

OnBehalfOfParameters parameters = OnBehalfOfParameters.builder(Collections.singleton("scopes"), new UserAssertion(TestHelper.signedToken)).build();

//The two acquireToken calls have the same parameters and should only cause one call from the HTTP client
IAuthenticationResult resultAppLevelTenant = cca.acquireToken(parameters).get();
cca.acquireToken(parameters).get();
assertEquals(1, cca.tokenCache.accessTokens.size());
verify(httpClientMock, times(1)).send(any());

when(httpClientMock.send(any(HttpRequest.class))).thenReturn(expectedResponse(200, getSuccessfulResponse("requestTenantToken")));
parameters = OnBehalfOfParameters.builder(Collections.singleton("scopes"), new UserAssertion(TestHelper.signedToken)).tenant("otherTenant").build();

//Overriding the tenant parameter in the request should lead to a new token call being made, but followup calls should not
IAuthenticationResult resultRequestLevelTenant = cca.acquireToken(parameters).get();
cca.acquireToken(parameters).get();
assertEquals(2, cca.tokenCache.accessTokens.size());
verify(httpClientMock, times(2)).send(any());
assertNotEquals(resultAppLevelTenant.accessToken(), resultRequestLevelTenant.accessToken());
}
}
36 changes: 32 additions & 4 deletions msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/TestHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@

package com.microsoft.aad.msal4j;

import com.nimbusds.jose.*;
import com.nimbusds.jose.crypto.RSASSASigner;
import com.nimbusds.jose.jwk.RSAKey;
import com.nimbusds.jose.jwk.gen.RSAKeyGenerator;

import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
Expand All @@ -12,10 +17,16 @@

class TestHelper {

static String readResource(Class<?> classInstance, String resource) throws IOException, URISyntaxException {
return new String(
Files.readAllBytes(
Paths.get(classInstance.getResource(resource).toURI())));
//Signed JWT which should be enough to pass the parsing/validation in the library, useful if a unit test needs an
// assertion in a request or token in a response but that is not the focus of the test
static String signedToken = generateToken();

static String readResource(Class<?> classInstance, String resource) {
try {
return new String(Files.readAllBytes(Paths.get(classInstance.getResource(resource).toURI())));
} catch (IOException | URISyntaxException e) {
throw new RuntimeException(e);
}
}

static void deleteFileContent(Class<?> classInstance, String resource)
Expand All @@ -27,4 +38,21 @@ static void deleteFileContent(Class<?> classInstance, String resource)
fileWriter.write("");
fileWriter.close();
}

static String generateToken() {
Copy link
Member

Choose a reason for hiding this comment

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

This is fine. Other MSALs just create a json to represent the JWK. MSALs do not validate headers nor signatures, so only the payload matters.

You don't really need a "proper" token for OBO. MSAL never looks into it.

You will need a proper id token though.

try {
RSAKey rsaJWK = new RSAKeyGenerator(2048)
.keyID("kid")
.generate();
JWSObject jwsObject = new JWSObject(
new JWSHeader.Builder(JWSAlgorithm.RS256).keyID(rsaJWK.getKeyID()).build(),
new Payload("payload"));

jwsObject.sign(new RSASSASigner(rsaJWK));

return jwsObject.serialize();
} catch (JOSEException e) {
throw new RuntimeException(e);
}
}
}
Loading