Skip to content

Fix bugs about connect to Azure AD by proxy #31213

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

Closed
chenrujun wants to merge 9 commits into from
Closed

Fix bugs about connect to Azure AD by proxy #31213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5470386
Fix issue 31191: duplicated scopes.
rujche Sep 29, 2022
8836ab2
Fix version error in CHANGELOG.md.
rujche Oct 8, 2022
88b1e04
Simplify AadRestTemplateCreator.
rujche Oct 8, 2022
5af9bed
Update the logic of unmodifiableMap, make it more robust.
rujche Oct 8, 2022
a647731
Fix bug: "addHeadersConverter" and "addParametersConverter" should on…
rujche Oct 8, 2022
057d4ab
Merge branch 'main' into fix-issue-31191-duplicated-scopes
rujche Oct 8, 2022
d76c78a
Fix error by adding "builder = builder.xxx"
rujche Oct 8, 2022
9f7fbf9
Fix #31218. Set restOperations even if jwkResolver is null.
rujche Oct 8, 2022
0f5851f
Fix error: AadJwtBearerGrantRequestEntityConverter should be used eve…
rujche Oct 8, 2022
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
5 changes: 5 additions & 0 deletions sdk/spring/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
# Release History

## 4.5.0-beta.2 (Unreleased)

Upgrade Spring Boot dependencies version to 2.7.4 and Spring Cloud dependencies version to 2021.0.4

### Bugs Fixed
- Fix bug: The parameter 'scope' is duplicated. [31191](https://github.com/Azure/azure-sdk-for-java/issues/31191).


## 4.4.0 (2022-09-26)
Upgrade Spring Boot dependencies version to 2.7.3 and Spring Cloud dependencies version to 2021.0.3
Upgrade Spring Boot dependencies version to 2.7.2 and Spring Cloud dependencies version to 2021.0.3.
Expand Down
37 changes: 16 additions & 21 deletions .../com/azure/spring/cloud/autoconfigure/aad/configuration/AadOAuth2ClientConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,13 @@ JwtBearerOAuth2AuthorizedClientProvider azureAdJwtBearerProvider(
ObjectProvider<OAuth2ClientAuthenticationJwkResolver> resolvers) {
JwtBearerOAuth2AuthorizedClientProvider provider = new JwtBearerOAuth2AuthorizedClientProvider();
OAuth2ClientAuthenticationJwkResolver resolver = resolvers.getIfUnique();
AadJwtBearerGrantRequestEntityConverter jwtBearerConverter = new AadJwtBearerGrantRequestEntityConverter();
if (resolver != null) {
jwtBearerConverter.addParametersConverter(new AadJwtClientAuthenticationParametersConverter<>(resolver::resolve));
}
DefaultJwtBearerTokenResponseClient responseClient = new DefaultJwtBearerTokenResponseClient();
responseClient.setRestOperations(createOAuth2AccessTokenResponseClientRestTemplate(restTemplateBuilder));
responseClient.setRequestEntityConverter(jwtBearerConverter);
AadJwtBearerGrantRequestEntityConverter converter = new AadJwtBearerGrantRequestEntityConverter();
if (resolver != null) {
converter.addParametersConverter(new AadJwtClientAuthenticationParametersConverter<>(resolver::resolve));
}
responseClient.setRequestEntityConverter(converter);
provider.setAccessTokenResponseClient(responseClient);
return provider;
}
Expand All @@ -159,47 +159,42 @@ RefreshTokenOAuth2AuthorizedClientProvider azureRefreshTokenProvider(
ObjectProvider<OAuth2ClientAuthenticationJwkResolver> resolvers) {
RefreshTokenOAuth2AuthorizedClientProvider provider = new RefreshTokenOAuth2AuthorizedClientProvider();
OAuth2ClientAuthenticationJwkResolver resolver = resolvers.getIfUnique();
DefaultRefreshTokenTokenResponseClient responseClient = new DefaultRefreshTokenTokenResponseClient();
responseClient.setRestOperations(createOAuth2AccessTokenResponseClientRestTemplate(restTemplateBuilder));
if (resolver != null) {
OAuth2RefreshTokenGrantRequestEntityConverter converter = new OAuth2RefreshTokenGrantRequestEntityConverter();
converter.addParametersConverter(new AadJwtClientAuthenticationParametersConverter<>(resolver::resolve));

DefaultRefreshTokenTokenResponseClient responseClient = new DefaultRefreshTokenTokenResponseClient();
responseClient.setRestOperations(createOAuth2AccessTokenResponseClientRestTemplate(restTemplateBuilder));
responseClient.setRequestEntityConverter(converter);
provider.setAccessTokenResponseClient(responseClient);
}
provider.setAccessTokenResponseClient(responseClient);
return provider;
}

private void passwordGrantBuilderAccessTokenResponseClientCustomizer(
OAuth2AuthorizedClientProviderBuilder.PasswordGrantBuilder builder,
OAuth2ClientAuthenticationJwkResolver resolver) {
DefaultPasswordTokenResponseClient responseClient = new DefaultPasswordTokenResponseClient();
responseClient.setRestOperations(createOAuth2AccessTokenResponseClientRestTemplate(restTemplateBuilder));
if (resolver != null) {
OAuth2PasswordGrantRequestEntityConverter converter = new OAuth2PasswordGrantRequestEntityConverter();
converter.addParametersConverter(new AadJwtClientAuthenticationParametersConverter<>(resolver::resolve));

DefaultPasswordTokenResponseClient client = new DefaultPasswordTokenResponseClient();
client.setRestOperations(createOAuth2AccessTokenResponseClientRestTemplate(restTemplateBuilder));
client.setRequestEntityConverter(converter);

builder.accessTokenResponseClient(client);
responseClient.setRequestEntityConverter(converter);
}
builder.accessTokenResponseClient(responseClient);
}

private void clientCredentialsGrantBuilderAccessTokenResponseClientCustomizer(
OAuth2AuthorizedClientProviderBuilder.ClientCredentialsGrantBuilder builder,
OAuth2ClientAuthenticationJwkResolver resolver) {
DefaultClientCredentialsTokenResponseClient responseClient = new DefaultClientCredentialsTokenResponseClient();
responseClient.setRestOperations(createOAuth2AccessTokenResponseClientRestTemplate(restTemplateBuilder));
if (resolver != null) {
OAuth2ClientCredentialsGrantRequestEntityConverter converter =
new OAuth2ClientCredentialsGrantRequestEntityConverter();
converter.addParametersConverter(new AadJwtClientAuthenticationParametersConverter<>(resolver::resolve));

DefaultClientCredentialsTokenResponseClient client = new DefaultClientCredentialsTokenResponseClient();
client.setRestOperations(createOAuth2AccessTokenResponseClientRestTemplate(restTemplateBuilder));
client.setRequestEntityConverter(converter);

builder.accessTokenResponseClient(client);
responseClient.setRequestEntityConverter(converter);
}
builder.accessTokenResponseClient(responseClient);
}

private AadAzureDelegatedOAuth2AuthorizedClientProvider azureDelegatedOAuth2AuthorizedClientProvider(
Expand Down
24 changes: 5 additions & 19 deletions .../java/com/azure/spring/cloud/autoconfigure/aad/implementation/AadRestTemplateCreator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,11 @@

import org.springframework.boot.web.client.RestTemplateBuilder;
import org.springframework.http.converter.FormHttpMessageConverter;
import org.springframework.http.converter.HttpMessageConverter;
import org.springframework.security.oauth2.client.http.OAuth2ErrorResponseErrorHandler;
import org.springframework.security.oauth2.core.http.converter.OAuth2AccessTokenResponseHttpMessageConverter;
import org.springframework.util.Assert;
import org.springframework.web.client.RestTemplate;

import java.util.List;

/**
* Util class used to create {@link RestTemplate}s for all Azure AD related http requests.
*/
Expand All @@ -27,25 +24,14 @@ public static RestTemplate createRestTemplate(RestTemplateBuilder builder) {
}

public static RestTemplate createOAuth2ErrorResponseHandledRestTemplate(RestTemplateBuilder builder) {
RestTemplate restTemplate = createRestTemplate(builder);
restTemplate.setErrorHandler(new OAuth2ErrorResponseErrorHandler());
return restTemplate;
builder = builder.errorHandler(new OAuth2ErrorResponseErrorHandler());
return createRestTemplate(builder);
}

public static RestTemplate createOAuth2AccessTokenResponseClientRestTemplate(RestTemplateBuilder builder) {
RestTemplate restTemplate = createOAuth2ErrorResponseHandledRestTemplate(builder);
List<HttpMessageConverter<?>> converters = restTemplate.getMessageConverters();
if (notContainsElementOfType(converters, FormHttpMessageConverter.class)) {
converters.add(new FormHttpMessageConverter());
}
if (notContainsElementOfType(converters, OAuth2AccessTokenResponseHttpMessageConverter.class)) {
converters.add(new OAuth2AccessTokenResponseHttpMessageConverter());
}
return restTemplate;
}

private static boolean notContainsElementOfType(List<?> list, Class<?> clazz) {
return list.stream().noneMatch(item -> item.getClass().equals(clazz));
builder = builder.messageConverters(
new FormHttpMessageConverter(), new OAuth2AccessTokenResponseHttpMessageConverter());
return createOAuth2ErrorResponseHandledRestTemplate(builder);
}

}
4 changes: 2 additions & 2 deletions ...java/com/azure/spring/cloud/autoconfigure/aad/implementation/jackson/SerializerUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import org.springframework.security.oauth2.client.OAuth2AuthorizedClient;
import org.springframework.security.oauth2.client.jackson2.OAuth2ClientJackson2Module;

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

public final class SerializerUtils {
Expand Down Expand Up @@ -45,7 +45,7 @@ public static String serializeOAuth2AuthorizedClientMap(Map<String, OAuth2Author

public static Map<String, OAuth2AuthorizedClient> deserializeOAuth2AuthorizedClientMap(String authorizedClientsString) {
if (authorizedClientsString == null) {
return new HashMap<>();
return Collections.emptyMap();
}
Map<String, OAuth2AuthorizedClient> authorizedClients;
try {
Expand Down
23 changes: 7 additions & 16 deletions ...aad/implementation/oauth2/AbstractOAuth2AuthorizationCodeGrantRequestEntityConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,10 @@
package com.azure.spring.cloud.autoconfigure.aad.implementation.oauth2;

import com.azure.spring.cloud.core.implementation.util.AzureSpringIdentifier;
import org.springframework.core.convert.converter.Converter;
import org.springframework.http.HttpHeaders;
import org.springframework.http.RequestEntity;
import org.springframework.security.oauth2.client.endpoint.OAuth2AuthorizationCodeGrantRequest;
import org.springframework.security.oauth2.client.endpoint.OAuth2AuthorizationCodeGrantRequestEntityConverter;
import org.springframework.util.LinkedMultiValueMap;
import org.springframework.util.MultiValueMap;

import java.util.Collections;
Expand All @@ -20,25 +19,17 @@
public abstract class AbstractOAuth2AuthorizationCodeGrantRequestEntityConverter
extends OAuth2AuthorizationCodeGrantRequestEntityConverter {

protected AbstractOAuth2AuthorizationCodeGrantRequestEntityConverter() {
addHeadersConverter((request) -> getHttpHeaders());
addParametersConverter(this::getHttpBody);
}

/**
* Gets the application ID.
*
* @return the application ID
*/
protected abstract String getApplicationId();

@Override
@SuppressWarnings("unchecked")
public RequestEntity<?> convert(OAuth2AuthorizationCodeGrantRequest request) {
addHeadersConverter(headersConverter);
addParametersConverter(parametersConverter);
return super.convert(request);
}

private final Converter<OAuth2AuthorizationCodeGrantRequest, HttpHeaders> headersConverter = (request) -> getHttpHeaders();

private final Converter<OAuth2AuthorizationCodeGrantRequest, MultiValueMap<String, String>> parametersConverter = this::getHttpBody;

/**
* Additional default headers information.
* @return HttpHeaders

@0x006EA1E5 0x006EA1E5 Oct 8, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What do you think about making this public HttpHeaders getHttpHeaders(OAuth2AuthorizationCodeGrantRequest request) {?

This is an abstract class, it seems like it seems like it might be nice to give implementors access to the request parameter

@chenrujun chenrujun Oct 11, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Will do.

Expand All @@ -57,6 +48,6 @@ public HttpHeaders getHttpHeaders() {
* @return MultiValueMap
*/
public MultiValueMap<String, String> getHttpBody(OAuth2AuthorizationCodeGrantRequest request) {
return null;
return new LinkedMultiValueMap<>();

@0x006EA1E5 0x006EA1E5 Oct 8, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

My personal preference would be to return an immutable collection here, if possible. Hence my suggestion of new MultiValueMapAdapter<>(Collections.emptyMap());

Maybe even have a static field to return, so there is no repeated allocation.

@chenrujun chenrujun Oct 11, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point. Will accept this suggestion.

}
}
7 changes: 4 additions & 3 deletions ...nfigure/aad/implementation/oauth2/JacksonHttpSessionOAuth2AuthorizedClientRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;

Expand Down Expand Up @@ -48,10 +49,10 @@ public void saveAuthorizedClient(OAuth2AuthorizedClient authorizedClient, Authen
Assert.notNull(authorizedClient, "authorizedClient cannot be null");
Assert.notNull(request, MSG_REQUEST_CANNOT_BE_NULL);
Assert.notNull(response, "response cannot be null");
Map<String, OAuth2AuthorizedClient> authorizedClients = this.getAuthorizedClients(request);
Map<String, OAuth2AuthorizedClient> authorizedClients = new HashMap<>(this.getAuthorizedClients(request));
authorizedClients.put(authorizedClient.getClientRegistration().getRegistrationId(), authorizedClient);
request.getSession().setAttribute(AUTHORIZED_CLIENTS_ATTR_NAME,
serializeOAuth2AuthorizedClientMap(authorizedClients));
serializeOAuth2AuthorizedClientMap(Collections.unmodifiableMap(authorizedClients)));
}

@Override
Expand All @@ -77,6 +78,6 @@ private Map<String, OAuth2AuthorizedClient> getAuthorizedClients(HttpServletRequ
.map(s -> s.getAttribute(AUTHORIZED_CLIENTS_ATTR_NAME))
.map(Object::toString)
.map(SerializerUtils::deserializeOAuth2AuthorizedClientMap)
.orElse(Collections.emptyMap());

@0x006EA1E5 0x006EA1E5 Oct 8, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't do this, as it means the execution of an additional lambda, whereas the Collections.emptyMap() method just returns the static java.util.Collections#EMPTY_MAP field (so there is no allocation cost saving which would otherwise justify the orElse).

@chenrujun chenrujun Oct 11, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@0x006EA1E5
Thank you for your suggestion.

But I have different understanding:

  1. lambda is just a pointer to a method, it will not need extra cost.
  2. In orElse(doSomething()), doSomething() will always be executed, it's a kind of waste (a method call will cost Stack Frame). Here is code to verify this:
public class Test {

    public static void main(String[] args) {
        System.out.println("Start orElse");
        String orElseResult = Optional.of("string")
                .orElse(doSomething());
        System.out.println("orElseResult = " + orElseResult);

        System.out.println();
        System.out.println("Start orElseGet");
        String orElseGetResult = Optional.of("string")
                .orElseGet(Test::doSomething);
        System.out.println("orElseGetResult = " + orElseGetResult);
    }
    public static String doSomething() {
        System.out.println("doSomething");
        return "doSomething";
    }
}

Execution result:
image

@0x006EA1E5 0x006EA1E5 Oct 11, 2022 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah, yes, you are right, it is a method reference so the lambda doesn't need to be created.

When I mentioned cost saving, I meant different scenario which doesn't apply here:

Optional
    ...
    .orElse(new ExpensiveObject(param));

vs

Optional
    ...
    .orElseGet(() -> new ExpensiveObject(param));

Where the second is obviously preferable, even with the extra lambda.

In any case, I suspect that the performance of .orElse(Collections.emptyMap()) will be indistinguishable from that of .orElseGet(Collections::emptyMap) in the real world, and they would both get optimised and inlined to the same thing if executed enough to make any difference.

Thanks for the response, please ignore my change suggestion :)

@chenrujun chenrujun Oct 14, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Got it. Thank you for your detailed explanation.

.orElseGet(Collections::emptyMap);
}
}
7 changes: 4 additions & 3 deletions ...a/com/azure/spring/cloud/autoconfigure/aad/implementation/AadRestTemplateCreatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,22 +43,23 @@ void testAadRestOperationConfiguration() {

RestTemplate restTemplate = createRestTemplate(builder);
ResponseErrorHandler handler = restTemplate.getErrorHandler();
assertNotEquals(handler.getClass(), OAuth2ErrorResponseErrorHandler.class);
assertNotEquals(OAuth2ErrorResponseErrorHandler.class, handler.getClass());
List<HttpMessageConverter<?>> converters = restTemplate.getMessageConverters();
assertFalse(hasItemOfClass(converters, FormHttpMessageConverter.class));
assertFalse(hasItemOfClass(converters, OAuth2AccessTokenResponseHttpMessageConverter.class));

restTemplate = createOAuth2ErrorResponseHandledRestTemplate(builder);
handler = restTemplate.getErrorHandler();
assertEquals(handler.getClass(), OAuth2ErrorResponseErrorHandler.class);
assertEquals(OAuth2ErrorResponseErrorHandler.class, handler.getClass());
converters = restTemplate.getMessageConverters();
assertFalse(hasItemOfClass(converters, FormHttpMessageConverter.class));
assertFalse(hasItemOfClass(converters, OAuth2AccessTokenResponseHttpMessageConverter.class));

restTemplate = createOAuth2AccessTokenResponseClientRestTemplate(builder);
handler = restTemplate.getErrorHandler();
assertEquals(handler.getClass(), OAuth2ErrorResponseErrorHandler.class);
assertEquals(OAuth2ErrorResponseErrorHandler.class, handler.getClass());
converters = restTemplate.getMessageConverters();
assertEquals(2, converters.size());
assertTrue(hasItemOfClass(converters, FormHttpMessageConverter.class));
assertTrue(hasItemOfClass(converters, OAuth2AccessTokenResponseHttpMessageConverter.class));
});
Expand Down