omniauth / omniauth-okta

OAuth2 strategy for Okta
MIT License
41 stars 37 forks source link

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

Closed amichal closed 2 years ago

amichal commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Out of curiosity, when will this get merged?

erikpaulmiller commented 3 years ago

Out of curiosity, when will this get merged?

@dandrews @jduff @BobbyMcWho ^^

rience commented 2 years ago

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

BobbyMcWho commented 2 years ago

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 commented 2 years ago

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

BobbyMcWho commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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