Skip to content
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.

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

Do not replace a request factory when creating a new TestRestTemplate with withBasicAuth #15982

Closed
wants to merge 1 commit into from

Conversation

nosan
Copy link
Contributor

@nosan nosan commented Feb 18, 2019

See gh-15443

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 18, 2019
@mbhave
Copy link
Contributor

mbhave commented Feb 25, 2019

@nosan Thanks for the PR but I don't think this is how we intended to fix it. We want to avoid using the same reuqestFactory instance when using withBasicAuth. For more details on why, take a look at the commit message here.

I think what we'd want to do instead is use the supplier to create a new instance of the custom requestFactory class and if that fails, fallback to the current behavior. @bclozel WDYT?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Feb 28, 2019
@mbhave mbhave added the for: team-attention An issue we'd like other members of the team to review label Mar 13, 2019
@bclozel
Copy link
Member

bclozel commented Mar 13, 2019

I agree with your analysis @mbhave (and sorry about the late reply, that notification got lost in my inbox).

@bclozel bclozel removed the for: team-attention An issue we'd like other members of the team to review label Mar 13, 2019
@mbhave
Copy link
Contributor

mbhave commented Mar 13, 2019

Thanks @bclozel.

@nosan Do you want to update the PR to do what I've mentioned above?

@nosan
Copy link
Contributor Author

nosan commented Mar 18, 2019

@mbhave
Sorry for the long delay, sure, I can update the PR. I am not sure, that I understood you correctly, hence, just to make sure, could you please provide more information about your approach?
Thanks in advance.

@bclozel
Copy link
Member

bclozel commented Mar 18, 2019

@nosan in the current PR, withBasicAuth is retrieving the current request factory instance from the embedded RestTemplate and reusing it. This line doesn't really change the behavior we're trying to fix.

Several HTTP client libraries are caching the authentication information at the connection pooling level. This means that reusing the same request factory will result in sending the Cookie request header even if the authentication was not meant to be included.

Creating a brand new instance of the configured request factory class (or as Madhura mentioned, falling back to the current behavior in case of problems) should avoid those issues. You can look at the ClientHttpRequestFactorySupplier to check how factories are instantiated.

@nosan
Copy link
Contributor Author

nosan commented Mar 18, 2019

@mbhave @bclozel Thanks,
I have updated the PR, please let me know if everything is ok or not.

@mbhave mbhave added type: bug A general bug and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 18, 2019
@mbhave mbhave added this to the 2.1.x milestone Mar 18, 2019
@mbhave mbhave removed the status: waiting-for-triage An issue we've not yet triaged label Mar 18, 2019
mbhave pushed a commit that referenced this pull request Mar 19, 2019
This commit updates the behavior of withBasicAuth on TestRestTemplate
by trying to use the same request factory type as the underlying restTemplate.
If creation of a new instance of the configured request factory class fails,
it falls back to the `ClientHttpRequestFactorySupplier`.

See gh-15982
mbhave added a commit that referenced this pull request Mar 19, 2019
* pr/15982:
  Polish "Fix request factory used with withBasicAuth"
  Fix request factory used with TestRestTemplate withBasicAuth
@mbhave mbhave closed this in 7ea8770 Mar 19, 2019
@mbhave
Copy link
Contributor

mbhave commented Mar 19, 2019

Thanks again, @nosan. This is now on master along with this additional test.

@mbhave mbhave modified the milestones: 2.1.x, 2.1.4 Mar 19, 2019
@nosan
Copy link
Contributor Author

nosan commented Mar 19, 2019

@mbhave Thanks!

@nosan nosan deleted the gh-15443 branch March 19, 2019 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants