[ELY-2813] Do not decode URI for processing

[michpetrov]

Issue: ELY-2813

[fjuma]

@michpetrov Thanks for working on this! This looks related to #2186 as well. As commented there, I found this related change that was made for the Keycloak adapter:

keycloak/keycloak#10895

We should make sure that we've incorporated anything else related from the Keycloak PR as well (if you haven't already). For example, the Keycloak adatper supports a couple different cases depending on the Elytron Web version in use and also makes some related changes in other files. We should verify that we can now safely ignore the case where the existing code was needed and also check that we've made the related updates in all places.

Would also be good to add tests.

[michpetrov]

@fjuma that seems to be essentially the same problem, so I could close this PR.

I don't quite understand the keycloak PR though, it looks like it's copying the bad behavior from here, i.e. decoding the query and forwarding it as the original and if that's required for elytron-web to function then I'd say there's a problem in elytron-web. But I'm just following a reproducer, I don't know if and when elytron-web comes into play or if that's a separate issue.

[fjuma]

The original code is needed by Keycloak when using an older version of Elytron Web. The new code is needed by Keycloak when using a newer version of Elytron Web because the newer version updates the way decoding is done.

I can look closer when I have some time to check if there is anything else from the Keycloak PR that is needed.

[michpetrov]

So if I'm understanding this right - older versions of elytron-web (anything before 1.9.2 plus 1.10.0 on its own) unintentionally encode the URI when forwarding it, so if Keycloak detects an old version it will decode the URI to fix this. All I'm seeing in the PR are changes to the getUri and getQueryParamValue methods and everything else is just for determining if the workaround should be applied. So if an old version is detected it behaves the way OidcHttpFacade behaves right now, which has been unnecessary (and undesirable) since the elytron-web fix.

Do we want the Keycloak workaround added to Elytron or? I'm seeing elytron-web is already on 4.x so are the old versions still supported?

[fjuma]

We don't need to worry about the older Elytron Web versions.

I haven't had time to look closely but the handling of the query parameters in the Keycloak code for the new version of Elytron Web looks a bit different from the Elytron code so we need to make sure that part is correct and make sure that we have appropriate tests for this.

[michpetrov]

There are some differences (in the UriUtils class). But I guess I'm missing why we're concerned about it, looking at Elytron the method is called to retrieve known parameters (so there isn't a need to make the method handle parameters that are just "key" instead of "key=value" if we know we do not process any such parameters). But if the concern is that Elytron might send unexpectedly encoded/decoded URI to Keycloak or vice versa, this method doesn't modify URI so it wouldn't be the problem.

[michpetrov]

[~fjuma], I'd like to expedite this. I can port the method over but like I said I don't think it makes a difference, it doesn't affect the URI that's being exchanged between Elytron and Keycloak.

[rsearls]

This look OK to me.

[michpetrov]

Right, this isn't JIRA :) @fjuma, please see my comment above

[darranl]

One small comment, if this is for a micro release this should be submitted against the relevant maintenance branch not 2.x which is the upstream for WildFly.

[ivassile]

@michpetrov Like @darranl suggested, can you change the PR to target 2.2.x branch (JBEAP-28033)? Thanks!

[michpetrov]

@ivassile well I tried but I don't think Git is built for that, I've sent a new PR instead - #2236