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

sso_auth: fix client authentication for revoke endpoint #217

Merged
merged 1 commit into from Jun 24, 2019

Conversation

Jusshersmith
Copy link
Contributor

Problem

/oauth2/sign_out does not currently work with the Okta provider:

An error occurred during sign out. Please try again.

is returned, and the request to sign the user out is unsuccessful.

Solution

This PR adds client authentication for the Revoke function (I think this must have been lost during development?), which is used to sign out via the /oauth2/sign_out endpoint.

Notes

Client authentication is provided using the same method as in the ValidateSessionState function:

form := url.Values{}
form.Add("token", s.AccessToken)
form.Add("token_type_hint", "access_token")
form.Add("client_id", p.ClientID)
form.Add("client_secret", p.ClientSecret)
err := p.oktaRequest("POST", p.ValidateURL.String(), form, []string{"action:validate"}, nil, &response)

@Jusshersmith
Copy link
Contributor Author

Jusshersmith commented Jun 24, 2019

I believe we want to revoke the refresh token, which also forces the access token to be revoked:

(https://developer.okta.com/docs/guides/revoke-tokens/overview/)

Revoking only the access token
Revoking only the access token will effectively force the use of the refresh token to retrieve a new access token. This could be useful if, for example, you have changed a user's data and you want this information to be reflected in a new access token.

Revoking only the refresh token
If you revoke only the refresh token then the access token will also be revoked. This allows you to, for example, force a user to reauthenticate.

@codecov
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

Merging #217 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage   61.21%   61.23%   +0.01%     
==========================================
  Files          50       50              
  Lines        4071     4073       +2     
==========================================
+ Hits         2492     2494       +2     
  Misses       1393     1393              
  Partials      186      186
Impacted Files Coverage Δ
internal/auth/providers/okta.go 61.88% <100%> (+0.31%) ⬆️

jphines
jphines previously approved these changes Jun 24, 2019
- Fix client authorisation
- Revoke refresh token, implicitly revoking access token as well
@Jusshersmith Jusshersmith merged commit 8acd64a into master Jun 24, 2019
@mccutchen mccutchen deleted the jusshermsith-fix-okta-revoke branch November 4, 2019 17:31
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.

None yet

2 participants