<feature>[identity]: add generic external tenant resource isolation framework#3496
Open
MatheMatrix wants to merge 1 commit intofeature-zcf-v0.1from
Open
<feature>[identity]: add generic external tenant resource isolation framework#3496MatheMatrix wants to merge 1 commit intofeature-zcf-v0.1from
MatheMatrix wants to merge 1 commit intofeature-zcf-v0.1from
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough新增外部租户追踪：添加数据库表、线程级外部租户上下文与 SPI、实体与 Inventory、资源所有权通知点、跟踪器、ZQL 过滤扩展及 REST 头解析与会话挂载，实现资源与外部租户的关联持久化与查询约束。（≤50字） Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Rest as RestServer
participant Provider as ExternalTenantProvider
participant Session as SessionInventory
participant Tracker as ExternalTenantResourceTracker
participant DB as Database
Client->>Rest: API 请求（含 X-Tenant-Source / X-Tenant-Id [/ X-Tenant-User]）
activate Rest
Rest->>Provider: validateTenant(ctx)
activate Provider
Provider-->>Rest: 验证结果
deactivate Provider
Rest->>Session: setExternalTenantContext(ctx)
Rest->>Tracker: 后续处理可访问线程上下文
Rest->>DB: 执行业务（可能创建资源）
DB-->>OwnedByAccountAspectHelper: 持久化 AccountResourceRefVO 完成
OwnedByAccountAspectHelper->>Tracker: notifyResourceOwnershipCreated(ref)
Tracker->>Tracker: 读取线程级 ExternalTenantContext 并决定是否持久化外部引用
Tracker->>DB: 保存 ExternalTenantResourceRefVO（如适用）
DB-->>Tracker: 确认
Tracker->>Provider: onResourceBound(ctx, resourceUuid, resourceType)
Provider-->>Tracker: 回调完成
Rest-->>Client: 返回响应（线程上下文/会话清理：实现中未见显式 finally 清理）
deactivate Rest
sequenceDiagram
participant AccountMgr as AccountManager
participant DB as Database
participant OwnedAspect as OwnedByAccountAspectHelper
participant Tracker as ExternalTenantResourceTracker
participant Provider as ExternalTenantProvider
AccountMgr->>DB: 创建 AccountResourceRefVO
activate DB
DB-->>OwnedAspect: 持久化完成回调
deactivate DB
OwnedAspect->>Tracker: notifyResourceOwnershipCreated(ref)
activate Tracker
Tracker->>Provider: 检查 provider 与 shouldTrackResource(type)
activate Provider
Provider-->>Tracker: 可跟踪/忽略
deactivate Provider
alt 可跟踪
Tracker->>DB: 保存 ExternalTenantResourceRefVO
activate DB
DB-->>Tracker: 确认
deactivate DB
Tracker->>Provider: onResourceBound(ctx, resourceUuid, type)
activate Provider
Provider-->>Tracker: 回调完成
deactivate Provider
end
Tracker-->>AccountMgr: 完成
deactivate Tracker
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 10
🧹 Nitpick comments (6)
identity/src/main/java/org/zstack/identity/AccountManagerImpl.java (1)
32-32: 建议移除重复导入，避免冗余。
ForEachFunction已在 Line 47 导入，Line 32 的同名导入是重复声明，建议保留一处即可。♻️ 建议修改
-import org.zstack.utils.function.ForEachFunction;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/AccountManagerImpl.java` at line 32, 在 AccountManagerImpl 中发现重复导入 ForEachFunction（在 import org.zstack.utils.function.ForEachFunction; 出现两次），删除多余的重复 import 保留一处即可；定位到类 AccountManagerImpl 并移除重复的 ForEachFunction 导入声明，确保只保留单一 import 导入语句以消除冗余编译警告。header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.java (1)
30-36: 可选：使用 Stream API 简化代码。使用 Stream 简化 valueOf 方法
public static List<ExternalTenantResourceRefInventory> valueOf(Collection<ExternalTenantResourceRefVO> vos) { - List<ExternalTenantResourceRefInventory> invs = new ArrayList<>(); - for (ExternalTenantResourceRefVO vo : vos) { - invs.add(valueOf(vo)); - } - return invs; + return vos.stream() + .map(ExternalTenantResourceRefInventory::valueOf) + .collect(Collectors.toList()); }需要额外导入
java.util.stream.Collectors。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.java` around lines 30 - 36, The valueOf(Collection<ExternalTenantResourceRefVO> vos) method in ExternalTenantResourceRefInventory can be simplified using Stream API: replace the manual loop that calls valueOf(vo) for each ExternalTenantResourceRefVO with a stream map and collect to a List (using java.util.stream.Collectors), keeping the existing static valueOf(ExternalTenantResourceRefVO) as the mapper; ensure you import Collectors and return the collected List.rest/src/main/java/org/zstack/rest/RestServer.java (2)
997-1001: 代码缩进不一致。
try块内的代码缩进不正确，if (APIQueryMessage.class.isAssignableFrom(api.apiClass))语句应该相对于try有额外的缩进层级。建议的修复
try { - if (APIQueryMessage.class.isAssignableFrom(api.apiClass)) { - handleQueryApi(api, session, req, rsp); - return; - } + if (APIQueryMessage.class.isAssignableFrom(api.apiClass)) { + handleQueryApi(api, session, req, rsp); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 997 - 1001, The code inside the try block has incorrect indentation: align the if statement and its body to be nested one level inside the try so that "if (APIQueryMessage.class.isAssignableFrom(api.apiClass))" and the subsequent "handleQueryApi(api, session, req, rsp); return;" are indented consistently within the try block; adjust the indentation around the try block in RestServer.java to make the nesting clear and match surrounding style.
1393-1395: 外部租户提供者注册逻辑正确。与
RestAuthenticationBackend的注册模式保持一致。考虑添加重复注册检查以便于调试。可选：添加重复注册检查
for (ExternalTenantProvider p : pluginRgty.getExtensionList(ExternalTenantProvider.class)) { + ExternalTenantProvider old = externalTenantProviders.get(p.getSource()); + if (old != null) { + throw new CloudRuntimeException(String.format("duplicate ExternalTenantProvider[%s, %s] for source[%s]", + p.getClass().getName(), old.getClass().getName(), p.getSource())); + } externalTenantProviders.put(p.getSource(), p); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 1393 - 1395, 当前在 RestServer 的注册循环直接用 externalTenantProviders.put(p.getSource(), p) 覆盖同 key 的旧值，建议在遍历 pluginRgty.getExtensionList(ExternalTenantProvider.class) 时先检查 externalTenantProviders 是否已包含该源：通过 p.getSource() 做重复键检测（或使用 containsKey/get），如果已存在则记录警告日志或抛出明确异常以便调试，否则再 put 进入 externalTenantProviders；参考 RestAuthenticationBackend 的注册模式实现相同的重复注册保护。identity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.java (1)
63-71: 建议：考虑更健壮的参数化方式。当前使用字符串拼接生成 SQL 子查询，虽然
escapeSql提供了基本的转义保护，但如果 ZQL 框架支持，使用参数化查询会更安全。此外，第 63 行的注释需要翻译为英文。As per coding guidelines:
**/*.*: 代码里不应当有有中文🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.java` around lines 63 - 71, The SQL subquery in ExternalTenantZQLExtension is built via string concatenation using escapeSql for tenantCtx.getSource()/getTenantId(), which is brittle; change it to use the ZQL framework's parameterized binding (or prepared statement placeholders) instead of injecting escaped strings—replace the String.format construction that uses inventoryAlias and primaryKey with a parameterized query and bind values for source and tenantId (still using escape/validation as needed), and update the Chinese comment on line 63 to an English comment describing "generate subquery to filter associated resources by source + tenantId".identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java (1)
68-77: 建议：提取重复代码到辅助方法。
notifyResourceOwnershipCreated（第 68-77 行）和afterResourceOwnershipCreated（第 97-106 行）中创建ExternalTenantResourceRefVO的逻辑高度重复，建议提取为私有辅助方法以符合 DRY 原则。♻️ 建议重构
private void bindResourceToExternalTenant(ExternalTenantContext ctx, ExternalTenantProvider provider, AccountResourceRefVO ref) { ExternalTenantResourceRefVO extRef = new ExternalTenantResourceRefVO(); extRef.setSource(ctx.getSource()); extRef.setTenantId(ctx.getTenantId()); extRef.setUserId(ctx.getUserId()); extRef.setResourceUuid(ref.getResourceUuid()); extRef.setResourceType(ref.getResourceType()); extRef.setAccountUuid(ref.getAccountUuid()); dbf.persist(extRef); provider.onResourceBound(ctx, ref.getResourceUuid(), ref.getResourceType()); }然后在两个方法中调用：
bindResourceToExternalTenant(ctx, provider, ref);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java` around lines 68 - 77, 提取重复的 ExternalTenantResourceRefVO 构建及持久化逻辑到一个私有辅助方法（比如 bindResourceToExternalTenant）并在 notifyResourceOwnershipCreated 和 afterResourceOwnershipCreated 中调用该方法：把 extRef 的字段赋值（使用 ctx.getSource()/getTenantId()/getUserId()，以及 ref.getResourceUuid()/getResourceType()/getAccountUuid()）、dbf.persist(extRef) 以及 provider.onResourceBound(ctx, ref.getResourceUuid(), ref.getResourceType()) 都迁移到该辅助方法，保留原方法签名并替换重复代码为对 bindResourceToExternalTenant(ctx, provider, ref) 的调用以满足 DRY 原则。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@conf/db/upgrade/V5.5.1__schema.sql`:
- Around line 3-8: Replace the Chinese COMMENT texts on the columns `source`,
`tenantId`, `userId`, `resourceUuid`, `resourceType`, and `accountUuid` with
English descriptions; locate the column definitions in the migration that
declare these fields and update each COMMENT string to an appropriate English
phrase (e.g., "source service identifier", "external tenant id", "external user
id (optional)", "resource UUID", "resource type (VO SimpleName)", "associated
ZStack account") ensuring wording is clear and conforms to coding standards.
In
`@header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java`:
- Around line 11-17: Replace Chinese comments in OwnedByAccountAspectHelper
(including the static field notifier and setResourceOwnershipCreationNotifier)
with clear English comments, and remove any silent exception swallowing in the
surrounding methods (lines referenced around 30-46): catch the exception and log
it using the project's logger with contextual message rather than ignoring it so
failures are observable; ensure setResourceOwnershipCreationNotifier still
assigns OwnedByAccountAspectHelper.notifier correctly and that any exceptions
thrown during notifier callbacks are caught, logged, and do not silently fail.
In `@header/src/main/java/org/zstack/header/identity/ExternalTenantContext.java`:
- Around line 5-31: Translate all Chinese comments and Javadoc in
ExternalTenantContext to English: update the class Javadoc (lines describing
purpose and usage), the inline ThreadLocal comment (explaining it's used to pass
per-request external tenant context set by RestServer and cleared after
request), and the field comments for source, tenantId, and userId; keep the same
semantics and reference the class name ExternalTenantContext, the ThreadLocal
current, and the methods setCurrent/getCurrent/clearCurrent so reviewers can
locate the changed comments.
In `@header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java`:
- Around line 3-51: Translate all Chinese Javadoc and comments in the
ExternalTenantProvider interface to clear, idiomatic English: update the
class/interface-level comment and the Javadocs for getSource(),
validateTenant(ExternalTenantContext), shouldTrackResource(String), and
onResourceBound(ExternalTenantContext, String, String) so they describe the
purpose, parameters, return values and behavior in English only (no Chinese),
preserving existing semantics and referenced types such as ExternalTenantContext
and ExternalTenantResourceRefVO; keep signatures and default implementations
unchanged and ensure spelling/grammar are correct.
In
`@header/src/main/java/org/zstack/header/identity/ResourceOwnershipCreatedExtensionPoint.java`:
- Around line 3-15: Replace Chinese Javadoc comments in the
ResourceOwnershipCreatedExtensionPoint interface with clear, correct English:
update the class-level comment to describe the extension point purpose (invoked
after AccountResourceRefVO is created by AccountManagerImpl to perform
post-ownership actions such as external tenant association) and update the
method-level comment for afterResourceOwnershipCreated(AccountResourceRefVO ref,
SessionInventory session) to explain parameters (ref is the newly created
ownership record, session is the current request Session, may be null); ensure
proper grammar and spelling and keep the JavaDoc tags (`@param`) intact for both
params.
In
`@identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java`:
- Around line 80-109: The file contains Chinese inline section comments in
ExternalTenantResourceTracker—translate the two Chinese comments "// --- 资源创建关联
---" and "// --- 资源删除清理 ---" to English; update them to descriptive English
phrases (e.g. "// --- resource creation binding ---" and "// --- resource
deletion cleanup ---") in the class around the afterResourceOwnershipCreated
method to comply with the codebase guideline banning Chinese comments.
- Around line 38-53: The file contains inline comments in Chinese around the AOP
resource-creation notifier (e.g., comments near
OwnedByAccountAspectHelper.setResourceOwnershipCreationNotifier(...) in the
start/stop methods and above
notifyResourceOwnershipCreated(AccountResourceRefVO)), so replace those Chinese
comments with concise English equivalents describing the intent (e.g., "Set
AOP-level notifier to avoid circular dependency", "Clear AOP-level notifier on
stop", "AOP cannot obtain session via method param; read from ThreadLocal"),
ensuring the comments remain accurate and placed next to the same symbols
(setResourceOwnershipCreationNotifier, notifyResourceOwnershipCreated) with no
other code changes.
In `@identity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.java`:
- Around line 13-23: The class-level Javadoc for ExternalTenantZQLExtension
(including the descriptive paragraph and the SQL example) is written in Chinese
and must be rewritten in proper English; update the comment block above the
ExternalTenantZQLExtension class to an English Javadoc that describes the
two-phase behavior (marshalZQLASTTree() inserts a placeholder RestrictExpr and
restrictByExpr() expands it into a SQL subquery) and translates the example SQL
filter into English while keeping the same technical details and correct
spelling.
- Around line 74-84: The Javadoc above the escapeSql method is written in
Chinese; please replace it with an English comment that conveys the same meaning
(simple SQL escaping to prevent injection and that external tenant info is
already validated by Provider.validateTenant() in RestServer so this is a
secondary safeguard). Update the comment block for the private static String
escapeSql(String value) method to be an English Javadoc describing purpose and
behavior (null -> empty string and replacements for single quote and backslash)
while keeping the existing method name and logic unchanged.
In `@rest/src/main/java/org/zstack/rest/RestServer.java`:
- Line 968: Replace the Chinese comment in the RestServer class with an English
comment; specifically change the inline comment that currently reads "解析外部租户
Header（AK 和 OAuth 两条路径都支持）" to a clear English equivalent (e.g., "Parse external
tenant header (supports both AK and OAuth paths)"), ensuring spelling and
grammar are correct and consistent with project coding standards.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.java`:
- Around line 30-36: The valueOf(Collection<ExternalTenantResourceRefVO> vos)
method in ExternalTenantResourceRefInventory can be simplified using Stream API:
replace the manual loop that calls valueOf(vo) for each
ExternalTenantResourceRefVO with a stream map and collect to a List (using
java.util.stream.Collectors), keeping the existing static
valueOf(ExternalTenantResourceRefVO) as the mapper; ensure you import Collectors
and return the collected List.
In `@identity/src/main/java/org/zstack/identity/AccountManagerImpl.java`:
- Line 32: 在 AccountManagerImpl 中发现重复导入 ForEachFunction（在 import
org.zstack.utils.function.ForEachFunction; 出现两次），删除多余的重复 import 保留一处即可；定位到类
AccountManagerImpl 并移除重复的 ForEachFunction 导入声明，确保只保留单一 import 导入语句以消除冗余编译警告。
In
`@identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java`:
- Around line 68-77: 提取重复的 ExternalTenantResourceRefVO 构建及持久化逻辑到一个私有辅助方法（比如
bindResourceToExternalTenant）并在 notifyResourceOwnershipCreated 和
afterResourceOwnershipCreated 中调用该方法：把 extRef 的字段赋值（使用
ctx.getSource()/getTenantId()/getUserId()，以及
ref.getResourceUuid()/getResourceType()/getAccountUuid()）、dbf.persist(extRef) 以及
provider.onResourceBound(ctx, ref.getResourceUuid(), ref.getResourceType())
都迁移到该辅助方法，保留原方法签名并替换重复代码为对 bindResourceToExternalTenant(ctx, provider, ref)
的调用以满足 DRY 原则。
In `@identity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.java`:
- Around line 63-71: The SQL subquery in ExternalTenantZQLExtension is built via
string concatenation using escapeSql for tenantCtx.getSource()/getTenantId(),
which is brittle; change it to use the ZQL framework's parameterized binding (or
prepared statement placeholders) instead of injecting escaped strings—replace
the String.format construction that uses inventoryAlias and primaryKey with a
parameterized query and bind values for source and tenantId (still using
escape/validation as needed), and update the Chinese comment on line 63 to an
English comment describing "generate subquery to filter associated resources by
source + tenantId".
In `@rest/src/main/java/org/zstack/rest/RestServer.java`:
- Around line 997-1001: The code inside the try block has incorrect indentation:
align the if statement and its body to be nested one level inside the try so
that "if (APIQueryMessage.class.isAssignableFrom(api.apiClass))" and the
subsequent "handleQueryApi(api, session, req, rsp); return;" are indented
consistently within the try block; adjust the indentation around the try block
in RestServer.java to make the nesting clear and match surrounding style.
- Around line 1393-1395: 当前在 RestServer 的注册循环直接用
externalTenantProviders.put(p.getSource(), p) 覆盖同 key 的旧值，建议在遍历
pluginRgty.getExtensionList(ExternalTenantProvider.class) 时先检查
externalTenantProviders 是否已包含该源：通过 p.getSource() 做重复键检测（或使用
containsKey/get），如果已存在则记录警告日志或抛出明确异常以便调试，否则再 put 进入 externalTenantProviders；参考
RestAuthenticationBackend 的注册模式实现相同的重复注册保护。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 47296bf2-eaf6-4f7e-862e-4836dd4e5e99
⛔ Files ignored due to path filters (1)
conf/springConfigXml/AccountManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (14)
conf/db/upgrade/V5.5.1__schema.sqlheader/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantContext.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantProvider.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO_.javaheader/src/main/java/org/zstack/header/identity/ResourceOwnershipCreatedExtensionPoint.javaheader/src/main/java/org/zstack/header/identity/SessionInventory.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javaidentity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.javaidentity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.javarest/src/main/java/org/zstack/rest/RestConstants.javarest/src/main/java/org/zstack/rest/RestServer.java
header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java
Show resolved
Hide resolved
header/src/main/java/org/zstack/header/identity/ExternalTenantContext.java
Outdated
Show resolved
Hide resolved
header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java
Show resolved
Hide resolved
header/src/main/java/org/zstack/header/identity/ResourceOwnershipCreatedExtensionPoint.java
Outdated
Show resolved
Hide resolved
identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java
Outdated
Show resolved
Hide resolved
identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java
Outdated
Show resolved
Hide resolved
identity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.java
Show resolved
Hide resolved
identity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.java
Show resolved
Hide resolved
MatheMatrix
force-pushed
the
sync/hanyu.liang/zcf-1147@@2
branch
2 times, most recently
from
ab4c5c7 to
6149274
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java (1)
31-37:⚠️ Potential issue | 🟠 Major不要在这里静默忽略 notifier 失败。
AccountResourceRefVO已经写入当前事务后，如果notifyResourceOwnershipCreated()失败却被吞掉，主流程仍会继续成功，外部租户绑定就可能永久缺失。后续查询/清理会出现“资源存在，但隔离关系丢失”的不一致。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java` around lines 31 - 37, The catch block that silently ignores exceptions from notifier.notifyResourceOwnershipCreated(ref) must be changed so notifier failures are not swallowed: log the exception (using existing logger) and rethrow it (wrap if needed in a RuntimeException) so the surrounding transaction in OwnedByAccountAspectHelper fails and the AccountResourceRefVO insert is rolled back; ensure the notifyResourceOwnershipCreated call remains inside the try but do not absorb exceptions — propagate them after logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java`:
- Around line 19-26: The validateTenant contract is ambiguous about which
exception to throw; update
ExternalTenantProvider.validateTenant(ExternalTenantContext) to require throwing
RestException for validation failures (either by adding "throws RestException"
to the method signature or by clearly documenting in the Javadoc that
implementations must throw RestException with an appropriate 4xx status such as
HttpStatus.BAD_REQUEST.value()), and update the Javadoc to state this
requirement and preferred HTTP status; ensure all implementations of
validateTenant are adjusted to throw RestException rather than raw
RuntimeException so the RestServer maps failures to client 4xx responses.
In `@header/src/main/java/org/zstack/header/identity/SessionInventory.java`:
- Around line 106-108: The current hasExternalTenant() check is too permissive
because it only verifies externalTenantContext.getTenantId(); update
hasExternalTenant() to require externalTenantContext != null and both
externalTenantContext.getSource() and externalTenantContext.getTenantId() are
non-null and non-blank (trimmed) so a half-populated ExternalTenantContext won't
be treated as a valid external tenant; use a trimmed emptiness check (or
StringUtils.hasText if available) on getSource() and getTenantId() when
modifying hasExternalTenant().
In
`@identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java`:
- Around line 52-77: The notifyResourceOwnershipCreated method (and the similar
block at 82-107) can insert duplicate ExternalTenantResourceRefVO and call
provider.onResourceBound twice; make the operation idempotent by enforcing a
uniqueness key on ExternalTenantResourceRefVO (source + tenantId + resourceUuid)
and changing the logic in notifyResourceOwnershipCreated to first check for an
existing ref (or perform an upsert/insert-ignore) and only call
provider.onResourceBound when the insert actually created a new row;
additionally handle race conditions by relying on the DB unique constraint and
catching duplicate-key/DataIntegrity exceptions from dbf.persist (or using a
transactional upsert) so duplicate attempts do not re-invoke
provider.onResourceBound.
- Around line 34-36: The loop that calls providers.put(p.getSource(), p) on
pluginRgty.getExtensionList(ExternalTenantProvider.class) silently overwrites
providers when two ExternalTenantProvider instances share the same getSource(),
so add a uniqueness check and fail-fast: iterate the extension list, for each
ExternalTenantProvider p check if providers.containsKey(p.getSource()) and if so
throw an exception (or log error and stop startup) identifying the duplicate
source and both provider classes; otherwise put the provider into providers.
This ensures duplicate getSource() values are detected at startup instead of
silently overwriting implementations.
In `@rest/src/main/java/org/zstack/rest/RestServer.java`:
- Around line 1393-1395: When registering ExternalTenantProvider implementations
in RestServer, do not silently overwrite duplicates: check
ExternalTenantProvider.getSource() against the externalTenantProviders map (used
with pluginRgty.getExtensionList(ExternalTenantProvider.class)) and if a source
already exists, reject the second provider (e.g., throw an IllegalStateException
or log an error and abort startup) so duplicate sources cannot replace the
previously-registered provider; include the conflicting source and both provider
class names in the error to aid debugging.
- Around line 968-995: The current logic only enables external-tenant handling
when both tenantSource and tenantId are present, which allows silent bypass if
one header is omitted; change RestServer to perform all-or-nothing validation:
if any of the related headers (RestConstants.HEADER_TENANT_SOURCE,
RestConstants.HEADER_TENANT_ID, RestConstants.HEADER_TENANT_USER) is present,
require tenantSource and tenantId to be non-null and non-blank after trim
(otherwise throw RestException with BAD_REQUEST), and also reject tenantUser if
present but blank; after these checks proceed to lookup ExternalTenantProvider
by tenantSource, call provider.validateTenant(ctx), set
session.setExternalTenantContext(ctx) and ExternalTenantContext.setCurrent(ctx)
as before.
---
Duplicate comments:
In
`@header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java`:
- Around line 31-37: The catch block that silently ignores exceptions from
notifier.notifyResourceOwnershipCreated(ref) must be changed so notifier
failures are not swallowed: log the exception (using existing logger) and
rethrow it (wrap if needed in a RuntimeException) so the surrounding transaction
in OwnedByAccountAspectHelper fails and the AccountResourceRefVO insert is
rolled back; ensure the notifyResourceOwnershipCreated call remains inside the
try but do not absorb exceptions — propagate them after logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 09311c3f-e0a2-4f06-8862-16cf5a360ec7
⛔ Files ignored due to path filters (1)
conf/springConfigXml/AccountManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (14)
conf/db/upgrade/V5.5.1__schema.sqlheader/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantContext.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantProvider.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO_.javaheader/src/main/java/org/zstack/header/identity/ResourceOwnershipCreatedExtensionPoint.javaheader/src/main/java/org/zstack/header/identity/SessionInventory.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javaidentity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.javaidentity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.javarest/src/main/java/org/zstack/rest/RestConstants.javarest/src/main/java/org/zstack/rest/RestServer.java
🚧 Files skipped from review as they are similar to previous changes (5)
- header/src/main/java/org/zstack/header/identity/ResourceOwnershipCreatedExtensionPoint.java
- header/src/main/java/org/zstack/header/identity/ExternalTenantContext.java
- identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
- header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO_.java
- conf/db/upgrade/V5.5.1__schema.sql
header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java
Show resolved
Hide resolved
identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java
Show resolved
Hide resolved
Comment on lines
+52
to
+77
| public void notifyResourceOwnershipCreated(AccountResourceRefVO ref) { | ||
| // AOP level cannot obtain session through method parameters, read from ThreadLocal | ||
| ExternalTenantContext ctx = ExternalTenantContext.getCurrent(); | ||
| if (ctx == null || ctx.getTenantId() == null) { | ||
| return; | ||
| } | ||
|
|
||
| ExternalTenantProvider provider = providers.get(ctx.getSource()); | ||
| if (provider == null) { | ||
| return; | ||
| } | ||
|
|
||
| if (!provider.shouldTrackResource(ref.getResourceType())) { | ||
| return; | ||
| } | ||
|
|
||
| ExternalTenantResourceRefVO extRef = new ExternalTenantResourceRefVO(); | ||
| extRef.setSource(ctx.getSource()); | ||
| extRef.setTenantId(ctx.getTenantId()); | ||
| extRef.setUserId(ctx.getUserId()); | ||
| extRef.setResourceUuid(ref.getResourceUuid()); | ||
| extRef.setResourceType(ref.getResourceType()); | ||
| extRef.setAccountUuid(ref.getAccountUuid()); | ||
| dbf.persist(extRef); | ||
|
|
||
| provider.onResourceBound(ctx, ref.getResourceUuid(), ref.getResourceType()); |
There was a problem hiding this comment.
资源绑定缺少幂等保护，可能重复落库并重复触发外部回调
两条路径都执行了相同的 persist + provider.onResourceBound(...)。如果同一资源在两个扩展路径都被触发，会出现重复 ExternalTenantResourceRefVO 及重复外部副作用。建议增加幂等策略（如 source + tenantId + resourceUuid 唯一约束/写前检查），并确保仅在首次成功绑定时调用 onResourceBound。
Also applies to: 82-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java`
around lines 52 - 77, The notifyResourceOwnershipCreated method (and the similar
block at 82-107) can insert duplicate ExternalTenantResourceRefVO and call
provider.onResourceBound twice; make the operation idempotent by enforcing a
uniqueness key on ExternalTenantResourceRefVO (source + tenantId + resourceUuid)
and changing the logic in notifyResourceOwnershipCreated to first check for an
existing ref (or perform an upsert/insert-ignore) and only call
provider.onResourceBound when the insert actually created a new row;
additionally handle race conditions by relying on the DB unique constraint and
catching duplicate-key/DataIntegrity exceptions from dbf.persist (or using a
transactional upsert) so duplicate attempts do not re-invoke
provider.onResourceBound.
Comment on lines
+1393
to
+1395
| for (ExternalTenantProvider p : pluginRgty.getExtensionList(ExternalTenantProvider.class)) { | ||
| externalTenantProviders.put(p.getSource(), p); | ||
| } |
There was a problem hiding this comment.
注册 provider 时应拒绝重复 source。
ExternalTenantProvider#getSource() 的约定是全局唯一，但这里直接 put() 会把先注册的实现静默覆盖。两个插件一旦重名，请求就会被路由到错误的 provider，租户校验和资源跟踪都会错位。
建议修改
for (ExternalTenantProvider p : pluginRgty.getExtensionList(ExternalTenantProvider.class)) {
- externalTenantProviders.put(p.getSource(), p);
+ String source = p.getSource() == null ? null : p.getSource().trim();
+ if (source == null || source.isEmpty()) {
+ throw new CloudRuntimeException(String.format(
+ "ExternalTenantProvider[%s] returned an empty source", p.getClass().getName()));
+ }
+
+ ExternalTenantProvider old = externalTenantProviders.putIfAbsent(source, p);
+ if (old != null) {
+ throw new CloudRuntimeException(String.format(
+ "duplicate ExternalTenantProvider[%s, %s] for source[%s]",
+ p.getClass().getName(), old.getClass().getName(), source));
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 1393 - 1395,
When registering ExternalTenantProvider implementations in RestServer, do not
silently overwrite duplicates: check ExternalTenantProvider.getSource() against
the externalTenantProviders map (used with
pluginRgty.getExtensionList(ExternalTenantProvider.class)) and if a source
already exists, reject the second provider (e.g., throw an IllegalStateException
or log an error and abort startup) so duplicate sources cannot replace the
previously-registered provider; include the conflicting source and both provider
class names in the error to aid debugging.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
rest/src/main/java/org/zstack/rest/RestServer.java (2)
1393-1395:⚠️ Potential issue | 🟠 Major注册
ExternalTenantProvider时应拒绝重复source，避免静默覆盖。当前
put()会让后注册实现覆盖先注册实现，可能将请求路由到错误 provider。建议对source做 trim + 非空校验，并在重复时直接失败。♻️ 建议修改
for (ExternalTenantProvider p : pluginRgty.getExtensionList(ExternalTenantProvider.class)) { - externalTenantProviders.put(p.getSource(), p); + String source = p.getSource() == null ? null : p.getSource().trim(); + if (source == null || source.isEmpty()) { + throw new CloudRuntimeException(String.format( + "ExternalTenantProvider[%s] returned an empty source", p.getClass().getName())); + } + + ExternalTenantProvider old = externalTenantProviders.putIfAbsent(source, p); + if (old != null) { + throw new CloudRuntimeException(String.format( + "duplicate ExternalTenantProvider[%s, %s] for source[%s]", + p.getClass().getName(), old.getClass().getName(), source)); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 1393 - 1395, ExternalTenantProvider registration silently overwrites earlier providers; validate and reject duplicates by trimming and checking non-empty source and failing on duplicates. In the loop over pluginRgty.getExtensionList(ExternalTenantProvider.class), call p.getSource(), trim it and ensure it's not null/empty, then check externalTenantProviders.containsKey(source) and if present throw an IllegalStateException (or a runtime exception appropriate for startup) instead of calling externalTenantProviders.put; otherwise put the entry. Include the failing source in the exception message to aid diagnosis.
968-995:⚠️ Potential issue | 🟠 Major外部租户头缺少 all-or-nothing 校验，存在静默绕过路径。
当前只在
tenantSource != null && tenantId != null时进入隔离逻辑；只传其中一个头（或传空白值）会回退到普通流程，既不校验 provider，也不挂载外部租户上下文。建议：任一相关头出现时，强制source与tenantId均为非空白；tenantUser若提供也应拒绝空白。♻️ 建议修改
if (session != null) { String tenantSource = req.getHeader(RestConstants.HEADER_TENANT_SOURCE); String tenantId = req.getHeader(RestConstants.HEADER_TENANT_ID); String tenantUser = req.getHeader(RestConstants.HEADER_TENANT_USER); - if (tenantSource != null && tenantId != null) { + boolean hasSourceHeader = tenantSource != null; + boolean hasTenantIdHeader = tenantId != null; + boolean hasTenantUserHeader = tenantUser != null; + boolean hasAnyTenantHeader = hasSourceHeader || hasTenantIdHeader || hasTenantUserHeader; + + if (hasAnyTenantHeader) { + boolean hasTenantSource = tenantSource != null && !tenantSource.trim().isEmpty(); + boolean hasTenantId = tenantId != null && !tenantId.trim().isEmpty(); + if (!hasTenantSource || !hasTenantId) { + throw new RestException(HttpStatus.BAD_REQUEST.value(), + String.format("headers[%s] and [%s] must be provided together and be non-blank", + RestConstants.HEADER_TENANT_SOURCE, RestConstants.HEADER_TENANT_ID)); + } + if (hasTenantUserHeader && tenantUser.trim().isEmpty()) { + throw new RestException(HttpStatus.BAD_REQUEST.value(), + String.format("header[%s] cannot be blank", RestConstants.HEADER_TENANT_USER)); + } + tenantSource = tenantSource.trim(); tenantId = tenantId.trim(); ExternalTenantProvider provider = externalTenantProviders.get(tenantSource); if (provider == null) {header/src/main/java/org/zstack/header/identity/SessionInventory.java (1)
106-108:⚠️ Potential issue | 🟠 Major
hasExternalTenant()判定过宽，可能误判为“有外部租户”。当前只检查
tenantId非空；当source缺失或空白时，调用方仍会得到true，容易导致后续隔离/跟踪行为不一致。建议同时要求source和tenantId都是非空白字符串。♻️ 建议修改
public boolean hasExternalTenant() { - return externalTenantContext != null && externalTenantContext.getTenantId() != null; + return externalTenantContext != null + && externalTenantContext.getSource() != null + && !externalTenantContext.getSource().trim().isEmpty() + && externalTenantContext.getTenantId() != null + && !externalTenantContext.getTenantId().trim().isEmpty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/SessionInventory.java` around lines 106 - 108, The hasExternalTenant() check is too permissive—update SessionInventory.hasExternalTenant to return true only when externalTenantContext is non-null and both externalTenantContext.getSource() and externalTenantContext.getTenantId() are non-blank; replace the current single tenantId null check with a combined emptiness check (e.g. using a trim/blank check or StringUtils.isBlank) on getSource() and getTenantId() so callers won't be misled when source is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@identity/src/main/java/org/zstack/identity/AccountManagerImpl.java`:
- Line 32: Remove the duplicate import of ForEachFunction in AccountManagerImpl:
there are two identical imports of org.zstack.utils.function.ForEachFunction
(one at the top and one at line 47); delete the redundant import so only a
single import of ForEachFunction remains to keep imports clean.
---
Duplicate comments:
In `@header/src/main/java/org/zstack/header/identity/SessionInventory.java`:
- Around line 106-108: The hasExternalTenant() check is too permissive—update
SessionInventory.hasExternalTenant to return true only when
externalTenantContext is non-null and both externalTenantContext.getSource() and
externalTenantContext.getTenantId() are non-blank; replace the current single
tenantId null check with a combined emptiness check (e.g. using a trim/blank
check or StringUtils.isBlank) on getSource() and getTenantId() so callers won't
be misled when source is missing.
In `@rest/src/main/java/org/zstack/rest/RestServer.java`:
- Around line 1393-1395: ExternalTenantProvider registration silently overwrites
earlier providers; validate and reject duplicates by trimming and checking
non-empty source and failing on duplicates. In the loop over
pluginRgty.getExtensionList(ExternalTenantProvider.class), call p.getSource(),
trim it and ensure it's not null/empty, then check
externalTenantProviders.containsKey(source) and if present throw an
IllegalStateException (or a runtime exception appropriate for startup) instead
of calling externalTenantProviders.put; otherwise put the entry. Include the
failing source in the exception message to aid diagnosis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 6ffc8eaa-386d-42f0-8a57-2df91a589a06
⛔ Files ignored due to path filters (1)
conf/springConfigXml/AccountManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (14)
conf/db/upgrade/V5.5.1__schema.sqlheader/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantContext.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantProvider.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO_.javaheader/src/main/java/org/zstack/header/identity/ResourceOwnershipCreatedExtensionPoint.javaheader/src/main/java/org/zstack/header/identity/SessionInventory.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javaidentity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.javaidentity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.javarest/src/main/java/org/zstack/rest/RestConstants.javarest/src/main/java/org/zstack/rest/RestServer.java
🚧 Files skipped from review as they are similar to previous changes (8)
- header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.java
- header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java
- identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java
- header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO_.java
- conf/db/upgrade/V5.5.1__schema.sql
- header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java
- rest/src/main/java/org/zstack/rest/RestConstants.java
- header/src/main/java/org/zstack/header/identity/ExternalTenantContext.java
MatheMatrix
force-pushed
the
sync/hanyu.liang/zcf-1147@@2
branch
from
6149274 to
e341019
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (7)
identity/src/main/java/org/zstack/identity/AccountManagerImpl.java (1)
32-32:⚠️ Potential issue | 🟡 Minor重复导入仍存在。
Line 32 与 Line 47 导入了同一个
ForEachFunction，建议删除其一。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/AccountManagerImpl.java` at line 32, The file AccountManagerImpl has a duplicate import of ForEachFunction; remove the redundant import statement so only one import of org.zstack.utils.function.ForEachFunction remains (keep the one that fits the existing import grouping/style), e.g., delete the extra ForEachFunction import line to avoid duplicate imports in AccountManagerImpl.header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java (1)
19-26:⚠️ Potential issue | 🟠 Major
validateTenant()需要明确异常契约。当前接口未明确“校验失败应抛什么异常”。建议在注释中明确实现方应抛
RestException（4xx，如BAD_REQUEST），避免实现方抛出通用运行时异常后被映射为 500。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java` around lines 19 - 26, Update the ExternalTenantProvider#validateTenant(ExternalTenantContext) contract to document the expected exception type: specify in its Javadoc that implementations should throw a RestException with a 4xx status (e.g., BAD_REQUEST) on validation failure rather than a generic RuntimeException, so callers/handlers won't map failures to 500; reference ExternalTenantProvider, validateTenant, ExternalTenantContext and suggest using RestException (BAD_REQUEST) for invalid tenant cases.rest/src/main/java/org/zstack/rest/RestServer.java (2)
1393-1395:⚠️ Potential issue | 🟠 Major注册
ExternalTenantProvider时应拒绝重复 source。Line 1393-1395 使用
put()会在 source 冲突时静默覆盖先注册实现。建议启动阶段对getSource()做唯一性校验并 fail-fast，错误信息包含冲突 source 与双方类名。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 1393 - 1395, When registering ExternalTenantProvider implementations iterate pluginRgty.getExtensionList(ExternalTenantProvider.class) and detect duplicate getSource() values before calling externalTenantProviders.put(...); if a source is already present, throw an exception (fail-fast) including the conflicting source and both provider class names (existing and new) so startup fails instead of silently overwriting; update the registration code that currently loops and calls externalTenantProviders.put(...) to perform this uniqueness check and error reporting.
974-995:⚠️ Potential issue | 🟠 Major外部租户 Header 需要“全有或全无”校验。
Line 974-995 当前仅在
tenantSource与tenantId同时存在时才处理；当只传一个时会静默回退普通流程。建议改为：只要出现任一X-Tenant-*头，就强制校验tenantSource和tenantId均为非空白，否则返回 400。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 974 - 995, The current logic in RestServer handling X-Tenant-* headers only acts when both tenantSource and tenantId are present and silently ignores cases where one is present and the other is missing; update the handler so that if any X-Tenant-* header is present (check tenantSource != null || tenantId != null || tenantUser != null) you must trim and then validate that tenantSource and tenantId are non-blank, otherwise throw a RestException with HttpStatus.BAD_REQUEST; after validation proceed as before by looking up ExternalTenantProvider from externalTenantProviders using tenantSource, constructing ExternalTenantContext (setSource, setTenantId, optionally setUserId when tenantUser non-null), call provider.validateTenant(ctx), session.setExternalTenantContext(ctx) and ExternalTenantContext.setCurrent(ctx).identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java (2)
34-36:⚠️ Potential issue | 🟠 MajorProvider 注册阶段需要 source 唯一性校验。
Line 34-36 当前
put()会覆盖同 source 的旧实现。建议在启动时检测重复 source 并直接失败，避免后续租户路由到错误 provider。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java` around lines 34 - 36, The loop in ExternalTenantResourceTracker that iterates pluginRgty.getExtensionList(ExternalTenantProvider.class) and blindly calls providers.put(p.getSource(), p) can silently overwrite providers with the same source; update the initialization to detect duplicate ExternalTenantProvider.getSource() values (e.g., check providers.containsKey(source) before put), and on duplicate immediately fail startup by throwing a clear exception (or aborting initialization) with the conflicting source and provider class names so duplicate registrations are rejected rather than overwritten.
68-77:⚠️ Potential issue | 🟠 Major资源绑定流程缺少幂等保护。
两条路径都可能执行同一资源的
persist + onResourceBound。若同一资源被重复触发，会造成重复ExternalTenantResourceRefVO记录和重复外部回调。建议增加唯一键（如source + tenantId + resourceUuid）并仅在首次成功插入后调用onResourceBound。Also applies to: 97-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java` around lines 68 - 77, The ExternalTenantResourceTracker currently does dbf.persist(new ExternalTenantResourceRefVO(...)) followed by provider.onResourceBound(...) which can run twice and create duplicate ExternalTenantResourceRefVO records and duplicate callbacks; modify the flow to enforce idempotency by adding a uniqueness constraint (e.g., source + tenantId + resourceUuid) on ExternalTenantResourceRefVO and change the logic in the bind path (the block creating ExternalTenantResourceRefVO and calling provider.onResourceBound) to attempt an atomic insert or existence check and only call provider.onResourceBound when the insert actually succeeds (or when the record did not previously exist); handle unique-constraint violations (or use upsert semantics) to swallow duplicates and avoid invoking provider.onResourceBound again.header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java (1)
31-37:⚠️ Potential issue | 🟠 Major不要静默吞掉扩展点异常。
Line 35-37 当前直接忽略异常，会让资源归属通知失败不可观测。建议至少记录
warn日志（包含resourceUuid与 provider/回调上下文），同时保持主流程不被中断。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java` around lines 31 - 37, The catch block in OwnedByAccountAspectHelper around notifier.notifyResourceOwnershipCreated(ref) currently swallows exceptions silently; change it to catch Exception e, log a warn-level message (use the class logger) that includes ref.getResourceUuid() and any provider/callback context available (e.g., notifier identity or callback metadata) along with the exception, and then continue without rethrowing so the main flow remains unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java`:
- Around line 31-37: The catch block in OwnedByAccountAspectHelper around
notifier.notifyResourceOwnershipCreated(ref) currently swallows exceptions
silently; change it to catch Exception e, log a warn-level message (use the
class logger) that includes ref.getResourceUuid() and any provider/callback
context available (e.g., notifier identity or callback metadata) along with the
exception, and then continue without rethrowing so the main flow remains
unaffected.
In `@header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java`:
- Around line 19-26: Update the
ExternalTenantProvider#validateTenant(ExternalTenantContext) contract to
document the expected exception type: specify in its Javadoc that
implementations should throw a RestException with a 4xx status (e.g.,
BAD_REQUEST) on validation failure rather than a generic RuntimeException, so
callers/handlers won't map failures to 500; reference ExternalTenantProvider,
validateTenant, ExternalTenantContext and suggest using RestException
(BAD_REQUEST) for invalid tenant cases.
In `@identity/src/main/java/org/zstack/identity/AccountManagerImpl.java`:
- Line 32: The file AccountManagerImpl has a duplicate import of
ForEachFunction; remove the redundant import statement so only one import of
org.zstack.utils.function.ForEachFunction remains (keep the one that fits the
existing import grouping/style), e.g., delete the extra ForEachFunction import
line to avoid duplicate imports in AccountManagerImpl.
In
`@identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java`:
- Around line 34-36: The loop in ExternalTenantResourceTracker that iterates
pluginRgty.getExtensionList(ExternalTenantProvider.class) and blindly calls
providers.put(p.getSource(), p) can silently overwrite providers with the same
source; update the initialization to detect duplicate
ExternalTenantProvider.getSource() values (e.g., check
providers.containsKey(source) before put), and on duplicate immediately fail
startup by throwing a clear exception (or aborting initialization) with the
conflicting source and provider class names so duplicate registrations are
rejected rather than overwritten.
- Around line 68-77: The ExternalTenantResourceTracker currently does
dbf.persist(new ExternalTenantResourceRefVO(...)) followed by
provider.onResourceBound(...) which can run twice and create duplicate
ExternalTenantResourceRefVO records and duplicate callbacks; modify the flow to
enforce idempotency by adding a uniqueness constraint (e.g., source + tenantId +
resourceUuid) on ExternalTenantResourceRefVO and change the logic in the bind
path (the block creating ExternalTenantResourceRefVO and calling
provider.onResourceBound) to attempt an atomic insert or existence check and
only call provider.onResourceBound when the insert actually succeeds (or when
the record did not previously exist); handle unique-constraint violations (or
use upsert semantics) to swallow duplicates and avoid invoking
provider.onResourceBound again.
In `@rest/src/main/java/org/zstack/rest/RestServer.java`:
- Around line 1393-1395: When registering ExternalTenantProvider implementations
iterate pluginRgty.getExtensionList(ExternalTenantProvider.class) and detect
duplicate getSource() values before calling externalTenantProviders.put(...); if
a source is already present, throw an exception (fail-fast) including the
conflicting source and both provider class names (existing and new) so startup
fails instead of silently overwriting; update the registration code that
currently loops and calls externalTenantProviders.put(...) to perform this
uniqueness check and error reporting.
- Around line 974-995: The current logic in RestServer handling X-Tenant-*
headers only acts when both tenantSource and tenantId are present and silently
ignores cases where one is present and the other is missing; update the handler
so that if any X-Tenant-* header is present (check tenantSource != null ||
tenantId != null || tenantUser != null) you must trim and then validate that
tenantSource and tenantId are non-blank, otherwise throw a RestException with
HttpStatus.BAD_REQUEST; after validation proceed as before by looking up
ExternalTenantProvider from externalTenantProviders using tenantSource,
constructing ExternalTenantContext (setSource, setTenantId, optionally setUserId
when tenantUser non-null), call provider.validateTenant(ctx),
session.setExternalTenantContext(ctx) and ExternalTenantContext.setCurrent(ctx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: bcb8c0f2-19ed-41ef-922b-bb93940073a9
⛔ Files ignored due to path filters (1)
conf/springConfigXml/AccountManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (14)
conf/db/upgrade/V5.5.1__schema.sqlheader/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantContext.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantProvider.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO_.javaheader/src/main/java/org/zstack/header/identity/ResourceOwnershipCreatedExtensionPoint.javaheader/src/main/java/org/zstack/header/identity/SessionInventory.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javaidentity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.javaidentity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.javarest/src/main/java/org/zstack/rest/RestConstants.javarest/src/main/java/org/zstack/rest/RestServer.java
🚧 Files skipped from review as they are similar to previous changes (6)
- header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.java
- conf/db/upgrade/V5.5.1__schema.sql
- header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO.java
- rest/src/main/java/org/zstack/rest/RestConstants.java
- header/src/main/java/org/zstack/header/identity/SessionInventory.java
- header/src/main/java/org/zstack/header/identity/ExternalTenantContext.java
MatheMatrix
force-pushed
the
sync/hanyu.liang/zcf-1147@@2
branch
from
e341019 to
7d46230
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
rest/src/main/java/org/zstack/rest/RestServer.java (2)
974-995:⚠️ Potential issue | 🟠 Major外部租户头校验需要 all-or-nothing，当前可被静默绕过。
目前只有
tenantSource和tenantId同时非 null 才进入隔离逻辑；缺一个头会回退普通流程。建议：只要任一X-Tenant-*出现，就强制校验source + tenantId非空白，并拒绝空白tenantUser。🛠️ 建议修复
- if (tenantSource != null && tenantId != null) { + boolean sourcePresent = tenantSource != null; + boolean tenantPresent = tenantId != null; + boolean userPresent = tenantUser != null; + + if (sourcePresent || tenantPresent || userPresent) { + boolean sourceValid = tenantSource != null && !tenantSource.trim().isEmpty(); + boolean tenantValid = tenantId != null && !tenantId.trim().isEmpty(); + if (!sourceValid || !tenantValid) { + throw new RestException(HttpStatus.BAD_REQUEST.value(), + String.format("headers[%s] and [%s] must be provided together and be non-blank", + RestConstants.HEADER_TENANT_SOURCE, RestConstants.HEADER_TENANT_ID)); + } + if (tenantUser != null && tenantUser.trim().isEmpty()) { + throw new RestException(HttpStatus.BAD_REQUEST.value(), + String.format("header[%s] cannot be blank", RestConstants.HEADER_TENANT_USER)); + } + tenantSource = tenantSource.trim(); tenantId = tenantId.trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 974 - 995, Change the current conditional so that the presence of any X-Tenant-* header triggers strict all-or-nothing validation: if tenantSource, tenantId, or tenantUser headers are present (non-null), trim them and require tenantSource and tenantId to be non-blank (otherwise throw a RestException(HttpStatus.BAD_REQUEST.value(), "...")), and also reject empty/blank tenantUser when provided; then lookup ExternalTenantProvider from externalTenantProviders using tenantSource, call provider.validateTenant(ctx), and continue to set the ExternalTenantContext on session via session.setExternalTenantContext(ctx) and ExternalTenantContext.setCurrent(ctx) as before.
1389-1391:⚠️ Potential issue | 🟠 Major注册
ExternalTenantProvider时应拒绝重复source，不要静默覆盖。当前
put()会覆盖先注册实现，重复 source 时会把请求路由到错误 provider。🛠️ 建议修复
for (ExternalTenantProvider p : pluginRgty.getExtensionList(ExternalTenantProvider.class)) { - externalTenantProviders.put(p.getSource(), p); + String source = p.getSource() == null ? null : p.getSource().trim(); + if (source == null || source.isEmpty()) { + throw new CloudRuntimeException(String.format( + "ExternalTenantProvider[%s] returned empty source", p.getClass().getName())); + } + + ExternalTenantProvider old = externalTenantProviders.putIfAbsent(source, p); + if (old != null) { + throw new CloudRuntimeException(String.format( + "duplicate ExternalTenantProvider[%s, %s] for source[%s]", + p.getClass().getName(), old.getClass().getName(), source)); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 1389 - 1391, When registering ExternalTenantProvider instances from pluginRgty.getExtensionList(ExternalTenantProvider.class), do not silently overwrite existing entries in externalTenantProviders by calling externalTenantProviders.put(p.getSource(), p); instead check if externalTenantProviders already contains the key p.getSource() and reject duplicates (e.g., throw an IllegalStateException or a dedicated configuration exception with a clear message including the duplicate source and both provider classes) so registration fails fast rather than routing requests to the wrong provider.header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java (1)
19-26:⚠️ Potential issue | 🟠 Major
validateTenant()的失败异常契约仍不明确，可能被映射为 500。建议在 Javadoc 明确：校验失败必须抛出
RestException（4xx，如BAD_REQUEST），避免实现方抛出通用异常导致客户端错误被当作服务端错误。🛠️ 建议修复（不破坏签名）
/** * Validate tenant context validity. * Called after RestServer parses Header and before injecting into Session. - * Throwing an exception indicates validation failure, and the request will be rejected. + * Validation failure must throw RestException with a 4xx status code + * (preferably HttpStatus.BAD_REQUEST.value()) so RestServer can return + * a client error instead of 500. * * `@param` ctx External tenant context (already parsed from Header) */ void validateTenant(ExternalTenantContext ctx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java` around lines 19 - 26, Update the Javadoc for ExternalTenantProvider.validateTenant(ExternalTenantContext) to explicitly require that implementations signal validation failures by throwing a RestException (or a subclass carrying a 4xx client error status, e.g., BAD_REQUEST) rather than generic exceptions so the framework maps failures to client errors instead of 500; keep the method signature unchanged and include guidance that other exceptions will be treated as server errors and should be avoided for validation failures.header/src/main/java/org/zstack/header/identity/SessionInventory.java (1)
106-108:⚠️ Potential issue | 🟠 Major
hasExternalTenant()判定过宽，半残上下文会被误判为有效。建议至少要求
source和tenantId都为非空白字符串，否则会让后续隔离/绑定流程行为不一致。🛠️ 建议修复
public boolean hasExternalTenant() { - return externalTenantContext != null && externalTenantContext.getTenantId() != null; + return externalTenantContext != null + && externalTenantContext.getSource() != null + && !externalTenantContext.getSource().trim().isEmpty() + && externalTenantContext.getTenantId() != null + && !externalTenantContext.getTenantId().trim().isEmpty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/SessionInventory.java` around lines 106 - 108, The current SessionInventory.hasExternalTenant() check is too permissive and treats partially populated externalTenantContext as valid; update hasExternalTenant() to require externalTenantContext != null and both externalTenantContext.getSource() and externalTenantContext.getTenantId() are non-null, non-empty, non-whitespace strings (e.g., trim-and-check or use StringUtils.isNotBlank) so that only fully-populated external tenant contexts are considered valid; reference externalTenantContext, getSource(), and getTenantId() when locating the method to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@conf/db/upgrade/V5.5.1__schema.sql`:
- Around line 15-18: 升级脚本中的外键引用未带 schema，需将 REFERENCES 目标表显式限定为 zstack 模式：在
V5.5.1__schema.sql 中把 CONSTRAINT fk_ext_tenant_resource 的 REFERENCES
`ResourceVO`(`uuid`) 改为 REFERENCES zstack.`ResourceVO`(`uuid`)，并把 CONSTRAINT
fk_ext_tenant_account 的 REFERENCES `AccountVO`(`uuid`) 改为 REFERENCES
zstack.`AccountVO`(`uuid`)，保持与 conf/db/upgrade 目录其它脚本的 schema-qualified 约定一致。
In `@rest/src/main/java/org/zstack/rest/RestServer.java`:
- Around line 993-995: ExternalTenantContext.setCurrent(ctx) is called but never
cleared, risking ThreadLocal leakage; update RestServer to call
ExternalTenantContext.clearCurrent() in the request teardown path (e.g., inside
a finally block around the request handling) so that every code path clears the
ThreadLocal; locate the request handling scope in RestServer (where
ExternalTenantContext.setCurrent is invoked) and add the clearCurrent() call in
the corresponding finally to guarantee cleanup on success, error, or early
return.
---
Duplicate comments:
In `@header/src/main/java/org/zstack/header/identity/ExternalTenantProvider.java`:
- Around line 19-26: Update the Javadoc for
ExternalTenantProvider.validateTenant(ExternalTenantContext) to explicitly
require that implementations signal validation failures by throwing a
RestException (or a subclass carrying a 4xx client error status, e.g.,
BAD_REQUEST) rather than generic exceptions so the framework maps failures to
client errors instead of 500; keep the method signature unchanged and include
guidance that other exceptions will be treated as server errors and should be
avoided for validation failures.
In `@header/src/main/java/org/zstack/header/identity/SessionInventory.java`:
- Around line 106-108: The current SessionInventory.hasExternalTenant() check is
too permissive and treats partially populated externalTenantContext as valid;
update hasExternalTenant() to require externalTenantContext != null and both
externalTenantContext.getSource() and externalTenantContext.getTenantId() are
non-null, non-empty, non-whitespace strings (e.g., trim-and-check or use
StringUtils.isNotBlank) so that only fully-populated external tenant contexts
are considered valid; reference externalTenantContext, getSource(), and
getTenantId() when locating the method to change.
In `@rest/src/main/java/org/zstack/rest/RestServer.java`:
- Around line 974-995: Change the current conditional so that the presence of
any X-Tenant-* header triggers strict all-or-nothing validation: if
tenantSource, tenantId, or tenantUser headers are present (non-null), trim them
and require tenantSource and tenantId to be non-blank (otherwise throw a
RestException(HttpStatus.BAD_REQUEST.value(), "...")), and also reject
empty/blank tenantUser when provided; then lookup ExternalTenantProvider from
externalTenantProviders using tenantSource, call provider.validateTenant(ctx),
and continue to set the ExternalTenantContext on session via
session.setExternalTenantContext(ctx) and ExternalTenantContext.setCurrent(ctx)
as before.
- Around line 1389-1391: When registering ExternalTenantProvider instances from
pluginRgty.getExtensionList(ExternalTenantProvider.class), do not silently
overwrite existing entries in externalTenantProviders by calling
externalTenantProviders.put(p.getSource(), p); instead check if
externalTenantProviders already contains the key p.getSource() and reject
duplicates (e.g., throw an IllegalStateException or a dedicated configuration
exception with a clear message including the duplicate source and both provider
classes) so registration fails fast rather than routing requests to the wrong
provider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 400cd424-71d3-4adc-aa6a-0ea73617ac13
⛔ Files ignored due to path filters (1)
conf/springConfigXml/AccountManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (14)
conf/db/upgrade/V5.5.1__schema.sqlheader/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantContext.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantProvider.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO.javaheader/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO_.javaheader/src/main/java/org/zstack/header/identity/ResourceOwnershipCreatedExtensionPoint.javaheader/src/main/java/org/zstack/header/identity/SessionInventory.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javaidentity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.javaidentity/src/main/java/org/zstack/identity/ExternalTenantZQLExtension.javarest/src/main/java/org/zstack/rest/RestConstants.javarest/src/main/java/org/zstack/rest/RestServer.java
🚧 Files skipped from review as they are similar to previous changes (5)
- header/src/main/java/org/zstack/header/identity/ResourceOwnershipCreatedExtensionPoint.java
- header/src/main/java/org/zstack/header/aspect/OwnedByAccountAspectHelper.java
- header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefInventory.java
- identity/src/main/java/org/zstack/identity/ExternalTenantResourceTracker.java
- header/src/main/java/org/zstack/header/identity/ExternalTenantResourceRefVO.java
Comment on lines
+15
to
+18
| CONSTRAINT fk_ext_tenant_resource FOREIGN KEY (`resourceUuid`) | ||
| REFERENCES `ResourceVO`(`uuid`) ON DELETE CASCADE, | ||
| CONSTRAINT fk_ext_tenant_account FOREIGN KEY (`accountUuid`) | ||
| REFERENCES `AccountVO`(`uuid`) ON DELETE CASCADE |
There was a problem hiding this comment.
升级脚本中的外键引用建议显式带上 zstack schema。
当前 FK 引用未带 schema，和本目录既有升级脚本约定不一致，建议统一写成 zstack.\ResourceVO`/zstack.`AccountVO``。
🛠️ 建议修复
- CONSTRAINT fk_ext_tenant_resource FOREIGN KEY (`resourceUuid`)
- REFERENCES `ResourceVO`(`uuid`) ON DELETE CASCADE,
- CONSTRAINT fk_ext_tenant_account FOREIGN KEY (`accountUuid`)
- REFERENCES `AccountVO`(`uuid`) ON DELETE CASCADE
+ CONSTRAINT `fk_ext_tenant_resource` FOREIGN KEY (`resourceUuid`)
+ REFERENCES `zstack`.`ResourceVO`(`uuid`) ON DELETE CASCADE,
+ CONSTRAINT `fk_ext_tenant_account` FOREIGN KEY (`accountUuid`)
+ REFERENCES `zstack`.`AccountVO`(`uuid`) ON DELETE CASCADEBased on learnings: In ZStack upgrade scripts under conf/db/upgrade, schema is fixed as zstack and table references should stay schema-qualified.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CONSTRAINT fk_ext_tenant_resource FOREIGN KEY (`resourceUuid`) | |
| REFERENCES `ResourceVO`(`uuid`) ON DELETE CASCADE, | |
| CONSTRAINT fk_ext_tenant_account FOREIGN KEY (`accountUuid`) | |
| REFERENCES `AccountVO`(`uuid`) ON DELETE CASCADE | |
| CONSTRAINT `fk_ext_tenant_resource` FOREIGN KEY (`resourceUuid`) | |
| REFERENCES `zstack`.`ResourceVO`(`uuid`) ON DELETE CASCADE, | |
| CONSTRAINT `fk_ext_tenant_account` FOREIGN KEY (`accountUuid`) | |
| REFERENCES `zstack`.`AccountVO`(`uuid`) ON DELETE CASCADE |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@conf/db/upgrade/V5.5.1__schema.sql` around lines 15 - 18, 升级脚本中的外键引用未带
schema，需将 REFERENCES 目标表显式限定为 zstack 模式：在 V5.5.1__schema.sql 中把 CONSTRAINT
fk_ext_tenant_resource 的 REFERENCES `ResourceVO`(`uuid`) 改为 REFERENCES
zstack.`ResourceVO`(`uuid`)，并把 CONSTRAINT fk_ext_tenant_account 的 REFERENCES
`AccountVO`(`uuid`) 改为 REFERENCES zstack.`AccountVO`(`uuid`)，保持与 conf/db/upgrade
目录其它脚本的 schema-qualified 约定一致。
Comment on lines
+993
to
+995
| ExternalTenantContext.setCurrent(ctx); | ||
| } | ||
| } |
There was a problem hiding this comment.
ExternalTenantContext 未在请求结束后清理，存在 ThreadLocal 污染风险。
这里设置了 ExternalTenantContext.setCurrent(ctx)，但请求链路里没有对应 clearCurrent()，线程复用下会把上一个请求的租户上下文泄漏到后续请求。
🛠️ 建议修复（在请求级 finally 清理）
- try {
+ try {
if (api instanceof Api) {
handleUniqueApi((Api) api, entity, req, rsp);
} else {
handleNonUniqueApi((Collection)api, entity, req, rsp);
}
} catch (RestException e) {
sendResponse(e.statusCode, e.error, rsp);
} catch (Throwable e) {
logger.warn(String.format("failed to handle API to %s", path), e);
sendResponse(HttpStatus.INTERNAL_SERVER_ERROR.value(), e.getMessage(), rsp);
+ } finally {
+ ExternalTenantContext.clearCurrent();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 993 - 995,
ExternalTenantContext.setCurrent(ctx) is called but never cleared, risking
ThreadLocal leakage; update RestServer to call
ExternalTenantContext.clearCurrent() in the request teardown path (e.g., inside
a finally block around the request handling) so that every code path clears the
ThreadLocal; locate the request handling scope in RestServer (where
ExternalTenantContext.setCurrent is invoked) and add the clearCurrent() call in
the corresponding finally to guarantee cleanup on success, error, or early
return.
d4bcb0e to
c6f2a0c
Compare
…ramework Implement a generic framework for external services (ZCF, AIOS, etc.) to isolate resources by tenant via HTTP headers (X-Tenant-Source/Id/User). Core components: - ExternalTenantContext: ThreadLocal DTO bridging REST layer to AOP layer - ExternalTenantProvider: SPI interface for tenant validation and tracking - ExternalTenantResourceRefVO: JPA entity with CASCADE delete on ResourceVO, UNIQUE KEY (resourceUuid, source, tenantId) for idempotent binding - ExternalTenantResourceTracker: AOP-driven resource binding on creation, cleanup on hard/soft delete, duplicate provider detection (fail-fast), idempotent binding check before persist, three-tier context restoration in beforeDeliveryMessage (session → msg headers → MDC) - ExternalTenantZQLExtension: two-phase ZQL AST injection for automatic tenant-scoped query filtering with SQL injection protection - RestServer integration: header parsing with all-or-nothing validation, provider lookup via @Autowired ExternalTenantResourceTracker (single registry, no duplicate map), validateTenant() exception wrapped as 400, ThreadLocal lifecycle management with try/finally cleanup - SessionInventory: @APINoSee externalTenantContext field, hasExternalTenant() requires both source and tenantId non-null and non-blank - OwnedByAccountAspectHelper: notifier exception rethrown to roll back transaction on binding failure - DB migration: V5.5.1__schema.sql for ExternalTenantResourceRefVO table Resolves: ZCF-1147 Change-Id: I7778676171646874706164777869707288976172
MatheMatrix
force-pushed
the
sync/hanyu.liang/zcf-1147@@2
branch
from
c6f2a0c to
3e9b290
Compare
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.
Implement a generic framework for external services (ZCF, AIOS, etc.) to
isolate resources by tenant via HTTP headers (X-Tenant-Source/Id/User).
cleanup on hard/soft delete via extension points
tenant-scoped query filtering with SQL injection protection
lifecycle management with try/finally cleanup
Resolves: ZCF-1147
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
sync from gitlab !9355