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.

Sign up for GitHub

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

Jump to bottom

Dynamic Provider #70

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Dynamic Provider #70

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a68d99
Dynamic Provider
HarryKodden Nov 7, 2018
19e76fd
update
HarryKodden Nov 7, 2018
006bde1
Update, for backwards compatibility, init_app will do an initial init…
HarryKodden Nov 8, 2018
c607d20
fix parameter error
HarryKodden Nov 8, 2018
2863bd8
was missing app_context...
HarryKodden Nov 8, 2018
32f9644
FIx on valid issuers
HarryKodden Nov 8, 2018
99e5554
Test scripts require google as valid Issuer...
HarryKodden Nov 8, 2018
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
171 changes: 150 additions & 21 deletions flask_oidc/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
import calendar

from six.moves.urllib.parse import urlencode
from six.moves.urllib.request import urlopen

from flask import request, session, redirect, url_for, g, current_app
from oauth2client.client import flow_from_clientsecrets, OAuth2WebServerFlow,\
AccessTokenRefreshError, OAuth2Credentials
Expand Down Expand Up @@ -103,7 +105,8 @@ class OpenIDConnect(object):
The core OpenID Connect client object.
"""
def __init__(self, app=None, credentials_store=None, http=None, time=None,
urandom=None):
urandom=None, provider=None):

self.credentials_store = credentials_store\
if credentials_store is not None\
else MemoryCredentials()
Expand All @@ -119,32 +122,45 @@ def __init__(self, app=None, credentials_store=None, http=None, time=None,
# By default, we do not have a custom callback
self._custom_callback = None

# In the beginning we know nothing ...
self.client_secrets = None

# get stuff from the app's config, which may override stuff set above
if app is not None:

if app:
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you change this line? The intention is that it'll be called with either a flask.Flask instance or None.

self.init_app(app)

# Backwards compatible: When provider argument is ommitted,
# the value would be None, then in method load_secrets, the
# client_secrets will be tried to load...
with app.app_context():
Copy link
Owner

Choose a reason for hiding this comment

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

This would need to move to init_app, since when we get here, the developer might not yet have provided an app instance.

self.init_provider(provider)

def __exit__(self, exception_type, exception_value, traceback):
Copy link
Owner

Choose a reason for hiding this comment

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

What is the use of this? I don't think there's an __enter__, so this class could not be used as a context manager anyway?
I also don't see a reason why this should support context manager calling methods?

self.logout()

self.client_secrets = None

current_app.config['OIDC_VALID_ISSUERS'] = GOOGLE_ISSUERS

def init_app(self, app):
"""
Do setup that requires a Flask app.

:param app: The application to initialize.
:type app: Flask
"""
secrets = self.load_secrets(app)
self.client_secrets = list(secrets.values())[0]
secrets_cache = DummySecretsCache(secrets)

# Set some default configuration options
app.config.setdefault('OIDC_CLIENT_SECRETS', None)
app.config.setdefault('OIDC_SCOPES', ['openid', 'email'])
app.config.setdefault('OIDC_GOOGLE_APPS_DOMAIN', None)
app.config.setdefault('OIDC_ID_TOKEN_COOKIE_NAME', 'oidc_id_token')
app.config.setdefault('OIDC_ID_TOKEN_COOKIE_PATH', '/')
app.config.setdefault('OIDC_ID_TOKEN_COOKIE_TTL', 7 * 86400) # 7 days
# should ONLY be turned off for local debugging
app.config.setdefault('OIDC_COOKIE_SECURE', True)
app.config.setdefault('OIDC_VALID_ISSUERS',
(self.client_secrets.get('issuer') or
GOOGLE_ISSUERS))
app.config.setdefault('OIDC_VALID_ISSUERS', GOOGLE_ISSUERS)
app.config.setdefault('OIDC_CLOCK_SKEW', 60) # 1 minute
app.config.setdefault('OIDC_REQUIRE_VERIFIED_EMAIL', False)
app.config.setdefault('OIDC_OPENID_REALM', None)
Expand All @@ -169,13 +185,6 @@ def init_app(self, app):
app.before_request(self._before_request)
app.after_request(self._after_request)

# Initialize oauth2client
self.flow = flow_from_clientsecrets(
app.config['OIDC_CLIENT_SECRETS'],
scope=app.config['OIDC_SCOPES'],
cache=secrets_cache)
assert isinstance(self.flow, OAuth2WebServerFlow)

# create signers using the Flask secret key
self.extra_data_serializer = JSONWebSignatureSerializer(
app.config['SECRET_KEY'])
Expand All @@ -187,11 +196,115 @@ def init_app(self, app):
except KeyError:
pass

def load_secrets(self, app):
# Load client_secrets.json to pre-initialize some configuration
return _json_loads(open(app.config['OIDC_CLIENT_SECRETS'],
'r').read())

def init_provider(self, provider):
"""
Do setup for a specific provider

:param provider: The provider to initialize.
:type provider: Dictionary with at lease 'base_url' item
Copy link
Owner

Choose a reason for hiding this comment

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

s/lease/least/

"""

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we want to add a check what there's no secrets loaded yet?
Since otherwise we might overwrite existing config without people being aware.

secrets = self.load_secrets(provider)
assert secrets != None, "Problem with loading secrets"
Copy link
Owner

Choose a reason for hiding this comment

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

Note that assert statements get removed when a python optimized bytecode file is written, as happens in packaging on various linux distros.
Please turn this into an explicit check and raise ValueError.


self.client_secrets = list(secrets.values())[0]
secrets_cache = DummySecretsCache(secrets)

# Initialize oauth2client
self.flow = flow_from_clientsecrets(
current_app.config['OIDC_CLIENT_SECRETS'],
scope=current_app.config['OIDC_SCOPES'],
cache=secrets_cache)

assert isinstance(self.flow, OAuth2WebServerFlow)

current_app.config['OIDC_VALID_ISSUERS'] = (self.client_secrets.get('issuer') or GOOGLE_ISSUERS)

def load_secrets(self, provider):

try:
static_secrets = current_app.config.get('OIDC_CLIENT_SECRETS', None)
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe return an error if provider is not None in this case, to avoid confusing people?

if static_secrets:
return _json_loads(open(static_secrets,'r').read())
except Exception as e:
raise Exception("Error reading secrets: {}".format(str(e)))

if not provider:
raise Exception("No Provider specified")

try:
Copy link
Owner

Choose a reason for hiding this comment

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

This function is kind of long.
Maybe add two helper functions def _perform_discovery() and def _perform_dynamic_registration()?

url = provider.get('base_url')

if not url.endswith('/'):
url += '/'

url += ".well-known/openid-configuration"

logger.debug("Loading: {}".format(url))

provider_info = json.load(
urlopen(url)
)

except Exception as e:
raise Exception("Can not obtain well known information: {}".format(str(e)))

for path in ['issuer', 'registration_endpoint', 'authorization_endpoint', 'token_endpoint', 'userinfo_endpoint', 'jwks_uri']:
if path in provider_info and provider_info[path].startswith('/'):
provider_info[path] = "{}{}".format(provider.get('base_url'), provider_info[path])

registration = provider.get('registration', None)

if not registration:
Copy link
Owner

Choose a reason for hiding this comment

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

So, while I think dynamic discovery is a reasonable idea, fully automatic dynamic registration without any configuration (e.g. provider['dynamic_registration'] = True) might be confusing to people.
Specifically because depending on the IdP in use, if people re-register every server start, their previously issued tokens might suddenly become unusable because they can no longer retrieve the token information.

Because of this, I think making the registration explicit would be a good idea.

try:
logger.debug("Dynamic Registration...")

registration = requests.post(
provider_info['registration_endpoint'],
data = json.dumps({
"redirect_uris": self._oidc_callback,
"grant_types": "authorization_code",
"client_name": provider.get('client_name', "Dynamic Registration"),
"response_types": "code",
"token_endpoint_auth_method": "client_secret_post",
"application_type": "native"
}),
headers = {
'Content-Type': "application/json",
'Cache-Control': "no-cache"
}
).json()

logger.debug("Registration: {}".format(registration))

except Exception as e:
raise Exception("Can not make client registration: {}".format(str(e)))

try:
try:
jwks_keys = json.load(
Copy link
Owner

Choose a reason for hiding this comment

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

I think that if we fail to load a jwks_uri, we should just error out entirely, and not even continue starting, since JWKS is required for the protocol.

urlopen(provider_info['jwks_uri'])
)
except:
jwks_keys = None

return {
'web' : {
'client_id': registration.get('client_id'),
'client_secret': registration.get('*client_secret*', registration.get('client_secret', None)),
'auth_uri': provider_info['authorization_endpoint'],
'token_uri': provider_info['token_endpoint'],
'userinfo_uri': provider_info['userinfo_endpoint'],
'jwks_keys': jwks_keys,
'redirect_uris': self._oidc_callback,
'issuer': provider_info['issuer'],
}
}
except Exception as e:
raise Exception("Error in preparing result: {}".format(str(e)))

raise Exception("No secrets loaded !")
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any way for us to arrive at this line of code?


@property
def user_loggedin(self):
"""
Expand Down Expand Up @@ -404,7 +517,9 @@ def _after_request(self, response):

def _before_request(self):
g.oidc_id_token = None
self.authenticate_or_redirect()

if self.client_secrets:
self.authenticate_or_redirect()

def authenticate_or_redirect(self):
"""
Expand Down Expand Up @@ -567,6 +682,16 @@ def redirect_to_auth_server(self, destination=None, customstate=None):
self._set_cookie_id_token(None)
return redirect(auth_url)

def token(self):
try:
return self.credentials_store[g.oidc_id_token['sub']]
except KeyError:
logger.debug("No Token !", exc_info=True)
return None

def details(self):
return self._retrieve_userinfo()

def _is_id_token_valid(self, id_token):
"""
Check if `id_token` is a current ID token for this application,
Expand All @@ -584,6 +709,10 @@ def _is_id_token_valid(self, id_token):
% id_token['iss'])
return False

# Make sure that we only have a list of audiende when there are more than 1...chec
if 'aud' in id_token and isinstance(id_token['aud'], list) and len(id_token['aud']) == 1:
id_token['aud'] = id_token['aud'][0]

if isinstance(id_token['aud'], list):
# step 3 for audience list
if self.flow.client_id not in id_token['aud']:
Expand Down