Dynamic Provider - #70
Conversation
|
Travis reports 1 test is failing on Python 2.6. |
puiterwijk
left a comment
puiterwijk
left a comment
There was a problem hiding this comment.
Thanks for this!
Could you look at my comments, and preferably look at adding some documentation for this?
| # 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(): |
There was a problem hiding this comment.
This would need to move to init_app, since when we get here, the developer might not yet have provided an app instance.
| with app.app_context(): | ||
| self.init_provider(provider) | ||
|
|
||
| def __exit__(self, exception_type, exception_value, traceback): |
There was a problem hiding this comment.
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?
| # get stuff from the app's config, which may override stuff set above | ||
| if app is not None: | ||
|
|
||
| if app: |
There was a problem hiding this comment.
Why did you change this line? The intention is that it'll be called with either a flask.Flask instance or None.
| """ | ||
|
|
||
| secrets = self.load_secrets(provider) | ||
| assert secrets != None, "Problem with loading secrets" |
There was a problem hiding this comment.
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.
| Do setup for a specific provider | ||
|
|
||
| :param provider: The provider to initialize. | ||
| :type provider: Dictionary with at lease 'base_url' item |
| def load_secrets(self, provider): | ||
|
|
||
| try: | ||
| static_secrets = current_app.config.get('OIDC_CLIENT_SECRETS', None) |
There was a problem hiding this comment.
Maybe return an error if provider is not None in this case, to avoid confusing people?
|
|
||
| registration = provider.get('registration', None) | ||
|
|
||
| if not registration: |
There was a problem hiding this comment.
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.
| if not provider: | ||
| raise Exception("No Provider specified") | ||
|
|
||
| try: |
There was a problem hiding this comment.
This function is kind of long.
Maybe add two helper functions def _perform_discovery() and def _perform_dynamic_registration()?
|
|
||
| try: | ||
| try: | ||
| jwks_keys = json.load( |
There was a problem hiding this comment.
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.
| except Exception as e: | ||
| raise Exception("Error in preparing result: {}".format(str(e))) | ||
|
|
||
| raise Exception("No secrets loaded !") |
There was a problem hiding this comment.
Is there any way for us to arrive at this line of code?
|
@HarryKodden If you want this resolved i'm maintaining a fork of this with some added extensibility. Feel free to open a PR here: |
3 participants
Add ability to connect to an OIDC OP at Runtime. For backwards compatibility the existing config option OIDC_CLIENT_SECRETS is still supported.
Usage for dynamic OP connectivity is:
Initialize the Flask App as normal:
Specify the Provider to connect with:
Now finalize the configuration for this OIDC Provider (meaning the discovery will take place)
And use the authentication as normal: