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

Fix federated IdP logout #425

Merged
merged 5 commits into from
Dec 14, 2023
Merged

Fix federated IdP logout #425

merged 5 commits into from
Dec 14, 2023

Conversation

AmshikaH
Copy link
Contributor

@AmshikaH AmshikaH commented Dec 8, 2023

Purpose

The playground app was not able to logout of federated IdPs previously. This PR fixes the issue and allows federated IdP logout. This is done by passing the id token hint as a logout parameter if it is not null or empty. However, if it is null or empty, the client id will be passed instead.

In the authorization code grant type flow, after logging in and prior to getting the access token, the id token hint will be not be available as is the expected behaviour. Therefore, the client id will be used in logout, which is only supported in IS version 6.0 onwards as of now. To enable client id logout in versions 6.0 and 6.1, add the following configurations to the deployment.toml file.

[oauth.oidc.logout_params]
use_client_id=true

Goals

Fix federated IdP logout in the playground app.

Approach

Updates the logout request to include the id token hint or client id as a parameter.

Related Issues

@@ -690,7 +690,7 @@
%>
<td>
<button type="button" class="button"
onclick="document.location.href='<%=(String)session.getAttribute(OAuth2Constants.OIDC_LOGOUT_ENDPOINT)%>';">
onclick="document.location.href='<%=(String)session.getAttribute(OAuth2Constants.OIDC_LOGOUT_ENDPOINT)%>?post_logout_redirect_uri=<%=ApplicationConfig.getPostLogoutRedirectUri()%>&client_id=<%=(String)session.getAttribute(OAuth2Constants.CONSUMER_KEY)%>';">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if these params empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the post redirect uri and the client id/id token hint (as per the updated flow) are both empty, federated IdP logout will not be possible and the playground app will only log out of the Identity Server.

@@ -687,10 +687,16 @@
<td><input type="submit" name="authorize" value="Get Access Token"></td>
<%
if (isOIDCLogoutEnabled) {
String logout_url = (String)session.getAttribute(OAuth2Constants.OIDC_LOGOUT_ENDPOINT) + "?post_logout_redirect_uri=" + ApplicationConfig.getPostLogoutRedirectUri();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we haven't configure PostLogoutRedirectUri what will be the behaviour? Ideally if it is empty we should redirect to default logout page right?

Copy link
Contributor Author

@AmshikaH AmshikaH Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, the post_logout_redirect_uri parameter is included in the logout request regardless of whether the "PostLogoutRedirectUri" config is provided or not. Therefore, if the "PostLogoutRedirectUri" config is empty, the parameter post_logout_redirect_uri will be included in the request without a value, for example https://<host>:<port>/oidc/logout?post_logout_redirect_uri=&client_id=<client_id_value>.

If either the client id or token id hint is available, but the post_logout_redirect_uri parameter is empty like https://<host>:<port>/oidc/logout?post_logout_redirect_uri=&client_id=<client_id_value>, logout from the Identity Server and federated IdP will be possible, but upon logout, an error like the one given below will be displayed in the browser.

  "traceId": "<trace-id>",
  "code": 401,
  "description": "Authorization failure. Authorization information was invalid or missing from your request.",
  "message": "Unauthorized"
}

However, if neither the client id nor token id hint are available and the post_logout_redirect_uri parameter is empty as well, the logout request will be in the format https://<host>:<port>/oidc/logout?post_logout_redirect_uri=. Here, the browser will be redirected to the default logout page upon logout. (Note that due to the lack of the client id/token id hint, only Identity Server logout will be possible, while federated IdP will not be possible.)

This will be fixed by conditionally adding the post_logout_redirect_uri parameter to the logout request only if the "PostLogoutRedirectUri" is configured, for example like https://<host>:<port>/oidc/logout?client_id=<client_id_value> or https://<host>:<port>/oidc/logout?token_id_hint=<token_id_hint>, in which case the browser will be redirected to the default logout page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been fixed as mentioned in the above comment; if the PostLogoutRedirectUri config is empty, the post_logout_redirect_uri parameter will not be included and the browser will be redirected to the default logout page.

if (StringUtils.isNotBlank(idToken)) {
logout_url+= "&id_token_hint=" + idToken;
} else {
logout_url+= "&client_id=" + (String)session.getAttribute(OAuth2Constants.CONSUMER_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If client id and idTokenHint both not available what will happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If neither the client id nor the id token hint are available, only Identity Server logout will be possible, while federated IdP logout will not as it is dependent on these values to identify the relying party and the browser will be redirected to the default logout page.

Copy link
Contributor Author

@AmshikaH AmshikaH Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, the client id is expected to be available in all the logout flows as this value is provided prior to login in the relevant grant types.

@AmshikaH AmshikaH marked this pull request as draft December 14, 2023 09:15
@AmshikaH AmshikaH marked this pull request as ready for review December 14, 2023 09:18
@nilasini nilasini merged commit 57a1c92 into wso2:master Dec 14, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants