feat: Add Multi-Tenant Support with Context Isolation, Improve Token Handling, and Update Tests #278
Conversation
WalkthroughAdds tenant-scoping: a new LoginUserContext.getTenantId() and DefaultLoginUserContext default; mapper signatures and SQL now accept and filter by tenantId; service and tests propagate tenantId into mapper calls; SSOInterceptor token extraction adjusted. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/src/main/java/com/tinyengine/it/mapper/AppMapper.java (1)
120-120: updateAppById() does not guarantee tenantId is set before execution.The method accepts an
Appobject, and the XML mapper (line 557) filters bytenant_id = #{tenantId}. However,MyMetaObjectHandler.updateFill()does not auto-filltenantIdduring UPDATE operations, only during INSERT.PageServiceImpl.setAppHomePage()(line 442) demonstrates this risk by creating an App object with onlyidandhomePageset, leavingtenantIdnull. This can cause incorrect filtering or silent UPDATE failures. Ensure all callers explicitly setApp.tenantIdbefore invokingupdateAppById().
🧹 Nitpick comments (4)
base/src/main/java/com/tinyengine/it/login/config/SSOInterceptor.java (1)
51-54: Add explicit null/empty check after token extraction (optional).While the current code safely handles invalid tokens via try-catch exception handling in
validateToken(), adding an explicit check for null or empty token after extraction improves code clarity and avoids unnecessary exception handling for obvious invalid cases.Consider adding:
String token = jwtUtil.getTokenFromRequest(authorization); String requestURI = request.getRequestURI(); log.info("Intercepting: {}, Token: {}", requestURI, token != null ? "present" : "null"); + +if (token == null || token.isEmpty()) { + log.warn("Token extraction failed or empty"); + response.sendRedirect(SSO_SERVER); + return false; +}base/src/main/java/com/tinyengine/it/service/app/impl/AppServiceImpl.java (1)
32-32: Unused import: Tenant.The
Tenantclass is imported but not used in this file.-import com.tinyengine.it.model.entity.Tenant;base/src/test/java/com/tinyengine/it/mapper/AppMapperTest.java (1)
39-44: Test correctly implements tenant-scoped queries and is ready for enabling.The test accurately retrieves
tenantIdfromLoginUserContextand passes it to the mapper method. Currently, the entire test class is@Disabled. To improve test coverage, consider:
- Enabling the test to verify tenant-scoping works in practice
- Adding test cases for tenant isolation (e.g., verify tenant A cannot access tenant B's apps)
- Testing edge cases where
tenantIdis null or invalidbase/src/main/resources/mappers/AppMapper.xml (1)
142-144: Add tenantId validation to queryAppByCondition service method or enforce it in the SQL fragment.The
AppByConditionSQL fragment makes tenantId filtering optional, and thequeryAppByConditionservice method does not validate that tenantId is provided. While the only current caller (createApp) validates tenantId before calling this method, the service method has no inherent protection. To prevent potential cross-tenant data access if this method is called from other code paths in the future, either add tenantId validation in the service method or make the SQL condition mandatory rather than optional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
base/src/main/java/com/tinyengine/it/common/context/LoginUserContext.java(1 hunks)base/src/main/java/com/tinyengine/it/login/config/SSOInterceptor.java(1 hunks)base/src/main/java/com/tinyengine/it/login/config/context/DefaultLoginUserContext.java(1 hunks)base/src/main/java/com/tinyengine/it/mapper/AppMapper.java(4 hunks)base/src/main/java/com/tinyengine/it/service/app/impl/AppServiceImpl.java(8 hunks)base/src/main/java/com/tinyengine/it/service/app/impl/AppTemplateServiceImpl.java(5 hunks)base/src/main/java/com/tinyengine/it/service/app/impl/PageServiceImpl.java(2 hunks)base/src/main/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImpl.java(3 hunks)base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java(1 hunks)base/src/main/resources/mappers/AppMapper.xml(7 hunks)base/src/test/java/com/tinyengine/it/mapper/AppMapperTest.java(2 hunks)base/src/test/java/com/tinyengine/it/service/app/impl/AppServiceImplTest.java(6 hunks)
🔇 Additional comments (12)
base/src/main/java/com/tinyengine/it/login/config/SSOInterceptor.java (1)
44-50: LGTM! Authorization header validation is correct.The check for missing or empty authorization headers with appropriate redirect behavior is properly implemented.
base/src/main/java/com/tinyengine/it/service/app/impl/AppTemplateServiceImpl.java (1)
95-96: LoginUserContext dependency added for tenant-scoping.The LoginUserContext is now injected to enable tenant-aware data access throughout the service.
base/src/main/java/com/tinyengine/it/service/app/impl/PageServiceImpl.java (2)
180-180: Tenant-scoped app retrieval added.The app lookup is now tenant-aware by passing
loginUserContext.getTenantId(). This ensures pages can only access apps within their tenant boundary.
498-498: Tenant-scoped app lookup in getAppHomePageId.Consistent with the tenant-scoping pattern applied throughout the PR.
base/src/main/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImpl.java (2)
146-147: LoginUserContext injected for tenant-aware app retrieval.Enables tenant-scoping in the v1 service implementation.
282-282: Tenant-scoped app lookup in getMetaDto.The schema generation now respects tenant boundaries by including tenantId in the app lookup.
base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java (1)
642-642: Tenant-scoped app lookup in listNew.The block listing now respects tenant boundaries when retrieving app information.
base/src/main/java/com/tinyengine/it/service/app/impl/AppServiceImpl.java (5)
79-80: LoginUserContext injected for tenant-scoping across all app operations.Enables comprehensive tenant isolation for CRUD operations.
89-89: Tenant-scoped app retrieval in queryAllApp.All apps are now filtered by tenant boundary.
112-115: Tenant-scoped pagination and total count.Both the paginated results and the total count now respect tenant boundaries, ensuring accurate pagination within the tenant context.
177-187: Tenant-scoped queries in updateAppById.Both the pre-update query (line 177) and post-update result retrieval (line 187) are tenant-scoped, ensuring the update operation stays within tenant boundaries.
158-159: Tenant-scoped deletion properly enforces cross-tenant isolation.Verification confirms the mapper implementation correctly handles tenant filtering in both operations:
- Line 158 (
queryAppById): The query includesAND A.tenant_id = #{tenantId}in the WHERE clause, ensuring only apps belonging to the specified tenant are retrieved.- Line 159 (
deleteAppById): The delete statement usesWHERE id = #{id} AND tenant_id = #{tenantId}, requiring both the app ID and tenant ID to match before deletion.The defense-in-depth approach is properly implemented. Both database operations enforce tenant isolation at the SQL layer.
base/src/main/java/com/tinyengine/it/common/context/LoginUserContext.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/login/config/context/DefaultLoginUserContext.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/login/config/context/DefaultLoginUserContext.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/service/app/impl/AppTemplateServiceImpl.java
Show resolved
Hide resolved
base/src/test/java/com/tinyengine/it/service/app/impl/AppServiceImplTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/src/main/resources/mappers/AppMapper.xml (1)
529-536: Enforce tenant context inqueryAppByCondition.This method lacks tenant isolation protection. While
createAppsetstenantIdbefore calling it, the public service method doesn't enforce tenant context, unlike other queries (deleteAppById,queryAppById,queryAllApp). TheAppByConditionfragment also omits tenant filtering present in other similar queries. Add tenant enforcement either by:
- Injecting
loginUserContext.getTenantId()into the App object before mapper call, or- Adding explicit tenant check to the
AppByConditionSQL fragment
♻️ Duplicate comments (2)
base/src/main/resources/mappers/AppMapper.xml (2)
458-460: Inconsistent tenant filtering creates security risk.This conditional tenant check allows cross-tenant data access when
tenantIdis null. Should be mandatory likequeryAllAppon line 433.
494-496: Same inconsistent tenant filtering issue.Apply mandatory tenant filtering here as well, consistent with other queries.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base/src/main/resources/mappers/AppMapper.xml(7 hunks)
🔇 Additional comments (5)
base/src/main/resources/mappers/AppMapper.xml (5)
429-435: LGTM - Mandatory tenant filtering correctly enforced.The
queryAllAppquery properly enforces tenant isolation with an unconditionalAND A.tenant_id = #{tenantId}clause.
509-516: LGTM - Proper tenant isolation for single-record lookup.The
queryAppByIdcorrectly enforces mandatory tenant filtering alongside the primary key lookup.
518-527: LGTM - Mandatory tenant filtering correctly applied.The
queryAppTemplateByIdproperly enforces tenant isolation.
538-543: LGTM - Critical tenant check on DELETE operation.The
deleteAppByIdcorrectly prevents cross-tenant record deletion by enforcing mandatorytenant_idfiltering.
545-553: LGTM - Critical tenant check on UPDATE operation.The
updateAppByIdcorrectly prevents cross-tenant record modification by enforcing mandatorytenant_idfiltering.
hexqi
changed the title
2 participants
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.