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

feat: add support for ldap compare request #2780

Merged
merged 1 commit into from
May 24, 2024

Conversation

tobikris
Copy link
Contributor

@tobikris tobikris commented May 19, 2024

This adds support for the LDAP CompareRequest. The implementation is using based on the existing Search operation.

I tried to follow this description of the request: https://ldap.com/the-ldap-compare-operation/.
As the Search operation is not exposing the reason for errors, I could not return invalidDNSyntax.
I decided on using the same authentication decision as for the Search.

Edit:
After the review, I changed the implementation to be standalone with copy-pasted code from do_search instead of calling the method.

Checklist

  • This pr contains no AI generated code
  • cargo fmt has been run
  • cargo clippy has been run
  • cargo test has been run and passes
  • book chapter included (if relevant)
  • design document included (if relevant)

@yaleman yaleman requested a review from Firstyear May 20, 2024 04:46
@yaleman yaleman added enhancement New feature or request ldap labels May 20, 2024
Copy link
Member

@Firstyear Firstyear left a comment

Choose a reason for hiding this comment

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

Thanks for this. There are quite a few comments but I hope we can help out get this to a state we can merge it.

server/lib/src/idm/ldap.rs Show resolved Hide resolved
server/lib/src/idm/ldap.rs Outdated Show resolved Hide resolved
server/lib/src/idm/ldap.rs Outdated Show resolved Hide resolved
server/lib/src/idm/ldap.rs Outdated Show resolved Hide resolved
@tobikris tobikris force-pushed the implement-compare branch 2 times, most recently from 82e06f5 to fefe698 Compare May 22, 2024 22:42
Firstyear
Firstyear previously approved these changes May 23, 2024
Copy link
Member

@Firstyear Firstyear left a comment

Choose a reason for hiding this comment

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

I think you got this spot on, great work :)

@tobikris tobikris force-pushed the implement-compare branch 2 times, most recently from 2782ade to 1ccb15a Compare May 23, 2024 16:35
@Firstyear Firstyear enabled auto-merge (squash) May 24, 2024 03:30
@Firstyear
Copy link
Member

This may need a rebase to latest master before we can merge but otherwise it looks great, thank you!

auto-merge was automatically disabled May 24, 2024 08:22

Head branch was pushed to by a user without write access

@tobikris
Copy link
Contributor Author

This may need a rebase to latest master before we can merge but otherwise it looks great, thank you!

I rebased, but this reset the auto-merge status.

@yaleman yaleman merged commit 814380a into kanidm:master May 24, 2024
22 of 24 checks passed
@tobikris tobikris deleted the implement-compare branch May 24, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ldap
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants