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

Changes to make compatible with OAM #64

Merged
merged 18 commits into from
Jan 14, 2025
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
"plone.protect",
"plone.restapi>=8.34.0",
"oic",
"requests",
"z3c.form",
],
extras_require={
Expand Down
8 changes: 8 additions & 0 deletions src/pas/plugins/oidc/controlpanel/classic.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,14 @@ def user_property_as_userid(self):
def user_property_as_userid(self, value):
self.settings.user_property_as_userid = value

@property
def identity_domain_name(self):
return self.settings.identity_domain_name

@identity_domain_name.setter
def identity_domain_name(self, value):
self.settings.identity_domain_name = value


class OIDCSettingsForm(controlpanel.RegistryEditForm):
schema = IOIDCSettings
Expand Down
7 changes: 7 additions & 0 deletions src/pas/plugins/oidc/interfaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,13 @@ class IOIDCSettings(Interface):
default="sub",
)

identity_domain_name = schema.TextLine(
title=_("Identity Domain Name"),
description=_("Required for Oracle Authentication Manager only"),
required=False,
default="",
)


class IOIDCControlpanel(IControlpanel):
"""OIDC Control panel"""
44 changes: 43 additions & 1 deletion src/pas/plugins/oidc/plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import itertools
import plone.api as api
import requests
import string


Expand Down Expand Up @@ -69,6 +70,29 @@ class IOIDCPlugin(Interface):
""" """


class OAMClient(Client):
"""Override so we can adjust the jwks_uri to add domain needed for OAM"""

def __init__(self, *args, domain=None, **xargs):
super().__init__(self, *args, **xargs)
self.domain = domain
if domain:
session = requests.Session()
session.headers.update({"x-oauth-identity-domain-name": domain})
self.settings.requests_session = session

def handle_provider_config(self, pcr, issuer, keys=True, endpoints=True):
domain = self.domain
if domain:
# we need to modify jwks_uri in the provider_info to add the identityDomain for OAM
# gets used in https://github.com/CZ-NIC/pyoidc/blob/0bd1eadcefc5ccb7ef6c69d9b631537a7d3cfe30/src/oic/oauth2/__init__.py#L1132
url = pcr["jwks_uri"]
req = requests.PreparedRequest()
req.prepare_url(url, dict(identityDomainName=domain))
pcr["jwks_uri"] = req.url
return super().handle_provider_config(pcr, issuer, keys, endpoints)


@implementer(IOIDCPlugin)
class OIDCPlugin(BasePlugin):
"""PAS Plugin OpenID Connect."""
Expand All @@ -92,6 +116,7 @@ class OIDCPlugin(BasePlugin):
use_deprecated_redirect_uri_for_logout = False
use_modified_openid_schema = False
user_property_as_userid = "sub"
identity_domain_name = ""

_properties = (
dict(id="title", type="string", mode="w", label="Title"),
Expand Down Expand Up @@ -160,6 +185,12 @@ class OIDCPlugin(BasePlugin):
mode="w",
label="User info property used as userid, default 'sub'",
),
dict(
id="identity_domain_name",
type="string",
mode="w",
label="Identity Domain Name (required for Oracle Authentication Manager only)",
),
)

def __init__(self, id, title=None):
Expand Down Expand Up @@ -331,8 +362,19 @@ def _setupJWTTicket(self, user_id, user):

# TODO: memoize (?)
def get_oauth2_client(self):
domain = self.getProperty("identity_domain_name")
try:
client = Client(client_authn_method=CLIENT_AUTHN_METHOD)
if domain:
client = OAMClient(
client_authn_method=CLIENT_AUTHN_METHOD,
domain=domain,
)
else:
client = Client(client_authn_method=CLIENT_AUTHN_METHOD)
client.allow["issuer_mismatch"] = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes! I noticed the same issue with the Microsoft implementation. However, in my opinion, it might be better to have this in a separate property, even though this plugin already seems to have too many configurations.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can put a toggle for it. It's not exactly clear what the implications are if allow mismatch. I guess its less secure? Could have a toggle that defaults mismatch allowed so it's less likely to trip up first timers?

True # Some providers aren't configured with configured and issuer urls the same even though they should.
)

# registration_response = client.register(provider_info["registration_endpoint"], redirect_uris=...)
# ... oic.exception.RegistrationError: {'error': 'insufficient_scope',
# 'error_description': "Policy 'Trusted Hosts' rejected request to client-registration service. Details: Host not trusted."}
Expand Down
2 changes: 2 additions & 0 deletions src/pas/plugins/oidc/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ def authorization_flow_args(plugin: plugins.OIDCPlugin, session: Session) -> dic
"nonce": session.get("nonce"),
"redirect_uri": plugin.get_redirect_uris(),
}
if plugin.getProperty("identity_domain_name"):
args["domain"] = plugin.getProperty("identity_domain_name", "")
if plugin.getProperty("use_pkce"):
# Build a random string of 43 to 128 characters
# and send it in the request as a base64-encoded urlsafe string of the sha256 hash of that string
Expand Down
1 change: 1 addition & 0 deletions tests/services/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ def keycloak(keycloak_service):
"scope": ("openid", "profile", "email"),
"redirect_uris": ("/login_oidc/oidc",),
"create_restapi_ticket": True,
"identity_domain_name": "blah", # ensure non OAM SP ignores extra params/header
}


Expand Down
Loading