Pass optional tenant override to internal silent call #886

Avery-Dunn · 2024-12-09T02:39:55Z

Fixes an issue described in #881. Essentially, there is an API at the request level which overrides the tenant set at the application level.

However, in some confidential flows we internally make a silent call with a new SilentParameters object which did not receive the tenant that may have been set with that API.

This PR fixes the two places where that API wasn't getting properly passed to that internal silent call, adds a new test for that behavior, and makes a few unrelated fixes/improvements to some integration tests.

…al flows

bgavrilMS · 2024-12-09T16:21:59Z

msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/OnBehalfOfIT.java

+        assertResultNotNull(resultOrganizations);
+        assertResultNotNull(resultOrganizationsCached);
+
+        assertNotEquals(resultNoOverride.accessToken(), resultOrganizations.accessToken());


nit: you really should get that improvement of exposing a fromCache flag in the auth result going. This string comparison is not great. ESTS-R for example caches tokens.

bgavrilMS · 2024-12-09T16:30:18Z

msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/OnBehalfOfIT.java

+        IAuthenticationResult resultOrganizations = cca.acquireToken(OnBehalfOfParameters.builder(
+                                Collections.singleton(cfg.graphDefaultScope()),
+                                new UserAssertion(accessToken))
+                                .tenant("organizations")


I am not sure I agree with this test. Afaik MSAL will use the tenant ID from the response, not from the request, so it's not that clear what this test does. It's also showcases 2 anti-patterns:

go from a tenant specific authority on the CCA to the lmo/organizations authority on the request. Usually it's the other way around.

OBO with "/common" or "/organizations". This is wrong, OBO should always be tenanted. It should use the client token's tid claim. Otherwise you introduce a bug where guest users can't login. See https://learn.microsoft.com/en-us/entra/msal/dotnet/acquiring-tokens/web-apps-apis/on-behalf-of-flow#important-note-on-on-behalf-of-obo-flow-with-guest-users

My recommendation is to rely on unit tests for this one:

app authority is { "lmo/organizations", "lmo/T1", "lmo/T2" }

req tenant is {T1, T2}

flow { OBO, client_creds, silent}

So there are quite a few unit tests (3x2x3), but mostly repetitive.

Then Assert on the correct MSAL behavior i.e. token from cache or from the correctly tenanted token endpoint

Agreed that unit tests would be better, originally I just did an integration test since there were ones to copy from but we didn't have any OBO unit tests.

In the latest commit I replaced this integration test with a couple of unit tests. It's not as many as you suggest, but they should show the correct behavior by checking for the correct number of HTTPClient calls, the token cache size, etc., depending on whether the tenant was set at the request level or not.

bgavrilMS

Needs a unit test

bgavrilMS · 2024-12-13T11:53:44Z

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/OnBehalfOfTests.java

+
+    private String getSuccessfulResponse(String accessToken) {
+        return "{\"access_token\":\""+accessToken+"\",\"expires_in\": \""+ 60*60*1000 +"\",\"token_type\":" +
+                "\"Bearer\",\"client_id\":\"client_id\",\"Content-Type\":\"text/html; charset=utf-8\"}";


Consider adding this to a common file. It's the standard /token response for S2S

OBO receives id tokens, refresh tokens etc. The response should reflect that.

I am not aware of the COntent-Type being part of the response content. It may be a header?

bgavrilMS · 2024-12-13T11:54:44Z

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/OnBehalfOfTests.java

+        when(httpClientMock.send(any(HttpRequest.class))).thenReturn(expectedResponse(200, getSuccessfulResponse("token")));
+
+        ConfidentialClientApplication cca =
+                ConfidentialClientApplication.builder("clientId", ClientCredentialFactory.createFromSecret("password"))


Consider using some constants for ClientID.

bgavrilMS · 2024-12-13T11:57:30Z

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/OnBehalfOfTests.java

+class OnBehalfOfTests {
+
+    private String getSuccessfulResponse(String accessToken) {
+        return "{\"access_token\":\""+accessToken+"\",\"expires_in\": \""+ 60*60*1000 +"\",\"token_type\":" +


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

bgavrilMS · 2024-12-13T11:59:01Z

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/OnBehalfOfTests.java

+                        .httpClient(httpClientMock)
+                        .build();
+
+        when(httpClientMock.send(any(HttpRequest.class))).thenReturn(expectedResponse(200, getSuccessfulResponse("appTenantToken")));


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.

bgavrilMS · 2024-12-13T12:00:25Z

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/OnBehalfOfTests.java

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


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

bgavrilMS · 2024-12-13T12:03:10Z

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/OnBehalfOfTests.java

+        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


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

bgavrilMS · 2024-12-13T12:05:22Z

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

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


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.

bgavrilMS · 2024-12-13T12:05:35Z

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenByClientCredentialSupplier.java

@@ -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())


This is not tested.

bgavrilMS

More testing is needed.

bgavrilMS · 2024-12-13T12:08:22Z

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/OnBehalfOfTests.java

+                        .httpClient(httpClientMock)
+                        .build();
+
+        when(httpClientMock.send(any(HttpRequest.class))).thenReturn(expectedResponse(200, getSuccessfulResponse("appTenantToken")));


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

Fix tenant override API not being properly used in certain confidenti…

0dcdc8b

…al flows

Avery-Dunn requested a review from a team as a code owner December 9, 2024 02:39

Avery-Dunn mentioned this pull request Dec 9, 2024

[Bug] On behalf of supplier doesn't pass tenant id to silent supplier #881

Open

bgavrilMS reviewed Dec 9, 2024

View reviewed changes

bgavrilMS requested changes Dec 9, 2024

View reviewed changes

Avery-Dunn added 2 commits December 10, 2024 15:15

Replace integration test with unit test

f042744

Improve unit test

0874c4c

bgavrilMS reviewed Dec 13, 2024

View reviewed changes

bgavrilMS requested changes Dec 13, 2024

View reviewed changes

bgavrilMS reviewed Dec 13, 2024

View reviewed changes

bgavrilMS approved these changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass optional tenant override to internal silent call #886

Pass optional tenant override to internal silent call #886

Avery-Dunn commented Dec 9, 2024

bgavrilMS Dec 9, 2024

bgavrilMS Dec 9, 2024

Avery-Dunn Dec 10, 2024

bgavrilMS left a comment

bgavrilMS Dec 13, 2024

bgavrilMS Dec 13, 2024

bgavrilMS Dec 13, 2024

bgavrilMS Dec 13, 2024

bgavrilMS Dec 13, 2024

bgavrilMS Dec 13, 2024

bgavrilMS Dec 13, 2024

bgavrilMS Dec 13, 2024

bgavrilMS left a comment

bgavrilMS Dec 13, 2024

Pass optional tenant override to internal silent call #886

Are you sure you want to change the base?

Pass optional tenant override to internal silent call #886

Conversation

Avery-Dunn commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment