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

Store the OAUTH2 id_token in the id_token extra data. #17

Closed
wants to merge 1 commit into from

Conversation

amichal
Copy link

@amichal amichal commented Apr 6, 2021

We needed to implement the logout flow for OKTA. Docs here https://developer.okta.com/docs/reference/api/oidc/#logout. It is not super clear but the id_token_hint param is expected to be the id_token provided by OKTA in response the /token request.

It turns out that the id_token and id_info field in extra where being populated from the access_token instead of the id token. The attached change fixes that to return the id_token.

I kept the nil check but the token should always be present in the response of https://developer.okta.com/docs/reference/api/oidc/#token as long we request the openid scope.

@amichal
Copy link
Author

amichal commented Apr 6, 2021

For those who get here because of the logout API reference:

When you have a valid id_token and id_info you can do this to build the OIDC logout API URL something like this. "auth" in this case is the payload you get in Devise::OmniauthCallbacksController request.env['omniauth.auth']. Presumably you stashed what you need from it.

def idp_logout_url(auth, post_logout_redirect_uri: nil, state: nil)
    issuer = auth.dig('extra', 'id_info', 'iss')
    id_token = auth.dig('credentials', 'id_token')

    return unless issuer.present? && id_token.present?

    "#{issuer}/v1/logout?" + {
      id_token_hint: id_token,
      post_logout_redirect_uri: post_logout_redirect_uri,
      state: state,
    }.compact.to_param
 end

@Andrew-Tolentino
Copy link

Out of curiosity, when will this get merged?

Andrew-Tolentino added a commit to Andrew-Tolentino/omniauth-okta that referenced this pull request Aug 26, 2021
Andrew-Tolentino added a commit to Andrew-Tolentino/omniauth-okta that referenced this pull request Aug 26, 2021
Andrew-Tolentino added a commit to Andrew-Tolentino/omniauth-okta that referenced this pull request Aug 27, 2021
@erikpaulmiller
Copy link

Out of curiosity, when will this get merged?

@dandrews @jduff @BobbyMcWho ^^

@rience
Copy link

rience commented Jan 11, 2022

We have the same situation where we need to implement logout url. Could this PR be merged?

@BobbyMcWho
Copy link
Member

I'll try and find some time this week to review this and merge if appropriate. I don't look at this codebase super frequently, so it takes a little bit to regather context

@ryanswood
Copy link

I ran into this issue today. Is there something I can do to help get this merged?

@BobbyMcWho
Copy link
Member

@ryanswood were you able to test it out in your app using the branch? I don't run any okta-authed apps at the moment, but I'm happy to cut a release if someone confirms expected behavior

@ryanswood
Copy link

@BobbyMcWho Apologies for not getting back to you sooner. I was pulled away to handle a work emergency. Next on my list is to validate the change using the PR. My first validation effort consisted of copying the change to my app using a custom inherited strategy.

@ryanswood
Copy link

@BobbyMcWho I was able to test the solution in this PR using an Okta instance. I decided to slightly improve the solution by checking presence for the proper Oauth2 token and adding a spec. My PR

@BobbyMcWho
Copy link
Member

I appreciate your patience on this folks, this has been released in v2.0.0. Released on rubygems.

@BobbyMcWho BobbyMcWho closed this Apr 18, 2022
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.

6 participants