From 1f895f6f6dcdbdf7bc98aa27618fa4eff114c947 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Mon, 6 Nov 2023 15:54:38 -0800 Subject: [PATCH] feat: add config to control "OIDC RP-initiated logout" - New config "performRpInitiatedLogout" (default to true to preserve existing behavior) added to /identity-providers API and uaa.yml. It is a flag controlling whether to log out of the external provider after a successful UAA logout per [OIDC RP-Initiated Logout](https://openid.net/specs/openid-connect-rpinitiated-1_0.html)" - doc changes [more context: https://github.com/cloudfoundry/uaa/issues/2589] [#184752215] --- ...actExternalOAuthIdentityProviderDefinition.java | 11 +++++++++++ .../ZoneAwareWhitelistLogoutHandler.java | 7 ++++--- .../provider/oauth/ExternalOAuthLogoutHandler.java | 7 +++++++ .../provider/oauth/OauthIDPWrapperFactoryBean.java | 3 +++ .../ZoneAwareWhitelistLogoutHandlerTests.java | 11 ++++++++++- .../oauth/ExternalOAuthLogoutHandlerTest.java | 11 +++++++++++ ...hIdentityProviderDefinitionFactoryBeanTest.java | 14 ++++++++++++++ uaa/slateCustomizations/source/index.html.md.erb | 2 +- .../uaa/integration/feature/OIDCLoginIT.java | 12 ++++++++++++ .../providers/IdentityProviderEndpointDocs.java | 4 ++++ 10 files changed, 77 insertions(+), 5 deletions(-) diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java b/model/src/main/java/org/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java index ce40fd96bf0..1a121524313 100644 --- a/model/src/main/java/org/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java +++ b/model/src/main/java/org/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java @@ -46,6 +46,7 @@ public enum OAuthGroupMappingMode { private String userPropagationParameter; private OAuthGroupMappingMode groupMappingMode; private boolean pkce = true; + private boolean performRpInitiatedLogout = true; public URL getAuthUrl() { return authUrl; @@ -214,6 +215,14 @@ public boolean isPkce() { return this.pkce; } + public boolean isPerformRpInitiatedLogout() { + return performRpInitiatedLogout; + } + + public void setPerformRpInitiatedLogout(boolean performRpInitiatedLogout) { + this.performRpInitiatedLogout = performRpInitiatedLogout; + } + @JsonIgnore public Class getParameterizedClass() { ParameterizedType parameterizedType = @@ -247,6 +256,7 @@ public boolean equals(Object o) { if (!Objects.equals(userPropagationParameter, that.userPropagationParameter)) return false; if (!Objects.equals(groupMappingMode, that.groupMappingMode)) return false; if (pkce != that.pkce) return false; + if (performRpInitiatedLogout != that.performRpInitiatedLogout) return false; return Objects.equals(responseType, that.responseType); } @@ -271,6 +281,7 @@ public int hashCode() { result = 31 * result + (groupMappingMode != null ? groupMappingMode.hashCode() : 0); result = 31 * result + (responseType != null ? responseType.hashCode() : 0); result = 31 * result + (pkce ? 1 : 0); + result = 31 * result + (performRpInitiatedLogout ? 1 : 0); return result; } } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandler.java b/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandler.java index 8eebec26c62..dabc6263e42 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandler.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandler.java @@ -47,11 +47,12 @@ public ZoneAwareWhitelistLogoutHandler(MultitenantClientServices clientDetailsSe public void onLogoutSuccess(HttpServletRequest request, HttpServletResponse response, Authentication authentication) throws IOException, ServletException { AbstractExternalOAuthIdentityProviderDefinition oauthConfig = externalOAuthLogoutHandler.getOAuthProviderForAuthentication(authentication); String logoutUrl = externalOAuthLogoutHandler.getLogoutUrl(oauthConfig); + Boolean shouldPerformRpInitiatedLogout = externalOAuthLogoutHandler.getPerformRpInitiatedLogout(oauthConfig); - if (logoutUrl == null) { - getZoneHandler().onLogoutSuccess(request, response, authentication); - } else { + if (shouldPerformRpInitiatedLogout && logoutUrl != null) { externalOAuthLogoutHandler.onLogoutSuccess(request, response, authentication); + } else { + getZoneHandler().onLogoutSuccess(request, response, authentication); } } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandler.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandler.java index d331938cb91..0b07251ca91 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandler.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandler.java @@ -123,4 +123,11 @@ private String getZoneDefaultUrl() { } return config.getLinks().getLogout().getRedirectUrl(); } + + public Boolean getPerformRpInitiatedLogout(AbstractExternalOAuthIdentityProviderDefinition oauthConfig) { + if (oauthConfig == null) { + return false; + } + return oauthConfig.isPerformRpInitiatedLogout(); + } } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java index 887f957c844..198dd94286a 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java @@ -166,6 +166,9 @@ protected void setCommonProperties(Map idpDefinitionMap, Abstrac if (idpDefinitionMap.get("clientAuthInBody") instanceof Boolean) { idpDefinition.setClientAuthInBody((boolean)idpDefinitionMap.get("clientAuthInBody")); } + if (idpDefinitionMap.get("performRpInitiatedLogout") instanceof Boolean) { + idpDefinition.setPerformRpInitiatedLogout((boolean)idpDefinitionMap.get("performRpInitiatedLogout")); + } } private static Map parseAdditionalParameters(Map idpDefinitionMap) { diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandlerTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandlerTests.java index 451bfef7099..62ea712e817 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandlerTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandlerTests.java @@ -163,12 +163,21 @@ public void test_external_client_redirect() { } @Test - public void test_exteral_logout() throws ServletException, IOException { + public void test_external_logout() throws ServletException, IOException { when(oAuthLogoutHandler.getLogoutUrl(null)).thenReturn(""); + when(oAuthLogoutHandler.getPerformRpInitiatedLogout(null)).thenReturn(true); handler.onLogoutSuccess(request, response, null); verify(oAuthLogoutHandler, times(1)).onLogoutSuccess(request, response, null); } + @Test + public void test_does_not_external_logout() throws ServletException, IOException { + when(oAuthLogoutHandler.getLogoutUrl(null)).thenReturn(""); + when(oAuthLogoutHandler.getPerformRpInitiatedLogout(null)).thenReturn(false); + handler.onLogoutSuccess(request, response, null); + verify(oAuthLogoutHandler, times(0)).onLogoutSuccess(request, response, null); + } + @Test public void test_logout() throws ServletException, IOException { handler.onLogoutSuccess(request, response, null); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandlerTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandlerTest.java index 1c6a99f7969..a0ea05121ad 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandlerTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandlerTest.java @@ -134,4 +134,15 @@ void getOAuthProviderForAuthentication() { void getNullOAuthProviderForAuthentication() { assertEquals(null, oAuthLogoutHandler.getOAuthProviderForAuthentication(null)); } + + @Test + void getPerformRpInitiatedLogout() { + oAuthIdentityProviderDefinition.setPerformRpInitiatedLogout(true); + assertEquals(true, oAuthLogoutHandler.getPerformRpInitiatedLogout(oAuthIdentityProviderDefinition)); + + oAuthIdentityProviderDefinition.setPerformRpInitiatedLogout(false); + assertEquals(false, oAuthLogoutHandler.getPerformRpInitiatedLogout(oAuthIdentityProviderDefinition)); + + assertEquals(false, oAuthLogoutHandler.getPerformRpInitiatedLogout(null)); + } } \ No newline at end of file diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIdentityProviderDefinitionFactoryBeanTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIdentityProviderDefinitionFactoryBeanTest.java index ff7a1ec05b4..b0ed32d74d1 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIdentityProviderDefinitionFactoryBeanTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIdentityProviderDefinitionFactoryBeanTest.java @@ -234,4 +234,18 @@ public void testNoAdditionalParametersInConfig() { Map receivedParameters = ((OIDCIdentityProviderDefinition) factoryBean.getProviders().get(0).getProvider().getConfig()).getAdditionalAuthzParameters(); assertEquals(0, receivedParameters.size()); } + + @Test + public void testPerformRpInitiatedLogoutTrue() { + idpDefinitionMap.put("performRpInitiatedLogout", true); + factoryBean.setCommonProperties(idpDefinitionMap, providerDefinition); + assertTrue(providerDefinition.isPerformRpInitiatedLogout()); + } + + @Test + public void testPerformRpInitiatedLogoutFalse() { + idpDefinitionMap.put("performRpInitiatedLogout", false); + factoryBean.setCommonProperties(idpDefinitionMap, providerDefinition); + assertFalse(providerDefinition.isPerformRpInitiatedLogout()); + } } diff --git a/uaa/slateCustomizations/source/index.html.md.erb b/uaa/slateCustomizations/source/index.html.md.erb index 181da452557..e681802d83e 100644 --- a/uaa/slateCustomizations/source/index.html.md.erb +++ b/uaa/slateCustomizations/source/index.html.md.erb @@ -635,7 +635,7 @@ _Error Codes_ ## Logout.do The logout endpoint is meant to be used by applications to log the user out of the UAA session. UAA will only log a user out of the UAA session if they also hit this endpoint, and may also perform Single Logout with SAML providers if configured to do so. -UAA will also log users out of OIDC proxied authenticated sessions based on [OpenID Connect Session Management](https://openid.net/specs/openid-connect-session-1_0.html). +UAA may also log users out of OIDC proxied authenticated sessions based on [OpenID Connect Session Management](https://openid.net/specs/openid-connect-session-1_0.html) if configured to do so. The recommendation for application authors is to: * provide a local logout feature specific to the client application diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/OIDCLoginIT.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/OIDCLoginIT.java index 6b1b647b647..a8ad919c2a9 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/OIDCLoginIT.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/OIDCLoginIT.java @@ -569,6 +569,18 @@ public void successfulUaaLogoutTriggersExternalOIDCProviderLogout() { Assert.assertThat("URL validation failed", webDriver.getCurrentUrl(), endsWith("/login")); } + @Test + public void successfulUaaLogoutDoesNotTriggersExternalOIDCProviderLogout_whenConfiguredNotTo() { + identityProvider.getConfig().setPerformRpInitiatedLogout(false); + updateProvider(); + + validateSuccessfulOIDCLogin(zoneUrl, testAccounts.getUserName(), testAccounts.getPassword()); + + String externalOIDCProviderLoginPage = baseUrl; + webDriver.get(externalOIDCProviderLoginPage); + Assert.assertThat(webDriver.getPageSource(), containsString("Where to?")); + } + private String getRefreshTokenResponse(ServerRunning serverRunning, String refreshToken) { MultiValueMap formData = new LinkedMultiValueMap<>(); formData.add("client_id", zoneClient.getClientId()); diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointDocs.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointDocs.java index b03b9c75605..45cd5ccf382 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointDocs.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointDocs.java @@ -457,6 +457,7 @@ void createOAuthIdentityProvider() throws Exception { definition.setAttributeMappings(getAttributeMappingMap()); definition.setUserPropagationParameter("username"); definition.setPkce(true); + definition.setPerformRpInitiatedLogout(true); identityProvider.setConfig(definition); identityProvider.setSerializeConfigRaw(true); @@ -478,6 +479,7 @@ void createOAuthIdentityProvider() throws Exception { fieldWithPath("config.responseType").optional("code").type(STRING).description("Response type for the authorize request, will be sent to OAuth server, defaults to `code`"), fieldWithPath("config.clientAuthInBody").optional(false).type(BOOLEAN).description("Sends the client credentials in the token retrieval call as body parameters instead of a Basic Authorization header."), fieldWithPath("config.pkce").optional(true).type(BOOLEAN).description("A flag controlling whether PKCE (RFC 7636) is active in authorization code flow when requesting tokens from the external provider."), + fieldWithPath("config.performRpInitiatedLogout").optional(true).type(BOOLEAN).description("A flag controlling whether to log out of the external provider after a successful UAA logout per [OIDC RP-Initiated Logout](https://openid.net/specs/openid-connect-rpinitiated-1_0.html)"), fieldWithPath("config.issuer").optional(null).type(STRING).description("The OAuth 2.0 token issuer. This value is used to validate the issuer inside the token."), fieldWithPath("config.userPropagationParameter").optional("username").type(STRING).description("Name of the request parameter that is used to pass a known username when redirecting to this identity provider from the account chooser"), fieldWithPath("config.attributeMappings.user_name").optional("sub").type(STRING).description("Map `user_name` to the attribute for user name in the provider assertion or token. The default for OpenID Connect is `sub`"), @@ -528,6 +530,7 @@ void createOidcIdentityProvider() throws Exception { definition.setRelyingPartySecret("secret"); definition.setShowLinkText(false); definition.setPkce(true); + definition.setPerformRpInitiatedLogout(true); definition.setAttributeMappings(getAttributeMappingMap()); definition.setUserPropagationParameter("username"); definition.setExternalGroupsWhitelist(Collections.singletonList("uaa.user")); @@ -555,6 +558,7 @@ void createOidcIdentityProvider() throws Exception { fieldWithPath("config.checkTokenUrl").optional(null).type(OBJECT).description("Reserved for future OAuth/OIDC use."), fieldWithPath("config.clientAuthInBody").optional(false).type(BOOLEAN).description("Only effective if relyingPartySecret is defined. Sends the client credentials in the token retrieval call as body parameters instead of a Basic Authorization header. It is recommended to set `jwtClientAuthentication:true` instead."), fieldWithPath("config.pkce").optional(true).type(BOOLEAN).description("A flag controlling whether PKCE (RFC 7636) is active in authorization code flow when requesting tokens from the external provider."), + fieldWithPath("config.performRpInitiatedLogout").optional(true).type(BOOLEAN).description("A flag controlling whether to log out of the external provider after a successful UAA logout per [OIDC RP-Initiated Logout](https://openid.net/specs/openid-connect-rpinitiated-1_0.html)"), fieldWithPath("config.userInfoUrl").optional(null).type(OBJECT).description("Reserved for future OIDC use. This can be left blank if a discovery URL is provided. If both are provided, this property overrides the discovery URL."), fieldWithPath("config.logoutUrl").optional(null).type(OBJECT).description("OIDC logout endpoint. This can be left blank if a discovery URL is provided. If both are provided, this property overrides the discovery URL."), fieldWithPath("config.responseType").optional("code").type(STRING).description("Response type for the authorize request, defaults to `code`, but can be `code id_token` if the OIDC server can return an id_token as a query parameter in the redirect."),