[keycloak-dev] TokenEndpoint is potentially too restrictive
Stian Thorgersen
sthorger at redhat.com
Mon Jul 11 06:10:11 EDT 2016
A required action is aimed at an action a user has to take, not as a way to
listen for events. So you'd end up with a null-op action just so that you
can log an event.
On 11 July 2016 at 10:28, Thomas Darimont <thomas.darimont at googlemail.com>
wrote:
> Hi,
>
> an EventListener would work as well - but in this case RequiredAction was
> IMHO simpler than a custom EventListener (less ceremony).
> Besides the configuration gotcha discussed above, do you see any advantage
> of using an EventListener here instead of an RequiredAction?
>
> Cheers,
> Thomas
>
> 2016-07-11 8:08 GMT+02:00 Stian Thorgersen <sthorger at redhat.com>:
>
>> Would it not be better to use an event listener for this?
>>
>> On 8 July 2016 at 14:13, Thomas Darimont <thomas.darimont at googlemail.com>
>> 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 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
>>>>
>>>>
>>>>
>>>
>>> _______________________________________________
>>> 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/20160711/8f3975bd/attachment-0001.html
More information about the keycloak-dev
mailing list