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

Issue 5052 - BUG - Custom filters prevented entry deletion #5060

Merged
merged 1 commit into from
May 10, 2023

Conversation

Firstyear
Copy link
Contributor

Bug Description: When a custom filter was provided, entries
which were deleted in AD did not have that event correctly
reflected in 389-ds. This was due to the behaviour that when
an entry in AD is deleted, it is marked with a "deleted" flag
which the objectClass=* filter would (accidentally) collect
when it did a search. However, a custom user filter being
specified would in some cases (such as a memberOf filter)
NOT show up the deletion since the entry was considered
to have moved out of scope rather than being a full delete.

Fix Description: In the case that we have a userfilter, we
wrap it in an OR condition that always requests isDeleted
flags so that we can correctly reflect the delete status.

fixes: #5052

Author: William Brown william@blackhats.net.au

Review by: ???

Bug Description: When a custom filter was provided, entries
which were deleted in AD did not have that event correctly
reflected in 389-ds. This was due to the behaviour that when
an entry in AD is deleted, it is marked with a "deleted" flag
which the objectClass=* filter would (accidentally) collect
when it did a search. However, a custom user filter being
specified would in some cases (such as a memberOf filter)
NOT show up the deletion since the entry was considered
to have moved out of scope rather than being a full delete.

Fix Description: In the case that we have a userfilter, we
wrap it in an OR condition that always requests isDeleted
flags so that we can correctly reflect the delete status.

fixes: 389ds#5052

Author: William Brown <william@blackhats.net.au>

Review by: ???
*/
size_t buflen = 18 + strlen(userfilter);
filter = slapi_ch_calloc(1, buflen);
snprintf(filter, buflen, "(|(isDeleted=*)%s)", userfilter);
Copy link
Contributor

Choose a reason for hiding this comment

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

As I am not expert of Winsync, sorry for this dummy question.
This new filters looks a bit weird to me, because it will return all the matching entries (userfilter) and all the deleted entries. Is it what you expect or rather something like "(&(I(isDeleted=)(objectclass=))%s)".
Also are we sure that isDeleted attribute is indexed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch @tbordaz it looks like https://docs.microsoft.com/en-us/windows/win32/adschema/a-isdeleted is not indexed. But because of how the dirsync works, it's similar to syncrepl so it only sends us what has changed between the previous cookie and the current.

The other possible way to do this filter is:

(| (userFilter ...)  (&(isDeleted=True)(userFilter ...))  )

That way we can lean on the userFilter to be indexed, but also means that we'll limit the isDeleted results to our userFilter set. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that 'isDeleted' is similar to tombstone entry. So the initial filter would match all tombstones whether or not they match the userFilter.
IIUC the need, the second filter you proposed looks good to me

(| (userFilter ...)  (&(isDeleted=True)(userFilter ...))  )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tbordaz See #5052 (comment) I think there is no way to fix this ....

Copy link
Contributor

Choose a reason for hiding this comment

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

@tbordaz @Firstyear - any update on this? Thierry do you approve this after Williams remarks? Or do we still have concerns?

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely, Sorry I missed that it was pending on my update

Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

LGTM

@Firstyear
Copy link
Contributor Author

Thanks @mreynolds389 and @tbordaz - I'll merge this later this week once I can do backports and builds and such :)

@Firstyear Firstyear merged commit 05528f6 into 389ds:main May 10, 2023
@Firstyear Firstyear deleted the 5052-winsync-filter-delete branch May 10, 2023 14:25
Firstyear added a commit that referenced this pull request May 10, 2023
Bug Description: When a custom filter was provided, entries
which were deleted in AD did not have that event correctly
reflected in 389-ds. This was due to the behaviour that when
an entry in AD is deleted, it is marked with a "deleted" flag
which the objectClass=* filter would (accidentally) collect
when it did a search. However, a custom user filter being
specified would in some cases (such as a memberOf filter)
NOT show up the deletion since the entry was considered
to have moved out of scope rather than being a full delete.

Fix Description: In the case that we have a userfilter, we
wrap it in an OR condition that always requests isDeleted
flags so that we can correctly reflect the delete status.

fixes: #5052

Author: William Brown <william@blackhats.net.au>

Review by: @mreynolds389 @tbordaz
Firstyear added a commit that referenced this pull request May 10, 2023
Bug Description: When a custom filter was provided, entries
which were deleted in AD did not have that event correctly
reflected in 389-ds. This was due to the behaviour that when
an entry in AD is deleted, it is marked with a "deleted" flag
which the objectClass=* filter would (accidentally) collect
when it did a search. However, a custom user filter being
specified would in some cases (such as a memberOf filter)
NOT show up the deletion since the entry was considered
to have moved out of scope rather than being a full delete.

Fix Description: In the case that we have a userfilter, we
wrap it in an OR condition that always requests isDeleted
flags so that we can correctly reflect the delete status.

fixes: #5052

Author: William Brown <william@blackhats.net.au>

Review by: @mreynolds389 @tbordaz
@Firstyear
Copy link
Contributor Author

To github.com:389ds/389-ds-base.git
   148ad351d..13e66bfa5  389-ds-base-2.3 -> 389-ds-base-2.3
To github.com:389ds/389-ds-base.git
   ee0ba3cec..f6c639768  389-ds-base-2.2 -> 389-ds-base-2.2

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.

Winsync OneWay fromWindows not replicate deletion
3 participants