Add dedicated feature flag for oauth device grant flow (#23891) #23892

thomasdarimont · 2023-10-11T09:00:22Z

The new flag "DEVICE_FLOW" is enabled by default for now. At a later stage we might disable the feature by default.

Users can now disable the device flow support for the whole server via: -Dkeycloak.profile.feature.device_flow=disabled
or
--features-disabled=device_flow

See discussion: #23700

Fixes #23891

cypress · 2023-10-11T09:24:31Z

2 flaky tests on run #9541 ↗︎

0	527	48	0	2

Details:

Merge `5841a1f` into `03a8f05`...
Project: Keycloak Admin UI	Commit: `85e78d1cf2 ℹ️`
Status: Passed	Duration: 11:15 💡
Started: Oct 23, 2023 8:41 PM	Ended: Oct 23, 2023 8:52 PM

clients_test.spec.ts • 1 flaky test • chrome

View Output Video

Test		Artifacts
Clients test > Accessibility tests for clients > Check a11y violations on load/ clients list tab		`Test Replay` `Output` `Screenshots`

realm_settings_general_tab_test.spec.ts • 1 flaky test • chrome

View Output Video

Test		Artifacts
Realm settings general tab tests > Test all general tab switches		`Test Replay` `Output` `Screenshots`

Review all test suite changes for PR #23892 ↗︎

ghost

Unreported flaky test detected, please review

ghost · 2023-10-11T10:32:39Z

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.ui.account2.LDAPAccountTest#createdNotVisibleTest

Keycloak CI - Account Console IT (firefox)

org.openqa.selenium.NoSuchElementException: 
Unable to locate element: #username
For documentation on this error, please visit: http://seleniumhq.org/exceptions/no_such_element.html
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
System info: host: 'fv-az951-282', ip: '10.1.0.194', os.name: 'Linux', os.arch: 'amd64', os.version: '6.2.0-1012-azure', java.version: '17.0.8.1'
...

Report flaky test

org.keycloak.testsuite.ui.account2.WelcomeScreenTest#resourcesTest

Keycloak CI - Account Console IT (firefox)

java.lang.AssertionError: Element should be visible expected:<true> but was:<false>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.keycloak.testsuite.util.UIAssert.assertElementVisible(UIAssert.java:47)
...

Report flaky test

org.keycloak.testsuite.forms.ResetCredentialsAlternativeFlowsTest#resetCredentialsVerifyCustomOtpLabelSetProperly

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:60)
	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-11T13:55:06Z

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.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

thomasdarimont · 2023-10-11T19:07:55Z

The following screenshots show the new behaviour when the device-flow is disabled for the Keycloak Server:

Realm Info lists device-flow as disabled feature

Tokens Tab in Realm Settings doesn't show device flow settings

When creating a new client Capability config doesn't show the device flow

When editing a client device flow is also not shown:

/realms/{realm}/device shows 404 page

ghost · 2023-10-11T21:52:10Z

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.ui.account2.DeviceActivityTest#currentSessionTest

Keycloak CI - Account Console IT (firefox)

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a org.keycloak.testsuite.util.URLAssert URL expected to begin with: https://localhost:8543/auth/realms/test/protocol/openid-connect/auth ; actual URL: https://localhost:8543/auth/realms/test/account/#/security/device-activity within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
...

Report flaky test

org.keycloak.testsuite.ui.account2.DeviceActivityTest#timesTests

Keycloak CI - Account Console IT (firefox)

org.openqa.selenium.UnhandledAlertException: 
Dismissed user prompt dialog: failed to initialize keycloak: 
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
System info: host: 'fv-az1233-258', ip: '10.1.0.100', os.name: 'Linux', os.arch: 'amd64', os.version: '6.2.0-1012-azure', java.version: '17.0.8.1'
Driver info: org.openqa.selenium.firefox.FirefoxDriver
...

Report flaky test

ghost

Unreported flaky test detected, please review

jonkoops

Changes to the front-end code look good to me. I will leave it to other team members to judge if this feature is something we want and if the Java code is adequate.

common/src/main/java/org/keycloak/common/Profile.java

Michito-Okai

Looks good to me. Please insert its test code afterwards.

thomasdarimont · 2023-10-17T18:46:53Z

@mposolda @tnorimat I just added the missing tests and I think the feature is complete now. Feel free to merge if the pipeline passes.

mposolda · 2023-10-20T07:59:18Z

Thanks for taking a look! What's different in "Keycloak CI / Base IT (6)"? (Perhaps a better name would really help here) Locally it works fine, I try to replicate whatever "Keycloak CI / Base IT (6)" later today.

@thomasdarimont It uses keycloak distribution instead of the undertow distribution. Maybe you can replicate the issue when you run the test with something like:

mvn clean install -DskipTests=true
cd testsuite/integration-arquillian
mvn clean install -Pauth-server-quarkus -Dtest=OAuth2DeviceAuthorizationGrantTest

?

Just a guess (did not tried it myself).

thomasdarimont · 2023-10-23T10:28:16Z

@mposolda I gave this another try, and it seems as if the @DisableFeature(value = Profile.Feature.DEVICE_FLOW, skipRestart = true) is not taken into account by the "Keycloak CI / Base IT (6)" testrun.

When I run the tests for OAuth2DeviceAuthorizationGrantTest in the IDE, it works fine, but fails when run with
mvn clean install -Pauth-server-quarkus -Dtest=OAuth2DeviceAuthorizationGrantTest -rf :integration-arquillian-tests-base

Could it be that the undertow based test-server implementation doesn't consider the feature flags?

thomasdarimont · 2023-10-23T10:36:41Z

@mposolda the problem seems to be that the test is run with $KEYCLOAK_PROJECT_HOME/testsuite/integration-arquillian/tests/base/target/containers/auth-server-undertow/profile.properties
which for some reason does not exist when I run only the tests from the OAuth2DeviceAuthorizationGrantTest...

thomasdarimont · 2023-10-23T13:49:48Z

I could successfully run the build via:

mvn clean install -Pauth-server-undertow -Dtest=OAuth2DeviceAuthorizationGrantTest --am

but it failes with:

 mvn clean install -Pauth-server-quarkus -Dtest=OAuth2DeviceAuthorizationGrantTest --am

I needed to configure OAuth2DeviceAuthorizationGrantTest to use @EnableFeature(value = Profile.Feature.DEVICE_FLOW, skipRestart = true) on class level to ensure that the device_flow feature remains active throughout the test.

I think we need something like: getTestingClient().resetFeature(Profile.Feature.DEVICE_FLOW); which removes the entry from profile.properties .

@mposolda do you have an idea how to ensure that the @EnableFeature annotation is also honored with -Pauth-server-quarkus?

thomasdarimont · 2023-10-23T20:30:33Z

...ests/base/src/test/java/org/keycloak/testsuite/oauth/OAuth2DeviceAuthorizationGrantTest.java

+    public void ensureDeviceFlowConfigNotPresentWhenDeviceFlowIsDisabled() throws Exception {
+
+        // this test currently does not work with -Pauth-server-quarkus
+        ContainerAssume.assumeAuthServerUndertow();


After many hours of trying to making this test work with -Pauth-server-quarkus I gave up and just assume that we run the test with undertow. The test works fine in the IDE and with -Pauth-server-undertow.

For some reason the @DisabledFeature annotation is now applied when the test suite is executed with -Pauth-server-quarkus.

That is fine, Thanks a lot for this investigation!

Created follow-up issue for looking at this test #24241. When thinking about this, I wonder if it can have some relation to OIDC well-known not being refreshed after feature is enabled/disabled? But not sure...

The new flag "DEVICE_FLOW" is enabled by default for now. At a later stage we might disable the feature by default. Users can now disable the device flow support for the whole server via: -Dkeycloak.profile.feature.device_flow=disabled or --features-disabled=device_flow See discussion: keycloak#23700 Fixes keycloak#23891

Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>

…GrantTest

For some reason @DisableFeature is not considered when executed with -Pauth-server-quarkus. Current workaround is to ignore exceptions during -Pauth-server-quarkus.

savage-alex · 2023-11-17T09:54:40Z

Thank you for this fix

ghost added the flaky-test label Oct 11, 2023

This was referenced Oct 11, 2023

Flaky test: org.keycloak.testsuite.ui.account2.ApplicationsTest#toggleApplicationDetailsTest #23362

Closed

Flaky test: org.keycloak.testsuite.federation.ldap.LDAPProvidersFullNameMapperTest#testUpdatingAttributesWorksEvenWithEmptyAttributes #23804

Closed

ghost reviewed Oct 11, 2023

View reviewed changes

thomasdarimont force-pushed the issue/GH-23891-device-flow-feature-flag branch from d2a5920 to 057cdf6 Compare October 11, 2023 12:27

ghost reviewed Oct 11, 2023

View reviewed changes

thomasdarimont force-pushed the issue/GH-23891-device-flow-feature-flag branch from 25136a7 to 5dcdf07 Compare October 11, 2023 19:36

ghost mentioned this pull request Oct 11, 2023

Flaky test: org.keycloak.testsuite.ui.account2.DeviceActivityTest#signOutTest #20915

Closed

ghost reviewed Oct 11, 2023

View reviewed changes

thomasdarimont force-pushed the issue/GH-23891-device-flow-feature-flag branch from 5dcdf07 to d0f6908 Compare October 12, 2023 20:23

jonkoops previously approved these changes Oct 14, 2023

View reviewed changes

common/src/main/java/org/keycloak/common/Profile.java Outdated Show resolved Hide resolved

Michito-Okai approved these changes Oct 16, 2023

View reviewed changes

thomasdarimont dismissed jonkoops’s stale review via fc5ba3e October 16, 2023 11:45

thomasdarimont force-pushed the issue/GH-23891-device-flow-feature-flag branch 2 times, most recently from fc5ba3e to 8692de8 Compare October 17, 2023 18:44

thomasdarimont marked this pull request as ready for review October 17, 2023 18:46

thomasdarimont requested review from a team as code owners October 17, 2023 18:46

thomasdarimont requested a review from a team October 17, 2023 18:46

ghost mentioned this pull request Oct 19, 2023

Flaky test: org.keycloak.testsuite.forms.ResetPasswordTest#resetPasswordLink #21422

Closed

mposolda mentioned this pull request Oct 20, 2023

Support for transient users for brokered accounts #23977

Merged

1 task

thomasdarimont force-pushed the issue/GH-23891-device-flow-feature-flag branch from dc37888 to 4ceb3d6 Compare October 23, 2023 13:49

ghost mentioned this pull request Oct 23, 2023

Flaky test: org.keycloak.testsuite.forms.VerifyProfileTest#testDefaultProfile #24198

Closed

thomasdarimont commented Oct 23, 2023

View reviewed changes

thomasdarimont added 11 commits October 23, 2023 22:31

Honor device flow feature in client capability config in admin ui

f9d6316

Revise Profile based on code review

1af17cc

Add tests for oauth device grant flow feature disabled (keycloak#23892)

66560b6

Revise tests for disabled device flow

d427b73

Add device-flow flag to approved help output references

1bdcfc5

Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>

Revise approved help output tests for disabled device flow

c1a9639

Revise test method name

25ab908

Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>

Activate device_flow feature by default for OAuth2DeviceAuthorization…

64ac80c

…GrantTest

Fix typo in TestingResourceProvider#enableFeature

621df83

Workaround non working feature toggle with -Pauth-server-quarkus

5841a1f

For some reason @DisableFeature is not considered when executed with -Pauth-server-quarkus. Current workaround is to ignore exceptions during -Pauth-server-quarkus.

thomasdarimont force-pushed the issue/GH-23891-device-flow-feature-flag branch from 3398c1c to 5841a1f Compare October 23, 2023 20:31

mposolda mentioned this pull request Oct 24, 2023

Make OAuth2DeviceAuthorizationGrantTest.ensureDeviceFlowConfigNotPresentWhenDeviceFlowIsDisabled working with Quarkus #24241

Open

mposolda approved these changes Oct 24, 2023

View reviewed changes

mposolda merged commit e567210 into keycloak:main Oct 24, 2023
71 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dedicated feature flag for oauth device grant flow (#23891) #23892

Add dedicated feature flag for oauth device grant flow (#23891) #23892

thomasdarimont commented Oct 11, 2023

cypress bot commented Oct 11, 2023 •

edited

Loading

ghost left a comment

ghost commented Oct 11, 2023

ghost left a comment

ghost commented Oct 11, 2023

thomasdarimont commented Oct 11, 2023

ghost commented Oct 11, 2023

ghost left a comment

jonkoops left a comment

Michito-Okai left a comment •

edited

Loading

thomasdarimont commented Oct 17, 2023

mposolda commented Oct 20, 2023

thomasdarimont commented Oct 23, 2023

thomasdarimont commented Oct 23, 2023

thomasdarimont commented Oct 23, 2023 •

edited

Loading

thomasdarimont Oct 23, 2023

mposolda Oct 24, 2023

savage-alex commented Nov 17, 2023

Add dedicated feature flag for oauth device grant flow (#23891) #23892

Add dedicated feature flag for oauth device grant flow (#23891) #23892

Conversation

thomasdarimont commented Oct 11, 2023

cypress bot commented Oct 11, 2023 • edited Loading

2 flaky tests on run #9541 ↗︎

Review all test suite changes for PR #23892 ↗︎

ghost left a comment

Choose a reason for hiding this comment

ghost commented Oct 11, 2023

Unreported flaky test detected

org.keycloak.testsuite.ui.account2.LDAPAccountTest#createdNotVisibleTest

org.keycloak.testsuite.ui.account2.WelcomeScreenTest#resourcesTest

org.keycloak.testsuite.forms.ResetCredentialsAlternativeFlowsTest#resetCredentialsVerifyCustomOtpLabelSetProperly

ghost left a comment

Choose a reason for hiding this comment

ghost commented Oct 11, 2023

Unreported flaky test detected

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

thomasdarimont commented Oct 11, 2023

ghost commented Oct 11, 2023

Unreported flaky test detected

org.keycloak.testsuite.ui.account2.DeviceActivityTest#currentSessionTest

org.keycloak.testsuite.ui.account2.DeviceActivityTest#timesTests

ghost left a comment

Choose a reason for hiding this comment

jonkoops left a comment

Choose a reason for hiding this comment

Michito-Okai left a comment • edited Loading

Choose a reason for hiding this comment

thomasdarimont commented Oct 17, 2023

mposolda commented Oct 20, 2023

thomasdarimont commented Oct 23, 2023

thomasdarimont commented Oct 23, 2023

thomasdarimont commented Oct 23, 2023 • edited Loading

thomasdarimont Oct 23, 2023

Choose a reason for hiding this comment

mposolda Oct 24, 2023

Choose a reason for hiding this comment

savage-alex commented Nov 17, 2023

cypress bot commented Oct 11, 2023 •

edited

Loading

Michito-Okai left a comment •

edited

Loading

thomasdarimont commented Oct 23, 2023 •

edited

Loading