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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 2 commits into from

Conversation

Wadeck
Copy link

@Wadeck Wadeck commented Jan 28, 2019

@Wadeck Wadeck mentioned this pull request Jan 28, 2019
Closed
3 tasks
jvz
jvz suggested changes Jan 28, 2019
View reviewed changes
Copy link
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.

src/main/java/hudson/security/UserAttributesHelper.java Outdated Show resolved Hide resolved
src/main/java/hudson/security/UserAttributesHelper.java Outdated Show resolved Hide resolved
src/main/java/hudson/security/UserAttributesHelper.java Show resolved Hide resolved
src/main/java/hudson/security/UserAttributesHelper.java Outdated Show resolved Hide resolved
@jvz jvz mentioned this pull request Jan 28, 2019
Merged
jvz
jvz reviewed May 7, 2019
View reviewed changes
src/main/java/hudson/security/UserAttributesHelper.java
// 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
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
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
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
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
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
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
rsandell
rsandell reviewed Mar 26, 2020
View reviewed changes
Copy link
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
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
Reviewers

@jvz jvz jvz approved these changes

@rsandell rsandell rsandell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Wadeck @jvz @stephenc @rsandell