diff --git a/docs/oidc.md b/docs/oidc.md index 0c3982e..7f85b39 100644 --- a/docs/oidc.md +++ b/docs/oidc.md @@ -30,7 +30,19 @@ Configure in **Settings → Security → Authentication Method → OIDC**. | Auto-Provision Users | Create accounts on first login | `true` | | Login Button Label | Custom text for the sign-in button | — | -Use **Test Connection** to verify discovery and client configuration before attempting login. +Use **Test Connection** to verify discovery, client configuration, and the provider's token signing keys (JWKS) before attempting login. + +> **Authentik users:** make sure your provider has a **Signing Key** selected (e.g. the default self-signed certificate). Without one, Authentik serves an empty JWKS document and every login fails with an OIDC callback error, even though the discovery document looks healthy. + +## Account Linking + +On login, Shelfmark matches the OIDC identity to a user account in this order: + +1. **OIDC subject** — a user who has logged in through this provider before. +2. **Email** — a local account with the same (unique) email address. This only happens when the provider also asserts `email_verified: true` for the address; an unverified email would let anyone claim a local account by registering its address at the IdP. +3. Otherwise, a new account is created when **Auto-Provision Users** is enabled (username conflicts get a numeric suffix), or the login is rejected with "Account not found" when it is disabled. + +If the `email_verified` claim is missing or `false`, email linking is silently skipped — a common surprise when the address was never verified at the identity provider (e.g. Keycloak's **Email verified** toggle on the user, or Authentik accounts created without email verification). Make sure the `email` scope is requested and the address is marked verified in your IdP. ## Environment Variables @@ -46,6 +58,8 @@ If `DISABLE_LOCAL_AUTH` and `OIDC_AUTO_REDIRECT` are both enabled, users are red ## Troubleshooting +- **No token signing keys (empty JWKS)** — The provider's JWKS endpoint returned no keys, so ID tokens can't be verified. In Authentik this happens when the provider has no **Signing Key** selected; pick one (e.g. the default self-signed certificate) and try again. - **Issuer validation failed** — The issuer in the token doesn't match the discovery document. Check your provider's external URL / issuer configuration. - **Callback URL mismatch** — Reverse proxy isn't forwarding `X-Forwarded-Proto` or `X-Forwarded-Host`, so the constructed callback URL doesn't match what's registered in the provider. -- **Account not found** — Auto-provision is disabled and the user hasn't been pre-created by an admin. +- **Account not found** — Auto-provision is disabled and the user hasn't been pre-created by an admin. If you pre-created the account with a matching email, see [Account Linking](#account-linking): the provider must send `email_verified: true` for linking to happen. +- **Login created a duplicate account instead of linking to my local one** — Email linking requires a verified email; see [Account Linking](#account-linking). With `DEBUG=true`, the log notes when linking is skipped because the address isn't verified. diff --git a/shelfmark/config/security_handlers.py b/shelfmark/config/security_handlers.py index 990e25e..c562aef 100644 --- a/shelfmark/config/security_handlers.py +++ b/shelfmark/config/security_handlers.py @@ -115,7 +115,7 @@ def check_oidc_connection( response.raise_for_status() document = response.json() - required_fields = ["issuer", "authorization_endpoint", "token_endpoint"] + required_fields = ["issuer", "authorization_endpoint", "token_endpoint", "jwks_uri"] missing_fields = [field for field in required_fields if field not in document] if missing_fields: return { @@ -123,6 +123,24 @@ def check_oidc_connection( "message": f"Discovery document missing fields: {', '.join(missing_fields)}", } + # Logins verify the ID token against the provider's JWKS, so an empty key + # set (e.g. an Authentik provider with no Signing Key selected) means every + # login will fail even though discovery looks healthy. + jwks_uri = str(document["jwks_uri"]) + jwks_response = requests.get(jwks_uri, timeout=10, verify=get_ssl_verify(jwks_uri)) + jwks_response.raise_for_status() + jwks_document = jwks_response.json() + jwks_keys = jwks_document.get("keys") if isinstance(jwks_document, dict) else None + if not jwks_keys: + return { + "success": False, + "message": ( + "Discovery document is valid, but the provider returned no token " + "signing keys (empty JWKS), so logins will fail. If you use " + "Authentik, select a Signing Key in the provider settings." + ), + } + return {"success": True, "message": f"Connected to {document['issuer']}"} except Exception as exc: logger.exception("OIDC connection test failed") diff --git a/shelfmark/core/oidc_routes.py b/shelfmark/core/oidc_routes.py index c0e589d..42ec8a6 100644 --- a/shelfmark/core/oidc_routes.py +++ b/shelfmark/core/oidc_routes.py @@ -33,6 +33,11 @@ logger = setup_logger(__name__) oauth = OAuth() _RETURN_TO_SESSION_KEY = "oidc_return_to" _OIDC_CLIENT_ERRORS = (OAuthError, OSError, RuntimeError, TypeError, ValueError) +_EMPTY_JWKS_MESSAGE = ( + "Authentication failed: the identity provider returned no token signing keys " + "(empty JWKS). If you use Authentik, select a Signing Key in the provider " + "settings and try again." +) class _ClaimsMappingLike(Protocol): @@ -121,6 +126,17 @@ def _normalize_return_to(raw_return_to: object) -> str | None: return urlunsplit(("", "", path, parsed.query, parsed.fragment)) +def _idp_jwks_has_no_keys(client: Any) -> bool: + """Return True when the IdP's JWKS document verifiably contains no signing keys.""" + try: + jwk_set = client.fetch_jwk_set(force=True) + except (*_OIDC_CLIENT_ERRORS, KeyError): + return False + if not isinstance(jwk_set, Mapping): + return False + return not jwk_set.get("keys") + + def _get_pending_return_to(*, clear: bool = False) -> str | None: """Read the pending post-login target from the session.""" raw_return_to = ( @@ -274,6 +290,17 @@ def register_oidc_routes(app: Flask, user_db: UserDB) -> None: return redirect( _login_error_url(f"OIDC token claim validation failed: {claim_name}") ) + except KeyError, ValueError: + # An IdP serving an empty JWKS document (e.g. an Authentik provider + # with no Signing Key selected) surfaces as KeyError('keys') while + # importing the key set. Test Connection only validates discovery, + # so this is the first place the misconfiguration becomes visible. + if _idp_jwks_has_no_keys(client): + logger.exception( + "OIDC callback failed: the IdP JWKS document contains no signing keys" + ) + return redirect(_login_error_url(_EMPTY_JWKS_MESSAGE)) + raise claims = _normalize_claims(token.get("userinfo")) # If userinfo is missing or claims are too sparse, request it explicitly. @@ -306,6 +333,12 @@ def register_oidc_routes(app: Flask, user_db: UserDB) -> None: is_admin = admin_group in groups allow_email_link = bool(user_info.get("email")) and _is_email_verified(claims) + if user_info.get("email") and not allow_email_link: + logger.debug( + "OIDC email %s is not marked verified by the IdP; skipping " + "email-based account linking", + user_info["email"], + ) user = provision_oidc_user( user_db, user_info, diff --git a/tests/config/test_security_handlers.py b/tests/config/test_security_handlers.py new file mode 100644 index 0000000..edd9166 --- /dev/null +++ b/tests/config/test_security_handlers.py @@ -0,0 +1,82 @@ +"""Tests for the OIDC Test Connection handler.""" + +from unittest.mock import MagicMock, patch + +from shelfmark.config.security_handlers import check_oidc_connection + +DISCOVERY_URL = "https://auth.example.com/.well-known/openid-configuration" + +DISCOVERY_DOCUMENT = { + "issuer": "https://auth.example.com", + "authorization_endpoint": "https://auth.example.com/authorize", + "token_endpoint": "https://auth.example.com/token", + "jwks_uri": "https://auth.example.com/jwks", +} + + +def _mock_response(payload): + response = MagicMock() + response.json.return_value = payload + response.raise_for_status.return_value = None + return response + + +def _run_check(responses): + """Run check_oidc_connection with requests.get returning the given responses.""" + with ( + patch("requests.get", side_effect=responses) as mock_get, + patch("shelfmark.config.security_handlers.get_ssl_verify", return_value=True), + ): + result = check_oidc_connection( + load_security_config=lambda: {"OIDC_DISCOVERY_URL": DISCOVERY_URL}, + current_values={}, + logger=MagicMock(), + ) + return result, mock_get + + +class TestCheckOIDCConnection: + def test_succeeds_when_discovery_and_jwks_are_valid(self): + responses = [ + _mock_response(DISCOVERY_DOCUMENT), + _mock_response({"keys": [{"kty": "RSA", "kid": "abc"}]}), + ] + result, mock_get = _run_check(responses) + assert result["success"] is True + assert "Connected to" in result["message"] + jwks_call = mock_get.call_args_list[1] + assert jwks_call.args[0] == DISCOVERY_DOCUMENT["jwks_uri"] + + def test_fails_with_signing_key_guidance_when_jwks_is_empty(self): + responses = [ + _mock_response(DISCOVERY_DOCUMENT), + _mock_response({}), + ] + result, _ = _run_check(responses) + assert result["success"] is False + assert "no token signing keys" in result["message"] + assert "Signing Key" in result["message"] + + def test_fails_with_signing_key_guidance_when_jwks_keys_list_is_empty(self): + responses = [ + _mock_response(DISCOVERY_DOCUMENT), + _mock_response({"keys": []}), + ] + result, _ = _run_check(responses) + assert result["success"] is False + assert "no token signing keys" in result["message"] + + def test_fails_when_discovery_document_missing_jwks_uri(self): + document = {k: v for k, v in DISCOVERY_DOCUMENT.items() if k != "jwks_uri"} + responses = [_mock_response(document)] + result, _ = _run_check(responses) + assert result["success"] is False + assert "jwks_uri" in result["message"] + + def test_fails_when_jwks_request_errors(self): + jwks_response = MagicMock() + jwks_response.raise_for_status.side_effect = RuntimeError("boom") + responses = [_mock_response(DISCOVERY_DOCUMENT), jwks_response] + result, _ = _run_check(responses) + assert result["success"] is False + assert "Connection failed" in result["message"] diff --git a/tests/core/test_oidc_routes.py b/tests/core/test_oidc_routes.py index 48774f6..2fe8a91 100644 --- a/tests/core/test_oidc_routes.py +++ b/tests/core/test_oidc_routes.py @@ -413,6 +413,43 @@ class TestOIDCCallbackEndpoint: assert error is not None assert "issuer validation failed" in error + @patch("shelfmark.core.oidc_routes._get_oidc_client") + def test_callback_redirects_with_signing_key_guidance_on_empty_jwks( + self, mock_get_client, client + ): + fake_client = Mock() + fake_client.authorize_access_token.side_effect = KeyError("keys") + fake_client.fetch_jwk_set.return_value = {} + mock_get_client.return_value = (fake_client, MOCK_OIDC_CONFIG) + + resp = client.get("/api/auth/oidc/callback?code=abc123&state=test-state") + error = _get_oidc_error(resp) + assert error is not None + assert "no token signing keys" in error + assert "Signing Key" in error + + @patch("shelfmark.core.oidc_routes._get_oidc_client") + def test_callback_uses_generic_error_when_jwks_has_keys(self, mock_get_client, client): + fake_client = Mock() + fake_client.authorize_access_token.side_effect = KeyError("keys") + fake_client.fetch_jwk_set.return_value = {"keys": [{"kty": "RSA", "kid": "abc"}]} + mock_get_client.return_value = (fake_client, MOCK_OIDC_CONFIG) + + resp = client.get("/api/auth/oidc/callback?code=abc123&state=test-state") + error = _get_oidc_error(resp) + assert error == "Authentication failed" + + @patch("shelfmark.core.oidc_routes._get_oidc_client") + def test_callback_uses_generic_error_when_jwks_diagnosis_fails(self, mock_get_client, client): + fake_client = Mock() + fake_client.authorize_access_token.side_effect = KeyError("keys") + fake_client.fetch_jwk_set.side_effect = RuntimeError("jwks fetch failed") + mock_get_client.return_value = (fake_client, MOCK_OIDC_CONFIG) + + resp = client.get("/api/auth/oidc/callback?code=abc123&state=test-state") + error = _get_oidc_error(resp) + assert error == "Authentication failed" + @patch("shelfmark.core.oidc_routes._get_oidc_client") def test_callback_redirects_when_auto_provision_disabled_and_no_email_match( self, mock_get_client, client