KEYCLOAK-1267 Add dedicated SSO timeouts for Remember-Me by thomasdarimont · Pull Request #3161 · keycloak/keycloak

thomasdarimont · 2016-08-22T22:48:10Z

Previously remember-me sessions where tied to the SSO max session
timeout which could lead to unexpected early session timeouts.
We now allow SSO timeouts to be configured separately for sessions
with enabled remember-me.
This enables users to opt-in for longer session timeouts.

SSO Session Timeouts for RememberMe can now be configured in the
tokens tab in the realm admin console. Max SSO timeouts for Remember-me
need to be equal or greater than normal max SSO timeouts.

Note that most of the work for this PR was done by @cfsnyder and
is based on his earlier PR #2338.
I just adapted his code to the latest Keycloak API changes.

Signed-off-by: Thomas Darimont [email protected]

cfsnyder · 2016-08-23T13:29:22Z

Awesome @thomasdarimont, thanks for following up and updating that one!

stianst

Sorry that it took so long to get back to you on this one. Looks OK, but I've added a couple comments inline. We need 'ssoSessionIdleTimeoutRememberMe' as well. This gives more flexibility in how to configure things. For example remember me allows cookie to last 365 days, but still needs a user to use the session at least every 30 days, while a non-remember me session is 30 days and idle 1 day.

stianst · 2016-09-20T07:03:34Z

Please don't introduce extra changeSet for a specific version, we only want one changeset per-version. Just append to existing changeSet element if there is one.

Okay, I moved the changes to the current changelog.

stianst · 2016-09-20T07:06:02Z

Would be simpler to allow null and use ssoSessionMaxLifespan if not specified. Having SQL in these files worries me for compatibility reasons (in general, I think this particular SQL is fine though).

@cfsnyder

Previosuly remember-me sessions where tied to the SSO max session timeout which could lead to unexpected early session timeouts. We now allow SSO timeouts to be configured separately for sessions with enabled remember-me. This enables users to opt-in for longer session timeouts. SSO Session Timeouts for RememberMe can now be configured in the tokens tab in the realm admin console. Max SSO timeouts for Remember-me need to be equal or greater than normal max SSO timeouts. Note that most of the work for this PR was done by @cfsnyder and is based on his earlier PR keycloak#2338. I just adapted his code to the latest Keycloak API changes. Signed-off-by: Thomas Darimont <[email protected]>

thomasdarimont · 2016-10-03T13:47:56Z

FYI I updated the PR with an additional idle timeout for sso remember me.
Would you mind having a look @stianst and @cfsnyder ?

We now allow to control the idle timeout for a SSO session created with "remember me". Modified the jpa-changelog according to discussions in keycloak#3161.

thomasdarimont · 2016-10-03T13:49:38Z

should the default be 1 hour or 10 hours (as in the examples above e.g.: basicauthrealm.json)?

thomasdarimont · 2016-10-03T13:50:00Z

I'm not sure what a better default would be here...

thomasdarimont · 2016-10-03T13:52:18Z

         </addColumn>
+
+         <addColumn tableName="REALM">
+             <column name="SSO_MAX_LIFESPAN_REMEMBER_ME" type="INT" defaultValueNumeric="36000"/>


set this to 10 hours by default - can anyone think of a better default?

I don't like having a default for this. Rather values for remember me should be optional and it should use non-remember me values if not specified.

thomasdarimont · 2016-10-03T13:53:51Z

+        boolean sessionMaxOk = (userSession.getStarted() + (userSession.isRememberMe() ? realm.getSsoSessionMaxLifespanRememberMe() : realm.getSsoSessionMaxLifespan())) > currentTime;
+        int maxRememberMeIdleSessionAge = userSession.getLastSessionRefresh() + realm.getSsoSessionIdleTimeoutRememberMe();
+        int maxSsoSessionAge = userSession.getLastSessionRefresh() + realm.getSsoSessionIdleTimeout();
+        boolean sessionIdleOk = (userSession.isRememberMe() && maxRememberMeIdleSessionAge > currentTime) || maxSsoSessionAge > currentTime;


Note that I now take into account the ssoSessionIdleTimeoutRememberMe

stianst

A few comments inline. In general I think it would be better to use non-remember me values by default and optionally allow setting different values for remember-me.

stianst · 2016-10-28T10:14:54Z

    "accessCodeLifespanUserAction": 300,
    "ssoSessionIdleTimeout": 600,
    "ssoSessionMaxLifespan": 36000,
+    "ssoSessionIdleTimeoutRememberMe": 36000,


Don't add to existing realm files. That should not be required.

stianst · 2016-10-28T10:14:58Z

    "accessCodeLifespanUserAction": 300,
    "ssoSessionIdleTimeout": 600,
    "ssoSessionMaxLifespan": 36000,
+    "ssoSessionIdleTimeoutRememberMe": 36000,


Don't add to existing realm files. That should not be required.

stianst · 2016-10-28T10:15:02Z

    "accessCodeLifespanUserAction": 300,
    "ssoSessionIdleTimeout": 600,
    "ssoSessionMaxLifespan": 36000,
+    "ssoSessionIdleTimeoutRememberMe": 36000,


Don't add to existing realm files. That should not be required.

stianst · 2016-10-28T10:15:06Z

    "accessCodeLifespanUserAction": 300,
    "ssoSessionIdleTimeout": 600,
    "ssoSessionMaxLifespan": 36000,
+    "ssoSessionIdleTimeoutRememberMe": 36000,


Don't add to existing realm files. That should not be required.

stianst · 2016-10-28T10:15:48Z


        <addColumn tableName="ADMIN_EVENT_ENTITY">
-            <column name="RESOURCE_TYPE" type="VARCHAR(64)"></column>
+            <column name="RESOURCE_TYPE" type="VARCHAR(64)"/>


Don't change code that is not relevant to your changes

stianst · 2016-10-28T10:18:21Z

                .ssoSessionIdleTimeout(3000)
                .accessTokenLifespan(10000)
                .ssoSessionMaxLifespan(10000)
+                .ssoSessionMaxLifespanRememberMe(10000)


Shouldn't be required to set this in existing tests. This test has nothing to do with your changes.

stianst · 2016-10-28T10:18:46Z

  "registrationAllowed": true,
  "resetPasswordAllowed": true,
  "editUsernameAllowed" : true,
+  "rememberMe" : false,


Why are you changing this?

stianst · 2016-10-28T10:19:08Z


    public void loginToMasterRealmAdminConsoleAs(UserRepresentation user) {
-        loginToAdminConsoleAs(adminConsolePage, loginPage, user);
+        loginToAdminConsoleAs(adminConsolePage, loginPage, user, false);


I don't understand the point in adding this. This is not the correct place to test remember me.

stianst · 2016-10-28T10:19:34Z

  "accessTokenLifespanForImplicitFlow" : 900,
  "ssoSessionIdleTimeout" : 2,
  "ssoSessionMaxLifespan" : 36000,
+  "ssoSessionIdleTimeoutRememberMe" : 36000,


Please don't add to irrelevant tests

stianst · 2016-10-28T10:20:20Z

        testStrategy.testLoginSSOMax();
    }

+    @Test


Adapter tests are not the correct place to test this. Rather use LoginTest or something else from the base tests inside the new Arquillian testsuite. Please remove all changes to the adapter tests inside testsuite/integration

Previously remember-me sessions where tied to the SSO max session timeout which could lead to unexpected early session timeouts. We now allow SSO timeouts to be configured separately for sessions with enabled remember-me. This enables users to opt-in for longer session timeouts. SSO session timeouts for remember-me can now be configured in the tokens tab in the realm admin console. This new configuration is optional and will tipically host values larger than the regular max SSO timeouts. If no value is specified for remember-me timeouts then the regular max SSO timeouts will be used. Work based on PR keycloak#3161 by Thomas Darimont <[email protected]>

Previously remember-me sessions where tied to the SSO max session timeout which could lead to unexpected early session timeouts. We now allow SSO timeouts to be configured separately for sessions with enabled remember-me. This enables users to opt-in for longer session timeouts. SSO session timeouts for remember-me can now be configured in the tokens tab in the realm admin console. This new configuration is optional and will tipically host values larger than the regular max SSO timeouts. If no value is specified for remember-me timeouts then the regular max SSO timeouts will be used. Work based on PR #3161 by Thomas Darimont <[email protected]>

Previously remember-me sessions where tied to the SSO max session timeout which could lead to unexpected early session timeouts. We now allow SSO timeouts to be configured separately for sessions with enabled remember-me. This enables users to opt-in for longer session timeouts. SSO session timeouts for remember-me can now be configured in the tokens tab in the realm admin console. This new configuration is optional and will tipically host values larger than the regular max SSO timeouts. If no value is specified for remember-me timeouts then the regular max SSO timeouts will be used. Work based on PR keycloak#3161 by Thomas Darimont <[email protected]>

thomasdarimont force-pushed the issue/KEYCLOAK-1267-improve-rememberme branch from 08be42b to b9a4ecc Compare September 15, 2016 09:41

stianst self-assigned this Sep 20, 2016

stianst reviewed Sep 20, 2016

View reviewed changes

stianst added the Incomplete label Sep 20, 2016

thomasdarimont force-pushed the issue/KEYCLOAK-1267-improve-rememberme branch from b9a4ecc to 8717142 Compare October 3, 2016 13:46

KEYCLOAK-1267 Add SSO idle timeouts for Remember-Me

d18b899

We now allow to control the idle timeout for a SSO session created with "remember me". Modified the jpa-changelog according to discussions in keycloak#3161.

thomasdarimont force-pushed the issue/KEYCLOAK-1267-improve-rememberme branch from 8717142 to d18b899 Compare October 3, 2016 13:51

thomasdarimont commented Oct 3, 2016

View reviewed changes

stianst reviewed Oct 28, 2016

View reviewed changes

stianst added 3.x and removed Incomplete labels Nov 28, 2016

stianst closed this Mar 14, 2017

sguilhen mentioned this pull request Nov 14, 2018

KEYCLOAK-1267 Add dedicated SSO timeouts for Remember-Me #5718

Merged

Conversation

thomasdarimont commented Aug 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfsnyder commented Aug 23, 2016

Uh oh!

stianst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasdarimont commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stianst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thomasdarimont commented Aug 22, 2016 •

edited

Loading