[JENKINS-55813] Improve analysis of AD attributes #96
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This brings us up to date with the equivalent feature development in LDAP. This checks for some standard Active Directory user attributes for account validity checks along with adding an escape hatch to disable the behavior. Signed-off-by: Matt Sicker <boards@gmail.com>
Signed-off-by: Matt Sicker <boards@gmail.com>
|
I managed to create a real Active Directory setup on GCP, so I'm going to manually test the changes here with the different account settings I can toggle (locked, expired, etc.) |
Signed-off-by: Matt Sicker <boards@gmail.com>
An account expiration date of 1601-01-01 at midnight is documented as indicating that the account does not expire. This would be easier to notice if IADsUser.accountExpirationDate() returned a long instead of a Date because it would have been 0. Signed-off-by: Matt Sicker <boards@gmail.com>
Signed-off-by: Matt Sicker <boards@gmail.com>
|
I've tested this plugin in both ADSI mode and manually configured with Windows Server 2019 and Active Directory with the three featured attributes (expired, locked, disabled) all manually tested. It was during this testing that I discovered why the original PR had caused a regression: in ADSI mode, the com4j interfaces created for calling the ADSI native code were converting the timestamp into a |
Signed-off-by: Matt Sicker <boards@gmail.com>
Signed-off-by: Matt Sicker <boards@gmail.com>
jvz
commented
Nov 24, 2020
| @@ -382,12 +384,14 @@ public UserDetails call() throws AuthenticationException, NamingException { | |||
| // locate this user's record | |||
| final String domainDN = toDC(domain.getName()); | |||
|
|
|||
| Attributes user = new LDAPSearchBuilder(context, domainDN).subTreeScope().searchOne("(& (userPrincipalName={0})(objectCategory=user))", userPrincipalName); | |||
| Attributes user = new LDAPSearchBuilder(context, domainDN).subTreeScope().returns("*", "+") | |||
| .searchOne("(& (userPrincipalName={0})(objectCategory=user))", userPrincipalName); | |||
There was a problem hiding this comment.
I'm no AD admin, but theoretically, these two LDAP filters can be combined into one like so:
(&(objectCategory=user)(|(userPrincipalName={0})(sAMAccountName={0})))
No idea how that would affect or potentially destroy performance of LDAP indices that may be set up.
|
@jvz, any reason why this test failure can be ignored? |
|
Might need to re-run that. Let me try that. |
|
I can't reproduce the test failure locally. |
|
I can't reproduce the test failure locally either, but given the substantial implementation differences between Unix and Windows, we can't just ignore it. It might be part of general issues with Windows test machines or infra. |
|
@jvz @jeffret-b I can easily reproduce the test failure. To reproduce it, you need to have Docker Engine up and running and have in your DNS settings configured with something like this - adapted to your OS (take special attention to de 127.0.0.1 thing - IIRC this is the only important thing from the DNS to add)
Then, run the following maven command the |
|
Seems as though one problem is that if docker isn't available, the tests pass with an error message. |
|
Ok, when I make sure Docker is running, and after running with that profile, I get three test failures: I get the same exact failures running those tests on the master branch. Can you confirm that the tests are passing in master and on what OS? |
|
I think those three tests will be fixed if you add in your /etc/hosts an entre for 127.0.0.1 samdom.example.com
However, the test I am worried about is |
|
That test seems to pass on my computer. I'm not sure if it's a timeout issue or docker-specific or what as the integration test itself has a lot of supporting infrastructure. I'll retry the tests with that hosts file entry and get back to you. |
|
Adding the hosts entries still have the tests failing. The assertion doesn't provide any information, either, so I'm not even sure how to go about debugging this, especially when running this test takes nearly 10 minutes on my machine for some reason. |
|
Oh wait, I didn't try setting my network settings to match your DNS screenshot. Let me try that as well and see if it makes a difference. |
Adding more actions in custom.sh causes the logs to be longer than expected by the test. Signed-off-by: Matt Sicker <boards@gmail.com>
|
Alright, I finally figured it out after much debugging. Turns out the log capturing wasn't capturing enough of the log anymore now that the setup was doing more things. |
|
Alright, I give up on that failing test on CI. It works on my machine. Log as proof: Line from /etc/hosts:
I have no idea why CI is failing. |
|
I will try to get time to review this. I just launched all the test on my local environment and all the tests are passing in the master branch. However, there is one failure in your PR |
|
I isolated the cause of the test failure by first removing my changes to custom.sh which made the test pass again. Changing the ordering of the commands in it didn't make the tests pass, but then I increased the log capture count which made it pass. If the tests are still failing for you, then that's beyond my knowledge here as it's specific to that test and not this PR. I didn't want to copy the test and create a brand new one just to avoid a flaky test. |
|
@fbelzunc @jeffret-b I split out the new test and reverted my changes to the old test. Everything should be good to go now. Please review. |
|
Re-triggering due to docker build timeout. |
|
Not sure why the test aborts before it's finished building the image, so this may take a few tries to get it to finish building. |
Signed-off-by: Matt Sicker <boards@gmail.com>
|
@jeffret-b @fbelzunc tests passing now |
jeffret-b
approved these changes
Dec 15, 2020
|
So this PR is ready to merge. However, based on some bug reports we got related to the LDAP change matching this AD change, there seem to be a plugin or two that also needs updating to properly handle the exceptions thrown by this PR. I'll link back to those here, then you can safely merge this when those are merged. |
|
jenkinsci/mailer-plugin#104 for one issue surfaced from the LDAP PR that would likewise apply to this PR. |
|
And filed jenkinsci/email-ext-plugin#277 for the other related issue. Once those are both merged, this PR is good to go. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts the revert of #89 to continue development.
@reviewbybees @Wadeck @fbelzunc