Re: [keycloak-dev] TokenEndpoint is potentially too restrictive

Would it not be better to use an event listener for this? On 8 July 2016 at 14:13, Thomas Darimont <thomas.darimont(a)googlemail.com&gt; wrote: > Thanks Marek, that makes sense now. > > Removing the "default" checkbox on my provider did the trick - yes I want > to execute this action for every user regardless on login in the sense of > an filter / interceptor. > > Btw. this use case I implemented is IMHO quite common - would be cool if > keycloak would ship with something like that out-of-the-box ;-) > > Cheers, > Thomas > > 2016-07-08 13:39 GMT+02:00 Marek Posolda <mposolda(a)redhat.com&gt;: > >> 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/bd2887aa77184d82e795e4200eb55a3... >> >> 1.9.x >> >> https://github.com/keycloak/keycloak/blob/e7822431fded5948a5e248766e6ffbf... >> >> 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" >> <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 listkeycloak-dev@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev >> >> >> > > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev >