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 all 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
2 changes: 1 addition & 1 deletion .secrets.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -167,5 +167,5 @@
}
]
},
"generated_at": "2023-09-19T21:25:48Z"
"generated_at": "2024-05-07T14:42:22Z"
}
28 changes: 27 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,23 @@ 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",
"id_token_username": "email",
"scope": "openid profile offline_access"
}
}
}
},
...
]
}
Expand All @@ -88,11 +105,20 @@ 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 `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"`.


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
21 changes: 16 additions & 5 deletions tests/aggregate_test.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import httpx
import json
import os
import uuid

from .conftest import (
Expand All @@ -8,6 +10,15 @@
)


def get_number_of_configured_idps():
with open(os.environ["SECRET_CONFIG"], "r") as f:
external_idps = json.load(f)["external_oidc"]
count = 0
for provider in external_idps:
count += len(provider.get("login_options", {}))
return count


def test_aggregate_user_user_endpoint(
app, client, persisted_refresh_tokens, auth_header
):
Expand All @@ -18,7 +29,7 @@ def test_aggregate_user_user_endpoint(
"""
res = client.get("/aggregate/user/user", headers=auth_header)
assert res.status_code == 200
assert len(res.json) == 2
assert len(res.json) == get_number_of_configured_idps()

default_commons_hostname = app.config["OIDC"]["default"]["commons_hostname"]
assert default_commons_hostname in res.json
Expand All @@ -45,7 +56,7 @@ def test_aggregate_user_user_endpoint_with_filters(
"/aggregate/user/user?filters=authz&filters=role", headers=auth_header
)
assert res.status_code == 200
assert len(res.json) == 2
assert len(res.json) == get_number_of_configured_idps()

default_commons_hostname = app.config["OIDC"]["default"]["commons_hostname"]
assert default_commons_hostname in res.json
Expand Down Expand Up @@ -95,7 +106,7 @@ def test_aggregate_endpoint_when_one_linked_commons_returns_500(
respx_mock.get(f"{idp_a_fence_url}/user").mock(return_value=httpx.Response(500))
res = client.get("/aggregate/user/user", headers=auth_header)
assert res.status_code == 200
assert len(res.json) == 2
assert len(res.json) == get_number_of_configured_idps()

default_commons_hostname = app.config["OIDC"]["default"]["commons_hostname"]
assert default_commons_hostname in res.json
Expand Down Expand Up @@ -152,7 +163,7 @@ def test_aggregate_authz_mapping_endpoint_with_no_connected_commons(
"""
res = client.get("/aggregate/authz/mapping", headers=auth_header)
assert res.status_code == 200
assert len(res.json) == 2
assert len(res.json) == get_number_of_configured_idps()

default_commons_hostname = app.config["OIDC"]["default"]["commons_hostname"]
assert default_commons_hostname in res.json
Expand Down Expand Up @@ -180,7 +191,7 @@ def test_aggregate_authz_mapping_endpoint_with_connected_commons(
"""
res = client.get("/aggregate/authz/mapping", headers=auth_header)
assert res.status_code == 200
assert len(res.json) == 2
assert len(res.json) == get_number_of_configured_idps()

default_commons_hostname = app.config["OIDC"]["default"]["commons_hostname"]
assert default_commons_hostname in res.json
Expand Down
6 changes: 6 additions & 0 deletions tests/app_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,13 @@ def test_app_config(app):
app.config["OIDC"]["idp_a"]["redirect_uri"]
== "https://workspace.planx-pla.net/wts-callback"
)
assert app.config["OIDC"]["idp_a"]["username_field"] == "context.user.name"
assert app.config["OIDC"]["idp_a"]["state_prefix"] == "test"
assert (
app.config["OIDC"]["externaldata-keycloak"]["authorize_url"]
== "https://external.data.repository/auth/realms/xyz/protocol/openid-connect/auth"
)
assert app.config["OIDC"]["externaldata-keycloak"]["username_field"] == "email"
assert (
app.config["OIDC"]["default"]["redirect_uri"]
== "https://test.workspace.planx-pla.net/wts/oauth2/authorize"
Expand Down
38 changes: 26 additions & 12 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ def mock_requests(
- obtaining JWT keys from Fence
- Fence's user info endpoint
Mock POST requests for:
- getting an access token from Fence using a refresh token
- getting an access token from Fence (and others) using a refresh token
"""
access_token_to_authz_resource = {}
for refresh_tokens in refresh_tokens_json.values():
Expand All @@ -278,11 +278,19 @@ def post_fence_token_side_effect(request):
)

def get_fence_user_side_effect(request):
access_token = request.headers["Authorization"].split(" ")[1]
assert access_token in access_token_to_authz_resource
return httpx.Response(
200,
json={
access_token = request.headers.get("Authorization", "").split(" ")
access_token = access_token[1] if len(access_token) > 0 else ""
if access_token not in access_token_to_authz_resource:
print(
"Mocked request '/user/user': no access token was provided. Assuming anonymous; returning empty access."
)
data = {
"authz": {},
"is_admin": False,
"role": "user",
}
else:
data = {
"authz": {
access_token_to_authz_resource[access_token]: [
{"method": "read", "service": "*"}
Expand All @@ -291,8 +299,8 @@ def get_fence_user_side_effect(request):
},
"is_admin": True,
"role": "admin",
},
)
}
return httpx.Response(200, json=data)

def get_arborist_authz_side_effect(request):
access_token = (
Expand All @@ -311,13 +319,19 @@ def create_mocks_for_user(app, respx_mock):
for idp_config in app.config["OIDC"].values():
fence_url = idp_config["api_base_url"].rstrip("/")

fence_token_url = f"{fence_url}/oauth2/token"
respx_mock.post(fence_token_url).mock(
# mock getting tokens
respx_mock.post(f"{fence_url}/oauth2/token").mock(
side_effect=post_fence_token_side_effect
)
respx_mock.post(
"https://external.data.repository/auth/realms/xyz/protocol/openid-connect/token"
).mock(side_effect=post_fence_token_side_effect)

fence_user_info_url = f"{fence_url}/user"
respx_mock.get(fence_user_info_url).mock(
# mock userinfo endpoint
respx_mock.get(f"{fence_url}/user").mock(
side_effect=get_fence_user_side_effect
)
respx_mock.get("https://external.data.repository/user/user").mock(
side_effect=get_fence_user_side_effect
)

Expand Down
18 changes: 18 additions & 0 deletions tests/test_settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,24 @@
"params": {"idp": "fence", "fence_idp": "shibboleth"}
}
}
},
{
"base_url": "https://external.data.repository",
"oidc_client_id": "test3",
"oidc_client_secret": "test3",
"redirect_uri": "https://workspace.planx-pla.net/wts-callback",
"login_options": {
"externaldata-keycloak": {
"name": "keycloak Login",
"params": {
"idp": "keycloak",
"auth_url": "auth/realms/xyz/protocol/openid-connect/auth",
"token_url": "auth/realms/xyz/protocol/openid-connect/token",
"id_token_username": "email",
"scope": "openid profile offline_access"
}
}
}
}
]
}
39 changes: 28 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,39 @@ 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", {})

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", "email")
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)

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
7 changes: 7 additions & 0 deletions wts/blueprints/aggregate.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ async def get_aggregate_response(endpoint):
# Initialzing refresh tokens with the keys of all the commons.
# This is needed to treat requests to un-connected commons as open access requests

# /!\ This does not support users being logged into more than 1 IDP for each Commons!!
# https://ctds-planx.atlassian.net/browse/PXP-11324
refresh_tokens = {
commons: None for commons in flask.current_app.config["COMMONS_HOSTNAMES"]
}
Expand Down Expand Up @@ -144,6 +146,11 @@ async def make_request(commons_hostname, endpoint, headers, parameters, filters)
)
)
return failure_indicator
except Exception as e:
flask.current_app.logger.error(
"Failed to get response from {}.".format(endpoint_url)
)
return failure_indicator

data = endpoint_response.json()
if filters:
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