From 05bac4ff0e433ea67fb5a3298f1c990bbfd315f2 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Fri, 28 Jul 2023 16:12:48 +0200 Subject: [PATCH] Remove option Nerver Expires for tokens in Advanced OIDC client configuration Closes https://github.com/keycloak/keycloak/issues/21927 --- .../topics/keycloak/changes-22_0_2.adoc | 3 ++ .../upgrading/topics/keycloak/changes.adoc | 4 ++ .../src/clients/advanced/AdvancedSettings.tsx | 6 ++- .../src/clients/advanced/TokenLifespan.tsx | 37 +++++++------- .../models/utils/SessionExpirationUtils.java | 48 ++++++++++++------- .../utils/SessionExpirationUtilsTest.java | 16 +++++++ 6 files changed, 76 insertions(+), 38 deletions(-) create mode 100644 docs/documentation/upgrading/topics/keycloak/changes-22_0_2.adoc diff --git a/docs/documentation/upgrading/topics/keycloak/changes-22_0_2.adoc b/docs/documentation/upgrading/topics/keycloak/changes-22_0_2.adoc new file mode 100644 index 00000000000..b32b6681d41 --- /dev/null +++ b/docs/documentation/upgrading/topics/keycloak/changes-22_0_2.adoc @@ -0,0 +1,3 @@ += Never expires option removed from client advanced settings combos + +The option `Never expires` is now removed from all the combos of the Advanced Settings client tab. This option was misleading because the different lifespans or idle timeouts were never infinite, but limited by the general user session or realm values. Therefore, this option is removed in favor of the other two remaining options: `Inherits from the realm settings` (the client uses general realm timeouts) and `Expires in` (the value is overriden for the client). Internally the `Never expires` was represented by `-1`. Now that value is shown with a warning in the Admin Console and cannot be set directly by the administrator. diff --git a/docs/documentation/upgrading/topics/keycloak/changes.adoc b/docs/documentation/upgrading/topics/keycloak/changes.adoc index 639b3bbbfc1..d195be9bccf 100644 --- a/docs/documentation/upgrading/topics/keycloak/changes.adoc +++ b/docs/documentation/upgrading/topics/keycloak/changes.adoc @@ -4,6 +4,10 @@ include::changes-23_0_0.adoc[leveloffset=3] +=== Migrating to 22.0.2 + +include::changes-22_0_2.adoc[leveloffset=3] + === Migrating to 22.0.0 include::changes-22_0_0.adoc[leveloffset=3] diff --git a/js/apps/admin-ui/src/clients/advanced/AdvancedSettings.tsx b/js/apps/admin-ui/src/clients/advanced/AdvancedSettings.tsx index 63b7391e645..1c32b881305 100644 --- a/js/apps/admin-ui/src/clients/advanced/AdvancedSettings.tsx +++ b/js/apps/admin-ui/src/clients/advanced/AdvancedSettings.tsx @@ -132,7 +132,11 @@ export const AdvancedSettings = ({ name={convertAttributeNameToForm( "attributes.client.offline.session.max.lifespan", )} - defaultValue={realm?.offlineSessionMaxLifespan} + defaultValue={ + realm?.offlineSessionMaxLifespanEnabled + ? realm?.offlineSessionMaxLifespan + : undefined + } units={["minute", "day", "hour"]} /> diff --git a/js/apps/admin-ui/src/clients/advanced/TokenLifespan.tsx b/js/apps/admin-ui/src/clients/advanced/TokenLifespan.tsx index 859ba5dd94f..738e8a566b9 100644 --- a/js/apps/admin-ui/src/clients/advanced/TokenLifespan.tsx +++ b/js/apps/admin-ui/src/clients/advanced/TokenLifespan.tsx @@ -24,7 +24,6 @@ type TokenLifespanProps = { }; const inherited = "tokenLifespan.inherited"; -const never = "tokenLifespan.never"; const expires = "tokenLifespan.expires"; export const TokenLifespan = ({ @@ -42,8 +41,8 @@ export const TokenLifespan = ({ const { control } = useFormContext(); const isExpireSet = (value: string | number) => - (typeof value === "number" && value !== -1) || - (typeof value === "string" && value !== "" && value !== "-1") || + typeof value === "number" || + (typeof value === "string" && value !== "") || focused; return ( @@ -73,30 +72,28 @@ export const TokenLifespan = ({ setOpen(false); }} selections={[ - isExpireSet(field.value) - ? t(expires) - : field.value === "" - ? t(inherited) - : t(never), + isExpireSet(field.value) ? t(expires) : t(inherited), ]} > {t(inherited)} - {t(never)} {t(expires)} - {field.value !== "-1" && field.value !== -1 && ( - - )} + )} diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/SessionExpirationUtils.java b/server-spi-private/src/main/java/org/keycloak/models/utils/SessionExpirationUtils.java index 66ea74a86f1..0186617a1cc 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/SessionExpirationUtils.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/SessionExpirationUtils.java @@ -21,6 +21,7 @@ import org.keycloak.models.ClientModel; import org.keycloak.models.Constants; import org.keycloak.models.RealmModel; import org.keycloak.protocol.oidc.OIDCConfigAttributes; +import org.keycloak.utils.StringUtil; /** *

Shared methods to calculate the session expiration and idle.

@@ -92,30 +93,29 @@ public class SessionExpirationUtils { long clientSessionCreated, long userSessionCreated, RealmModel realm, ClientModel client) { long timestamp = -1; if (offline) { - if (realm.isOfflineSessionMaxLifespanEnabled()) { - long clientOfflineSessionMaxLifespan = TimeUnit.SECONDS.toMillis(getOfflineSessionMaxLifespan(realm)); - - String clientOfflineSessionMaxLifespanPerClient = client == null? null : client.getAttribute(OIDCConfigAttributes.CLIENT_OFFLINE_SESSION_MAX_LIFESPAN); - if (clientOfflineSessionMaxLifespanPerClient != null && !clientOfflineSessionMaxLifespanPerClient.trim().isEmpty()) { - clientOfflineSessionMaxLifespan = TimeUnit.SECONDS.toMillis(Long.parseLong(clientOfflineSessionMaxLifespanPerClient)); + long clientOfflineSessionMaxLifespan = getClientAttributeTimeout(client, OIDCConfigAttributes.CLIENT_OFFLINE_SESSION_MAX_LIFESPAN); + if (realm.isOfflineSessionMaxLifespanEnabled() || clientOfflineSessionMaxLifespan > 0) { + if (clientOfflineSessionMaxLifespan > 0) { + clientOfflineSessionMaxLifespan = TimeUnit.SECONDS.toMillis(clientOfflineSessionMaxLifespan); } else if (realm.getClientOfflineSessionMaxLifespan() > 0) { clientOfflineSessionMaxLifespan = TimeUnit.SECONDS.toMillis(realm.getClientOfflineSessionMaxLifespan()); + } else { + clientOfflineSessionMaxLifespan = TimeUnit.SECONDS.toMillis(getOfflineSessionMaxLifespan(realm)); } - timestamp = clientSessionCreated + clientOfflineSessionMaxLifespan; long userSessionExpires = calculateUserSessionMaxLifespanTimestamp(offline, isRememberMe, userSessionCreated, realm); - timestamp = Math.min(timestamp, userSessionExpires); + timestamp = userSessionExpires > 0? Math.min(timestamp, userSessionExpires) : timestamp; } } else { long clientSessionMaxLifespan = TimeUnit.SECONDS.toMillis(getSsoSessionMaxLifespan(realm)); if (isRememberMe) { clientSessionMaxLifespan = Math.max(clientSessionMaxLifespan, TimeUnit.SECONDS.toMillis(realm.getSsoSessionMaxLifespanRememberMe())); } - String clientSessionMaxLifespanPerClient = client == null? null : client.getAttribute(OIDCConfigAttributes.CLIENT_SESSION_MAX_LIFESPAN); - if (clientSessionMaxLifespanPerClient != null && !clientSessionMaxLifespanPerClient.trim().isEmpty()) { - clientSessionMaxLifespan = TimeUnit.SECONDS.toMillis(Long.parseLong(clientSessionMaxLifespanPerClient)); + long clientSessionMaxLifespanPerClient = getClientAttributeTimeout(client, OIDCConfigAttributes.CLIENT_SESSION_MAX_LIFESPAN); + if (clientSessionMaxLifespanPerClient > 0) { + clientSessionMaxLifespan = TimeUnit.SECONDS.toMillis(clientSessionMaxLifespanPerClient); } else if (realm.getClientSessionMaxLifespan() > 0) { clientSessionMaxLifespan = TimeUnit.SECONDS.toMillis(realm.getClientSessionMaxLifespan()); } @@ -144,9 +144,9 @@ public class SessionExpirationUtils { long timestamp; if (offline) { long clientOfflineSessionIdleTimeout = TimeUnit.SECONDS.toMillis(getOfflineSessionIdleTimeout(realm)); - String clientOfflineSessionIdleTimeoutPerClient = client == null? null : client.getAttribute(OIDCConfigAttributes.CLIENT_OFFLINE_SESSION_IDLE_TIMEOUT); - if (clientOfflineSessionIdleTimeoutPerClient != null && !clientOfflineSessionIdleTimeoutPerClient.trim().isEmpty()) { - clientOfflineSessionIdleTimeout = TimeUnit.SECONDS.toMillis(Long.parseLong(clientOfflineSessionIdleTimeoutPerClient)); + long clientOfflineSessionIdleTimeoutPerClient = getClientAttributeTimeout(client, OIDCConfigAttributes.CLIENT_OFFLINE_SESSION_IDLE_TIMEOUT); + if (clientOfflineSessionIdleTimeoutPerClient > 0) { + clientOfflineSessionIdleTimeout = TimeUnit.SECONDS.toMillis(clientOfflineSessionIdleTimeoutPerClient); } else if (realm.getClientOfflineSessionIdleTimeout() > 0) { clientOfflineSessionIdleTimeout = TimeUnit.SECONDS.toMillis(realm.getClientOfflineSessionIdleTimeout()); } @@ -157,9 +157,9 @@ public class SessionExpirationUtils { if (isRememberMe) { clientSessionIdleTimeout = Math.max(clientSessionIdleTimeout, TimeUnit.SECONDS.toMillis(realm.getSsoSessionIdleTimeoutRememberMe())); } - String clientSessionIdleTimeoutPerClient = client == null? null : client.getAttribute(OIDCConfigAttributes.CLIENT_SESSION_IDLE_TIMEOUT); - if (clientSessionIdleTimeoutPerClient != null && !clientSessionIdleTimeoutPerClient.trim().isEmpty()) { - clientSessionIdleTimeout = TimeUnit.SECONDS.toMillis(Long.parseLong(clientSessionIdleTimeoutPerClient)); + long clientSessionIdleTimeoutPerClient = getClientAttributeTimeout(client, OIDCConfigAttributes.CLIENT_SESSION_IDLE_TIMEOUT); + if (clientSessionIdleTimeoutPerClient > 0) { + clientSessionIdleTimeout = TimeUnit.SECONDS.toMillis(clientSessionIdleTimeoutPerClient); } else if (realm.getClientSessionIdleTimeout() > 0){ clientSessionIdleTimeout = TimeUnit.SECONDS.toMillis(realm.getClientSessionIdleTimeout()); } @@ -200,4 +200,18 @@ public class SessionExpirationUtils { } return idle; } + + private static long getClientAttributeTimeout(ClientModel client, String attr) { + if (client != null) { + final String value = client.getAttribute(attr); + if (StringUtil.isNotBlank(value)) { + try { + return Long.parseLong(value); + } catch (NumberFormatException e) { + // no-op + } + } + } + return -1; + } } diff --git a/server-spi-private/src/test/java/org/keycloak/models/utils/SessionExpirationUtilsTest.java b/server-spi-private/src/test/java/org/keycloak/models/utils/SessionExpirationUtilsTest.java index 07bb52b6752..0a2461463a5 100644 --- a/server-spi-private/src/test/java/org/keycloak/models/utils/SessionExpirationUtilsTest.java +++ b/server-spi-private/src/test/java/org/keycloak/models/utils/SessionExpirationUtilsTest.java @@ -180,6 +180,9 @@ public class SessionExpirationUtilsTest { realmMap.put("getSsoSessionMaxLifespan", 1000); realmMap.put("getSsoSessionMaxLifespanRememberMe", 2000); Assert.assertEquals(2000 * 1000L, SessionExpirationUtils.calculateClientSessionMaxLifespanTimestamp(false, true, t, t, realm, client) - t); + // set -1 in the client and should be not taken into account + clientMap.put(OIDCConfigAttributes.CLIENT_SESSION_MAX_LIFESPAN, "-1"); + Assert.assertEquals(2000 * 1000L, SessionExpirationUtils.calculateClientSessionMaxLifespanTimestamp(false, true, t, t, realm, client) - t); } @Test @@ -207,6 +210,13 @@ public class SessionExpirationUtilsTest { long t2 = t - 100; realmMap.put("getOfflineSessionMaxLifespan", 2000); Assert.assertEquals(2000 * 1000L, SessionExpirationUtils.calculateClientSessionMaxLifespanTimestamp(true, false, t, t2, realm, client) - t2); + // set -1 in the client and should be not taken into account + clientMap.put(OIDCConfigAttributes.CLIENT_OFFLINE_SESSION_MAX_LIFESPAN, "-1"); + Assert.assertEquals(2000 * 1000L, SessionExpirationUtils.calculateClientSessionMaxLifespanTimestamp(true, false, t, t, realm, client) - t); + // set no expiration at realm but set expiration at client level + realmMap.put("isOfflineSessionMaxLifespanEnabled", false); + clientMap.put(OIDCConfigAttributes.CLIENT_OFFLINE_SESSION_MAX_LIFESPAN, "2000"); + Assert.assertEquals(2000 * 1000L, SessionExpirationUtils.calculateClientSessionMaxLifespanTimestamp(true, false, t, t, realm, client) - t); } @Test @@ -230,6 +240,9 @@ public class SessionExpirationUtilsTest { // override value in client clientMap.put(OIDCConfigAttributes.CLIENT_SESSION_IDLE_TIMEOUT, "3000"); Assert.assertEquals(3000 * 1000L, SessionExpirationUtils.calculateClientSessionIdleTimestamp(false, false, t, realm, client) - t); + // set -1 in the client and should be not taken into account + clientMap.put(OIDCConfigAttributes.CLIENT_SESSION_IDLE_TIMEOUT, "-1"); + Assert.assertEquals(4000 * 1000L, SessionExpirationUtils.calculateClientSessionIdleTimestamp(false, false, t, realm, client) - t); } @Test @@ -253,5 +266,8 @@ public class SessionExpirationUtilsTest { // override value in client clientMap.put(OIDCConfigAttributes.CLIENT_OFFLINE_SESSION_IDLE_TIMEOUT, "3000"); Assert.assertEquals(3000 * 1000L, SessionExpirationUtils.calculateClientSessionIdleTimestamp(true, false, t, realm, client) - t); + // set -1 in the client and should be not taken into account + clientMap.put(OIDCConfigAttributes.CLIENT_OFFLINE_SESSION_IDLE_TIMEOUT, "-1"); + Assert.assertEquals(4000 * 1000L, SessionExpirationUtils.calculateClientSessionIdleTimestamp(true, false, t, realm, client) - t); } }