sbglasius commented on code in PR #16174:
URL: https://github.com/apache/grails-core/pull/16174#discussion_r3825111026


##########
grails-spring-security/cas/plugin/src/main/groovy/grails/plugin/springsecurity/cas/SpringSecurityCasGrailsPlugin.groovy:
##########
@@ -35,25 +36,28 @@ import 
org.springframework.security.cas.authentication.CasAuthenticationProvider
 import org.springframework.security.cas.authentication.NullStatelessTicketCache
 import org.springframework.security.cas.web.CasAuthenticationEntryPoint
 import org.springframework.security.cas.web.CasAuthenticationFilter
+import 
org.springframework.security.web.authentication.session.NullAuthenticatedSessionStrategy
 
+import grails.plugin.springsecurity.BeanTypeResolver
 import grails.plugin.springsecurity.SecurityFilterPosition
 import grails.plugin.springsecurity.SpringSecurityUtils
 import grails.plugins.Plugin
 
+@Slf4j
 @CompileStatic
 class SpringSecurityCasGrailsPlugin extends Plugin {
 
-    String grailsVersion = '7.0.0 > *'
+    String grailsVersion = '8.0.0-SNAPSHOT > *'
     String author = 'Burt Beckwith'

Review Comment:
   Done in 2391490. Removed both `author` and `authorEmail` (the latter was an 
empty string and meaningless without the former).



##########
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:
   Good catch, that section directly contradicted the change. Rewritten in 
2391490: it now leads with the fact that it's opt-in and *why* (enabling it 
disables session fixation prevention, because CAS maps the service ticket to 
the session id), shows the `useSingleSignout: true` snippet, and mentions the 
startup warning. The former "if you don't want the filter registered, disable 
it" paragraph is now framed as returning to the default.



##########
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:
   Fixed in 2391490 — renumbered to 48.
   
   Worth flagging that this was more than cosmetic: 45, 46 and 47 all landed 
upstream while this PR was open, so the file actually had *two* sections 
numbered 46 and would have produced duplicate asciidoctor anchors. I checked 
the whole file for other collisions and there are none.



##########
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:
   Agreed, and this is the failure mode most likely to bite in CI. Fixed in 
2391490: added a `REQUEST_TIMEOUT` of 60s applied to both `get()` and 
`postForm()` via `HttpRequest.timeout(...)`, so a container that accepts the 
connection but never answers fails the spec in a minute instead of hanging 
until the runner's job-level kill. Kept the 30s `connectTimeout` as-is.
   
   I went with a per-request timeout rather than `@Timeout` so the failure 
points at the specific stalled call rather than the whole feature method.



##########
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:
   You're right that the annotation was dead, and right about the reasoning: 
the docs this PR adds say the plugin logs a warning, and an app capturing logs 
but not stdout would never see a `println`. Restored `log.warn` alongside the 
`println` in 2391490, matching `SpringSecurityCoreGrailsPlugin:423-424`. 
Whitespace-only line trimmed too.



##########
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:
   Correct, and the root cause was worse than the docs: `bootRun` forks its own 
JVM, so `-DTESTCONFIG` never reached the app at all — there was no way to 
exercise the endpoint by hand.
   
   Rather than just note the limitation, `build.gradle` now forwards 
`TESTCONFIG` and `casContainerVersion` to `bootRun`, and the README documents 
`bootRun -DTESTCONFIG=casProxy` (2391490). Verified end-to-end: drove a login 
against the container and `/secure/proxyStatus` returned 
`PROXY_TICKET:PT-2-1B26izig…` instead of `NO_PROXY_TICKET`.



##########
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:
   Fixed in 2391490, 4 spaces per `.editorconfig`. `git diff 
--ignore-all-space` on that file is empty, so it's purely whitespace.



##########
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:
   Reworded in 2391490 to "which the app opts into via `cas.useSingleSignout`", 
and added a pointer to `CasNoSingleSignOutSpec` as the spec covering the 
default.



-- 
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]

Reply via email to