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

Refactor authentication API into pipeline format #6579

Merged
merged 1 commit into from
Mar 8, 2016
Merged

Refactor authentication API into pipeline format #6579

merged 1 commit into from
Mar 8, 2016

Conversation

jaswilli
Copy link
Contributor

@jaswilli jaswilli commented Mar 7, 2016

Refs #5508

  • Adds tests for authentication API methods that didn't have them.
  • Adds some missing catch handlers for inverted tests.
  • Adds a new error type TokenRevocationError that returns an HTTP status code 503 in the event there's a failure during token revocation so that the client is aware the token may still be valid.
  • No longer consider token_type_hint during the revocation process. When presented with a token /revoke will do its best to invalidate even without a valid hint.
  • Do not attempt to recover from missing owner user during setup. Missing fixtures should be fatal.

@sebgie
Copy link
Contributor

sebgie commented Mar 8, 2016

Hey, thank you for taking on this issue!

Although it looks like an oversight, not returning an error for invalid tokens was done according to the specification (https://tools.ietf.org/html/rfc7009):

The authorization server responds with HTTP status code 200 if the token has been revoked successfully or if the client submitted an invalid token.

Note: invalid tokens do not cause an error response since the client cannot handle such an error in a reasonable way. Moreover, the purpose of the revocation request, invalidating the particular token, is already achieved.

Not ignoring token_type_hint was a bug 👍.

An invalid token type hint value is ignored by the authorization server and does not influence the revocation response.

@jaswilli
Copy link
Contributor Author

jaswilli commented Mar 8, 2016

Although it looks like an oversight, not returning an error for invalid tokens was done according to the specification (https://tools.ietf.org/html/rfc7009):

That's how this is intended to work. An invalid/non-existent/previously revoked token will return a 200 status. However, if there's another type of error encountered (e.g., the server is somehow in a bad state) then a 503 status is returned so that the client knows that the token may not have been revoked and is free to retry at a later time. (https://tools.ietf.org/html/rfc7009#section-2.2.1)

@sebgie
Copy link
Contributor

sebgie commented Mar 8, 2016

oh, thanks for clarifying 👍

sebgie added a commit that referenced this pull request Mar 8, 2016
Refactor authentication API into pipeline format
@sebgie sebgie merged commit 411dd47 into TryGhost:master Mar 8, 2016
@jaswilli jaswilli deleted the auth-api branch March 8, 2016 14:57
@jaswilli
Copy link
Contributor Author

jaswilli commented Mar 8, 2016

Thanks for taking the time to review!

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