[#1172] Refuse a second check-references-filter-criteria value for an attribute type, and enforce all the filters of a configuration that already has one

[vharseko]

Fixes #1172

Problem

ReferentialIntegrityPlugin.applyConfigurationChange stored each check-references-filter-criteria value with newAttrFiltMap.put(attrType, filter), so a later value for the same attribute type overwrote an earlier one, while the validation checked each value on its own and accepted the configuration. Which filter was enforced depended on the sort order of the normalized values: the one that sorts last won. Affected pairs: the same spelling (manager:(a) + manager:(b)), a name and an OID or alias of the same type (manager + 0.9.2342.19200300.100.1.10), and, since #1158, a space before the colon (manager:(a) + manager :(b)).

Fix

The approach follows #1118 in the same plugin: refuse what is being configured now, load what is already stored.

• Enabling the plugin or changing its configuration is refused when two values name the same attribute type (compared as AttributeType, so by OID): new ERR_PLUGIN_REFERENT_DUPLICATE_FILTER_CRITERIA_133, one reason per attribute type, naming the attribute and all of its values.
• A configuration already stored with such values is still loaded when the server starts, with WARN_PLUGIN_REFERENT_DUPLICATE_FILTER_CRITERIA_134, and the filters of one attribute type are combined in an AND filter: a reference has to match all of them. Refusing it would stop the plugin from loading, including the delete and modify DN clean-up; keeping "last one wins" would keep an arbitrary choice.
• Disabling a plugin loaded with a warning is accepted, for this case and for the The shipped Referential Integrity plugin entry lacks the preOperationAdd and preOperationModify plugin types, so check-references checks nothing #1118 one: a disabled configuration is checked only for what loading it needs, as PluginConfigManager does, and enabling it again is still refused.
• isConfigurationAcceptableIgnoringCheckReferencesPluginTypes is renamed isConfigurationLoadable, since it now lets through both the The shipped Referential Integrity plugin entry lacks the preOperationAdd and preOperationModify plugin types, so check-references checks nothing #1118 and the Referential integrity: two check-references-filter-criteria values for the same attribute type are both accepted, but only one is enforced #1172 cases.
• The property description (configuration XML) and the Developer's Guide (chap-groups.adoc) say that an attribute has one filter, that several criteria go into one AND filter, and that a second value is refused while the plugin is enabled. The XML has no (&…) example: the description goes into the generated Javadoc of opendj-config, where & fails doclint (bad HTML entity).

Message ordinals 133–134 in plugin.properties: no other open PR changes that file.

Tests

ReferentialIntegrityPluginTestCase, each for the three pairs above (manager:(employeeType=manager) with (description=approved) as manager:, as the OID, and as manager :):

• testDuplicateFilterCriteriaIsNotAcceptable: isConfigurationAcceptable is false, with one reason naming manager and both values.
• testDuplicateFilterCriteriaIsLoadedWithWarning: initializePlugin succeeds and logs message 134 with the DN, the attribute and both values.
• testDuplicateFilterCriteriaAreAllEnforced: a plugin loaded with both values refuses an employee whose manager matches only (employeeType=manager), refuses one whose manager matches only (description=approved), and accepts one whose manager matches both. The two "only one" steps make sure that neither filter alone is the one enforced.
• testDuplicateFilterCriteriaIsRejected: on the running plugin, adding manager :(description=approved) next to manager:(employeeType=manager) is refused, and the reason names both values.
• testDuplicateFilterCriteriaCanBeDisabled, and testCheckReferencesWithoutPreOperationTypesCanBeDisabled for the The shipped Referential Integrity plugin entry lacks the preOperationAdd and preOperationModify plugin types, so check-references checks nothing #1118 configurations: a plugin loaded with the warning accepts its configuration with enabled: false, and still refuses it with enabled: true.
• testFilterCriteriaForTwoAttributeTypesAreNotDuplicates: manager:(…) with member:(…) is acceptable, and a manager that matches only the manager filter is accepted.

The #1158 case of validConfigs with a colon inside the filter now has one value, member:(&(o=urn:example)(cn:dn:=x)), so that it stays a clean load.

Without the fix (on #1167) the 10 cases of the first round fail and the other 65 pass. With it the class passes 82/82. The 6 disable cases fail with the first round's isConfigurationChangeAcceptable, and testFilterCriteriaForTwoAttributeTypesAreNotDuplicates fails when every value is grouped under the first attribute type, whether in the duplicate check or in the filters that are enforced.

Related

While writing the enforcement test I found that both doPreOperation hooks stop after their first check, because a passing check's result code is null rather than SUCCESS; that is why the test checks continueProcessing(). Not changed here: tracked in #1173.

[maximthomas]

praise: The fix closes the "last value wins" gap where it starts, and keeps stored configurations loading.

• getDuplicateFilterCriteria groups by AttributeType, so the name, the OID and manager : all land on one key, and ERR_PLUGIN_REFERENT_DUPLICATE_FILTER_CRITERIA_133 names every value.
• testDuplicateFilterCriteriaAreAllEnforced refuses a manager that matches only the first filter, then one that matches only the second, so neither filter alone can pass for the AND.
• isConfigurationLoadable loads a stored duplicate configuration with WARN_PLUGIN_REFERENT_DUPLICATE_FILTER_CRITERIA_134, so the delete and modify DN clean-up keeps running after an upgrade.

---

question (non-blocking): Should set enabled:false be accepted on a plugin that was loaded at boot with duplicate filter criteria?

opendj-server-legacy/src/main/java/org/opends/server/plugins/ReferentialIntegrityPlugin.java:299-304, :468

initializePlugin registers the plugin's own change listener (:183). isConfigurationChangeAcceptable delegates to isConfigurationAcceptable, and its duplicate loop refuses whatever enabled says. ConfigurationHandler.replaceEntry (:620-628) asks every listener on the DN, so an operator who sees the 134 warning and runs dsconfig set-plugin-prop --set enabled:false gets ERR_CONFIG_FILE_MODIFY_REJECTED_BY_CHANGE_LISTENER. The disable only goes through if the same modify also drops a value. PluginConfigManager itself skips a disabled configuration. The PR body says changing the configuration is refused, but it does not say the disable is. The #1118 arm already behaves this way at BASE. Minor as it stands; Major if a disable was meant to stay possible. If it was, one change fixes both arms:

  @Override
  public boolean isConfigurationChangeAcceptable(
          ReferentialIntegrityPluginCfg configuration,
          List<LocalizableMessage> unacceptableReasons)
  {
    // A disabled plugin checks no references: let the disable through, as PluginConfigManager does.
    return configuration.isEnabled()
        ? isConfigurationAcceptable(configuration, unacceptableReasons)
        : isConfigurationLoadable(configuration, unacceptableReasons);
  }

Pin: initialise a plugin from duplicateFilterCriteriaEntry(first, second), then call isConfigurationChangeAcceptable on the same entry with ds-cfg-enabled: false. It returns false at this head and true with the change.

---

suggestion (non-blocking): No test pins that getDuplicateFilterCriteria groups values by attribute type.

opendj-server-legacy/src/main/java/org/opends/server/plugins/ReferentialIntegrityPlugin.java:323, :241

Each multi-value fixture names one type twice: the duplicateFilterCriteria provider, and validConfigs at ReferentialIntegrityPluginTestCase.java:532-533. Take a mutant that files every value under one key, or flags any configuration with size() > 1. It refuses the ordinary member:(…) + uniqueMember:(…) configuration, and all 75 cases stay green. An AND across types at :241 would survive the same way.

  /** Issue #1172: filter criteria for two different attribute types are not duplicates. */
  @Test
  public void testFilterCriteriaForTwoAttributeTypesAreAcceptable() throws Exception
  {
    Entry e = TestCaseUtils.makeEntry(
        "dn: cn=Referential Integrity,cn=Plugins,cn=config",
        "objectClass: top",
        "objectClass: ds-cfg-plugin",
        "objectClass: ds-cfg-referential-integrity-plugin",
        "cn: Referential Integrity",
        "ds-cfg-java-class: org.opends.server.plugins.ReferentialIntegrityPlugin",
        "ds-cfg-enabled: true",
        "ds-cfg-plugin-type: postOperationDelete",
        "ds-cfg-plugin-type: postOperationModifyDN",
        "ds-cfg-plugin-type: subordinateModifyDN",
        "ds-cfg-plugin-type: subordinateDelete",
        "ds-cfg-plugin-type: preOperationAdd",
        "ds-cfg-plugin-type: preOperationModify",
        "ds-cfg-attribute-type: manager",
        "ds-cfg-attribute-type: member",
        "ds-cfg-base-dn: dc=example,dc=com",
        "ds-cfg-check-references: true",
        "ds-cfg-check-references-filter-criteria: manager:(employeeType=manager)",
        "ds-cfg-check-references-filter-criteria: member:(objectclass=person)");
    List<LocalizableMessage> reasons = new ArrayList<>();

    boolean acceptable = new ReferentialIntegrityPlugin().isConfigurationAcceptable(
        InitializationUtils.getConfiguration(ReferentialIntegrityPluginCfgDefn.getInstance(), e), reasons);

    assertTrue(acceptable, reasons.toString());
  }

Pin: green at BASE and at this head, red under the single-key mutant.

---

question (non-blocking): Should the colon-in-filter validConfigs entry keep two member: values, now that #1172 refuses that shape?

opendj-server-legacy/src/test/java/org/opends/server/plugins/ReferentialIntegrityPluginTestCase.java:532-533

This entry comes from the stacked #1158 commit, and #1172 leaves it as it is. testInitializeWithValidConfigs loads it through isConfigurationLoadable, which logs message 134 and ANDs the filters. isConfigurationAcceptable refuses the same entry with message 133. So the case runs the legacy warning road, which testDuplicateFilterCriteriaIsLoadedWithWarning already covers, and not a clean load. No #1158 mutant survives, so this is about what the fixture means.

            "ds-cfg-check-references-filter-criteria: member:(&(o=urn:example)(cn:dn:=x))"

Pin: replace both lines with the one above. The case becomes a clean load, and it still covers a colon inside the filter.

---

issue (non-blocking): The new Developer's Guide sentence says OpenDJ refuses a second value for the same attribute. That is not true while the plugin is disabled.

opendj-doc-generated-ref/src/main/asciidoc/server-dev-guide/chap-groups.adoc:542

cn=Referential Integrity ships with ds-cfg-enabled: false. A disabled plugin has no instance and no listener of its own, and PluginConfigManager accepts any change to a disabled configuration. So the second value is stored, and the refusal only comes at set enabled:true. The check-references sentence in the same paragraph is already qualified with "When the plugin is enabled".

When the plugin is enabled, OpenDJ refuses a second value for the same attribute, and it refuses to enable a plugin configured that way. A configuration that already has one when the server starts is loaded with a warning and requires all of its filters.

[vharseko]

@maximthomas all four points are taken in cb3d276. #1167 is merged, so the branch is now rebased onto master 6e6cbc7 and carries only the #1172 commits.

question: disabling a plugin loaded with a warning: a disable was meant to stay possible, so this was a bug, in the #1118 arm too. isConfigurationChangeAcceptable now uses your version. A configuration with enabled: false is only checked with isConfigurationLoadable, the same check that let the plugin load at startup, so the plugin's own applyConfigurationChange still never receives a filter it cannot parse. Enabling the plugin again is still refused: PluginConfigManager checks the new instance with isConfigurationAcceptable. There are two pins, testDuplicateFilterCriteriaCanBeDisabled and testCheckReferencesWithoutPreOperationTypesCanBeDisabled, one for each arm. Each loads the plugin from the configuration and expects isConfigurationChangeAcceptable to accept the same entry with enabled: false and to refuse it with enabled: true.

suggestion: grouping by attribute type: added as testFilterCriteriaForTwoAttributeTypesAreNotDuplicates, with manager:(employeeType=manager) and member:(description=groupMember). Your snippet's isConfigurationAcceptable check only covers the grouping in getDuplicateFilterCriteria. The test also loads the plugin and adds an employee whose manager matches only the manager filter, which pins the AND at :241 as well.

question: the colon-in-filter validConfigs entry: replaced with your single value member:(&(o=urn:example)(cn:dn:=x)). It is a clean load again, and it still covers #1158: a split at the last colon would read member:(&(o=urn:example)(cn:dn as the attribute.

issue: the guide sentence: replaced with your wording.

Run locally: ReferentialIntegrityPluginTestCase 82/82 in the reactor. Three mutants are each caught by the new tests and by nothing else:

• the first round's isConfigurationChangeAcceptable: the 6 disable cases;
• every value grouped under the first attribute type in getDuplicateFilterCriteria: testFilterCriteriaForTwoAttributeTypesAreNotDuplicates;
• every filter ANDed under the first attribute type in applyConfigurationChange: the same test.

[maximthomas]

praise: The round-1 feedback is resolved in the code, the documentation and the tests.

• isConfigurationChangeAcceptable now branches on isEnabled(). A plugin loaded with the #1118 or #1172 warning can be disabled without first fixing its configuration, and enabling that same configuration is still refused (testDuplicateFilterCriteriaCanBeDisabled, testCheckReferencesWithoutPreOperationTypesCanBeDisabled).
• chap-groups.adoc:542 now limits the refusal to "When the plugin is enabled", which is when PluginConfigManager actually checks it.
• testDuplicateFilterCriteriaAreAllEnforced has two "matches only one" steps, so neither filter alone can be the one enforced