From 0d823bacd046864de77156d124e060a8c8f06a22 Mon Sep 17 00:00:00 2001
From: srosse <stephane.rosse@frentix.com>
Date: Fri, 14 Aug 2020 11:52:59 +0200
Subject: [PATCH] OO-4794: adapt deletion process and add unit test for it
---
.../basesecurity/BaseSecurityManager.java | 19 +-----
.../manager/AuthenticationDAO.java | 43 +++++++++++++
.../java/org/olat/ldap/LDAPLoginManager.java | 2 +-
.../ldap/manager/LDAPLoginManagerImpl.java | 61 +++++++++----------
.../org/olat/ldap/ui/LDAPAdminController.java | 2 +-
.../manager/AuthenticationDAOTest.java | 24 ++++++++
.../ldap/manager/LDAPLoginManagerTest.java | 24 ++++++++
7 files changed, 124 insertions(+), 51 deletions(-)
diff --git a/src/main/java/org/olat/basesecurity/BaseSecurityManager.java b/src/main/java/org/olat/basesecurity/BaseSecurityManager.java
index 65635d6948a..41566256ffd 100644
--- a/src/main/java/org/olat/basesecurity/BaseSecurityManager.java
+++ b/src/main/java/org/olat/basesecurity/BaseSecurityManager.java
@@ -767,24 +767,7 @@ public class BaseSecurityManager implements BaseSecurity, UserDataDeletable {
@Override
public Authentication findAuthentication(IdentityRef identity, String provider) {
- if (identity==null) {
- throw new IllegalArgumentException("identity must not be null");
- }
-
- StringBuilder sb = new StringBuilder();
- sb.append("select auth from ").append(AuthenticationImpl.class.getName())
- .append(" as auth where auth.identity.key=:identityKey and auth.provider=:provider");
-
- List<Authentication> results = dbInstance.getCurrentEntityManager()
- .createQuery(sb.toString(), Authentication.class)
- .setParameter("identityKey", identity.getKey())
- .setParameter("provider", provider)
- .getResultList();
- if (results == null || results.isEmpty()) return null;
- if (results.size() > 1) {
- throw new AssertException("Found more than one Authentication for a given subject and a given provider.");
- }
- return results.get(0);
+ return authenticationDao.getAuthentication(identity, provider);
}
@Override
diff --git a/src/main/java/org/olat/basesecurity/manager/AuthenticationDAO.java b/src/main/java/org/olat/basesecurity/manager/AuthenticationDAO.java
index 6c5a5e456cb..0dd9f9088e6 100644
--- a/src/main/java/org/olat/basesecurity/manager/AuthenticationDAO.java
+++ b/src/main/java/org/olat/basesecurity/manager/AuthenticationDAO.java
@@ -32,6 +32,7 @@ import org.olat.basesecurity.IdentityRef;
import org.olat.core.commons.persistence.DB;
import org.olat.core.id.Identity;
import org.olat.core.logging.AssertException;
+import org.olat.core.util.StringHelper;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;
@@ -126,6 +127,48 @@ public class AuthenticationDAO {
.getResultList();
}
+ /**
+ *
+ * @param provider The authentication provider
+ * @return A list of identities (the user is not fetched)
+ */
+ public List<Authentication> getAuthentications(String provider) {
+ StringBuilder sb = new StringBuilder(256);
+ sb.append("select auth from ").append(AuthenticationImpl.class.getName()).append(" as auth")
+ .append(" inner join fetch auth.identity as ident")
+ .append(" inner join fetch ident.user as identUser")
+ .append(" where auth.provider=:provider");
+ return dbInstance.getCurrentEntityManager()
+ .createQuery(sb.toString(), Authentication.class)
+ .setParameter("provider", provider)
+ .getResultList();
+ }
+
+ public Authentication getAuthentication(IdentityRef identity, String provider) {
+ if (identity == null || !StringHelper.containsNonWhitespace(provider)) {
+ throw new IllegalArgumentException("identity must not be null");
+ }
+
+ StringBuilder sb = new StringBuilder();
+ sb.append("select auth from ").append(AuthenticationImpl.class.getName()).append(" as auth")
+ .append(" inner join auth.identity as ident")
+ .append(" inner join ident.user as user")
+ .append(" where auth.identity.key=:identityKey and auth.provider=:provider");
+
+ List<Authentication> results = dbInstance.getCurrentEntityManager()
+ .createQuery(sb.toString(), Authentication.class)
+ .setParameter("identityKey", identity.getKey())
+ .setParameter("provider", provider)
+ .getResultList();
+ if (results == null || results.isEmpty()) {
+ return null;
+ }
+ if (results.size() > 1) {
+ throw new AssertException("Found more than one Authentication for a given subject and a given provider.");
+ }
+ return results.get(0);
+ }
+
public Authentication getAuthentication(String authUsername, String provider) {
StringBuilder sb = new StringBuilder(256);
sb.append("select auth from ").append(AuthenticationImpl.class.getName()).append(" as auth")
diff --git a/src/main/java/org/olat/ldap/LDAPLoginManager.java b/src/main/java/org/olat/ldap/LDAPLoginManager.java
index fb2fd84a067..a937094eb06 100644
--- a/src/main/java/org/olat/ldap/LDAPLoginManager.java
+++ b/src/main/java/org/olat/ldap/LDAPLoginManager.java
@@ -51,7 +51,7 @@ public interface LDAPLoginManager {
public Map<String,String> prepareUserPropertyForSync(Attributes attributes, Identity identity);
- public List<Identity> getIdentitysDeletedInLdap(LdapContext ctx);
+ public List<Identity> getIdentitiesDeletedInLdap(LdapContext ctx);
public Identity findIdentityByLdapAuthentication(Attributes attrs, LDAPError errors);
diff --git a/src/main/java/org/olat/ldap/manager/LDAPLoginManagerImpl.java b/src/main/java/org/olat/ldap/manager/LDAPLoginManagerImpl.java
index 9657a972412..d2e893f920e 100644
--- a/src/main/java/org/olat/ldap/manager/LDAPLoginManagerImpl.java
+++ b/src/main/java/org/olat/ldap/manager/LDAPLoginManagerImpl.java
@@ -409,7 +409,9 @@ public class LDAPLoginManagerImpl implements LDAPLoginManager, AuthenticationPro
*/
@Override
public boolean changePassword(Identity identity, String pwd, LDAPError errors) {
- String uid = identity.getName();
+ String uid = identity.getName();//TODO username
+
+
String ldapUserPasswordAttribute = syncConfiguration.getLdapUserPasswordAttribute();
try {
LdapContext ctx = bindSystem();
@@ -724,10 +726,12 @@ public class LDAPLoginManagerImpl implements LDAPLoginManager, AuthenticationPro
LdapContext ctx = bindSystem();
if (ctx == null) {
log.error("could not bind to ldap");
+ return;
}
String ldapUserIDAttribute = syncConfiguration.getOlatPropertyToLdapAttribute(LDAPConstants.LDAP_USER_IDENTIFYER);
- String filter = ldapDao.buildSearchUserFilter(ldapUserIDAttribute, identity.getName());
+ Authentication authentication = authenticationDao.getAuthentication(identity, LDAPAuthenticationController.PROVIDER_LDAP);
+ String filter = ldapDao.buildSearchUserFilter(ldapUserIDAttribute, authentication.getAuthusername());
boolean withCoacheOfGroups = StringHelper.containsNonWhitespace(syncConfiguration.getCoachedGroupAttribute());
List<String> ldapBases = syncConfiguration.getLdapBases();
@@ -874,10 +878,11 @@ public class LDAPLoginManagerImpl implements LDAPLoginManager, AuthenticationPro
* @throws NamingException
*/
@Override
- public List<Identity> getIdentitysDeletedInLdap(LdapContext ctx) {
+ public List<Identity> getIdentitiesDeletedInLdap(LdapContext ctx) {
if (ctx == null) {
- return null;
+ return Collections.emptyList();
}
+
// Find all LDAP Users
String userID = syncConfiguration.getOlatPropertyToLdapAttribute(LDAPConstants.LDAP_USER_IDENTIFYER);
String userFilter = syncConfiguration.getLdapUserFilter();
@@ -897,18 +902,18 @@ public class LDAPLoginManagerImpl implements LDAPLoginManager, AuthenticationPro
}, (userFilter == null ? "" : userFilter), new String[] { userID }, ctx);
if (ldapList.isEmpty()) {
- log.warn("No users in LDAP found, can't create deletionList!!");
+ log.warn("No users in LDAP found, can't create the deletion list.");
return Collections.emptyList();
}
-
+
List<Identity> identityListToDelete = new ArrayList<>();
- List<Identity> olatListIdentity = authenticationDao.getIdentitiesWithAuthentication(LDAPAuthenticationController.PROVIDER_LDAP);
- for (Identity ida:olatListIdentity) {
- // compare usernames with lowercase
- if (!ldapList.contains(ida.getName().toLowerCase())) {
- identityListToDelete.add(ida);
+ List<Authentication> ldapAuthentications = authenticationDao.getAuthentications(LDAPAuthenticationController.PROVIDER_LDAP);
+ for (Authentication ldapAuthentication:ldapAuthentications) {
+ if (!ldapList.contains(ldapAuthentication.getAuthusername().toLowerCase())) {
+ identityListToDelete.add(ldapAuthentication.getIdentity());
}
}
+ dbInstance.commitAndCloseSession();
return identityListToDelete;
}
@@ -1167,7 +1172,7 @@ public class LDAPLoginManagerImpl implements LDAPLoginManager, AuthenticationPro
break;
}
default: {
- log.error("LDAP Role synchronization not supported for: " + role);
+ log.error("LDAP Role synchronization not supported for: {}", role);
}
}
}
@@ -1175,11 +1180,12 @@ public class LDAPLoginManagerImpl implements LDAPLoginManager, AuthenticationPro
private void doBatchSyncDeletedUsers(LdapContext ctx, String sinceSentence) {
// create User to Delete List
- List<Identity> deletedUserList = getIdentitysDeletedInLdap(ctx);
+ List<Identity> deletedUserList = getIdentitiesDeletedInLdap(ctx);
// delete old users
if (deletedUserList == null || deletedUserList.isEmpty()) {
- log.info("LDAP batch sync: no users to delete" + sinceSentence);
+ log.info("LDAP batch sync: no users to delete {}", sinceSentence);
} else {
+ int deletedUserListSize = deletedUserList.size();
if (ldapLoginModule.isDeleteRemovedLDAPUsersOnSync()) {
// check if more not more than the defined percentages of
// users managed in LDAP should be deleted
@@ -1189,31 +1195,24 @@ public class LDAPLoginManagerImpl implements LDAPLoginManager, AuthenticationPro
if (olatListIdentity.isEmpty())
log.info("No users managed by LDAP, can't delete users");
else {
- int prozente = (int) (((float)deletedUserList.size() / (float) olatListIdentity.size())*100);
- if (prozente >= ldapLoginModule.getDeleteRemovedLDAPUsersPercentage()) {
- log.info("LDAP batch sync: more than "
- + ldapLoginModule.getDeleteRemovedLDAPUsersPercentage()
- + "% of LDAP managed users should be deleted. Please use Admin Deletion Job. Or increase deleteRemovedLDAPUsersPercentage. "
- + prozente
- + "% tried to delete.");
+ int prozente = (int) (((float)deletedUserListSize / (float) olatListIdentity.size()) * 100.0);
+ int cutValue = ldapLoginModule.getDeleteRemovedLDAPUsersPercentage();
+ if (prozente >= cutValue) {
+ log.info("LDAP batch sync: more than {}% of LDAP managed users should be deleted. Please use Admin Deletion Job. Or increase deleteRemovedLDAPUsersPercentage. {}% tried to delete.", cutValue, prozente);
} else {
// delete users
deleteIdentities(deletedUserList, null);
- log.info("LDAP batch sync: " + deletedUserList.size() + " users deleted" + sinceSentence);
+ log.info("LDAP batch sync: {} users deleted {}", deletedUserListSize, sinceSentence);
}
}
} else {
// Do nothing, only log users to logfile
- StringBuilder users = new StringBuilder();
+ StringBuilder users = new StringBuilder(deletedUserListSize * 42);
for (Identity toBeDeleted : deletedUserList) {
- users.append(toBeDeleted.getName()).append(',');
+ users.append(toBeDeleted.getKey()).append(',');
}
- log.info("LDAP batch sync: "
- + deletedUserList.size()
- + " users detected as to be deleted"
- + sinceSentence
- + ". Automatic deleting is disabled in LDAPLoginModule, delete these users manually::["
- + users.toString() + "]");
+ log.info("LDAP batch sync: {} users detected as to be deleted {}. Automatic deleting is disabled in LDAPLoginModule, delete these users manually::[{}]",
+ deletedUserListSize, sinceSentence, users);
}
}
dbInstance.commitAndCloseSession();
@@ -1558,7 +1557,7 @@ public class LDAPLoginManagerImpl implements LDAPLoginManager, AuthenticationPro
}
String ldapUserIDAttribute = syncConfiguration.getLdapUserLoginAttribute();
- Authentication authentication = securityManager.findAuthentication(ident, LDAPAuthenticationController.PROVIDER_LDAP);
+ Authentication authentication = authenticationDao.getAuthentication(ident, LDAPAuthenticationController.PROVIDER_LDAP);
String filter = ldapDao.buildSearchUserFilter(ldapUserIDAttribute, authentication.getAuthusername());
List<Attributes> ldapUserAttrs = new ArrayList<>();
diff --git a/src/main/java/org/olat/ldap/ui/LDAPAdminController.java b/src/main/java/org/olat/ldap/ui/LDAPAdminController.java
index 4c8cc68041f..b5df6f6f3bf 100644
--- a/src/main/java/org/olat/ldap/ui/LDAPAdminController.java
+++ b/src/main/java/org/olat/ldap/ui/LDAPAdminController.java
@@ -190,7 +190,7 @@ public class LDAPAdminController extends BasicController implements GenericEvent
// get deleted users
List<Identity> identitiesToDelete = null;
try {
- identitiesToDelete = ldapLoginManager.getIdentitysDeletedInLdap(ctx);
+ identitiesToDelete = ldapLoginManager.getIdentitiesDeletedInLdap(ctx);
ctx.close();
} catch (NamingException e) {
showError("delete.error.connection.close");
diff --git a/src/test/java/org/olat/basesecurity/manager/AuthenticationDAOTest.java b/src/test/java/org/olat/basesecurity/manager/AuthenticationDAOTest.java
index 722c3b224e3..eb116d908e5 100644
--- a/src/test/java/org/olat/basesecurity/manager/AuthenticationDAOTest.java
+++ b/src/test/java/org/olat/basesecurity/manager/AuthenticationDAOTest.java
@@ -70,6 +70,17 @@ public class AuthenticationDAOTest extends OlatTestCase {
Assert.assertEquals(newToken, updatedAuth.getCredential());
}
+ @Test
+ public void getAuthenticationIdentityProvider() {
+ IdentityWithLogin ident = JunitTestHelper.createAndPersistRndUser("authdao-1-");
+ dbInstance.commitAndCloseSession();
+
+ //check if the new token was saved
+ Authentication authentication = authenticationDao.getAuthentication(ident.getIdentity(), "OLAT");
+ Assert.assertEquals(ident.getIdentity(), authentication.getIdentity());
+ Assert.assertEquals(ident.getLogin(), authentication.getAuthusername());
+ }
+
@Test
public void getIdentitiesWithAuthentication() {
String token = UUID.randomUUID().toString();
@@ -142,6 +153,19 @@ public class AuthenticationDAOTest extends OlatTestCase {
Assert.assertEquals(ident.getIdentity(), olatAuthentications.get(0).getIdentity());
}
+ @Test
+ public void getAuthenticationsByProvider() {
+ IdentityWithLogin ident = JunitTestHelper.createAndPersistRndUser("authdao-2-");
+ dbInstance.commitAndCloseSession();
+ Assert.assertNotNull(ident);
+
+ List<Authentication> olatAuthentications = authenticationDao.getAuthentications("OLAT");
+ Assert.assertNotNull(olatAuthentications);
+ for(Authentication authentication:olatAuthentications) {
+ Assert.assertEquals("OLAT", authentication.getProvider());
+ }
+ }
+
@Test
public void getAuthenticationsByAuthusername_providersList() {
String token = UUID.randomUUID().toString();
diff --git a/src/test/java/org/olat/ldap/manager/LDAPLoginManagerTest.java b/src/test/java/org/olat/ldap/manager/LDAPLoginManagerTest.java
index e107d4b668e..a8bbc666384 100644
--- a/src/test/java/org/olat/ldap/manager/LDAPLoginManagerTest.java
+++ b/src/test/java/org/olat/ldap/manager/LDAPLoginManagerTest.java
@@ -22,9 +22,11 @@ package org.olat.ldap.manager;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import java.util.UUID;
import java.util.stream.Collectors;
import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapContext;
import org.junit.Assert;
import org.junit.Assume;
@@ -47,6 +49,7 @@ import org.olat.ldap.LDAPLoginManager;
import org.olat.ldap.LDAPLoginModule;
import org.olat.ldap.LDAPSyncConfiguration;
import org.olat.ldap.ui.LDAPAuthenticationController;
+import org.olat.test.JunitTestHelper;
import org.olat.test.OlatTestCase;
import org.olat.user.UserManager;
import org.springframework.beans.factory.annotation.Autowired;
@@ -106,6 +109,27 @@ public class LDAPLoginManagerTest extends OlatTestCase {
Assert.assertNotNull(identity);
}
+ @Test
+ public void getIdentitiesDeletedInLdap() {
+ Assume.assumeTrue(ldapLoginModule.isLDAPEnabled());
+ Identity orphan = JunitTestHelper.createAndPersistIdentityAsRndUser("ldap-orphan");
+ securityManager.createAndPersistAuthentication(orphan, LDAPAuthenticationController.PROVIDER_LDAP,
+ UUID.randomUUID().toString(), null, null);
+ dbInstance.commitAndCloseSession();
+
+ LdapContext ctx = ldapManager.bindSystem();
+ List<Identity> identities = ldapManager.getIdentitiesDeletedInLdap(ctx);
+
+ Assert.assertNotNull(identities);
+ Assert.assertTrue(identities.contains(orphan));
+
+ // historic
+ Identity identity1 = userManager.findUniqueIdentityByEmail("hhuerlimann@openolat.com");
+ Assert.assertFalse(identities.contains(identity1));
+ Identity identity2 = userManager.findUniqueIdentityByEmail("ahentschel@openolat.com");
+ Assert.assertFalse(identities.contains(identity2));
+ }
+
@Test
public void doSyncSingleUserWithLoginAttribute() throws LDAPException {
Assume.assumeTrue(ldapLoginModule.isLDAPEnabled());
--
GitLab