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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ldap/servers/plugins/replication/repl5_agmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,8 @@ agmt_new_from_entry(Slapi_Entry *e)
ra->agreement_type = REPLICA_TYPE_WINDOWS;
windows_init_agreement_from_entry(ra, e);
} else {
slapi_log_err(SLAPI_LOG_REPL, repl_plugin_name,
"agmt_new_from_entry: type -> %d\n", replica_get_type(replica));
slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name,
"agmt_new_from_entry: failed to initialise windows replication"
"agreement \"%s\" - replica is not a supplier (may be hub or consumer).\n",
Expand Down
16 changes: 14 additions & 2 deletions ldap/servers/plugins/replication/windows_connection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,14 +776,26 @@ send_dirsync_search(Repl_Connection *conn)
slapi_log_err(SLAPI_LOG_REPL, windows_repl_plugin_name, "send_dirsync_search - Calling dirsync search request plugin\n");
userfilter = windows_private_get_windows_userfilter(conn->agmt);
if (userfilter) {
filter = slapi_ch_strdup(userfilter);
/*
* When we have a userfilter, we encounter an issue where a previously
* matching object that is *deleted* that we had synced, was not being
* deleted. This is because in the unfiltered case, we relied on the
* objectClass=* to get everything, but when we apply a filter we are
* removing items that were deleted, especially if they were members of
* a group. As a result, we need to *always* request the isDeleted flag
* so that we can correct delete any remnants on our side.
*/
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

} else {
filter = slapi_ch_strdup("(objectclass=*)");
}

winsync_plugin_call_dirsync_search_params_cb(conn->agmt, old_dn, &dn, &scope, &filter,
&attrs, &server_controls);
slapi_log_err(SLAPI_LOG_REPL, windows_repl_plugin_name, "send_dirsync_search - Sending dirsync search request\n");
slapi_log_err(SLAPI_LOG_REPL, windows_repl_plugin_name, "send_dirsync_search - Sending dirsync search request %s %d %s\n",
dn, scope, filter);

rc = ldap_search_ext(conn->ld, dn, scope, filter, attrs, PR_FALSE, server_controls,
NULL /* ClientControls */, 0, 0, &msgid);
Expand Down