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 client_id and client_secret on revoke_token #71

Merged
merged 1 commit into from
Feb 9, 2016

Conversation

tinogomes
Copy link
Contributor

Adapted from Django OAuth Tookit - Part 4 - Revoking an OAuth2 Token

Depending on the client type you’re using, the token revocation request you may submit to the authentication server may vary. A Public client, for example, will not have access to your Client Secret. A revoke request from a public client would omit that secret, but requires client_id. If your application type is Confidential , it requires the client_secret, so you will have to add it as one of the parameters.

@@ -151,7 +151,6 @@ function OAuthProvider() {

getRefreshToken() {
var data = {
client_id: config.clientId,
Copy link
Member

Choose a reason for hiding this comment

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

Why did you removed the client_id here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

id_client is not required when request has authorization header. See more at http://tools.ietf.org/html/rfc6749#section-6

But, I will rollback this change, because it is not about "Revoke Token"

@ruipenso
Copy link
Member

ruipenso commented Feb 4, 2016

@tinogomes Can you please apply the minor fixes as commented? Also, please squash the commits. Thanks.

@tinogomes tinogomes force-pushed the pullrequest branch 2 times, most recently from a84ebe3 to a6063e0 Compare February 4, 2016 16:42
@tinogomes
Copy link
Contributor Author

@ruipenso DONE!

token: OAuthToken.getRefreshToken() ? OAuthToken.getRefreshToken() : OAuthToken.getAccessToken()
});

if(null !== config.clientSecret) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing space after if, i.e., if (.

@tinogomes
Copy link
Contributor Author

@ruipenso REDONE!

@@ -363,6 +363,7 @@ describe('OAuthProvider', function() {
queryString.stringify.callCount.should.equal(1);
queryString.stringify.firstCall.args.should.have.lengthOf(1);
queryString.stringify.firstCall.args[0].should.eql({
client_id: default.clientId,
Copy link
Member

Choose a reason for hiding this comment

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

defaults not default. The tests are not passing.

@tinogomes
Copy link
Contributor Author

@ruipenso done! All tests passed.

};

// To remove null values
for (var index in data) {
Copy link
Member

Choose a reason for hiding this comment

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

Ahm. You changed the latest code I reviewed.

Sorry, but can you keep the logic used in the other methods, i.e.,

var data = {
  client_id: config.clientId,
  token: OAuthToken.getRefreshToken() ? OAuthToken.getRefreshToken() : OAuthToken.getAccessToken()
};

if (null !== config.clientSecret) {
  data.client_secret = config.clientSecret;
}

data = queryString.stringify(data);

@tinogomes
Copy link
Contributor Author

@ruipenso I rollback last code, but on tests, the sort for expected keys can not be ascending

@ruipenso
Copy link
Member

ruipenso commented Feb 9, 2016

@tinogomes I'm aware of it - no problem.

Thanks for the PR.

ruipenso added a commit that referenced this pull request Feb 9, 2016
Add `client_id` and `client_secret` on revoke_token
@ruipenso ruipenso merged commit 5518ed1 into oauthjs:master Feb 9, 2016
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.

2 participants