Skip to content

[JENKINS-55813] Improve analysis of the AD attributes#34

Open
Wadeck wants to merge 2 commits intojenkinsci:masterfrom
Wadeck:JENKINS-55813_IMPROVE_ATTRIBUTES_ANALYSIS
Open

[JENKINS-55813] Improve analysis of the AD attributes#34
Wadeck wants to merge 2 commits intojenkinsci:masterfrom
Wadeck:JENKINS-55813_IMPROVE_ATTRIBUTES_ANALYSIS

Conversation

@Wadeck
Copy link
Copy Markdown

@Wadeck Wadeck commented Jan 28, 2019

Copy link
Copy Markdown
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

Small changes to consider, but otherwise looks good.

// documentation: https://docs.microsoft.com/en-us/windows/desktop/adschema/a-accountexpires
// code inspired by https://community.oracle.com/thread/1157460
private static long getWin32EpochHundredNanos() {
GregorianCalendar win32Epoch = new GregorianCalendar(1601, Calendar.JANUARY, 1);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ZonedDateTime.of(1601, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC) could be easier to calculate with here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

  1. I followed the code as mentioned in the javadoc
  2. I do not find the proposed one to be more readable than the one in the PR

If anybody else agree on your proposal I will change ;)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My proposal was taking a random java.time class that sounded appropriate. Calendar is much more annoying to use.

@jvz
Copy link
Copy Markdown
Member

jvz commented May 7, 2019

Can we get another review from one of the previous maintainers? @olamy @jglick @stephenc @batmat @rsandell @andresrc @dwnusbaum

@stephenc
Copy link
Copy Markdown
Member

stephenc commented May 7, 2019

🤷‍♀️ This is beyond my ken!

@batmat batmat self-requested a review May 8, 2019 13:27
@jvz
Copy link
Copy Markdown
Member

jvz commented May 10, 2019

Do you think that LDAPSecurityRealm.LDAPAuthenticationManager.authenticate() should be updated to check UserDetails validity as well? Or should that be the responsibility of AbstractPasswordBasedSecurityRealm?

@batmat batmat removed their request for review June 12, 2019 07:12
Copy link
Copy Markdown
Member

@rsandell rsandell left a comment

Choose a reason for hiding this comment

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

LGTM but needs tests

@jvz
Copy link
Copy Markdown
Member

jvz commented Sep 1, 2020

Taking a fresh look at this, I believe the configured UserDetailsChecker bean should be used in the authenticate() method based on the exception signatures provided. The UserDetailsService bean should only load the values for those attributes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants