mirror of
https://github.com/galaxyproject/galaxy.git
synced 2026-09-24 16:30:27 +08:00
fix(authnz): parse <idphint> from oidc_backends_config.xml
The <idphint> element was already defined in the XSD schema
(oidc_backends_config.xsd, minOccurs=0) but _parse_idp_config() in
managers.py had no branch to read it into the oidc_backend_config dict.
As a result, PSAAuthnz._setup_idp() always set IDPHINT=None (from
oidc_backend_config.get('idphint') returning None), and the Keycloak
backend's auth_extra_params() silently skipped kc_idp_hint entirely:
# keycloak.py
if idphint := self.setting('IDPHINT', 'oidc'): # None → falsy → skipped
params['kc_idp_hint'] = idphint
This caused Keycloak to show its own login form instead of redirecting
users directly to the configured federated Identity Provider (e.g.
Switch edu-ID, AAI, SWITCHaai).
Fix: add the missing parsing branch after the checkin_env block,
exactly following the pattern used for every other optional element.
Also adds three regression tests:
- test_parse_idphint_from_xml: <idphint> is in the parsed dict
- test_idphint_propagated_to_psa_config: IDPHINT flows through to PSAAuthnz
- test_missing_idphint_is_none: absent <idphint> → IDPHINT=None (no kc_idp_hint sent)
This commit is contained in:
@@ -187,6 +187,12 @@ class AuthnzManager:
|
||||
if config_xml.find("checkin_env") is not None:
|
||||
rtv["checkin_env"] = config_xml.find("checkin_env").text
|
||||
|
||||
# Keycloak/CILogon IDP hint: tells Keycloak which federated IdP to redirect
|
||||
# to directly (kc_idp_hint), bypassing the Keycloak login page.
|
||||
# Corresponds to <idphint> in oidc_backends_config.xml (already in XSD).
|
||||
if config_xml.find("idphint") is not None:
|
||||
rtv["idphint"] = config_xml.find("idphint").text
|
||||
|
||||
return rtv
|
||||
|
||||
def _parse_custos_config(self, config_xml):
|
||||
|
||||
@@ -119,3 +119,82 @@ def test_psa_authnz_config(mock_app):
|
||||
app_config=mock_app.config,
|
||||
)
|
||||
assert psa_authnz.config[setting_name("USERNAME_KEY")] == config_values["username_key"]
|
||||
|
||||
|
||||
def _create_backend_config_with_idphint(idphint_value: str = None) -> tuple[str, str]:
|
||||
"""Create a Keycloak backend config, optionally including an <idphint> element."""
|
||||
idphint_element = f" <idphint>{idphint_value}</idphint>" if idphint_value else ""
|
||||
contents = f"""<?xml version="1.0"?>
|
||||
<OIDC>
|
||||
<provider name="keycloak">
|
||||
<url>https://auth.example.org/realms/MyRealm/</url>
|
||||
<client_id>galaxy-oidc</client_id>
|
||||
<client_secret>secret</client_secret>
|
||||
<redirect_uri>https://galaxy.example.org/authnz/keycloak/callback</redirect_uri>
|
||||
<enable_idp_logout>true</enable_idp_logout>
|
||||
{idphint_element}
|
||||
</provider>
|
||||
</OIDC>
|
||||
"""
|
||||
file = tempfile.NamedTemporaryFile(mode="w", delete=False, suffix=".xml")
|
||||
file.write(contents)
|
||||
file.flush()
|
||||
return contents, file.name
|
||||
|
||||
|
||||
def test_parse_idphint_from_xml(mock_app):
|
||||
"""
|
||||
Regression test: <idphint> in oidc_backends_config.xml must be parsed into
|
||||
the oidc_backend_config dict so that PSAAuthnz can forward it as IDPHINT to
|
||||
the Keycloak/CILogon backends (which use it to set kc_idp_hint).
|
||||
|
||||
Previously, _parse_idp_config() had no branch for <idphint>, so the element
|
||||
was silently ignored and oidc_backend_config.get("idphint") always returned
|
||||
None, causing kc_idp_hint to never be sent to Keycloak.
|
||||
"""
|
||||
_, oidc_path = create_oidc_config()
|
||||
_, backend_path = _create_backend_config_with_idphint(idphint_value="my-switch-edu-id")
|
||||
manager = AuthnzManager(app=mock_app, oidc_config_file=oidc_path, oidc_backends_config_file=backend_path)
|
||||
parsed = manager.oidc_backends_config["keycloak"]
|
||||
assert "idphint" in parsed, "<idphint> element must be parsed into oidc_backend_config dict"
|
||||
assert parsed["idphint"] == "my-switch-edu-id"
|
||||
|
||||
|
||||
def test_idphint_propagated_to_psa_config(mock_app):
|
||||
"""
|
||||
When <idphint> is configured, PSAAuthnz must expose it as IDPHINT in its
|
||||
config so the Keycloak/CILogon PSA backend can add kc_idp_hint to the
|
||||
authorization URL.
|
||||
"""
|
||||
from galaxy.authnz.psa_authnz import PSAAuthnz
|
||||
|
||||
_, oidc_path = create_oidc_config()
|
||||
_, backend_path = _create_backend_config_with_idphint(idphint_value="stage")
|
||||
manager = AuthnzManager(app=mock_app, oidc_config_file=oidc_path, oidc_backends_config_file=backend_path)
|
||||
psa = PSAAuthnz(
|
||||
provider="keycloak",
|
||||
oidc_config=manager.oidc_config,
|
||||
oidc_backend_config=manager.oidc_backends_config["keycloak"],
|
||||
app_config=mock_app.config,
|
||||
)
|
||||
assert psa.config.get("IDPHINT") == "stage", "IDPHINT must be 'stage' when <idphint>stage</idphint> is in XML"
|
||||
|
||||
|
||||
def test_missing_idphint_is_none(mock_app):
|
||||
"""
|
||||
When <idphint> is absent from the XML, IDPHINT must be None (not a hardcoded
|
||||
default string like 'oidc'), so the Keycloak backend omits kc_idp_hint
|
||||
entirely rather than sending a wrong value.
|
||||
"""
|
||||
from galaxy.authnz.psa_authnz import PSAAuthnz
|
||||
|
||||
_, oidc_path = create_oidc_config()
|
||||
_, backend_path = _create_backend_config_with_idphint(idphint_value=None)
|
||||
manager = AuthnzManager(app=mock_app, oidc_config_file=oidc_path, oidc_backends_config_file=backend_path)
|
||||
psa = PSAAuthnz(
|
||||
provider="keycloak",
|
||||
oidc_config=manager.oidc_config,
|
||||
oidc_backend_config=manager.oidc_backends_config["keycloak"],
|
||||
app_config=mock_app.config,
|
||||
)
|
||||
assert psa.config.get("IDPHINT") is None, "IDPHINT must be None when <idphint> is absent from XML"
|
||||
|
||||
Reference in New Issue
Block a user