Fix bugs about connect to Azure AD by proxy by chenrujun · Pull Request #31213 · Azure/azure-sdk-for-java

chenrujun · 2022-09-29T10:00:09Z

Fix error: OAuth2AccessTokenResponseClientRestTemplate contains too many converters. It should only contain 2 converters: FormHttpMessageConverter and OAuth2AccessTokenResponseHttpMessageConverter.
Fix error: Collections.emptyMap().put(..) is not supported.
Fix error: addParametersConverter and addHeadersConverter executed multiple times.
Fix error: RestOperations is not well-configured when jwkResolver is null.

azure-sdk · 2022-09-29T10:07:42Z

API change check

API changes are not detected in this pull request.

1. For RestTemplate used to get access token, only keep 2 converters: FormHttpMessageConverter and OAuth2AccessTokenResponseHttpMessageConverter. Because keep more converters may cause the 2 converters not been used. 2. Return new HashMap() instead of Collections.emptyMap(). Because the returned value needs to add new values, but Collections.emptyMap() does not support add new value.

saragluna · 2022-09-30T06:45:49Z

        List<HttpMessageConverter<?>> converters = restTemplate.getMessageConverters();
-        if (notContainsElementOfType(converters, FormHttpMessageConverter.class)) {
-            converters.add(new FormHttpMessageConverter());
-        }
-        if (notContainsElementOfType(converters, OAuth2AccessTokenResponseHttpMessageConverter.class)) {
-            converters.add(new OAuth2AccessTokenResponseHttpMessageConverter());
-        }
+        converters.clear();
+        converters.add(new FormHttpMessageConverter());
+        converters.add(new OAuth2AccessTokenResponseHttpMessageConverter());


Why not calling this method public RestTemplateBuilder messageConverters(HttpMessageConverter<?>... messageConverters)?

0x006EA1E5 · 2022-10-06T13:49:19Z

This is not working for me, and indeed does not appear to address the root cause.

The problem as far as I can see is that AbstractOAuth2AuthorizationCodeGrantRequestEntityConverter#convert adds the parametersConverter (AadOAuth2AuthorizationCodeGrantRequestEntityConverter#getHttpBody) each time it is called, which will be once per request.

Looking at AbstractOAuth2AuthorizationGrantRequestEntityConverter#addParametersConverter in spring-security-oauth2-client 5.7.3, this creates a new composed lambda which first calls currentParametersConverter, and then calls the provided parametersConverter (AadOAuth2AuthorizationCodeGrantRequestEntityConverter#getHttpBody); the new lambda is set as the new value for AbstractOAuth2AuthorizationGrantRequestEntityConverter#parametersConverter.

The initial call this works correctly (AbstractOAuth2AuthorizationCodeGrantRequestEntityConverter#convert adds the implementing class's AadOAuth2AuthorizationCodeGrantRequestEntityConverter#getHttpBody), which means we end up with AbstractOAuth2AuthorizationGrantRequestEntityConverter#parametersConverter being a lambda composed of the original default parametersConverter plus the provided AadOAuth2AuthorizationCodeGrantRequestEntityConverter#getHttpBody.

However, on any subsequent call, we call AbstractOAuth2AuthorizationGrantRequestEntityConverter#addParametersConverter again, meaning we update AbstractOAuth2AuthorizationGrantRequestEntityConverter#parametersConverter to be a composition of the previous correctly composed parametersConverter with another call to AadOAuth2AuthorizationCodeGrantRequestEntityConverter#getHttpBody.

Hence, on the second call, we do AadOAuth2AuthorizationCodeGrantRequestEntityConverter#getHttpBody twice, which results on the observed error: The request is not properly formatted. The parameter 'scope' is duplicated.

Indeed, on each additional request, we call AbstractOAuth2AuthorizationCodeGrantRequestEntityConverter#convert again, and add yet another AadOAuth2AuthorizationCodeGrantRequestEntityConverter#getHttpBody, meaning that AbstractOAuth2AuthorizationGrantRequestEntityConverter#parametersConverter grows, and is a memory leak that would eventually blow the stack, and that each additional request will contain an additional identical scope.

n.b., this is in part due to the fact that parametersConverter uses a MultiValueMap, so adding the same value multiple times will not overwrite / update, but will add.

It seems a correct solution would be instead to call AbstractOAuth2AuthorizationGrantRequestEntityConverter#addParametersConverter just once, in a constructor.

The same goes for addHeadersConverter(...).

0x006EA1E5 · 2022-10-06T14:55:36Z

PR to fix above #31290

check-enforcer · 2022-10-08T03:09:30Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

…ly be executed once.

chenrujun · 2022-10-08T08:22:02Z

Hi, @saragluna , @0x006EA1E5 please help to review this PR when you have time.

…n if jwkResolver is null.

0x006EA1E5 · 2022-10-08T13:28:13Z

                       .map(s -> s.getAttribute(AUTHORIZED_CLIENTS_ATTR_NAME))
                       .map(Object::toString)
                       .map(SerializerUtils::deserializeOAuth2AuthorizedClientMap)
-                       .orElse(Collections.emptyMap());


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).

@0x006EA1E5
Thank you for your suggestion.

But I have different understanding:

lambda is just a pointer to a method, it will not need extra cost.

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:

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 :)

Got it. Thank you for your detailed explanation.

0x006EA1E5 · 2022-10-08T13:35:20Z

     */
    public MultiValueMap<String, String> getHttpBody(OAuth2AuthorizationCodeGrantRequest request) {
-        return null;
+        return new LinkedMultiValueMap<>();


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.

Good point. Will accept this suggestion.

0x006EA1E5 · 2022-10-08T13:46:39Z

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


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

0x006EA1E5

Looks good to me

chenrujun · 2022-10-14T08:40:23Z

Closing this PR, fix one bug by one PR:

chenrujun requested review from Netyyyy, backwind1233, fangjian0423, hui1110, moarychan, saragluna, stliu and yiliuTo as code owners September 29, 2022 10:00

ghost added the azure-spring All azure-spring related issues label Sep 29, 2022

chenrujun self-assigned this Sep 29, 2022

chenrujun added bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. labels Sep 29, 2022

chenrujun added this to the 2022-11 milestone Sep 29, 2022

This was referenced Sep 29, 2022

[BUG] v.4.4.0-beta.1: Put a value into Collections.emptyMap - JacksonHttpSessionOAuth2AuthorizedClientRepository #31190

Closed

[BUG] The parameter 'scope' is duplicated #31191

Closed

chenrujun mentioned this pull request Sep 29, 2022

[BUG] Spring Boot Starter for Azure Active Directory could not reflect timeout settings of application.properties #31052

Closed

3 tasks

chenrujun force-pushed the fix-issue-31191-duplicated-scopes branch from abfa967 to 5470386 Compare September 29, 2022 10:16

saragluna requested changes Sep 30, 2022

View reviewed changes

damir78 mentioned this pull request Oct 4, 2022

[BUG] NPE when execute Collections.emptyMap().put() #31256

Closed

Fix version error in CHANGELOG.md.

8836ab2

rujche added 3 commits October 8, 2022 11:35

Simplify AadRestTemplateCreator.

88b1e04

Update the logic of unmodifiableMap, make it more robust.

5af9bed

Fix bug: "addHeadersConverter" and "addParametersConverter" should on…

a647731

…ly be executed once.

chenrujun force-pushed the fix-issue-31191-duplicated-scopes branch from 1fda10b to a647731 Compare October 8, 2022 06:41

Merge branch 'main' into fix-issue-31191-duplicated-scopes

057d4ab

chenrujun mentioned this pull request Oct 8, 2022

Fixes 31191 Duplicated Scopes #31290

Closed

6 tasks

Fix error by adding "builder = builder.xxx"

d76c78a

Fix Azure#31218. Set restOperations even if jwkResolver is null.

9f7fbf9

chenrujun mentioned this pull request Oct 8, 2022

[BUG] RestOperations is not well configured when jwkResolver is null #31218

Closed

chenrujun changed the title ~~Fix issue 31191: duplicated scopes.~~ Fix bugs about connect to Azure AD by proxy Oct 8, 2022

Fix error: AadJwtBearerGrantRequestEntityConverter should be used eve…

0f5851f

…n if jwkResolver is null.

0x006EA1E5 reviewed Oct 8, 2022

View reviewed changes

0x006EA1E5 approved these changes Oct 11, 2022

View reviewed changes

chenrujun closed this Oct 14, 2022

chenrujun deleted the fix-issue-31191-duplicated-scopes branch October 14, 2022 08:40

chenrujun mentioned this pull request Oct 17, 2022

Issue 31482 rest template used to get access token should only contains 2 converters #31489

Merged

Uh oh!

Conversation

chenrujun commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-sdk commented Sep 29, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x006EA1E5 commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x006EA1E5 commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

check-enforcer Bot commented Oct 8, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

chenrujun commented Oct 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x006EA1E5 Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x006EA1E5 left a comment

Choose a reason for hiding this comment

Uh oh!

chenrujun commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chenrujun commented Sep 29, 2022 •

edited

Loading

0x006EA1E5 commented Oct 6, 2022 •

edited

Loading

0x006EA1E5 commented Oct 6, 2022 •

edited

Loading

0x006EA1E5 Oct 11, 2022 •

edited

Loading