Skip to content

Allows multiple values for 'iss' - #31

Merged
mpdavis merged 3 commits into
mpdavis:masterfrom
bjmc:multiple_iss
Jul 27, 2016
Merged

Allows multiple values for 'iss'#31
mpdavis merged 3 commits into
mpdavis:masterfrom
bjmc:multiple_iss

Conversation

@bjmc

@bjmcbjmc commented Jul 27, 2016

Copy link
Copy Markdown
Contributor

Apparently Google may send either https://accounts.google.com or accounts.google.com for their 'iss' value, so it's necessary to check both.

This pull request allows passing a list or tuple to jwt.decode(issuer= so, for example, you can check against both valid Google values, instead of having to catch an exception and re-validate.

@codecov-io

codecov-io commented Jul 27, 2016

Copy link
Copy Markdown

Current coverage is 95.78% (diff: 100%)

Merging #31 into master will increase coverage by 0.01%

@@ master #31 diff @@
==========================================
Files 7 7 Lines 496 498 +2 Methods 0 0 Messages 0 0 Branches 0 0 ==========================================
+ Hits 475 477 +2 
Misses 21 21 Partials 0 0 

Powered by Codecov. Last update 8766f13...2cf888f

Comment threadjose/jwt.py Outdated
if issuer is not None:
if claims.get('iss') != issuer:
if isinstance(issuer, string_types):
issuer = [issuer]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Since we don't need to mutate this, can we construct this as a tuple instead of a list?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good point. I changed jws._get_keys() to use a tuple as well, for consistency. (I do think 1-element tuples are kind of ugly, though)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I can't disagree that they are ugly.

@mpdavis

Copy link
Copy Markdown
Owner

lgtm

@mpdavis
mpdavis merged commit 2055f0a into mpdavis:masterJul 27, 2016
@mpdavis

Copy link
Copy Markdown
Owner

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@bjmc@codecov-io@mpdavis