<feature>[ldap]: refactor LDAP login logic and add two-factor authentication support#3725
Open
ZStack-Robot wants to merge 2 commits intozsv_5.0.0from
Open
<feature>[ldap]: refactor LDAP login logic and add two-factor authentication support#3725ZStack-Robot wants to merge 2 commits intozsv_5.0.0from
ZStack-Robot wants to merge 2 commits intozsv_5.0.0from
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新增 LDAP 辅助方法 Changes
Sequence Diagram(s)sequenceDiagram
participant 客户端 as Client
participant LDAP管理 as LdapManagerImpl
participant LDAP服 as LDAP
participant 数据库 as DB
客户端->>LDAP管理: 发起 login 或 collectUserInfo 请求（含凭据）
LDAP管理->>LDAP管理: 解析当前 LDAP server UUID 并计算 full‑DN
LDAP管理->>数据库: 查询 AccountThirdPartyAccountSourceRefVO(ldapUuid, DN)
数据库-->>LDAP管理: 返回 AccountThirdPartyAccountSourceRefVO (含 accountUuid) 或 错误/空
LDAP管理->>LDAP服: 使用提供凭据尝试绑定（验证凭据）
LDAP服-->>LDAP管理: 绑定成功 / 失败
LDAP管理->>客户端: 返回 LoginSessionInfo（含 accountUuid）或 返回错误
预估代码审查工作量🎯 4 (Complex) | ⏱️ ~45 分钟 诗歌
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.java`:
- Around line 12-15: 为接口方法添加 Javadoc 注释：为 LdapManager 接口中的 isValid,
findAccountThirdPartyAccountSourceRefByName 和 findCurrentLdapServerUuid
三个方法补充明确的
Javadoc，说明每个方法的行为语义（例如是否会进行认证）、参数含义（uid、password、ldapLoginName、ldapLoginPassword）、返回值含义（何时返回
true/false、ErrorableValue 中可能包含的 ErrorCode/错误情形）以及是否会抛出异常或返回
null/空值；同时移除任何多余的访问修饰符（接口方法不应显式声明 public）。确保注释使用标准 Javadoc
标签（@param、@return、@throws 如适用）并覆盖边界/失败场景以避免语义漂移。
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java`:
- Around line 485-493: Summary: collectUserInfoIntoContext() was changed to call
findAccountThirdPartyAccountSourceRefByName(...) which performs LDAP password
validation and can throw, turning this helper into a primary authentication step
and conflicting with actions like GetTwoFactorAuthenticationSecretAction that
rely on isSuppressCredentialCheck = true. Fix: revert
collectUserInfoIntoContext() to only resolve accountUuid by username (do not
validate credentials or throw on LDAP auth failure) — either call or add a
lookup method that only maps username → AccountThirdPartyAccountSourceRefVO
without checking the password, or add a flag parameter to
findAccountThirdPartyAccountSourceRefByName(...) to skip credential validation;
ensure only authenticate()/login() performs LDAP password checks and that
collectUserInfoIntoContext() returns quietly when the lookup cannot resolve an
account instead of throwing.
- Around line 411-420: The code fetches currentLdapServer once in
findAccountThirdPartyAccountSourceRefByName but then calls isValid(...) which
internally calls findCurrentLdapServer() again, risking using different LDAP
servers in one auth flow; change validation to use the same LdapServerVO
snapshot: update isValid to accept the LdapServerVO (e.g., add
isValid(LdapServerVO ldap, String ldapLoginName, String ldapLoginPassword) or an
overload) and call that from findAccountThirdPartyAccountSourceRefByName using
the local variable ldap so all checks (password validation and DN/bind lookup)
use the same ldap instance.
🪄 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: 4a343e3c-d552-4bf2-ad9b-a1f02c6f7dd0
📒 Files selected for processing (4)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.javaplugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.javasdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.javasdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.java
Comment on lines
12
to
15
| public interface LdapManager { | ||
| boolean isValid(String uid, String password); | ||
|
|
||
| ErrorableValue<AccountThirdPartyAccountSourceRefVO> findAccountThirdPartyAccountSourceRefByName(String ldapLoginName, String ldapLoginPassword); | ||
| ErrorableValue<String> findCurrentLdapServerUuid(); |
There was a problem hiding this comment.
给新接口方法补上 Javadoc。
这是对外契约，当前只看签名无法判断这里是否一定会做认证、失败时会返回哪些 ErrorCode，后续实现和调用方都容易出现语义漂移。
As per coding guidelines, “接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.java` around lines 12 -
15, 为接口方法添加 Javadoc 注释：为 LdapManager 接口中的 isValid,
findAccountThirdPartyAccountSourceRefByName 和 findCurrentLdapServerUuid
三个方法补充明确的
Javadoc，说明每个方法的行为语义（例如是否会进行认证）、参数含义（uid、password、ldapLoginName、ldapLoginPassword）、返回值含义（何时返回
true/false、ErrorableValue 中可能包含的 ErrorCode/错误情形）以及是否会抛出异常或返回
null/空值；同时移除任何多余的访问修饰符（接口方法不应显式声明 public）。确保注释使用标准 Javadoc
标签（@param、@return、@throws 如适用）并覆盖边界/失败场景以避免语义漂移。
Collaborator
|
Comment from yaohua.wu: Review: MR !9594 — ZSV-11833MR 概要：重构 LDAP 登录逻辑，提取公共方法 Jira 背景：ZSV-11833 要求 AD/LDAP 账号支持双因子认证。用户流程为：启用 2FA → 添加 LDAP 服务器 → 调用 🔴 Critical —
|
| 级别 | 数量 | 说明 |
|---|---|---|
| 🔴 Critical | 1 | collectUserInfoIntoContext 不应做密码校验，与 isSuppressCredentialCheck 语义冲突 |
| 🟡 Warning | 3 | LDAP 服务器双重获取风险；SDK Breaking Change；死代码 |
| 🟢 Suggestion | 2 | 代码风格；接口 Javadoc |
核心问题在于提取公共方法时，将「账号查找」与「密码校验」绑定在一起，导致所有调用方都被迫执行认证。建议将两者拆分，collectUserInfoIntoContext() 仅做纯查找，确保双因子认证获取 secret 的流程不受影响。
🤖 Robot Reviewer
MatheMatrix
force-pushed
the
sync/zstackio/ZSV-11833@@2
branch
from
c89c44c to
893727d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java (3)
441-441: 修复代码风格：双空格。
if语句中有多余的空格。♻️ 建议修改
- if (!accountThirdPartyAccountSourceRef.isSuccess()) { + if (!accountThirdPartyAccountSourceRef.isSuccess()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java` at line 441, Remove the extra space between the if keyword and the opening parenthesis in the conditional at LdapManagerImpl (the line using if (!accountThirdPartyAccountSourceRef.isSuccess())). Update it to use standard Java style: if (!accountThirdPartyAccountSourceRef.isSuccess()) so the code follows proper spacing conventions.
411-429: 参数ldapLoginPassword未被使用。方法签名中的
ldapLoginPassword参数在方法体内未被使用。如果是为了将来扩展或保持 API 一致性而保留，请添加注释说明；否则建议移除该参数以避免混淆。♻️ 建议修改
- public ErrorableValue<AccountThirdPartyAccountSourceRefVO> findAccountThirdPartyAccountSourceRefByName(String ldapLoginName, String ldapLoginPassword) { + public ErrorableValue<AccountThirdPartyAccountSourceRefVO> findAccountThirdPartyAccountSourceRefByName(String ldapLoginName) {相应地，调用处也需要同步更新。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java` around lines 411 - 429, The parameter ldapLoginPassword in findAccountThirdPartyAccountSourceRefByName is unused; either remove it from the method signature and update all callers, or if it must remain for API compatibility, document it explicitly and avoid warnings (e.g., add a comment or annotate as unused) and, if appropriate, pass it into createDriver().getFullUserDn or other auth logic in LdapManagerImpl to reflect intended use; locate the method by name and update references (call sites) or add the explanatory comment/unused annotation next to ldapLoginPassword to make intent clear.
431-444: 同一认证流程中存在两次 LDAP 服务器查询。
isValid()(Line 433) 和findAccountThirdPartyAccountSourceRefByName()(Line 439) 各自独立调用findCurrentLdapServer()。虽然全局配置在请求期间变更的概率较低，但严格来说，如果两次调用之间配置发生变化，可能导致密码校验与账号绑定查询使用不同的 LDAP 服务器。建议将
LdapServerVO实例传递给isValid和findAccountThirdPartyAccountSourceRefByName，确保同一请求中使用同一服务器快照。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java` around lines 431 - 444, The login method currently calls findCurrentLdapServer() twice via isValid(...) and findAccountThirdPartyAccountSourceRefByName(...), risking inconsistent server snapshots; change login(LoginContext, ReturnValueCompletion) to call findCurrentLdapServer() once (capture the LdapServerVO) and pass that LdapServerVO into isValid(...) and findAccountThirdPartyAccountSourceRefByName(...) (or add overloads that accept an LdapServerVO) so both credential validation and account binding lookup use the same server instance; update the signatures of isValid and/or findAccountThirdPartyAccountSourceRefByName to accept LdapServerVO and use that instead of calling findCurrentLdapServer() internally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java`:
- Around line 502-504: The null-check branch guarding
accountThirdPartyAccountSourceRef.result in LdapManagerImpl is dead because
findAccountThirdPartyAccountSourceRefByName() returns an ErrorableValue with an
error rather than a successful value containing null; remove the unreachable "if
(accountThirdPartyAccountSourceRef.result == null) return;" branch, or if you
prefer to keep it for defensive clarity, add a `@SuppressWarnings` annotation on
the enclosing method and a one-line comment referencing
findAccountThirdPartyAccountSourceRefByName()’s ErrorableValue behavior
explaining why the null-check is unnecessary.
---
Nitpick comments:
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java`:
- Line 441: Remove the extra space between the if keyword and the opening
parenthesis in the conditional at LdapManagerImpl (the line using if
(!accountThirdPartyAccountSourceRef.isSuccess())). Update it to use standard
Java style: if (!accountThirdPartyAccountSourceRef.isSuccess()) so the code
follows proper spacing conventions.
- Around line 411-429: The parameter ldapLoginPassword in
findAccountThirdPartyAccountSourceRefByName is unused; either remove it from the
method signature and update all callers, or if it must remain for API
compatibility, document it explicitly and avoid warnings (e.g., add a comment or
annotate as unused) and, if appropriate, pass it into
createDriver().getFullUserDn or other auth logic in LdapManagerImpl to reflect
intended use; locate the method by name and update references (call sites) or
add the explanatory comment/unused annotation next to ldapLoginPassword to make
intent clear.
- Around line 431-444: The login method currently calls findCurrentLdapServer()
twice via isValid(...) and findAccountThirdPartyAccountSourceRefByName(...),
risking inconsistent server snapshots; change login(LoginContext,
ReturnValueCompletion) to call findCurrentLdapServer() once (capture the
LdapServerVO) and pass that LdapServerVO into isValid(...) and
findAccountThirdPartyAccountSourceRefByName(...) (or add overloads that accept
an LdapServerVO) so both credential validation and account binding lookup use
the same server instance; update the signatures of isValid and/or
findAccountThirdPartyAccountSourceRefByName to accept LdapServerVO and use that
instead of calling findCurrentLdapServer() internally.
🪄 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: e8e41b6c-2efa-45fa-a9ec-520d11324786
⛔ Files ignored due to path filters (1)
test/src/test/resources/springConfigXml/license.xmlis excluded by!**/*.xml
📒 Files selected for processing (4)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.javaplugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.javasdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.javasdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.java
🚧 Files skipped from review as they are similar to previous changes (3)
- plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.java
- sdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.java
- sdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.java
MatheMatrix
force-pushed
the
sync/zstackio/ZSV-11833@@2
branch
from
893727d to
2040660
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java (1)
410-416:⚠️ Potential issue | 🟠 Major同一次登录流程里不要两次读取“当前 LDAP”。
这里先通过
isValid()做一次密码校验，再在findAccountThirdPartyAccountSourceRefByName()里重新取一次当前 LDAP。若两次调用之间CURRENT_LDAP_SERVER_UUID被切换，密码校验和绑定账号解析就可能落到不同服务器，导致这次登录的认证结果与账号映射不一致。建议先取一次LdapServerVO，并在本次登录流程里复用同一个实例。🔧 建议修改
+ private ErrorableValue<AccountThirdPartyAccountSourceRefVO> findAccountThirdPartyAccountSourceRefByName(LdapServerVO ldap, String ldapLoginName) { + String dn = createDriver().getFullUserDn(ldap, ldap.getUsernameProperty(), ldapLoginName); + AccountThirdPartyAccountSourceRefVO vo = Q.New(AccountThirdPartyAccountSourceRefVO.class) + .eq(AccountThirdPartyAccountSourceRefVO_.credentials, dn) + .eq(AccountThirdPartyAccountSourceRefVO_.accountSourceUuid, ldap.getUuid()) + .find(); + if (vo == null) { + return ErrorableValue.ofErrorCode(err(IdentityErrors.AUTHENTICATION_ERROR, + "The ldapUid does not have a binding account.")); + } + return ErrorableValue.of(vo); + } + `@Override` public void login(LoginContext loginContext, ReturnValueCompletion<LoginSessionInfo> completion) { - if (!isValid(loginContext.getUsername(), loginContext.getPassword())) { + final ErrorableValue<LdapServerVO> currentLdapServer = findCurrentLdapServer(); + if (!currentLdapServer.isSuccess()) { + completion.fail(currentLdapServer.error); + return; + } + final LdapServerVO ldap = currentLdapServer.result; + + if (!createDriver().isValid(loginContext.getUsername(), loginContext.getPassword(), ldap)) { completion.fail(err(IdentityErrors.AUTHENTICATION_ERROR, "Login validation failed in LDAP")); return; } - final ErrorableValue<AccountThirdPartyAccountSourceRefVO> accountThirdPartyAccountSourceRef = findAccountThirdPartyAccountSourceRefByName(loginContext.getUsername(), loginContext.getPassword()); + final ErrorableValue<AccountThirdPartyAccountSourceRefVO> accountThirdPartyAccountSourceRef = + findAccountThirdPartyAccountSourceRefByName(ldap, loginContext.getUsername());Also applies to: 432-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java` around lines 410 - 416, 当前登录流程中出现两次读取“当前 LDAP”的问题：在调用 isValid() 做密码校验后又在 findAccountThirdPartyAccountSourceRefByName() 内重新调用 findCurrentLdapServer()，可能导致两次获取到不同的 LdapServerVO（CURRENT_LDAP_SERVER_UUID 被切换），从而使校验与账号映射不一致。修复方法是只在登录流程开始时获取一次 LdapServerVO（result 的 LdapServerVO），并在后续复用该实例；为此修改 findAccountThirdPartyAccountSourceRefByName(...) 的签名或调用方以接受一个 LdapServerVO 参数（或传入该实例作为额外参数），并在 isValid(...)、findAccountThirdPartyAccountSourceRefByName(...) 等使用到当前 LDAP 的地方都使用同一个 LdapServerVO 实例（注意同时修复在同文件的另一处 432-438 区域同样的重复读取问题）。
🧹 Nitpick comments (1)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java (1)
410-428: 把“用户名→绑定账号”解析抽成无密码 helper，避免继续复制查询逻辑。
findAccountThirdPartyAccountSourceRefByName()现在已经不做 LDAP 密码校验了，但签名仍然要求ldapLoginPassword。这会误导调用方，也让getAccountIdByName()只能再复制一遍 DN 和绑定账号查询逻辑。更稳妥的是抽一个只依赖LdapServerVO + username的 lookup 方法，login()前面单独做密码校验即可。♻️ 建议修改
+ private ErrorableValue<AccountThirdPartyAccountSourceRefVO> findAccountRefByUsername(LdapServerVO ldap, String username) { + String dn = createDriver().getFullUserDn(ldap, ldap.getUsernameProperty(), username); + AccountThirdPartyAccountSourceRefVO vo = Q.New(AccountThirdPartyAccountSourceRefVO.class) + .eq(AccountThirdPartyAccountSourceRefVO_.credentials, dn) + .eq(AccountThirdPartyAccountSourceRefVO_.accountSourceUuid, ldap.getUuid()) + .find(); + if (vo == null) { + return ErrorableValue.ofErrorCode(err(IdentityErrors.AUTHENTICATION_ERROR, + "The ldapUid does not have a binding account.")); + } + return ErrorableValue.of(vo); + } + public ErrorableValue<AccountThirdPartyAccountSourceRefVO> findAccountThirdPartyAccountSourceRefByName(String ldapLoginName, String ldapLoginPassword) { final ErrorableValue<LdapServerVO> currentLdapServer = findCurrentLdapServer(); if (!currentLdapServer.isSuccess()) { return ErrorableValue.ofErrorCode(currentLdapServer.error); } - final LdapServerVO ldap = currentLdapServer.result; - - String dn = createDriver().getFullUserDn(ldap, ldap.getUsernameProperty(), ldapLoginName); - AccountThirdPartyAccountSourceRefVO vo = Q.New(AccountThirdPartyAccountSourceRefVO.class) - .eq(AccountThirdPartyAccountSourceRefVO_.credentials, dn) - .eq(AccountThirdPartyAccountSourceRefVO_.accountSourceUuid, ldap.getUuid()) - .find(); - - if (vo == null) { - return ErrorableValue.ofErrorCode(err(IdentityErrors.AUTHENTICATION_ERROR, - "The ldapUid does not have a binding account.")); - } - return ErrorableValue.of(vo); + return findAccountRefByUsername(currentLdapServer.result, ldapLoginName); } `@Override` public String getAccountIdByName(String username) { final ErrorableValue<LdapServerVO> currentLdapServer = findCurrentLdapServer(); @@ - String dn = createDriver().getFullUserDn(ldap, ldap.getUsernameProperty(), username); - if (dn == null || dn.isEmpty()) { - return null; - } - - String accountUuid = Q.New(AccountThirdPartyAccountSourceRefVO.class) - .select(AccountThirdPartyAccountSourceRefVO_.accountUuid) - .eq(AccountThirdPartyAccountSourceRefVO_.credentials, dn) - .eq(AccountThirdPartyAccountSourceRefVO_.accountSourceUuid, ldap.getUuid()) - .findValue(); - return accountUuid; + ErrorableValue<AccountThirdPartyAccountSourceRefVO> ref = findAccountRefByUsername(ldap, username); + return ref.isSuccess() ? ref.result.getAccountUuid() : null; }As per coding guidelines, “命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途。”
Also applies to: 475-492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java` around lines 410 - 428, The method findAccountThirdPartyAccountSourceRefByName currently takes ldapLoginPassword but does not use it and forces callers (like getAccountIdByName) to duplicate DN lookup/query logic; refactor by introducing a password-less helper such as findAccountThirdPartyAccountSourceRefByUsername(LdapServerVO ldap, String username) or lookupAccountThirdPartyRefByUsername(LdapServerVO, String) that uses createDriver().getFullUserDn(...) and queries AccountThirdPartyAccountSourceRefVO, update findAccountThirdPartyAccountSourceRefByName to obtain the LdapServerVO via findCurrentLdapServer() and delegate to the new helper (removing the unused ldapLoginPassword parameter), and change callers (getAccountIdByName and login) to call the helper and perform password validation separately using createDriver().validatePassword(...) or equivalent; ensure error returns remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java`:
- Around line 410-416: 当前登录流程中出现两次读取“当前 LDAP”的问题：在调用 isValid() 做密码校验后又在
findAccountThirdPartyAccountSourceRefByName() 内重新调用
findCurrentLdapServer()，可能导致两次获取到不同的 LdapServerVO（CURRENT_LDAP_SERVER_UUID
被切换），从而使校验与账号映射不一致。修复方法是只在登录流程开始时获取一次 LdapServerVO（result 的
LdapServerVO），并在后续复用该实例；为此修改 findAccountThirdPartyAccountSourceRefByName(...)
的签名或调用方以接受一个 LdapServerVO 参数（或传入该实例作为额外参数），并在
isValid(...)、findAccountThirdPartyAccountSourceRefByName(...) 等使用到当前 LDAP
的地方都使用同一个 LdapServerVO 实例（注意同时修复在同文件的另一处 432-438 区域同样的重复读取问题）。
---
Nitpick comments:
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java`:
- Around line 410-428: The method findAccountThirdPartyAccountSourceRefByName
currently takes ldapLoginPassword but does not use it and forces callers (like
getAccountIdByName) to duplicate DN lookup/query logic; refactor by introducing
a password-less helper such as
findAccountThirdPartyAccountSourceRefByUsername(LdapServerVO ldap, String
username) or lookupAccountThirdPartyRefByUsername(LdapServerVO, String) that
uses createDriver().getFullUserDn(...) and queries
AccountThirdPartyAccountSourceRefVO, update
findAccountThirdPartyAccountSourceRefByName to obtain the LdapServerVO via
findCurrentLdapServer() and delegate to the new helper (removing the unused
ldapLoginPassword parameter), and change callers (getAccountIdByName and login)
to call the helper and perform password validation separately using
createDriver().validatePassword(...) or equivalent; ensure error returns remain
identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78648d1b-4e97-4fb2-8fe6-19d4f43dde46
⛔ Files ignored due to path filters (1)
test/src/test/resources/springConfigXml/license.xmlis excluded by!**/*.xml
📒 Files selected for processing (4)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.javaplugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.javasdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.javasdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.java
🚧 Files skipped from review as they are similar to previous changes (3)
- plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.java
- sdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.java
- sdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.java
MatheMatrix
force-pushed
the
sync/zstackio/ZSV-11833@@2
branch
from
2040660 to
b43b4d5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java (1)
431-443:⚠️ Potential issue | 🟠 Major同一次登录链路里仍然会读取两次“当前 LDAP”。
Line 432 先通过
findAccountThirdPartyAccountSourceRefByName(...)解析了一次当前 LDAP，Line 439 又通过isValid(...)再取一次。如果这两步之间全局当前 LDAP 被切换，绑定查询和口令校验就可能落到不同服务器上，最终把 A 服务器的账号映射和 B 服务器的认证结果拼在一起。这里最好在login()开头只解析一次LdapServerVO，后续查绑定和isValid都复用同一个快照。🔧 建议修改
+ private ErrorableValue<AccountThirdPartyAccountSourceRefVO> findAccountThirdPartyAccountSourceRefByName(LdapServerVO ldap, String ldapLoginName) { + String dn = createDriver().getFullUserDn(ldap, ldap.getUsernameProperty(), ldapLoginName); + AccountThirdPartyAccountSourceRefVO vo = Q.New(AccountThirdPartyAccountSourceRefVO.class) + .eq(AccountThirdPartyAccountSourceRefVO_.credentials, dn) + .eq(AccountThirdPartyAccountSourceRefVO_.accountSourceUuid, ldap.getUuid()) + .find(); + if (vo == null) { + return ErrorableValue.ofErrorCode(err(IdentityErrors.AUTHENTICATION_ERROR, + "The ldapUid does not have a binding account.")); + } + return ErrorableValue.of(vo); + } + `@Override` public void login(LoginContext loginContext, ReturnValueCompletion<LoginSessionInfo> completion) { - final ErrorableValue<AccountThirdPartyAccountSourceRefVO> accountThirdPartyAccountSourceRef = findAccountThirdPartyAccountSourceRefByName(loginContext.getUsername(), loginContext.getPassword()); + final ErrorableValue<LdapServerVO> currentLdapServer = findCurrentLdapServer(); + if (!currentLdapServer.isSuccess()) { + completion.fail(currentLdapServer.error); + return; + } + + final LdapServerVO ldap = currentLdapServer.result; + final ErrorableValue<AccountThirdPartyAccountSourceRefVO> accountThirdPartyAccountSourceRef = + findAccountThirdPartyAccountSourceRefByName(ldap, loginContext.getUsername()); - if (!accountThirdPartyAccountSourceRef.isSuccess()) { + if (!accountThirdPartyAccountSourceRef.isSuccess()) { completion.fail(accountThirdPartyAccountSourceRef.error); return; } - if (!isValid(loginContext.getUsername(), loginContext.getPassword())) { + if (!createDriver().isValid(loginContext.getUsername(), loginContext.getPassword(), ldap)) { completion.fail(err(IdentityErrors.AUTHENTICATION_ERROR, "Login validation failed in LDAP")); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java` around lines 431 - 443, The login method currently resolves the current LDAP twice (via findAccountThirdPartyAccountSourceRefByName(...) and again inside isValid(...)), which can race if the global "current LDAP" flips between calls; change login(LoginContext, ReturnValueCompletion) to resolve the LdapServerVO once at the start (use the result from findAccountThirdPartyAccountSourceRefByName(...) / accountThirdPartyAccountSourceRef), store that snapshot, and pass it into subsequent operations (binding lookup and password validation) so both the binding lookup and isValid use the same LdapServerVO instance; if needed add an overload or helper that accepts the resolved LdapServerVO to avoid re-reading the global current server.
🧹 Nitpick comments (1)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java (1)
410-428: 这个辅助方法的签名现在有误导性。
ldapLoginPassword在这里没有任何用途，但这个 public 方法的签名会让调用方误以为它顺带做了 LDAP 凭证校验。既然它现在只负责“登录名 → 绑定账号”的解析，建议去掉这个参数，或者至少补一个不带密码参数的 lookup-only 重载，并把校验责任明确留在login()/authenticate()。 As per coding guidelines, "命名应尽量用完整的单词组合表达意图" 且 "代码应尽量做到自解释"。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java` around lines 410 - 428, The method findAccountThirdPartyAccountSourceRefByName currently accepts ldapLoginPassword but never uses it; update the API to avoid misleading callers by either removing the unused ldapLoginPassword parameter from findAccountThirdPartyAccountSourceRefByName or add an overload findAccountThirdPartyAccountSourceRefByName(String ldapLoginName) that is explicitly lookup-only; then update all callers to use the no-password variant and ensure any credential validation remains in the login()/authenticate() flow (keep createDriver().getFullUserDn(…) and the Q.New(...) lookup logic unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java`:
- Around line 431-443: The login method currently resolves the current LDAP
twice (via findAccountThirdPartyAccountSourceRefByName(...) and again inside
isValid(...)), which can race if the global "current LDAP" flips between calls;
change login(LoginContext, ReturnValueCompletion) to resolve the LdapServerVO
once at the start (use the result from
findAccountThirdPartyAccountSourceRefByName(...) /
accountThirdPartyAccountSourceRef), store that snapshot, and pass it into
subsequent operations (binding lookup and password validation) so both the
binding lookup and isValid use the same LdapServerVO instance; if needed add an
overload or helper that accepts the resolved LdapServerVO to avoid re-reading
the global current server.
---
Nitpick comments:
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java`:
- Around line 410-428: The method findAccountThirdPartyAccountSourceRefByName
currently accepts ldapLoginPassword but never uses it; update the API to avoid
misleading callers by either removing the unused ldapLoginPassword parameter
from findAccountThirdPartyAccountSourceRefByName or add an overload
findAccountThirdPartyAccountSourceRefByName(String ldapLoginName) that is
explicitly lookup-only; then update all callers to use the no-password variant
and ensure any credential validation remains in the login()/authenticate() flow
(keep createDriver().getFullUserDn(…) and the Q.New(...) lookup logic
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 94e1878e-e75c-4585-beb2-17115182bcee
⛔ Files ignored due to path filters (1)
test/src/test/resources/springConfigXml/license.xmlis excluded by!**/*.xml
📒 Files selected for processing (4)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.javaplugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.javasdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.javasdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.java
🚧 Files skipped from review as they are similar to previous changes (3)
- sdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.java
- plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.java
- sdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.java
…ication support 1. Extract common method findAccountThirdPartyAccountSourceRefByName - Extract the logic of finding third-party account source reference from login method into a standalone method - Unify account lookup logic in both login and collectUserInfoIntoContext methods 2. Add two-factor authentication support - getRequiredAdditionalAuthFeature method now returns a list containing both basicLoginControl and twoFactor - Support two-factor authentication for LDAP login Resolves: ZSV-11833 Change-Id: I7578626465676e6569686a786271786b64667179
MatheMatrix
force-pushed
the
sync/zstackio/ZSV-11833@@2
branch
from
b43b4d5 to
82bc126
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java (1)
410-427:⚠️ Potential issue | 🟠 Major同一次登录流程里仍然可能跨两个 LDAP 服务器快照。
这里先在
findAccountThirdPartyAccountSourceRefByName()里取一次当前 LDAP 并完成 DN/绑定查询，随后 Line 439 又通过isValid()再取一次当前 LDAP。若这两步之间“当前 LDAP”被切换，绑定账号和密码校验就可能落到不同服务器，认证结果会不一致。建议把同一个LdapServerVO快照贯穿整个登录流程。🔧 建议方向
- final ErrorableValue<AccountThirdPartyAccountSourceRefVO> accountThirdPartyAccountSourceRef = findAccountThirdPartyAccountSourceRefByName(loginContext.getUsername(), loginContext.getPassword()); + final ErrorableValue<LdapServerVO> currentLdapServer = findCurrentLdapServer(); + if (!currentLdapServer.isSuccess()) { + completion.fail(currentLdapServer.error); + return; + } + final LdapServerVO ldap = currentLdapServer.result; + + final ErrorableValue<AccountThirdPartyAccountSourceRefVO> accountThirdPartyAccountSourceRef = + findAccountThirdPartyAccountSourceRefByName(ldap, loginContext.getUsername()); if (!accountThirdPartyAccountSourceRef.isSuccess()) { completion.fail(accountThirdPartyAccountSourceRef.error); return; } - if (!isValid(loginContext.getUsername(), loginContext.getPassword())) { + if (!createDriver().isValid(loginContext.getUsername(), loginContext.getPassword(), ldap)) { completion.fail(err(IdentityErrors.AUTHENTICATION_ERROR, "Login validation failed in LDAP")); return; }Also applies to: 431-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java` around lines 410 - 427, The current login flow can observe two different LdapServerVO snapshots because findAccountThirdPartyAccountSourceRefByName() calls findCurrentLdapServer() and later other code calls isValid() which re-reads the current LDAP; to fix, capture and pass a single LdapServerVO snapshot through the entire authentication flow instead of re-fetching: modify findAccountThirdPartyAccountSourceRefByName() (and callers) to accept an LdapServerVO parameter (or return both the VO and any needed values) so subsequent calls to isValid(), createDriver(), and credential checks use the same LdapServerVO instance; ensure methods like findCurrentLdapServer(), createDriver(), isValid(), and any call sites that currently re-read the server are updated to use the provided LdapServerVO to keep a consistent snapshot during one login attempt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java`:
- Around line 410-427: The current login flow can observe two different
LdapServerVO snapshots because findAccountThirdPartyAccountSourceRefByName()
calls findCurrentLdapServer() and later other code calls isValid() which
re-reads the current LDAP; to fix, capture and pass a single LdapServerVO
snapshot through the entire authentication flow instead of re-fetching: modify
findAccountThirdPartyAccountSourceRefByName() (and callers) to accept an
LdapServerVO parameter (or return both the VO and any needed values) so
subsequent calls to isValid(), createDriver(), and credential checks use the
same LdapServerVO instance; ensure methods like findCurrentLdapServer(),
createDriver(), isValid(), and any call sites that currently re-read the server
are updated to use the provided LdapServerVO to keep a consistent snapshot
during one login attempt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a4905365-2197-4896-8a9e-c6b5b18d5fd3
⛔ Files ignored due to path filters (1)
test/src/test/resources/springConfigXml/license.xmlis excluded by!**/*.xml
📒 Files selected for processing (6)
plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.javaplugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.javasdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.javasdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.javatestlib/src/main/java/org/zstack/testlib/identity/ldap/LdapDriverSimulator.groovytestlib/src/main/java/org/zstack/testlib/identity/ldap/LdapVirtualEndpointSpec.groovy
🚧 Files skipped from review as they are similar to previous changes (3)
- sdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.java
- plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.java
- sdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.java
Resolves: ZSV-11833 Change-Id: I696c6e746965616478786e6f6e6263636d766868
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.
Extract common method findAccountThirdPartyAccountSourceRefByName
Add two-factor authentication support
Resolves: ZSV-11833
Change-Id: I7578626465676e6569686a786271786b64667179
sync from gitlab !9594