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

Add Elytron ldap cache #39665

Merged
merged 1 commit into from
Mar 29, 2024
Merged

Add Elytron ldap cache #39665

merged 1 commit into from
Mar 29, 2024

Conversation

jonasjensen-brolstark
Copy link
Contributor

@jonasjensen-brolstark jonasjensen-brolstark commented Mar 25, 2024 •
edited by geoand
Loading

Add configuration for Elytron ldap cache.

sberyozkin
sberyozkin approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

LGTM, @michalvavrik, do you agree ?

@sberyozkin sberyozkin requested a review from michalvavrik March 25, 2024 11:09
@jonasjensen-brolstark
Copy link
Contributor Author

@michalvavrik i have added your suggestions and added a IT test, where the LDAP server i shutdown between requests.

I am bit in doubt about the warning text. So if you have any comments/suggestions it would be nice 🙂

I will squash my commits, if you approve.

@michalvavrik michalvavrik requested a review from sberyozkin March 26, 2024 11:35
@sberyozkin
Copy link
Member

Thanks @jonasjensen-brolstark, @michalvavrik, Michal, if you are happy, please add your approval too

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
michalvavrik
michalvavrik reviewed Mar 26, 2024
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

Sorry @sberyozkin about the review, I remembered to request your review, but forgot to submit my comments that I had here.

test-framework/ldap/src/main/java/io/quarkus/test/ldap/LdapServerTestResource.java Show resolved Hide resolved
...ytron-security-ldap/runtime/src/main/java/io/quarkus/elytron/security/ldap/LdapRecorder.java Outdated Show resolved Hide resolved
@michalvavrik
Copy link
Member

Anyway, I think current code is good, thanks @jonasjensen-brolstark .

michalvavrik
michalvavrik approved these changes Mar 27, 2024
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am fine with changes, but I think the place where you put the test with shutting down server is suspicious. I'd make it declaratively last as the default method order depends on complex algorithm and I'm not able to say if it is last. anyway, good job, thanks

...security-ldap/src/test/java/io/quarkus/elytron/security/ldap/it/ElytronSecurityLdapTest.java Show resolved Hide resolved
@sberyozkin sberyozkin added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 27, 2024
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@sberyozkin
Copy link
Member

@jonasjensen-brolstark FYI:

Caused by: org.apache.maven.plugin.MojoFailureException: File '/home/runner/work/quarkus/quarkus/extensions/elytron-security-ldap/runtime/src/main/java/io/quarkus/elytron/security/ldap/LdapRecorder.java' has not been previously formatted. Please format file (for example by invoking `mvn -f extensions/elytron-security-ldap/runtime net.revelc.code.formatter:formatter-maven-plugin:2.23.0:format`) and commit before running validation!

@geoand
Copy link
Contributor

geoand commented Mar 29, 2024

I updated and force pushed the change

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Mar 29, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 4dc83f9.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand geoand merged commit 4f28004 into quarkusio:main Mar 29, 2024
19 checks passed
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Mar 29, 2024
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Mar 29, 2024
@sberyozkin
Copy link
Member

Thanks @geoand

@geoand
Copy link
Contributor

geoand commented Mar 29, 2024

👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sberyozkin sberyozkin sberyozkin approved these changes

@michalvavrik michalvavrik michalvavrik approved these changes

Assignees
No one assigned
Labels
area/security area/testing kind/enhancement New feature or request
Projects
None yet
Milestone
3.10.0.CR1
Development

Successfully merging this pull request may close these issues.

LDAP Security - Don't re-authenticate on every request
4 participants
@jonasjensen-brolstark @sberyozkin @michalvavrik @geoand