From 1cc3bd90b688e7bc65d5e7ce79ccadeba3ede628 Mon Sep 17 00:00:00 2001
From: Chris Ridd <chris.ridd@forgerock.com>
Date: Thu, 28 Jan 2016 13:47:41 +0000
Subject: [PATCH] OPENDJ-1906: Protect StaticGroups with a RWLock to allow updating in-place
---
opendj-server-legacy/src/main/java/org/opends/server/api/Group.java | 18 ++
opendj-server-legacy/src/main/java/org/opends/server/core/GroupManager.java | 55 +++++++-
opendj-server-legacy/src/main/java/org/opends/server/extensions/VirtualStaticGroup.java | 11 +
opendj-server-legacy/src/main/java/org/opends/server/extensions/DynamicGroup.java | 13 ++
opendj-server-legacy/src/main/java/org/opends/server/extensions/StaticGroup.java | 213 +++++++++++++++++++++++++++++------
5 files changed, 261 insertions(+), 49 deletions(-)
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/api/Group.java b/opendj-server-legacy/src/main/java/org/opends/server/api/Group.java
index 7eb4f60..7633852 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/api/Group.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/api/Group.java
@@ -22,7 +22,7 @@
*
*
* Copyright 2006-2010 Sun Microsystems, Inc.
- * Portions Copyright 2011-2015 ForgeRock AS
+ * Portions Copyright 2011-2016 ForgeRock AS
*/
package org.opends.server.api;
@@ -40,6 +40,7 @@
import org.opends.server.types.Entry;
import org.opends.server.types.InitializationException;
import org.opends.server.types.MemberList;
+import org.opends.server.types.Modification;
import org.opends.server.types.SearchFilter;
import org.forgerock.opendj.ldap.SearchScope;
@@ -459,6 +460,21 @@
/**
+ * Attempt to make multiple changes to the group's member list.
+ *
+ * @param modifications The list of modifications being made to the group,
+ * which may include changes to non-member attributes.
+ * @throws UnsupportedOperationException If this group does not support
+ * altering the member list.
+ * @throws DirectoryException If a problem occurs while attempting to
+ * update the members.
+ */
+ public abstract void updateMembers(List<Modification> modifications)
+ throws UnsupportedOperationException, DirectoryException;
+
+
+
+ /**
* Attempts to add the provided user as a member of this group. The
* change should be committed to persistent storage through an
* internal operation.
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/core/GroupManager.java b/opendj-server-legacy/src/main/java/org/opends/server/core/GroupManager.java
index 777e65f..8088a3d 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/core/GroupManager.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/core/GroupManager.java
@@ -22,7 +22,7 @@
*
*
* Copyright 2007-2010 Sun Microsystems, Inc.
- * Portions Copyright 2011-2015 ForgeRock AS
+ * Portions Copyright 2011-2016 ForgeRock AS
*/
package org.opends.server.core;
@@ -42,6 +42,7 @@
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.forgerock.i18n.LocalizableMessage;
@@ -76,6 +77,7 @@
import org.opends.server.types.DirectoryException;
import org.opends.server.types.Entry;
import org.opends.server.types.InitializationException;
+import org.opends.server.types.Modification;
import org.opends.server.types.SearchFilter;
import org.opends.server.types.SearchResultEntry;
import org.opends.server.types.operation.PluginOperation;
@@ -128,7 +130,7 @@
private DITCacheMap<Group<?>> groupInstances;
/** Lock to protect internal data structures. */
- private final ReentrantReadWriteLock lock;
+ private final ReadWriteLock lock;
/** Dummy configuration DN for Group Manager. */
private static final String CONFIG_DN = "cn=Group Manager,cn=config";
@@ -767,13 +769,36 @@
/**
+ * Scan the list of provided modifications looking for any changes to the objectClass,
+ * which might change the entry to another kind of group, or even to a non-group.
+ *
+ * @param modifications List of modifications to the current group
+ *
+ * @return {@code true} if the objectClass is changed in any way, {@code false} otherwise.
+ */
+ private boolean updatesObjectClass(List<Modification> modifications)
+ {
+ for (Modification mod : modifications)
+ {
+ if (mod.getAttribute().getAttributeType().isObjectClass())
+ {
+ return true;
+ }
+ }
+ return false;
+ }
+
+
+
+ /**
* In this case, if the entry is associated with a registered
* group instance, then that instance will be recreated from
* the contents of the provided entry and re-registered with
* the group manager.
*/
private void doPostModify(PluginOperation modifyOperation,
- Entry oldEntry, Entry newEntry)
+ Entry oldEntry, Entry newEntry,
+ List<Modification> modifications)
{
if (hasGroupMembershipUpdateControl(modifyOperation))
{
@@ -798,16 +823,27 @@
lock.writeLock().lock();
try
{
- if (groupInstances.containsKey(oldEntry.getName()))
+ Group<?> group = groupInstances.get(oldEntry.getName());
+ if (group != null)
{
- if (! oldEntry.getName().equals(newEntry.getName()))
+ if (!oldEntry.getName().equals(newEntry.getName())
+ || !group.mayAlterMemberList()
+ || updatesObjectClass(modifications))
{
- // This should never happen, but check for it anyway.
groupInstances.remove(oldEntry.getName());
+ // This updates the refreshToken
+ createAndRegisterGroup(newEntry);
}
- createAndRegisterGroup(newEntry);
+ else
+ {
+ group.updateMembers(modifications);
+ }
}
}
+ catch (UnsupportedOperationException | DirectoryException e)
+ {
+ logger.traceException(e);
+ }
finally
{
lock.writeLock().unlock();
@@ -901,7 +937,8 @@
{
doPostModify(modifyOperation,
modifyOperation.getCurrentEntry(),
- modifyOperation.getModifiedEntry());
+ modifyOperation.getModifiedEntry(),
+ modifyOperation.getModifications());
}
// If we've gotten here, then everything is acceptable.
@@ -959,7 +996,7 @@
Entry modEntry = modifyOperation.getModifiedEntry();
if (entry != null && modEntry != null)
{
- doPostModify(modifyOperation, entry, modEntry);
+ doPostModify(modifyOperation, entry, modEntry, modifyOperation.getModifications());
}
}
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/extensions/DynamicGroup.java b/opendj-server-legacy/src/main/java/org/opends/server/extensions/DynamicGroup.java
index f014cc4..2bc2cdf 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/extensions/DynamicGroup.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/extensions/DynamicGroup.java
@@ -50,6 +50,7 @@
import org.opends.server.types.InitializationException;
import org.opends.server.types.LDAPURL;
import org.opends.server.types.MemberList;
+import org.opends.server.types.Modification;
import org.opends.server.types.ObjectClass;
import org.opends.server.types.SearchFilter;
@@ -339,6 +340,18 @@
/** {@inheritDoc} */
@Override
+ public void updateMembers(List<Modification> modifications)
+ throws UnsupportedOperationException, DirectoryException
+ {
+ // Dynamic groups don't support altering the member list.
+ LocalizableMessage message = ERR_DYNAMICGROUP_ALTERING_MEMBERS_NOT_SUPPORTED.get();
+ throw new UnsupportedOperationException(message.toString());
+ }
+
+
+
+ /** {@inheritDoc} */
+ @Override
public void addMember(Entry userEntry)
throws UnsupportedOperationException, DirectoryException
{
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/extensions/StaticGroup.java b/opendj-server-legacy/src/main/java/org/opends/server/extensions/StaticGroup.java
index 64f9592..ca65fdc 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/extensions/StaticGroup.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/extensions/StaticGroup.java
@@ -31,6 +31,8 @@
import java.util.LinkedList;
import java.util.List;
import java.util.Set;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.forgerock.i18n.LocalizableMessage;
import org.forgerock.i18n.LocalizedIllegalArgumentException;
@@ -42,6 +44,7 @@
import org.forgerock.opendj.ldap.ModificationType;
import org.forgerock.opendj.ldap.ResultCode;
import org.forgerock.opendj.ldap.SearchScope;
+import org.forgerock.util.Reject;
import org.opends.server.admin.std.server.GroupImplementationCfg;
import org.opends.server.admin.std.server.StaticGroupImplementationCfg;
import org.opends.server.api.Group;
@@ -104,6 +107,9 @@
/** Passed to the group manager to see if the nested group list needs to be refreshed. */
private long nestedGroupRefreshToken = DirectoryServer.getGroupManager().refreshToken();
+ /** Read/write lock protecting memberDNs and nestedGroups. */
+ private ReadWriteLock lock = new ReentrantReadWriteLock();
+
private ServerContext serverContext;
/**
@@ -129,7 +135,7 @@
LinkedHashSet<CompactDn> memberDNs)
{
super();
- ifNull(groupEntryDN, memberAttributeType, memberDNs);
+ Reject.ifNull(groupEntryDN, memberAttributeType, memberDNs);
this.serverContext = serverContext;
this.groupEntryDN = groupEntryDN;
@@ -149,7 +155,7 @@
@Override
public StaticGroup newInstance(ServerContext serverContext, Entry groupEntry) throws DirectoryException
{
- ifNull(groupEntry);
+ Reject.ifNull(groupEntry);
// Determine whether it is a groupOfNames, groupOfEntries or
// groupOfUniqueNames entry. If not, then that's a problem.
@@ -239,7 +245,7 @@
@Override
public boolean isGroupDefinition(Entry entry)
{
- ifNull(entry);
+ Reject.ifNull(entry);
// FIXME -- This needs to exclude enhanced groups once we have support for them.
if (hasObjectClass(entry, OC_VIRTUAL_STATIC_GROUP))
@@ -295,12 +301,23 @@
@Override
public List<DN> getNestedGroupDNs()
{
- try {
+ try
+ {
reloadIfNeeded();
- } catch (DirectoryException ex) {
+ }
+ catch (DirectoryException ex)
+ {
return Collections.<DN>emptyList();
}
- return nestedGroups;
+ lock.readLock().lock();
+ try
+ {
+ return nestedGroups;
+ }
+ finally
+ {
+ lock.readLock().unlock();
+ }
}
/** {@inheritDoc} */
@@ -308,9 +325,10 @@
public void addNestedGroup(DN nestedGroupDN)
throws UnsupportedOperationException, DirectoryException
{
- ifNull(nestedGroupDN);
+ Reject.ifNull(nestedGroupDN);
- synchronized (this)
+ lock.writeLock().lock();
+ try
{
if (nestedGroups.contains(nestedGroupDN))
{
@@ -335,6 +353,10 @@
newMemberDNs.add(toCompactDn(nestedGroupDN));
memberDNs = newMemberDNs;
}
+ finally
+ {
+ lock.writeLock().unlock();
+ }
}
/** {@inheritDoc} */
@@ -342,9 +364,10 @@
public void removeNestedGroup(DN nestedGroupDN)
throws UnsupportedOperationException, DirectoryException
{
- ifNull(nestedGroupDN);
+ Reject.ifNull(nestedGroupDN);
- synchronized (this)
+ lock.writeLock().lock();
+ try
{
if (! nestedGroups.contains(nestedGroupDN))
{
@@ -369,6 +392,10 @@
newMemberDNs.remove(toCompactDn(nestedGroupDN));
memberDNs = newMemberDNs;
}
+ finally
+ {
+ lock.writeLock().unlock();
+ }
}
/** {@inheritDoc} */
@@ -377,25 +404,33 @@
{
reloadIfNeeded();
CompactDn compactUserDN = toCompactDn(userDN);
- if (memberDNs.contains(compactUserDN))
+ lock.readLock().lock();
+ try
{
- return true;
- }
- else if (!examinedGroups.add(getGroupDN()))
- {
- return false;
- }
- else
- {
- for(DN nestedGroupDN : nestedGroups)
+ if (memberDNs.contains(compactUserDN))
{
- Group<? extends GroupImplementationCfg> group = getGroupManager().getGroupInstance(nestedGroupDN);
- if (group != null && group.isMember(userDN, examinedGroups))
+ return true;
+ }
+ else if (!examinedGroups.add(getGroupDN()))
+ {
+ return false;
+ }
+ else
+ {
+ for (DN nestedGroupDN : nestedGroups)
{
- return true;
+ Group<? extends GroupImplementationCfg> group = getGroupManager().getGroupInstance(nestedGroupDN);
+ if (group != null && group.isMember(userDN, examinedGroups))
+ {
+ return true;
+ }
}
}
}
+ finally
+ {
+ lock.readLock().unlock();
+ }
return false;
}
@@ -417,14 +452,18 @@
//the current token.
if (DirectoryServer.getGroupManager().hasInstancesChanged(nestedGroupRefreshToken))
{
- synchronized (this)
+ lock.writeLock().lock();
+ try
{
Group<?> thisGroup = DirectoryServer.getGroupManager().getGroupInstance(groupEntryDN);
// Check if the group itself has been removed
- if (thisGroup == null) {
+ if (thisGroup == null)
+ {
throw new DirectoryException(ResultCode.NO_SUCH_ATTRIBUTE,
ERR_STATICGROUP_GROUP_INSTANCE_INVALID.get(groupEntryDN));
- } else if (thisGroup != this) {
+ }
+ else if (thisGroup != this)
+ {
LinkedHashSet<CompactDn> newMemberDNs = new LinkedHashSet<>();
MemberList memberList = thisGroup.getMembers();
while (memberList.hasMoreMembers())
@@ -440,18 +479,21 @@
}
memberDNs = newMemberDNs;
}
- LinkedList<DN> newNestedGroups = new LinkedList<>();
+ nestedGroups.clear();
for (CompactDn compactDn : memberDNs)
{
DN dn = fromCompactDn(compactDn);
Group<?> group = DirectoryServer.getGroupManager().getGroupInstance(dn);
if (group != null)
{
- newNestedGroups.add(group.getGroupDN());
+ nestedGroups.add(group.getGroupDN());
}
}
nestedGroupRefreshToken = DirectoryServer.getGroupManager().refreshToken();
- nestedGroups=newNestedGroups;
+ }
+ finally
+ {
+ lock.writeLock().unlock();
}
}
}
@@ -461,7 +503,15 @@
public MemberList getMembers() throws DirectoryException
{
reloadIfNeeded();
- return new SimpleStaticGroupMemberList(groupEntryDN, memberDNs);
+ lock.readLock().lock();
+ try
+ {
+ return new SimpleStaticGroupMemberList(groupEntryDN, memberDNs);
+ }
+ finally
+ {
+ lock.readLock().unlock();
+ }
}
/** {@inheritDoc} */
@@ -469,11 +519,19 @@
public MemberList getMembers(DN baseDN, SearchScope scope, SearchFilter filter) throws DirectoryException
{
reloadIfNeeded();
- if (baseDN == null && filter == null)
+ lock.readLock().lock();
+ try
{
- return new SimpleStaticGroupMemberList(groupEntryDN, memberDNs);
+ if (baseDN == null && filter == null)
+ {
+ return new SimpleStaticGroupMemberList(groupEntryDN, memberDNs);
+ }
+ return new FilteredStaticGroupMemberList(groupEntryDN, memberDNs, baseDN, scope, filter);
}
- return new FilteredStaticGroupMemberList(groupEntryDN, memberDNs, baseDN, scope, filter);
+ finally
+ {
+ lock.readLock().unlock();
+ }
}
/** {@inheritDoc} */
@@ -485,11 +543,80 @@
/** {@inheritDoc} */
@Override
+ public void updateMembers(List<Modification> modifications)
+ throws UnsupportedOperationException, DirectoryException
+ {
+ Reject.ifNull(memberDNs);
+ Reject.ifNull(nestedGroups);
+
+ reloadIfNeeded();
+ lock.writeLock().lock();
+ try
+ {
+ for (Modification mod : modifications)
+ {
+ Attribute attribute = mod.getAttribute();
+ if (attribute.getAttributeType().equals(memberAttributeType))
+ {
+ switch (mod.getModificationType().asEnum())
+ {
+ case ADD:
+ for (ByteString v : attribute)
+ {
+ DN member = DN.decode(v);
+ memberDNs.add(toCompactDn(member));
+ if (DirectoryServer.getGroupManager().getGroupInstance(member) != null)
+ {
+ nestedGroups.add(member);
+ }
+ }
+ break;
+ case DELETE:
+ if (attribute.isEmpty())
+ {
+ memberDNs.clear();
+ nestedGroups.clear();
+ }
+ else
+ {
+ for (ByteString v : attribute)
+ {
+ DN member = DN.decode(v);
+ memberDNs.remove(toCompactDn(member));
+ nestedGroups.remove(member);
+ }
+ }
+ break;
+ case REPLACE:
+ memberDNs.clear();
+ nestedGroups.clear();
+ for (ByteString v : attribute)
+ {
+ DN member = DN.decode(v);
+ memberDNs.add(toCompactDn(member));
+ if (DirectoryServer.getGroupManager().getGroupInstance(member) != null)
+ {
+ nestedGroups.add(member);
+ }
+ }
+ break;
+ }
+ }
+ }
+ }
+ finally {
+ lock.writeLock().unlock();
+ }
+ }
+
+ /** {@inheritDoc} */
+ @Override
public void addMember(Entry userEntry) throws UnsupportedOperationException, DirectoryException
{
- ifNull(userEntry);
+ Reject.ifNull(userEntry);
- synchronized (this)
+ lock.writeLock().lock();
+ try
{
DN userDN = userEntry.getName();
CompactDn compactUserDN = toCompactDn(userDN);
@@ -512,16 +639,21 @@
newMemberDNs.add(compactUserDN);
memberDNs = newMemberDNs;
}
+ finally
+ {
+ lock.writeLock().unlock();
+ }
}
/** {@inheritDoc} */
@Override
public void removeMember(DN userDN) throws UnsupportedOperationException, DirectoryException
{
- ifNull(userDN);
+ Reject.ifNull(userDN);
CompactDn compactUserDN = toCompactDn(userDN);
- synchronized (this)
+ lock.writeLock().lock();
+ try
{
if (! memberDNs.contains(compactUserDN))
{
@@ -541,12 +673,17 @@
newMemberDNs.remove(compactUserDN);
memberDNs = newMemberDNs;
//If it is in the nested group list remove it.
- if(nestedGroups.contains(userDN)) {
+ if (nestedGroups.contains(userDN))
+ {
LinkedList<DN> newNestedGroups = new LinkedList<>(nestedGroups);
newNestedGroups.remove(userDN);
nestedGroups = newNestedGroups;
}
}
+ finally
+ {
+ lock.writeLock().unlock();
+ }
}
private ModifyOperation newModifyOperation(ModificationType modType, DN userDN)
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/extensions/VirtualStaticGroup.java b/opendj-server-legacy/src/main/java/org/opends/server/extensions/VirtualStaticGroup.java
index f2cf48f..fe91446 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/extensions/VirtualStaticGroup.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/extensions/VirtualStaticGroup.java
@@ -47,6 +47,7 @@
import org.opends.server.types.Entry;
import org.opends.server.types.InitializationException;
import org.opends.server.types.MemberList;
+import org.opends.server.types.Modification;
import org.opends.server.types.ObjectClass;
import org.opends.server.types.SearchFilter;
@@ -385,7 +386,15 @@
return false;
}
-
+ /** {@inheritDoc} */
+ @Override
+ public void updateMembers(List<Modification> modifications)
+ throws UnsupportedOperationException, DirectoryException
+ {
+ // Virtual static groups don't support altering the member list.
+ LocalizableMessage message = ERR_VIRTUAL_STATIC_GROUP_ALTERING_MEMBERS_NOT_SUPPORTED.get(groupEntryDN);
+ throw new UnsupportedOperationException(message.toString());
+ }
/** {@inheritDoc} */
@Override
--
Gitblit v1.10.0