jdaugherty commented on code in PR #16174: URL: https://github.com/apache/grails-core/pull/16174#discussion_r3824861585
########## grails-doc/src/en/guide/security/securityPlugins/springSecurity/cas/usage.adoc: ########## @@ -25,7 +25,7 @@ under the License. Configuring your CAS server is beyond the scope of this document. There are many different approaches and this will most likely be done by IT staff. It's assumed here that you already have a running CAS server. ==== -https://www.jasig.org/cas[CAS] is a popular single sign-on implementation. It's open source and has an Apache-like license, and is easy to get started with but is also highly configurable. In addition it has clients written in Java, .Net, PHP, Perl, and other languages. +https://apereo.github.io/cas[CAS] is a popular single sign-on implementation. It's open source and has an Apache-like license, and is easy to get started with but is also highly configurable. In addition it has clients written in Java, .Net, PHP, Perl, and other languages. Review Comment: Not this line, but this file: the `=== Single Signout` section below (line 62) still says "Single signout is enabled by default" and only documents how to disable it. Since this PR flips `cas.useSingleSignout` to opt-in, that section needs updating to match the new default (off, enable explicitly). ########## grails-test-examples/spring-security/cas/README.md: ########## @@ -14,21 +14,77 @@ See the License for the specific language governing permissions and limitations under the License. --> -This is a CAS-enabled test application. To run it successfully, a CAS -server is required. The URL for the CAS server is configured in the -[application.groovy](test1/grails-app/conf/application.groovy) -file. Setting up a CAS server is out of the scope of this document, but -good places to start are [Apereo CAS GitHub](https://github.com/apereo/cas) -and the [CAS Initializr](https://getcas.apereo.org/ui) service. +This is a CAS-enabled test application. It no longer needs a hand-run CAS server: an +[Apereo CAS](https://github.com/apereo/cas) server is started in a container by +[CasContainerHolder](test1/src/main/groovy/grails/plugin/springsecurity/cas/test/CasContainerHolder.groovy), +and [CasTestEnvironmentPostProcessor](test1/src/main/groovy/grails/plugin/springsecurity/cas/test/CasTestEnvironmentPostProcessor.groovy) +points the CAS plugin at it before the application context is built. Docker (or a compatible +container runtime) is therefore required to run or test this application. -The test application can be run with: +## Running the tests -`./gradlew :testapp-spring-security-cas-test1:bootRun` +The application is exercised under three configurations, selected with the `TESTCONFIG` system +property. Each has to be its own run, because the configuration is applied at application startup. + +The specs only run when one of these is selected, so a build that covers every Spring Security +example does not repeat a configuration that the dedicated per-configuration CI job already runs. +Running `check` without `-DTESTCONFIG` therefore reports no CAS tests. + +| `TESTCONFIG` | Configuration | Covered by | +|---|---|---| +| `cas` (default) | `proxyCallbackUrl` and `proxyReceptorUrl` unset, single signout enabled | `CasLoginSpec`, `CasNoProxyReceptorSpec`, `CasSingleSignOutSpec` | +| `casProxy` | both proxy settings configured, single signout enabled | `CasLoginSpec`, `CasProxyTicketSpec`, `CasSingleSignOutSpec` | +| `casNoSingleSignout` | `cas.useSingleSignout` left at its default | `CasLoginSpec`, `CasNoProxyReceptorSpec`, `CasNoSingleSignOutSpec` | + +``` +./gradlew :grails-test-examples-spring-security-cas-test1:check -DTESTCONFIG=cas +./gradlew :grails-test-examples-spring-security-cas-test1:check -DTESTCONFIG=casProxy +./gradlew :grails-test-examples-spring-security-cas-test1:check -DTESTCONFIG=casNoSingleSignout +``` + +`cas.useSingleSignout` is opt-in as of Grails 8. The app enables it for the first two configurations +so the single signout filter is exercised; enabling it disables session fixation prevention, and the +plugin warns about that at startup. + +The CAS image is pinned to a known-good version and can be overridden: + +``` +./gradlew :grails-test-examples-spring-security-cas-test1:check -PcasContainerVersion=7.3.6 +``` + +The specs skip themselves when no Docker daemon is available. + +## Running the application + +``` +./gradlew :grails-test-examples-spring-security-cas-test1:bootRun +``` The test application URLs are: * [http://localhost:8081/secure/admins](http://localhost:8081/secure/admins) * [http://localhost:8081/secure/users](http://localhost:8081/secure/users) +* [http://localhost:8081/secure/proxyStatus](http://localhost:8081/secure/proxyStatus) — asks CAS for a proxy ticket Review Comment: Under `bootRun`, `TESTCONFIG` is unset, so `CasTestEnvironmentPostProcessor` never configures `proxyReceptorUrl`/`proxyCallbackUrl` and this endpoint can only ever return `NO_PROXY_TICKET`. A developer following this section will conclude the proxy support is broken. Worth either noting that limitation here or documenting how to start `bootRun` with the proxy configuration so the endpoint can actually be exercised. ########## grails-test-examples/spring-security/cas/test1/src/integration-test/groovy/grails/plugin/springsecurity/cas/test/CasSingleSignOutSpec.groovy: ########## @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package grails.plugin.springsecurity.cas.test + +import spock.lang.IgnoreIf + +import java.net.http.HttpResponse + +/** + * Covers single sign-out, which the plugin enables by default via {@code cas.useSingleSignout} by Review Comment: "which the plugin enables by default" is stale — this PR makes `cas.useSingleSignout` opt-in (default `false`), which is exactly what `CasNoSingleSignOutSpec` asserts. Suggest rewording to something like "which the app opts into via `cas.useSingleSignout`". ########## grails-test-examples/spring-security/cas/test1/src/integration-test/groovy/grails/plugin/springsecurity/cas/test/CasLoginSpec.groovy: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package grails.plugin.springsecurity.cas.test + + +import spock.lang.IgnoreIf + +import java.net.http.HttpResponse + +/** + * Covers the CAS handshake the plugin exists to perform: redirect to CAS, service ticket back, + * ticket validated against the CAS server, and the resulting authentication carrying the roles + * looked up in GORM. + */ +@IgnoreIf({ !CasTestConfig.configured }) +class CasLoginSpec extends AbstractCasSpec { + + void 'an unauthenticated request is redirected to the CAS login page for this service'() { + when: + HttpResponse<String> response = get(appClient, "${appBaseUrl}/secure/users") + + then: + response.statusCode() == 302 + + and: 'the redirect targets the CAS server the container is running' + location(response).startsWith("${casBaseUrl}/login") + + and: 'it asks CAS to send the ticket back to this application' + location(response).contains('service=') + location(response).contains(URLEncoder.encode(CasTestConfig.serviceUrl(serverPort), 'UTF-8')) + } + + void 'a user authenticated at CAS reaches a ROLE_USER action'() { + when: + HttpResponse<String> response = login('/secure/users', 'user', 'user') + + then: + response.statusCode() == 200 + response.body().contains('Logged in with ROLE_USER') + } + + void 'an admin authenticated at CAS reaches a ROLE_ADMIN action'() { + when: + HttpResponse<String> response = login('/secure/admins', 'admin', 'admin') + + then: + response.statusCode() == 200 + response.body().contains('Logged in with ROLE_ADMIN') + } + + void 'a user without the role is denied a ROLE_ADMIN action'() { + given: + login('/secure/users', 'user', 'user') + + when: + HttpResponse<String> response = followRedirects(get(appClient, "${appBaseUrl}/secure/admins")) + + then: 'access is refused rather than granted' + response.statusCode() == 403 || !response.body().contains('Logged in with ROLE_ADMIN') + } + + void 'bad credentials do not authenticate'() { + when: + HttpResponse<String> challenge = get(appClient, "${appBaseUrl}/secure/users") + String loginUrl = location(challenge) + HttpResponse<String> form = get(casClient, loginUrl) + String execution = form.body().find(/name="execution"\s+value="([^"]+)"/) { full, token -> token } + HttpResponse<String> submitted = postForm(casClient, loginUrl, Review Comment: This re-implements the execution-token extraction inline, duplicating `AbstractCasSpec.extractExecution` but dropping the null guard the other paths get. If a future `casContainerVersion` renders the login form with different attribute order or quoting, `execution` is null here and `postForm` fails with an opaque NPE from `URLEncoder.encode(null)` inside the when-block, instead of the clear missing-token assertion `authenticateAtCas` produces. Reuse the base-class extraction and guard. ########## grails-test-examples/spring-security/cas/test1/grails-app/controllers/grails/plugin/springsecurity/cas/test/SecureController.groovy: ########## @@ -0,0 +1,58 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package grails.plugin.springsecurity.cas.test + +import grails.plugin.springsecurity.annotation.Secured +import org.apereo.cas.client.authentication.AttributePrincipal +import org.springframework.security.cas.authentication.CasAuthenticationToken +import org.springframework.security.core.Authentication +import org.springframework.security.core.context.SecurityContextHolder + +class SecureController { + Review Comment: The body of this file is indented with tabs; `.editorconfig` sets `indent_style = space` (4 spaces) for Groovy sources. Since git treats this as a new file (the old controller path became `resources.groovy` in rename detection), it should come in with the standard indentation. ########## grails-doc/src/en/guide/upgrading/upgrading80x.adoc: ########## @@ -2632,3 +2632,36 @@ used to apply to its own message source. Adding or removing a base name now needs a restart, because Spring Boot reads the configured base names once when it builds the message source. + +==== 46. CAS Single Sign-Out Is Opt-In Review Comment: Sections 46 (Startup Banner, line 2481) and 47 (Message Bundles, line 2517) already exist above, so this should be `==== 48.` to keep the numbering sequential. ########## grails-spring-security/cas/plugin/src/main/groovy/grails/plugin/springsecurity/cas/SpringSecurityCasGrailsPlugin.groovy: ########## @@ -87,10 +91,29 @@ class SpringSecurityCasGrailsPlugin extends Plugin { if (conf.cas.useSingleSignout) { - // session fixation prevention breaks single signout because - // the service ticket is mapped to the session id which changes + // Session fixation prevention breaks single signout because the service ticket is + // mapped to the session id, which changes when the session is replaced on login. + // Disabling it is a security trade-off the application has opted into, so say so. + String message = ''' + WARNING: cas.useSingleSignout is enabled, so session fixation prevention has been disabled. + CAS maps the service ticket to the HTTP session id, and a logout request cannot be matched to a + session that was replaced when the user authenticated. Set cas.useSingleSignout to false to keep + session fixation prevention and handle logout in the application instead. + ''' + println message Review Comment: With `log.warn` removed, the `@Slf4j` annotation added above is now unused. Two options: drop the annotation, or keep both outputs like the core plugin does (`SpringSecurityCoreGrailsPlugin` pairs `println` with `log.warn` for its warnings). The docs added in this PR say the plugin "logs a warning at startup", and an app that captures logs but not stdout will never see a `println`, so pairing them seems preferable. Also, the line just below is whitespace-only and should be trimmed. ########## grails-test-examples/spring-security/cas/test1/src/integration-test/groovy/grails/plugin/springsecurity/cas/test/AbstractCasSpec.groovy: ########## @@ -0,0 +1,178 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package grails.plugin.springsecurity.cas.test + +import grails.testing.mixin.integration.Integration +import spock.lang.Requires +import spock.lang.Specification + +import java.net.http.HttpClient +import java.net.http.HttpRequest +import java.net.http.HttpResponse +import java.time.Duration + +/** + * Drives the CAS protocol by hand against the containerised CAS server. + * + * <p>The app and CAS are both reached on {@code localhost}, and cookies ignore ports, so a single + * cookie jar would send the app's session cookie to CAS and vice versa. Each side therefore gets + * its own client, and redirects are followed explicitly so the right one is used for each hop.</p> + */ +@Integration +@Requires({ isDockerAvailable() }) +abstract class AbstractCasSpec extends Specification { + + HttpClient appClient + HttpClient casClient + + /** + * A cookie-less client for the calls CAS makes to the application on its own connection, such + * as the back-channel logout request. Using the authenticated client instead would hide which + * filter acted: an unconsumed POST to the CAS login path fails authentication and clears the + * session by itself, which looks the same from outside as single signout working. + */ + HttpClient backChannelClient + + /** The service ticket CAS issued during the most recent {@link #login} call. */ + String lastServiceTicket + + void setup() { + appClient = newClient() + casClient = newClient() + backChannelClient = newClient() + } + + /** + * Mirrors the probe used by the hibernate7 specs. Checking the socket avoids the macOS failure + * mode where asking Testcontainers for a client throws when the daemon API version differs. + */ + static boolean isDockerAvailable() { + List<String> candidates = [ + System.getProperty('user.home') + '/.docker/run/docker.sock', + '/var/run/docker.sock', + System.getenv('DOCKER_HOST') ?: '' + ] + candidates.any { it && new File(it).exists() } + } + + String getAppBaseUrl() { + "http://localhost:${serverPort}"; + } + + String getCasBaseUrl() { + CasContainerHolder.serverUrlPrefix + } + + /** CAS redirects to the container-visible host name; the test client has to use localhost. */ + static String toLocalUrl(String url) { + url.replace(CasTestConfig.CONTAINER_VISIBLE_HOST, 'localhost') + } + + HttpResponse<String> get(HttpClient client, String url) { + client.send(HttpRequest.newBuilder(URI.create(url)).GET().build(), + HttpResponse.BodyHandlers.ofString()) + } + + HttpResponse<String> postForm(HttpClient client, String url, Map<String, String> form) { + String body = form.collect { k, v -> "${encode(k)}=${encode(v)}" }.join('&') + HttpRequest request = HttpRequest.newBuilder(URI.create(url)) + .header('Content-Type', 'application/x-www-form-urlencoded') + .POST(HttpRequest.BodyPublishers.ofString(body)) + .build() + client.send(request, HttpResponse.BodyHandlers.ofString()) + } + + static String location(HttpResponse<?> response) { + response.headers().firstValue('Location').orElse(null) + } + + /** + * Authenticates at CAS and returns the service ticket URL it redirects back to, already + * rewritten to localhost. + */ + String authenticateAtCas(String loginUrl, String username, String password) { + HttpResponse<String> form = get(casClient, loginUrl) + assert form.statusCode() == 200 + String execution = extractExecution(form.body()) + assert execution, 'CAS login form did not contain an execution token' + + HttpResponse<String> submitted = postForm(casClient, loginUrl, + [username: username, password: password, execution: execution, _eventId: 'submit']) + assert submitted.statusCode() == 302, + "expected CAS to redirect after login but got ${submitted.statusCode()}" + toLocalUrl(location(submitted)) + } + + /** Full login: hit a secured URL, authenticate at CAS, and follow the ticket back to the app. */ + HttpResponse<String> login(String securedPath, String username, String password) { + HttpResponse<String> challenge = get(appClient, appBaseUrl + securedPath) + assert challenge.statusCode() == 302, + "expected a redirect to CAS but got ${challenge.statusCode()}" + String ticketUrl = authenticateAtCas(location(challenge), username, password) + lastServiceTicket = extractTicket(ticketUrl) + assert lastServiceTicket, "CAS redirect carried no service ticket: ${ticketUrl}" + followRedirects(get(appClient, ticketUrl)) + } + + HttpResponse<String> followRedirects(HttpResponse<String> response, int limit = 5) { + HttpResponse<String> current = response + for (int i = 0; i < limit && current.statusCode() in [301, 302, 303, 307, 308]; i++) { + current = get(appClient, absolute(location(current))) + } + current + } + + String absolute(String location) { + String local = toLocalUrl(location) + local.startsWith('http') ? local : appBaseUrl + local + } + + /** The message CAS sends to a service on back-channel logout. */ + static String logoutRequest(String serviceTicket) { + """<samlp:LogoutRequest xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" \ +ID="LR-1-${System.nanoTime()}" Version="2.0" IssueInstant="2026-01-01T00:00:00Z">\ +<saml:NameID xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">@NOT_USED@</saml:NameID>\ +<samlp:SessionIndex>${serviceTicket}</samlp:SessionIndex>\ +</samlp:LogoutRequest>""" + } + + static String extractTicket(String url) { + def matcher = url =~ /[?&]ticket=([^&]+)/ + matcher.find() ? matcher.group(1) : null + } + + private static String extractExecution(String html) { + def matcher = html =~ /name="execution"\s+value="([^"]+)"/ + matcher.find() ? matcher.group(1) : null + } + + private static String encode(String value) { + URLEncoder.encode(value, 'UTF-8') + } + + private static HttpClient newClient() { + CookieManager cookieManager = new CookieManager(null, CookiePolicy.ACCEPT_ALL) + HttpClient.newBuilder() + .cookieHandler(cookieManager) + .followRedirects(HttpClient.Redirect.NEVER) + .connectTimeout(Duration.ofSeconds(30)) + .build() Review Comment: Only a connect timeout is set here — `client.send()` has no request timeout and none of the specs use `@Timeout`, so if the CAS container accepts the TCP connection but never responds (mid-startup stall, OOM), `get()`/`postForm()` block indefinitely and the CI matrix job hangs until the runner's job-level kill. Consider adding `.timeout(Duration.ofSeconds(...))` to the `HttpRequest` builders (or a Spock `@Timeout`) so a wedged container fails within minutes instead. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
