Required actions execution order (session and user required actions)

[Mara Hegemann]

Hi, 

I configured some required actions like so:

* password reset (priority 1)

* verify email (priority 2)

* terms and conditions (priority 3)

When the user triggers a password reset (via forgot password) I expect the following order:

action |context

--------|-------

password reset| session

verify email |user

terms and conditions |user

but instead they execute in the following order:

action        | context

--------|-------

verify email |user

terms and conditions |user

password reset| session

This seems like a bug to me and I looked into the code and it seems to go

wrong [here](https://github.com/keycloak/keycloak/blob/5fe675b6128fffa6173209adaa2c742075cb35b5/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java#L1101-L1107)

```java

Stream<String> requiredActions = user.getRequiredActionsStream();

// Here all the user required actions are executed, even if there are higher priority actions stored in the session...

Response action = executionActions(session, authSession, request, event, realm, user, requiredActions);

if (action != null) return action;

//... which are executed afterwards

action = executionActions(session, authSession, request, event, realm, user, authSession.getRequiredActions().stream());

if (action != null) return action;

```

One potential way of fixing this would be to concatenate user required actions and session required actions to execute them in one step.

The only obvious difference would be that we can't return early after executing the user actions, but I don't understand when that is actually desired.

Could you provide some context and would you like me to prepare a PR?

Kind regards,

Mara

PS.: I posted this already in the community forum but was advised to ask my question here instead 

[Thomas Darimont]

Hi Mara,

I think merging the user required actions with the auth-session required actions makes sense. However I think we cannot just concatenate those streams since

we need to honor the order/priority that was configured for the required actions.

I think the proper way to do that would be:

- Merge the user required actions with the session required actions (with duplicate elimination) -> effective required actions

- Order the effective required actions according to the priority from the required actions registration.

- execute the required actions one by one according to their priority

Would that work for you?

Cheers,

Thomas

[Thomas Darimont]

However changing this might break current implementations that rely on the current "ordering", e.g. if an admin assigned required actions to a user explicitly in a certain order.

But I think it is a good default to rely on the order / priority of the required actions that is configured in the required actions list under authentication/required actions in the admin-console.

If users need to deviate from that default ordering, then we'd need to think about a way how this could be expressed.

Cheers,

Thomas

[Thomas Darimont]

I just gave this a try and the following does the trick:

In org.keycloak.services.managers.AuthenticationManager#actionRequired we could replace the code in 

https://github.com/keycloak/keycloak/blob/5fe675b6128fffa6173209adaa2c742075cb35b5/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java#L1101-L1107

with:

```

...

        // combine the required actions configured for a user with the auth-session actions ordered

        // by the overall required actions priority

        Stream<String> requiredActions = effectiveRequiredActions(

                realm,

                user.getRequiredActionsStream(),

                authSession.getRequiredActions().stream());

        // executionActions() method should remove any duplicate actions that might be in the clientSession

        Response action = executionActions(session, authSession, request, event, realm, user, requiredActions);

        if (action != null) return action;

...

```

```

    private static Stream<String> effectiveRequiredActions(RealmModel realm, Stream<String> userRequiredActions, Stream<String> sessionRequiredActions) {

        Set<String> requiredActionNames = Stream.concat(userRequiredActions, sessionRequiredActions).collect(Collectors.toSet());

        return realm.getRequiredActionProvidersStream()

                .filter(rap -> requiredActionNames.contains(rap.getProviderId()))

                .sorted(Comparator.comparing(RequiredActionProviderModel::getPriority))

                .map(RequiredActionProviderModel::getProviderId);

    }

```

This effectively combines the user and auth-session required actions and orders them according to their configured priorities.

I think this would be a good addition to the code base, since it makes the required action ordering more predictable.

Cheers,

Thomas

[Thomas Darimont]

Here is the final version of the merge method:

```

    /**
     * Merges the given {@link Stream Stream's} with {@link RequiredActionProviderModel} provider id's ordered according
     * to their configured priorities.
     * @param realm
     * @param firstProviderIds
     * @param secondProviderIds
     * @return the merged required action provider id's
     */
    private static Stream<String> mergeRequiredActions(RealmModel realm,
                                                       Stream<String> firstProviderIds,
                                                       Stream<String> secondProviderIds) {
        // Note: collecting providerIds here should be cheap as there should be only a handful of required actions overall
        Set<String> requiredActionProviderIds = Stream.concat(firstProviderIds,secondProviderIds).collect(Collectors.toSet());
        return realm.getRequiredActionProvidersStream()
                .filter(rap -> requiredActionProviderIds.contains(rap.getProviderId()))
                .sorted(RequiredActionProviderModel.RequiredActionComparator.SINGLETON)
                .map(RequiredActionProviderModel::getProviderId);
    }

```

You need to ensure that your required actions all have a different(!) priority set, since the built-in `RequiredActionProviderModel.RequiredActionComparator.SINGLETON`

orders required actions by priority - and if there is a tie it uses the name as a differencer. This might lead to hard to predict orderings. I assume that this was done for backwards compatibility.

With explicit priorities one should be safe.

Cheers,

Thomas

--
You received this message because you are subscribed to a topic in the Google Groups "Keycloak Dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/keycloak-dev/MqpYd7kKa3U/unsubscribe.
To unsubscribe from this group and all its topics, send an email to keycloak-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/c61b7177-fb48-420c-89a0-4a6218dad334n%40googlegroups.com.

[Thomas Darimont]

Just noticed that the explicit sorting of actions is not necessary, since it is done as the first step of 

org.keycloak.services.managers.AuthenticationManager#executionActions

[Thomas Darimont]

Then it's really just:

        // combine the required actions configured for a user with the auth-session actions ordered
        // by the overall required actions priority

        Stream<String> requiredActions = Stream.concat(user.getRequiredActionsStream(), authSession.getRequiredActions().stream());

[Mara Hegemann]

Hi Thomas,

thanks for your prompt and extensive response.

Funny, you have retraced my thought process step by step :).The proposed solution does not cover the backwards compatibility for the recent "ordering" of the required actions, as you already mentioned. How would we go about that? 

Is there maybe a mechanism by which we can warn users about a deviation in ordering between the old and new version and introduce a deprecation period first? We could then put the new implementation behind a feature flag and give people some time to upgrade.

This is just a suggestion, if there is an established way of dealing with breaking changes like this in keycloak I'd be happy to go with that instead.

Greetings,
Mara

[Thomas Darimont]

Hi Mara,

Regarding:

> The proposed solution does not cover the backwards compatibility for the recent "ordering" of the required actions, as you already mentioned. How would we go about that? 

Actually, at the moment I think the "ordering" is not guaranteed at all, since the executeActions(..) method sorts the given actions again based on their priority.

https://github.com/keycloak/keycloak/blob/5fe675b6128fffa6173209adaa2c742075cb35b5/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java#L1202

So it sorts the realm required actions AND the user required actions separately, but according to the priority from the realm action configuration.

So the "order" of the actions that are explicitly assigned to a user is in fact also based on the required action priority.

I think this behavior is currently a bit unpredictable to users and changing that to a more predictable one makes sense to me.

Cheers,

Thomas

To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/a2852902-81d8-4a97-848d-9ef09c00aa57n%40googlegroups.com.