allow creating ldap accounts after linking accounts to the same the d…

plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/OpenLdapUserManagerImpl.java

@@ -74,7 +74,12 @@
         return new LdapUser(username, email, firstname, lastname, principal, domain, disabled, memberships);
     }
 
-    private String generateSearchFilter(final String username, Long domainId) {
+    /**
+     * @param restrictToLinkedGroups scope to groups already linked in this domain; only valid for
+     *                               browsing/importing. Applied to a single known username, it wrongly
+     *                               blocks creating one ldap account once another is linked to a group.
+     */
+    private String generateSearchFilter(final String username, Long domainId, final boolean restrictToLinkedGroups) {
         final StringBuilder userObjectFilter = new StringBuilder();
         userObjectFilter.append("(objectClass=");
         userObjectFilter.append(_ldapConfiguration.getUserObject(domainId));
@@ -89,14 +94,16 @@
 
         String memberOfAttribute = getMemberOfAttribute(domainId);
         StringBuilder ldapGroupsFilter = new StringBuilder();
-        // this should get the trustmaps for this domain
-        List<String> ldapGroups = getMappedLdapGroups(domainId);
-        if (null != ldapGroups && ldapGroups.size() > 0) {
-            ldapGroupsFilter.append("(|");
-            for (String ldapGroup : ldapGroups) {
-                ldapGroupsFilter.append(getMemberOfGroupString(ldapGroup, memberOfAttribute));
+        if (restrictToLinkedGroups) {
+            // this should get the trustmaps for this domain
+            List<String> ldapGroups = getMappedLdapGroups(domainId);
+            if (null != ldapGroups && ldapGroups.size() > 0) {
+                ldapGroupsFilter.append("(|");
+                for (String ldapGroup : ldapGroups) {
+                    ldapGroupsFilter.append(getMemberOfGroupString(ldapGroup, memberOfAttribute));
+                }
+                ldapGroupsFilter.append(')');
             }
-            ldapGroupsFilter.append(')');
         }
         // make sure only users in the principle group are retrieved
         String pricipleGroup = _ldapConfiguration.getSearchGroupPrinciple(domainId);
@@ -167,10 +174,14 @@
         return result.toString();
     }
 
+    /**
+     * Looks up one known username, unscoped by linked groups, so an existing group link
+     * elsewhere in the domain can't block finding this user.
+     */
     @Override
     public LdapUser getUser(final String username, final LdapContext context, Long domainId) throws NamingException, IOException {
-        List<LdapUser> result = searchUsers(username, context, domainId);
+        List<LdapUser> result = searchUsers(username, context, domainId, false);
         if (result!= null && result.size() == 1) {
             return result.get(0);
         } else {
             throw new NamingException("No user found for username " + username);
@@ -311,6 +322,10 @@
 
     @Override
     public List<LdapUser> searchUsers(final String username, final LdapContext context, Long domainId) throws NamingException, IOException {
+        return searchUsers(username, context, domainId, true);
+    }
+
+    private List<LdapUser> searchUsers(final String username, final LdapContext context, Long domainId, final boolean restrictToLinkedGroups) throws NamingException, IOException {
 
         final SearchControls searchControls = new SearchControls();
 
@@ -327,7 +342,7 @@
         final List<LdapUser> users = new ArrayList<LdapUser>();
         NamingEnumeration<SearchResult> results;
         do {
-            results = context.search(basedn, generateSearchFilter(username, domainId), searchControls);
+            results = context.search(basedn, generateSearchFilter(username, domainId, restrictToLinkedGroups), searchControls);
             while (results.hasMoreElements()) {
                 final SearchResult result = results.nextElement();
                 if (!isUserDisabled(result)) {

plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/OpenLdapUserManagerImplTest.java

@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.ldap;
+
+import com.cloud.user.Account;
+import org.apache.cloudstack.ldap.dao.LdapTrustMapDao;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import javax.naming.NamingEnumeration;
+import javax.naming.NamingException;
+import javax.naming.directory.SearchControls;
+import javax.naming.directory.SearchResult;
+import javax.naming.ldap.LdapContext;
+
+import java.util.Collections;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Regression tests: creating an ldap account must not fail just because another
+ * account is already linked to an ldap group in the domain; browsing/importing
+ * still honours that group scope.
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class OpenLdapUserManagerImplTest {
+
+    private static final Long DOMAIN_ID = 1L;
+    private static final String LINKED_GROUP = "cn=test admins,ou=groups,dc=my,dc=domain,dc=com";
+
+    private OpenLdapUserManagerImpl openLdapUserManager;
+
+    private MockedStatic<LdapConfiguration> ldapConfigurationMockedStatic;
+
+    @Mock
+    private LdapConfiguration ldapConfigurationMock;
+
+    @Mock
+    private LdapTrustMapDao ldapTrustMapDaoMock;
+
+    @Mock
+    private LdapContext ldapContextMock;
+
+    @Before
+    public void setup() throws Exception {
+        // getUserMemberOfAttribute is static, unlike its LdapConfiguration siblings; mock statically.
+        ldapConfigurationMockedStatic = Mockito.mockStatic(LdapConfiguration.class, Mockito.CALLS_REAL_METHODS);
+        when(LdapConfiguration.getUserMemberOfAttribute(any())).thenReturn("memberOf");
+
+        openLdapUserManager = new OpenLdapUserManagerImpl(ldapConfigurationMock);
+        openLdapUserManager._ldapTrustMapDao = ldapTrustMapDaoMock;
+
+        when(ldapConfigurationMock.getScope()).thenReturn(SearchControls.SUBTREE_SCOPE);
+        when(ldapConfigurationMock.getReturnAttributes(any())).thenReturn(new String[]{"uid"});
+        when(ldapConfigurationMock.getSearchGroupPrinciple(any())).thenReturn(null);
+        when(ldapConfigurationMock.getBaseDn(any())).thenReturn("dc=my,dc=domain,dc=com");
+        when(ldapConfigurationMock.getUsernameAttribute(any())).thenReturn("uid");
+        when(ldapConfigurationMock.getUserObject(any())).thenReturn("inetOrgPerson");
+        when(ldapConfigurationMock.getLdapPageSize(any())).thenReturn(1000);
+
+        LdapTrustMapVO linkedGroup = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, LINKED_GROUP, Account.Type.NORMAL, 5L);
+        when(ldapTrustMapDaoMock.searchByDomainId(anyLong())).thenReturn(Collections.singletonList(linkedGroup));
+
+        NamingEnumeration<SearchResult> noResults = mock(NamingEnumeration.class);
+        when(noResults.hasMoreElements()).thenReturn(false);
+        when(ldapContextMock.search(any(String.class), any(String.class), any(SearchControls.class))).thenReturn(noResults);
+        when(ldapContextMock.getResponseControls()).thenReturn(null);
+    }
+
+    @After
+    public void tearDown() {
+        ldapConfigurationMockedStatic.close();
+    }
+
+    @Test
+    public void getUserDoesNotRestrictToAlreadyLinkedGroups() throws Exception {
+        try {
+            openLdapUserManager.getUser("test_user", ldapContextMock, DOMAIN_ID);
+        } catch (NamingException expected) {
+            // no results stubbed; only the filter sent matters here
+        }
+
+        String filter = capturedSearchFilter();
+        assertFalse("a lookup for one specific username must not be scoped to already-linked groups: " + filter,
+                filter.contains("memberOf=" + LINKED_GROUP));
+    }
+
+    @Test
+    public void getUsersStillRestrictsToAlreadyLinkedGroups() throws Exception {
+        openLdapUserManager.getUsers("test_user", ldapContextMock, DOMAIN_ID);
+
+        String filter = capturedSearchFilter();
+        assertTrue("browsing/importing users should still be scoped to already-linked groups: " + filter,
+                filter.contains("memberOf=" + LINKED_GROUP));
+    }
+
+    private String capturedSearchFilter() throws Exception {
+        ArgumentCaptor<String> filterCaptor = ArgumentCaptor.forClass(String.class);
+        verify(ldapContextMock, atLeastOnce()).search(any(String.class), filterCaptor.capture(), any(SearchControls.class));
+        return filterCaptor.getValue();
+    }
+}