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

Support URIs userInfo data in REST Client Reactive #30344

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jan 12, 2023

Fixes: #30289

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 12, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

I'm missing more context here :/ I don't follow why setting the userInfo leads to adding the Authorization header.
Reading the issue and also checking how the Rest Client classic works, the expectation is that the userInfo should be appended to the URI:

MyClient client = RestClientBuilder.newBuilder()
     .baseUri(UriBuilder.fromUri(baseUri).userInfo("foo:bar").build())
     .build(MyClient.class);

In Resteasy Rest Client, the resulting request is: http://foo:bar@localhost:8081/endpoint-userinfo. And checking the resource, I don't see any Authorization headers:

@GET
    @Path("/endpoint-userinfo")
    public String callUsingUserInfo(@Context UriInfo uriInfo, @Context HttpHeaders headers) {
        // uriInfo.getRequestUri().getUserInfo() is null ??
        // headers does not contain Authorization headers
    }

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2023

@Sgitario see this.

Essentially when user:pass is added to an HTTP URL, most clients support converting these into basic authorization info.

On our side, we should not have to force the user to add .userInfo(..) - moreover that call never comes into play when the URL is provided as a configuration property and the interface is automatically registered.

Does that answer your questions?

@Sgitario
Copy link
Contributor

@Sgitario see this.

Essentially when user:pass is added to an HTTP URL, most clients support converting these into basic authorization info.

On our side, we should not have to force the user to add .userInfo(..) - moreover that call never comes into play when the URL is provided as a configuration property and the interface is automatically registered.

Does that answer your questions?

Mmm, I see this is a behavior for most clients, but should it not be done on the server side? I mean what I think REST Client is doing is appending the user information to the URI and sending it to the server which will extract this token to generate the authentication. It's now up to the server to accept this very insecure convention. But I think REST Client and REST Client Reactive extensions should behave the same.

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2023

I didn't see any information anywhere mentioning that the server should do the same thing, all information I read mentioned that the user info in the URL is turned into a basic authorization header - of course this is not secure, but that's not our problem (and can be partly mitigated when HTTPS is used) :)

@Sgitario
Copy link
Contributor

I didn't see any information anywhere mentioning that the server should do the same thing, all information I read mentioned that the user info in the URL is turned into a basic authorization header - of course this is not secure, but that's not our problem (and can be partly mitigated when HTTPS is used) :)

If you are confident about this thing, ok for me!

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2023

If you are confident about this thing, ok for me!

One can never be 100% sure, but I am confident it's the right way to go :)

@geoand geoand merged commit ca34a5a into quarkusio:main Jan 13, 2023
@quarkus-bot quarkus-bot bot added this to the 2.17 - main milestone Jan 13, 2023
@geoand geoand deleted the #30289 branch January 13, 2023 06:33
if (parts.length != 2) {
return;
}
ClientRequestHeaders specHeaders = requestSpec.headers;
Copy link

Choose a reason for hiding this comment

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

I have tested manually using POSTman and this is equivalent to put the user info (USER:PASSWORD) in the URL ✅

@Cs4r
Copy link

Cs4r commented Jan 13, 2023

@geoand In which build will the changes be available?

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2023

It should be part of 2.16.0.Final

@gsmet gsmet modified the milestones: 2.17 - main, 2.16.0.Final Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

quarkus-rest-client-reactive-jackson removing credentials from URIs and URLs
4 participants