-
Notifications
You must be signed in to change notification settings - Fork 3
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
HP-1361 Feat/keycloak external OIDC #77
Conversation
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
idp_client = idp_params.get("idp", "") | ||
|
||
if "auth_url" in idp_params and "token_url" in idp_params: # Generic OIDC | ||
auth_endpoint = idp_params.get("auth_url") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is truly "Generic OIDC" b/c it's possible other providers have different fields for the username (for example).
I still think we need a little more abstraction here. A function call at minimum. It's unclear how to expand what's here if a new external OIDC format is introduced.
We need something like:
if idp == "keycloak":
idp_metadata = get_idp_metadata_from_keycloak()
elif idp == "foobar":
idp_metadata = get_idp_metadata_from_foobar()
else:
idp_metadata = get_idp_metadata_from_default()
Or a more OOO pattern. But this is maybe over engineering
def get_idp_metadata(idp, metadata):
if idp == "keycloak":
idp_metadata = IdpMetadataKeycloak(metadata)
elif idp == "foobar":
idp_metadata = IdpMetadataFoobar(metadata)
else:
idp_metadata = IdpMetadataDefault(metadata)
return idp_metadata
def IdpMetadataBase(object):
def __init__(metadata):
# do common stuff here
def IdpMetadataKeycloak(IdpMetadataBase):
def __init__(metadata):
super().__init__(metadata)
# do idp-specific stuff here and/or override common stuff
self.scope = ...
self.authorization_url = ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But with the changes the other providers can specify their field for the username in the config.
I can change it to the first solution, but then every new idp that comes in needs a new function. I feel like OIDC is specific enough that we don't need to have an if statement for every idp or a new function for each idc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Avantol13's comment is not resolved but this lgtm, i think it's configurable enough to support generic oidc
It's unclear how to expand what's here if a new external OIDC format is introduced.
if we need to configure with something else than auth_url
and token_url
(eg with a .well-known
url) i think we can address that then
README.md
Outdated
The `id_token_username` property for OIDC clients can be configured with `.` in between strings for a nested username inside a token. | ||
For example if the token jwt has username encoded in the json as `token["context"]["user"]["name"]`. | ||
We can write this in the paramaters as `"id_token_username": "context.user.name"` | ||
The default if nothing is specified for a fence client it defaults to `"context.user.name"` for a non-fence client the default is `"email"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `id_token_username` property for OIDC clients can be configured with `.` in between strings for a nested username inside a token. | |
For example if the token jwt has username encoded in the json as `token["context"]["user"]["name"]`. | |
We can write this in the paramaters as `"id_token_username": "context.user.name"` | |
The default if nothing is specified for a fence client it defaults to `"context.user.name"` for a non-fence client the default is `"email"` | |
The `id_token_username` property for OIDC clients can be configured with `.` in between strings for a nested username inside a token. | |
For example if the token jwt has username encoded in the json as `token["context"]["user"]["name"]`: | |
We can write this in the parameters as `"id_token_username": "context.user.name"`. | |
If nothing is specified, for a fence client the default is `"context.user.name"`, for a non-fence client the default is `"email"`. |
*addressed |
Link to JIRA ticket if there is one: https://ctds-planx.atlassian.net/browse/HP-1361
New Features
Adding non-fence external token flow to WTS. This can be configured in the appcreds for other OIDC clients.