Skip to content

Fix #31191: duplicated "scope" parameter #31419

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

Merged
chenrujun merged 5 commits into from
Oct 15, 2022
Merged

Fix #31191: duplicated "scope" parameter #31419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
76565ce
Fix #31191. duplicated "scope" parameter.
rujche Oct 12, 2022
5baeae8
Add a new item in CHANGELOG.md.
rujche Oct 12, 2022
46e4fd4
Delete unnecessary method: Override and did nothing except call "sup…
rujche Oct 14, 2022
fda0a41
1. Change "getHttpHeaders" and "getHttpBody" to "protected" method.
rujche Oct 14, 2022
d7e2d45
Update the logic of testing http header.
rujche Oct 14, 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
1 change: 1 addition & 0 deletions sdk/spring/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Upgrade Spring Boot dependencies version to 2.7.4 and Spring Cloud dependencies

#### Bugs Fixed
- Fix bug: Put a value into Collections.emptyMap(). [#31190](https://github.com/Azure/azure-sdk-for-java/issues/31190).
- Fix bug: Duplicated "scope" parameter. [#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
Expand Down
29 changes: 12 additions & 17 deletions ...aad/implementation/oauth2/AbstractOAuth2AuthorizationCodeGrantRequestEntityConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@
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.MultiValueMap;
import org.springframework.util.MultiValueMapAdapter;

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

private static final MultiValueMap<String, String> EMPTY_MULTI_VALUE_MAP =
new MultiValueMapAdapter<>(Collections.emptyMap());

protected AbstractOAuth2AuthorizationCodeGrantRequestEntityConverter() {
addHeadersConverter(this::getHttpHeaders);
addParametersConverter(this::getHttpBody);
}
Comment on lines +25 to +28

@moarychan moarychan Oct 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should explicitly use this default constructor in the subclasses
https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/spring/spring-cloud-azure-autoconfigure/src/main/java/com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2AuthorizationCodeGrantRequestEntityConverter.java#L34-L36

@chenrujun chenrujun Oct 17, 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.

Not necessary.
Refs:
https://stackoverflow.com/questions/2054022/is-it-unnecessary-to-put-super-in-constructor

Test:

image


/**
* 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
*/
public HttpHeaders getHttpHeaders() {
protected HttpHeaders getHttpHeaders(OAuth2AuthorizationCodeGrantRequest request) {
HttpHeaders httpHeaders = new HttpHeaders();
httpHeaders.put("x-client-SKU", Collections.singletonList(getApplicationId()));
httpHeaders.put("x-client-VER", Collections.singletonList(AzureSpringIdentifier.VERSION));
Expand All @@ -56,7 +51,7 @@ public HttpHeaders getHttpHeaders() {
* @param request OAuth2AuthorizationCodeGrantRequest
* @return MultiValueMap
*/
public MultiValueMap<String, String> getHttpBody(OAuth2AuthorizationCodeGrantRequest request) {
return null;
protected MultiValueMap<String, String> getHttpBody(OAuth2AuthorizationCodeGrantRequest request) {
return EMPTY_MULTI_VALUE_MAP;
}
}
42 changes: 28 additions & 14 deletions ...aad/implementation/webapp/AadOAuth2AuthorizationCodeGrantRequestEntityConverterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import com.azure.spring.cloud.autoconfigure.aad.AadClientRegistrationRepository;
import com.azure.spring.cloud.autoconfigure.aad.implementation.WebApplicationContextRunnerUtils;
import org.hamcrest.Matcher;
import com.azure.spring.cloud.core.implementation.util.AzureSpringIdentifier;
import org.junit.jupiter.api.Test;
import org.springframework.boot.test.context.runner.WebApplicationContextRunner;
import org.springframework.http.HttpEntity;
Expand All @@ -19,12 +19,12 @@
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationResponse;
import org.springframework.util.MultiValueMap;

import java.util.Collections;
import java.util.Optional;

import static com.azure.spring.cloud.autoconfigure.aad.AadClientRegistrationRepository.AZURE_CLIENT_REGISTRATION_ID;
import static org.hamcrest.CoreMatchers.hasItems;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

class AadOAuth2AuthorizationCodeGrantRequestEntityConverterTests {

Expand Down Expand Up @@ -62,26 +62,41 @@ void addScopeForAuthorizationCodeClient() {
}

@Test
@SuppressWarnings("unchecked")
void onlyAddScopeOnceEvenConvertMethodExecutedMultipleTimes() {
getContextRunner().run(context -> {
AadClientRegistrationRepository repository =
(AadClientRegistrationRepository) context.getBean(ClientRegistrationRepository.class);
AadOAuth2AuthorizationCodeGrantRequestEntityConverter converter =
new AadOAuth2AuthorizationCodeGrantRequestEntityConverter(repository.getAzureClientAccessTokenScopes());
ClientRegistration azure = repository.findByRegistrationId(AZURE_CLIENT_REGISTRATION_ID);
OAuth2AuthorizationCodeGrantRequest request = createCodeGrantRequest(azure);
// Convert method execute 2 times
converter.convert(request);
RequestEntity<?> entity = converter.convert(request);
MultiValueMap<String, String> map = WebApplicationContextRunnerUtils.toMultiValueMap(entity);
assertEquals(1, map.get("scope").size());
});
}

@Test
void addHeadersForAzureClient() {
getContextRunner().run(context -> {
AadClientRegistrationRepository repository =
(AadClientRegistrationRepository) context.getBean(ClientRegistrationRepository.class);
ClientRegistration azure = repository.findByRegistrationId(AZURE_CLIENT_REGISTRATION_ID);
HttpHeaders httpHeaders = convertedHeaderOf(repository, createCodeGrantRequest(azure));
assertThat(httpHeaders.entrySet(), (Matcher) hasItems(expectedHeaders(repository)));
testHttpHeaders(httpHeaders);
});
}

@Test
@SuppressWarnings("unchecked")
void addHeadersForAuthorizationCodeClient() {
getContextRunner().run(context -> {
AadClientRegistrationRepository repository =
(AadClientRegistrationRepository) context.getBean(ClientRegistrationRepository.class);
ClientRegistration arm = repository.findByRegistrationId("arm");
HttpHeaders httpHeaders = convertedHeaderOf(repository, createCodeGrantRequest(arm));
assertThat(httpHeaders.entrySet(), (Matcher) hasItems(expectedHeaders(repository)));
testHttpHeaders(httpHeaders);
});
}

Expand All @@ -95,13 +110,12 @@ private HttpHeaders convertedHeaderOf(AadClientRegistrationRepository repository
.orElse(null);
}

private Object[] expectedHeaders(AadClientRegistrationRepository repository) {
return new AadOAuth2AuthorizationCodeGrantRequestEntityConverter(repository.getAzureClientAccessTokenScopes())
.getHttpHeaders()
.entrySet()
.stream()
.filter(entry -> !entry.getKey().equals("client-request-id"))
.toArray();
private void testHttpHeaders(HttpHeaders headers) {
assertTrue(headers.containsKey("x-client-SKU"));
assertEquals(Collections.singletonList(AzureSpringIdentifier.AZURE_SPRING_AAD), headers.get("x-client-SKU"));
assertTrue(headers.containsKey("x-client-VER"));
assertEquals(Collections.singletonList(AzureSpringIdentifier.VERSION), headers.get("x-client-VER"));
assertTrue(headers.containsKey("client-request-id"));
}

private MultiValueMap<String, String> convertedBodyOf(AadClientRegistrationRepository repository,
Expand Down