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 support for revoking tokens #586

Merged
merged 1 commit into from
Aug 27, 2015

Conversation

phindmarsh
Copy link
Contributor

Implementation follows the Draft RFC7009 OAuth 2.0 Token Revocation.

Essentially adds a unsetAccessToken method to all the storage objects, and exposes a revoke endpoint on controllers. The only interesting thing is the ResponseType\AccessToken::revokeToken() method which has to check all token types if the specified type doesn't exist.

This goes some of the way to addressing #345, (adds unsetAccessToken methods etc), but doesn't account for bulk revoking of all tokens against a client or user. For this I've just extended the built-in storage class in our implementation and added the appropriate queries for doing the bulk stuff myself.

@bshaffer I'm not sure the tests I've done are comprehensive enough, can you cast your eye over and let me know if you'd like more in a certain area.

@bshaffer
Copy link
Owner

wow, this is fantastic! Your testing is great and the implementation is spot on. I'll dive deeper in this tomorrow, but just glancing at it I have only two minor suggestions:

  1. run php-cs-fixer to fix some of the whitespace
  2. we shouldn't add the interface changes to the develop branch, because it will break BC. And then we can submit the interface changes to just the 2.x branch

@phindmarsh
Copy link
Contributor Author

No problem, what level do you run the linter at? I've just done a pass using the defaults and ended up with 92 modified files 😮

Once you've had a chance to review and I've done the cs-fixer I'll close and resubmit against the v2.x branch

@phindmarsh
Copy link
Contributor Author

I've re-run the php-cs-fixer with --level=psr2 and discarded the changes to files this PR hasn't touched - I figured you'd rather this PR was only dealing with the revoke changes, not style changes to files unaffected by it.

@bshaffer
Copy link
Owner

good question, I usually run php-cs-fixer without phpdoc_params, so like so:

# php-cs-fixer fix . --fixers=-phpdoc_params

Even so, I found that you are right, and quite a few small cs issues seem to have escaped my notice :)

$this->markTestSkipped('Skipped Storage: ' . $storage->getMessage());

return;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Since this isn't part of the interface, it wouldn't be a bad idea to check the unsetAccessToken method exists, and skip this test if it doesn't. We would also want a @todo to remove this for 2.x

@bshaffer
Copy link
Owner

Only one real required change here (we need to remove the interface addition). Otherwise it looks great!

@bshaffer
Copy link
Owner

Then, I can merge this into 2.x and we can add the interface changes in another commit.

@phindmarsh
Copy link
Contributor Author

Righto, I've commented out (but left a @todo note) on each of the new methods defined in the interfaces, added a check to make sure the unsetAccessToken method exists before running the test.

Obviously while this doesn't break BC, it will cause a fatal error if someone attempts to use the revoke endpoint with a storage that doesn't implement that method, since there are no runtime checks to ensure that method exists. I can do this if you like, although not sure what it should do instead (throw an exception, fail quietly etc).

I assume at some point we'll put another PR against the 2.x branch with the interface methods added back again?

@bshaffer
Copy link
Owner

I think throwing an exception would be the nicest thing to do for the
developer, as the message we define will be more clear than a fatal error

As for the interface changes, yes we should submit them to 2.x in a
separate PR
On Mon, May 11, 2015 at 3:16 PM Patrick Hindmarsh notifications@github.com
wrote:

Righto, I've commented out (but left a @todo https://github.com/todo
note) on each of the new methods defined in the interfaces, added a check
to make sure the unsetAccessToken method exists before running the test.

Obviously while this doesn't break BC, it will cause a fatal error if
someone attempts to use the revoke endpoint with a storage that doesn't
implement that method, since there are no runtime checks to ensure that
method exists. I can do this if you like, although not sure what it should
do instead (throw an exception, fail quietly etc).

I assume at some point we'll put another PR against the 2.x branch with
the interface methods added back again?


Reply to this email directly or view it on GitHub
#586 (comment)
.

@phindmarsh
Copy link
Contributor Author

Noted, added a \RuntimeException to each call that was defined in that interface, and added a @todo to remove it when 2.0 lands.

That should be all the changes needed.

@phindmarsh
Copy link
Contributor Author

Ping @bshaffer, did you want to do anything here?

@bshaffer
Copy link
Owner

No, it looks great! However you will need to rebase it first, as we cannot merge it automatically.

}

return $revoked;
}

Choose a reason for hiding this comment

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

Hi, I'm merging your PR into my forked oauth repo as I needed it asap, and I was just writing some extra test and noticed some potential issue.

In this function, if you would pass some invalid $tokenTypeHint value in the first call (something different from "refresh_token" or "access_token") and the token you want to revoke would be stored in the tokenStorage, the expanded search for all token types would not work, as it would just retry once more always trying the refreshStorage and not finding anything.
If the token would be stored in refreshStorage, then it would succeed deleting it, out of luck.

I noticed that AccessTokenTest is not really testing AccessToken class, but instead just the storage underneath it. So I noticed this when writing some test for the AccessToken class itself, which would cover the behaviour of revokeToken function.

Choose a reason for hiding this comment

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

I noticed afterwards that the check for the correct value of token_type_hint is done in TokenController and this is actually test covered, so there is no issue from the POV of the project overall.

Still I think that a defensive programming is better, where every module/class/method is safe for itself and not relying on details of other code. Just my constructive 50 cents.

@opengeek
Copy link

Any news on this being merged? I am looking for exactly this feature without resorting to directly manipulating the storage...

@phindmarsh
Copy link
Contributor Author

Sorry I keep forgetting to rebase this up to master. I'll do it now and then it should be ready to go.

@phindmarsh
Copy link
Contributor Author

Righto, all up to date now, @bshaffer this is ready when you are.

@bshaffer
Copy link
Owner

These changes are great @phindmarsh! However, @hugocardenas has a great point. I am submitting a PR to your token-revoke branch with a few changes to fix that issue, and once you merge that one in (and rebase out the merge commit) this'll be good to go

@bshaffer
Copy link
Owner

added phindmarsh/pull/1 to address these issues

Implementation follows the Draft RFC7009 OAuth 2.0 Token Revocation.

Essentially adds a unsetAccessToken method to all the storage objects, and exposes a revoke endpoint on controllers. The only interesting thing is the ResponseType\AccessToken::revokeToken() method which has to check all token types if the specified type doesn't exist.

Maintains BC with v1.x by commenting out the AccessToken interface methods, and annotated an @todo to implement these in 2.x. Throws a \RuntimeException if storage/response objects don't support the interface
@phindmarsh
Copy link
Contributor Author

Merged and rebased back to a single commit, thanks @bshaffer 👍

bshaffer added a commit that referenced this pull request Aug 27, 2015
Added support for revoking tokens
@bshaffer bshaffer merged commit 6cf428f into bshaffer:develop Aug 27, 2015
@bshaffer
Copy link
Owner

and there was much rejoicing! Thank you @phindmarsh and everyone else involved for your hard work on this!!

@bshaffer bshaffer mentioned this pull request Sep 8, 2015
@bshaffer bshaffer mentioned this pull request Sep 18, 2015
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.

4 participants