Avoid profile race conditions by tracking entryUSN

Avoid race conditions in the LDAPProfileSubsystem by tracking the
most recently known entryUSN of profiles' LDAP entries.

As part of this change, add the commitProfile method to the
IProfileSubsystem interface, remove commit behaviour from the
enableProfile and disableProfile methods and update ProfileService
and ProfileApproveServlet to commit the profile (using the
commitProfile method) where needed.

Part of: https://fedorahosted.org/pki/ticket/1700

3 files changed, 103 insertions, 26 deletions

diff --git a/base/server/cmscore/src/com/netscape/cmscore/base/LDAPConfigStore.java b/base/server/cmscore/src/com/netscape/cmscore/base/LDAPConfigStore.java
index b7b4ca46e..0b4ff707d 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/base/LDAPConfigStore.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/base/LDAPConfigStore.java
@@ -26,13 +26,17 @@ import java.util.Map;
 import netscape.ldap.LDAPAttribute;
 import netscape.ldap.LDAPAttributeSet;
 import netscape.ldap.LDAPConnection;
+import netscape.ldap.LDAPConstraints;
+import netscape.ldap.LDAPControl;
 import netscape.ldap.LDAPEntry;
 import netscape.ldap.LDAPException;
 import netscape.ldap.LDAPModification;
 
-import com.netscape.certsrv.base.EBaseException;
 import com.netscape.certsrv.base.IConfigStore;
+import com.netscape.certsrv.ldap.ELdapException;
 import com.netscape.certsrv.ldap.ILdapConnFactory;
+import com.netscape.cmsutil.ldap.LDAPPostReadControl;
+import com.netscape.cmsutil.ldap.LDAPUtil;
 
 /**
  * LDAPConfigStore:
@@ -65,8 +69,6 @@ public class LDAPConfigStore extends PropConfigStore implements IConfigStore {
      * @param attr Name of attribute containing config store
      * @param createAttrs Set of initial attributes if creating the entry.  Should
      *              contain cn, objectclass and possibly other attributes.
-     *
-     * @exception EBaseException failed to create file configuration
      */
     public LDAPConfigStore(
         ILdapConnFactory dbFactory,
@@ -102,7 +104,17 @@ public class LDAPConfigStore extends PropConfigStore implements IConfigStore {
      *
      * @param createBackup Ignored.
      */
-    public void commit(boolean createBackup) throws EBaseException {
+    public void commit(boolean createBackup) throws ELdapException {
+        String[] attrs = {};
+        commitReturn(createBackup, attrs);
+    }
+
+    /**
+     * This version of commit also returns the post-read entry that
+     * the change resulted in.
+     */
+    public LDAPEntry commitReturn(boolean createBackup, String[] attrs)
+            throws ELdapException {
         ByteArrayOutputStream data = new ByteArrayOutputStream();
         save(data, null);
 
@@ -110,26 +122,37 @@ public class LDAPConfigStore extends PropConfigStore implements IConfigStore {
 
         LDAPConnection conn = dbFactory.getConn();
 
+        LDAPConstraints cons = new LDAPConstraints();
+        cons.setServerControls(new LDAPPostReadControl(true, attrs));
+
+        LDAPControl[] responseControls;
+
         // first attempt to modify; if modification fails (due
         // to no such object), try and add the entry instead.
         try {
             try {
-                commitModify(conn, configAttr);
+                commitModify(conn, configAttr, cons);
             } catch (LDAPException e) {
                 if (e.getLDAPResultCode() == LDAPException.NO_SUCH_OBJECT) {
-                    commitAdd(conn, configAttr);
+                    commitAdd(conn, configAttr, cons);
                 } else {
                     throw e;
                 }
             }
+            responseControls = conn.getResponseControls();
         } catch (LDAPException e) {
-            throw new EBaseException(
+            throw new ELdapException(
                 "Error writing LDAPConfigStore '"
                 + dn + "': " + e.toString()
             );
         } finally {
             dbFactory.returnConn(conn);
         }
+
+        LDAPPostReadControl control = (LDAPPostReadControl)
+            LDAPUtil.getControl(LDAPPostReadControl.class, responseControls);
+
+        return control.getEntry();
     }
 
     /**
@@ -139,12 +162,14 @@ public class LDAPConfigStore extends PropConfigStore implements IConfigStore {
      * @param configAttr Config store attribute.
      * @return true on success, false if the entry does not exist.
      */
-    private void commitModify(LDAPConnection conn, LDAPAttribute configAttr)
-        throws LDAPException
-    {
+    private void commitModify(
+            LDAPConnection conn,
+            LDAPAttribute configAttr,
+            LDAPConstraints cons)
+            throws LDAPException {
         LDAPModification ldapMod =
             new LDAPModification(LDAPModification.REPLACE, configAttr);
-        conn.modify(dn, ldapMod);
+        conn.modify(dn, ldapMod, cons);
     }
 
     /**
@@ -154,12 +179,14 @@ public class LDAPConfigStore extends PropConfigStore implements IConfigStore {
      * @param configAttr Config store attribute.
      * @return true on success, false if the entry already exists.
      */
-    private void commitAdd(LDAPConnection conn, LDAPAttribute configAttr)
-        throws LDAPException
-    {
+    private void commitAdd(
+            LDAPConnection conn,
+            LDAPAttribute configAttr,
+            LDAPConstraints cons)
+            throws LDAPException {
         LDAPAttributeSet attrSet = new LDAPAttributeSet(createAttrs);
         attrSet.add(configAttr);
         LDAPEntry ldapEntry = new LDAPEntry(dn, attrSet);
-        conn.add(ldapEntry);
+        conn.add(ldapEntry, cons);
     }
 }
diff --git a/base/server/cmscore/src/com/netscape/cmscore/profile/AbstractProfileSubsystem.java b/base/server/cmscore/src/com/netscape/cmscore/profile/AbstractProfileSubsystem.java
index cf5d77f19..e93e090de 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/profile/AbstractProfileSubsystem.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/profile/AbstractProfileSubsystem.java
@@ -95,10 +95,6 @@ public abstract class AbstractProfileSubsystem implements IProfileSubsystem {
 
         profile.getConfigStore().putString(PROP_ENABLE, "true");
         profile.getConfigStore().putString(PROP_ENABLE_BY, enableBy);
-        try {
-            profile.getConfigStore().commit(false);
-        } catch (EBaseException e) {
-        }
     }
 
     /**
@@ -117,9 +113,19 @@ public abstract class AbstractProfileSubsystem implements IProfileSubsystem {
         IProfile profile = mProfiles.get(id);
 
         profile.getConfigStore().putString(PROP_ENABLE, "false");
+    }
+
+    /**
+     * Commits a profile.
+     */
+    public void commitProfile(String id)
+            throws EProfileException {
         try {
-            profile.getConfigStore().commit(false);
+            mProfiles.get(id).getConfigStore().commit(false);
         } catch (EBaseException e) {
+            throw new EProfileException(
+                "Failed to commit config store of profile '" + id + ": " + e,
+                e);
         }
     }
 
diff --git a/base/server/cmscore/src/com/netscape/cmscore/profile/LDAPProfileSubsystem.java b/base/server/cmscore/src/com/netscape/cmscore/profile/LDAPProfileSubsystem.java
index 7be70dff1..b16a33fe2 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/profile/LDAPProfileSubsystem.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/profile/LDAPProfileSubsystem.java
@@ -19,9 +19,9 @@ package com.netscape.cmscore.profile;
 
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
-import java.util.Enumeration;
 import java.util.Hashtable;
 import java.util.LinkedHashMap;
+import java.util.TreeMap;
 
 import netscape.ldap.LDAPAttribute;
 import netscape.ldap.LDAPConnection;
@@ -58,6 +58,10 @@ public class LDAPProfileSubsystem
     private boolean stopped = false;
     private Thread monitor;
 
+    /* Map of profileId -> entryUSN for the most recent view
+     * of the profile entry that this instance has seen */
+    private TreeMap<String,Integer> entryUSNs;
+
     /**
      * Initializes this subsystem with the given configuration
      * store.
@@ -74,6 +78,7 @@ public class LDAPProfileSubsystem
         // (re)init member collections
         mProfiles = new LinkedHashMap<String, IProfile>();
         mProfileClassIds = new Hashtable<String, String>();
+        entryUSNs = new TreeMap<>();
 
         IConfigStore cs = CMS.getConfigStore();
         IConfigStore dbCfg = cs.getSubStore("internaldb");
@@ -101,7 +106,7 @@ public class LDAPProfileSubsystem
     /**
      * Read the given LDAPEntry into the profile subsystem.
      */
-    private void readProfile(LDAPEntry ldapProfile) {
+    private synchronized void readProfile(LDAPEntry ldapProfile) {
         IPluginRegistry registry = (IPluginRegistry)
             CMS.getSubsystem(CMS.SUBSYSTEM_REGISTRY);
 
@@ -113,12 +118,24 @@ public class LDAPProfileSubsystem
         }
         profileId = LDAPDN.explodeDN(dn, true)[0];
 
+        Integer newEntryUSN = new Integer(
+                ldapProfile.getAttribute("entryUSN").getStringValueArray()[0]);
+        CMS.debug("readProfile: new entryUSN = " + newEntryUSN);
+
+        Integer knownEntryUSN = entryUSNs.get(profileId);
+        if (knownEntryUSN != null) {
+            CMS.debug("readProfile: known entryUSN = " + knownEntryUSN);
+            if (newEntryUSN <= knownEntryUSN) {
+                CMS.debug("readProfile: data is current");
+                return;
+            }
+        }
+
         String classId = (String)
             ldapProfile.getAttribute("classId").getStringValues().nextElement();
 
-        Enumeration<String> vals =
-            ldapProfile.getAttribute("certProfileConfig").getStringValues();
-        InputStream data = new ByteArrayInputStream(vals.nextElement().getBytes());
+        InputStream data = new ByteArrayInputStream(
+                ldapProfile.getAttribute("certProfileConfig").getByteValueArray()[0]);
 
         IPluginInfo info = registry.getPluginInfo("profile", classId);
         if (info == null) {
@@ -127,6 +144,7 @@ public class LDAPProfileSubsystem
             try {
                 CMS.debug("Start Profile Creation - " + profileId + " " + classId + " " + info.getClassName());
                 createProfile(profileId, classId, info.getClassName(), data);
+                entryUSNs.put(profileId, newEntryUSN);
                 CMS.debug("Done Profile Creation - " + profileId);
             } catch (EProfileException e) {
                 CMS.debug("Error creating profile '" + profileId + "'; skipping.");
@@ -210,6 +228,29 @@ public class LDAPProfileSubsystem
             readProfile(entry);
     }
 
+    @Override
+    public synchronized void commitProfile(String id) throws EProfileException {
+        LDAPConfigStore cs = (LDAPConfigStore) mProfiles.get(id).getConfigStore();
+        try {
+            String[] attrs = {"entryUSN"};
+            LDAPEntry entry = cs.commitReturn(false, attrs);
+
+            LDAPAttribute attr = null;
+            if (entry != null)
+                attr = entry.getAttribute("entryUSN");
+
+            Integer entryUSN = null;
+            if (attr != null)
+                entryUSN = new Integer(attr.getStringValueArray()[0]);
+
+            entryUSNs.put(id, entryUSN);
+            CMS.debug("commitProfile: new entryUSN = " + entryUSN);
+        } catch (ELdapException e) {
+            throw new EProfileException(
+                "Failed to commit config store of profile '" + id + ": " + e);
+        }
+    }
+
     /**
      * Forget a profile without deleting it from the database.
      *
@@ -219,6 +260,7 @@ public class LDAPProfileSubsystem
     private void forgetProfile(String id) {
         mProfiles.remove(id);
         mProfileClassIds.remove(id);
+        entryUSNs.remove(id);
     }
 
     private void forgetProfile(LDAPEntry entry) {
@@ -253,6 +295,7 @@ public class LDAPProfileSubsystem
     private void forgetAllProfiles() {
         mProfiles.clear();
         mProfileClassIds.clear();
+        entryUSNs.clear();
     }
 
     /**
@@ -285,9 +328,10 @@ public class LDAPProfileSubsystem
                 cons.setServerControls(persistCtrl);
                 cons.setBatchSize(1);
                 cons.setServerTimeLimit(0 /* seconds */);
+                String[] attrs = {"*", "entryUSN"};
                 LDAPSearchResults results = conn.search(
                     dn, LDAPConnection.SCOPE_ONE, "(objectclass=*)",
-                    null, false, cons);
+                    attrs, false, cons);
                 while (!stopped && results.hasMoreElements()) {
                     LDAPEntry entry = results.next();
                     LDAPEntryChangeControl changeControl = (LDAPEntryChangeControl)