Skip to content

<feature>[ldap]: refactor LDAP login logic and add two-factor authentication support #3725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ZStack-Robot wants to merge 2 commits into from
Closed

<feature>[ldap]: refactor LDAP login logic and add two-factor authentication support #3725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82bc126
<feature>[ldap]: refactor LDAP login logic and add two-factor authent…
Apr 11, 2026
09e649d
<test>[testlib]: fix unexecute test cases
Apr 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion plugin/ldap/src/main/java/org/zstack/ldap/LdapManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import org.zstack.core.Platform;
import org.zstack.header.errorcode.ErrorableValue;
import org.zstack.identity.imports.entity.AccountThirdPartyAccountSourceRefVO;
import org.zstack.ldap.driver.LdapUtil;
import org.zstack.ldap.entity.LdapServerVO;

Expand All @@ -10,7 +11,7 @@
*/
public interface LdapManager {
boolean isValid(String uid, String password);

ErrorableValue<AccountThirdPartyAccountSourceRefVO> findAccountThirdPartyAccountSourceRefByName(String ldapLoginName, String ldapLoginPassword);
ErrorableValue<String> findCurrentLdapServerUuid();
Comment on lines 12 to 15
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

给新接口方法补上 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 如适用）并覆盖边界/失败场景以避免语义漂移。

ErrorableValue<LdapServerVO> findCurrentLdapServer();

Expand Down
68 changes: 44 additions & 24 deletions plugin/ldap/src/main/java/org/zstack/ldap/LdapManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,44 +407,49 @@ public LoginType getLoginType() {
return loginType;
}

@Override
public void login(LoginContext loginContext, ReturnValueCompletion<LoginSessionInfo> completion) {
public ErrorableValue<AccountThirdPartyAccountSourceRefVO> findAccountThirdPartyAccountSourceRefByName(String ldapLoginName, String ldapLoginPassword) {
final ErrorableValue<LdapServerVO> currentLdapServer = findCurrentLdapServer();
if (!currentLdapServer.isSuccess()) {
logger.debug("failed to login by LDAP: failed to find current LdapServer: " + currentLdapServer.error.getDetails());
completion.fail(err(IdentityErrors.AUTHENTICATION_ERROR,
"Login validation failed in LDAP"));
return;
return ErrorableValue.ofErrorCode(currentLdapServer.error);
}
final LdapServerVO ldap = currentLdapServer.result;

String ldapLoginName = loginContext.getUsername();
if (!isValid(ldapLoginName, loginContext.getPassword())) {
completion.fail(err(IdentityErrors.AUTHENTICATION_ERROR,
"Login validation failed in LDAP"));
return;
}

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) {
completion.fail(err(IdentityErrors.AUTHENTICATION_ERROR,
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());

if (!accountThirdPartyAccountSourceRef.isSuccess()) {
completion.fail(accountThirdPartyAccountSourceRef.error);
return;
}

if (!isValid(loginContext.getUsername(), loginContext.getPassword())) {
completion.fail(err(IdentityErrors.AUTHENTICATION_ERROR,
"Login validation failed in LDAP"));
return;
}
String accountUuid = accountThirdPartyAccountSourceRef.result.getAccountUuid();
final AccountState state = Q.New(AccountVO.class)
.eq(AccountVO_.uuid, vo.getAccountUuid())
.eq(AccountVO_.uuid, accountUuid)
.select(AccountVO_.state)
.findValue();

if (state == null || state == AccountState.Staled) {
completion.fail(operr(
"Account[uuid:%s] Not Found!!!", vo.getAccountUuid()));
"Account[uuid:%s] Not Found!!!", accountUuid));
return;
}
if (state == AccountState.Disabled) {
Expand All @@ -453,7 +458,7 @@ public void login(LoginContext loginContext, ReturnValueCompletion<LoginSessionI
}

LoginSessionInfo info = new LoginSessionInfo();
info.setAccountUuid(vo.getAccountUuid());
info.setAccountUuid(accountUuid);
completion.success(info);
}

Expand All @@ -468,21 +473,36 @@ public boolean authenticate(String username, String password) {

@Override
public String getAccountIdByName(String username) {
final ErrorableValue<LdapServerVO> property = findCurrentLdapServer();
if (property.isSuccess()) {
final String fullUserDn = createDriver().getFullUserDn(property.result, username);
return "".equals(fullUserDn) ? null : fullUserDn;
final ErrorableValue<LdapServerVO> currentLdapServer = findCurrentLdapServer();
if (!currentLdapServer.isSuccess()) {
return null;
}
final LdapServerVO ldap = currentLdapServer.result;

String dn = createDriver().getFullUserDn(ldap, ldap.getUsernameProperty(), username);
if (dn == null || dn.isEmpty()) {
return null;
}
return null;

String accountUuid = Q.New(AccountThirdPartyAccountSourceRefVO.class)
.select(AccountThirdPartyAccountSourceRefVO_.accountUuid)
.eq(AccountThirdPartyAccountSourceRefVO_.credentials, dn)
.eq(AccountThirdPartyAccountSourceRefVO_.accountSourceUuid, ldap.getUuid())
.findValue();
return accountUuid;
}

@Override
public void collectUserInfoIntoContext(LoginContext loginContext) {
loginContext.setAccountUuid(getAccountIdByName(loginContext.getUsername()));
ErrorableValue<AccountThirdPartyAccountSourceRefVO> accountThirdPartyAccountSourceRef = findAccountThirdPartyAccountSourceRefByName(loginContext.getUsername(), loginContext.getPassword());
if (!accountThirdPartyAccountSourceRef.isSuccess()) {
return;
}
loginContext.setAccountUuid(accountThirdPartyAccountSourceRef.result.getAccountUuid());
}

@Override
public List<AdditionalAuthFeature> getRequiredAdditionalAuthFeature() {
return Collections.singletonList(LoginAuthConstant.basicLoginControl);
return Arrays.asList(LoginAuthConstant.basicLoginControl, LoginAuthConstant.twoFactor);
}
}
3 changes: 3 additions & 0 deletions sdk/src/main/java/org/zstack/sdk/GetTwoFactorAuthenticationSecretAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ public Result throwExceptionIfError() {
@Param(required = false, nonempty = false, nullElements = false, emptyString = true, noTrim = false)
public java.lang.String verifyCode;

@Param(required = true, validValues = {"account","ldap"}, nonempty = true, nullElements = false, emptyString = true, noTrim = false)
public java.lang.String type = "account";

@Param(required = false)
public java.util.List systemTags;

Expand Down
3 changes: 3 additions & 0 deletions sdk/src/main/java/org/zstack/sdk/ResetTwoFactorAuthenticationSecretAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ public Result throwExceptionIfError() {
@Param(required = false, nonempty = false, nullElements = false, emptyString = true, noTrim = false)
public java.lang.String verifyCode;

@Param(required = true, validValues = {"account","ldap"}, nonempty = true, nullElements = false, emptyString = true, noTrim = false)
public java.lang.String type = "account";

@Param(required = false)
public java.util.List systemTags;

Expand Down
2 changes: 1 addition & 1 deletion test/src/test/resources/springConfigXml/Kvm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@
<zstack:extension interface="org.zstack.header.vm.VmInstanceMigrateExtensionPoint"/>
</zstack:plugin>
</bean>
<bean id="HypervisorMetadataCollector" class="org.zstack.kvm.hypervisor.HypervisorMetadataCollectorForTest">
<bean id="HypervisorMetadataCollector" class="org.zstack.kvm.hypervisor.HypervisorMetadataCollectorSimulator">
</bean>

<bean id="KvmHypervisorZQLExtension" class="org.zstack.kvm.hypervisor.KvmHypervisorZQLExtension">
Expand Down
20 changes: 0 additions & 20 deletions test/src/test/resources/springConfigXml/license.xml
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions ...r/HypervisorMetadataCollectorForTest.java → ...HypervisorMetadataCollectorSimulator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
*
* Created by Wenhao.Zhang on 23/03/01
*/
public class HypervisorMetadataCollectorForTest extends HypervisorMetadataCollectorImpl {
public class HypervisorMetadataCollectorSimulator extends HypervisorMetadataCollectorImpl {
public static final String QEMU_VERSION_FOR_TEST = "4.2.0-632.g6a6222b.el7";
private Function<Path, List<HypervisorMetadataDefinition>> folderScannerSimulator;
private Function<HypervisorMetadataDefinition, String> collectMetadataSimulator;
Expand Down Expand Up @@ -41,11 +41,11 @@ protected List<HypervisorMetadataDefinition> scanFolder(Path rootPath) throws IO
}

public List<HypervisorMetadataDefinition> scanFolderForDefault(Path rootPath) {
HypervisorMetadataDefinitionForTest d1 = new HypervisorMetadataDefinitionForTest();
HypervisorMetadataDefinitionSimulator d1 = new HypervisorMetadataDefinitionSimulator();
d1.setArchitecture(CpuArchitecture.x86_64.name());
d1.setOsReleaseSimpleVersion("c76");

HypervisorMetadataDefinitionForTest d2 = new HypervisorMetadataDefinitionForTest();
HypervisorMetadataDefinitionSimulator d2 = new HypervisorMetadataDefinitionSimulator();
d2.setArchitecture(CpuArchitecture.x86_64.name());
d2.setOsReleaseSimpleVersion("c79");

Expand Down Expand Up @@ -74,7 +74,7 @@ public String collectMetadataForDefault(HypervisorMetadataDefinition definition)
QEMU_VERSION_FOR_TEST, osReleaseVersion[0], osReleaseVersion[1], osReleaseVersion[2]);
}

public static class HypervisorMetadataDefinitionForTest extends HypervisorMetadataDefinition {
public static class HypervisorMetadataDefinitionSimulator extends HypervisorMetadataDefinition {
@Override
public boolean isValid() {
return true;
Expand Down
2 changes: 1 addition & 1 deletion ...ib/identity/ldap/LdapDriverForTest.groovy → .../identity/ldap/LdapDriverSimulator.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import org.zstack.ldap.entity.LdapServerVO_
import java.util.function.Function
import java.util.function.Supplier

class LdapDriverForTest extends LdapUtil {
class LdapDriverSimulator extends LdapUtil {
public Supplier<List<LdapVirtualEndpointSpec>> findAllEndpointsFunction
public Function<LdapServerVO, LdapVirtualEndpointSpec> findEndpointByLdapVOFunction

Expand Down
2 changes: 1 addition & 1 deletion testlib/src/main/java/org/zstack/testlib/identity/ldap/LdapVirtualEndpointSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class LdapVirtualEndpointSpec extends Spec {
endpointUuid = uuid

envSpec.mockFactory(LdapUtil.class) {
def driver = new LdapDriverForTest()
def driver = new LdapDriverSimulator()
driver.findAllEndpointsFunction = { findAllEndpoints() }
driver.findEndpointByLdapVOFunction = { LdapServerVO ldap -> findEndpointByLdapVO(ldap) }
return driver
Expand Down
8 changes: 4 additions & 4 deletions ...ack/testlib/vops/VOpsClientForTest.groovy → ...k/testlib/vops/VOpsClientSimulator.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ import java.util.function.Function

import static org.zstack.externalservice.vops.VOpsCommands.*;

class VOpsClientForTest extends VOpsClient {
class VOpsClientSimulator extends VOpsClient {
public final VOpsVirtualEndpointSpec parent

VOpsClientForTest(VOpsVirtualEndpointSpec parent) {
VOpsClientSimulator(VOpsVirtualEndpointSpec parent) {
this.parent = parent
}

Expand Down Expand Up @@ -75,9 +75,9 @@ class VOpsClientForTest extends VOpsClient {
}

static class HttpForTest<T> extends RestHttp<T> {
final VOpsClientForTest client
final VOpsClientSimulator client

HttpForTest(Class<T> returnClass, VOpsClientForTest client) {
HttpForTest(Class<T> returnClass, VOpsClientSimulator client) {
super(returnClass)
this.client = client
this.errorCodeBuilder = { Exception e, http2 -> client.errorFacade.throwableToOperationError(e) }
Expand Down
14 changes: 7 additions & 7 deletions testlib/src/main/java/org/zstack/testlib/vops/VOpsVirtualEndpointSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class VOpsVirtualEndpointSpec extends Spec {

@Override
SpecID create(String uuid, String sessionId) {
mockFactory(VOpsClient.class, { return new VOpsClientForTest(this) })
mockFactory(VOpsClient.class, { return new VOpsClientSimulator(this) })
return id(endpointName, endpointUuid = uuid)
}

Expand All @@ -34,15 +34,15 @@ class VOpsVirtualEndpointSpec extends Spec {
Test.functionForMockTestObjectFactory.remove(VOpsClient.class)
}

public List<PostHandlerPair<VOpsClientForTest.HttpForTest, Object>> postHandlers = []
public List<PostHandlerPair<VOpsClientSimulator.HttpForTest, Object>> postHandlers = []

/**
* @return a function to remove this handler
*/
BooleanSupplier registerPostHttpHandler(
Predicate<VOpsClientForTest.HttpForTest> predicate,
BiFunction<VOpsClientForTest.HttpForTest, ErrorableValue<Object>, ErrorableValue<Object>> handler) {
def pair = new PostHandlerPair<VOpsClientForTest.HttpForTest, Object>(
Predicate<VOpsClientSimulator.HttpForTest> predicate,
BiFunction<VOpsClientSimulator.HttpForTest, ErrorableValue<Object>, ErrorableValue<Object>> handler) {
def pair = new PostHandlerPair<VOpsClientSimulator.HttpForTest, Object>(
Objects.requireNonNull(predicate),
Objects.requireNonNull(handler))

Expand All @@ -55,7 +55,7 @@ class VOpsVirtualEndpointSpec extends Spec {
*/
BooleanSupplier registerPostHttpHandler(
String path,
BiFunction<VOpsClientForTest.HttpForTest, ErrorableValue<Object>, ErrorableValue<Object>> handler) {
BiFunction<VOpsClientSimulator.HttpForTest, ErrorableValue<Object>, ErrorableValue<Object>> handler) {
return registerPostHttpHandler({ it.path == path || it.pathWithoutIpAndPort == path }, handler)
}

Expand All @@ -65,7 +65,7 @@ class VOpsVirtualEndpointSpec extends Spec {
BooleanSupplier registerPostHttpHandler(
String path,
HttpMethod method,
BiFunction<VOpsClientForTest.HttpForTest, ErrorableValue<Object>, ErrorableValue<Object>> handler) {
BiFunction<VOpsClientSimulator.HttpForTest, ErrorableValue<Object>, ErrorableValue<Object>> handler) {
return registerPostHttpHandler(
{
(it.path == path || it.pathWithoutIpAndPort == path) && it.method == method
Expand Down