Copilot commented on code in PR #1205: URL: https://github.com/apache/grails-spring-security/pull/1205#discussion_r2817115542
########## plugin-core/plugin/src/main/groovy/grails/plugin/springsecurity/SecurityAutoConfigurationExcluder.groovy: ########## @@ -0,0 +1,101 @@ +/* + * 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 + +import groovy.transform.CompileStatic +import org.springframework.boot.autoconfigure.AutoConfigurationImportFilter +import org.springframework.boot.autoconfigure.AutoConfigurationMetadata + +/** + * Automatically excludes Spring Boot security auto-configuration classes that + * conflict with the Grails Spring Security plugin. + * + * <p>When the Grails Spring Security plugin is on the classpath, Spring Boot's + * security auto-configurations (e.g. {@code SecurityAutoConfiguration}, + * {@code SecurityFilterAutoConfiguration}) create duplicate + * {@code SecurityFilterChain} beans and other security infrastructure that + * conflicts with the plugin's own bean definitions in + * {@link SpringSecurityCoreGrailsPlugin#doWithSpring}.</p> + * + * <p>Previously, users had to manually exclude up to 7 auto-configuration classes + * in {@code application.yml}. This filter removes that requirement by + * automatically filtering them out during Spring Boot's auto-configuration + * discovery phase.</p> + * + * <p>Registered via {@code META-INF/spring.factories} as an + * {@link AutoConfigurationImportFilter}. This runs before auto-configuration + * bytecode is loaded, so there is no performance overhead from excluded classes.</p> + * + * @since 7.0.2 + * @see AutoConfigurationImportFilter + */ +@CompileStatic +class SecurityAutoConfigurationExcluder implements AutoConfigurationImportFilter { + + /** + * Spring Boot security auto-configuration classes that conflict with the + * Grails Spring Security plugin. These are excluded unconditionally when the + * plugin is on the classpath. + * + * <ul> + * <li>{@code SecurityAutoConfiguration} — creates a default {@code SecurityFilterChain} + * that conflicts with the plugin's {@code FilterChainProxy}</li> + * <li>{@code SecurityFilterAutoConfiguration} — registers a + * {@code DelegatingFilterProxyRegistrationBean} that duplicates the plugin's + * {@code springSecurityFilterChainRegistrationBean}</li> + * <li>{@code UserDetailsServiceAutoConfiguration} — creates an in-memory + * {@code UserDetailsService} that conflicts with the plugin's + * {@code GormUserDetailsService}</li> + * <li>{@code OAuth2ClientAutoConfiguration} (servlet) — conflicts when the + * plugin-oauth2 module manages OAuth2 configuration</li> + * <li>{@code OAuth2ResourceServerAutoConfiguration} — conflicts with the + * plugin's resource server security setup</li> + * <li>{@code ManagementWebSecurityAutoConfiguration} — Actuator security + * that conflicts when Actuator is on the classpath</li> + * </ul> Review Comment: The Javadoc list of excluded auto-configurations is incomplete: `EXCLUDED_AUTO_CONFIGURATIONS` contains 7 entries (including both the servlet and non-servlet `OAuth2ClientAutoConfiguration` classes), but the bullet list documents only 6 and omits `org.springframework.boot.autoconfigure.security.oauth2.client.OAuth2ClientAutoConfiguration`. Please update the list/rationale so the documentation matches the actual exclusions. ########## plugin-core/plugin/src/test/groovy/grails/plugin/springsecurity/SecurityAutoConfigurationExcluderSpec.groovy: ########## @@ -0,0 +1,164 @@ +/* + * 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 + +import spock.lang.Specification +import spock.lang.Subject +import spock.lang.Unroll + +/** + * Tests for {@link SecurityAutoConfigurationExcluder}. + * + * Verifies that Spring Boot security auto-configuration classes that conflict + * with the Grails Spring Security plugin are filtered out during the + * auto-configuration discovery phase. + */ +class SecurityAutoConfigurationExcluderSpec extends Specification { + + @Subject + SecurityAutoConfigurationExcluder excluder = new SecurityAutoConfigurationExcluder() + + @Unroll + def "match excludes conflicting auto-configuration: #className"() { + given: + String[] autoConfigs = [className] as String[] + + when: + boolean[] results = excluder.match(autoConfigs, null) + + then: 'the conflicting auto-configuration is excluded (false = filtered out)' + !results[0] + + where: + className << [ + 'org.springframework.boot.autoconfigure.security.servlet.SecurityAutoConfiguration', + 'org.springframework.boot.autoconfigure.security.servlet.SecurityFilterAutoConfiguration', + 'org.springframework.boot.autoconfigure.security.servlet.UserDetailsServiceAutoConfiguration', + 'org.springframework.boot.autoconfigure.security.oauth2.client.servlet.OAuth2ClientAutoConfiguration', + 'org.springframework.boot.autoconfigure.security.oauth2.client.OAuth2ClientAutoConfiguration', + 'org.springframework.boot.autoconfigure.security.oauth2.resource.servlet.OAuth2ResourceServerAutoConfiguration', + 'org.springframework.boot.actuate.autoconfigure.security.servlet.ManagementWebSecurityAutoConfiguration', + ] + } + + @Unroll + def "match preserves non-security auto-configuration: #className"() { + given: + String[] autoConfigs = [className] as String[] + + when: + boolean[] results = excluder.match(autoConfigs, null) + + then: 'non-security auto-configurations pass through (true = included)' + results[0] + + where: + className << [ + 'org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfiguration', + 'org.springframework.boot.autoconfigure.orm.jpa.HibernateJpaAutoConfiguration', + 'org.springframework.boot.autoconfigure.web.servlet.DispatcherServletAutoConfiguration', + 'org.springframework.boot.autoconfigure.jackson.JacksonAutoConfiguration', + 'org.springframework.boot.autoconfigure.cache.CacheAutoConfiguration', + ] + } + + def "match handles mixed array of included and excluded auto-configurations"() { + given: + String[] autoConfigs = [ + 'org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfiguration', + 'org.springframework.boot.autoconfigure.security.servlet.SecurityAutoConfiguration', + 'org.springframework.boot.autoconfigure.jackson.JacksonAutoConfiguration', + 'org.springframework.boot.autoconfigure.security.servlet.SecurityFilterAutoConfiguration', + 'org.springframework.boot.autoconfigure.web.servlet.DispatcherServletAutoConfiguration', + ] as String[] + + when: + boolean[] results = excluder.match(autoConfigs, null) + + then: + results[0] // DataSource — included + !results[1] // SecurityAutoConfiguration — excluded + results[2] // Jackson — included + !results[3] // SecurityFilterAutoConfiguration — excluded + results[4] // DispatcherServlet — included + } + + def "match handles empty array"() { + given: + String[] autoConfigs = [] as String[] + + when: + boolean[] results = excluder.match(autoConfigs, null) + + then: + results.length == 0 + } + + def "match handles null metadata parameter gracefully"() { + given: 'autoConfigurationMetadata is null (not used by this filter)' + String[] autoConfigs = [ + 'org.springframework.boot.autoconfigure.security.servlet.SecurityAutoConfiguration', + ] as String[] + + when: + boolean[] results = excluder.match(autoConfigs, null) + + then: 'still works correctly' + !results[0] + } + + def "getExcludedAutoConfigurations returns all 7 known conflicting classes"() { + when: + Set<String> excluded = SecurityAutoConfigurationExcluder.excludedAutoConfigurations + + then: + excluded.size() == 7 + excluded.contains('org.springframework.boot.autoconfigure.security.servlet.SecurityAutoConfiguration') + excluded.contains('org.springframework.boot.autoconfigure.security.servlet.SecurityFilterAutoConfiguration') + excluded.contains('org.springframework.boot.autoconfigure.security.servlet.UserDetailsServiceAutoConfiguration') + excluded.contains('org.springframework.boot.autoconfigure.security.oauth2.client.servlet.OAuth2ClientAutoConfiguration') + excluded.contains('org.springframework.boot.autoconfigure.security.oauth2.client.OAuth2ClientAutoConfiguration') + excluded.contains('org.springframework.boot.autoconfigure.security.oauth2.resource.servlet.OAuth2ResourceServerAutoConfiguration') + excluded.contains('org.springframework.boot.actuate.autoconfigure.security.servlet.ManagementWebSecurityAutoConfiguration') + } + + def "getExcludedAutoConfigurations returns unmodifiable set"() { + when: + Set<String> excluded = SecurityAutoConfigurationExcluder.excludedAutoConfigurations + excluded.add('some.new.AutoConfiguration') + + then: + thrown(UnsupportedOperationException) + } + + def "spring.factories registers the filter correctly"() { + when: 'reading the spring.factories resource from the classpath' + URL resource = getClass().getClassLoader().getResource('META-INF/spring.factories') + + then: 'the resource exists' + resource != null + + when: + String content = resource.text + + then: 'it registers SecurityAutoConfigurationExcluder as an AutoConfigurationImportFilter' + content.contains('org.springframework.boot.autoconfigure.AutoConfigurationImportFilter') + content.contains('grails.plugin.springsecurity.SecurityAutoConfigurationExcluder') Review Comment: This test reads a single `META-INF/spring.factories` via `getResource(...)`, but many dependencies on the test classpath also contain a `META-INF/spring.factories`. Depending on classpath ordering, this can load the wrong resource and make the test flaky or give false confidence. Prefer asserting registration via `SpringFactoriesLoader.loadFactories(AutoConfigurationImportFilter, classLoader)` (or enumerating `getResources(...)` and verifying the expected entry is present) instead of assuming `getResource` returns this module’s file. -- 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]
