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

Allow list of valid audiences (#205 continued) #306

Merged
merged 11 commits into from
Nov 27, 2017

Conversation

r-springer
Copy link
Contributor

@r-springer r-springer commented Nov 14, 2017

Extending @codervinod's work in #246 for #205 (since there's no progress there recently), addressing the PR feedback.

@FraBle
Copy link

FraBle commented Nov 14, 2017

👍

@coveralls
Copy link

coveralls commented Nov 14, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling dd3df0f on r-springer:master into c1253ec on jpadilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling dd3df0f on r-springer:master into c1253ec on jpadilla:master.

@coveralls
Copy link

coveralls commented Nov 14, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling db5951e on r-springer:master into c1253ec on jpadilla:master.

@coveralls
Copy link

coveralls commented Nov 15, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 02b8162 on r-springer:master into c1253ec on jpadilla:master.

Copy link
Contributor

@mark-adams mark-adams left a comment

Choose a reason for hiding this comment

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

Conceptually, I think this is definitely an improvement but I'd like a couple changes.

jwt/api_jwt.py Outdated
@@ -103,8 +102,8 @@ def _validate_claims(self, payload, options, audience=None, issuer=None,
if isinstance(leeway, timedelta):
leeway = leeway.total_seconds()

if not isinstance(audience, (string_types, type(None))):
raise TypeError('audience must be a string or None')
if not isinstance(audience, (string_types, type(None), list)):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to use collections.Iterable to allow any iterable type to be used instead of just a list. Seems a little more idiomatic that way.

jwt/api_jwt.py Outdated
if isinstance(audience, string_types):
audience = [audience]

for aud in audience:
Copy link
Contributor

Choose a reason for hiding this comment

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

What about nixing 183 - 186 and doing:

if not any((aud in audience for aud in audience_claims)):
   raise InvalidAudience('Invalid audience')

jwt/api_jwt.py Outdated
if not isinstance(audience, (string_types, type(None))):
raise TypeError('audience must be a string or None')
if not isinstance(audience, (string_types, type(None), list)):
raise TypeError('audience must be a string, list of strings, or None')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this iterable instead of list of strings

CHANGELOG.md Outdated
@@ -11,6 +11,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- Dropped support for python 2.6 and 3.3 [#297][297]

### Fixed

- Audience parameter now supports lists in compliance with RFC 7519 [#306][306]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's drop in compliance with RFC 7519. This is purely an API design decision in the library. RFC 7519 does not require libraries to allow multiple audience values when validating a token. It does require libraries to allow the token to contain multiple values for the aud claim. This change does not make the library any more or less compliant with the RFC, it just makes the API a little more pleasant in the use case that you have multiple possible allowed audiences in your use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I actually misread that. Dropped the mentioned part and moved it to changed instead of fixed.

Copy link
Contributor Author

@r-springer r-springer left a comment

Choose a reason for hiding this comment

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

Thanks for the review. Feedback incorporated 🙂

CHANGELOG.md Outdated
@@ -11,6 +11,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- Dropped support for python 2.6 and 3.3 [#297][297]

### Fixed

- Audience parameter now supports lists in compliance with RFC 7519 [#306][306]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I actually misread that. Dropped the mentioned part and moved it to changed instead of fixed.

@coveralls
Copy link

coveralls commented Nov 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 504f63c on r-springer:master into c1253ec on jpadilla:master.

@coveralls
Copy link

coveralls commented Nov 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6fa0f74 on r-springer:master into c1253ec on jpadilla:master.

jwt/api_jwt.py Outdated
raise InvalidAudienceError('Invalid audience')

return
Copy link
Contributor

Choose a reason for hiding this comment

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

This return is extraneous since we're at the end of the function, right?

@@ -92,7 +91,7 @@ def test_decode_with_invalid_audience_param_throws_exception(self, jwt):
jwt.decode(example_jwt, secret, audience=1)

exception = context.value
assert str(exception) == 'audience must be a string or None'
assert str(exception) == 'audience must be a string, Iterable, or None'
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably ok to just say iterable here instead of Iterable. I think most people have an understanding of what an iterable is with regards to Python.

@mark-adams
Copy link
Contributor

I left a couple additional comments. Once those are taken care of, I think we'll be good to go.

@coveralls
Copy link

coveralls commented Nov 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 0a0a423 on r-springer:master into c1253ec on jpadilla:master.

@mark-adams mark-adams merged commit 7f7d524 into jpadilla:master Nov 27, 2017
@mark-adams
Copy link
Contributor

Thanks for the excellent contribution @codervinod! 🎉

@jpadilla jpadilla added this to the v1.6.0 milestone Feb 27, 2018
CorreyL added a commit to CorreyL/pyjwt that referenced this pull request Apr 24, 2020
Demonstrate to users reading the documentation that the `audience`
parameter is not restricted to the `string` type, but can also accept an
iterable, as implemented in PR#306

jpadilla#306
jpadilla pushed a commit that referenced this pull request Apr 26, 2020
* Add code example for `aud` being an array

The previous code example only showed the `aud` claim as a single
case-sensitive string, despite the documentation mentioning that the
`aud` claim can be an array of case-sensitive strings

Add a code block demonstrating the `aud` claim being an array of
case-sensitive strings to make it more clear to the user that it is a
permitted use of the `aud` claim

* Add example of the `audience` param as an iterable

Demonstrate to users reading the documentation that the `audience`
parameter is not restricted to the `string` type, but can also accept an
iterable, as implemented in PR#306

#306

* Fix short title underlines

Short title underlines throw warnings in reStructuredText linters
jpadilla pushed a commit that referenced this pull request Jun 19, 2020
* Add code example for `aud` being an array

The previous code example only showed the `aud` claim as a single
case-sensitive string, despite the documentation mentioning that the
`aud` claim can be an array of case-sensitive strings

Add a code block demonstrating the `aud` claim being an array of
case-sensitive strings to make it more clear to the user that it is a
permitted use of the `aud` claim

* Add example of the `audience` param as an iterable

Demonstrate to users reading the documentation that the `audience`
parameter is not restricted to the `string` type, but can also accept an
iterable, as implemented in PR#306

#306

* Fix short title underlines

Short title underlines throw warnings in reStructuredText linters
rylanhall33 added a commit to rylanhall33/pyjwt that referenced this pull request Jun 15, 2022
* Add code example for `aud` being an array

The previous code example only showed the `aud` claim as a single
case-sensitive string, despite the documentation mentioning that the
`aud` claim can be an array of case-sensitive strings

Add a code block demonstrating the `aud` claim being an array of
case-sensitive strings to make it more clear to the user that it is a
permitted use of the `aud` claim

* Add example of the `audience` param as an iterable

Demonstrate to users reading the documentation that the `audience`
parameter is not restricted to the `string` type, but can also accept an
iterable, as implemented in PR#306

jpadilla/pyjwt#306

* Fix short title underlines

Short title underlines throw warnings in reStructuredText linters
xmas7 pushed a commit to RubyOnWorld/pyjwt that referenced this pull request Sep 6, 2022
* Add code example for `aud` being an array

The previous code example only showed the `aud` claim as a single
case-sensitive string, despite the documentation mentioning that the
`aud` claim can be an array of case-sensitive strings

Add a code block demonstrating the `aud` claim being an array of
case-sensitive strings to make it more clear to the user that it is a
permitted use of the `aud` claim

* Add example of the `audience` param as an iterable

Demonstrate to users reading the documentation that the `audience`
parameter is not restricted to the `string` type, but can also accept an
iterable, as implemented in PR#306

jpadilla/pyjwt#306

* Fix short title underlines

Short title underlines throw warnings in reStructuredText linters
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.

6 participants