[keycloak-dev] TokenEndpoint is potentially too restrictive

Fri Jul 8 07:39:43 EDT 2016

There was already some quite a long discussion about similar stuff. It 
was about consents though, not about required actions. However IMO both 
consents and requiredActions are quite similar thing and it's by design 
that user is not able to login with directGrant if he has either consent 
or requiredAction on himself.

However when I look at your particular use-case, it seems that you are 
using LoginStatsRecordingRequiredActionProvider as an interceptor, which 
doesn't need any real requiredAction on user, but you just want to 
ensure that "evaluateTriggers" is called after each user login. Is it 
correct?

Then you can just check your requiredAction provider as "enabled", but 
NOT as "default" . Note that "evaluateTriggers" will be always invoked 
for every user after his login even if user doesn't have the particular 
action on him. The purpose of "evaluateTriggers" is actually to check, 
if requiredAction should be added to the user if some specific 
conditions occurs. For example see VerifyEmail.evaluateTriggers, which 
adds the VERIFY_EMAIL requiredAction to user if he doesn't yet have 
verified email.

Marek

On 08/07/16 10:35, Thomas Darimont wrote:
> 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 <http://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());
> }
> }
>
>
>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/keycloak-dev/attachments/20160708/dd326215/attachment-0001.html 