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

Added method to revoke refresh tokens #111

Merged
merged 3 commits into from
Apr 26, 2017

Conversation

cocojoe
Copy link
Member

@cocojoe cocojoe commented Apr 19, 2017

Public API func revoke(refreshToken: String)
Added tests

@cocojoe cocojoe requested a review from hzalaz April 19, 2017 17:34
@@ -176,6 +176,16 @@ struct Auth0Authentication: Authentication {
return Request(session: session, url: oauthToken, method: "POST", handle: authenticationObject, payload: payload, logger: self.logger, telemetry: self.telemetry)
}

func revoke(refreshToken: String) -> Request<Void, AuthenticationError> {
let payload: [String: Any] = [
"tokenEndpointAuthMethod": "none",
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed

Copy link
Member

Choose a reason for hiding this comment

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

Also this is missing the grant_type AFAIK, did it work without it?

Copy link
Member Author

@cocojoe cocojoe Apr 26, 2017

Choose a reason for hiding this comment

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

I based on https://auth0.com/docs/tokens/preview/refresh-token#revoke-a-refresh-token
grant_type not mentioned either. (I don't have access to auth0-server to check source)

Copy link
Member

Choose a reason for hiding this comment

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

But you can ask the rest of the team who already implemented the feature

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is already done in Android right @lbalmaceda ?

Copy link
Member

Choose a reason for hiding this comment

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

Disregard the grant_type comment since its not needed here (got confused with refresh token) so the referenced parameter is not needed. Checked with @lbalmaceda

@@ -57,7 +57,7 @@ struct Response<E: Auth0Error> {
return json
}
// This piece of code is dedicated to our friends the backend devs :)
if response.url?.lastPathComponent == "change_password" {
if response.url?.lastPathComponent == "change_password" || response.url?.lastPathComponent == "revoke" {
Copy link
Member

Choose a reason for hiding this comment

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

Why should we need this?

Copy link
Member Author

@cocojoe cocojoe Apr 26, 2017

Choose a reason for hiding this comment

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

Well ideally we shouldn't, no reason API couldn't return JSON eg { 'success': 1 }. Both endpoints change_password and revoke return no body (HTTP 2OO only) success, so paired it up with the original hack/workaround (otherwise it will throw as there is no content).

In general code is set up to expect JSON, you could change this if/else block to return nil. The Handlers.swift should certainly be able to deal with it, but no doubt other potential issues hence the original workaround?

Copy link
Member

Choose a reason for hiding this comment

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

That if is a hack to avoid blowing up for change_password since it does not follow the rules of content negotiation. I'd avoid adding extra hacks like this, I though we have a way to tell there is no response or I will ignore the response (Maybe in Guardian.swift we did something like it)

Copy link
Member Author

@cocojoe cocojoe Apr 26, 2017

Choose a reason for hiding this comment

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

It's dealt with in Guardian, but not so transferrable https://github.com/auth0/Guardian.swift/blob/master/Guardian/Request.swift#L88

I looked at the changed_password response, forgot it returned plain text.
I've made a change, feel free to tell me if it sucks :).

@@ -728,27 +759,27 @@ class AuthenticationSpec: QuickSpec {
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Check that you are adding spaces that should not be there

Copy link
Member Author

Choose a reason for hiding this comment

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

Odd...

Copy link
Member Author

@cocojoe cocojoe Apr 26, 2017

Choose a reason for hiding this comment

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

Only change recently in editor was upgrading to 8.3.2, generally with tests I run Structure/Re-Indent

Copy link
Member

Choose a reason for hiding this comment

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

Yes on update the config get messed up most of the times.

@cocojoe cocojoe force-pushed the added-authentication-revoke-refresh-token branch from 7bf039f to f3a1116 Compare April 26, 2017 10:20
@hzalaz hzalaz added this to the v1-Next milestone Apr 26, 2017
@hzalaz hzalaz merged commit ef71d0f into master Apr 26, 2017
@hzalaz hzalaz deleted the added-authentication-revoke-refresh-token branch April 26, 2017 21:25
@cocojoe cocojoe mentioned this pull request Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants