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

Jump to bottom

Issue 5170 - BUG - ldapsubentries were incorrectly returned #5285

Merged
merged 1 commit into from
May 14, 2022
Merged

Issue 5170 - BUG - ldapsubentries were incorrectly returned #5285

merged 1 commit into from
May 14, 2022

Conversation

Firstyear
Copy link
Contributor

Bug Description: Due to a change in the logic with filter
optimiser, in ldbm search we accidentally returned ldap
subentrys

Fix Description: Clean up the logic and comments in the
ldbm_search.c file to prevent this.

fixes: #5170

Author: William Brown william@blackhats.net.au

Review by: ???

@Firstyear
Copy link
Contributor Author

I have confirmed it passes tests/suites/filter/complex_filters_test.py

@mreynolds389
Copy link
Contributor

I have confirmed it passes tests/suites/filter/complex_filters_test.py

The "subentries" test is now failing...

@progier389 progier389 mentioned this pull request May 9, 2022
Closed
@Firstyear
Copy link
Contributor Author

I have confirmed it passes tests/suites/filter/complex_filters_test.py

The "subentries" test is now failing...

I can't reproduce, they all pass for me .... including subentries.

@progier389
Copy link
Contributor

Maybe you should try to rerun this PR tests and check if subentries test still fail...

@mreynolds389
Copy link
Contributor

I have confirmed it passes tests/suites/filter/complex_filters_test.py

The "subentries" test is now failing...

I can't reproduce, they all pass for me .... including subentries.

The subentries test fails for me with your patch - just like it does in the Gtihub CI tests.

subentries_test.py F.... 
...
...
=================================================================================================== short test summary info ===================================================================================================
FAILED subentries_test.py::test_subentries[(objectclass=inetorgperson)-True-True-5-True] - assert 0 == 5
========================================================================================== 1 failed, 4 passed, 5 warnings in 12.93s =============================

@Firstyear
Copy link
Contributor Author

Ahh you meant the subentries suite, not the subentries test in complex.

Okay, that I can reproduce, I'll look next week.

@Firstyear
Issue 5170 - BUG - ldapsubentries were incorrectly returned
64d376b 
Bug Description: Due to a change in the logic with filter
optimiser, in ldbm search we accidentally returned ldap
subentrys

Fix Description: Clean up the logic and comments in the
ldbm_search.c file to prevent this.

fixes: 389ds#5170

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

Review by: ???
@Firstyear Firstyear force-pushed the 5170-resolve-ldapsubentry-check-issue branch from 5f192cb to 64d376b Compare May 13, 2022 05:53
@Firstyear
Copy link
Contributor Author

Fixed.

mreynolds389
mreynolds389 approved these changes May 13, 2022
View reviewed changes
Copy link
Contributor

@mreynolds389 mreynolds389 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 Firstyear merged commit 682bbfb into 389ds:master May 14, 2022
@Firstyear
Copy link
Contributor Author

@mreynolds389 Do we need to double check the other branches that we applied 5170 to?

@mreynolds389
Copy link
Contributor

@mreynolds389 Do we need to double check the other branches that we applied 5170 to?

Yeah this needs to be cherry-picked to 2.1 and 2.0

@Firstyear
Copy link
Contributor Author

@mreynolds389 I'm sick man, can you do this? So sorry.

@mreynolds389
Copy link
Contributor

mreynolds389 commented May 16, 2022 •
edited
Loading

@mreynolds389 I'm sick man, can you do this? So sorry.

No problem. FYI there are still regressions with all of this. Working on a testcase at this point, but might need you to look into at later. See:

#5289

mreynolds389 pushed a commit that referenced this pull request May 16, 2022
@Firstyear @mreynolds389
Issue 5170 - BUG - ldapsubentries were incorrectly returned (#5285)
a2f8348 
Bug Description: Due to a change in the logic with filter
optimiser, in ldbm search we accidentally returned ldap
subentrys

Fix Description: Clean up the logic and comments in the
ldbm_search.c file to prevent this.

fixes: #5170

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

Review by: @mreynolds389
@Firstyear
Copy link
Contributor Author

I'll try and look when I'm doing better :(

mreynolds389 pushed a commit that referenced this pull request May 17, 2022
@Firstyear @mreynolds389
Issue 5170 - BUG - ldapsubentries were incorrectly returned (#5285)
26a9945 
Bug Description: Due to a change in the logic with filter
optimiser, in ldbm search we accidentally returned ldap
subentrys

Fix Description: Clean up the logic and comments in the
ldbm_search.c file to prevent this.

fixes: #5170

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

Review by: @mreynolds389
Firstyear added a commit to Firstyear/389-ds-base that referenced this pull request May 30, 2022
@Firstyear
Revert "Issue 5170 - BUG - ldapsubentries were incorrectly returned (3…
db6ee91 
…89ds#5285)"

This reverts commit 682bbfb.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mreynolds389 mreynolds389 mreynolds389 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filter optimiser
3 participants
@Firstyear @mreynolds389 @progier389