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

HP-1361 Feat/keycloak external OIDC #77

Merged
merged 44 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
30b8bb3
adding keycloak idp auth url
MichaelLukowski Mar 19, 2024
44038b7
update keycloak idp if statement
MichaelLukowski Mar 19, 2024
9ea986b
fix link and debug statment
MichaelLukowski Mar 19, 2024
b2c3811
config changes for keycloak
MichaelLukowski Mar 19, 2024
b664f46
fix python syntax
MichaelLukowski Mar 19, 2024
f781b33
scope changes for keycloak
MichaelLukowski Mar 19, 2024
4e30fd5
adding debug statement for refresh
MichaelLukowski Apr 1, 2024
8a0903d
ensure oidc configure correctly
MichaelLukowski Apr 1, 2024
8f68e0f
udpate keycloak token url
MichaelLukowski Apr 2, 2024
accc308
update format for token exchange urls
MichaelLukowski Apr 2, 2024
bc3699b
change keycloak scope
MichaelLukowski Apr 2, 2024
83d4ed4
update keycloak token fetch
MichaelLukowski Apr 2, 2024
55c8b15
typo fix
MichaelLukowski Apr 2, 2024
61a2c06
change auth grant_type for keycloak
MichaelLukowski Apr 2, 2024
eed8251
another change to token fetching
MichaelLukowski Apr 2, 2024
8aa2f89
adding debug for authorization url
MichaelLukowski Apr 2, 2024
9f7c60c
fix debug statement
MichaelLukowski Apr 2, 2024
7f3315d
fix realms typo
MichaelLukowski Apr 3, 2024
f42074f
add loging statement for tokens that are not from fence:
MichaelLukowski Apr 4, 2024
c4fe619
expiration for keycloak
MichaelLukowski Apr 4, 2024
bf3bc7a
change keycloak expiration
MichaelLukowski Apr 10, 2024
2d47904
remove most debug statements and clean up
MichaelLukowski Apr 16, 2024
4016e94
update api setup for keycloak
MichaelLukowski Apr 16, 2024
4c22cd3
adding debug statements
MichaelLukowski Apr 16, 2024
f332c15
debug oidc auth url
MichaelLukowski Apr 16, 2024
e487c06
fix keycloak urls
MichaelLukowski Apr 16, 2024
cafb326
keycloak auth url
MichaelLukowski Apr 16, 2024
11fef27
fix scope
MichaelLukowski Apr 16, 2024
d7e77e4
update readme
MichaelLukowski Apr 16, 2024
531ffb8
clean up comments
MichaelLukowski Apr 16, 2024
162751d
update external oidc check
MichaelLukowski May 1, 2024
991d181
debug oauth client
MichaelLukowski May 1, 2024
606c255
new token username parsing
MichaelLukowski May 2, 2024
585ab75
fix oidc client metadata
MichaelLukowski May 2, 2024
53a6fd7
change config and added debug
MichaelLukowski May 2, 2024
f473e11
update urls to use urljoin
MichaelLukowski May 3, 2024
ce109ac
address PR comments
MichaelLukowski May 6, 2024
a78b851
update config tests
MichaelLukowski May 6, 2024
090643e
update tests for new config
MichaelLukowski May 7, 2024
96704c6
Fix aggregate tests
paulineribeyre May 8, 2024
eb5876a
Fix to support aggregate tests' anonymous requests
paulineribeyre May 8, 2024
4a078e4
link ticket
paulineribeyre May 8, 2024
28d165c
update readme for username
MichaelLukowski May 8, 2024
d0e1a61
Update README.md
paulineribeyre May 9, 2024
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
20 changes: 19 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,22 @@ Two methods exist for authenticating a request to the `/token` endpoint:
...
}
},
{
"base_url": "https://non-fence-data.org/",
"oidc_client_id": "xxx",
"oidc_client_secret": "xxx",
"login_options": {
"externaldata-keycloak": {
"name": "keycloak Login",
"params": {
"idp": "keycloak",
"auth_url": "auth/realms/xyz/protocol/openid-connect/auth",
paulineribeyre marked this conversation as resolved.
Show resolved Hide resolved
"token_url": "auth/realms/xyz/protocol/openid-connect/token",
"scope": "openid profile offline_access"
}
}
}
},
...
]
}
Expand All @@ -88,11 +104,13 @@ Note that IDP IDs (`other-google` and `other-orcid` in the example above) must b
Also note that the OIDC clients you create must be granted `read-storage` access to all the data in the external
Data Commons via the data-commons' `user.yaml`.

Finally, the `redirect_uri` property for external OIDC providers is
The `redirect_uri` property for external OIDC providers is
an optional field that supports sharing OIDC client
configuration between multiple workspace deployments
as part of a multi-account application system.

Finally, non fence IDPs can be provided given their auth url, token url, and necessary scope as a part part of the `params` of the external IDP.

The key `aggregate_endpoint_allowlist` is an optional key which consists of a list of endpoints that are supported by the `/aggregate` api.

## Dev-Test
Expand Down
40 changes: 29 additions & 11 deletions wts/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import flask
from flask import Flask
import json
from urllib.parse import urlparse
from urllib.parse import urlparse, urljoin
from cdislogging import get_logger
from cdiserrors import APIError

Expand Down Expand Up @@ -59,9 +59,10 @@ def load_settings(app):
"client_secret": get_var("OIDC_CLIENT_SECRET"),
"commons_hostname": urlparse(fence_base_url).netloc,
"api_base_url": fence_base_url,
"authorize_url": fence_base_url + "oauth2/authorize",
"access_token_url": fence_base_url + "oauth2/token",
"redirect_uri": wts_base_url + "oauth2/authorize",
"authorize_url": urljoin(fence_base_url, "oauth2/authorize"),
"access_token_url": urljoin(fence_base_url, "oauth2/token"),
"redirect_uri": urljoin(wts_base_url, "oauth2/authorize"),
"username_field": "context.user.name",
"scope": "openid data user",
"state_prefix": "",
}
Expand All @@ -74,23 +75,40 @@ def load_settings(app):
redirect_uri = get_var("REDIRECT_URI", default="", secret_config=conf)
state_prefix = wts_hostname or ""
if not redirect_uri:
redirect_uri = wts_base_url + "oauth2/authorize"
redirect_uri = urljoin(wts_base_url, "oauth2/authorize")
state_prefix = ""

for idp, idp_conf in conf.get("login_options", {}).items():
authorization_url = fence_base_url + "oauth2/authorize"
authorization_url = add_params_to_uri(
authorization_url, idp_conf.get("params", {})
)
scope = "openid data user"
idp_params = idp_conf.get("params", {})
idp_client = idp_params.get("idp", "")
paulineribeyre marked this conversation as resolved.
Show resolved Hide resolved

if "auth_url" in idp_params and "token_url" in idp_params: # Generic OIDC
auth_endpoint = idp_params.get("auth_url")
Copy link
Contributor

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 = ...

Copy link
Member Author

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.

token_endpoint = idp_params.get("token_url")
username_field = idp_params.get("id_token_username_field", "")
paulineribeyre marked this conversation as resolved.
Show resolved Hide resolved
scope = idp_params.get("scope", scope)
authorization_url = urljoin(url, auth_endpoint)
paulineribeyre marked this conversation as resolved.
Show resolved Hide resolved
access_token_url = urljoin(url, token_endpoint)
paulineribeyre marked this conversation as resolved.
Show resolved Hide resolved

else: # Default Gen3 Fence Integration
authorization_url = urljoin(fence_base_url, "oauth2/authorize")
access_token_url = urljoin(fence_base_url, "oauth2/token")
username_field = "context.user.name"
authorization_url = add_params_to_uri(
authorization_url, idp_conf.get("params", {})
)

app.config["OIDC"][idp] = {
"client_id": get_var("OIDC_CLIENT_ID", secret_config=conf),
"client_secret": get_var("OIDC_CLIENT_SECRET", secret_config=conf),
"commons_hostname": urlparse(fence_base_url).netloc,
"api_base_url": fence_base_url,
"username_field": username_field,
"authorize_url": authorization_url,
"access_token_url": fence_base_url + "oauth2/token",
"access_token_url": access_token_url,
"redirect_uri": redirect_uri,
"scope": "openid data user",
"scope": scope,
"state_prefix": state_prefix,
}

Expand Down
2 changes: 1 addition & 1 deletion wts/blueprints/oauth2.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def get_authorization_url():

requested_idp = flask.request.args.get("idp", "default")
client = get_oauth_client(idp=requested_idp)
# This will be the value that was put in the ``metadata`` in config.
# This will be the value is in /wts/api.py
state_prefix = client.metadata.get("state_prefix")
authorize_url = client.metadata.get("authorize_url")
state = generate_token()
Expand Down
18 changes: 15 additions & 3 deletions wts/resources/oauth2.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,22 @@ def client_do_authorize():
requested_idp = flask.session.get("idp", "default")
client = get_oauth_client(idp=requested_idp)
token_url = client.metadata["access_token_url"]

# username_field is defined for external oidc clients but it defaults
# to context.user.name for fence clients
username_field = client.metadata["username_field"]

mismatched_state = (
"state" not in flask.request.args
or "state" not in flask.session
or flask.request.args["state"] != flask.session.pop("state")
)

if mismatched_state:
raise AuthError("could not authorize; state did not match across auth requests")
try:
tokens = client.fetch_token(token_url, **flask.request.args.to_dict())
refresh_refresh_token(tokens, requested_idp)
refresh_refresh_token(tokens, requested_idp, username_field)
except KeyError as e:
raise AuthError("error in token response: {}".format(tokens))
except AuthlibBaseError as e:
Expand All @@ -43,7 +49,7 @@ def find_valid_refresh_token(username, idp):
return has_valid


def refresh_refresh_token(tokens, idp):
def refresh_refresh_token(tokens, idp, username_field):
"""
store new refresh token in db and purge all old tokens for the user
"""
Expand All @@ -59,6 +65,7 @@ def refresh_refresh_token(tokens, idp):
"verify_at_hash": False,
"leeway": 0,
}

refresh_token = tokens["refresh_token"]
id_token = tokens["id_token"]
# TODO: verify signature with authutils
Expand All @@ -84,9 +91,14 @@ def refresh_refresh_token(tokens, idp):
user = current_user
username = user.username

idp_username = id_token
# username field is written like "context.user.name" so we split it and loop through the segments
for field in username_field.split("."):
idp_username = idp_username.get(field)

flask.current_app.logger.info(
'Linking username "{}" for IdP "{}" to current user "{}"'.format(
id_token["context"]["user"]["name"], idp, username
idp_username, idp, username
)
)
new_token = RefreshToken(
Expand Down
Loading