OPENDJ-1906: Protect StaticGroups with a RWLock to allow updating in-place

Chris Ridd

19.33.2015 1cc3bd90b688e7bc65d5e7ce79ccadeba3ede628

OPENDJ-1906: Protect StaticGroups with a RWLock to allow updating in-place

 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.

 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());
    }
  }


 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
  {

 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)

 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