diff --git a/server/skillhub-app/src/main/resources/application.yml b/server/skillhub-app/src/main/resources/application.yml index 34777806..30e5dc17 100644 --- a/server/skillhub-app/src/main/resources/application.yml +++ b/server/skillhub-app/src/main/resources/application.yml @@ -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: diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/ConcurrentLdapFirstLoginTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/ConcurrentLdapFirstLoginTest.java new file mode 100644 index 00000000..cb489c11 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/ConcurrentLdapFirstLoginTest.java @@ -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 login = () -> localAuthService.login("alice", "alice123"); + Future first = pool.submit(login); + Future 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(); + } + } +} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/LdapDirectoryUnavailableTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/LdapDirectoryUnavailableTest.java index 7d361108..b9bd55b0 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/LdapDirectoryUnavailableTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/LdapDirectoryUnavailableTest.java @@ -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) diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/LdapIntegrationTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/LdapIntegrationTest.java index c2a4adf8..916ef601 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/LdapIntegrationTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/ldap/LdapIntegrationTest.java @@ -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 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 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 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) diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/LdapProperties.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/LdapProperties.java index 46cfc80a..a9ff8d33 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/LdapProperties.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/LdapProperties.java @@ -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; + } } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/ldap/LdapAuthService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/ldap/LdapAuthService.java index 21ea29bb..ef514b30 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/ldap/LdapAuthService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/ldap/LdapAuthService.java @@ -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 buildJndiEnvironment(String principal, String credentials) { Hashtable 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 { *
  • Duplicate accounts for email-less LDAP users on repeated logins
    • * */ - 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) { diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/local/LocalAuthService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/local/LocalAuthService.java index 4812b57c..a91b6f41 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/local/LocalAuthService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/local/LocalAuthService.java @@ -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()); diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/config/LdapPropertiesTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/config/LdapPropertiesTest.java new file mode 100644 index 00000000..9c8915ae --- /dev/null +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/config/LdapPropertiesTest.java @@ -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(); + } +} diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/ldap/LdapAuthServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/ldap/LdapAuthServiceTest.java index c008bf9b..60ab01b0 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/ldap/LdapAuthServiceTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/ldap/LdapAuthServiceTest.java @@ -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 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");