Skip to content

Fix ReferentialIntegrityPlugin silently bypassing check-references on modify operations#600

Open
Copilot wants to merge 7 commits intomasterfrom
copilot/fix-referential-integrity-plugin-bug
Open

Fix ReferentialIntegrityPlugin silently bypassing check-references on modify operations#600
Copilot wants to merge 7 commits intomasterfrom
copilot/fix-referential-integrity-plugin-bug

Conversation

Copy link
Copy Markdown

Copilot AI commented Mar 27, 2026

  • Add Portions copyright 2026 3A Systems, LLC to ReferentialIntegrityPlugin.java (modified file)
  • Add Portions copyright 2026 3A Systems, LLC to ReferentialIntegrityPluginTestCase.java (modified file)
Original prompt

Problem

Fixes issue #579: When check-references: true is configured in the Referential Integrity Plugin, adding a deleted (non-existent) user to a group via ldapmodify (modifying uniqueMember or member) succeeds without throwing a CONSTRAINT_VIOLATION error. The check is silently bypassed.

Root Cause

There are two bugs in the doPreOperation(PreOperationModifyOperation modifyOperation) method in:

opendj-server-legacy/src/main/java/org/opends/server/plugins/ReferentialIntegrityPlugin.java

Bug 1: break instead of continue (lines 1009–1013)

for (Modification mod : mods)
{
  final ModificationType modType = mod.getModificationType();

  /* Process only ADD and REPLACE modification types. */
  if (modType != ModificationType.ADD
      && modType != ModificationType.REPLACE)
  {
    break;  // ← BUG: stops the whole loop instead of skipping this modification
  }
  ...
}

If the first modification in the list is not ADD or REPLACE (e.g. DELETE), the loop exits entirely and no integrity check is performed for any subsequent ADD modifications.

Fix: Replace break with continue.

Bug 2: Reading attribute from the pre-modification entry instead of from the modification itself (line 1015)

Attribute modifiedAttribute = entry.getAttribute(mod.getAttribute().getAttributeDescription());

Here entry = modifyOperation.getModifiedEntry() is the current state of the entry before modifications are applied. If the attribute (e.g. uniqueMember) does not yet exist on the entry (first-time add to a group), getAttribute() returns null, the if (modifiedAttribute != null) check fails silently, and the integrity check is skipped entirely.

Fix: Read the attribute directly from the modification (mod.getAttribute()), which contains exactly the values being added/replaced.

Required Fix

In doPreOperation(PreOperationModifyOperation modifyOperation), replace:

for (Modification mod : mods)
{
  final ModificationType modType = mod.getModificationType();

  /* Process only ADD and REPLACE modification types. */
  if (modType != ModificationType.ADD
      && modType != ModificationType.REPLACE)
  {
    break;
  }

  Attribute modifiedAttribute = entry.getAttribute(mod.getAttribute().getAttributeDescription());
  if (modifiedAttribute != null)
  {
    PluginResult.PreOperation result =
    isIntegrityMaintained(modifiedAttribute, entryDN, entryBaseDN);
    if (result.getResultCode() != ResultCode.SUCCESS)
    {
      return result;
    }
  }
}

With:

for (Modification mod : mods)
{
  final ModificationType modType = mod.getModificationType();

  /* Process only ADD and REPLACE modification types. */
  if (modType != ModificationType.ADD
      && modType != ModificationType.REPLACE)
  {
    continue;  // ← fixed: skip, don't break
  }

  // ← fixed: use the attribute from the modification, not from the pre-mod entry
  Attribute modifiedAttribute = mod.getAttribute();
  if (modifiedAttribute != null && !modifiedAttribute.isEmpty())
  {
    PluginResult.PreOperation result =
        isIntegrityMaintained(modifiedAttribute, entryDN, entryBaseDN);
    if (result.getResultCode() != ResultCode.SUCCESS)
    {
      return result;
    }
  }
}

Additional Notes

  • The entry variable (obtained via modifyOperation.getModifiedEntry()) is no longer needed after this fix and can be removed (though entryDN still needs to be obtained — it can come from the modification's attribute or from the operation itself via modifyOperation.getEntryDN()).
  • A test should be added (or the existing test testEnforceIntegrityModifyGroupAddMissingMember verified) that:
    1. Deletes a user entry.
    2. Attempts to add that deleted user as uniqueMember of a group.
    3. Expects CONSTRAINT_VIOLATION.
  • The existing test testEnforceIntegrityModifyGroupAddMissingMember in ReferentialIntegrityPluginTestCase.java already covers the member attribute but should be extended to cover uniqueMember as well, matching the user's configuration.

Files to Modify

  • opendj-server-legacy/src/main/java/org/opends/server/plugins/ReferentialIntegrityPlugin.java
  • opendj-server-legacy/src/test/java/org/opends/server/plugins/ReferentialIntegrityPluginTestCase.java

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: прокомментируй

Assistant: Вот подробный анализ баг-репорта на основе изучения исходного кода.


Анализ Issue #579

Суть проблемы

Пользователь настроил Referential Integrity Plugin с check-references: true, ожидая, что при попытке добавить несуществующего пользователя в группу (uniqueMember) операция будет отклонена с ошибкой. Однако ldapmodify завершается успешно, и "мёртвая" ссылка попадает в атрибут.

Причина: баг в doPreOperation(PreOperationModifyOperation)

В коде плагина есть критическая ошибка в обработке modify-операций:


</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>

<!-- START COPILOT CODING AGENT TIPS -->
---

📍 Connect Copilot coding agent with [Jira](https://gh.io/cca-jira-docs), [Azure Boards](https://gh.io/cca-azure-boards-docs) or [Linear](https://gh.io/cca-linear-docs) to delegate work to Copilot in one click without leaving your project management tool.

…for modify operations

Agent-Logs-Url: https://github.com/OpenIdentityPlatform/OpenDJ/sessions/294a922f-c00a-410a-bc78-529db303e337

Co-authored-by: vharseko <6818498+vharseko@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix issue with silent bypass of CONSTRAINT_VIOLATION in Referential Integrity Plugin Fix ReferentialIntegrityPlugin silently bypassing check-references on modify operations Mar 27, 2026
Copilot AI requested a review from vharseko March 27, 2026 10:31
@vharseko vharseko marked this pull request as ready for review March 27, 2026 15:48
Copilot AI review requested due to automatic review settings March 27, 2026 15:48
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes Referential Integrity Plugin enforcement for check-references: true during modify operations so missing referenced entries (e.g., deleted users added to group membership attributes) correctly yield CONSTRAINT_VIOLATION instead of being silently accepted.

Changes:

  • Fix doPreOperation(PreOperationModifyOperation) to continue scanning modifications (not break early) and to validate the attribute values being added/replaced.
  • Add a regression test ensuring adding a missing uniqueMember via modify fails with CONSTRAINT_VIOLATION.
  • Add 2026 “Portions copyright …” headers to the modified source and test files.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
opendj-server-legacy/src/main/java/org/opends/server/plugins/ReferentialIntegrityPlugin.java Corrects modify pre-op integrity checks to properly validate added/replaced reference values.
opendj-server-legacy/src/test/java/org/opends/server/plugins/ReferentialIntegrityPluginTestCase.java Adds coverage for missing uniqueMember on modify when check-references is enabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@vharseko
Copy link
Copy Markdown
Member

@copilot fix build error

…() instead of getAttributeType()

Agent-Logs-Url: https://github.com/OpenIdentityPlatform/OpenDJ/sessions/e4c3f74e-8d96-4b8f-bdcc-91c814ffb791

Co-authored-by: vharseko <6818498+vharseko@users.noreply.github.com>
Copy link
Copy Markdown
Author

Copilot AI commented Mar 30, 2026

@copilot fix build error

Fixed in commit 943ae0d. The build error was a compilation failure: the code review suggestion used modifiedAttribute.getAttributeType(), but the Attribute type doesn't expose that method directly — the correct call is modifiedAttribute.getAttributeDescription().getAttributeType().

Copilot AI requested a review from vharseko March 30, 2026 09:05
@vharseko vharseko requested a review from Copilot March 30, 2026 10:15
@vharseko
Copy link
Copy Markdown
Member

@copilot fix build error

@vharseko
Copy link
Copy Markdown
Member

@copilot Error: /home/runner/work/OpenDJ/OpenDJ/opendj-server-legacy/src/test/java/org/opends/server/plugins/ReferentialIntegrityPluginTestCase.java:[1878,40] error: cannot find symbol
symbol: method newModifyRequest(DN)
location: class ReferentialIntegrityPluginTestCase

Copy link
Copy Markdown
Author

Copilot AI commented Mar 30, 2026

@copilot fix build error

Both compilation errors have been resolved: the getAttributeType() issue was fixed in 943ae0d, and the newModifyRequest symbol error was fixed in 148028b by using Requests.newModifyRequest(DN.valueOf(ugroup)). The builds for 148028b are currently queued.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • central.sonatype.com
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.14/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.14/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.14 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.14/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/OpenDJ/OpenDJ org.codehaus.plexus.classworlds.launcher.Launcher compile -pl opendj-server-legacy -am -q --no-transfer-progress (dns block)
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.14/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.14/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.14 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.14/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/OpenDJ/OpenDJ org.codehaus.plexus.classworlds.launcher.Launcher install -pl opendj-maven-plugin -q --no-transfer-progress (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copy link
Copy Markdown
Author

Copilot AI commented Mar 30, 2026

@copilot Error: /home/runner/work/OpenDJ/OpenDJ/opendj-server-legacy/src/test/java/org/opends/server/plugins/ReferentialIntegrityPluginTestCase.java:[1878,40] error: cannot find sy...

The error was already fixed in 148028bnewModifyRequest(DN.valueOf(ugroup)) was changed to Requests.newModifyRequest(DN.valueOf(ugroup)), where Requests resolves to the explicitly imported org.forgerock.opendj.ldap.requests.Requests which has the newModifyRequest(DN) method. The CI builds for 148028b are queued and should pass.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…he multi-modify

Added a description attribute to ugroup to ensure DELETE modification succeeds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants