Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attribute Uniqueness Plugin uses wrong subtree on ModRDN #4763

Closed
theSuess opened this issue May 10, 2021 · 6 comments · Fixed by #4871 or #4907
Closed

Attribute Uniqueness Plugin uses wrong subtree on ModRDN #4763

theSuess opened this issue May 10, 2021 · 6 comments · Fixed by #4871 or #4907
Assignees
Labels
In JIRA ticket is in JIRA priority_high need urgent fix / highly valuable / easy to fix
Milestone

Comments

@theSuess
Copy link

Issue Description
When using the Attribute uniqueness plugin, restricted to one subtree, moving an object with an already existing attribute to this subtree does not raise any exceptions. It appears that the originating subtree is searched instead.

Package Version and Platform:

  • Platform: Containerized Fedora (tested on 33 and 34)
  • Package and version: Tested with all versions >= 2.0.2

Steps to Reproduce

  1. Create two container objects e.g ou=c1,dc=example,dc=com and ou=c2,dc=example,dc=com
  2. Create an entry for the attr-uniq plugin restricted to the c1 subtree, restricting (for example) the mail attribute.
  3. Create two objects with the same value for mail in c2 (this should be allowed).
  4. Move one object to c1
  5. Move the second object to c1. This should raise an exeption as another object with the same value for mail already exists in c1 but works without issues.
  6. Additionally, trying to move one of these objects back to c2 raises an exception even though there is no uniqueness constraint on c2.

Expected results
The attr-uniq plugin should prevent invalid move operations.

Additional context
I've tried to get to the root cause of this issue, and debug logs suggest that https://github.com/389ds/389-ds-base/blob/master/ldap/servers/plugins/uiduniq/uid.c#L1395 is called with the old parentDN. However, as this is my first time digging through the codebase I was unable to figure out why this happens.

@theSuess theSuess added the needs triage The issue will be triaged during scrum label May 10, 2021
@theSuess theSuess changed the title Attribute Uniqueness Plugin uses wrong tree subtree on ModRDN Attribute Uniqueness Plugin uses wrong subtree on ModRDN May 10, 2021
@mreynolds389 mreynolds389 added priority_high need urgent fix / highly valuable / easy to fix and removed needs triage The issue will be triaged during scrum labels Jun 30, 2021
@mreynolds389 mreynolds389 added this to the 1.4.4 milestone Jun 30, 2021
@mreynolds389
Copy link
Contributor

@mreynolds389 mreynolds389 added the In JIRA ticket is in JIRA label Aug 4, 2021
@progier389
Copy link
Contributor

IMHO the issue is that we use "sdn" (the target entry parent) instead of "superior" (the renamed entry parent) while calling findSubtreeAndSearch / searchAllSubtrees

@droideck droideck self-assigned this Aug 9, 2021
droideck added a commit that referenced this issue Aug 18, 2021
…#4871)

Bug Description: When using the Attribute uniqueness plugin, restricted
to one subtree, moving an object with an already existing attribute
to this subtree does not raise any exceptions. It appears that the
originating subtree is searched instead.

Fix Description: Use parent DN of the new entry when searching
for attribute uniqueness.
Add test to plugins/attruniq_test.py suite.

Fixes: #4763

Reviewed by: @tbordaz (Thanks!)
@mreynolds389
Copy link
Contributor

Can this backported down to 1.4.3 please?

droideck added a commit that referenced this issue Aug 18, 2021
…#4871)

Bug Description: When using the Attribute uniqueness plugin, restricted
to one subtree, moving an object with an already existing attribute
to this subtree does not raise any exceptions. It appears that the
originating subtree is searched instead.

Fix Description: Use parent DN of the new entry when searching
for attribute uniqueness.
Add test to plugins/attruniq_test.py suite.

Fixes: #4763

Reviewed by: @tbordaz (Thanks!)
droideck added a commit that referenced this issue Aug 18, 2021
…#4871)

Bug Description: When using the Attribute uniqueness plugin, restricted
to one subtree, moving an object with an already existing attribute
to this subtree does not raise any exceptions. It appears that the
originating subtree is searched instead.

Fix Description: Use parent DN of the new entry when searching
for attribute uniqueness.
Add test to plugins/attruniq_test.py suite.

Fixes: #4763

Reviewed by: @tbordaz (Thanks!)
droideck added a commit that referenced this issue Aug 18, 2021
…#4871)

Bug Description: When using the Attribute uniqueness plugin, restricted
to one subtree, moving an object with an already existing attribute
to this subtree does not raise any exceptions. It appears that the
originating subtree is searched instead.

Fix Description: Use parent DN of the new entry when searching
for attribute uniqueness.
Add test to plugins/attruniq_test.py suite.

Fixes: #4763

Reviewed by: @tbordaz (Thanks!)
@droideck
Copy link
Member

c7e7259..2aff98c 389-ds-base-1.4.3 -> 389-ds-base-1.4.3
4c5a75f..a3d4f63 389-ds-base-1.4.4 -> 389-ds-base-1.4.4
c7a6796..9cf2517 389-ds-base-2.0 -> 389-ds-base-2.0

@tbordaz
Copy link
Contributor

tbordaz commented Sep 2, 2021

@droideck would you revisit the fix as it creates a regression detected by IPA (#2000420).
The fix changes the target DN from the source to destination entry of MODRDN. This param is just to prevent Uniqueness checking to fail because of the target entry itself. I think the code was using 'sdn' was possibly valid and I wonder if the test was successful even without the fix.

The scope of testing is defined by the plugin attribute, not by the source or destination entry.

@tbordaz tbordaz reopened this Sep 2, 2021
@progier389
Copy link
Contributor

@droideck ,
As the search functions check the suffixes handled by the plugin, IMHO Thierry is wrong and the providing source or destination matters
But the point I missed is that these search functions also used the dn to exclude the source entry from the result set.
So if I understand rightly: ==>
In findSubtreeAndSearch case, we changed the wrong parameter:
it is the "parent" that should be changed to the ndn instead of the "target"
In searchAllSubtrees a new parameter must be added to distinguish the destination dn
(that should be used to check the suffixes handled by the plugin) from the source dn (that should be used to ignore the entry in the result set )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In JIRA ticket is in JIRA priority_high need urgent fix / highly valuable / easy to fix
Projects
None yet
5 participants