Numerous improvements #16
Conversation
- configure_with_multiple_jwt_issuers() can now be called with a list of dicts for issuers, instead of strings, in the format:
```python
{
'url': str, # complete url to the jwks file
'name': str, # should match the iss claim,
'audience': str|list[str] # audiences to allow from this issuer
}
```
- _authorize() (and calling functions) now accept an optional audience parameter allowing checks for a specific aud on decorators, etc
- claims that are lists can now be checked with _authorize()
- prevent exception if scope claim is not present
| if not isinstance(self.aud, list): | ||
| self.aud = [self.aud] | ||
|
|
||
| if len(set(self.aud)&set(aud)) == 0: |
There was a problem hiding this comment.
whitespace around the & operator would be preferable IMO
| jwks_url = issuer + "/jwks" | ||
| self.validators[issuer] = JwtValidator(jwks_url, issuer, audience, self.verify_ssl) | ||
| elif isinstance(issuer, dict): | ||
| self.validators[issuer['name']] = JwtValidator(issuer['url'], issuer['name'], issuer['audience'], |
There was a problem hiding this comment.
what says the issuer is in the name entry of the dictionary? Similar question for url and audience.
| if not isinstance(token_claims['aud'], list): | ||
| token_claims['aud'] = [token_claims['aud']] | ||
|
|
||
| if len(set(endpoint_audience)&set(token_claims['aud'])) == 0: |
There was a problem hiding this comment.
whitespace before/after &.
| return False | ||
|
|
||
| if endpoint_claims[claim] is not None: |
There was a problem hiding this comment.
Does it make sense to factor this out? It's getting to be a lot of nesting here.
| if endpoint_claims[claim] is not None: | ||
| if isinstance(token_claims[claim], list): | ||
| if isinstance(endpoint_claims[claim], list): | ||
| if len(set(token_claims[claim])&set(endpoint_claims[claim])) == 0: |
There was a problem hiding this comment.
Hey @dangoscomb,
Thanks for your contribution. Some nice additions to the library!
You made a good point about the missing tests so we've recently added some basic coverage. Are you able to add tests for the changes that you've made?
There are some minor comments to the PR left by me and Travis. Can you have a look at these also?
| @@ -48,7 +48,7 @@ def configure_with_jwt(self, jwks_url, issuer, audience, scopes=None): | |||
| if scopes is not None: | |||
| self.scopes = scopes | |||
|
|
|||
| def configure_with_multiple_jwt_issuers(self, issuers, audience, scopes=None): | |||
| def configure_with_multiple_jwt_issuers(self, issuers, audience=None, scopes=None): | |||
There was a problem hiding this comment.
Maybe there should be a check that if the issuer is a string, then the audience is required. Currently, the validator can be configured with a None audience.
| jwks_url = issuer + "/jwks" | ||
| self.validators[issuer] = JwtValidator(jwks_url, issuer, audience, self.verify_ssl) | ||
| elif isinstance(issuer, dict): | ||
| self.validators[issuer['name']] = JwtValidator(issuer['url'], issuer['name'], issuer['audience'], |
There was a problem hiding this comment.
Can you update the README and describe that structure? I wonder whether there should be some checks if all the necessary elements of the dictionary are present.
Also, I'm not quite sold yet on the idea that the audience is a per-issuer setting. Is it because you have different URLs/identifiers for the same API?
Hi
I have made a number of improvements so that this fits in our environment. I have tried to maintain backwards compatibility but lack of unit tests has made it a little difficult so this would probably need a little testing.
Changes made:
{ 'url': str, # complete url to the jwks file 'name': str, # should match the iss claim, 'audience': str|list[str] # audiences to allow from this issuer }