From b43ab5e02cc0cc96cee1215cb239cf4cac27afd5 Mon Sep 17 00:00:00 2001 From: Patrick Plate Date: Wed, 24 Jun 2026 20:22:36 +0200 Subject: [PATCH] =?UTF-8?q?fix(sprint-0):=20panel-review-v2=20blockers=20?= =?UTF-8?q?=E2=80=94=20scoped=20security=20chain,=20fail-closed=20CORS,=20?= =?UTF-8?q?no=20@ComponentScan,=20drop=20dead=20RefreshToken?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-v2 (Sprint-0-Plan-Review-v2) blockers: - B1: SecurityConfig chain now securityMatcher-scoped to plate-auth endpoints so it cannot hijack the consuming app's routes - B2: removed @ComponentScan from auto-config; explicit @Import of @Configuration + @Service/@RestController classes - B4: CORS fails closed (same-origin) when allowed-origins empty instead of defaulting to '*' - B5: removed dead RefreshToken entity + repo; v0.1 uses stateless JWT refresh (rotation deferred to v0.3) - W-A: documented OnboardingHook transaction contract Verified: mvn -pl plate-auth-starter compile succeeds. --- CHANGELOG.md | 15 +++++ .../auth/PlateAuthAutoConfiguration.java | 37 +++++++++++- .../platesoft/auth/config/SecurityConfig.java | 60 +++++++++++++++---- .../platesoft/auth/entity/RefreshToken.java | 42 ------------- .../repository/RefreshTokenRepository.java | 12 ---- .../de/platesoft/auth/spi/OnboardingHook.java | 17 ++++++ 6 files changed, 117 insertions(+), 66 deletions(-) delete mode 100644 plate-auth-starter/src/main/java/de/platesoft/auth/entity/RefreshToken.java delete mode 100644 plate-auth-starter/src/main/java/de/platesoft/auth/repository/RefreshTokenRepository.java diff --git a/CHANGELOG.md b/CHANGELOG.md index abd0502..849a0cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,21 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Security / Correctness — Review-v2 blockers fixed +- **B1:** `SecurityConfig` `SecurityFilterChain` is now `securityMatcher`-scoped to plate-auth's own + endpoints (`/api/auth/**`, `/api/invitations/**`, `/api/access-requests/**`, `/api/admin/**`, `/api/me`, + `/api/memberships/**`). Previously an unscoped `@Order(-100)` chain with `anyRequest().authenticated()` + would hijack the consuming app's own routes. (panel B1) +- **B2:** Removed `@ComponentScan(basePackages="de.platesoft.auth")` from `PlateAuthAutoConfiguration` + (auto-configuration anti-pattern per Spring Boot guidance). Replaced with explicit `@Import` of the + concrete `@Configuration` classes + `@Service`/`@RestController` components. (panel B2) +- **B4:** CORS now fails closed by default. Empty `plate.auth.cors.allowed-origins` disables CORS for + plate-auth endpoints (same-origin only) instead of defaulting to `allowedOriginPatterns("*")`. (panel B4) +- **B5:** Removed dead `RefreshToken` entity + `RefreshTokenRepository`. v0.1 issues stateless JWT refresh + tokens (per the documented threat model); rotation/family-tracking is deferred to v0.3. (panel B5) +- **W-A:** Documented the `OnboardingHook` transaction contract (hooks run inside the exchange + transaction; keep them fast + idempotent). + ### Added - Initial project scaffold (W1) - Maven parent POM with `${revision}` CI-friendly versioning diff --git a/plate-auth-starter/src/main/java/de/platesoft/auth/PlateAuthAutoConfiguration.java b/plate-auth-starter/src/main/java/de/platesoft/auth/PlateAuthAutoConfiguration.java index babb444..fd92ee6 100644 --- a/plate-auth-starter/src/main/java/de/platesoft/auth/PlateAuthAutoConfiguration.java +++ b/plate-auth-starter/src/main/java/de/platesoft/auth/PlateAuthAutoConfiguration.java @@ -1,5 +1,13 @@ package de.platesoft.auth; +import de.platesoft.auth.config.PlateAuthFlywayConfig; +import de.platesoft.auth.config.PlateAuthExceptionHandler; +import de.platesoft.auth.config.SecurityConfig; +import de.platesoft.auth.controller.OAuthController; +import de.platesoft.auth.service.ExchangeService; +import de.platesoft.auth.service.JwtService; +import de.platesoft.auth.service.LoginEventService; +import de.platesoft.auth.service.MembershipService; import de.platesoft.auth.spi.*; import de.platesoft.auth.spi.defaults.*; import org.springframework.boot.autoconfigure.AutoConfiguration; @@ -8,13 +16,38 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.ComponentScan; +import org.springframework.context.annotation.Import; import org.springframework.data.jpa.repository.config.EnableJpaRepositories; import org.springframework.scheduling.annotation.EnableAsync; +/** + * plate-auth auto-configuration. + * + *

Review-v2 fix B2 — no {@code @ComponentScan}. Spring Boot's auto-configuration guidance is + * explicit that starter auto-config classes must not use {@code @ComponentScan}. It previously + * swept the whole {@code de.platesoft.auth} package, which is unpredictable in a consumer and can + * double-instantiate beans. Instead we now {@link Import @Import} the concrete {@code @Configuration} + * classes explicitly ({@link SecurityConfig}, {@link PlateAuthFlywayConfig}, + * {@link PlateAuthExceptionHandler}) and declare every SPI default as an explicit {@code @Bean} below. + * + *

Every concrete {@code @Service} / {@code @RestController} class the starter publishes is also + * explicitly {@link Import @Import}ed here so the starter is self-sufficient: it does not rely + * on the consuming application scanning {@code de.platesoft.auth}. New services/controllers added in + * later workstreams must be appended to the {@code @Import} list (this is the trade-off vs the old + * broad scan — it is intentional and keeps the bean surface explicit and predictable). + */ @AutoConfiguration @EnableConfigurationProperties(PlateAuthProperties.class) -@ComponentScan(basePackages = "de.platesoft.auth") +@Import({ + SecurityConfig.class, + PlateAuthFlywayConfig.class, + PlateAuthExceptionHandler.class, + ExchangeService.class, + JwtService.class, + LoginEventService.class, + MembershipService.class, + OAuthController.class +}) @AutoConfigurationPackage(basePackages = "de.platesoft.auth.entity") @EnableJpaRepositories(basePackages = "de.platesoft.auth.repository") @EnableAsync diff --git a/plate-auth-starter/src/main/java/de/platesoft/auth/config/SecurityConfig.java b/plate-auth-starter/src/main/java/de/platesoft/auth/config/SecurityConfig.java index cf7104c..030dde1 100644 --- a/plate-auth-starter/src/main/java/de/platesoft/auth/config/SecurityConfig.java +++ b/plate-auth-starter/src/main/java/de/platesoft/auth/config/SecurityConfig.java @@ -3,6 +3,7 @@ package de.platesoft.auth.config; import de.platesoft.auth.PlateAuthProperties; import de.platesoft.auth.filter.JwtAuthenticationFilter; import de.platesoft.auth.service.JwtService; +import lombok.extern.slf4j.Slf4j; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.annotation.Order; @@ -20,12 +21,32 @@ import org.springframework.web.cors.UrlBasedCorsConfigurationSource; import java.util.List; /** - * Security configuration for plate-auth. Registers a SecurityFilterChain - * that handles JWT validation and allows public access to auth endpoints. + * Security configuration for plate-auth. + * + *

Review-v2 fix B1 — scoped chain. This {@link SecurityFilterChain} is bound with + * {@code securityMatcher(...)} to only the endpoints plate-auth owns. This is mandatory for a + * starter: an unscoped chain at a high priority (previously {@code @Order(-100)} with + * {@code anyRequest().authenticated()}) would otherwise hijack the consuming application's own + * {@code SecurityFilterChain} and override its public routes. Consumers keep their own default + * (catch-all) chain for every path outside {@link #PLATE_AUTH_PATHS}. */ +@Slf4j @Configuration public class SecurityConfig { + /** + * Paths owned by plate-auth. The library's {@link SecurityFilterChain} only matches these. + * Everything else falls through to the consuming application's own security configuration. + */ + public static final String[] PLATE_AUTH_PATHS = { + "/api/auth/**", + "/api/invitations/**", + "/api/access-requests/**", + "/api/admin/**", + "/api/me", + "/api/memberships/**" + }; + @Bean public PasswordEncoder plateAuthPasswordEncoder() { return new BCryptPasswordEncoder(); @@ -36,13 +57,21 @@ public class SecurityConfig { return new JwtAuthenticationFilter(jwtService); } + /** + * plate-auth's security chain, scoped to {@link #PLATE_AUTH_PATHS} only. + * + *

Carries an explicit {@code securityMatcher} so it never competes with the consuming app's + * own (catch-all) {@code SecurityFilterChain}. Consumers retain full control of every path + * plate-auth does not own. + */ @Bean - @Order(-100) + @Order(100) public SecurityFilterChain plateAuthSecurityFilterChain( HttpSecurity http, JwtAuthenticationFilter jwtFilter, PlateAuthProperties props) throws Exception { http + .securityMatcher(PLATE_AUTH_PATHS) .csrf(AbstractHttpConfigurer::disable) .cors(cors -> cors.configurationSource(corsConfigurationSource(props))) .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) @@ -53,7 +82,8 @@ public class SecurityConfig { "/api/auth/register", "/api/auth/refresh", "/api/auth/config", - "/actuator/health" + "/api/access-requests", + "/api/access-requests/**" ).permitAll() .requestMatchers("/api/admin/**").hasAuthority("ROLE_ADMIN") .anyRequest().authenticated() @@ -62,18 +92,28 @@ public class SecurityConfig { return http.build(); } + /** + * CORS configuration source. + * + *

Review-v2 fix B4 — fail closed by default. When {@code plate.auth.cors.allowed-origins} + * is empty (the default), CORS is disabled for plate-auth's endpoints (same-origin only). + * An authentication library must never default to {@code allowedOriginPatterns("*")}. Consumers + * opt into cross-origin access by listing their origins explicitly. + */ private CorsConfigurationSource corsConfigurationSource(PlateAuthProperties props) { - CorsConfiguration config = new CorsConfiguration(); + UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource(); if (props.getCors().getAllowedOrigins().isEmpty()) { - config.setAllowedOriginPatterns(List.of("*")); - } else { - config.setAllowedOrigins(props.getCors().getAllowedOrigins()); - config.setAllowCredentials(true); + // Fail closed: register no CORS rule → Spring rejects cross-origin requests to auth paths. + log.warn("plate.auth.cors.allowed-origins is empty — CORS disabled for plate-auth endpoints " + + "(same-origin only). Set allowed-origins to enable cross-origin access."); + return source; } + CorsConfiguration config = new CorsConfiguration(); + config.setAllowedOrigins(props.getCors().getAllowedOrigins()); + config.setAllowCredentials(true); config.setAllowedMethods(List.of("GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS")); config.setAllowedHeaders(List.of("*")); config.setMaxAge(3600L); - UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource(); source.registerCorsConfiguration("/**", config); return source; } diff --git a/plate-auth-starter/src/main/java/de/platesoft/auth/entity/RefreshToken.java b/plate-auth-starter/src/main/java/de/platesoft/auth/entity/RefreshToken.java deleted file mode 100644 index f908fac..0000000 --- a/plate-auth-starter/src/main/java/de/platesoft/auth/entity/RefreshToken.java +++ /dev/null @@ -1,42 +0,0 @@ -package de.platesoft.auth.entity; - -import jakarta.persistence.*; -import lombok.*; -import org.hibernate.envers.Audited; - -import java.time.Instant; -import java.util.UUID; - -/** - * Refresh token entity for rotation tracking. - */ -@Entity -@Table(name = "refresh_tokens") -@Getter @Setter @NoArgsConstructor @AllArgsConstructor @Builder -public class RefreshToken { - - @Id - @Column(columnDefinition = "uuid") - private UUID id; - - @Column(name = "user_id", nullable = false, columnDefinition = "uuid") - private UUID userId; - - @Column(nullable = false, unique = true, length = 255) - private String token; - - @Column(name = "expires_at", nullable = false) - private Instant expiresAt; - - @Column(nullable = false) - private boolean revoked; - - @Column(name = "created_at", nullable = false, updatable = false) - private Instant createdAt; - - @PrePersist - void prePersist() { - if (id == null) id = UUID.randomUUID(); - if (createdAt == null) createdAt = Instant.now(); - } -} diff --git a/plate-auth-starter/src/main/java/de/platesoft/auth/repository/RefreshTokenRepository.java b/plate-auth-starter/src/main/java/de/platesoft/auth/repository/RefreshTokenRepository.java deleted file mode 100644 index a526b77..0000000 --- a/plate-auth-starter/src/main/java/de/platesoft/auth/repository/RefreshTokenRepository.java +++ /dev/null @@ -1,12 +0,0 @@ -package de.platesoft.auth.repository; - -import de.platesoft.auth.entity.RefreshToken; -import org.springframework.data.jpa.repository.JpaRepository; - -import java.util.Optional; -import java.util.UUID; - -public interface RefreshTokenRepository extends JpaRepository { - Optional findByToken(String token); - void deleteByUserId(UUID userId); -} diff --git a/plate-auth-starter/src/main/java/de/platesoft/auth/spi/OnboardingHook.java b/plate-auth-starter/src/main/java/de/platesoft/auth/spi/OnboardingHook.java index 1898640..b8a3e2d 100644 --- a/plate-auth-starter/src/main/java/de/platesoft/auth/spi/OnboardingHook.java +++ b/plate-auth-starter/src/main/java/de/platesoft/auth/spi/OnboardingHook.java @@ -6,8 +6,25 @@ import de.platesoft.auth.entity.User; /** * Called on first and subsequent sign-ins. Consumers wire their T3 onboarding logic here. * The default implementation is a no-op. + * + *

Transaction contract (Review-v2 W-A). Both methods are invoked inside the + * {@code @Transactional} boundary of {@code ExchangeService.verifyAndExchange} — i.e. within the same + * transaction that creates/updates the user and records the login event. Two consequences consumers + * must respect: + *

+ * Future v0.2 may move these calls to a post-commit phase; until then assume "in-transaction". */ public interface OnboardingHook { + /** + * Called the first time a user authenticates via a provider (no prior {@code user_identity}). + * Runs inside the exchange transaction — see class-level contract. + */ void onFirstSignIn(User user, LoginProvider provider); default void onSubsequentSignIn(User user, LoginProvider provider) {