Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider #2590

Merged
merged 7 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -214,6 +215,14 @@ public boolean isPkce() {
return this.pkce;
}

public boolean isPerformRpInitiatedLogout() {
peterhaochen47 marked this conversation as resolved.
Show resolved Hide resolved
return performRpInitiatedLogout;
}

public void setPerformRpInitiatedLogout(boolean performRpInitiatedLogout) {
this.performRpInitiatedLogout = performRpInitiatedLogout;
}

@JsonIgnore
public Class getParameterizedClass() {
ParameterizedType parameterizedType =
Expand Down Expand Up @@ -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);

}
Expand All @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import org.cloudfoundry.identity.uaa.oauth.KeyInfoService;
import org.cloudfoundry.identity.uaa.provider.AbstractExternalOAuthIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.provider.OIDCIdentityProviderDefinition;
import org.cloudfoundry.identity.uaa.provider.oauth.ExternalOAuthLogoutHandler;
import org.cloudfoundry.identity.uaa.zone.MultitenantClientServices;
import org.cloudfoundry.identity.uaa.zone.IdentityZoneConfiguration;
Expand Down Expand Up @@ -45,13 +46,14 @@ public ZoneAwareWhitelistLogoutHandler(MultitenantClientServices clientDetailsSe

@Override
public void onLogoutSuccess(HttpServletRequest request, HttpServletResponse response, Authentication authentication) throws IOException, ServletException {
AbstractExternalOAuthIdentityProviderDefinition oauthConfig = externalOAuthLogoutHandler.getOAuthProviderForAuthentication(authentication);
AbstractExternalOAuthIdentityProviderDefinition<OIDCIdentityProviderDefinition> oauthConfig = externalOAuthLogoutHandler.getOAuthProviderForAuthentication(authentication);
String logoutUrl = externalOAuthLogoutHandler.getLogoutUrl(oauthConfig);
boolean shouldPerformRpInitiatedLogout = externalOAuthLogoutHandler.getPerformRpInitiatedLogout(oauthConfig);

if (logoutUrl == null) {
getZoneHandler().onLogoutSuccess(request, response, authentication);
bruce-ricard marked this conversation as resolved.
Show resolved Hide resolved
} else {
if (shouldPerformRpInitiatedLogout && logoutUrl != null) {
externalOAuthLogoutHandler.onLogoutSuccess(request, response, authentication);
} else {
getZoneHandler().onLogoutSuccess(request, response, authentication);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,11 @@ private String getZoneDefaultUrl() {
}
return config.getLinks().getLogout().getRedirectUrl();
}

public boolean getPerformRpInitiatedLogout(AbstractExternalOAuthIdentityProviderDefinition<OIDCIdentityProviderDefinition> oauthConfig) {
if (oauthConfig == null) {
return false;
}
return oauthConfig.isPerformRpInitiatedLogout();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ protected void setCommonProperties(Map<String, Object> 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"));
}
bruce-ricard marked this conversation as resolved.
Show resolved Hide resolved
}

private static Map<String, String> parseAdditionalParameters(Map<String, Object> idpDefinitionMap) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,29 @@ 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);
bruce-ricard marked this conversation as resolved.
Show resolved Hide resolved
}

@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_does_not_external_logout_when_logout_url_is_null() throws ServletException, IOException {
when(oAuthLogoutHandler.getLogoutUrl(null)).thenReturn(null);
when(oAuthLogoutHandler.getPerformRpInitiatedLogout(null)).thenReturn(true);
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,4 +134,15 @@ void getOAuthProviderForAuthentication() {
void getNullOAuthProviderForAuthentication() {
assertEquals(null, oAuthLogoutHandler.getOAuthProviderForAuthentication(null));
}

@Test
void getPerformRpInitiatedLogout() {
oAuthIdentityProviderDefinition.setPerformRpInitiatedLogout(true);
assertEquals(true, oAuthLogoutHandler.getPerformRpInitiatedLogout(oAuthIdentityProviderDefinition));
bruce-ricard marked this conversation as resolved.
Show resolved Hide resolved

oAuthIdentityProviderDefinition.setPerformRpInitiatedLogout(false);
assertEquals(false, oAuthLogoutHandler.getPerformRpInitiatedLogout(oAuthIdentityProviderDefinition));

assertEquals(false, oAuthLogoutHandler.getPerformRpInitiatedLogout(null));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -234,4 +234,18 @@ public void testNoAdditionalParametersInConfig() {
Map<String, String> 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());
}
}
2 changes: 1 addition & 1 deletion uaa/slateCustomizations/source/index.html.md.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.endsWith;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -181,6 +182,7 @@ public void setUp() throws Exception {
config.setTokenKeyUrl(new URL(urlBase + "/token_key"));
config.setIssuer(urlBase + "/oauth/token");
config.setUserInfoUrl(new URL(urlBase + "/userinfo"));
config.setLogoutUrl(new URL(urlBase + "/logout.do"));

config.setShowLinkText(true);
config.setLinkText("My OIDC Provider");
Expand Down Expand Up @@ -558,6 +560,32 @@ public void testResponseTypeRequired() {
assertThat(webDriver.getCurrentUrl(), containsString("error_description=Missing%20response_type%20in%20authorization%20request"));
}

@Test
public void successfulUaaLogoutTriggersExternalOIDCProviderLogout_whenConfiguredTo() {
identityProvider.getConfig().setPerformRpInitiatedLogout(true);
updateProvider();

validateSuccessfulOIDCLogin(zoneUrl, testAccounts.getUserName(), testAccounts.getPassword());
bruce-ricard marked this conversation as resolved.
Show resolved Hide resolved

String externalOIDCProviderLoginPage = baseUrl;
webDriver.get(externalOIDCProviderLoginPage);
Assert.assertThat("Did not land on the external OIDC provider login page (as an unauthenticated user).",
webDriver.getCurrentUrl(), endsWith("/login"));
}

@Test
public void successfulUaaLogoutDoesNotTriggerExternalOIDCProviderLogout_whenConfiguredNotTo() {
identityProvider.getConfig().setPerformRpInitiatedLogout(false);
updateProvider();

validateSuccessfulOIDCLogin(zoneUrl, testAccounts.getUserName(), testAccounts.getPassword());

String externalOIDCProviderLoginPage = baseUrl;
webDriver.get(externalOIDCProviderLoginPage);
Assert.assertThat("Did not land on the external OIDC provider home page (as an authenticated user).",
webDriver.getPageSource(), containsString("Where to?"));
}

private String getRefreshTokenResponse(ServerRunning serverRunning, String refreshToken) {
MultiValueMap<String, String> formData = new LinkedMultiValueMap<>();
formData.add("client_id", zoneClient.getClientId());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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`"),
Expand Down Expand Up @@ -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"));
Expand Down Expand Up @@ -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."),
Expand Down