From ace0cb2dd8fa43f0289a7d022fa7bb9ca409152e Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Wed, 27 May 2026 15:19:11 +0800 Subject: [PATCH] =?UTF-8?q?fix(auth):=20address=20CAS=20SSO=20review=20?= =?UTF-8?q?=E2=80=94=20XXE,=20AccessPolicy,=20timeouts,=20release=20wiring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blockers: - Harden XML parsing against XXE (disallow DOCTYPE, external entities/DTDs, enable FEATURE_SECURE_PROCESSING) and switch to UTF-8 byte decoding. - Generalize AccessPolicy.evaluate from OAuthClaims to IdentityClaims; extract IdentityAuthenticator so OAuth and CAS share allow/deny/pending evaluation. CAS callback now goes through the policy instead of bypassing it with a direct bindOrCreate call. - Configure JDK HttpClient with connect/read timeouts (5s/10s) and disable HTTP redirects to prevent ticket exfiltration via a malicious CAS server. Major: - Require HTTPS for skillhub.auth.cas.service-url in addition to server-url. - Stop logging raw service tickets; log claims.subject() instead. - Remove the dead authCasEnabled web flag — the backend AuthMethodCatalog is the single source of truth for CAS visibility, matching how OAuth works. - Wire SKILLHUB_AUTH_CAS_* env vars into compose.release.yml and add a fully documented section in .env.release.example. Minor: - CasProtocolVersion enum replaces string comparisons in the validator. - JSON multi-value array attributes are preserved as List instead of silently dropping all but the first element. - AuthMethod.methodType union adds 'CAS_REDIRECT'. - application.yml notes that service-url must equal ${SKILLHUB_PUBLIC_BASE_URL}/api/v1/auth/cas/callback. Tests: - CasTicketValidatorTest tightens URL matching to assert ticket/service/format parameters and adds XXE + billion-laughs regression cases. - IdentityAuthenticatorTest covers ALLOW / PENDING / DENY paths. - AuthMethodCatalogTest exercises both cas.enabled=true and =false. - isExternalRedirectMethod predicate extracted and unit-tested. --- .env.release.example | 17 ++ compose.release.yml | 8 + .../src/main/resources/application.yml | 3 + .../service/AuthMethodCatalogTest.java | 48 +++++ .../skillhub/auth/cas/CasLoginController.java | 48 ++--- .../skillhub/auth/cas/CasProperties.java | 24 ++- .../skillhub/auth/cas/CasProtocolVersion.java | 46 +++++ .../skillhub/auth/cas/CasTicketValidator.java | 68 +++++-- .../AccessDeniedByPolicyException.java | 10 + .../auth/identity/IdentityAuthenticator.java | 46 +++++ .../auth/oauth/OAuthLoginFlowService.java | 30 +-- .../skillhub/auth/policy/AccessPolicy.java | 4 +- .../auth/policy/EmailDomainAccessPolicy.java | 6 +- .../auth/policy/OpenAccessPolicy.java | 6 +- .../policy/ProviderAllowlistAccessPolicy.java | 6 +- .../policy/SubjectWhitelistAccessPolicy.java | 4 +- .../auth/cas/CasLoginControllerTest.java | 47 ++++- .../auth/cas/CasTicketValidatorTest.java | 173 ++++++++++++++---- .../identity/IdentityAuthenticatorTest.java | 78 ++++++++ ...Auth2AuthorizationRequestResolverTest.java | 6 +- .../auth/oauth/OAuthLoginFlowServiceTest.java | 12 +- web/docker-entrypoint.d/30-runtime-config.sh | 3 +- web/runtime-config.js.template | 3 +- web/src/api/client.ts | 12 -- web/src/api/types.ts | 2 +- web/src/bootstrap.ts | 1 - web/src/features/auth/login-button.test.ts | 49 +++-- web/src/features/auth/login-button.tsx | 18 +- 28 files changed, 620 insertions(+), 158 deletions(-) create mode 100644 server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasProtocolVersion.java create mode 100644 server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/AccessDeniedByPolicyException.java create mode 100644 server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityAuthenticator.java create mode 100644 server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityAuthenticatorTest.java diff --git a/.env.release.example b/.env.release.example index cdbddf06..b43f0129 100644 --- a/.env.release.example +++ b/.env.release.example @@ -88,6 +88,23 @@ SKILLHUB_AUTH_DIRECT_ENABLED=false SKILLHUB_WEB_AUTH_DIRECT_ENABLED=false SKILLHUB_WEB_AUTH_DIRECT_PROVIDER= +# CAS 2.0 / 3.0 SSO. To enable: +# - SKILLHUB_AUTH_CAS_ENABLED=true +# - SKILLHUB_AUTH_CAS_SERVER_URL=https://cas.example.com (CAS server base URL, must be HTTPS) +# - SKILLHUB_AUTH_CAS_SERVICE_URL=https://skillhub.example.com/api/v1/auth/cas/callback +# (must equal ${SKILLHUB_PUBLIC_BASE_URL}/api/v1/auth/cas/callback) +# Once enabled, /api/v1/auth/methods exposes a CAS_REDIRECT entry to the web UI automatically. +SKILLHUB_AUTH_CAS_ENABLED=false +SKILLHUB_AUTH_CAS_SERVER_URL= +SKILLHUB_AUTH_CAS_SERVICE_URL= +SKILLHUB_AUTH_CAS_PROTOCOL_VERSION=3.0 +# Override CAS attribute names if your server does not return uid/cn/mail. +SKILLHUB_AUTH_CAS_ATTR_USERNAME=uid +SKILLHUB_AUTH_CAS_ATTR_DISPLAY_NAME=cn +SKILLHUB_AUTH_CAS_ATTR_EMAIL=mail +# Development-only: allow http:// CAS server / service URLs. Never set this in production. +SKILLHUB_AUTH_CAS_ALLOW_INSECURE=false + # SMTP configuration for password reset verification emails. SPRING_MAIL_HOST= SPRING_MAIL_PORT=587 diff --git a/compose.release.yml b/compose.release.yml index 1b878731..3a042f60 100644 --- a/compose.release.yml +++ b/compose.release.yml @@ -73,6 +73,14 @@ services: SKILLHUB_SECURITY_SCANNER_URL: http://skill-scanner:8000 SKILLHUB_SECURITY_SCANNER_MODE: upload SKILLHUB_AUTH_DIRECT_ENABLED: ${SKILLHUB_AUTH_DIRECT_ENABLED:-false} + SKILLHUB_AUTH_CAS_ENABLED: ${SKILLHUB_AUTH_CAS_ENABLED:-false} + SKILLHUB_AUTH_CAS_SERVER_URL: ${SKILLHUB_AUTH_CAS_SERVER_URL:-} + SKILLHUB_AUTH_CAS_SERVICE_URL: ${SKILLHUB_AUTH_CAS_SERVICE_URL:-} + SKILLHUB_AUTH_CAS_PROTOCOL_VERSION: ${SKILLHUB_AUTH_CAS_PROTOCOL_VERSION:-3.0} + SKILLHUB_AUTH_CAS_ALLOW_INSECURE: ${SKILLHUB_AUTH_CAS_ALLOW_INSECURE:-false} + SKILLHUB_AUTH_CAS_ATTR_USERNAME: ${SKILLHUB_AUTH_CAS_ATTR_USERNAME:-uid} + SKILLHUB_AUTH_CAS_ATTR_DISPLAY_NAME: ${SKILLHUB_AUTH_CAS_ATTR_DISPLAY_NAME:-cn} + SKILLHUB_AUTH_CAS_ATTR_EMAIL: ${SKILLHUB_AUTH_CAS_ATTR_EMAIL:-mail} BOOTSTRAP_ADMIN_ENABLED: ${BOOTSTRAP_ADMIN_ENABLED:-false} BOOTSTRAP_ADMIN_USER_ID: ${BOOTSTRAP_ADMIN_USER_ID:-docker-admin} BOOTSTRAP_ADMIN_USERNAME: ${BOOTSTRAP_ADMIN_USERNAME:-admin} diff --git a/server/skillhub-app/src/main/resources/application.yml b/server/skillhub-app/src/main/resources/application.yml index ddc9ad73..bd868051 100644 --- a/server/skillhub-app/src/main/resources/application.yml +++ b/server/skillhub-app/src/main/resources/application.yml @@ -106,7 +106,10 @@ skillhub: email-from-name: ${SKILLHUB_AUTH_PASSWORD_RESET_FROM_NAME:SkillHub} cas: enabled: ${SKILLHUB_AUTH_CAS_ENABLED:false} + # Base URL of the CAS server (e.g. https://cas.example.com). Must be HTTPS unless allow-insecure-server=true. server-url: ${SKILLHUB_AUTH_CAS_SERVER_URL:} + # Must equal ${skillhub.public.base-url}/api/v1/auth/cas/callback — the CAS server validates + # the service ticket against this exact URL, so a mismatch yields INVALID_SERVICE. service-url: ${SKILLHUB_AUTH_CAS_SERVICE_URL:} protocol-version: ${SKILLHUB_AUTH_CAS_PROTOCOL_VERSION:3.0} allow-insecure-server: ${SKILLHUB_AUTH_CAS_ALLOW_INSECURE:false} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AuthMethodCatalogTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AuthMethodCatalogTest.java index 145cc6d5..328e1d95 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AuthMethodCatalogTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AuthMethodCatalogTest.java @@ -125,4 +125,52 @@ class AuthMethodCatalogTest { "bootstrap-private-sso:private-sso" ); } + + @Test + void listMethodsExposesCasWhenEnabled() { + OAuth2ClientProperties oauthProperties = new OAuth2ClientProperties(); + DirectAuthProperties directAuthProperties = new DirectAuthProperties(); + AuthSessionBootstrapProperties bootstrapProperties = new AuthSessionBootstrapProperties(); + + CasProperties casProperties = new CasProperties(); + casProperties.setEnabled(true); + casProperties.setServerUrl("https://cas.example.com"); + casProperties.setServiceUrl("https://skillhub.example.com/api/v1/auth/cas/callback"); + casProperties.setProtocolVersion("3.0"); + casProperties.setAllowInsecureServer(true); + casProperties.validate(); + + AuthMethodCatalog catalog = new AuthMethodCatalog( + oauthProperties, + directAuthProperties, + bootstrapProperties, + casProperties, + List.of(), + List.of() + ); + + assertThat(catalog.listMethods(null)) + .extracting(method -> method.id() + ":" + method.methodType() + ":" + method.actionUrl()) + .contains("cas:CAS_REDIRECT:/api/v1/auth/cas/login"); + } + + @Test + void listMethodsOmitsCasWhenDisabled() { + OAuth2ClientProperties oauthProperties = new OAuth2ClientProperties(); + DirectAuthProperties directAuthProperties = new DirectAuthProperties(); + AuthSessionBootstrapProperties bootstrapProperties = new AuthSessionBootstrapProperties(); + + AuthMethodCatalog catalog = new AuthMethodCatalog( + oauthProperties, + directAuthProperties, + bootstrapProperties, + new CasProperties(), + List.of(), + List.of() + ); + + assertThat(catalog.listMethods(null)) + .extracting(method -> method.id()) + .doesNotContain("cas"); + } } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasLoginController.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasLoginController.java index 2dcd01c6..1a8cc75b 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasLoginController.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasLoginController.java @@ -1,12 +1,12 @@ package com.iflytek.skillhub.auth.cas; -import com.iflytek.skillhub.auth.identity.IdentityBindingService; +import com.iflytek.skillhub.auth.identity.AccessDeniedByPolicyException; +import com.iflytek.skillhub.auth.identity.IdentityAuthenticator; import com.iflytek.skillhub.auth.oauth.AccountDisabledException; import com.iflytek.skillhub.auth.oauth.AccountPendingException; import com.iflytek.skillhub.auth.oauth.OAuthLoginRedirectSupport; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.auth.session.PlatformSessionService; -import com.iflytek.skillhub.domain.user.UserStatus; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpSession; import org.slf4j.Logger; @@ -19,6 +19,8 @@ import org.springframework.web.util.UriComponentsBuilder; /** * Handles CAS SSO login flow: redirect to CAS server and callback with ticket validation. + * Delegates access-policy evaluation and principal provisioning to {@link IdentityAuthenticator} + * so the same allow/deny/pending decisions apply to OAuth and CAS uniformly. */ @Controller @RequestMapping("/api/v1/auth/cas") @@ -28,24 +30,21 @@ public class CasLoginController { private final CasProperties casProperties; private final CasTicketValidator ticketValidator; - private final IdentityBindingService identityBindingService; + private final IdentityAuthenticator identityAuthenticator; private final PlatformSessionService sessionService; public CasLoginController( CasProperties casProperties, CasTicketValidator ticketValidator, - IdentityBindingService identityBindingService, + IdentityAuthenticator identityAuthenticator, PlatformSessionService sessionService ) { this.casProperties = casProperties; this.ticketValidator = ticketValidator; - this.identityBindingService = identityBindingService; + this.identityAuthenticator = identityAuthenticator; this.sessionService = sessionService; } - /** - * Initiates CAS login by redirecting to the CAS server. - */ @GetMapping("/login") public String login( @RequestParam(required = false) String returnTo, @@ -71,9 +70,6 @@ public class CasLoginController { return "redirect:" + casLoginUrl; } - /** - * Handles CAS callback with ticket validation and session establishment. - */ @GetMapping("/callback") public String callback( @RequestParam(required = false) String ticket, @@ -89,11 +85,18 @@ public class CasLoginController { return "redirect:/login?error=missing_ticket"; } + CasIdentityClaims claims; try { - CasIdentityClaims claims = ticketValidator.validate(ticket); - log.info("CAS ticket validated successfully for user: {}", claims.subject()); + claims = ticketValidator.validate(ticket); + } catch (CasValidationException e) { + log.error("CAS ticket validation failed: {}", e.getMessage()); + return "redirect:/login?error=cas_validation_failed"; + } - PlatformPrincipal principal = identityBindingService.bindOrCreate(claims, UserStatus.ACTIVE); + log.info("CAS ticket validated for subject={}", claims.subject()); + + try { + PlatformPrincipal principal = identityAuthenticator.authenticate(claims); sessionService.establishSession(principal, request); HttpSession session = request.getSession(false); @@ -103,21 +106,24 @@ public class CasLoginController { session.removeAttribute(OAuthLoginRedirectSupport.SESSION_RETURN_TO_ATTRIBUTE); } - String targetUrl = returnTo != null ? returnTo : OAuthLoginRedirectSupport.DEFAULT_TARGET_URL; + String targetUrl = OAuthLoginRedirectSupport.sanitizeReturnTo(returnTo); + if (targetUrl == null) { + targetUrl = OAuthLoginRedirectSupport.DEFAULT_TARGET_URL; + } log.debug("CAS login successful, redirecting to: {}", targetUrl); return "redirect:" + targetUrl; } catch (AccountPendingException e) { - log.warn("CAS user account pending approval: {}", ticket); + log.warn("CAS user pending approval: subject={}", claims.subject()); return "redirect:/pending-approval"; } catch (AccountDisabledException e) { - log.warn("CAS user account disabled: {}", ticket); + log.warn("CAS user disabled: subject={}", claims.subject()); + return "redirect:/access-denied"; + } catch (AccessDeniedByPolicyException e) { + log.warn("CAS user denied by policy: subject={}", claims.subject()); return "redirect:/access-denied"; - } catch (CasValidationException e) { - log.error("CAS ticket validation failed: {}", e.getMessage()); - return "redirect:/login?error=cas_validation_failed"; } catch (Exception e) { - log.error("Unexpected error during CAS callback", e); + log.error("Unexpected error during CAS callback for subject={}", claims.subject(), e); return "redirect:/login?error=internal_error"; } } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasProperties.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasProperties.java index 0c1102f1..39d29178 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasProperties.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasProperties.java @@ -20,6 +20,7 @@ public class CasProperties { private String protocolVersion = "3.0"; private boolean allowInsecureServer = false; private Map attributes = new HashMap<>(); + private CasProtocolVersion resolvedProtocolVersion = CasProtocolVersion.V3_0; @PostConstruct public void validate() { @@ -42,8 +43,19 @@ public class CasProperties { ); } - if (!"2.0".equals(protocolVersion) && !"3.0".equals(protocolVersion)) { - throw new IllegalStateException("skillhub.auth.cas.protocol-version must be either '2.0' or '3.0'"); + if (!allowInsecureServer && !serviceUrl.startsWith("https://")) { + throw new IllegalStateException( + "CAS service URL must use HTTPS in production (otherwise the service ticket " + + "is transmitted in plaintext). " + + "Set skillhub.auth.cas.allow-insecure-server=true to override for development." + ); + } + + try { + this.resolvedProtocolVersion = CasProtocolVersion.from(protocolVersion); + } catch (IllegalArgumentException e) { + throw new IllegalStateException( + "skillhub.auth.cas.protocol-version must be either '2.0' or '3.0'", e); } if (attributes.get("username") == null || attributes.get("username").isBlank()) { @@ -57,6 +69,14 @@ public class CasProperties { } } + /** + * Resolved protocol version after validation. Use this in the call path instead of + * {@link #getProtocolVersion()} string comparisons. + */ + public CasProtocolVersion resolvedProtocolVersion() { + return resolvedProtocolVersion; + } + public boolean isEnabled() { return enabled; } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasProtocolVersion.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasProtocolVersion.java new file mode 100644 index 00000000..02b0f94d --- /dev/null +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasProtocolVersion.java @@ -0,0 +1,46 @@ +package com.iflytek.skillhub.auth.cas; + +/** + * CAS protocol version: determines the validation endpoint and response format. + */ +public enum CasProtocolVersion { + /** CAS 2.0: /serviceValidate, XML response. */ + V2_0("2.0", "/serviceValidate", false), + /** CAS 3.0: /p3/serviceValidate, JSON response (with format=JSON). */ + V3_0("3.0", "/p3/serviceValidate", true); + + private final String wireValue; + private final String validatePath; + private final boolean json; + + CasProtocolVersion(String wireValue, String validatePath, boolean json) { + this.wireValue = wireValue; + this.validatePath = validatePath; + this.json = json; + } + + public String wireValue() { + return wireValue; + } + + public String validatePath() { + return validatePath; + } + + public boolean isJson() { + return json; + } + + public static CasProtocolVersion from(String value) { + if (value == null) { + return V3_0; + } + return switch (value.trim()) { + case "2.0" -> V2_0; + case "3.0" -> V3_0; + default -> throw new IllegalArgumentException( + "Unsupported CAS protocol version: " + value + " (expected '2.0' or '3.0')" + ); + }; + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasTicketValidator.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasTicketValidator.java index 713dae89..182a54a6 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasTicketValidator.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/cas/CasTicketValidator.java @@ -5,6 +5,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.http.client.JdkClientHttpRequestFactory; import org.springframework.stereotype.Component; import org.springframework.web.client.RestClient; import org.springframework.web.util.UriComponentsBuilder; @@ -15,6 +16,8 @@ import org.w3c.dom.NodeList; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import java.io.ByteArrayInputStream; +import java.net.http.HttpClient; +import java.time.Duration; import java.util.HashMap; import java.util.Map; @@ -26,6 +29,8 @@ import java.util.Map; public class CasTicketValidator { private static final Logger log = LoggerFactory.getLogger(CasTicketValidator.class); + private static final Duration CONNECT_TIMEOUT = Duration.ofSeconds(5); + private static final Duration READ_TIMEOUT = Duration.ofSeconds(10); private final CasProperties casProperties; private final RestClient restClient; @@ -33,7 +38,7 @@ public class CasTicketValidator { @Autowired public CasTicketValidator(CasProperties casProperties, ObjectMapper objectMapper) { - this(casProperties, objectMapper, RestClient.builder().build()); + this(casProperties, objectMapper, defaultRestClient()); } CasTicketValidator(CasProperties casProperties, ObjectMapper objectMapper, RestClient restClient) { @@ -42,6 +47,16 @@ public class CasTicketValidator { this.restClient = restClient; } + private static RestClient defaultRestClient() { + HttpClient httpClient = HttpClient.newBuilder() + .connectTimeout(CONNECT_TIMEOUT) + .followRedirects(HttpClient.Redirect.NEVER) + .build(); + JdkClientHttpRequestFactory factory = new JdkClientHttpRequestFactory(httpClient); + factory.setReadTimeout(READ_TIMEOUT); + return RestClient.builder().requestFactory(factory).build(); + } + /** * Validates a CAS ticket and returns the user attributes. * @@ -67,7 +82,7 @@ public class CasTicketValidator { throw new CasValidationException("Empty response from CAS server"); } - if ("3.0".equals(casProperties.getProtocolVersion())) { + if (casProperties.resolvedProtocolVersion().isJson()) { return parseJsonResponse(response); } else { return parseXmlResponse(response); @@ -81,16 +96,14 @@ public class CasTicketValidator { } private String buildValidationUrl(String ticket) { - String endpoint = "3.0".equals(casProperties.getProtocolVersion()) - ? "/p3/serviceValidate" - : "/serviceValidate"; + CasProtocolVersion version = casProperties.resolvedProtocolVersion(); UriComponentsBuilder builder = UriComponentsBuilder - .fromHttpUrl(casProperties.getServerUrl() + endpoint) + .fromHttpUrl(casProperties.getServerUrl() + version.validatePath()) .queryParam("ticket", ticket) .queryParam("service", casProperties.getServiceUrl()); - if ("3.0".equals(casProperties.getProtocolVersion())) { + if (version.isJson()) { builder.queryParam("format", "JSON"); } @@ -124,9 +137,19 @@ public class CasTicketValidator { JsonNode value = entry.getValue(); if (value.isTextual()) { attributes.put(entry.getKey(), value.asText()); - } else if (value.isArray() && value.size() > 0) { - attributes.put(entry.getKey(), value.get(0).asText()); - } else { + } else if (value.isArray()) { + if (value.size() == 1) { + attributes.put(entry.getKey(), value.get(0).asText()); + } else if (value.size() > 1) { + java.util.List values = new java.util.ArrayList<>(value.size()); + for (JsonNode item : value) { + values.add(item.asText()); + } + attributes.put(entry.getKey(), java.util.List.copyOf(values)); + } + } else if (value.isNumber() || value.isBoolean()) { + attributes.put(entry.getKey(), value.asText()); + } else if (!value.isNull()) { attributes.put(entry.getKey(), value.toString()); } }); @@ -137,9 +160,16 @@ public class CasTicketValidator { private CasIdentityClaims parseXmlResponse(String response) throws Exception { DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature(javax.xml.XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setXIncludeAware(false); + factory.setExpandEntityReferences(false); factory.setNamespaceAware(true); DocumentBuilder builder = factory.newDocumentBuilder(); - Document doc = builder.parse(new ByteArrayInputStream(response.getBytes())); + Document doc = builder.parse(new ByteArrayInputStream(response.getBytes(java.nio.charset.StandardCharsets.UTF_8))); Element root = doc.getDocumentElement(); @@ -192,10 +222,20 @@ public class CasTicketValidator { String displayNameAttr = casProperties.getAttributes().get("display-name"); String emailAttr = casProperties.getAttributes().get("email"); - String subject = attributes.getOrDefault(usernameAttr, user).toString(); - String displayName = attributes.getOrDefault(displayNameAttr, user).toString(); - String email = attributes.containsKey(emailAttr) ? attributes.get(emailAttr).toString() : null; + String subject = firstStringOr(attributes.get(usernameAttr), user); + String displayName = firstStringOr(attributes.get(displayNameAttr), user); + String email = firstStringOr(attributes.get(emailAttr), null); return new CasIdentityClaims(subject, email, displayName, attributes); } + + private static String firstStringOr(Object value, String fallback) { + if (value == null) { + return fallback; + } + if (value instanceof java.util.List list) { + return list.isEmpty() ? fallback : String.valueOf(list.get(0)); + } + return value.toString(); + } } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/AccessDeniedByPolicyException.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/AccessDeniedByPolicyException.java new file mode 100644 index 00000000..27015794 --- /dev/null +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/AccessDeniedByPolicyException.java @@ -0,0 +1,10 @@ +package com.iflytek.skillhub.auth.identity; + +/** + * Thrown when an authenticated upstream identity is rejected by the configured AccessPolicy. + */ +public class AccessDeniedByPolicyException extends RuntimeException { + public AccessDeniedByPolicyException() { + super("Access denied by policy"); + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityAuthenticator.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityAuthenticator.java new file mode 100644 index 00000000..254717f0 --- /dev/null +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityAuthenticator.java @@ -0,0 +1,46 @@ +package com.iflytek.skillhub.auth.identity; + +import com.iflytek.skillhub.auth.oauth.AccountPendingException; +import com.iflytek.skillhub.auth.policy.AccessDecision; +import com.iflytek.skillhub.auth.policy.AccessPolicy; +import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import com.iflytek.skillhub.domain.user.UserStatus; +import org.springframework.stereotype.Service; + +/** + * Provider-neutral identity authentication: evaluates access policy and creates or binds a + * platform principal. Used by all upstream identity flows (OAuth, CAS, etc.) so that the same + * allow/deny/pending decisions apply regardless of protocol. + */ +@Service +public class IdentityAuthenticator { + + private final AccessPolicy accessPolicy; + private final IdentityBindingService identityBindingService; + + public IdentityAuthenticator(AccessPolicy accessPolicy, IdentityBindingService identityBindingService) { + this.accessPolicy = accessPolicy; + this.identityBindingService = identityBindingService; + } + + /** + * Evaluates policy and returns a platform principal for an allowed identity. + * + * @throws AccountPendingException if the policy yields PENDING_APPROVAL + * @throws AccessDeniedByPolicyException if the policy yields DENY + * @throws com.iflytek.skillhub.auth.oauth.AccountDisabledException if the user is disabled + */ + public PlatformPrincipal authenticate(IdentityClaims claims) { + AccessDecision decision = accessPolicy.evaluate(claims); + + if (decision == AccessDecision.PENDING_APPROVAL) { + identityBindingService.createPendingUserIfAbsent(claims); + throw new AccountPendingException(); + } + if (decision == AccessDecision.DENY) { + throw new AccessDeniedByPolicyException(); + } + + return identityBindingService.bindOrCreate(claims, UserStatus.ACTIVE); + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java index e5c7dc3d..7bce9d17 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java @@ -1,10 +1,9 @@ package com.iflytek.skillhub.auth.oauth; -import com.iflytek.skillhub.auth.identity.IdentityBindingService; -import com.iflytek.skillhub.auth.policy.AccessDecision; -import com.iflytek.skillhub.auth.policy.AccessPolicy; +import com.iflytek.skillhub.auth.identity.AccessDeniedByPolicyException; +import com.iflytek.skillhub.auth.identity.IdentityAuthenticator; +import com.iflytek.skillhub.auth.identity.IdentityClaims; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; -import com.iflytek.skillhub.domain.user.UserStatus; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpSession; import java.net.URLEncoder; @@ -31,16 +30,13 @@ public class OAuthLoginFlowService { private final DefaultOAuth2UserService delegate = new DefaultOAuth2UserService(); private final Map extractors; - private final AccessPolicy accessPolicy; - private final IdentityBindingService identityBindingService; + private final IdentityAuthenticator identityAuthenticator; public OAuthLoginFlowService(List extractorList, - AccessPolicy accessPolicy, - IdentityBindingService identityBindingService) { + IdentityAuthenticator identityAuthenticator) { this.extractors = extractorList.stream() .collect(Collectors.toMap(OAuthClaimsExtractor::getProvider, Function.identity())); - this.accessPolicy = accessPolicy; - this.identityBindingService = identityBindingService; + this.identityAuthenticator = identityAuthenticator; } public AuthenticatedLoginContext loadLoginContext(OAuth2UserRequest request) { @@ -59,20 +55,14 @@ public class OAuthLoginFlowService { return new AuthenticatedLoginContext(upstreamUser, principal); } - public PlatformPrincipal authenticate(OAuthClaims claims) { - AccessDecision decision = accessPolicy.evaluate(claims); - - if (decision == AccessDecision.PENDING_APPROVAL) { - identityBindingService.createPendingUserIfAbsent(claims); - throw new AccountPendingException(); - } - if (decision == AccessDecision.DENY) { + public PlatformPrincipal authenticate(IdentityClaims claims) { + try { + return identityAuthenticator.authenticate(claims); + } catch (AccessDeniedByPolicyException e) { throw new OAuth2AuthenticationException( new OAuth2Error("access_denied", "Access denied by policy", null) ); } - - return identityBindingService.bindOrCreate(claims, UserStatus.ACTIVE); } public void rememberReturnTo(HttpServletRequest request) { diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/AccessPolicy.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/AccessPolicy.java index 8ac38ea6..4ad21f2e 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/AccessPolicy.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/AccessPolicy.java @@ -1,10 +1,10 @@ package com.iflytek.skillhub.auth.policy; -import com.iflytek.skillhub.auth.oauth.OAuthClaims; +import com.iflytek.skillhub.auth.identity.IdentityClaims; /** * Policy contract for deciding whether externally authenticated users may enter the platform. */ public interface AccessPolicy { - AccessDecision evaluate(OAuthClaims claims); + AccessDecision evaluate(IdentityClaims claims); } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/EmailDomainAccessPolicy.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/EmailDomainAccessPolicy.java index d688f2a2..3296b923 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/EmailDomainAccessPolicy.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/EmailDomainAccessPolicy.java @@ -1,10 +1,10 @@ package com.iflytek.skillhub.auth.policy; -import com.iflytek.skillhub.auth.oauth.OAuthClaims; +import com.iflytek.skillhub.auth.identity.IdentityClaims; import java.util.Set; /** - * Access policy that allows login only when the OAuth email belongs to an approved domain. + * Access policy that allows login only when the upstream email belongs to an approved domain. */ public class EmailDomainAccessPolicy implements AccessPolicy { private final Set allowedDomains; @@ -14,7 +14,7 @@ public class EmailDomainAccessPolicy implements AccessPolicy { } @Override - public AccessDecision evaluate(OAuthClaims claims) { + public AccessDecision evaluate(IdentityClaims claims) { if (claims.email() == null) return AccessDecision.DENY; String domain = claims.email().substring(claims.email().indexOf('@') + 1); return allowedDomains.contains(domain.toLowerCase()) diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/OpenAccessPolicy.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/OpenAccessPolicy.java index 9febc325..2b6130ed 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/OpenAccessPolicy.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/OpenAccessPolicy.java @@ -1,13 +1,13 @@ package com.iflytek.skillhub.auth.policy; -import com.iflytek.skillhub.auth.oauth.OAuthClaims; +import com.iflytek.skillhub.auth.identity.IdentityClaims; /** - * Access policy that accepts all OAuth-authenticated users. + * Access policy that accepts all externally authenticated users. */ public class OpenAccessPolicy implements AccessPolicy { @Override - public AccessDecision evaluate(OAuthClaims claims) { + public AccessDecision evaluate(IdentityClaims claims) { return AccessDecision.ALLOW; } } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/ProviderAllowlistAccessPolicy.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/ProviderAllowlistAccessPolicy.java index d44ec348..e3dc38bb 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/ProviderAllowlistAccessPolicy.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/ProviderAllowlistAccessPolicy.java @@ -1,10 +1,10 @@ package com.iflytek.skillhub.auth.policy; -import com.iflytek.skillhub.auth.oauth.OAuthClaims; +import com.iflytek.skillhub.auth.identity.IdentityClaims; import java.util.Set; /** - * Access policy that limits login to explicitly allowed OAuth providers. + * Access policy that limits login to explicitly allowed identity providers. */ public class ProviderAllowlistAccessPolicy implements AccessPolicy { private final Set allowedProviders; @@ -14,7 +14,7 @@ public class ProviderAllowlistAccessPolicy implements AccessPolicy { } @Override - public AccessDecision evaluate(OAuthClaims claims) { + public AccessDecision evaluate(IdentityClaims claims) { return allowedProviders.contains(claims.provider()) ? AccessDecision.ALLOW : AccessDecision.DENY; } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/SubjectWhitelistAccessPolicy.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/SubjectWhitelistAccessPolicy.java index 0b9546f5..8044afdc 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/SubjectWhitelistAccessPolicy.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/SubjectWhitelistAccessPolicy.java @@ -1,6 +1,6 @@ package com.iflytek.skillhub.auth.policy; -import com.iflytek.skillhub.auth.oauth.OAuthClaims; +import com.iflytek.skillhub.auth.identity.IdentityClaims; import java.util.Set; /** @@ -14,7 +14,7 @@ public class SubjectWhitelistAccessPolicy implements AccessPolicy { } @Override - public AccessDecision evaluate(OAuthClaims claims) { + public AccessDecision evaluate(IdentityClaims claims) { String key = claims.provider() + ":" + claims.subject(); return whitelistedSubjects.contains(key) ? AccessDecision.ALLOW : AccessDecision.DENY; diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/cas/CasLoginControllerTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/cas/CasLoginControllerTest.java index 1560da96..62f4a589 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/cas/CasLoginControllerTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/cas/CasLoginControllerTest.java @@ -7,12 +7,12 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import com.iflytek.skillhub.auth.identity.IdentityBindingService; +import com.iflytek.skillhub.auth.identity.AccessDeniedByPolicyException; +import com.iflytek.skillhub.auth.identity.IdentityAuthenticator; import com.iflytek.skillhub.auth.oauth.AccountDisabledException; import com.iflytek.skillhub.auth.oauth.AccountPendingException; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.auth.session.PlatformSessionService; -import com.iflytek.skillhub.domain.user.UserStatus; import java.util.Map; import java.util.Set; import org.junit.jupiter.api.BeforeEach; @@ -30,7 +30,7 @@ class CasLoginControllerTest { private CasTicketValidator ticketValidator; @Mock - private IdentityBindingService identityBindingService; + private IdentityAuthenticator identityAuthenticator; @Mock private PlatformSessionService sessionService; @@ -47,7 +47,7 @@ class CasLoginControllerTest { casProperties.setProtocolVersion("3.0"); casProperties.setAllowInsecureServer(true); - controller = new CasLoginController(casProperties, ticketValidator, identityBindingService, sessionService); + controller = new CasLoginController(casProperties, ticketValidator, identityAuthenticator, sessionService); } @Test @@ -99,7 +99,7 @@ class CasLoginControllerTest { PlatformPrincipal principal = new PlatformPrincipal("usr_123", "Zhang San", "zhangsan@example.com", null, "cas", Set.of("USER")); when(ticketValidator.validate("ST-12345")).thenReturn(claims); - when(identityBindingService.bindOrCreate(claims, UserStatus.ACTIVE)).thenReturn(principal); + when(identityAuthenticator.authenticate(claims)).thenReturn(principal); String result = controller.callback("ST-12345", request); @@ -115,13 +115,31 @@ class CasLoginControllerTest { PlatformPrincipal principal = new PlatformPrincipal("usr_456", "User One", null, null, "cas", Set.of("USER")); when(ticketValidator.validate("ST-99999")).thenReturn(claims); - when(identityBindingService.bindOrCreate(claims, UserStatus.ACTIVE)).thenReturn(principal); + when(identityAuthenticator.authenticate(claims)).thenReturn(principal); String result = controller.callback("ST-99999", request); assertThat(result).isEqualTo("redirect:/dashboard"); } + @Test + void callback_sanitizesUnsafeReturnTo() { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession session = new MockHttpSession(); + session.setAttribute("skillhub.oauth.returnTo", "https://evil.example/steal"); + request.setSession(session); + + CasIdentityClaims claims = new CasIdentityClaims("u", null, "U", Map.of()); + PlatformPrincipal principal = new PlatformPrincipal("usr_x", "U", null, null, "cas", Set.of("USER")); + + when(ticketValidator.validate("ST-evil")).thenReturn(claims); + when(identityAuthenticator.authenticate(claims)).thenReturn(principal); + + String result = controller.callback("ST-evil", request); + + assertThat(result).isEqualTo("redirect:/dashboard"); + } + @Test void callback_missingTicket_redirectsWithError() { MockHttpServletRequest request = new MockHttpServletRequest(); @@ -157,7 +175,7 @@ class CasLoginControllerTest { CasIdentityClaims claims = new CasIdentityClaims("pending-user", null, "Pending", Map.of()); when(ticketValidator.validate("ST-pending")).thenReturn(claims); - when(identityBindingService.bindOrCreate(claims, UserStatus.ACTIVE)).thenThrow(new AccountPendingException()); + when(identityAuthenticator.authenticate(claims)).thenThrow(new AccountPendingException()); String result = controller.callback("ST-pending", request); @@ -170,13 +188,26 @@ class CasLoginControllerTest { CasIdentityClaims claims = new CasIdentityClaims("disabled-user", null, "Disabled", Map.of()); when(ticketValidator.validate("ST-disabled")).thenReturn(claims); - when(identityBindingService.bindOrCreate(claims, UserStatus.ACTIVE)).thenThrow(new AccountDisabledException()); + when(identityAuthenticator.authenticate(claims)).thenThrow(new AccountDisabledException()); String result = controller.callback("ST-disabled", request); assertThat(result).isEqualTo("redirect:/access-denied"); } + @Test + void callback_accessPolicyDeny_redirectsToAccessDenied() { + MockHttpServletRequest request = new MockHttpServletRequest(); + + CasIdentityClaims claims = new CasIdentityClaims("denied-user", "denied@bad.example", "Denied", Map.of()); + when(ticketValidator.validate("ST-denied")).thenReturn(claims); + when(identityAuthenticator.authenticate(claims)).thenThrow(new AccessDeniedByPolicyException()); + + String result = controller.callback("ST-denied", request); + + assertThat(result).isEqualTo("redirect:/access-denied"); + } + @Test void callback_validationFailed_redirectsWithError() { MockHttpServletRequest request = new MockHttpServletRequest(); diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/cas/CasTicketValidatorTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/cas/CasTicketValidatorTest.java index bdc13980..ba0d080a 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/cas/CasTicketValidatorTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/cas/CasTicketValidatorTest.java @@ -2,11 +2,16 @@ package com.iflytek.skillhub.auth.cas; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.startsWith; import static org.springframework.test.web.client.match.MockRestRequestMatchers.requestTo; import static org.springframework.test.web.client.response.MockRestResponseCreators.withSuccess; import com.fasterxml.jackson.databind.ObjectMapper; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -14,9 +19,6 @@ import org.springframework.http.MediaType; import org.springframework.test.web.client.MockRestServiceServer; import org.springframework.web.client.RestClient; -import static org.springframework.test.web.client.match.MockRestRequestMatchers.requestToUriTemplate; -import static org.hamcrest.Matchers.containsString; - class CasTicketValidatorTest { private MockRestServiceServer mockServer; @@ -25,28 +27,37 @@ class CasTicketValidatorTest { @BeforeEach void setUp() { - casProperties = new CasProperties(); - casProperties.setEnabled(true); - casProperties.setServerUrl("https://cas.example.com"); - casProperties.setServiceUrl("https://skillhub.example.com/api/v1/auth/cas/callback"); - casProperties.setProtocolVersion("3.0"); - casProperties.setAllowInsecureServer(true); + casProperties = newProperties("3.0"); + validator = newValidator(casProperties); + } + + private CasProperties newProperties(String protocolVersion) { + CasProperties props = new CasProperties(); + props.setEnabled(true); + props.setServerUrl("https://cas.example.com"); + props.setServiceUrl("https://skillhub.example.com/api/v1/auth/cas/callback"); + props.setProtocolVersion(protocolVersion); + props.setAllowInsecureServer(true); Map attributes = new HashMap<>(); attributes.put("username", "uid"); attributes.put("display-name", "cn"); attributes.put("email", "mail"); - casProperties.setAttributes(attributes); + props.setAttributes(attributes); + // Trigger @PostConstruct logic (resolvedProtocolVersion etc.) + props.validate(); + return props; + } + + private CasTicketValidator newValidator(CasProperties props) { RestClient.Builder builder = RestClient.builder(); mockServer = MockRestServiceServer.bindTo(builder).build(); - RestClient restClient = builder.build(); - - validator = new CasTicketValidator(casProperties, new ObjectMapper(), restClient); + return new CasTicketValidator(props, new ObjectMapper(), builder.build()); } @Test - void validate_cas30_json_success() { + void validate_cas30_json_success_buildsExactValidationUrl() { String jsonResponse = """ { "serviceResponse": { @@ -63,8 +74,12 @@ class CasTicketValidatorTest { } """; - mockServer.expect(requestTo(containsString("/p3/serviceValidate"))) - .andRespond(withSuccess(jsonResponse, MediaType.APPLICATION_JSON)); + mockServer.expect(requestTo(allOf( + startsWith("https://cas.example.com/p3/serviceValidate"), + containsString("ticket=ST-12345"), + containsString("service=https"), + containsString("format=JSON") + ))).andRespond(withSuccess(jsonResponse, MediaType.APPLICATION_JSON)); CasIdentityClaims claims = validator.validate("ST-12345"); @@ -101,11 +116,9 @@ class CasTicketValidatorTest { } @Test - void validate_cas20_xml_success() { - casProperties.setProtocolVersion("2.0"); - RestClient.Builder builder = RestClient.builder(); - mockServer = MockRestServiceServer.bindTo(builder).build(); - validator = new CasTicketValidator(casProperties, new ObjectMapper(), builder.build()); + void validate_cas20_xml_success_buildsExactValidationUrl() { + casProperties = newProperties("2.0"); + validator = newValidator(casProperties); String xmlResponse = """ @@ -120,8 +133,13 @@ class CasTicketValidatorTest { """; - mockServer.expect(requestTo(containsString("/serviceValidate"))) - .andRespond(withSuccess(xmlResponse, MediaType.APPLICATION_XML)); + mockServer.expect(requestTo(allOf( + startsWith("https://cas.example.com/serviceValidate"), + not(containsString("/p3/")), + containsString("ticket=ST-67890"), + containsString("service=https"), + not(containsString("format=JSON")) + ))).andRespond(withSuccess(xmlResponse, MediaType.APPLICATION_XML)); CasIdentityClaims claims = validator.validate("ST-67890"); @@ -135,10 +153,8 @@ class CasTicketValidatorTest { @Test void validate_cas20_xml_authenticationFailure() { - casProperties.setProtocolVersion("2.0"); - RestClient.Builder builder = RestClient.builder(); - mockServer = MockRestServiceServer.bindTo(builder).build(); - validator = new CasTicketValidator(casProperties, new ObjectMapper(), builder.build()); + casProperties = newProperties("2.0"); + validator = newValidator(casProperties); String xmlResponse = """ @@ -192,7 +208,7 @@ class CasTicketValidatorTest { } @Test - void validate_cas30_json_arrayAttributes() { + void validate_cas30_json_singleArrayAttribute_unwrapped() { String jsonResponse = """ { "serviceResponse": { @@ -201,8 +217,7 @@ class CasTicketValidatorTest { "attributes": { "uid": ["wangwu"], "cn": ["Wang Wu"], - "mail": ["wangwu@example.com"], - "memberOf": ["group1", "group2"] + "mail": ["wangwu@example.com"] } } } @@ -212,7 +227,7 @@ class CasTicketValidatorTest { mockServer.expect(requestTo(containsString("/p3/serviceValidate"))) .andRespond(withSuccess(jsonResponse, MediaType.APPLICATION_JSON)); - CasIdentityClaims claims = validator.validate("ST-array"); + CasIdentityClaims claims = validator.validate("ST-array1"); assertThat(claims.subject()).isEqualTo("wangwu"); assertThat(claims.providerLogin()).isEqualTo("Wang Wu"); @@ -221,6 +236,37 @@ class CasTicketValidatorTest { mockServer.verify(); } + @Test + void validate_cas30_json_multiValueArray_preservedAsList() { + String jsonResponse = """ + { + "serviceResponse": { + "authenticationSuccess": { + "user": "wangwu", + "attributes": { + "uid": "wangwu", + "cn": "Wang Wu", + "memberOf": ["group1", "group2", "group3"] + } + } + } + } + """; + + mockServer.expect(requestTo(containsString("/p3/serviceValidate"))) + .andRespond(withSuccess(jsonResponse, MediaType.APPLICATION_JSON)); + + CasIdentityClaims claims = validator.validate("ST-multi"); + + assertThat(claims.extra()).containsKey("memberOf"); + assertThat(claims.extra().get("memberOf")).isInstanceOf(List.class); + @SuppressWarnings("unchecked") + List memberOf = (List) claims.extra().get("memberOf"); + assertThat(memberOf).containsExactly("group1", "group2", "group3"); + + mockServer.verify(); + } + @Test void validate_fallsBackToUserWhenAttributesMissing() { String jsonResponse = """ @@ -245,4 +291,69 @@ class CasTicketValidatorTest { mockServer.verify(); } + + @Test + void validate_cas20_xml_xxePayload_isRejected() { + casProperties = newProperties("2.0"); + validator = newValidator(casProperties); + + // XXE attempt: external entity referencing a local file. With XXE hardening enabled, the + // parser must refuse the DOCTYPE outright (or refuse to resolve the entity); either way + // the local file content must NOT appear in the parsed user attribute. + String xxePayload = """ + + + ]> + + + &xxe; + + &xxe; + + + + """; + + mockServer.expect(requestTo(containsString("/serviceValidate"))) + .andRespond(withSuccess(xxePayload, MediaType.APPLICATION_XML)); + + assertThatThrownBy(() -> validator.validate("ST-xxe")) + .isInstanceOfAny(CasValidationException.class) + .satisfies(e -> { + // The error must NOT contain the local file content; it should signal a parse + // refusal or a missing-user condition (the entity could not be expanded). + String msg = e.getMessage() == null ? "" : e.getMessage(); + assertThat(msg).doesNotContain("root:"); + assertThat(msg).doesNotContain("/bin/bash"); + }); + } + + @Test + void validate_cas20_xml_billionLaughs_isRejected() { + casProperties = newProperties("2.0"); + validator = newValidator(casProperties); + + String billionLaughs = """ + + + + + ]> + + + &lol3; + + + """; + + mockServer.expect(requestTo(containsString("/serviceValidate"))) + .andRespond(withSuccess(billionLaughs, MediaType.APPLICATION_XML)); + + // disallow-doctype-decl=true means the parser refuses any DOCTYPE; we expect a validation + // exception rather than the parser dutifully expanding billions of entities. + assertThatThrownBy(() -> validator.validate("ST-laugh")) + .isInstanceOf(CasValidationException.class); + } } diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityAuthenticatorTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityAuthenticatorTest.java new file mode 100644 index 00000000..dbb4a9f9 --- /dev/null +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityAuthenticatorTest.java @@ -0,0 +1,78 @@ +package com.iflytek.skillhub.auth.identity; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.iflytek.skillhub.auth.oauth.AccountPendingException; +import com.iflytek.skillhub.auth.oauth.OAuthClaims; +import com.iflytek.skillhub.auth.policy.AccessDecision; +import com.iflytek.skillhub.auth.policy.AccessPolicy; +import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import com.iflytek.skillhub.domain.user.UserStatus; +import java.util.Map; +import java.util.Set; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class IdentityAuthenticatorTest { + + @Mock + private AccessPolicy accessPolicy; + + @Mock + private IdentityBindingService bindingService; + + private IdentityAuthenticator authenticator; + + @BeforeEach + void setUp() { + authenticator = new IdentityAuthenticator(accessPolicy, bindingService); + } + + @Test + void authenticate_allowDecision_delegatesToBindOrCreate() { + IdentityClaims claims = new OAuthClaims("github", "gh_1", "u@example.com", true, "user", Map.of()); + PlatformPrincipal expected = new PlatformPrincipal("usr_1", "user", "u@example.com", null, "github", Set.of("USER")); + + when(accessPolicy.evaluate(claims)).thenReturn(AccessDecision.ALLOW); + when(bindingService.bindOrCreate(claims, UserStatus.ACTIVE)).thenReturn(expected); + + PlatformPrincipal result = authenticator.authenticate(claims); + + assertThat(result).isSameAs(expected); + } + + @Test + void authenticate_pendingDecision_createsPendingUserAndThrows() { + IdentityClaims claims = new OAuthClaims("cas", "user-x", "x@example.com", true, "X", Map.of()); + + when(accessPolicy.evaluate(claims)).thenReturn(AccessDecision.PENDING_APPROVAL); + + assertThatThrownBy(() -> authenticator.authenticate(claims)) + .isInstanceOf(AccountPendingException.class); + + verify(bindingService).createPendingUserIfAbsent(claims); + verify(bindingService, never()).bindOrCreate(any(), any()); + } + + @Test + void authenticate_denyDecision_throwsAccessDeniedByPolicy_andDoesNotBind() { + IdentityClaims claims = new OAuthClaims("cas", "user-y", "y@bad.example", true, "Y", Map.of()); + + when(accessPolicy.evaluate(claims)).thenReturn(AccessDecision.DENY); + + assertThatThrownBy(() -> authenticator.authenticate(claims)) + .isInstanceOf(AccessDeniedByPolicyException.class); + + verify(bindingService, never()).bindOrCreate(any(), any()); + verify(bindingService, never()).createPendingUserIfAbsent(any()); + } +} diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuth2AuthorizationRequestResolverTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuth2AuthorizationRequestResolverTest.java index 357ada33..b770c4b9 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuth2AuthorizationRequestResolverTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuth2AuthorizationRequestResolverTest.java @@ -1,7 +1,6 @@ package com.iflytek.skillhub.auth.oauth; -import com.iflytek.skillhub.auth.identity.IdentityBindingService; -import com.iflytek.skillhub.auth.policy.AccessPolicy; +import com.iflytek.skillhub.auth.identity.IdentityAuthenticator; import jakarta.servlet.http.HttpSession; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -32,8 +31,7 @@ class OAuth2AuthorizationRequestResolverTest { .build(); OAuthLoginFlowService oauthLoginFlowService = new OAuthLoginFlowService( java.util.List.of(), - mock(AccessPolicy.class), - mock(IdentityBindingService.class) + mock(IdentityAuthenticator.class) ); resolver = new SkillHubOAuth2AuthorizationRequestResolver( new InMemoryClientRegistrationRepository(github), diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java index 029ec294..84a6f81b 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java @@ -1,7 +1,6 @@ package com.iflytek.skillhub.auth.oauth; -import com.iflytek.skillhub.auth.identity.IdentityBindingService; -import com.iflytek.skillhub.auth.policy.AccessPolicy; +import com.iflytek.skillhub.auth.identity.IdentityAuthenticator; import jakarta.servlet.http.HttpSession; import java.util.List; import org.junit.jupiter.api.Test; @@ -18,8 +17,7 @@ class OAuthLoginFlowServiceTest { void rememberReturnTo_stores_sanitized_return_target() { OAuthLoginFlowService service = new OAuthLoginFlowService( List.of(), - mock(AccessPolicy.class), - mock(IdentityBindingService.class) + mock(IdentityAuthenticator.class) ); MockHttpServletRequest request = new MockHttpServletRequest(); request.setParameter("returnTo", "/dashboard/publish"); @@ -36,8 +34,7 @@ class OAuthLoginFlowServiceTest { void resolveFailureRedirect_maps_access_denied_to_user_facing_page() { OAuthLoginFlowService service = new OAuthLoginFlowService( List.of(), - mock(AccessPolicy.class), - mock(IdentityBindingService.class) + mock(IdentityAuthenticator.class) ); String redirect = service.resolveFailureRedirect( @@ -52,8 +49,7 @@ class OAuthLoginFlowServiceTest { void consumeReturnTo_clearsUnsafeSessionValue() { OAuthLoginFlowService service = new OAuthLoginFlowService( List.of(), - mock(AccessPolicy.class), - mock(IdentityBindingService.class) + mock(IdentityAuthenticator.class) ); MockHttpServletRequest request = new MockHttpServletRequest(); HttpSession session = request.getSession(true); diff --git a/web/docker-entrypoint.d/30-runtime-config.sh b/web/docker-entrypoint.d/30-runtime-config.sh index 482849eb..a8bf88a4 100644 --- a/web/docker-entrypoint.d/30-runtime-config.sh +++ b/web/docker-entrypoint.d/30-runtime-config.sh @@ -15,10 +15,9 @@ set -eu : "${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_ENABLED:=false}" : "${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_PROVIDER:=}" : "${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_AUTO:=false}" -: "${SKILLHUB_WEB_AUTH_CAS_ENABLED:=false}" # Generate runtime-config.js -envsubst '${SKILLHUB_WEB_API_BASE_URL} ${SKILLHUB_PUBLIC_BASE_URL} ${SKILLHUB_WEB_AUTH_DIRECT_ENABLED} ${SKILLHUB_WEB_AUTH_DIRECT_PROVIDER} ${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_ENABLED} ${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_PROVIDER} ${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_AUTO} ${SKILLHUB_WEB_AUTH_CAS_ENABLED}' \ +envsubst '${SKILLHUB_WEB_API_BASE_URL} ${SKILLHUB_PUBLIC_BASE_URL} ${SKILLHUB_WEB_AUTH_DIRECT_ENABLED} ${SKILLHUB_WEB_AUTH_DIRECT_PROVIDER} ${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_ENABLED} ${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_PROVIDER} ${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_AUTO}' \ < /usr/share/nginx/html/runtime-config.js.template \ > /usr/share/nginx/html/runtime-config.js diff --git a/web/runtime-config.js.template b/web/runtime-config.js.template index de67ab94..1375a380 100644 --- a/web/runtime-config.js.template +++ b/web/runtime-config.js.template @@ -5,6 +5,5 @@ window.__SKILLHUB_RUNTIME_CONFIG__ = { authDirectProvider: "${SKILLHUB_WEB_AUTH_DIRECT_PROVIDER}", authSessionBootstrapEnabled: "${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_ENABLED}", authSessionBootstrapProvider: "${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_PROVIDER}", - authSessionBootstrapAuto: "${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_AUTO}", - authCasEnabled: "${SKILLHUB_WEB_AUTH_CAS_ENABLED}" + authSessionBootstrapAuto: "${SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_AUTO}" }; diff --git a/web/src/api/client.ts b/web/src/api/client.ts index a9b3a661..3204d56a 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -63,7 +63,6 @@ type RuntimeConfig = { authSessionBootstrapEnabled?: string authSessionBootstrapProvider?: string authSessionBootstrapAuto?: string - authCasEnabled?: string } declare global { @@ -164,17 +163,6 @@ export function getSessionBootstrapRuntimeConfig(): SessionBootstrapRuntimeConfi } } -export type CasAuthRuntimeConfig = { - enabled: boolean -} - -export function getCasAuthRuntimeConfig(): CasAuthRuntimeConfig { - const config = getRuntimeConfig() - return { - enabled: parseBooleanFlag(config.authCasEnabled), - } -} - type ApiEnvelope = { code: number msg: string diff --git a/web/src/api/types.ts b/web/src/api/types.ts index bde5ec41..88c4bc36 100644 --- a/web/src/api/types.ts +++ b/web/src/api/types.ts @@ -17,7 +17,7 @@ export type OAuthProvider = Omit { - it('exports LoginButton component', () => { - expect(loginButton.LoginButton).toBeTypeOf('function') +function method(overrides: Partial): AuthMethod { + return { + id: overrides.id ?? 'test', + methodType: overrides.methodType ?? 'PASSWORD', + provider: overrides.provider ?? 'test', + displayName: overrides.displayName ?? 'Test', + actionUrl: overrides.actionUrl ?? '/test', + } +} + +describe('isExternalRedirectMethod', () => { + it('matches OAUTH_REDIRECT methods', () => { + expect(isExternalRedirectMethod(method({ methodType: 'OAUTH_REDIRECT' }))).toBe(true) + }) + + it('matches CAS_REDIRECT methods', () => { + expect(isExternalRedirectMethod(method({ methodType: 'CAS_REDIRECT' }))).toBe(true) + }) + + it('rejects local password method', () => { + expect(isExternalRedirectMethod(method({ methodType: 'PASSWORD' }))).toBe(false) + }) + + it('rejects direct password method', () => { + expect(isExternalRedirectMethod(method({ methodType: 'DIRECT_PASSWORD' }))).toBe(false) + }) + + it('rejects session-bootstrap method', () => { + expect(isExternalRedirectMethod(method({ methodType: 'SESSION_BOOTSTRAP' }))).toBe(false) + }) +}) + +describe('login-button module exports', () => { + it('exports LoginButton component', () => { + expect(LoginButton).toBeTypeOf('function') }) }) diff --git a/web/src/features/auth/login-button.tsx b/web/src/features/auth/login-button.tsx index 6a5e0cd5..a942d83b 100644 --- a/web/src/features/auth/login-button.tsx +++ b/web/src/features/auth/login-button.tsx @@ -1,5 +1,6 @@ import { useTranslation } from 'react-i18next' import { Button } from '@/shared/ui/button' +import type { AuthMethod } from '@/api/types' import { useAuthMethods } from './use-auth-methods' interface LoginButtonProps { @@ -7,9 +8,14 @@ interface LoginButtonProps { } /** - * Returns the appropriate icon for a given OAuth provider. + * Method types this button renders. CAS uses the same redirect-and-callback shape as OAuth from + * the UI's perspective, so we treat both as external-redirect providers. */ -function OAuthIcon({ provider }: { provider: string }) { +export function isExternalRedirectMethod(method: AuthMethod): boolean { + return method.methodType === 'OAUTH_REDIRECT' || method.methodType === 'CAS_REDIRECT' +} + +function ExternalProviderIcon({ provider }: { provider: string }) { const normalizedProvider = provider.toLowerCase() return ( method.methodType === 'OAUTH_REDIRECT' || method.methodType === 'CAS_REDIRECT', - ) + const providers = (data ?? []).filter(isExternalRedirectMethod) if (isLoading) { return ( @@ -53,7 +57,7 @@ export function LoginButton({ returnTo }: LoginButtonProps) { window.location.href = provider.actionUrl }} > - + {t('loginButton.loginWith', { name: provider.displayName })} ))}