[keycloak-dev] TokenEndpoint is potentially too restrictive

[Thomas Darimont]

Hello Group,

I just found out that as long as a user has a required_action set up one
cannot authenticate via grant_type password throught the TokenEndpoint
because of this line in
TokenEndpoint.buildResourceOwnerPasswordCredentialsGrant

if (user.getRequiredActions() != null && user.getRequiredActions().size() >
0) {
   event.error(Errors.RESOLVE_REQUIRED_ACTIONS);
   throw new ErrorResponseException("invalid_grant", "Account is not fully
set up", Response.Status.BAD_REQUEST);
}

current master:
https://github.com/keycloak/keycloak/blob/bd2887aa77184d82e795e4200eb55a3d9b11e4d4/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java#L388

1.9.x
https://github.com/keycloak/keycloak/blob/e7822431fded5948a5e248766e6ffbf86d476cf8/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java#L388

I think that this is too restrictive. Imagine a default required action
that has no user interaction but records some user login statistics like
last login date, count logins, login failures etc.
In this case it would be better to check if the configured required_action
requires user-interaction (e.g. via a new method boolean isInteractive())
or communicate that required actions should not be used for actions that
don't require user interactions - an authenticator is then probably the
better choice.

Cheers,
Thomas

I tested this with a required action like the
LoginStatsRecordingRequiredActionProvider listed below.
Now setup a client with the name test-client and a user with the name
tester.

With that deployed to keycloak as a default action one can use the
following curl command
to see the problem.

KC_REALM=test-realm
KC_USERNAME=tester
KC_PASSWORD=test
KC_CLIENT=test-client
KC_CLIENT_SECRET=e9ca8fad-d399-4455-5290-817102e7f9ff
KC_SERVER=192.168.99.1:8080
KC_CONTEXT=auth

curl -k -v --noproxy 192.168.99.1 \
        -H "Content-Type: application/x-www-form-urlencoded" \
        -d "grant_type=password" \
        -d "username=$KC_USERNAME" \
        -d "password=$KC_PASSWORD" \
        -d "client_id=$KC_CLIENT" \
        -d "client_secret=$KC_CLIENT_SECRET" \
        "http://
$KC_SERVER/$KC_CONTEXT/realms/$KC_REALM/protocol/openid-connect/token"

Gives:
< HTTP/1.1 400 Bad Request
< Connection: keep-alive
< X-Powered-By: Undertow/1
< Server: WildFly/10
< Content-Type: application/json
< Content-Length: 75
< Date: Fri, 08 Jul 2016 08:24:59 GMT
{"error_description":"Account is not fully set up","error":"invalid_grant"}


----


package com.acme.keycloak.ext.authentication;

import static java.util.Arrays.asList;

import java.time.Instant;
import java.time.LocalDateTime;
import java.util.Arrays;
import java.util.List;
import java.util.TimeZone;

import org.jboss.logging.Logger;
import org.keycloak.Config.Scope;
import org.keycloak.authentication.RequiredActionContext;
import org.keycloak.authentication.RequiredActionFactory;
import org.keycloak.authentication.RequiredActionProvider;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.KeycloakSessionFactory;
import org.keycloak.models.UserLoginFailureModel;
import org.keycloak.models.UserModel;

/**
 * Collects information about User Login behaviour.
 * <p>
 * Collects the following:
 * <ul>
 * <li>Date / Time of first login</li>
 * <li>Date / Time of recent login</li>
 * <li>Count of logins since first login</li>
 * <li>Count of failed logins since the configured <code>Failure Reset
Time</code> unter <code>Security Defenses</code></li>
 * </ul>
 * <p>
 * This {@link RequiredActionProvider} is Stateless and can be reused.
 *
 * @author tdarimont
 */
public class LoginStatsRecordingRequiredActionProvider implements
RequiredActionProvider, RequiredActionFactory {

private static final Logger LOG =
Logger.getLogger(LoginStatsRecordingRequiredActionProvider.class);

private static final String PROVIDER_ID = "login_stats_action";
private static final String RECORD_LOGIN_STATISTICS_ACTION = "Record Login
Statistics Action";

private static final String LOGIN_LOGIN_COUNT = "login.login-count";
private static final String LOGIN_FAILED_LOGIN_COUNT =
"login.failed-login-count";
private static final String LOGIN_FAILED_LOGIN_DATE =
"login.failed-login-date";

private static final String LOGIN_FIRST_LOGIN_DATE =
"login.first-login-date";
private static final String LOGIN_RECENT_LOGIN_DATE =
"login.recent-login-date";

private static final String ONE = "1";
private static final String ZERO = "0";

private static final LoginStatsRecordingRequiredActionProvider INSTANCE =
new LoginStatsRecordingRequiredActionProvider();

@Override
public void close() {
// NOOP
}

@Override
public void evaluateTriggers(RequiredActionContext context) {

UserModel user = context.getUser();

try {
recordFirstLogin(user);
} catch (Exception ex) {
LOG.warnv(ex, "Couldn't record first login <{0}>", this);
}

try {
recordRecentLogin(user);
} catch (Exception ex) {
LOG.warnv(ex, "Couldn't record recent login <{0}>", this);
}

try {
recordLoginCount(user);
} catch (Exception ex) {
LOG.warnv(ex, "Couldn't record login count <{0}>", this);
}

try {
recordFailedLogin(user, context);
} catch (Exception ex) {
LOG.warnv(ex, "Couldn't record failed login <{0}>", this);
}
}

private void recordFailedLogin(UserModel user, RequiredActionContext
context) {

UserLoginFailureModel loginFailures = context.getSession().sessions()
.getUserLoginFailure(context.getRealm(), user.getUsername());

if (loginFailures != null) {
user.setAttribute(LOGIN_FAILED_LOGIN_COUNT,
Arrays.asList(String.valueOf(loginFailures.getNumFailures())));
user.setAttribute(LOGIN_FAILED_LOGIN_DATE,
Arrays.asList(getLocalDateTimeFromTimestamp(loginFailures.getLastFailure()).toString()));
} else {
user.setAttribute(LOGIN_FAILED_LOGIN_COUNT, Arrays.asList(ZERO));
}
}

private void recordLoginCount(UserModel user) {

List<String> list = user.getAttribute(LOGIN_LOGIN_COUNT);

if (list == null || list.isEmpty()) {
list = asList(ONE);
} else {
list = asList(String.valueOf(Long.parseLong(list.get(0)) + 1));
}

user.setAttribute(LOGIN_LOGIN_COUNT, list);
}

private void recordRecentLogin(UserModel user) {
user.setAttribute(LOGIN_RECENT_LOGIN_DATE,
asList(getLocalDateTimeFromTimestamp(System.currentTimeMillis()).toString()));
}

private void recordFirstLogin(UserModel user) {

List<String> list = user.getAttribute(LOGIN_FIRST_LOGIN_DATE);

if (list == null || list.isEmpty()) {
user.setAttribute(LOGIN_FIRST_LOGIN_DATE,
asList(getLocalDateTimeFromTimestamp(System.currentTimeMillis()).toString()));
}
}

@Override
public void requiredActionChallenge(RequiredActionContext context) {
// NOOP
}

@Override
public void processAction(RequiredActionContext context) {
context.success();
}

@Override
public RequiredActionProvider create(KeycloakSession session) {
return INSTANCE;
}

@Override
public void init(Scope config) {
LOG.infov("Creating IdM Keycloak extension <{0}>", this);
// NOOP
}

@Override
public void postInit(KeycloakSessionFactory factory) {
// NOOP
}

@Override
public String getId() {
return PROVIDER_ID;
}

@Override
public String getDisplayText() {
return RECORD_LOGIN_STATISTICS_ACTION;
}

private LocalDateTime getLocalDateTimeFromTimestamp(long timestampMillis) {
return LocalDateTime.ofInstant(Instant.ofEpochSecond(timestampMillis /
1000), TimeZone.getDefault().toZoneId());
}
}
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/keycloak-dev/attachments/20160708/23079270/attachment-0001.html