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 id_token_hint to the post logout redirect uri #149

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

CSDUMMI
Copy link
Contributor

@CSDUMMI CSDUMMI commented Mar 1, 2023

The OneLogin IdP requires the id_token_hint field to be set to the previously issued access token in the end session uri to perform a logout and redirect the user to the post logout uri.1

The Keycloak IdP requires the id_token_hint field to be set to avoid a confirmation dialog before redirecting them to the post logout uri.2

This PR adds the id_token_hint to support this behavior by the IdPs. I'm open to only enabling this through an option that is disabled by default.

Footnotes

  1. See OneLogin OIDC post_logout_redirect_uri issue #140 and https://developers.onelogin.com/openid-connect/api/logout

  2. See keycloak documentation: https://www.keycloak.org/docs/latest/securing_apps/#logout

@CSDUMMI CSDUMMI force-pushed the master branch 3 times, most recently from 7af5a58 to 5ff1097 Compare March 1, 2023 13:48
@kitebuggy
Copy link

Great work, thank you.

@CSDUMMI CSDUMMI force-pushed the master branch 2 times, most recently from 3fdb802 to 5a1a29c Compare March 7, 2023 18:46
@CSDUMMI
Copy link
Contributor Author

CSDUMMI commented Mar 8, 2023

The tests fail because access_token is now called by encoded_post_logout_redirect_uri and this function calls client.access_token! if no access token has previously been fetched.

The tests do not mock this function and thus a request to example.com is made - expecting an access token but receiving HTML.

I don't know enough about stubbing in Ruby to stub this particular behavior.

@CSDUMMI
Copy link
Contributor Author

CSDUMMI commented Mar 19, 2023

@stanhu can you review this PR or is there somebody else I can talk to?

@@ -45,7 +45,7 @@ def test_logout_phase_with_discovery
end

def test_logout_phase_with_discovery_and_post_logout_redirect_uri
expected_redirect = 'https://example.com/logout?post_logout_redirect_uri=https%3A%2F%2Fmysite.com'
expected_redirect = 'https://example.com/logout?post_logout_redirect_uri=https%3A%2F%2Fmysite.com&id_token_hint'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this value be blank? Should the key be omitted entirely if there is no token?

Copy link

Choose a reason for hiding this comment

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

I suspect this needs two tests:

  • one with an ID token present (such as this) where the redirect URL should contain the id_token_hint param with a value of <id-token>
  • one where the ID token is not present and the redirect URL does not contain the id_token_hint param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether it's worth the extra effort to create a test case for the second case.

@@ -45,7 +45,7 @@ def test_logout_phase_with_discovery
end

def test_logout_phase_with_discovery_and_post_logout_redirect_uri
expected_redirect = 'https://example.com/logout?post_logout_redirect_uri=https%3A%2F%2Fmysite.com'
expected_redirect = 'https://example.com/logout?post_logout_redirect_uri=https%3A%2F%2Fmysite.com&id_token_hint'
Copy link

Choose a reason for hiding this comment

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

Suggested change
expected_redirect = 'https://example.com/logout?post_logout_redirect_uri=https%3A%2F%2Fmysite.com&id_token_hint'
access_token = stub('OpenIDConnect::AccessToken')
access_token.stubs(:id_token).returns(jwt.to_s)
expected_redirect = "https://example.com/logout?post_logout_redirect_uri=https%3A%2F%2Fmysite.com&id_token_hint=#{access_token.id_token}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. I applied them and the case passed.

…direct redirect (without confirmation) with some IdP sofware (keycloak)
@drjole
Copy link

drjole commented Nov 16, 2023

Hello everyone! Will this PR be merged at some point? Thanks for your work.

@evgenyneu
Copy link

I would like this to be merged. Are there any concerns with this code? Need any help?

@coberlin
Copy link

coberlin commented Nov 1, 2024

I also need to send id_token_hint, but this solution doesn't work for my client. I'm using authorization_code flow and the call to access_token fails with "invalid grant" because this flow requires that I send the authorization_code, which is supplied by params['code'] but that is not passed in the logout uri. I could send this code as a parameter, but that seems like overkill to require a fetch of the access_token just to allow logout. My application keeps track of the id_token, and I'd rather pass it as a parameter to the logout uri directly. I have a PR for this.

@evgenyneu
Copy link

evgenyneu commented Nov 2, 2024

I found out that even if this gets merged, I still can't use it with Amazon Cognito because they're not compliant with the OpenID Connect logout specification. Cognito uses different URL parameters (client_id and logout_uri), see their documentation. Just a heads up! 😄

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.

8 participants