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

Improve 'none' algorithm handling #365

Merged
merged 1 commit into from Oct 5, 2020
Merged

Improve 'none' algorithm handling #365

merged 1 commit into from Oct 5, 2020

Conversation

@danleyden
Copy link
Contributor

@danleyden danleyden commented Jul 23, 2020

when decoding a valid token with algorithm: none, unless explicitly
requesting not to verify the token, it could not be validated and raised
DecodeError with a message about insufficient segments.

This error message is misleading, because there are the correct number
of segments provided for that algorithm.

With this change, when a token with algorithm: none is provided:

  • if the caller requests verification && does not specify none as an
    algorithm (default behaviour) it now raises IncorrectAlgorithm,
    which is a subclass of DecodeError to make the issue more clear.
    This is technically a minor change, but should not be breaking -
    it is returning a subclass, so the same rescues will still work. The
    message provided will be different.

  • if the caller requests verification && specifies none as an allowed
    algorithm, it verifies the claims and decodes the token as it would
    for a valid, signed token.
    This is new behaviour supporting claims verification for 'none' which
    was not previously available and is only "accessed" through explicit
    settings

  • if the caller explicitly requests no verification, the token is
    decoded without checking anything (no change in behaviour)

when decoding a valid token with `algorithm: none`, unless explicitly
requesting not to verify the token, it could not be validated and raised
DecodeError with a message about insufficient segments.

This error message is misleading, because there are the correct number
of segments provided for that algorithm.

With this change, when a token with `algorithm: none` is provided:
* if the caller requests verification && does not specify `none` as an
  algorithm (default behaviour) it now raises `IncorrectAlgorithm`,
  which is a subclass of `DecodeError` to make the issue more clear.
  This is technically a minor change, but should not be breaking -
  it is returning a subclass, so the same rescues will still work. The
  message provided will be different.

* if the caller requests verification && specifies `none` as an allowed
  algorithm, it verifies the claims and decodes the token as it would
  for a valid, signed token.
  This is new behaviour supporting claims verification for 'none' which
  was not previously available and is only "accessed" through explicit
  settings

* if the caller explicitly requests no verification, the token is
  decoded without checking anything (no change in behaviour)
@sourcelevel-bot
Copy link

@sourcelevel-bot sourcelevel-bot bot commented Jul 23, 2020

Hello, @danleyden! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@excpt excpt requested review from excpt and anakinj Jul 24, 2020
@excpt excpt added this to the Version 2.3.0 milestone Jul 24, 2020
@excpt excpt linked an issue that may be closed by this pull request Jul 24, 2020
Copy link
Member

@anakinj anakinj left a comment

This looks good to me

@excpt
excpt approved these changes Sep 1, 2020
@excpt excpt merged commit 98ae462 into jwt:master Oct 5, 2020
3 checks passed
3 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sourcelevel-bot
sourcelevel SourceLevel has found 15 fixed issues.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants