Make required actions configurable (#28400) #28419

thomasdarimont · 2024-04-03T20:48:58Z

This PR adds support for configurable required actions.

Some implementation notes:

Introduced RequiredActionConfigModel to hold the configration of a RequiredAction
Extended AuthenticationManagementResource with CRUD operations for RequiredAction config management
Extended admin-client libraries with RequiredAction config management methods
Exposed RequiredActionConfigModel via RequiredActionContext to users
Add tests for crud operations on configurable required actions

Fixes #28400

thomasdarimont · 2024-04-03T20:56:49Z

@stianst FYI this is the PR for initial support for configurable required actions we spoke about yesterday.

What's already working:

Storage of required actions configuration in the required_action_config table
Configuration per required actions instance in a realm
CRUD for required actions with configuration
Users can make their own required action implementations configurable by overriding the org.keycloak.provider.ProviderFactory#getConfigMetadata()
Initial tests for RequiredAction CRUD config management
Extended admin-client libraries

What's still missing:

Admin UI integration ("cog" settings button and settings form for required actions)
Documentation
"Proper" way to validate config settings before save

Some UI Experiment:

I temporarily made the Update Password required action configurable. When I now click on the "Settings icon", I see this :)

thomasdarimont

Some remarks inline.

js/apps/admin-ui/src/authentication/RequiredActions.tsx

thomasdarimont · 2024-04-03T21:15:21Z

server-spi-private/src/main/java/org/keycloak/authentication/RequiredActionFactory.java

+     * @param realm
+     * @param model
+     */
+    default void validateConfig(RealmModel realm, RequiredActionConfigModel model) {


I'm not sure whether this is the proper way to perform validation, but it could be used to prevent bad configurations to be written to the database.

thomasdarimont · 2024-04-03T21:16:16Z

server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java

@@ -90,6 +95,33 @@ public static ConfigurableAuthenticatorFactory getConfigurableAuthenticatorFacto
         return factory;
    }

+    public static RequiredActionFactory getConfigurableRequiredActionFactory(KeycloakSession session, String providerId) {


This and the following helper functions should be moved elsewhere... just placed them here because the authenticator config methods were also located here.

Nitpick: Can be ok to introduce new class like RequiredActionHelper and add those methods here as they don't have much to do with credentials...

thomasdarimont · 2024-04-04T11:24:26Z

I now have an initial version working, that allows to inspect, add, update and remove required action configuration via the admin ui.

Required Actions List:
I now only show the settings icon, if the required action is configurable. Instead of using a dedicated column for the button, we could also place the icon next to the required action label.

Required Actions Config Modal:
In the config modal I currently only show the actual settings. It would be good to show the actual name of the current required action as a label somewhere.

thomasdarimont

Added some more remarks to the places where I'm unsure

js/apps/admin-ui/src/authentication/components/RequiredActionConfigModal.tsx

thomasdarimont · 2024-04-04T12:08:01Z

js/libs/keycloak-admin-client/test/authenticationManagement.spec.ts

@@ -133,6 +133,19 @@ describe("Authentication management", () => {
      );
    });

+    // TODO use required action that is actually configurable


Here we need to provide some configurable dummy required action to test the retrieval of the configurable properties.

thomasdarimont · 2024-04-04T12:10:08Z

services/src/main/java/org/keycloak/authentication/requiredactions/UpdatePassword.java

@@ -213,4 +218,20 @@ public int getMaxAuthAge() {

        return maxAge;
    }
+
+    // TODO remove me: this is just for the sake of demonstrating configurable required actions


This is just for the sake of the example, and will be removed from the final PR

thomasdarimont · 2024-04-12T12:54:04Z

Thanks to @edewit the new UI now looks a bit better:

Config screen:

@edewit would it be possible to show the translated name instead of the alias? (Update Password instead of "UPDATE_PASSWORD"). Also I think the config dialog would scale better if the name of the required action would be on a separat line. Some users tend to have quite long (but descriptive) names for required actions.

mposolda

Thanks,

I've added one minor comment inline. In general, I am not 100% sure if we want to rely on ComponentModel, which was the approach discussed on the meeting AFAIR. But maybe not... (added some more details in the email).

mposolda · 2024-04-15T07:16:55Z

server-spi/src/main/java/org/keycloak/models/RequiredActionProviderModel.java

@@ -120,4 +123,12 @@ public Map<String, String> getConfig() {
    public void setConfig(Map<String, String> config) {
        this.config = config;
    }
+
+    public boolean isConfigurable() {


Do we need isConfigurable and setConfigurable on the model? It seems that the configurable field is not backed in the DB and it is always derived based on the factory (like in RealmAdapter.entityToModel(RequiredActionProviderEntity entity)).

Also I believe it is not needed in the JSON representation object, which is used for export/import, but I understand it can be useful for have this in the UI layer. Is it possible to decouple those?

Thanks for taking a look! Yes I added the configurable property to the RequiredActionProviderModel and RequiredActionProviderRepresentation . Initially I thought about deriving the "configurability" of a RequiredAction from a non-empty configuration. However, with this we currently cannot differentiate an empty configuration from a non-configurable required action, as the config propery is currently always initialized with an empty map here: https://github.com/keycloak/keycloak/blob/main/core/src/main/java/org/keycloak/representations/idm/RequiredActionProviderRepresentation.java#L35
If we would change this to be null by default, then I could remove the configurable from the model and the representation property.

I was also thinking about using the component model but then realized that required actions already have a database backed config HashMap, so I kept using that.

I understand that the info about "configurable" is needed in the UI. But I think it is not needed in the model layer and might be cleaner to remove it from here.

I see the options like:

Keep configurable at the model layer and RequiredActionProviderRepresentation. Not so clean IMO.

Use the null to indicate. It can work, but just afraid a bit of things like NullPointerException etc.

Add stuff inside admin UI java extension (module rest/admin-ui-ext in the workspace) and make admin console to use it. I can imagine something like this in the package org.keycloak.admin.ui.rest.model inside admin UI extension (the stuff in the module rest, which contains REST endpoints used solely by admin console):

public class ConfigurableRequiredActionProviderRepresentation extends RequiredActionProviderRepresentation { private boolean configurable; // Getter and setter for "configurable" here ... }

And then make org.keycloak.admin.ui.rest.AuthenticationManagementResource to return required actions like those, so the admin console UI is able to figure if action is configurable or not. The disadvantage of the extension is additional code.

My vote is either (2) or (3). WDYT?

thomasdarimont · 2024-04-18T23:26:00Z

A quick experiment for better validation support:

thomasdarimont · 2024-04-18T23:42:02Z

Now using the required action name instead of alias, as this is more user friendly.

thomasdarimont · 2024-04-21T21:52:50Z

@mposolda thanks for your review and the helpful suggestions! Regarding your proposal for moving forward, I went with a combination of (2) and (3) as discussued above:

(2) use null to indicate no required action config is set
(3) add admin ui specific endpoint that returns the configurable property for a required action.

Changes

The latest changes of this PR include:

Removed boolean configurable from RequiredActionProviderRepresentation and RequiredActionConfigModel classes
Adapted UI and API to work with requiredaction provider alias instead of providerId for consistency
Introduced admin-ui specific endpoint org.keycloak.admin.ui.rest.AuthenticationManagementResource#getRequiredActions for retrieving required actions with config metadata

Alias vs ProviderId

Since all the endpoints that handle requiredactions work with the requiredaction alias value,
I adapted all places that work with requiredaction configs to use aliases as well.
When I have the providerId, then I also check if the providerId matches that of the given RequiredActionConfigModel.

The only difference is the following, which only has an alias available at the moment:
org.keycloak.models.jpa.RealmAdapter#getRequiredActionConfigByAlias

However, using the aliases all the time means that required action aliases within a realm must be unique - is this guaranteed somehow?

Common config properties for Required Actions

I also tried to have max_auth_age as a generic configuration parameter for all required actions, but I needed to rollback that change I wasn't sure whether this really makes sense for all required actions and wanted to discuss this first. Also that would make all required actions configurable!

Update Password example for config check and validation

As an example I adapted the UPDATE_PASSWORD required action to consider the configured value for max_auth_age with a preference for the password policy if set.

Import / Export

Export in a realm with required action config and importing it again correctly shows the required action config values.

Current state and next steps

The PR - at least for the initial capability is on the finish line. I stell need to add a test for the client library but after that we should be good.

Perhaps @edewit could take a look again on the translation of error messages if a validation fails :)
I need some help to render the proper error message based on the server response, see:

thomasdarimont · 2024-05-14T08:45:53Z

@jonkoops This PR currently fails due to an unrelated test:

  Running:  realm_settings_user_profile_enabled.spec.ts                                     (7 of 7)

  User profile tabs
    Attributes sub tab tests
      ✓ Goes to create attribute page (4828ms)
      ✓ Completes new attribute form and performs cancel (2892ms)
      ✓ Completes new attribute form and performs submit (2619ms)
      ✓ Modifies existing attribute and performs save (3569ms)
      ✓ Adds and removes validator to/from existing attribute and performs save (3593ms)
    Attribute groups sub tab tests 
      (Attempt 1 of 4) Deletes an attributes group  <---- This fails
      (Attempt 2 of 4) Deletes an attributes group
      (Attempt 3 of 4) Deletes an attributes group
      1) Deletes an attributes group

I ran the cypress tests locally and there it fails too. I currently don't see a link to my changes. Do you have an idea what the problem is?

jonkoops · 2024-05-14T09:04:32Z

Not sure what is happening there, perhaps someone from the UI team can take a look. @edewit @hmlnarik

thomasdarimont · 2024-05-14T09:15:22Z

Thanks for calling help :) It seems to fail because the menu popup does not work:

However, when I try this myself with a fresh build it works fine.

hmlnarik · 2024-05-14T09:25:32Z

Looks like a test interference. I have introduced #29507 and rerun the admin UI testsuite from Generate test seed on.

thomasdarimont · 2024-05-16T20:48:11Z

Unfortunately I'm blocked by the failing UI tests, which are unrelated to this PR. The same tests also fail on my machine locally when I run the latest Keycloak main (I tested this with cypress on electron, and chrome).
The ui test that currently fails is: client_authorization_test.spec.ts.

Any help would be highly appreciated :)

edewit · 2024-05-21T09:00:44Z

Any help would be highly appreciated :)

Tests have been fixed on main, I have rebased your PR

mposolda

@thomasdarimont Thanks for all the updates!

I've added few comments inline. It can be good to address them, but at the same time, I don't consider any of them as a blocker, so we can possibly do as a follow-up. Whatever you prefer.

Will need review from UI team as well.

mposolda · 2024-05-22T10:20:28Z

server-spi/src/main/java/org/keycloak/models/RequiredActionProviderModel.java

@@ -43,6 +45,7 @@ public interface RequiredActionComparator extends Comparator<RequiredActionProvi
    private boolean defaultAction;
    private int priority;
    private Map<String, String> config = new HashMap<>();
+    private boolean configurable;


Nitpick: This field is not used and can be removed from this class (probably all the changes from this class can be removed from this PR)

mposolda · 2024-05-22T10:23:56Z

server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java

@@ -90,6 +95,33 @@ public static ConfigurableAuthenticatorFactory getConfigurableAuthenticatorFacto
         return factory;
    }

+    public static RequiredActionFactory getConfigurableRequiredActionFactory(KeycloakSession session, String providerId) {


Nitpick: Can be ok to introduce new class like RequiredActionHelper and add those methods here as they don't have much to do with credentials...

mposolda · 2024-05-22T10:26:04Z

services/src/main/java/org/keycloak/authentication/requiredactions/UpdatePassword.java

+
+            // we need to figure out the alias for the current required action
+            String providerId = authSession.getClientNote(Constants.KC_ACTION);
+            RequiredActionProviderModel requiredAction = realm.getRequiredActionProvidersStream() //


Nitpick: I can imagine it can be useful to have some util method (maybe in the RequiredActionsHelper as suggested in other comments or in KeycloakModelUtils) like:

RequiredActionConfigModel getRequiredActionConfig(RealmModel realm, String requiredActionProviderId)

as I suppose the similar code will apply always when retrieving configurations from required action

mposolda · 2024-05-22T10:29:41Z

services/src/main/java/org/keycloak/authentication/requiredactions/UpdatePassword.java

+                .property() //
+                .name(MAX_AUTH_AGE_KEY) //
+                .label("Maximum Age of Authentication") //
+                .helpText("Configures the duration in seconds this action can be used after the last authentication before the user is required to re-authenticate.") //


Suggested change

.helpText("Configures the duration in seconds this action can be used after the last authentication before the user is required to re-authenticate.") //

.helpText("Configures the duration in seconds this action can be used after the last authentication before the user is required to re-authenticate. This parameter is used just in the context of AIA when the kc_action parameter is available in the request, which is for instance when user himself updates his password in the account console. When the 'Maximum Authentication Age' password policy is used in the realm, it's value has precedence over the value configured here.") //

Nitpick: just suggesting to add a bit more info to the helpText to clarify it is used just in the context of AIA and about the fact that password policy has preference.

mposolda · 2024-05-22T11:42:04Z

@thomasdarimont Since there is conflict and this PR will require changes, WDYT about adding my "nitpicks" (if they makes sense for you) when you work on resolving conflict? :-) Thanks!

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-05-22T14:03:03Z

Unreported flaky test detected

If the 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#testAllConcurrently

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:61)
...

Report flaky test

org.keycloak.testsuite.model.session.UserSessionPersisterProviderTest#testMigrateSession

Keycloak CI - Store Model Tests

java.lang.AssertionError: expected:<3> but was:<1>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

- Add tests for crud operations on configurable required actions - Add support exposing the required action configuration via RequiredActionContext - Make configSaveError message reusable in other contexts - Introduced admin-ui specific endpoint for retrieving required actions with config metadata Fixes keycloak#28400 Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com> Co-authored-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

thomasdarimont · 2024-05-22T15:26:18Z

@mposolda Thanks for your review and the recommendations. I fixed all your nitpicks and rebased to latest main.
I think the PR is now ready for merge.

mposolda

@thomasdarimont @edewit Thanks a lot for all the updates and review!

thomasdarimont force-pushed the poc/required-actions-config branch from 5c02f24 to f586f87 Compare April 3, 2024 20:57

This comment was marked as resolved.

Sign in to view

thomasdarimont force-pushed the poc/required-actions-config branch from f586f87 to ca2527a Compare April 3, 2024 21:19

thomasdarimont commented Apr 3, 2024

View reviewed changes

thomasdarimont force-pushed the poc/required-actions-config branch 2 times, most recently from c00553b to adb2e54 Compare April 3, 2024 21:48

keycloak-github-bot bot added the flaky-test label Apr 3, 2024

keycloak-github-bot bot mentioned this pull request Apr 3, 2024

Flaky test: org.keycloak.testsuite.oauth.OfflineTokenTest#offlineTokenBrowserFlowIdleTimeExpired #22845

Open

thomasdarimont force-pushed the poc/required-actions-config branch 2 times, most recently from e0b6341 to 4b14b48 Compare April 4, 2024 10:14

thomasdarimont commented Apr 4, 2024

View reviewed changes

thomasdarimont force-pushed the poc/required-actions-config branch from 2b07eb1 to add85a9 Compare April 8, 2024 09:04

mposolda reviewed Apr 15, 2024

View reviewed changes

mposolda self-assigned this Apr 15, 2024

thomasdarimont force-pushed the poc/required-actions-config branch from 72b1f78 to 6853f8c Compare April 18, 2024 22:33

thomasdarimont force-pushed the poc/required-actions-config branch 6 times, most recently from 3030ba3 to 42f0d33 Compare April 21, 2024 21:40

thomasdarimont force-pushed the poc/required-actions-config branch 2 times, most recently from 77ef3df to 537b222 Compare April 22, 2024 08:52

mposolda mentioned this pull request Apr 22, 2024

Always order required actions by priority (regardless of context) #28956

Merged

thomasdarimont force-pushed the poc/required-actions-config branch from 11842d3 to 18ca9ea Compare May 14, 2024 10:41

This was referenced May 14, 2024

Flaky test: org.keycloak.testsuite.x509.X509BrowserLoginTest#loginWithRevalidateCertEnabledCertIsTrusted #25444

Open

Flaky test: org.keycloak.testsuite.forms.RecoveryAuthnCodesAuthenticatorTest#test03AuthenticateRecoveryAuthnCodes #26176

Open

thomasdarimont force-pushed the poc/required-actions-config branch 4 times, most recently from 6a35e7a to a98b7df Compare May 16, 2024 09:47

thomasdarimont force-pushed the poc/required-actions-config branch 2 times, most recently from 3afaec2 to 76bd1c8 Compare May 20, 2024 09:53

edewit force-pushed the poc/required-actions-config branch from 76bd1c8 to 0e6d552 Compare May 21, 2024 08:59

mposolda reviewed May 22, 2024

View reviewed changes

mposolda previously approved these changes May 22, 2024

View reviewed changes

edewit previously approved these changes May 22, 2024

View reviewed changes

thomasdarimont dismissed stale reviews from edewit and mposolda via d992693 May 22, 2024 13:03

thomasdarimont force-pushed the poc/required-actions-config branch from 0e6d552 to d992693 Compare May 22, 2024 13:03

keycloak-github-bot bot reviewed May 22, 2024

View reviewed changes

thomasdarimont force-pushed the poc/required-actions-config branch from d992693 to 2724bfe Compare May 22, 2024 14:05

mposolda approved these changes May 23, 2024

View reviewed changes

mposolda merged commit ab376d9 into keycloak:main May 23, 2024
71 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make required actions configurable (#28400) #28419

Make required actions configurable (#28400) #28419

thomasdarimont commented Apr 3, 2024

thomasdarimont commented Apr 3, 2024 •

edited

This comment was marked as resolved.

thomasdarimont left a comment

thomasdarimont Apr 3, 2024

thomasdarimont Apr 3, 2024

mposolda May 22, 2024

thomasdarimont commented Apr 4, 2024

thomasdarimont left a comment

thomasdarimont Apr 4, 2024

thomasdarimont Apr 4, 2024

thomasdarimont commented Apr 12, 2024

mposolda left a comment

mposolda Apr 15, 2024

thomasdarimont Apr 15, 2024 •

edited

mposolda Apr 19, 2024

thomasdarimont commented Apr 18, 2024

thomasdarimont commented Apr 18, 2024

thomasdarimont commented Apr 21, 2024 •

edited

thomasdarimont commented May 14, 2024

jonkoops commented May 14, 2024

thomasdarimont commented May 14, 2024 •

edited

hmlnarik commented May 14, 2024

thomasdarimont commented May 16, 2024

edewit commented May 21, 2024

mposolda left a comment

mposolda May 22, 2024

mposolda May 22, 2024

mposolda May 22, 2024

mposolda May 22, 2024

mposolda commented May 22, 2024

keycloak-github-bot bot left a comment

keycloak-github-bot bot commented May 22, 2024

thomasdarimont commented May 22, 2024

mposolda left a comment

Make required actions configurable (#28400) #28419

Make required actions configurable (#28400) #28419

Conversation

thomasdarimont commented Apr 3, 2024

thomasdarimont commented Apr 3, 2024 • edited

This comment was marked as resolved.

thomasdarimont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasdarimont commented Apr 4, 2024

thomasdarimont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasdarimont commented Apr 12, 2024

mposolda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasdarimont Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasdarimont commented Apr 18, 2024

thomasdarimont commented Apr 18, 2024

thomasdarimont commented Apr 21, 2024 • edited

thomasdarimont commented May 14, 2024

jonkoops commented May 14, 2024

thomasdarimont commented May 14, 2024 • edited

hmlnarik commented May 14, 2024

thomasdarimont commented May 16, 2024

edewit commented May 21, 2024

mposolda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mposolda commented May 22, 2024

keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

keycloak-github-bot bot commented May 22, 2024

Unreported flaky test detected

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

org.keycloak.testsuite.model.session.UserSessionPersisterProviderTest#testMigrateSession

thomasdarimont commented May 22, 2024

mposolda left a comment

Choose a reason for hiding this comment

thomasdarimont commented Apr 3, 2024 •

edited

thomasdarimont Apr 15, 2024 •

edited

thomasdarimont commented Apr 21, 2024 •

edited

thomasdarimont commented May 14, 2024 •

edited