Made adapters compatible with KC23 "iss" parameter change by vickeybrown · Pull Request #23583 · keycloak/keycloak

vickeybrown · 2023-09-28T15:11:17Z

cypress · 2023-09-28T15:36:47Z

Passing run #9194 ↗︎

0	527	48	0	0

Details:

Merge `da54be1` into `31396ea`...
Project: Keycloak Admin UI	Commit: `5e29c6ce93 ℹ️`
Status: Passed	Duration: 17:39 💡
Started: Oct 3, 2023 1:02 PM	Ended: Oct 3, 2023 1:19 PM

Review all test suite changes for PR #23583 ↗︎

ghost

Unreported flaky test detected, please review

ghost · 2023-09-28T16:43:33Z

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.admin.concurrency.ConcurrencyTest#createRemoveClient

Keycloak CI - Base IT (1)

java.lang.RuntimeException: There were failures in threads. Failures count: 1
	at org.keycloak.testsuite.admin.concurrency.AbstractConcurrencyTest.run(AbstractConcurrencyTest.java:122)
	at org.keycloak.testsuite.admin.concurrency.AbstractConcurrencyTest.run(AbstractConcurrencyTest.java:63)
	at org.keycloak.testsuite.admin.concurrency.AbstractConcurrencyTest.run(AbstractConcurrencyTest.java:59)
	at org.keycloak.testsuite.admin.concurrency.ConcurrencyTest.concurrentTest(ConcurrencyTest.java:60)
...

Report flaky test

org.keycloak.testsuite.admin.concurrency.ConcurrencyTest#createClient

Keycloak CI - Base IT (1)

java.lang.RuntimeException: There were failures in threads. Failures count: 1
	at org.keycloak.testsuite.admin.concurrency.AbstractConcurrencyTest.run(AbstractConcurrencyTest.java:122)
	at org.keycloak.testsuite.admin.concurrency.AbstractConcurrencyTest.run(AbstractConcurrencyTest.java:63)
	at org.keycloak.testsuite.admin.concurrency.AbstractConcurrencyTest.run(AbstractConcurrencyTest.java:59)
	at org.keycloak.testsuite.admin.concurrency.ConcurrencyTest.concurrentTest(ConcurrencyTest.java:60)
...

Report flaky test

org.keycloak.testsuite.forms.VerifyProfileTest#testAttributeRequiredForDefaultScope

Keycloak CI - Forms IT (chrome)

java.lang.IllegalArgumentException: No enum constant org.keycloak.testsuite.pages.AppPage.RequestType.
	at java.base/java.lang.Enum.valueOf(Enum.java:273)
	at org.keycloak.testsuite.pages.AppPage$RequestType.valueOf(AppPage.java:56)
	at org.keycloak.testsuite.pages.AppPage.getRequestType(AppPage.java:49)
	at jdk.internal.reflect.GeneratedMethodAccessor552.invoke(Unknown Source)
...

Report flaky test

pedroigor · 2023-09-28T21:19:42Z

Could you please add a link to the issue/PR we are backporting from upstream?

I'm also not following these changes because the change you are referring to is actually adding the iss parameter to authorization responses. The opposite you are proposing here.

What we actually need to implement is https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-23#section-4.4.2.1.

I'm also not sure about changes in Java adapters as they are deprecated and they should work fine with the latest Keycloak releases. For this particular case, there is a compatibility mode setting at the client settings that allows you to remove this parameter from responses if you have issues with it.

vickeybrown · 2023-09-29T13:11:36Z

Hi @pedroigor, I'll update the description with the Issue and a better description of what I'm trying to do. I'm only wanting to backport the ability for the adapters to handle receiving the "iss" parameter to allow for an easier transition to this change in KC23.

jsorah · 2023-10-02T14:26:20Z

@pedroigor - To add more background here - we're looking to make the KC22 adapters "forward compatible" with the change coming in KC23, we're not looking to backport entire functionality of RFC 9027 to KC22.

This is to solve some logistical issues with making a shift from prior Keycloak versions to something that will work with KC23 without significant problems. If one were to use even the KC22 versions of KeycloakJS or the Java based Keycloak adapters (which I do understand they're deprecated) with nightly / KC23, those clients will not function correctly. The adapter code for KC22 and prior appends the 'iss' param back to the redirect URI used in the code to token exchange, hence it will not match the original value supplied during the initial auth request. The KeycloakJS change is a little more nuance and only breaks in some instances, but still has an impact.

So that means every Keycloak administrator must ensure their clients are either

using the KC23 version of adapters or something else before they upgrade
have the compatibility flag enabled for impacted clients

This is certainly no small chore in a largely integrated system with hundreds of OIDC clients.

I think additionally, we are uniquely building against nightly, so we don't actually have a new, compatible version of the adapters to point folks to for now, until KC23 is released, and that places time constraints on us.

Backporting this "forward compatible" change would eliminate the time constraint we have and could ease transition for folks from KC22 -> KC23 as they could plan to make any necessary updates sooner, prior to KC23 being released.

pedroigor

Sure, I understand that. But the changes you are referring to are actually a server-side thing whereas Keycloak will actually include the iss in responses. Not the opposite.

At least in Java Adapters, the changes you are introducing are basically about the parameters the client is sending to the server. What I'm a bit confused.

jsorah · 2023-10-02T14:56:26Z

Sure, I understand that. But the changes you are referring to are actually a server-side thing whereas Keycloak will actually include the iss in responses. Not the opposite.

At least in Java Adapters, the changes you are introducing are basically about the parameters the client is sending to the server. What I'm a bit confused.

Yeah I think its a little weird because the adapters seem to rely on the authorization server to tell it what redirect URI was used based on the auth response, instead of maintaining that state itself. It then uses that same redirect URI from the auth server to use in code to token. This is where the trouble is with the new iss param and why it is added to those methods to strip that query param from the redirect URI.

So its a little something like

Initial auth request to http://authserver -> use redirect URI http://someclient/somepath
Auth response -> redirect URI -> http://someclient/somepath?code=SplxlOBeZQQYbYS6WxSbIA&state=xyz&iss=http://authserver
Adapters remove extraneous OAuth params from redirect URI sent back from authz server
Adapters use modified redirect URI in code to token exchange

mposolda

Thanks for the PR. Looks good to me and agree that "forward compatibility" of adapters with server changes in Keycloak 23 is beneficial.

@pedroigor Do you have more comments to this or can we merge?

ghost

Unreported flaky test detected, please review

ghost · 2023-10-03T14:56:43Z

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.forms.ResetCredentialsAlternativeFlowsTest#testResetCredentialsFlowWithUsernameProvidedFromBrowserFlow

Keycloak CI - Forms IT (chrome)

java.lang.IllegalArgumentException: No enum constant org.keycloak.testsuite.pages.AppPage.RequestType.
	at java.base/java.lang.Enum.valueOf(Enum.java:273)
	at org.keycloak.testsuite.pages.AppPage$RequestType.valueOf(AppPage.java:56)
	at org.keycloak.testsuite.pages.AppPage.getRequestType(AppPage.java:49)
	at jdk.internal.reflect.GeneratedMethodAccessor552.invoke(Unknown Source)
...

Report flaky test

ghost

Unreported flaky test detected, please review

ghost · 2023-10-03T17:43:21Z

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.forms.ResetCredentialsAlternativeFlowsTest#testResetCredentialsFlowWithUsernameProvidedFromBrowserFlow

Keycloak CI - Forms IT (chrome)

java.lang.IllegalArgumentException: No enum constant org.keycloak.testsuite.pages.AppPage.RequestType.
	at java.base/java.lang.Enum.valueOf(Enum.java:273)
	at org.keycloak.testsuite.pages.AppPage$RequestType.valueOf(AppPage.java:56)
	at org.keycloak.testsuite.pages.AppPage.getRequestType(AppPage.java:49)
	at jdk.internal.reflect.GeneratedMethodAccessor552.invoke(Unknown Source)
...

Report flaky test

pedroigor

I see now. Sorry for the noise.

pedroigor · 2023-10-05T19:42:38Z

@ssilvert Could you please review the changes to JS adapter?

ssilvert · 2023-10-06T01:34:20Z

LGTM, but @jonkoops knows the adapter better than I. I'll let him weigh in.

jonkoops

Changes to the client look fine to me.

cypress · 2023-10-06T15:13:09Z

1 flaky test on run #9270 ↗︎

0	527	48	0	1

Details:

Made adapters compatible with KC23 "iss" parameter change (#23583)
Project: Keycloak Admin UI	Commit: `8535452a51`
Status: Passed	Duration: 18:58 💡
Started: Oct 6, 2023 2:54 PM	Ended: Oct 6, 2023 3:13 PM

cypress/e2e/realm_settings_events_test.spec.ts • 1 flaky test • chrome

View Output Video

Test		Artifacts
Realm settings events tab tests > Should remove event from event listener		`Output` `Screenshots`

Review all test suite changes for PR #23583 ↗︎

Made adapters compatible with KC23 "iss" parameter change

bd6347d

vickeybrown requested review from a team as code owners September 28, 2023 15:11

ghost added team/ui labels Sep 28, 2023

ghost added the flaky-test label Sep 28, 2023

ghost reviewed Sep 28, 2023

View reviewed changes

pedroigor reviewed Oct 2, 2023

View reviewed changes

mposolda approved these changes Oct 3, 2023

View reviewed changes

mposolda self-assigned this Oct 3, 2023

ssilvert added the team/core label Oct 3, 2023

ssilvert assigned jonkoops Oct 3, 2023

Merge branch 'release/22.0' into backport-20584

da54be1

ghost reviewed Oct 3, 2023

View reviewed changes

ghost mentioned this pull request Oct 3, 2023

Flaky test: org.keycloak.testsuite.forms.BrowserFlowTest#testAlternativeNonInteractiveExecutorInSubflow #22644

Closed

ghost reviewed Oct 3, 2023

View reviewed changes

pedroigor approved these changes Oct 5, 2023

View reviewed changes

ssilvert requested a review from jonkoops October 6, 2023 01:33

jonkoops approved these changes Oct 6, 2023

View reviewed changes

jonkoops mentioned this pull request Oct 6, 2023

Made adapters compatible with KC23 "iss" parameter change #23794

Closed

jonkoops linked an issue Oct 6, 2023 that may be closed by this pull request

Made adapters compatible with KC23 "iss" parameter change #23794

Closed

jonkoops merged commit 8535452 into keycloak:release/22.0 Oct 6, 2023

stianst mentioned this pull request Nov 14, 2023

eap docs #24733

Closed

stianst mentioned this pull request May 6, 2024

Ignore github links when checking links #29291

Merged

Uh oh!

Conversation

vickeybrown commented Sep 28, 2023 • edited by jonkoops Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cypress Bot commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Passing run #9194 ↗︎

Review all test suite changes for PR #23583 ↗︎

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 28, 2023

Unreported flaky test detected

org.keycloak.testsuite.admin.concurrency.ConcurrencyTest#createRemoveClient

org.keycloak.testsuite.admin.concurrency.ConcurrencyTest#createClient

org.keycloak.testsuite.forms.VerifyProfileTest#testAttributeRequiredForDefaultScope

Uh oh!

pedroigor commented Sep 28, 2023

Uh oh!

vickeybrown commented Sep 29, 2023

Uh oh!

jsorah commented Oct 2, 2023

Uh oh!

pedroigor left a comment • 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!

jsorah commented Oct 2, 2023

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 3, 2023

Unreported flaky test detected

org.keycloak.testsuite.forms.ResetCredentialsAlternativeFlowsTest#testResetCredentialsFlowWithUsernameProvidedFromBrowserFlow

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 3, 2023

Unreported flaky test detected

org.keycloak.testsuite.forms.ResetCredentialsAlternativeFlowsTest#testResetCredentialsFlowWithUsernameProvidedFromBrowserFlow

Uh oh!

pedroigor left a comment

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Oct 5, 2023

Uh oh!

ssilvert commented Oct 6, 2023

Uh oh!

jonkoops left a comment

Choose a reason for hiding this comment

Uh oh!

cypress Bot commented Oct 6, 2023

1 flaky test on run #9270 ↗︎

Review all test suite changes for PR #23583 ↗︎

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vickeybrown commented Sep 28, 2023 •

edited by jonkoops

Loading

cypress Bot commented Sep 28, 2023 •

edited

Loading

pedroigor left a comment •

edited

Loading