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] Consider the UserDetails attributes for validity #3866

Closed
wants to merge 4 commits into from
Closed

[JENKINS-55813] Consider the UserDetails attributes for validity #3866

wants to merge 4 commits into from

Conversation

Wadeck
Copy link
Contributor

@Wadeck Wadeck commented Jan 28, 2019 •
edited by batmat
Loading

See JENKINS-55813.
Required PR for #89 in active-directory-plugin and #34 in ldap-plugin.

Proposed changelog entries

  • JENKINS-55813: Consider the additional attributes from UserDetails to determine the validity of the account before using API tokens.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

@Wadeck Wadeck added the work-in-progress The PR is under active development, not ready to the final review label Jan 28, 2019
jvz
jvz approved these 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.

Looks good overall, just want to make sure you're intentionally introducing new public APIs.

core/src/main/java/hudson/security/UserDetailsHelper.java Show resolved Hide resolved
batmat
batmat approved these changes Apr 9, 2019
View reviewed changes
Copy link
Member

@batmat batmat left a comment

Choose a reason for hiding this comment

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

LGTM

@batmat
Copy link
Member

batmat commented Apr 9, 2019

Waiting for @Wadeck to confirm this is actually not work-in-progress anymore. If confimed then I think we'll be able to move to merging this.

@oleg-nenashev
Copy link
Member

@Wadeck gentle ping

@Wadeck
Copy link
Contributor Author

Wadeck commented May 9, 2019

@batmat @oleg-nenashev As discussed with Baptiste internally, it's still work-in-progress, internal reference: FNDJEN-599

The remaining task is to determine other usage of the UserDetails that should require specific treatment like the one already present.

jvz
jvz suggested changes May 10, 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.

There are some more potential places to consider the validity of UserDetails before proceeding:

  • User.UserIDCanonicalIdResolver.resolveCanonicalId may or may not want to return here.
  • AbstractPasswordBasedSecurityRealm.doAuthenticate() may wish to implement this same check.
  • FederatedLoginService.FederatedIdentity.signin() should probably verify this as well.

@oleg-nenashev
Copy link
Member

@jvz made an assumption that the changes in LDAP and AD plugins are the root cause of instabilities in the recent weekly community ratings. e.g. https://issues.jenkins-ci.org/browse/JENKINS-57607 , https://issues.jenkins-ci.org/browse/JENKINS-57569

@Wadeck
Copy link
Contributor Author

Wadeck commented May 24, 2019

@oleg-nenashev AFAICT not possible for JENKINS-57569 because the related PR was not merged yet for this one. The other is more "interesting".

@alecharp
Copy link
Member

alecharp commented Aug 1, 2019

@Wadeck would you have time to come back on this one?

@Wadeck
Copy link
Contributor Author

Wadeck commented Aug 1, 2019

@alecharp not those days unfortunately, perhaps in 1-2 weeks or later... :(

@Wadeck Wadeck added on-hold This pull request depends on another event/release, and it cannot be merged right now and removed work-in-progress The PR is under active development, not ready to the final review labels Sep 3, 2019
@daniel-beck daniel-beck added the plugin-api-changes Changes the API of Jenkins available for use in plugins. label Oct 11, 2019
@Wadeck
Copy link
Contributor Author

Wadeck commented Dec 20, 2019

Note: for regular login, Acegi is using DefaultPreAuthenticationChecks to ensure the same checks are done.

@varyvol
Copy link

varyvol commented Feb 6, 2020

@Wadeck taking into account it's been on hold for several months, what do you think about proposing it for close? It can always be reopened if you plan to work on it again in the future.

@timja timja added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Feb 20, 2020
@timja
Copy link
Member

timja commented Feb 20, 2020

@Wadeck see @varyvol's ping above

@varyvol
Copy link

varyvol commented Mar 19, 2020

@Wadeck I'm closing this given you don't seem to have the capacity to work on this right now. Please feel free to reopen when you have the bandwith.

@varyvol varyvol closed this Mar 19, 2020
jvz
jvz reviewed Sep 1, 2020
View reviewed changes
core/src/main/java/hudson/security/UserDetailsHelper.java
*
* @since TODO
*/
public static boolean isFullyActive(@Nonnull UserDetails ud){
Copy link
Member

Choose a reason for hiding this comment

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

It seems like the appropriate API to use here would be UserDetailsChecker rather than reinventing that API here.

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

@jsoref jsoref jsoref left review comments

@jvz jvz jvz requested changes

@daniel-beck daniel-beck daniel-beck approved these changes

@batmat batmat batmat approved these changes

Assignees
No one assigned
Labels
on-hold This pull request depends on another event/release, and it cannot be merged right now plugin-api-changes Changes the API of Jenkins available for use in plugins. proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Wadeck @batmat @oleg-nenashev @alecharp @varyvol @timja @jvz @daniel-beck @jsoref