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

Numerous improvements #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion flask_of_oil/jwt_validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,10 @@ def validate(self, jwt):
if not isinstance(aud, list):
aud = [aud]

if self.aud not in aud:
if not isinstance(self.aud, list):
self.aud = [self.aud]

if len(set(self.aud)&set(aud)) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

whitespace around the & operator would be preferable IMO

self.logger.debug("Invalid audience %s, expected %s" % (aud, self.aud))
return {"active": False}

Expand Down
50 changes: 40 additions & 10 deletions flask_of_oil/oauth_filter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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.

"""

:param issuers: List of issuer values.
Expand All @@ -61,8 +61,12 @@ def configure_with_multiple_jwt_issuers(self, issuers, audience, scopes=None):
self.scopes = scopes

for issuer in issuers:
jwks_url = issuer + "/jwks"
self.validators[issuer] = JwtValidator(jwks_url, issuer, audience, self.verify_ssl)
if isinstance(issuer, str):
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'],
Copy link
Member

Choose a reason for hiding this comment

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

what says the issuer is in the name entry of the dictionary? Similar question for url and audience.

Copy link
Member

Choose a reason for hiding this comment

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

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?

self.verify_ssl)

def configure_with_opaque(self, introspection_url, client_id, client_secret, scopes=None):
"""
Expand Down Expand Up @@ -111,16 +115,39 @@ def _extract_access_token(incoming_request=None):

return query_param_access_token

def _authorize(self, token_claims, endpoint_scopes, endpoint_claims):
def _authorize(self, token_claims, endpoint_scopes, endpoint_claims, endpoint_audience):

if endpoint_audience is not None:
if not isinstance(endpoint_audience, list):
endpoint_audience = [endpoint_audience]

if not isinstance(token_claims['aud'], list):
token_claims['aud'] = [token_claims['aud']]

if len(set(endpoint_audience)&set(token_claims['aud'])) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

whitespace before/after &.

return False

if endpoint_claims is None:
endpoint_claims = {}

for claim in endpoint_claims:
if claim not in token_claims or \
(endpoint_claims[claim] is not None and token_claims[claim] != endpoint_claims[claim]):
if claim not in token_claims:
return False

if endpoint_claims[claim] is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to factor this out? It's getting to be a lot of nesting here.

if isinstance(token_claims[claim], list):
if isinstance(endpoint_claims[claim], list):
if len(set(token_claims[claim])&set(endpoint_claims[claim])) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

whitespace &

return False
else:
if endpoint_claims[claim] not in token_claims[claim]:
return False
else:
if token_claims[claim] != endpoint_claims[claim]:
return False

if 'scope' not in token_claims:
token_claims['scope'] = []
scope = token_claims['scope']
if isinstance(token_claims['scope'], (list, tuple)):
incoming_scopes = scope
Expand All @@ -134,12 +161,13 @@ def _authorize(self, token_claims, endpoint_scopes, endpoint_claims):

return all(s in incoming_scopes for s in required_scopes)

def protect(self, scopes=None, claims=None):
def protect(self, scopes=None, claims=None, audience=None):
"""
This is a decorator function that can be used on a flask route:
@_oauth.protect(["read","write]) or @_oauth.protect()
:param claims: The claims that are required for the protected endpoint (dict)
:param scopes: The scopes that are required for the protected endpoint (list or space separated string)
:param audience: The audience that is required for the protected endpoint (string)
"""

if scopes is None:
Expand All @@ -158,7 +186,7 @@ def protect(self, scopes=None, claims=None):
def decorator(f):
@wraps(f)
def inner_decorator(*args, **kwargs):
if self.filter(scopes=scopes, claims=claims) is None:
if self.filter(scopes=scopes, claims=claims, audience=audience) is None:
return f(*args, **kwargs)
else:
abort(500)
Expand All @@ -167,7 +195,8 @@ def inner_decorator(*args, **kwargs):

return decorator

def filter(self, scopes=None, claims=None):

def filter(self, scopes=None, claims=None, audience=None):
self.logger.debug("Request method = " + str(request.method))
token = self._extract_access_token(request)
validated_token = dict(active=False)
Expand Down Expand Up @@ -196,7 +225,8 @@ def filter(self, scopes=None, claims=None):
abort(401)

# Authorize scope
authorized = self._authorize(validated_token, endpoint_scopes=scopes, endpoint_claims=claims)
authorized = self._authorize(validated_token, endpoint_scopes=scopes, endpoint_claims=claims,
endpoint_audience=audience)
if not authorized:
abort(403)

Expand Down