Allow exposing data to anonymous users as config option#1137
Merged
Conversation
📝 WalkthroughWalkthroughAdds an ChangesAnonymous Access Feature Flag
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebSecurityConfig
participant FafApiProperties
participant MethodSecurityConfig
participant ElideUser
Client->>WebSecurityConfig: request to /data/**
WebSecurityConfig->>FafApiProperties: isAllowAnonymous()
alt allowAnonymous = true
FafApiProperties-->>WebSecurityConfig: true
WebSecurityConfig-->>Client: permitAll (no auth required)
else allowAnonymous = false
FafApiProperties-->>WebSecurityConfig: false
WebSecurityConfig->>MethodSecurityConfig: enforce method security
MethodSecurityConfig->>ElideUser: isInRole(role)
ElideUser->>ElideUser: fafAuthentication != null?
alt fafAuthentication exists
ElideUser-->>MethodSecurityConfig: hasRole(role) result
else fafAuthentication is null
ElideUser-->>MethodSecurityConfig: false
end
MethodSecurityConfig-->>WebSecurityConfig: authorization decision
WebSecurityConfig-->>Client: authorized or forbidden
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/faforever/api/config/security/WebSecurityConfig.java (1)
46-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
/actuator/**anonymous access is broader than health/readiness.This permits any exposed actuator endpoint anonymously. Limit anonymous access to health probes (typically
"/actuator/health"and"/actuator/health/**"), ideally GET-only.Suggested tightening
authorizeConfig.requestMatchers( "/swagger-ui/**", "/swagger-resources/**", "/v3/api-docs/**", - "/", - "/actuator/**" + "/" ).permitAll(); + authorizeConfig.requestMatchers( + HttpMethod.GET, + "/actuator/health", + "/actuator/health/**" + ).permitAll();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/faforever/api/config/security/WebSecurityConfig.java` around lines 46 - 52, The current security configuration permits anonymous access to all actuator endpoints using the `/actuator/**` pattern, which is overly permissive. Replace this broad pattern with specific patterns for health probes only, such as `/actuator/health` and `/actuator/health/**`. Additionally, restrict anonymous access to these health endpoints to GET requests only by using `requestMatchers` with the `.permitAll()` method being applied only to GET requests on health endpoints, while other actuator endpoints remain protected.src/main/java/com/faforever/api/security/ElideUser.java (1)
21-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNull-safe role check is incomplete;
getName()can still NPE.
isInRolenow handles null auth, but Line 22 still dereferencesfafAuthenticationdirectly. With anonymous/non-FAF principals, this remains a crash path.Suggested fix
`@Override` public String getName() { - return fafAuthentication.getName(); + return fafAuthentication != null ? fafAuthentication.getName() : super.getName(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/faforever/api/security/ElideUser.java` around lines 21 - 27, The getName() method in the ElideUser class dereferences fafAuthentication directly without a null check, creating a crash path for anonymous or non-FAF principals, while the isInRole() method has been updated with proper null-safety. Apply the same null-safe pattern to the getName() method by checking if fafAuthentication is not null before calling getName() on it, and return an appropriate default value (such as null or an empty string) if fafAuthentication is null, matching the defensive approach used in isInRole().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/faforever/api/config/security/MethodSecurityConfig.java`:
- Around line 11-16: The MethodSecurityConfig class uses `@ConditionalOnProperty`
to disable method security globally when faf-api.allow-anonymous is true, which
is overly broad and removes all `@PreAuthorize/`@Secured enforcement. Remove the
`@ConditionalOnProperty` annotation from `@EnableMethodSecurity`(securedEnabled =
true) to ensure method security is always enabled. Instead, configure anonymous
access at the request matcher or controller level through your security
configuration (such as in SecurityFilterChain or by using `@PermitAll` on specific
controller methods) so that only specific endpoints allow anonymous access while
protected endpoints remain enforced.
---
Outside diff comments:
In `@src/main/java/com/faforever/api/config/security/WebSecurityConfig.java`:
- Around line 46-52: The current security configuration permits anonymous access
to all actuator endpoints using the `/actuator/**` pattern, which is overly
permissive. Replace this broad pattern with specific patterns for health probes
only, such as `/actuator/health` and `/actuator/health/**`. Additionally,
restrict anonymous access to these health endpoints to GET requests only by
using `requestMatchers` with the `.permitAll()` method being applied only to GET
requests on health endpoints, while other actuator endpoints remain protected.
In `@src/main/java/com/faforever/api/security/ElideUser.java`:
- Around line 21-27: The getName() method in the ElideUser class dereferences
fafAuthentication directly without a null check, creating a crash path for
anonymous or non-FAF principals, while the isInRole() method has been updated
with proper null-safety. Apply the same null-safe pattern to the getName()
method by checking if fafAuthentication is not null before calling getName() on
it, and return an appropriate default value (such as null or an empty string) if
fafAuthentication is null, matching the defensive approach used in isInRole().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1025f318-8ffd-47f6-8fa3-6ff6dbb727e7
📒 Files selected for processing (5)
src/main/java/com/faforever/api/config/FafApiProperties.javasrc/main/java/com/faforever/api/config/security/MethodSecurityConfig.javasrc/main/java/com/faforever/api/config/security/WebSecurityConfig.javasrc/main/java/com/faforever/api/security/ElideUser.javasrc/main/resources/config/application-local.yml
Comment on lines
+11
to
16
| @ConditionalOnProperty( | ||
| value = "faf-api.allow-anonymous", | ||
| havingValue = "false", | ||
| matchIfMissing = true | ||
| ) | ||
| @EnableMethodSecurity(securedEnabled = true) |
There was a problem hiding this comment.
Global method-security disablement is too broad.
When faf-api.allow-anonymous=true, this removes all @PreAuthorize/@Secured enforcement, not just for health/readiness or /data/**. Keep method security enabled and scope anonymous access at request matcher/controller level instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/faforever/api/config/security/MethodSecurityConfig.java`
around lines 11 - 16, The MethodSecurityConfig class uses `@ConditionalOnProperty`
to disable method security globally when faf-api.allow-anonymous is true, which
is overly broad and removes all `@PreAuthorize/`@Secured enforcement. Remove the
`@ConditionalOnProperty` annotation from `@EnableMethodSecurity`(securedEnabled =
true) to ensure method security is always enabled. Instead, configure anonymous
access at the request matcher or controller level through your security
configuration (such as in SecurityFilterChain or by using `@PermitAll` on specific
controller methods) so that only specific endpoints allow anonymous access while
protected endpoints remain enforced.
Member
Sheikah45
force-pushed
the
security-fixes
branch
from
7a92474 to
af3a150
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/faforever/api/config/security/WebSecurityConfig.java (1)
70-72:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExposing
/data/**contradicts the stated PR objective of exposing actuator endpoints.The PR objectives specify exposing "actuator endpoints to anonymous users to enable health and readiness checks," but this implementation exposes
/data/**, which in Elide-based applications typically represents the entire JSON:API data access layer (CRUD operations on entities), not actuator endpoints.Combined with
MethodSecurityConfigbeing disabled whenallow-anonymous=true(see context snippet fromMethodSecurityConfig.java), this creates a wide-open API surface with no authentication or authorization when the flag is enabled.Expected behavior for health/readiness checks:
if (fafApiProperties.isAllowAnonymous()) { authorizeConfig.requestMatchers( "/actuator/health", "/actuator/readiness" ).permitAll(); }If the broader
/data/**exposure is intentional for the Tilt local development workflow, please:
- Add a clear comment explaining why the entire data API needs anonymous access (not just actuator endpoints)
- Update the PR description to reflect the actual scope
- Consider whether this aligns with the concern raised by Brutus5000 about unnecessary exposure beyond PR
#1138🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/faforever/api/config/security/WebSecurityConfig.java` around lines 70 - 72, The permitAll() configuration for `/data/**` in the anonymous access block exposes the entire JSON:API data layer instead of just the actuator endpoints specified in the PR objectives. Replace the `/data/**` matcher with the specific actuator endpoints `/actuator/health` and `/actuator/readiness` to align with the stated goal of enabling health and readiness checks. If the broader `/data/**` exposure is intentional for local development, add a clear code comment explaining why the entire data API requires anonymous access and verify this aligns with the security concerns raised in the PR description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/java/com/faforever/api/config/security/WebSecurityConfig.java`:
- Around line 70-72: The permitAll() configuration for `/data/**` in the
anonymous access block exposes the entire JSON:API data layer instead of just
the actuator endpoints specified in the PR objectives. Replace the `/data/**`
matcher with the specific actuator endpoints `/actuator/health` and
`/actuator/readiness` to align with the stated goal of enabling health and
readiness checks. If the broader `/data/**` exposure is intentional for local
development, add a clear code comment explaining why the entire data API
requires anonymous access and verify this aligns with the security concerns
raised in the PR description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78d21b6e-300c-444a-ac94-def7370bfe37
📒 Files selected for processing (1)
src/main/java/com/faforever/api/config/security/WebSecurityConfig.java
Sheikah45
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also add other security config changes to support development with gitops-stack tilt
Summary by CodeRabbit
Release Notes
New Features
faf-api.allow-anonymousconfiguration to control anonymous access./data/**can be accessed without authentication.Bug Fixes
Chores