Conversation
This allows a frontend to make requests to the tool support admin endpoints.
There was a problem hiding this comment.
Pull request overview
This PR enables CORS (Cross-Origin Resource Sharing) support for admin endpoints to allow frontend applications to make requests to the tool support admin API. The implementation adds configurable CORS origins through Spring Boot configuration properties and integrates them into the existing security configuration.
Key changes:
- Introduces a new
AdminPropertiesconfiguration class to manage admin-specific settings including CORS origins - Adds CORS configuration to the admin security filter chain with support for credentials and configurable allowed origins
- Updates test configuration to include the new
AdminPropertiesbean
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/main/java/uk/ac/ox/ctl/admin/AdminProperties.java | New configuration properties class that defines the admin.corsOrigins property for CORS configuration |
| src/main/java/uk/ac/ox/ctl/admin/AdminWebSecurity.java | Adds CORS configuration method and integrates it into the admin security filter chain with credential support |
| src/test/java/uk/ac/ox/ctl/admin/AdminControllerWebTest.java | Imports AdminProperties to ensure the configuration bean is available during web MVC tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @WebMvcTest(properties = {"spring.security.user.name=user", "spring.security.user.password=pass1234"}, controllers = AdminController.class, excludeFilters = @ComponentScan.Filter(type = FilterType.REGEX, pattern = "uk\\.ac\\.ox\\.ctl\\.(canvasproxy|ltiauth)\\..*")) | ||
| @Import({AdminWebSecurity.class}) | ||
| @Import({AdminWebSecurity.class, AdminProperties.class}) |
There was a problem hiding this comment.
The newly added CORS configuration for admin endpoints lacks test coverage. Since this file contains comprehensive tests for other AdminController functionality, the CORS behavior should also be tested. Consider adding tests to verify that CORS headers are properly set when configured origins are provided, and that CORS requests are rejected when origins don't match the configuration.
| return NOOP_PASSWORD_PREFIX + password; | ||
| } | ||
|
|
||
| public CorsConfigurationSource corsConfigurationSource(AdminProperties adminProperties) { |
There was a problem hiding this comment.
The corsConfigurationSource method deviates from the established pattern in WebSecurityConfiguration where a similar method is annotated with @bean. For consistency and to follow the existing codebase patterns, consider making this a @bean method and giving it a distinctive name (e.g., "adminCorsConfigurationSource") to avoid bean name conflicts.
| log.info("Admin CORS allowed origins: {}", String.join(", ", adminProperties.getCorsOrigins())); | ||
| } | ||
| CorsConfiguration config = new CorsConfiguration(); | ||
| config.setAllowCredentials(true); |
There was a problem hiding this comment.
Setting allowCredentials to true while allowing wildcard origins (CorsConfiguration.ALL which is '*') creates a security vulnerability. When allowCredentials is true, browsers will not accept wildcard origins. This configuration will fail at runtime if a wildcard origin is configured. Either remove allowCredentials(true) or ensure that wildcard origins cannot be used with credentials.
There was a problem hiding this comment.
We don't need the credentials so we no longer ask browsers to send it, this is because we manually add the Authorization header. Wildcard origins allow for easier development.
| } | ||
| return NOOP_PASSWORD_PREFIX + password; | ||
| } | ||
|
|
There was a problem hiding this comment.
The corsConfigurationSource method lacks documentation explaining its purpose, parameters, and how it integrates with the security configuration. Consider adding a Javadoc comment to explain the CORS configuration behavior, especially the relationship with AdminProperties and the security implications of the configuration choices.
| /** | |
| * Builds a {@link CorsConfigurationSource} for the admin endpoints. | |
| * <p> | |
| * The allowed origins are taken from {@link AdminProperties#getCorsOrigins()}. | |
| * For each configured origin, a corresponding allowed origin is added to the | |
| * CORS configuration. If the special {@link CorsConfiguration#ALL} value | |
| * ({@code "*"}) is present, a warning is logged as this is generally not | |
| * recommended for production environments. | |
| * <p> | |
| * The resulting configuration: | |
| * <ul> | |
| * <li>applies only to {@code /admin/**} paths,</li> | |
| * <li>allows credentials,</li> | |
| * <li>allows all headers, and</li> | |
| * <li>allows all HTTP methods.</li> | |
| * </ul> | |
| * This method is used by {@link #adminConfiguration(HttpSecurity, AdminProperties)} | |
| * to enable CORS handling for the admin security filter chain. | |
| * | |
| * @param adminProperties configuration properties providing the list of | |
| * allowed CORS origins for the admin endpoints | |
| * @return a {@link CorsConfigurationSource} to be used by Spring Security | |
| * when processing CORS requests to {@code /admin/**} | |
| */ |
| @ConfigurationProperties(prefix = "admin") | ||
| public class AdminProperties { | ||
|
|
There was a problem hiding this comment.
The AdminProperties class lacks documentation explaining its purpose and usage. As this is a configuration class that controls CORS behavior for admin endpoints, it should include class-level and field-level documentation explaining what the corsOrigins property does and providing examples of proper configuration values.
| @ConfigurationProperties(prefix = "admin") | |
| public class AdminProperties { | |
| @ConfigurationProperties(prefix = "admin") | |
| /** | |
| * Configuration properties for admin endpoints. | |
| * <p> | |
| * This class is bound to properties with the {@code admin} prefix | |
| * (for example {@code admin.cors-origins}) and is used to control | |
| * behaviour that is specific to administrative APIs and UIs. | |
| * <p> | |
| * Typical configuration examples (in {@code application.yml}): | |
| * <pre>{@code | |
| * admin: | |
| * cors-origins: | |
| * - "https://admin.example.com" | |
| * - "https://ops.example.org" | |
| * }</pre> | |
| */ | |
| public class AdminProperties { | |
| /** | |
| * List of allowed CORS origins for admin endpoints. | |
| * <p> | |
| * Each value must be a valid origin, such as | |
| * {@code https://admin.example.com}. An empty list | |
| * means that no additional CORS origins are configured | |
| * for admin endpoints. | |
| * <p> | |
| * Example (in {@code application.properties}): | |
| * <pre>{@code | |
| * admin.cors-origins=https://admin.example.com,https://ops.example.org | |
| * }</pre> | |
| */ |
As we're manually managing the Authorization header we don't need to allow credentials through CORS.
This allows a frontend to make requests to the tool support admin endpoints.