[keycloak-dev] TokenEndpoint is potentially too restrictive
Thomas Darimont
thomas.darimont at googlemail.com
Fri Jul 8 08:13:18 EDT 2016
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 at redhat.com>:
> 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
> 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 at lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/keycloak-dev/attachments/20160708/d327e437/attachment-0001.html
More information about the keycloak-dev
mailing list