MX-391: Give tenant branding its own security filter chain - #185
Open
YousufFFFF wants to merge 1 commit into
Open
MX-391: Give tenant branding its own security filter chain#185YousufFFFF wants to merge 1 commit into
YousufFFFF wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdded a dedicated tenant branding security chain for both branding URL forms. It supports tenant-aware platform authentication, anonymous reads, authenticated writes, stateless sessions, conditional CORS, and integration coverage. ChangesTenant branding security
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BrandingSecurityChain
participant TenantAwareAuthentication
participant PlatformAuthenticationProvider
Client->>BrandingSecurityChain: Request branding endpoint
BrandingSecurityChain->>TenantAwareAuthentication: Resolve tenant context
TenantAwareAuthentication->>PlatformAuthenticationProvider: Authenticate credentials
PlatformAuthenticationProvider-->>BrandingSecurityChain: Authentication result
BrandingSecurityChain-->>Client: Permit GET or authorize write
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 3
🤖 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/org/apache/fineract/branding/starter/TenantBrandingSecurityConfiguration.java`:
- Around line 84-96: Update TenantBrandingSecurityConfiguration to use
constructor injection for all collaborators currently declared in the shown
`@Autowired` fields, making those fields final and removing field injection. Add a
constructor (or generated equivalent) that accepts each dependency, preserving
the customAuthenticationProvider and basicAuthenticationEntryPoint qualifiers on
their constructor parameters and assigning every dependency to its corresponding
field.
In
`@src/test/java/org/apache/fineract/branding/security/TenantBrandingSecurityFilterChainIntegrationTest.java`:
- Around line 199-207: Update the notRejected ResultMatcher to require an HTTP
404 response for anonymous reads, failing for 400, 401, 403, 500, or any other
status instead of only rejecting 401 and 403. Preserve the existing assertion
context identifying the requested path and actual status.
- Around line 175-193: Add a happy-path test alongside anonymousWrite_isRejected
that performs authenticated PUT requests with valid platform Basic
authentication for both PREFIXED_BRANDING_PATH and BRANDING_PATH, asserting each
returns the accepted 401/403 security outcome used by this MockMvc setup. Reuse
the existing tenant header and branding payload conventions, and keep the
assertions focused on security rather than endpoint registration.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fe30507-7e18-4f7c-bb71-a14503d61669
📒 Files selected for processing (4)
src/main/java/org/apache/fineract/branding/starter/TenantBrandingSecurityConfiguration.javasrc/test/java/org/apache/fineract/branding/security/TenantBrandingSecurityFilterChainIntegrationTest.javasrc/test/java/org/apache/fineract/branding/security/TenantBrandingSecurityTestConfig.javasrc/test/java/org/apache/fineract/branding/service/TenantBrandingServiceTest.java
Comment on lines
+84
to
+96
| @Autowired | ||
| @Qualifier("customAuthenticationProvider") private DaoAuthenticationProvider platformAuthenticationProvider; | ||
|
|
||
| @Autowired | ||
| @Qualifier("basicAuthenticationEntryPoint") private BasicAuthenticationEntryPoint platformAuthenticationEntryPoint; | ||
|
|
||
| @Autowired private ToApiJsonSerializer<PlatformRequestLog> toApiJsonSerializer; | ||
| @Autowired private ConfigurationDomainService configurationDomainService; | ||
| @Autowired private CacheWritePlatformService cacheWritePlatformService; | ||
| @Autowired private UserNotificationService userNotificationService; | ||
| @Autowired private AuthTenantDetailsService basicAuthTenantDetailsService; | ||
| @Autowired private BusinessDateReadPlatformService businessDateReadPlatformService; | ||
| @Autowired private FineractProperties fineractProperties; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate target file and relevant Lombok/dependency config =="
fd -a 'TenantBrandingSecurityConfiguration\.java|pom\.xml|build\.gradle|build\.gradle\.kts' . | sed 's#^\./##'
echo
echo "== Target file outline/imports/header =="
wc -l src/main/java/org/apache/fineract/branding/starter/TenantBrandingSecurityConfiguration.java
sed -n '1,130p' src/main/java/org/apache/fineract/branding/starter/TenantBrandingSecurityConfiguration.java
echo
echo "== Lombok/config references =="
rg -n "lombok\.use\.class\.path\.scan|lombok\.add.*=|RequiredArgsConstructor|onConstructor|`@NonNull`|`@RequiredArgsConstructor`|`@AllArgsConstructor`" pom.xml .mvn mvnw* src/main/resources src/main/java/org/apache/fineract/branding/starter/TenantBrandingSecurityConfiguration.java 2>/dev/null || true
echo
echo "== Field declarations in target =="
python3 - <<'PY'
from pathlib import Path
p=Path('src/main/java/org/apache/fineract/branding/starter/TenantBrandingSecurityConfiguration.java')
text=p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if i in range(80,100) or any(s in line for s in ['Autowired', 'Qualifier', 'private ', 'requiredArgsConstructor', 'AllArgsConstructor']):
print(f'{i}: {line}')
PYRepository: openMF/selfservice-plugin
Length of output: 9815
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pom.xml Lombok dependencies and Spotless/config references =="
python3 - <<'PY'
from pathlib import Path
p=Path('pom.xml')
text=p.read_text()
lines=text.splitlines()
for i,line in enumerate(lines,1):
if any(s in line for s in [
'lombok', 'Spotless', 'spotless', 'RequiredArgsConstructor', 'AllArgsConstructor',
'annotationProcessorPaths', 'build-helper-maven-plugin', 'maven-compiler-plugin'
]):
start=max(1,i-8); end=min(len(lines),i+12)
print(f'--- around line {i} ---')
for j in range(start,end+1):
print(f'{j}: {lines[j-1]}')
print()
PY
echo "== Lombok Maven plugin config snippets =="
rg -n "lombok-version|org.projectlombok:lombok|annotationProcessorPaths|maven-compiler-plugin|spotless-java|lombok\.annotationCopy" pom.xml 2>/dev/null || true
echo "== Project Lombok plugin config directories =="
fd -a 'lombok.config|pom.xml' .mvn . 2>/dev/null | sed 's#^\./##' | head -20
fd -ia 'lombok\.config' . 2>/dev/nullRepository: openMF/selfservice-plugin
Length of output: 14625
Use constructor injection for security collaborators.
These security collaborators are still injected into mutable fields at lines 84-96. Replace them with final fields injected through a constructor or generated constructor. Preserve the @Qualifier("customAuthenticationProvider") and @Qualifier("basicAuthenticationEntryPoint") selections on the constructor parameters so the intended providers/input points are wired.
🤖 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/org/apache/fineract/branding/starter/TenantBrandingSecurityConfiguration.java`
around lines 84 - 96, Update TenantBrandingSecurityConfiguration to use
constructor injection for all collaborators currently declared in the shown
`@Autowired` fields, making those fields final and removing field injection. Add a
constructor (or generated equivalent) that accepts each dependency, preserving
the customAuthenticationProvider and basicAuthenticationEntryPoint qualifiers on
their constructor parameters and assigning every dependency to its corresponding
field.
Sources: Coding guidelines, Path instructions
Comment on lines
+175
to
+193
| /** The colour is tenant configuration: reading it is open, changing it is not. */ | ||
| @Test | ||
| @DisplayName("an anonymous write is rejected under both spellings") | ||
| void anonymousWrite_isRejected() throws Exception { | ||
| mockMvc | ||
| .perform( | ||
| put(PREFIXED_BRANDING_PATH) | ||
| .header(TENANT_HEADER, "default") | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content("{\"primaryColor\":\"green\"}")) | ||
| .andExpect(rejected(PREFIXED_BRANDING_PATH)); | ||
| mockMvc | ||
| .perform( | ||
| put(BRANDING_PATH) | ||
| .header(TENANT_HEADER, "default") | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content("{\"primaryColor\":\"green\"}")) | ||
| .andExpect(rejected(BRANDING_PATH)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/test/java/org/apache/fineract/branding/security/TenantBrandingSecurityFilterChainIntegrationTest.java"
if [ -f "$file" ]; then
echo "== file exists =="
wc -l "$file"
echo "== outline =="
ast-grep outline "$file" || true
echo "== relevant lines 1-240 =="
sed -n '1,240p' "$file" | nl -ba | sed -n '1,240p'
else
echo "missing: $file"
fd -a 'TenantBrandingSecurityFilterChainIntegrationTest.java' .
fi
echo "== search for branding path constants and write tests =="
rg -n "PREFIXED_BRANDING_PATH|BRANDING_PATH|anonymousWrite|primaryColor|Basic|authorizeHttpRequests|hasAnyRole|mockMvc|MockServer|Platform|Authentication|MockMvc" src/test/java/org/apache/fineract/branding/security src/test/java/org/apache/fineract -S || trueRepository: openMF/selfservice-plugin
Length of output: 825
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate security filter chain/source files likely controlling branding =="
rg -n "TenantBrandingSecurityFilterChain|branding|primaryColor|/branding|PREFIXED_BRANDING_PATH|BRANDING_PATH" src/main src/test -S || true
echo "== inspect candidate configuration/service files =="
python3 - <<'PY'
from pathlib import Path
files = []
for root in [Path("src/main"), Path("src/test")]:
if root.exists():
for p in root.rglob("*"):
if p.is_file() and (p.suffix in {".java"}):
txt = p.read_text(errors="ignore")
if any(s in txt for s in ["TenantBrandingSecurityFilterChain", "BrandingController", "`@PreAuthorize`", "permitAll", "authenticated", "authorizeHttpRequests", "/branding"]):
files.append(p)
for p in files[:80]:
print(f"\n--- {p} ({p.stat().st_size} bytes) ---")
lines = p.read_text(errors="ignore").splitlines()
for i,l in enumerate(lines,1):
if any(s in l for s in ["TenantBrandingSecurityFilterChain", "BrandingController", "`@PreAuthorize`", "permitAll", "authenticated", "authorizeHttpRequests"]):
start=max(1,i-8); end=min(len(lines),i+12)
print(f"[lines {start}-{end}]")
for j in range(start,end+1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: openMF/selfservice-plugin
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/test/java/org/apache/fineract/branding/security/TenantBrandingSecurityFilterChainIntegrationTest.java"
echo "== test file with line numbers =="
awk '{printf "%5d %s\n", NR, $0}' "$file"
echo "== focused branding integration tests =="
awk '{printf "%5d %s\n", NR, $0}' "$file" | sed -n '130,215p'
echo "== branding service/resource paths =="
rg -n -i "TenantBrandingSecurityFilterChain|branding|PrimaryColor|primaryColor|branded" src/main src/test -S \
--glob '*.java' \
-g '!**/SelfServiceSecurityConfiguration.java' \
--max-count 200 || trueRepository: openMF/selfservice-plugin
Length of output: 40561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact branding security references =="
rg -n -i 'resource="tenantbranding"|resourceName|`@PreAuthorize`|hasRole|hasAnyRole|tenantBrand|tenantBrand' src/main/java src/test/java --glob '*.java' || true
echo "== tenant branding API resource =="
cat -n src/main/java/org/apache/fineract/branding/api/TenantBrandingApiResource.java | sed -n '1,120p'
echo "== tenant branding service relevant methods =="
cat -n src/main/java/org/apache/fineract/branding/service/TenantBrandingService.java | sed -n '1,125p'
echo "== branding security configuration relevant sections =="
cat -n src/main/java/org/apache/fineract/branding/starter/TenantBrandingSecurityConfiguration.java | sed -n '1,220p'Repository: openMF/selfservice-plugin
Length of output: 36964
Add a valid platform-authenticated branding-write test.
Line 175 only asserts that anonymous writes are rejected. A valid platform Basic-authenticated write is required as the happy path, and it should be asserted for both /v1/branding and /api/v1/branding. In this MockMvc setup, write security is only exercised through 401/403; the rest of the endpoint is not registered with MockMvc.
🤖 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/test/java/org/apache/fineract/branding/security/TenantBrandingSecurityFilterChainIntegrationTest.java`
around lines 175 - 193, Add a happy-path test alongside
anonymousWrite_isRejected that performs authenticated PUT requests with valid
platform Basic authentication for both PREFIXED_BRANDING_PATH and BRANDING_PATH,
asserting each returns the accepted 401/403 security outcome used by this
MockMvc setup. Reuse the existing tenant header and branding payload
conventions, and keep the assertions focused on security rather than endpoint
registration.
Source: Path instructions
Comment on lines
+199
to
+207
| private static ResultMatcher notRejected(final String path) { | ||
| return result -> { | ||
| final int status = result.getResponse().getStatus(); | ||
| if (status == 401 || status == 403) { | ||
| throw new AssertionError( | ||
| "An anonymous read of " + path + " was rejected with " + status + "."); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/test/java/org/apache/fineract/branding/security/TenantBrandingSecurityFilterChainIntegrationTest.java"
if [ -f "$file" ]; then
echo "== file exists =="
wc -l "$file"
echo "== outline =="
ast-grep outline "$file" || true
echo "== relevant snippets =="
sed -n '1,260p' "$file" | cat -n
else
echo "missing $file"
echo "candidate files:"
fd -i 'TenantBrandingSecurityFilterChainIntegrationTest.java' .
fiRepository: openMF/selfservice-plugin
Length of output: 13922
Assert the expected dispatcher fall-through.
notRejected still passes 400, 404, and 500. The anonymous-read test should reject those and require 404, which is the expected response after security permits the request.
🤖 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/test/java/org/apache/fineract/branding/security/TenantBrandingSecurityFilterChainIntegrationTest.java`
around lines 199 - 207, Update the notRejected ResultMatcher to require an HTTP
404 response for anonymous reads, failing for 400, 401, 403, 500, or any other
status instead of only rejecting 401 and 403. Preserve the existing assertion
context identifying the requested path and actual status.
Source: Path instructions
IOhacker
requested changes
Aug 5, 2026
IOhacker
left a comment
IOhacker
left a comment
Contributor
There was a problem hiding this comment.
This PR is not required...
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.
Fixes MX-391
Problem
GET /v1/brandingreturns 401 for callers sending valid platform credentials.The 401 carries
WWW-Authenticate: Basic realm="Fineract Self Service API"— a realm onlySelfServiceSecurityConfigurationsets, so the self-service chain produced it, and that chain authenticates against the self-service user store where platform users don't exist. The body is Spring Boot's default error shape rather than Fineract's, so the request never reached the resource.That means removing
context.authenticatedUser()fromretrieveBranding()cannot fix this — the rejection happens in the filter chain, before the resource runs.Cause
Branding is the only resource outside
/v1/self/, so it belongs to neither chain that can claim it. Where the self-service chain claims it, platform users are rejected. Where no chain claims it, the request skips authentication and tenant resolution —TenantAwareBasicAuthenticationFilteris what sets the tenant — so every tenant silently reads the default colour.Change
A dedicated chain for branding, ordered ahead of the others:
/api/v1/brandingand/v1/branding, so protection doesn't depend on the deployment's API prefixpermitAllon GET - the colour isn't sensitive and the login screen needs it before anyone authenticatesUPDATE_CONFIGURATIONstays in the resourceThe order is on the
@Beanmethod: a class-level@Orderdoesn't reach theSecurityFilterChainbeans a configuration produces.Tests
6 new chain tests (both spellings claimed, branding chain wins first, self-service chain claims neither, anonymous read permitted, anonymous write rejected) plus one pinning the null-tenant read. No regressions in the existing self-service security suites.
Summary by CodeRabbit
New Features
/api/v1/brandingand/v1/brandingURL formats.Tests