-
Notifications
You must be signed in to change notification settings - Fork 6k
Modify Password Extended Operation Support #5922
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
Conversation
testCompile 'org.slf4j:jcl-over-slf4j' | ||
testCompile 'org.slf4j:slf4j-api' | ||
} | ||
|
||
integrationTest { | ||
include('**/ApacheDSServerIntegrationTests.class', '**/ApacheDSEmbeddedLdifTests.class') | ||
include('**/ApacheDSServerIntegrationTests.class', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually necessary since we have a separate source folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason, as far as I can tell, is because ApacheDSServerIntegrationTests
uses the @Suite
annotation to invoke other dependent integration tests (in the same integration-test directory). If the tests are run independent from this orchestrating test, then they fail because they are dependent on the server that ApacheDSServerIntegrationTests
starts up.
The password-modify tests are not written in this way, but need to be included in the list since we are already using the include
directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really a fan of it either. If you think it valuable, I'd recommend a ticket to rewrite the ldap tests to not depend on a parent integration test. Then we could remove this include
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yes thanks for the reminder about that. Yes let's do that as a separate ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - #5942
* By default, {@code usePasswordModifyExtensionOperation} is false. | ||
* | ||
* @param usePasswordModifyExtensionOperation | ||
* @since 5.1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should backport this to 5.0.x and 4.2.x too so we need to change this to 4.2.9
@jzheaux This is ready to merge. Can you please merge and backport it to 4.2.x and 5.0.x? |
d213061
to
f61c856
Compare
Basic server startup test now asserts a successful server startup instead of a failed one. Issue: spring-projectsgh-5920
Setting a port of 0 will now induce the container to select a random port. Fixes: spring-projectsgh-5920
LdapUserDetailsManager can be configured to either use direct attribute modification or the LDAP Password Modify Extended Operation to change a user's password. Fixes: spring-projectsgh-3392
Issue: gh-3392