mirror of
https://github.com/iflytek/skillhub.git
synced 2026-10-09 03:17:52 +00:00
fix(auth): 加固 LDAP 并发首登、空密码与 TLS 配置边界
- 并发首登同一 LDAP subject 时，唯一约束冲突经 REQUIRES_NEW 子事务回滚后按 subject 重查命中既有账号，双方登录均成功且不重复建号 - null/空密码在 authenticateLdap 显式归类 401，避免 Hashtable NPE 导致 500 - 已绑定用户 email 变更增加碰撞检查（排除自身账号），与首登 409 规则一致 - LDAPS 支持自定义 truststore 配置（tls-trust-store*），企业自签 CA 可用 - 属性名白名单补 displayNameFallbackAttribute；LDAP 配置启动期校验（url/base/超时） - LDAP 登录成功与开始日志降为 debug - 新增并发首登集成测试（真实 OpenLDAP）、空密码 401 用例、email 刷新碰撞单测、truststore env 单测、LdapProperties 校验单测 Signed-off-by: jangrui <admin@jangrui.com>
This commit is contained in:
jangrui
parent
f8ddd2fdc4
commit
70dcc9d5f6
9 changed files with 458 additions and 34 deletions
|
|
@ -121,6 +121,11 @@ skillhub:
|
|||
email-attribute: ${SKILLHUB_LDAP_EMAIL_ATTRIBUTE:mail}
|
||||
connect-timeout-millis: ${SKILLHUB_LDAP_CONNECT_TIMEOUT_MILLIS:5000}
|
||||
read-timeout-millis: ${SKILLHUB_LDAP_READ_TIMEOUT_MILLIS:10000}
|
||||
# Custom trust store for LDAPS certificate validation (internal/self-signed CAs).
|
||||
# Leave empty to use the JVM default trust store.
|
||||
tls-trust-store: ${SKILLHUB_LDAP_TLS_TRUST_STORE:}
|
||||
tls-trust-store-password: ${SKILLHUB_LDAP_TLS_TRUST_STORE_PASSWORD:}
|
||||
tls-trust-store-type: ${SKILLHUB_LDAP_TLS_TRUST_STORE_TYPE:JKS}
|
||||
public:
|
||||
base-url: ${SKILLHUB_PUBLIC_BASE_URL:}
|
||||
access-policy:
|
||||
|
|
|
|||
|
|
@ -0,0 +1,118 @@
|
|||
package com.iflytek.skillhub.auth.ldap;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
|
||||
import com.iflytek.skillhub.auth.local.LocalAuthService;
|
||||
import com.iflytek.skillhub.auth.rbac.PlatformPrincipal;
|
||||
import com.iflytek.skillhub.auth.repository.IdentityBindingRepository;
|
||||
import com.iflytek.skillhub.domain.namespace.GlobalNamespaceMembershipService;
|
||||
import com.iflytek.skillhub.domain.user.UserAccountRepository;
|
||||
import java.util.concurrent.Callable;
|
||||
import java.util.concurrent.ExecutorService;
|
||||
import java.util.concurrent.Executors;
|
||||
import java.util.concurrent.Future;
|
||||
import org.junit.jupiter.api.BeforeAll;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.boot.test.context.SpringBootTest;
|
||||
import org.springframework.boot.test.mock.mockito.MockBean;
|
||||
import org.springframework.test.context.ActiveProfiles;
|
||||
import org.springframework.test.context.DynamicPropertyRegistry;
|
||||
import org.springframework.test.context.DynamicPropertySource;
|
||||
import org.testcontainers.containers.Container.ExecResult;
|
||||
import org.testcontainers.containers.GenericContainer;
|
||||
import org.testcontainers.junit.jupiter.Container;
|
||||
import org.testcontainers.junit.jupiter.Testcontainers;
|
||||
import org.testcontainers.utility.MountableFile;
|
||||
|
||||
/**
|
||||
* Concurrency coverage for LDAP first-login provisioning: two simultaneous first logins for the
|
||||
* same LDAP subject must resolve to a single account and a single identity binding. One login
|
||||
* wins the {@code (provider_code, subject)} unique-constraint race; the loser re-resolves the
|
||||
* existing identity in a fresh transaction and still succeeds.
|
||||
*/
|
||||
@SpringBootTest
|
||||
@ActiveProfiles("test")
|
||||
@Testcontainers(disabledWithoutDocker = true)
|
||||
class ConcurrentLdapFirstLoginTest {
|
||||
|
||||
private static final String BASE_DN = "dc=example,dc=org";
|
||||
private static final String BIND_DN = "cn=admin," + BASE_DN;
|
||||
|
||||
@Container
|
||||
static final GenericContainer<?> LDAP = new GenericContainer<>("osixia/openldap:1.5.0")
|
||||
.withEnv("LDAP_ORGANISATION", "Example Inc")
|
||||
.withEnv("LDAP_DOMAIN", "example.org")
|
||||
.withEnv("LDAP_ADMIN_PASSWORD", "admin")
|
||||
.withExposedPorts(389);
|
||||
|
||||
@DynamicPropertySource
|
||||
static void ldapProperties(DynamicPropertyRegistry registry) {
|
||||
registry.add("skillhub.ldap.enabled", () -> "true");
|
||||
registry.add("skillhub.ldap.url", () -> "ldap://" + LDAP.getHost() + ":" + LDAP.getMappedPort(389));
|
||||
registry.add("skillhub.ldap.base", () -> BASE_DN);
|
||||
registry.add("skillhub.ldap.username", () -> BIND_DN);
|
||||
registry.add("skillhub.ldap.password", () -> "admin");
|
||||
}
|
||||
|
||||
@Autowired
|
||||
private LocalAuthService localAuthService;
|
||||
|
||||
@Autowired
|
||||
private UserAccountRepository userAccountRepository;
|
||||
|
||||
@Autowired
|
||||
private IdentityBindingRepository identityBindingRepository;
|
||||
|
||||
// Namespace seeding is unrelated to the concurrency behavior under test.
|
||||
@MockBean
|
||||
private GlobalNamespaceMembershipService globalNamespaceMembershipService;
|
||||
|
||||
@BeforeAll
|
||||
static void seedDirectory() throws Exception {
|
||||
LDAP.copyFileToContainer(MountableFile.forClasspathResource("ldap/seed-users.ldif"), "/tmp/seed-users.ldif");
|
||||
awaitLdapReady();
|
||||
ExecResult add = LDAP.execInContainer("ldapadd", "-x", "-H", "ldap://localhost",
|
||||
"-D", BIND_DN, "-w", "admin", "-f", "/tmp/seed-users.ldif");
|
||||
assertThat(add.getExitCode())
|
||||
.as("ldapadd failed: %s", add.getStdout() + add.getStderr())
|
||||
.isZero();
|
||||
}
|
||||
|
||||
private static void awaitLdapReady() throws Exception {
|
||||
long deadline = System.currentTimeMillis() + 30_000;
|
||||
while (System.currentTimeMillis() < deadline) {
|
||||
try {
|
||||
ExecResult r = LDAP.execInContainer("ldapsearch", "-x", "-H", "ldap://localhost",
|
||||
"-b", BASE_DN, "-D", BIND_DN, "-w", "admin", "(objectClass=*)", "dn");
|
||||
if (r.getExitCode() == 0) {
|
||||
return;
|
||||
}
|
||||
} catch (Exception ignored) {
|
||||
}
|
||||
Thread.sleep(500);
|
||||
}
|
||||
throw new IllegalStateException("OpenLDAP did not become ready");
|
||||
}
|
||||
|
||||
@Test
|
||||
void concurrentFirstLogin_sameSubject_singleAccountAndBothSucceed() throws Exception {
|
||||
ExecutorService pool = Executors.newFixedThreadPool(2);
|
||||
try {
|
||||
Callable<PlatformPrincipal> login = () -> localAuthService.login("alice", "alice123");
|
||||
Future<PlatformPrincipal> first = pool.submit(login);
|
||||
Future<PlatformPrincipal> second = pool.submit(login);
|
||||
|
||||
PlatformPrincipal r1 = first.get();
|
||||
PlatformPrincipal r2 = second.get();
|
||||
|
||||
// Both concurrent first logins must succeed and resolve to the same account;
|
||||
// exactly one identity binding and one account exist afterwards.
|
||||
assertThat(r1.userId()).isEqualTo(r2.userId());
|
||||
assertThat(identityBindingRepository.findAll()).hasSize(1);
|
||||
assertThat(userAccountRepository.findByEmailIgnoreCase("alice@example.com")).isPresent();
|
||||
} finally {
|
||||
pool.shutdownNow();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -10,10 +10,12 @@ import com.iflytek.skillhub.auth.repository.IdentityBindingRepository;
|
|||
import com.iflytek.skillhub.auth.repository.UserRoleBindingRepository;
|
||||
import com.iflytek.skillhub.domain.namespace.GlobalNamespaceMembershipService;
|
||||
import com.iflytek.skillhub.domain.user.UserAccountRepository;
|
||||
import jakarta.persistence.EntityManager;
|
||||
import java.io.IOException;
|
||||
import java.net.ServerSocket;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.springframework.http.HttpStatus;
|
||||
import org.springframework.transaction.PlatformTransactionManager;
|
||||
|
||||
/**
|
||||
* Integration coverage for the "directory unavailable" error classification required by the
|
||||
|
|
@ -37,7 +39,9 @@ class LdapDirectoryUnavailableTest {
|
|||
mock(UserAccountRepository.class),
|
||||
mock(UserRoleBindingRepository.class),
|
||||
mock(GlobalNamespaceMembershipService.class),
|
||||
mock(IdentityBindingRepository.class));
|
||||
mock(IdentityBindingRepository.class),
|
||||
mock(EntityManager.class),
|
||||
mock(PlatformTransactionManager.class));
|
||||
|
||||
assertThatThrownBy(() -> svc.login("alice", "secret"))
|
||||
.isInstanceOf(AuthFlowException.class)
|
||||
|
|
|
|||
|
|
@ -14,6 +14,7 @@ import com.iflytek.skillhub.auth.repository.UserRoleBindingRepository;
|
|||
import com.iflytek.skillhub.domain.namespace.GlobalNamespaceMembershipService;
|
||||
import com.iflytek.skillhub.domain.user.UserAccount;
|
||||
import com.iflytek.skillhub.domain.user.UserAccountRepository;
|
||||
import jakarta.persistence.EntityManager;
|
||||
import java.util.Optional;
|
||||
import org.junit.jupiter.api.BeforeAll;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
|
@ -24,7 +25,7 @@ import org.springframework.http.HttpStatus;
|
|||
import org.springframework.test.context.ActiveProfiles;
|
||||
import org.springframework.test.context.DynamicPropertyRegistry;
|
||||
import org.springframework.test.context.DynamicPropertySource;
|
||||
import org.springframework.transaction.annotation.Transactional;
|
||||
import org.springframework.transaction.PlatformTransactionManager;
|
||||
import org.testcontainers.containers.Container.ExecResult;
|
||||
import org.testcontainers.containers.GenericContainer;
|
||||
import org.testcontainers.junit.jupiter.Container;
|
||||
|
|
@ -44,7 +45,6 @@ import org.testcontainers.utility.MountableFile;
|
|||
@SpringBootTest
|
||||
@ActiveProfiles("test")
|
||||
@Testcontainers(disabledWithoutDocker = true)
|
||||
@Transactional
|
||||
class LdapIntegrationTest {
|
||||
|
||||
private static final String BASE_DN = "dc=example,dc=org";
|
||||
|
|
@ -133,7 +133,7 @@ class LdapIntegrationTest {
|
|||
}
|
||||
|
||||
@Test
|
||||
void repeatLogin_returnsSameAccount_andDoesNotDuplicate() {
|
||||
void repeatLogin_returnsSameAccount_andDoesNotDuplicate() throws Exception {
|
||||
PlatformPrincipal first = localAuthService.login("alice", "alice123");
|
||||
PlatformPrincipal second = localAuthService.login("alice", "alice123");
|
||||
|
||||
|
|
@ -141,7 +141,11 @@ class LdapIntegrationTest {
|
|||
Optional<UserAccount> account = userAccountRepository.findByEmailIgnoreCase("alice@example.com");
|
||||
assertThat(account).isPresent();
|
||||
assertThat(account.get().getId()).isEqualTo(first.userId());
|
||||
assertThat(identityBindingRepository.findAll()).hasSize(1);
|
||||
// Exactly one binding exists for this subject; no duplicate provisioning happened.
|
||||
Optional<IdentityBinding> binding =
|
||||
identityBindingRepository.findByProviderCodeAndSubject("ldap", directoryEntryUuid("alice"));
|
||||
assertThat(binding).isPresent();
|
||||
assertThat(binding.get().getUserId()).isEqualTo(first.userId());
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -154,7 +158,7 @@ class LdapIntegrationTest {
|
|||
}
|
||||
|
||||
@Test
|
||||
void emailCollision_returnsConflict_andDoesNotTakeOverExistingAccount() {
|
||||
void emailCollision_returnsConflict_andDoesNotTakeOverExistingAccount() throws Exception {
|
||||
UserAccount local = new UserAccount("usr_local_carol", "Carol Local", "carol@example.com", null);
|
||||
userAccountRepository.save(local);
|
||||
|
||||
|
|
@ -170,7 +174,8 @@ class LdapIntegrationTest {
|
|||
Optional<UserAccount> untouched = userAccountRepository.findById(local.getId());
|
||||
assertThat(untouched).isPresent();
|
||||
assertThat(untouched.get().getDisplayName()).isEqualTo("Carol Local");
|
||||
assertThat(identityBindingRepository.findAll()).isEmpty();
|
||||
assertThat(identityBindingRepository.findByProviderCodeAndSubject("ldap", directoryEntryUuid("carol")))
|
||||
.isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -180,6 +185,16 @@ class LdapIntegrationTest {
|
|||
.satisfies(e -> assertThat(((AuthFlowException) e).getStatus()).isEqualTo(HttpStatus.UNAUTHORIZED));
|
||||
}
|
||||
|
||||
@Test
|
||||
void emptyPassword_returnsUnauthorized_notServerError() {
|
||||
// Direct service-level coverage: a null password must be classified as a credential
|
||||
// failure (401), never a NullPointerException/500. (The HTTP layer already rejects
|
||||
// blank passwords via @NotBlank; this guards service-level callers.)
|
||||
assertThatThrownBy(() -> localAuthService.login("alice", null))
|
||||
.isInstanceOf(AuthFlowException.class)
|
||||
.satisfies(e -> assertThat(((AuthFlowException) e).getStatus()).isEqualTo(HttpStatus.UNAUTHORIZED));
|
||||
}
|
||||
|
||||
@Test
|
||||
void attributeChanges_areRefreshedOnNextLogin() throws Exception {
|
||||
PlatformPrincipal before = localAuthService.login("dave", "dave123");
|
||||
|
|
@ -208,7 +223,9 @@ class LdapIntegrationTest {
|
|||
mock(UserAccountRepository.class),
|
||||
mock(UserRoleBindingRepository.class),
|
||||
mock(GlobalNamespaceMembershipService.class),
|
||||
mock(IdentityBindingRepository.class));
|
||||
mock(IdentityBindingRepository.class),
|
||||
mock(EntityManager.class),
|
||||
mock(PlatformTransactionManager.class));
|
||||
|
||||
assertThatThrownBy(() -> svc.login("alice", "alice123"))
|
||||
.isInstanceOf(AuthFlowException.class)
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ package com.iflytek.skillhub.auth.config;
|
|||
|
||||
import org.springframework.boot.context.properties.ConfigurationProperties;
|
||||
import org.springframework.stereotype.Component;
|
||||
import jakarta.annotation.PostConstruct;
|
||||
|
||||
/**
|
||||
* Configuration properties for LDAP authentication.
|
||||
|
|
@ -79,6 +80,42 @@ public class LdapProperties {
|
|||
*/
|
||||
private int readTimeoutMillis = 10000;
|
||||
|
||||
/**
|
||||
* Path to a custom trust store used for LDAPS certificate validation. When empty,
|
||||
* the JVM default trust store is used. Configure this for directories signed by
|
||||
* internal/self-signed CAs.
|
||||
*/
|
||||
private String tlsTrustStorePath = "";
|
||||
|
||||
/**
|
||||
* Password for the custom trust store. Only used when {@link #tlsTrustStorePath} is set.
|
||||
*/
|
||||
private String tlsTrustStorePassword = "";
|
||||
|
||||
/**
|
||||
* Trust store type (JKS, PKCS12). Defaults to JKS for compatibility.
|
||||
*/
|
||||
private String tlsTrustStoreType = "JKS";
|
||||
|
||||
@PostConstruct
|
||||
void validate() {
|
||||
if (!enabled) {
|
||||
return;
|
||||
}
|
||||
if (url == null || url.isBlank()) {
|
||||
throw new IllegalStateException("skillhub.ldap.url must be configured when LDAP is enabled");
|
||||
}
|
||||
if (base == null || base.isBlank()) {
|
||||
throw new IllegalStateException("skillhub.ldap.base must be configured when LDAP is enabled");
|
||||
}
|
||||
if (connectTimeoutMillis <= 0) {
|
||||
throw new IllegalStateException("skillhub.ldap.connect-timeout-millis must be a positive number");
|
||||
}
|
||||
if (readTimeoutMillis <= 0) {
|
||||
throw new IllegalStateException("skillhub.ldap.read-timeout-millis must be a positive number");
|
||||
}
|
||||
}
|
||||
|
||||
public boolean isEnabled() {
|
||||
return enabled;
|
||||
}
|
||||
|
|
@ -182,4 +219,28 @@ public class LdapProperties {
|
|||
public void setReadTimeoutMillis(int readTimeoutMillis) {
|
||||
this.readTimeoutMillis = readTimeoutMillis;
|
||||
}
|
||||
|
||||
public String getTlsTrustStorePath() {
|
||||
return tlsTrustStorePath;
|
||||
}
|
||||
|
||||
public void setTlsTrustStorePath(String tlsTrustStorePath) {
|
||||
this.tlsTrustStorePath = tlsTrustStorePath;
|
||||
}
|
||||
|
||||
public String getTlsTrustStorePassword() {
|
||||
return tlsTrustStorePassword;
|
||||
}
|
||||
|
||||
public void setTlsTrustStorePassword(String tlsTrustStorePassword) {
|
||||
this.tlsTrustStorePassword = tlsTrustStorePassword;
|
||||
}
|
||||
|
||||
public String getTlsTrustStoreType() {
|
||||
return tlsTrustStoreType;
|
||||
}
|
||||
|
||||
public void setTlsTrustStoreType(String tlsTrustStoreType) {
|
||||
this.tlsTrustStoreType = tlsTrustStoreType;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -13,9 +13,11 @@ import com.iflytek.skillhub.domain.user.UserAccountRepository;
|
|||
import com.iflytek.skillhub.domain.user.UserStatus;
|
||||
import java.security.cert.CertificateException;
|
||||
import java.util.Hashtable;
|
||||
import java.util.Locale;
|
||||
import java.util.Set;
|
||||
import java.util.UUID;
|
||||
import java.util.stream.Collectors;
|
||||
import jakarta.persistence.EntityManager;
|
||||
import javax.net.ssl.SSLException;
|
||||
import javax.naming.AuthenticationException;
|
||||
import javax.naming.CommunicationException;
|
||||
|
|
@ -31,10 +33,13 @@ import javax.naming.directory.SearchResult;
|
|||
import javax.naming.ldap.LdapName;
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
import org.springframework.dao.DataIntegrityViolationException;
|
||||
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
|
||||
import org.springframework.http.HttpStatus;
|
||||
import org.springframework.stereotype.Service;
|
||||
import org.springframework.transaction.annotation.Transactional;
|
||||
import org.springframework.transaction.PlatformTransactionManager;
|
||||
import org.springframework.transaction.support.TransactionTemplate;
|
||||
import org.springframework.transaction.TransactionDefinition;
|
||||
|
||||
/**
|
||||
* Handles LDAP authentication for enterprise directory integration.
|
||||
|
|
@ -55,22 +60,47 @@ public class LdapAuthService {
|
|||
// LDAP attribute names only allow ASCII letters, digits, and hyphens.
|
||||
private static final Pattern ATTRIBUTE_NAME_PATTERN = Pattern.compile("^[a-zA-Z][a-zA-Z0-9-]*$");
|
||||
|
||||
/**
|
||||
* Signals that a concurrent first login wrote the same LDAP subject binding first. The
|
||||
* provisioning (sub-)transaction has already been rolled back cleanly; callers must
|
||||
* re-resolve the identity by subject in a fresh transaction.
|
||||
*/
|
||||
private static final class LdapBindingRaceException extends RuntimeException {
|
||||
LdapBindingRaceException(Throwable cause) {
|
||||
super(cause);
|
||||
}
|
||||
}
|
||||
|
||||
private final LdapProperties ldapProperties;
|
||||
private final UserAccountRepository userAccountRepository;
|
||||
private final UserRoleBindingRepository userRoleBindingRepository;
|
||||
private final GlobalNamespaceMembershipService globalNamespaceMembershipService;
|
||||
private final IdentityBindingRepository identityBindingRepository;
|
||||
private final EntityManager entityManager;
|
||||
/**
|
||||
* Programmatic REQUIRES_NEW template for account/binding provisioning. A separate physical
|
||||
* transaction is required so a failed concurrent insert (which marks its transaction
|
||||
* rollback-only) can be rolled back cleanly and the identity re-resolved in a fresh
|
||||
* transaction. Spring's annotation-driven propagation cannot be used here because the
|
||||
* provisioning methods are invoked internally (self-invocation bypasses the proxy).
|
||||
*/
|
||||
private final TransactionTemplate ldapProvisioningTx;
|
||||
|
||||
public LdapAuthService(LdapProperties ldapProperties,
|
||||
UserAccountRepository userAccountRepository,
|
||||
UserRoleBindingRepository userRoleBindingRepository,
|
||||
GlobalNamespaceMembershipService globalNamespaceMembershipService,
|
||||
IdentityBindingRepository identityBindingRepository) {
|
||||
IdentityBindingRepository identityBindingRepository,
|
||||
EntityManager entityManager,
|
||||
PlatformTransactionManager transactionManager) {
|
||||
this.ldapProperties = ldapProperties;
|
||||
this.userAccountRepository = userAccountRepository;
|
||||
this.userRoleBindingRepository = userRoleBindingRepository;
|
||||
this.globalNamespaceMembershipService = globalNamespaceMembershipService;
|
||||
this.identityBindingRepository = identityBindingRepository;
|
||||
this.entityManager = entityManager;
|
||||
this.ldapProvisioningTx = new TransactionTemplate(transactionManager);
|
||||
this.ldapProvisioningTx.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -82,9 +112,8 @@ public class LdapAuthService {
|
|||
* @return PlatformPrincipal if authentication succeeds
|
||||
* @throws AuthFlowException if authentication fails
|
||||
*/
|
||||
@Transactional
|
||||
public PlatformPrincipal login(String username, String password) {
|
||||
log.info("Starting LDAP authentication for username: {}", username);
|
||||
log.debug("Starting LDAP authentication for username: {}", username);
|
||||
|
||||
if (!ldapProperties.isEnabled()) {
|
||||
log.warn("LDAP authentication is not enabled");
|
||||
|
|
@ -131,15 +160,23 @@ public class LdapAuthService {
|
|||
throw new AuthFlowException(HttpStatus.SERVICE_UNAVAILABLE, "error.auth.ldap.directoryUnavailable");
|
||||
}
|
||||
|
||||
// Find or create local user account anchored on the stable LDAP subject
|
||||
// Find or create local user account anchored on the stable LDAP subject. Account and
|
||||
// binding creation run in their own (sub-)transaction; a concurrent first login for the
|
||||
// same subject is recovered by re-resolving the identity in a fresh transaction.
|
||||
log.debug("Finding or creating local user account for username: {}", username);
|
||||
UserAccount user = findOrCreateLdapUser(username, userAttributes);
|
||||
UserAccount user;
|
||||
try {
|
||||
user = ldapProvisioningTx.execute(status -> findOrCreateLdapUser(username, userAttributes));
|
||||
} catch (LdapBindingRaceException e) {
|
||||
log.warn("Concurrent first login detected for LDAP subject of username {}; resolving existing account", username);
|
||||
user = ldapProvisioningTx.execute(status -> resolveReturningUser(userAttributes, username));
|
||||
}
|
||||
|
||||
// Check if user can login (status check)
|
||||
log.debug("Checking user status for user: {}, status: {}", username, user.getStatus());
|
||||
ensureUserCanLogin(user);
|
||||
|
||||
log.info("LDAP authentication successful for username: {}", username);
|
||||
log.debug("LDAP authentication successful for username: {}", username);
|
||||
return buildPrincipal(user);
|
||||
}
|
||||
|
||||
|
|
@ -230,6 +267,14 @@ public class LdapAuthService {
|
|||
consistent across search, bind, and attribute-read operations.
|
||||
*/
|
||||
private DirContext createLdapContext(String principal, String credentials) throws NamingException {
|
||||
return new InitialDirContext(buildJndiEnvironment(principal, credentials));
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds the JNDI environment for one LDAP operation. Package-private so tests can assert
|
||||
* connection/timeout/TLS settings without opening a real directory connection.
|
||||
*/
|
||||
Hashtable<String, String> buildJndiEnvironment(String principal, String credentials) {
|
||||
Hashtable<String, String> env = new Hashtable<>();
|
||||
env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
|
||||
env.put(Context.PROVIDER_URL, ldapProperties.getUrl());
|
||||
|
|
@ -244,7 +289,15 @@ public class LdapAuthService {
|
|||
env.put(Context.SECURITY_PRINCIPAL, bindPrincipal);
|
||||
env.put(Context.SECURITY_CREDENTIALS, bindCredentials);
|
||||
}
|
||||
return new InitialDirContext(env);
|
||||
// LDAPS certificate validation uses the JVM default trust store unless a custom store is
|
||||
// configured; this lets operators trust internal/self-signed directory CAs.
|
||||
String trustStorePath = ldapProperties.getTlsTrustStorePath();
|
||||
if (trustStorePath != null && !trustStorePath.isEmpty()) {
|
||||
env.put("javax.net.ssl.trustStore", trustStorePath);
|
||||
env.put("javax.net.ssl.trustStorePassword", ldapProperties.getTlsTrustStorePassword());
|
||||
env.put("javax.net.ssl.trustStoreType", ldapProperties.getTlsTrustStoreType());
|
||||
}
|
||||
return env;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -305,6 +358,13 @@ public class LdapAuthService {
|
|||
* Authenticates a user against the LDAP server using their DN and password.
|
||||
*/
|
||||
private boolean authenticateLdap(String userDn, String password) {
|
||||
// Reject null/empty passwords explicitly: a null value would otherwise reach
|
||||
// Hashtable.put (NPE -> 500), and an empty password could be accepted by directories
|
||||
// that allow anonymous/weak binds. Treat both as credential failures (401).
|
||||
if (password == null || password.isEmpty()) {
|
||||
log.debug("LDAP bind rejected: empty password for DN {}", userDn);
|
||||
return false;
|
||||
}
|
||||
DirContext ctx = null;
|
||||
try {
|
||||
// Bind as the authenticating user to verify credentials (shared factory handles env).
|
||||
|
|
@ -374,16 +434,10 @@ public class LdapAuthService {
|
|||
* <li>Duplicate accounts for email-less LDAP users on repeated logins</li>
|
||||
* </ul>
|
||||
*/
|
||||
private UserAccount findOrCreateLdapUser(String username, Attributes attributes) {
|
||||
UserAccount findOrCreateLdapUser(String username, Attributes attributes) {
|
||||
String subject = getAttributeValue(attributes, ldapProperties.getSubjectAttribute());
|
||||
String email = getAttributeValue(attributes, ldapProperties.getEmailAttribute());
|
||||
String displayName = getAttributeValue(attributes, ldapProperties.getDisplayNameAttribute());
|
||||
if (displayName == null || displayName.isEmpty()) {
|
||||
displayName = getAttributeValue(attributes, ldapProperties.getDisplayNameFallbackAttribute());
|
||||
}
|
||||
if (displayName == null || displayName.isEmpty()) {
|
||||
displayName = username;
|
||||
}
|
||||
String displayName = resolveDisplayName(attributes, username);
|
||||
|
||||
if (subject == null || subject.isEmpty()) {
|
||||
log.error("LDAP entry for {} has no stable subject attribute '{}'; cannot bind identity",
|
||||
|
|
@ -444,18 +498,72 @@ public class LdapAuthService {
|
|||
user = userAccountRepository.save(user);
|
||||
globalNamespaceMembershipService.ensureMember(user.getId());
|
||||
|
||||
IdentityBinding newBinding = new IdentityBinding(user.getId(), LDAP_PROVIDER, subject, username);
|
||||
identityBindingRepository.save(newBinding);
|
||||
try {
|
||||
IdentityBinding newBinding = new IdentityBinding(user.getId(), LDAP_PROVIDER, subject, username);
|
||||
// saveAndFlush surfaces the unique (provider_code, subject) constraint immediately, so
|
||||
// a concurrent first login for the same subject is detected here and the whole
|
||||
// sub-transaction (account + membership + binding) is rolled back.
|
||||
identityBindingRepository.saveAndFlush(newBinding);
|
||||
} catch (DataIntegrityViolationException e) {
|
||||
// A concurrent first login for the same subject committed its binding first. Clear the
|
||||
// failed persist state (otherwise Hibernate throws AssertionFailure while rolling back
|
||||
// the session), then signal the race so the identity is re-resolved in a fresh
|
||||
// transaction. The insert failure already marked this sub-transaction rollback-only,
|
||||
// so it can never be committed with the re-resolved state.
|
||||
entityManager.clear();
|
||||
throw new LdapBindingRaceException(e);
|
||||
}
|
||||
|
||||
return user;
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-resolves a returning LDAP user in a fresh transaction after a concurrent first-login
|
||||
* race. Called only when the racing transaction has committed its binding, so the lookup is
|
||||
* guaranteed to hit the existing account.
|
||||
*/
|
||||
UserAccount resolveReturningUser(Attributes attributes, String username) {
|
||||
String subject = getAttributeValue(attributes, ldapProperties.getSubjectAttribute());
|
||||
String email = getAttributeValue(attributes, ldapProperties.getEmailAttribute());
|
||||
String displayName = resolveDisplayName(attributes, username);
|
||||
IdentityBinding binding = identityBindingRepository
|
||||
.findByProviderCodeAndSubject(LDAP_PROVIDER, subject)
|
||||
.orElseThrow(() -> new IllegalStateException("No LDAP binding found after race for subject " + subject));
|
||||
UserAccount user = userAccountRepository.findById(binding.getUserId())
|
||||
.orElseThrow(() -> new IllegalStateException("User not found for LDAP binding " + subject));
|
||||
updateFromAttributes(user, displayName, email);
|
||||
return userAccountRepository.save(user);
|
||||
}
|
||||
|
||||
private String resolveDisplayName(Attributes attributes, String username) {
|
||||
String displayName = getAttributeValue(attributes, ldapProperties.getDisplayNameAttribute());
|
||||
if (displayName == null || displayName.isEmpty()) {
|
||||
displayName = getAttributeValue(attributes, ldapProperties.getDisplayNameFallbackAttribute());
|
||||
}
|
||||
if (displayName == null || displayName.isEmpty()) {
|
||||
displayName = username;
|
||||
}
|
||||
return displayName;
|
||||
}
|
||||
|
||||
private void updateFromAttributes(UserAccount user, String displayName, String email) {
|
||||
if (displayName != null && !displayName.isEmpty()) {
|
||||
user.setDisplayName(displayName);
|
||||
}
|
||||
if (email != null && !email.isEmpty()) {
|
||||
user.setEmail(email.toLowerCase());
|
||||
String normalizedEmail = email.toLowerCase(Locale.ROOT);
|
||||
// A bound user may update their own email, but must never silently adopt an email
|
||||
// that already belongs to a different account (same rule as first login).
|
||||
if (user.getEmail() == null || !user.getEmail().equalsIgnoreCase(normalizedEmail)) {
|
||||
UserAccount existingByEmail = userAccountRepository
|
||||
.findByEmailIgnoreCase(normalizedEmail).orElse(null);
|
||||
if (existingByEmail != null && !existingByEmail.getId().equals(user.getId())) {
|
||||
log.warn("LDAP user {} email {} collides with another account {} on refresh; refusing to update",
|
||||
user.getDisplayName(), normalizedEmail, existingByEmail.getId());
|
||||
throw new AuthFlowException(HttpStatus.CONFLICT, "error.auth.ldap.emailConflict");
|
||||
}
|
||||
}
|
||||
user.setEmail(normalizedEmail);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -549,6 +657,7 @@ public class LdapAuthService {
|
|||
ldapProperties.getUserSearchAttribute(),
|
||||
ldapProperties.getSubjectAttribute(),
|
||||
ldapProperties.getDisplayNameAttribute(),
|
||||
ldapProperties.getDisplayNameFallbackAttribute(),
|
||||
ldapProperties.getEmailAttribute()
|
||||
};
|
||||
for (String name : attrNames) {
|
||||
|
|
|
|||
|
|
@ -139,14 +139,14 @@ public class LocalAuthService {
|
|||
log.warn("LDAP is enabled but LdapAuthService bean is unavailable; rejecting login for username: {}", username);
|
||||
throw invalidCredentials();
|
||||
}
|
||||
log.info("Local user not found, attempting LDAP authentication for username: {}", username);
|
||||
log.debug("Local user not found, attempting LDAP authentication for username: {}", username);
|
||||
log.debug("LDAP enabled: {}, host: {}, base: {}",
|
||||
ldapProperties.isEnabled(),
|
||||
LdapAuthService.safeLogHost(ldapProperties.getUrl()),
|
||||
ldapProperties.getBase());
|
||||
try {
|
||||
PlatformPrincipal ldapPrincipal = ldapAuthService.login(username, password);
|
||||
log.info("LDAP authentication successful for username: {}", username);
|
||||
log.debug("LDAP authentication successful for username: {}", username);
|
||||
return ldapPrincipal;
|
||||
} catch (AuthFlowException e) {
|
||||
log.warn("LDAP authentication failed for username: {}, error: {}", username, e.getMessage());
|
||||
|
|
|
|||
|
|
@ -0,0 +1,41 @@
|
|||
package com.iflytek.skillhub.auth.config;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThatCode;
|
||||
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
/**
|
||||
* Startup configuration validation for the LDAP properties block.
|
||||
*/
|
||||
class LdapPropertiesTest {
|
||||
|
||||
@Test
|
||||
void validate_requiresUrlAndBase_whenEnabled() {
|
||||
LdapProperties props = new LdapProperties();
|
||||
props.setEnabled(true);
|
||||
|
||||
assertThatThrownBy(props::validate)
|
||||
.isInstanceOf(IllegalStateException.class)
|
||||
.hasMessageContaining("skillhub.ldap.url");
|
||||
}
|
||||
|
||||
@Test
|
||||
void validate_rejectsNonPositiveTimeouts_whenEnabled() {
|
||||
LdapProperties props = new LdapProperties();
|
||||
props.setEnabled(true);
|
||||
props.setUrl("ldap://localhost:389");
|
||||
props.setBase("dc=example,dc=org");
|
||||
props.setConnectTimeoutMillis(0);
|
||||
|
||||
assertThatThrownBy(props::validate)
|
||||
.isInstanceOf(IllegalStateException.class)
|
||||
.hasMessageContaining("connect-timeout-millis");
|
||||
}
|
||||
|
||||
@Test
|
||||
void validate_skipsChecks_whenDisabled() {
|
||||
LdapProperties props = new LdapProperties();
|
||||
assertThatCode(props::validate).doesNotThrowAnyException();
|
||||
}
|
||||
}
|
||||
|
|
@ -21,12 +21,14 @@ import com.iflytek.skillhub.auth.repository.IdentityBindingRepository;
|
|||
import com.iflytek.skillhub.auth.repository.UserRoleBindingRepository;
|
||||
import java.lang.reflect.Method;
|
||||
import java.util.Optional;
|
||||
import jakarta.persistence.EntityManager;
|
||||
import javax.naming.directory.Attributes;
|
||||
import javax.naming.directory.BasicAttribute;
|
||||
import javax.naming.directory.BasicAttributes;
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.springframework.http.HttpStatus;
|
||||
import org.springframework.transaction.PlatformTransactionManager;
|
||||
|
||||
/**
|
||||
* Behavior-level unit tests for {@link LdapAuthService} identity provisioning.
|
||||
|
|
@ -47,6 +49,8 @@ class LdapAuthServiceTest {
|
|||
private UserRoleBindingRepository userRoleBindingRepository;
|
||||
private GlobalNamespaceMembershipService globalNamespaceMembershipService;
|
||||
private IdentityBindingRepository identityBindingRepository;
|
||||
private EntityManager entityManager;
|
||||
private PlatformTransactionManager transactionManager;
|
||||
private LdapAuthService ldapAuthService;
|
||||
|
||||
@BeforeEach
|
||||
|
|
@ -56,12 +60,16 @@ class LdapAuthServiceTest {
|
|||
userRoleBindingRepository = mock(UserRoleBindingRepository.class);
|
||||
globalNamespaceMembershipService = mock(GlobalNamespaceMembershipService.class);
|
||||
identityBindingRepository = mock(IdentityBindingRepository.class);
|
||||
entityManager = mock(EntityManager.class);
|
||||
transactionManager = mock(PlatformTransactionManager.class);
|
||||
ldapAuthService = new LdapAuthService(
|
||||
ldapProperties,
|
||||
userAccountRepository,
|
||||
userRoleBindingRepository,
|
||||
globalNamespaceMembershipService,
|
||||
identityBindingRepository);
|
||||
identityBindingRepository,
|
||||
entityManager,
|
||||
transactionManager);
|
||||
}
|
||||
|
||||
/** Directory attributes: subject=entryUUID, email=mail, displayName=displayName. */
|
||||
|
|
@ -95,7 +103,7 @@ class LdapAuthServiceTest {
|
|||
assertThat(created.getDisplayName()).isEqualTo(DISPLAY_NAME);
|
||||
assertThat(created.getEmail()).isEqualTo(EMAIL);
|
||||
verify(globalNamespaceMembershipService).ensureMember(created.getId());
|
||||
verify(identityBindingRepository).save(any(IdentityBinding.class));
|
||||
verify(identityBindingRepository).saveAndFlush(any(IdentityBinding.class));
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -118,7 +126,7 @@ class LdapAuthServiceTest {
|
|||
verify(userAccountRepository, never()).save(org.mockito.ArgumentMatchers.argThat(
|
||||
u -> !existingUserId.equals(u.getId())));
|
||||
// Critical: no new binding written on repeat login
|
||||
verify(identityBindingRepository, never()).save(any(IdentityBinding.class));
|
||||
verify(identityBindingRepository, never()).saveAndFlush(any(IdentityBinding.class));
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -165,7 +173,7 @@ class LdapAuthServiceTest {
|
|||
|
||||
// No account created, no binding written
|
||||
verify(userAccountRepository, never()).save(any(UserAccount.class));
|
||||
verify(identityBindingRepository, never()).save(any(IdentityBinding.class));
|
||||
verify(identityBindingRepository, never()).saveAndFlush(any(IdentityBinding.class));
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -186,7 +194,7 @@ class LdapAuthServiceTest {
|
|||
// Then — placeholder email follows the ldap:{username}@internal convention
|
||||
assertThat(created.getEmail()).isEqualTo("ldap:bob@internal");
|
||||
verify(userAccountRepository, never()).findByEmailIgnoreCase(any());
|
||||
verify(identityBindingRepository).save(any(IdentityBinding.class));
|
||||
verify(identityBindingRepository).saveAndFlush(any(IdentityBinding.class));
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -244,6 +252,67 @@ class LdapAuthServiceTest {
|
|||
assertThat(created.getDisplayName()).isEqualTo("Common Name");
|
||||
}
|
||||
|
||||
@Test
|
||||
void returningUser_emailCollision_refusesSilentUpdate_throwsConflict() throws Exception {
|
||||
// Given — the LDAP subject is bound, but the directory now reports an email that already
|
||||
// belongs to a different account. The refresh must refuse to adopt it (409), matching the
|
||||
// first-login email-collision rule.
|
||||
String userId = "usr_alice";
|
||||
UserAccount existing = new UserAccount(userId, "Alice", "alice@example.com", null);
|
||||
existing.setStatus(UserStatus.ACTIVE);
|
||||
UserAccount other = new UserAccount("usr_other", "Other User", "other@example.com", null);
|
||||
given(identityBindingRepository.findByProviderCodeAndSubject("ldap", SUBJECT))
|
||||
.willReturn(Optional.of(new IdentityBinding(userId, "ldap", SUBJECT, "alice")));
|
||||
given(userAccountRepository.findById(userId)).willReturn(Optional.of(existing));
|
||||
given(userAccountRepository.findByEmailIgnoreCase("other@example.com")).willReturn(Optional.of(other));
|
||||
|
||||
AuthFlowException thrown = null;
|
||||
try {
|
||||
invokeFindOrCreate("alice", directoryAttributes(SUBJECT, "other@example.com", "Alice"));
|
||||
} catch (java.lang.reflect.InvocationTargetException ite) {
|
||||
thrown = (AuthFlowException) ite.getCause();
|
||||
}
|
||||
assertThat(thrown).isNotNull();
|
||||
assertThat(thrown.getStatus()).isEqualTo(HttpStatus.CONFLICT);
|
||||
assertThat(thrown.getMessageCode()).isEqualTo("error.auth.ldap.emailConflict");
|
||||
// The user's own email must remain untouched.
|
||||
assertThat(existing.getEmail()).isEqualTo("alice@example.com");
|
||||
}
|
||||
|
||||
@Test
|
||||
void returningUser_sameEmail_isAllowedToRefresh() throws Exception {
|
||||
// Given — the directory reports the same email the bound account already owns; the
|
||||
// collision lookup must exclude the user's own account.
|
||||
String userId = "usr_alice";
|
||||
UserAccount existing = new UserAccount(userId, "Alice", "alice@example.com", null);
|
||||
existing.setStatus(UserStatus.ACTIVE);
|
||||
given(identityBindingRepository.findByProviderCodeAndSubject("ldap", SUBJECT))
|
||||
.willReturn(Optional.of(new IdentityBinding(userId, "ldap", SUBJECT, "alice")));
|
||||
given(userAccountRepository.findById(userId)).willReturn(Optional.of(existing));
|
||||
given(userAccountRepository.save(any(UserAccount.class))).willAnswer(inv -> inv.getArgument(0));
|
||||
|
||||
UserAccount result = invokeFindOrCreate("alice",
|
||||
directoryAttributes(SUBJECT, "alice@example.com", "Alice Smith"));
|
||||
|
||||
assertThat(result.getId()).isEqualTo(userId);
|
||||
assertThat(result.getEmail()).isEqualTo("alice@example.com");
|
||||
}
|
||||
|
||||
@Test
|
||||
void buildJndiEnvironment_includesCustomTrustStoreSettings() {
|
||||
ldapProperties.setUrl("ldaps://ldap.example.com:636");
|
||||
ldapProperties.setTlsTrustStorePath("/certs/ldap-truststore.jks");
|
||||
ldapProperties.setTlsTrustStorePassword("secret");
|
||||
ldapProperties.setTlsTrustStoreType("PKCS12");
|
||||
|
||||
java.util.Hashtable<String, String> env = ldapAuthService.buildJndiEnvironment(null, null);
|
||||
|
||||
assertThat(env)
|
||||
.containsEntry("javax.net.ssl.trustStore", "/certs/ldap-truststore.jks")
|
||||
.containsEntry("javax.net.ssl.trustStorePassword", "secret")
|
||||
.containsEntry("javax.net.ssl.trustStoreType", "PKCS12");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isTlsFailure_detectsSslHandshakeInCauseChain() {
|
||||
javax.naming.CommunicationException comm = new javax.naming.CommunicationException("LDAP connect failed");
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue