<feature>[header]: add AccountVO.source#4018
Closed
ZStack-Robot wants to merge 1 commit into
Closed
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本 PR 引入 AccountSource 枚举并在模型、数据库迁移、账户创建/导入/LDAP 同步和 RBAC 初始化中加入或回填 source 字段，同时弃用 AccountType.ThirdParty，统一将其转为 Normal 并以 source 区分来源类型。 变更内容Account Source 数据模型与暴露层
账户导入与第三方源集成
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 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 `@conf/db/zsv/V5.1.0__schema.sql`:
- Line 4: The column createDate currently uses a hard-coded default ('1999-12-31
23:59:59'), which breaks time semantics; update the column definition for
createDate in the migration (the `createDate` timestamp NOT NULL line) to use a
dynamic default by replacing the literal with DEFAULT CURRENT_TIMESTAMP while
preserving NOT NULL, and ensure any subsequent DDL in this migration that relies
on that column uses the new CURRENT_TIMESTAMP behavior.
In `@identity/src/main/java/org/zstack/identity/AccountManagerImpl.java`:
- Around line 543-545: 在 AccountManagerImpl 中不要直接调用
AccountSource.valueOf(msg.getSource())，而应对 msg.getSource() 做显式校验并将非法值转换为业务错误：在设置
vo.setSource(...) 的位置使用安全解析（try-catch IllegalArgumentException
或先做枚举包含检查），如果解析失败抛出一个可读的参数错误（包含字段名 "source" 和传入值），例如将异常转换为项目中统一的
API/参数错误异常，从而避免运行时崩溃并返回可理解的错误给调用方。
In `@identity/src/main/java/org/zstack/identity/rbac/RBACManagerImpl.java`:
- Around line 179-182: The current query in RBACManagerImpl that builds
accountUuidList uses only AccountVO_.type == AccountType.Normal, which will
include migrated third‑party accounts now marked as Normal; update the query to
also filter by account source to include only local/native accounts (e.g., add a
condition on AccountVO_.source == <local-source-constant> or the enum/constant
used for local sources) so third‑party-sourced accounts are excluded from preset
role initialization; reference AccountVO, AccountVO_.type, AccountVO_.source,
and AccountType.Normal when making this change.
In `@plugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java`:
- Around line 376-379: findGlobalUuidKey() currently calls switch
(taskSpec.getServerType()) without handling null which can throw NPE; mirror
toAccountSource()'s null-safe behavior by first normalizing the server type
(e.g., treat null as LdapServerType.Unknown) or explicitly check
taskSpec.getServerType() == null before the switch and handle that branch,
ensuring findGlobalUuidKey() uses LdapServerType.Unknown (or the same fallback
used by toAccountSource()) when server type is null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9d788275-26c1-453c-a9eb-6bac5eba3718
📒 Files selected for processing (13)
conf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/identity/AccountInventory.javaheader/src/main/java/org/zstack/header/identity/AccountInventoryDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/AccountSource.javaheader/src/main/java/org/zstack/header/identity/AccountType.javaheader/src/main/java/org/zstack/header/identity/AccountVO.javaheader/src/main/java/org/zstack/header/identity/AccountVO_.javaheader/src/main/java/org/zstack/header/identity/CreateAccountMsg.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javaidentity/src/main/java/org/zstack/identity/rbac/RBACManagerImpl.javaplugin/account-import/src/main/java/org/zstack/identity/imports/header/ImportAccountItem.javaplugin/account-import/src/main/java/org/zstack/identity/imports/source/AbstractAccountSourceBase.javaplugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java
| CREATE TABLE IF NOT EXISTS `zstack`.`TpmKeyBackupVO` ( | ||
| `uuid` char(32) NOT NULL UNIQUE, | ||
| `lastOpDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, | ||
| `createDate` timestamp NOT NULL DEFAULT '1999-12-31 23:59:59', |
There was a problem hiding this comment.
createDate 使用固定历史时间会导致数据时间语义失真。
这里把 createDate 默认值写死为 1999-12-31 23:59:59，会让新插入记录的创建时间不准确，影响审计、排序和排障。建议改为 DEFAULT CURRENT_TIMESTAMP。
💡建议修改
- `createDate` timestamp NOT NULL DEFAULT '1999-12-31 23:59:59',
+ `createDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP,As per coding guidelines **/*.sql: “Do not use DEFAULT 0000-00-00 00:00:00 , use DEFAULT CURRENT_TIMESTAMP instead”.
📝 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
| `createDate` timestamp NOT NULL DEFAULT '1999-12-31 23:59:59', | |
| `createDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, |
🤖 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 `@conf/db/zsv/V5.1.0__schema.sql` at line 4, The column createDate currently
uses a hard-coded default ('1999-12-31 23:59:59'), which breaks time semantics;
update the column definition for createDate in the migration (the `createDate`
timestamp NOT NULL line) to use a dynamic default by replacing the literal with
DEFAULT CURRENT_TIMESTAMP while preserving NOT NULL, and ensure any subsequent
DDL in this migration that relies on that column uses the new CURRENT_TIMESTAMP
behavior.
Comment on lines
179
to
182
| List<String> accountUuidList = Q.New(AccountVO.class) | ||
| .in(AccountVO_.type, list(AccountType.Normal, AccountType.ThirdParty)) | ||
| .in(AccountVO_.type, AccountType.Normal) | ||
| .select(AccountVO_.uuid) | ||
| .listValues(); |
There was a problem hiding this comment.
type=Normal 过滤在迁移后会误纳入第三方来源账户
Line 180 现在只按 AccountType.Normal 过滤；但本次迁移会把历史第三方账户也转成 Normal，这会让这些账户重新被纳入预置角色初始化，和“排除第三方账户”的目标冲突。建议在这里增加 source 条件做排除（例如仅纳入本地来源账户）。
建议修复（示意）
List<String> accountUuidList = Q.New(AccountVO.class)
- .in(AccountVO_.type, AccountType.Normal)
+ .eq(AccountVO_.type, AccountType.Normal)
+ .eq(AccountVO_.source, AccountSource.Local)
.select(AccountVO_.uuid)
.listValues();📝 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
| List<String> accountUuidList = Q.New(AccountVO.class) | |
| .in(AccountVO_.type, list(AccountType.Normal, AccountType.ThirdParty)) | |
| .in(AccountVO_.type, AccountType.Normal) | |
| .select(AccountVO_.uuid) | |
| .listValues(); | |
| List<String> accountUuidList = Q.New(AccountVO.class) | |
| .eq(AccountVO_.type, AccountType.Normal) | |
| .eq(AccountVO_.source, AccountSource.Local) | |
| .select(AccountVO_.uuid) | |
| .listValues(); |
🤖 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 `@identity/src/main/java/org/zstack/identity/rbac/RBACManagerImpl.java` around
lines 179 - 182, The current query in RBACManagerImpl that builds
accountUuidList uses only AccountVO_.type == AccountType.Normal, which will
include migrated third‑party accounts now marked as Normal; update the query to
also filter by account source to include only local/native accounts (e.g., add a
condition on AccountVO_.source == <local-source-constant> or the enum/constant
used for local sources) so third‑party-sourced accounts are excluded from preset
role initialization; reference AccountVO, AccountVO_.type, AccountVO_.source,
and AccountType.Normal when making this change.
Comment on lines
+376
to
+379
| private AccountSource toAccountSource(LdapServerType serverType) { | ||
| return AccountSource.fromLdapServerTypeName( | ||
| serverType == null ? LdapServerType.Unknown.name() : serverType.name()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -i 'LdapSyncHelper.java' | head -n 1)"
echo "Inspect file: $file"
nl -ba "$file" | sed -n '372,410p'
python - <<'PY'
from pathlib import Path
p = next(Path('.').rglob('LdapSyncHelper.java'))
text = p.read_text(encoding='utf-8')
print("contains toAccountSource null fallback:",
"serverType == null ? LdapServerType.Unknown.name() : serverType.name()" in text)
anchor = "public String findGlobalUuidKey()"
idx = text.find(anchor)
snippet = text[idx: idx + 320] if idx != -1 else ""
print("findGlobalUuidKey has explicit null guard:",
"taskSpec.getServerType() == null" in snippet)
print("findGlobalUuidKey uses switch(taskSpec.getServerType()):",
"switch (taskSpec.getServerType())" in snippet)
PYRepository: MatheMatrix/zstack
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="plugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java"
echo "=== Examining file: $file ==="
echo ""
echo "=== Lines 372-410 (code context) ==="
sed -n '372,410p' "$file" | cat -n
echo ""
echo "=== Python verification ==="
python3 <<'PY'
from pathlib import Path
p = Path("plugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java")
text = p.read_text(encoding='utf-8')
lines = text.split('\n')
# Check toAccountSource null fallback
print("1. Checking toAccountSource() method:")
for i, line in enumerate(lines[375:380], start=376):
print(f" Line {i}: {line}")
# Check findGlobalUuidKey null guard
print("\n2. Checking findGlobalUuidKey() method:")
for i, line in enumerate(lines):
if "public String findGlobalUuidKey()" in line:
print(f" Found at line {i+1}")
# Print 20 lines from this point
for j in range(20):
if i+j < len(lines):
print(f" Line {i+j+1}: {lines[i+j]}")
break
# Verify the specific conditions
print("\n3. Verification results:")
print(f" - toAccountSource has null fallback: {'serverType == null ?' in text}")
print(f" - findGlobalUuidKey has null guard: {'findGlobalUuidKey' in text and 'taskSpec.getServerType() == null' in text[text.find('findGlobalUuidKey'):text.find('findGlobalUuidKey')+1000] if 'findGlobalUuidKey' in text else False}")
# Find the actual switch statement context
idx = text.find("public String findGlobalUuidKey()")
if idx != -1:
snippet = text[idx:idx+800]
print(f" - Uses switch(taskSpec.getServerType()): {'switch (taskSpec.getServerType())' in snippet}")
print(f" - Has null guard before switch: {'== null' in snippet[:300]}")
PYRepository: MatheMatrix/zstack
Length of output: 3800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="plugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java"
echo "=== Looking for taskSpec field definition ==="
grep -n "taskSpec" "$file" | head -20
echo ""
echo "=== Looking for TaskSpec class and getServerType method ==="
fd "TaskSpec.java" -t f
echo ""
echo "=== Checking taskSpec initialization/assignment in the file ==="
grep -n "taskSpec\s*=" "$file" | head -10
echo ""
echo "=== Searching for getServerType() definition in codebase ==="
rg "getServerType\(\)" -A 2 -B 2 --max-count 10Repository: MatheMatrix/zstack
Length of output: 13375
补齐 findGlobalUuidKey() 中的空值检查，与 toAccountSource() 保持一致的 null 兜底处理。
toAccountSource() 已对 serverType == null 进行兜底处理，但 findGlobalUuidKey() 在第 399 行直接使用 switch (taskSpec.getServerType()) 而无空值检查。由于 LdapServerType 在更新时可被设置为 null（见 LdapManagerImpl 的 spec.setServerType(msg.getServerType() == null ? null : ...)），当 getServerType() 返回 null 时会直接触发 NullPointerException，导致 LDAP 同步提前失败。
建议在 switch 前添加 null 检查，与 toAccountSource() 保持一致的兜底处理策略。
🤖 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 `@plugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java` around
lines 376 - 379, findGlobalUuidKey() currently calls switch
(taskSpec.getServerType()) without handling null which can throw NPE; mirror
toAccountSource()'s null-safe behavior by first normalizing the server type
(e.g., treat null as LdapServerType.Unknown) or explicitly check
taskSpec.getServerType() == null before the switch and handle that branch,
ensuring findGlobalUuidKey() uses LdapServerType.Unknown (or the same fallback
used by toAccountSource()) when server type is null.
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap@@2
branch
from
a7eaf69 to
00088b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@header/src/main/java/org/zstack/header/identity/AccountSource.java`:
- Around line 19-24: The method AccountSource.fromLdapServerTypeName currently
maps any null or unknown serverType to WindowsAD causing silent mislabeling;
change it to explicitly handle known values only by comparing serverType to
OpenLdap.name() and WindowsAD.name(), and throw an IllegalArgumentException (or
return null if preferred by project convention) when serverType is null or does
not match either OpenLdap or WindowsAD so unknown inputs fail fast instead of
being mapped to WindowsAD; update the method
AccountSource.fromLdapServerTypeName to perform those explicit checks and raise
the error using clear messaging that includes the original serverType.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b2ae346-48ac-40a6-873e-8abc27fe96f1
📒 Files selected for processing (13)
conf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/identity/AccountInventory.javaheader/src/main/java/org/zstack/header/identity/AccountInventoryDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/AccountSource.javaheader/src/main/java/org/zstack/header/identity/AccountType.javaheader/src/main/java/org/zstack/header/identity/AccountVO.javaheader/src/main/java/org/zstack/header/identity/AccountVO_.javaheader/src/main/java/org/zstack/header/identity/CreateAccountMsg.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javaidentity/src/main/java/org/zstack/identity/rbac/RBACManagerImpl.javaplugin/account-import/src/main/java/org/zstack/identity/imports/header/ImportAccountItem.javaplugin/account-import/src/main/java/org/zstack/identity/imports/source/AbstractAccountSourceBase.javaplugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java
🚧 Files skipped from review as they are similar to previous changes (12)
- header/src/main/java/org/zstack/header/identity/AccountVO_.java
- header/src/main/java/org/zstack/header/identity/AccountInventory.java
- identity/src/main/java/org/zstack/identity/rbac/RBACManagerImpl.java
- header/src/main/java/org/zstack/header/identity/CreateAccountMsg.java
- plugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java
- plugin/account-import/src/main/java/org/zstack/identity/imports/header/ImportAccountItem.java
- header/src/main/java/org/zstack/header/identity/AccountInventoryDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/identity/AccountType.java
- conf/db/zsv/V5.1.0__schema.sql
- header/src/main/java/org/zstack/header/identity/AccountVO.java
- plugin/account-import/src/main/java/org/zstack/identity/imports/source/AbstractAccountSourceBase.java
- identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
Comment on lines
+19
to
+24
| public static AccountSource fromLdapServerTypeName(@Nullable String serverType) { | ||
| if (OpenLdap.name().equals(serverType)) { | ||
| return OpenLdap; | ||
| } | ||
| return WindowsAD; | ||
| } |
There was a problem hiding this comment.
避免将未知/空 LDAP 类型静默映射为 WindowsAD。
当前逻辑会把 null 或任意未知 serverType 都写成 WindowsAD，会造成来源字段误标。建议显式匹配 WindowsAD，其余输入直接失败快。
建议修改
public static AccountSource fromLdapServerTypeName(`@Nullable` String serverType) {
- if (OpenLdap.name().equals(serverType)) {
- return OpenLdap;
- }
- return WindowsAD;
+ if (OpenLdap.name().equals(serverType)) {
+ return OpenLdap;
+ }
+ if (WindowsAD.name().equals(serverType)) {
+ return WindowsAD;
+ }
+ throw new IllegalArgumentException(String.format("Unsupported LDAP server type: %s", serverType));
}📝 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
| public static AccountSource fromLdapServerTypeName(@Nullable String serverType) { | |
| if (OpenLdap.name().equals(serverType)) { | |
| return OpenLdap; | |
| } | |
| return WindowsAD; | |
| } | |
| public static AccountSource fromLdapServerTypeName(`@Nullable` String serverType) { | |
| if (OpenLdap.name().equals(serverType)) { | |
| return OpenLdap; | |
| } | |
| if (WindowsAD.name().equals(serverType)) { | |
| return WindowsAD; | |
| } | |
| throw new IllegalArgumentException(String.format("Unsupported LDAP server type: %s", serverType)); | |
| } |
🤖 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 `@header/src/main/java/org/zstack/header/identity/AccountSource.java` around
lines 19 - 24, The method AccountSource.fromLdapServerTypeName currently maps
any null or unknown serverType to WindowsAD causing silent mislabeling; change
it to explicitly handle known values only by comparing serverType to
OpenLdap.name() and WindowsAD.name(), and throw an IllegalArgumentException (or
return null if preferred by project convention) when serverType is null or does
not match either OpenLdap or WindowsAD so unknown inputs fail fast instead of
being mapped to WindowsAD; update the method
AccountSource.fromLdapServerTypeName to perform those explicit checks and raise
the error using clear messaging that includes the original serverType.
There was a problem hiding this comment.
@zstack-robot-1，感谢 wenhao.zhang 的说明。了解了——由于 LDAP 侧实际只会传入 null 或 "Unknown"，回退到 WindowsAD 是对齐现有默认值的有意设计，不撤回之前的建议，但不再坚持。
为了让后续维护者更容易理解这一回退语义，建议在 return WindowsAD; 前加一行简短注释，例如：
// null or "Unknown" serverType defaults to WindowsAD to align with legacy behavior
return WindowsAD;这不影响任何逻辑，只是让意图更自解释。采纳与否由团队决定。
(´• ω •`) 🐇
✏️ Learnings added
Learnt from: zstack-robot-1
Repo: MatheMatrix/zstack PR: 4018
File: header/src/main/java/org/zstack/header/identity/AccountSource.java:19-24
Timestamp: 2026-05-19T09:46:14.609Z
Learning: 在 `AccountSource.fromLdapServerTypeName(Nullable String serverType)` 方法中（`header/src/main/java/org/zstack/header/identity/AccountSource.java`），将 `null` 或 `"Unknown"` 的 LDAP serverType 回退映射到 `WindowsAD` 是有意为之的行为，与项目现有默认值保持一致。代码审查中不应将此视为静默误标问题。
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ZStack-Robot
Repo: MatheMatrix/zstack PR: 3826
File: compute/src/main/java/org/zstack/compute/allocator/HostAllocatorChain.java:261-265
Timestamp: 2026-04-22T04:38:00.864Z
Learning: 在 ZStack 的 `Platform.err()` / `Platform.toRawString()` 调用中，传入用于格式化的 `%s` 参数若实现了 `org.zstack.header.core.I18nMessage`，框架会自动通过 `getDetails()` 取得原始文本、并通过 `getI18nDetails()` 取得国际化文本。因此，在代码审查中不应将“直接把实现了 `I18nMessage` 的对象作为 `%s` 参数传入而可能打印出对象地址/不安全”当作问题；此用法是正确且应当被视为安全的（例如将 `HostCandidate.RejectedCandidate` 这类实现了 `I18nMessage` 的对象作为 `%s` 参数传入）。
3e72a5b to
a0d38d0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
identity/src/main/java/org/zstack/identity/rbac/RBACManagerImpl.java (1)
179-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick win这里只按
type=Normal过滤会把迁移后的第三方账户重新纳入预置角色初始化。本次迁移会把历史
ThirdParty账户统一改成Normal，这里只看type已经无法排除 LDAP/其他外部来源账户。建议补上source == AccountSource.Local（或等价的本地来源条件），否则这两个预置角色会错误关联到非本地账户。建议修复
List<String> accountUuidList = Q.New(AccountVO.class) .eq(AccountVO_.type, AccountType.Normal) + .eq(AccountVO_.source, AccountSource.Local) .select(AccountVO_.uuid) .listValues();🤖 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 `@identity/src/main/java/org/zstack/identity/rbac/RBACManagerImpl.java` around lines 179 - 182, The current Account UUID query in RBACManagerImpl (the accountUuidList built with Q.New(AccountVO.class) and filters on AccountVO_.type) only filters by AccountType.Normal and will include migrated third-party accounts; update the query to also restrict to local accounts by adding a condition on AccountVO_.source == AccountSource.Local (or equivalent local-source check) so only truly local accounts are returned when initializing the preset roles.plugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java (1)
377-379:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
serverType的空值语义还没有闭环。这里已经把
null作为toAccountSource()的合法输入处理了，但后续buildReturnAttribute()仍会走到 Line 399 的switch (taskSpec.getServerType())。一旦serverType为空，LDAP 同步会在真正发起查询前直接触发NullPointerException。建议先把serverType统一归一化，再同时用于source映射和globalUuidKey选择。🤖 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 `@plugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java` around lines 377 - 379, Normalize null serverType early in the flow (before calling toAccountSource and before buildReturnAttribute) so the same non-null value is used for both AccountSource.fromLdapServerTypeName(...) and the globalUuidKey selection; specifically, ensure taskSpec.getServerType() is replaced or assigned to a normalized LdapServerType variable (e.g., resolvedServerType) and use that variable when calling toAccountSource(...) and inside the switch in buildReturnAttribute (the switch that currently uses taskSpec.getServerType()), guaranteeing no NullPointerException and consistent mapping between source and globalUuidKey.
🤖 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.
Duplicate comments:
In `@identity/src/main/java/org/zstack/identity/rbac/RBACManagerImpl.java`:
- Around line 179-182: The current Account UUID query in RBACManagerImpl (the
accountUuidList built with Q.New(AccountVO.class) and filters on
AccountVO_.type) only filters by AccountType.Normal and will include migrated
third-party accounts; update the query to also restrict to local accounts by
adding a condition on AccountVO_.source == AccountSource.Local (or equivalent
local-source check) so only truly local accounts are returned when initializing
the preset roles.
In `@plugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.java`:
- Around line 377-379: Normalize null serverType early in the flow (before
calling toAccountSource and before buildReturnAttribute) so the same non-null
value is used for both AccountSource.fromLdapServerTypeName(...) and the
globalUuidKey selection; specifically, ensure taskSpec.getServerType() is
replaced or assigned to a normalized LdapServerType variable (e.g.,
resolvedServerType) and use that variable when calling toAccountSource(...) and
inside the switch in buildReturnAttribute (the switch that currently uses
taskSpec.getServerType()), guaranteeing no NullPointerException and consistent
mapping between source and globalUuidKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cae5c4e0-9d57-44c1-a123-7404b9ed8e71
📒 Files selected for processing (15)
conf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/identity/AccountInventory.javaheader/src/main/java/org/zstack/header/identity/AccountInventoryDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/AccountSource.javaheader/src/main/java/org/zstack/header/identity/AccountType.javaheader/src/main/java/org/zstack/header/identity/AccountVO.javaheader/src/main/java/org/zstack/header/identity/AccountVO_.javaheader/src/main/java/org/zstack/header/identity/CreateAccountMsg.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javaidentity/src/main/java/org/zstack/identity/rbac/RBACManagerImpl.javaplugin/account-import/src/main/java/org/zstack/identity/imports/header/ImportAccountSpec.javaplugin/account-import/src/main/java/org/zstack/identity/imports/source/AbstractAccountSourceBase.javaplugin/account-import/src/main/java/org/zstack/identity/imports/source/ImportThirdPartyAccountContext.javaplugin/ldap/src/main/java/org/zstack/ldap/compute/LdapSyncHelper.javasdk/src/main/java/org/zstack/sdk/AccountInventory.java
✅ Files skipped from review due to trivial changes (1)
- header/src/main/java/org/zstack/header/identity/AccountType.java
🚧 Files skipped from review as they are similar to previous changes (12)
- sdk/src/main/java/org/zstack/sdk/AccountInventory.java
- header/src/main/java/org/zstack/header/identity/CreateAccountMsg.java
- header/src/main/java/org/zstack/header/identity/AccountVO_.java
- header/src/main/java/org/zstack/header/identity/AccountVO.java
- plugin/account-import/src/main/java/org/zstack/identity/imports/header/ImportAccountSpec.java
- header/src/main/java/org/zstack/header/identity/AccountInventoryDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/identity/AccountSource.java
- plugin/account-import/src/main/java/org/zstack/identity/imports/source/ImportThirdPartyAccountContext.java
- identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
- plugin/account-import/src/main/java/org/zstack/identity/imports/source/AbstractAccountSourceBase.java
- conf/db/zsv/V5.1.0__schema.sql
- header/src/main/java/org/zstack/header/identity/AccountInventory.java
Collaborator
|
Comment from yaohua.wu: Review: MR !9912 — ZSV-12269总体评价此 MR 是 ZCenter Account 重构第一轮的基础设施层，整体方向与 Jira 设计文档一致：
设计实现紧贴方案，无明显架构偏离。下面列出需要修复或确认的几处问题。 关联 MR
🔴 Critical
🟡 Warning
🟢 Suggestion
Coverage
Verdict: REVISION_REQUIRED
🤖 Robot Reviewer |
Owner
|
Comment from wenhao.zhang: 上面第二点：APICreateAccountMsg 根本没有 source 字段，不存在如下行为： 普通管理员可以通过 API 创建出 |
Resolves: ZSV-12269 DBImpact Change-Id: I7a6c61667a6874657376736b70767a68767a7862
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap@@2
branch
from
a0d38d0 to
1620b5f
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
3 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.
Resolves: ZSV-12269
Change-Id: I7a6c61667a6874657376736b70767a68767a7862
sync from gitlab !9912