Azure-sdk-for-java: [BUG] azure ad graph client ignores odata.nextLink

[ppartarr]

Describe the bug
In AzureADGraphClient.java of package com.microsoft.azure.spring.autoconfigure.aad: when querying the /memberOf endpoint to obtain group membership information, it is possible that a user is a member of more than 100 groups. In this case, the response will look like this:

{
   "odata.metadata":"https://graph.windows.net/myorganization/$metadata#directoryObjects",
   "odata.nextLink":"directoryObjects/$/Microsoft.DirectoryServices.User/userId/memberOf?$skiptoken=X'token'",
   "value":[
      ...
   ]
}

Notice the odata.nextLink JSON key, which isn't discussed in the docs for this endpoint. The AzureADGraphClient ignores the nextLink and simply uses the first 100 groups in the JSON response to further populate the user's granted authorities in Spring.

This is an issue for applications relying on the user's active directory group memberships to determine the granted authorities because not all of the users groups will be converted to authorities

Exception or Stack Trace
I can provide logs of the /memberOf request and response if needed. We can clearly see that the response contains odata.nextLink but the logs only show one /memberOf request.

To Reproduce

1. build a spring application with the following dependency:

        <dependency>
            <groupId>com.microsoft.azure</groupId>
            <artifactId>azure-spring-boot</artifactId>
        </dependency>

1. write a GrantedAuthoritiesMapper based on https://docs.spring.io/spring-security/site/docs/current/reference/html5/#oauth2login-advanced-map-authorities-grantedauthoritiesmapper
2. create 100+ AD groups and add a user to 100+ AD groups

For step 3, the following PowerShell scripts are quite usefull: mkGroups.ps1 and rmGroups.ps1

Code Snippet
https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/spring/azure-spring-boot/src/main/java/com/microsoft/azure/spring/autoconfigure/aad/AzureADGraphClient.java#L123

Expected behavior
The function loadGroups should call getUserMembershipv1 as long as the returned JSON contains a odata.nextLink JSON key.

Setup (please complete the following information):

• OS: Windows 10 Pro
• IDE: Eclipse version 2019-09 R (4.13.0)
• Version of the Library used: 2.2.1 but all version are affected

Additional context
Please assign this issue to me, I am currently working on a fix and will make a PR asap

Information Checklist
Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

• [x] Bug Description Added
• [x] Repro Steps Added
• [x] Setup information Added

[joshfree]

Thanks for filing this github issue, @ppartarr. Someone from the azure-spring team will follow up with you shortly.

/cc @jialindai

[ppartarr]

@chenrujun @jialindai can you have a look at the fix? It's ready to merge https://github.com/Azure/azure-sdk-for-java/pull/14305

[peter-c-larsson]

Was this really fixed?

We still see this issue with the latest 2.3.5 version

I see it in the release notes of the latest release: https://mvnrepository.com/artifact/com.microsoft.azure/azure-active-directory-spring-boot-starter/2.3.5

# Release History

## 2.3.5 (2020-09-14)
_Bug Fixes_
- Get full list of groups the user belongs to from Graph API

I see part of the the bug fix here in the forked version:
https://github.com/ppartarr/azure-sdk-for-java/blob/master/sdk/spring/azure-spring-boot/src/main/java/com/microsoft/azure/spring/autoconfigure/aad/AzureADGraphClient.java#L151

And not in the either the tagged version:
https://github.com/Azure/azure-sdk-for-java/blob/azure-spring-boot_2.3.5/sdk/spring/azure-spring-boot/src/main/java/com/microsoft/azure/spring/autoconfigure/aad/AzureADGraphClient.java

Or the master after the package restructure.
https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/spring/azure-spring-boot/src/main/java/com/azure/spring/autoconfigure/aad/AzureADGraphClient.java

Or am I missing something?

[ppartarr]

Seems like you're right @peter-c-larsson. The fix was overwritten by @chenrujun in this commit. It's probably worth adding a test for this

[peter-c-larsson]

@jialindai, @chenrujun and @joshfree: Can we have this issue reopened again and any ETA on when the the fix can be reapplied and released?

[chenrujun]

Hi, @peter-c-larsson

Was this really fixed?

Yes, this was fixed since 2.3.5.

Here is related code: https://github.com/Azure/azure-sdk-for-java/blob/azure-spring-boot_2.3.5/sdk/spring/azure-spring-boot/src/main/java/com/microsoft/azure/spring/autoconfigure/aad/AzureADGraphClient.java#L125-L135

Or am I missing something?

I rewrite this part of code. Sorry for making you confused.

@ppartarr

It's probably worth adding a test

Already added integration test.
Here it is: https://github.com/Azure/azure-sdk-for-java/blob/azure-spring-boot_2.3.5/sdk/spring/azure-spring-boot-test-aad/src/test/java/com/microsoft/azure/test/aad/groups/AADGroupsCountIT.java#L41-L66

@peter-c-larsson

Can we have this issue reopened again and any ETA on when the the fix can be reapplied and released?

I reopened it.

• Could you please have a detailed description of your problem?

• Or please debug in your localhost about the following code: https://github.com/Azure/azure-sdk-for-java/blob/azure-spring-boot_2.3.5/sdk/spring/azure-spring-boot/src/main/java/com/microsoft/azure/spring/autoconfigure/aad/AzureADGraphClient.java#L125-L135

[peter-c-larsson]

Thanks. We have 2 users who can't access our application and the only thing that stands out is that they have a lot of groups 100+.

Will investigate further and update the case with some more info with what I find.

@ppartarr : Did you test or verify the released version?

[ppartarr]

Ah indeed @chenrujun, I read over too fast and skipped the assignment of the oDataNextLink to urlString here, mb!

@peter-c-larsson I couldn't wait for the bug fix to be released so I backported the fix to 2.3.2 and have been using that version since. You can use these powershell scripts to test if the number of group memberships is indeed causing the issue: [1] [2]. I will also do some testing on my end.

[1] https://github.com/ppartarr/active-directory-aspnetcore-webapp-openidconnect-v2/blob/master/5-WebApp-AuthZ/5-2-Groups/AppCreationScripts/BulkCreateGroups.ps1
[2] https://github.com/ppartarr/active-directory-aspnetcore-webapp-openidconnect-v2/blob/master/5-WebApp-AuthZ/5-2-Groups/AppCreationScripts/BulkRemoveGroups.ps1

[peter-c-larsson]

Hi!

Now I have managed to reproduce the issue and it seems that the second link always returns null

To make it easier to test for me:

azure.service.endpoints.global-v2-graph.aadMembershipRestUri=https://graph.microsoft.com/v1.0/me/memberOf
==>
azure.service.endpoints.global-v2-graph.aadMembershipRestUri=https://graph.microsoft.com/v1.0/me/memberOf?$top=5

The added debug:

        System.out.println("=== urlString 1 debug: " + urlString);        
        while (urlString != null) {
            String responseInJson = getUserMemberships(graphApiToken, urlString);
            UserGroups userGroups = objectMapper.readValue(responseInJson, UserGroups.class);
            userGroups.getValue()
                      .stream()
                      .filter(this::isMatchingUserGroupKey)
                      .forEach(userGroupList::add);
            System.out.println("=== number of groups: " + userGroups.getValue().size());    
            urlString = Optional.of(userGroups)
                                .map(UserGroups::getOdataNextLink)
                                .map(this::getUrlStringFromODataNextLink)
                                .orElse(null);
            System.out.println("=== urlString 2 debug: " + urlString);    
        }

And the result:

=== urlString 1 debug: https://graph.microsoft.com/v1.0/me/memberOf?$top=5
=== number of groups: 5
=== urlString 2 debug: null

[lu-cheng]

Hi @peter-c-larsson This bug has been fixed in the latest version.

[peter-c-larsson]

Thanks @lu-cheng !

I don't see any new version here however:
https://mvnrepository.com/artifact/com.microsoft.azure/azure-active-directory-spring-boot-starter

So I take it that it is just this version that has the fix:
https://mvnrepository.com/artifact/com.microsoft.azure/azure-spring-boot

And once it leaves beta both versions will be updated?

[lu-cheng]

@peter-c-larsson The fixed code has not been released, please wait for the next version, the time is uncertain.The fixed PR: #17210

[saragluna]

Hey all, we've just released the azure-spring-boot-starter-active-directory 3.0.0. But in this release, we brought several breaking changes and new features, so please refer to this to upgrade.

Closing this issue now.