[JENKINS-31575] Adding SSO/Client certificate authentication against Active Directory #16
Conversation
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
This pull request also contained some fixs on some findjobs warnings/errors coming from the master branch of this projects. This was really annoying of having red errors and no checks correctly passed in ci Jenkins build. So I decided to remove them for having my pull request fully green |
llecaroz
changed the title
llecaroz
changed the title
|
Jira ticket created as recommended : JENKINS-31575 Adding SSO/Client certificate authentication against Active Director |
|
Interesting work. Thanks. Some quick remarks (I'm not the maintainer, just saw your mail on the list and gave it a quick shot). /me think you should rewrite commits so that they have an user.name. Would be better for future potential forensics. And potentially squash your multiple merge commits so that the PR is more logically separated. And it's generally common practice to try and separate PRs between features and (apparently) unrelated change (here Findbugs). This makes a bigger chance for your PR to be accepted, since it makes it easier to review. My 2 cents |
| this.realm.getSsoEnabled().getCheckMode()!=null && | ||
| this.realm.getSsoEnabled().getCheckMode().equalsIgnoreCase("no_check")==true && | ||
| this.realm.getSsoEnabled().getDefaultGroup()!=null && | ||
| this.realm.getSsoEnabled().getDefaultGroup().equals(groupname)==true) |
There was a problem hiding this comment.
a matter of preference maybe, but when conditions get complex, I generally extract it to a small dedicated boolean method. Something like isBlahMode() or hasSomethingSpecificBluhEnabled().
|
Hi batmat, I took care of most of your feedbacks & integrated in my last changes. I really appreciated your feedbacks & fully agree with most of them. |
bind dn username/password are mandatory when working with sso except in
no_check mode. In the case of no_check mode:
-A suffix will be added on username not to check against LDAP & mix them with LDAP real users authentications.
-Users logged in SSO will be part of the default group defined in the configuration.
For all other modes, user groups will be those downloaded from LDAP. Because depending on companies, SSO DN username can differ from LDAP username (lower/upper case issues), a strategy can be applied.
For more information, see new options in the Jenkins Global security configuration