Skip to content

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

Closed
Wadeck wants to merge 4 commits intojenkinsci:masterfrom
Wadeck:JENKINS-55813_USER_DETAILS_USAGE
Closed

[JENKINS-55813] Consider the UserDetails attributes for validity#3866
Wadeck wants to merge 4 commits intojenkinsci:masterfrom
Wadeck:JENKINS-55813_USER_DETAILS_USAGE

Conversation

@Wadeck
Copy link
Copy Markdown
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
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.

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

jsoref
jsoref reviewed Mar 10, 2019
View reviewed changes
daniel-beck
daniel-beck approved these changes Apr 8, 2019
View reviewed changes
@daniel-beck @Wadeck
Update core/src/main/java/hudson/security/BasicAuthenticationFilter.java
68cee29 
Co-Authored-By: Wadeck <Wadeck@users.noreply.github.com>
batmat
batmat approved these changes Apr 9, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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.

@daniel-beck @Wadeck
Update core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthent…
9369c22 
…icator.java

Co-Authored-By: Wadeck <Wadeck@users.noreply.github.com>
@oleg-nenashev
Copy link
Copy Markdown
Member

oleg-nenashev commented May 6, 2019

@Wadeck gentle ping

@Wadeck
Copy link
Copy Markdown
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
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.

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
Copy Markdown
Member

oleg-nenashev commented May 24, 2019

@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
Copy Markdown
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
Copy Markdown
Member

alecharp commented Aug 1, 2019

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

@Wadeck
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member

timja commented Feb 20, 2020

@Wadeck see @varyvol's ping above

@varyvol
Copy link
Copy Markdown

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
Copy Markdown
Member

@jvz jvz Sep 1, 2020

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

@batmat batmat batmat approved these changes

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

+2 more reviewers

@jvz jvz jvz requested changes

@jsoref jsoref jsoref left review comments

Reviewers whose approvals may not affect merge requirements

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