I have created a gerrit review for this patchset: https://review.gerrithub.io/#/c/357607/ Thanks, Fraser On Tue, Feb 07, 2017 at 09:39:52PM +1000, Fraser Tweedale wrote:

Please review the attached patches which fix https://fedorahosted.org/pki/ticket/2588, a bug in profile modification where config params can only be added or changed, but not removed.

Thanks, Fraser

From 0a86f63cfe2d5391befe401541e9dcc0dae6ce29 Mon Sep 17 00:00:00 2001 From: Fraser Tweedale <ftweedal@redhat.com> Date: Tue, 7 Feb 2017 17:27:06 +1000 Subject: [PATCH 159/161] LDAPProfileSubsystem: avoid duplicating logic in superclass

Part of: https://fedorahosted.org/pki/ticket/2588 --- .../cmscore/profile/AbstractProfileSubsystem.java | 7 +++- .../cmscore/profile/LDAPProfileSubsystem.java | 43 ++++------------------ 2 files changed, 13 insertions(+), 37 deletions(-)

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 116b8e2026e80b012fb87647fd8924b567194fa3..2a209ad5b2656d65db57d36b7ecb2745527ab081 100644 --- a/base/server/cmscore/src/com/netscape/cmscore/profile/AbstractProfileSubsystem.java +++ b/base/server/cmscore/src/com/netscape/cmscore/profile/AbstractProfileSubsystem.java @@ -121,7 +121,7 @@ public abstract class AbstractProfileSubsystem implements IProfileSubsystem { /** * Commits a profile. */ - public void commitProfile(String id) + public synchronized void commitProfile(String id) throws EProfileException { IConfigStore cs = mProfiles.get(id).getConfigStore();

@@ -157,6 +157,11 @@ public abstract class AbstractProfileSubsystem implements IProfileSubsystem {

// finally commit the configStore // + commitConfigStore(id, cs); + } + + protected void commitConfigStore(String id, IConfigStore cs) + throws EProfileException { try { cs.commit(false); } catch (EBaseException 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 fff8ead3f2088aedaf5856c308dd33be90af7779..bce675e7bf993d97a086fb830e34d50000c4f396 100644 --- a/base/server/cmscore/src/com/netscape/cmscore/profile/LDAPProfileSubsystem.java +++ b/base/server/cmscore/src/com/netscape/cmscore/profile/LDAPProfileSubsystem.java @@ -303,43 +303,14 @@ public class LDAPProfileSubsystem readProfile(entry); }

+ /** + * Commit the configStore and track the resulting + * entryUSN and (in case of add) the nsUniqueId + */ @Override - public synchronized void commitProfile(String id) throws EProfileException { - LDAPConfigStore cs = (LDAPConfigStore) mProfiles.get(id).getConfigStore(); - - // first create a *new* profile object from the configStore - // and initialise it with the updated configStore - // - IPluginRegistry registry = (IPluginRegistry) - CMS.getSubsystem(CMS.SUBSYSTEM_REGISTRY); - String classId = mProfileClassIds.get(id); - IPluginInfo info = registry.getPluginInfo("profile", classId); - String className = info.getClassName(); - IProfile newProfile = null; - try { - newProfile = (IProfile) Class.forName(className).newInstance(); - } catch (ClassNotFoundException | InstantiationException | IllegalAccessException e) { - throw new EProfileException("Could not instantiate class '" - + classId + "' for profile '" + id + "': " + e); - } - newProfile.setId(id); - try { - newProfile.init(this, cs); - } catch (EBaseException e) { - throw new EProfileException( - "Failed to initialise profile '" + id + "': " + e); - } - - // next replace the existing profile with the new profile; - // this is to avoid any intermediate state where the profile - // is not fully initialised with its inputs, outputs and - // policy objects. - // - mProfiles.put(id, newProfile); - - // finally commit the configStore and track the resulting - // entryUSN and (in case of add) the nsUniqueId - // + protected void commitConfigStore(String id, IConfigStore configStore) + throws EProfileException { + LDAPConfigStore cs = (LDAPConfigStore) configStore; try { String[] attrs = {"entryUSN", "nsUniqueId"}; LDAPEntry entry = cs.commitReturn(false, attrs); -- 2.9.3

From ca09f58f4a953fb8d40898a1924f236bba42fa29 Mon Sep 17 00:00:00 2001 From: Fraser Tweedale <ftweedal@redhat.com> Date: Tue, 7 Feb 2017 17:39:33 +1000 Subject: [PATCH 160/161] ISourceConfigStore: add clear() method to interface

The SourceConfigStore load() method does not clear the config store, but this might be necessary to avoid stale data if wanting to perform a complete replacement of the data (e.g. reload from file).

We should not change the behaviour of load() in case some code is relying on the current behaviour, so add the clear() method to the interface.

Part of: https://fedorahosted.org/pki/ticket/2588 --- base/common/src/com/netscape/certsrv/base/ISourceConfigStore.java | 5 +++++ .../cmscore/src/com/netscape/cmscore/base/PropConfigStore.java | 4 ++++ 2 files changed, 9 insertions(+)

diff --git a/base/common/src/com/netscape/certsrv/base/ISourceConfigStore.java b/base/common/src/com/netscape/certsrv/base/ISourceConfigStore.java index 42637c258258472d8469fd8c691a826e3bcb1b3e..8eb86c2ba5cfd1522f52b98a676d1a509424c270 100644 --- a/base/common/src/com/netscape/certsrv/base/ISourceConfigStore.java +++ b/base/common/src/com/netscape/certsrv/base/ISourceConfigStore.java @@ -63,6 +63,11 @@ public interface ISourceConfigStore extends Serializable { public Enumeration<String> keys();

/** + * Clear the config store. + */ + public void clear(); + + /** * Reads a config store from an input stream. * * @param in input stream where the properties are located diff --git a/base/server/cmscore/src/com/netscape/cmscore/base/PropConfigStore.java b/base/server/cmscore/src/com/netscape/cmscore/base/PropConfigStore.java index cc16e247d01428f958d0d397ff95127fcb8d2f45..acf28441337b76628d47dc58351a0233568397d8 100644 --- a/base/server/cmscore/src/com/netscape/cmscore/base/PropConfigStore.java +++ b/base/server/cmscore/src/com/netscape/cmscore/base/PropConfigStore.java @@ -223,6 +223,10 @@ public class PropConfigStore implements IConfigStore, Cloneable { } }

+ public synchronized void clear() { + mSource.clear(); + } + /** * Reads a config store from an input stream. * -- 2.9.3

From 87ceb6f2e0ad93359f6ca94a08a8cb7f2bbfeaaa Mon Sep 17 00:00:00 2001 From: Fraser Tweedale <ftweedal@redhat.com> Date: Tue, 7 Feb 2017 21:12:08 +1000 Subject: [PATCH 161/161] ProfileService: clear profile attributes when modifying

When modifying a profile, attributes are not cleared. Attributes that were removed in the updated profile configuration are not actually removed.

When updating a profile via PUT /ca/rest/profiles/{id}/raw, clear the config store before loading the new configuration.

Fixes: https://fedorahosted.org/pki/ticket/2588 --- base/ca/src/org/dogtagpki/server/ca/rest/ProfileService.java | 1 + 1 file changed, 1 insertion(+)

diff --git a/base/ca/src/org/dogtagpki/server/ca/rest/ProfileService.java b/base/ca/src/org/dogtagpki/server/ca/rest/ProfileService.java index 41d009b9d38003f054f45c2dd8070de8f46065f3..26e86c51d388d83fc7070b355505f011426350c0 100644 --- a/base/ca/src/org/dogtagpki/server/ca/rest/ProfileService.java +++ b/base/ca/src/org/dogtagpki/server/ca/rest/ProfileService.java @@ -738,6 +738,7 @@ public class ProfileService extends PKIService implements ProfileResource { }

// no error thrown, so commit updated profile config + profile.getConfigStore().clear(); profile.getConfigStore().load(new ByteArrayInputStream(data)); ps.disableProfile(profileId); ps.commitProfile(profileId); -- 2.9.3

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel