From aa847bec9fd7ec7c51858544d0c81514780d6085 Mon Sep 17 00:00:00 2001 From: James Nord Date: Wed, 14 Aug 2024 17:06:29 +0100 Subject: [PATCH 1/4] ignore checkstyle in eclipse --- pom.xml | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/pom.xml b/pom.xml index 675735d7..f99414c2 100644 --- a/pom.xml +++ b/pom.xml @@ -129,6 +129,36 @@ + + + + + org.eclipse.m2e + lifecycle-mapping + 1.0.0 + + + + + + + org.apache.maven.plugins + maven-checkstyle-plugin + [3.4.0,) + + check + + + + + + + + + + + + maven-release-plugin From c851fc2b150f8b6e1b3ad04dfe9de5e451799f2d Mon Sep 17 00:00:00 2001 From: James Nord Date: Wed, 14 Aug 2024 17:07:37 +0100 Subject: [PATCH 2/4] Support Microsoft ADFS as a provider AD FS returns data via a POST not a get, so this adapts the code to handle a call to finishLogin with via an HTTP POST. --- pom.xml | 10 ++ .../plugins/oic/OicCrumbExclusion.java | 34 ++++++ .../org/jenkinsci/plugins/oic/OicSession.java | 10 ++ .../plugins/oic/OicCrumbExclusionTest.java | 101 ++++++++++++++++++ 4 files changed, 155 insertions(+) create mode 100644 src/main/java/org/jenkinsci/plugins/oic/OicCrumbExclusion.java create mode 100644 src/test/java/org/jenkinsci/plugins/oic/OicCrumbExclusionTest.java diff --git a/pom.xml b/pom.xml index f99414c2..6b620b0c 100644 --- a/pom.xml +++ b/pom.xml @@ -112,6 +112,16 @@ 4.5.14 test + + org.mockito + mockito-core + test + + + org.mockito + mockito-junit-jupiter + test + diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicCrumbExclusion.java b/src/main/java/org/jenkinsci/plugins/oic/OicCrumbExclusion.java new file mode 100644 index 00000000..7d33bbe8 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/oic/OicCrumbExclusion.java @@ -0,0 +1,34 @@ +package org.jenkinsci.plugins.oic; + +import hudson.Extension; +import hudson.security.SecurityRealm; +import hudson.security.csrf.CrumbExclusion; +import java.io.IOException; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import jenkins.model.Jenkins; + +/** + * Crumb exclusion to allow POSTing to {@link OicSecurityRealm#doFinishLogin(org.kohsuke.stapler.StaplerRequest)} + */ +@Extension +public class OicCrumbExclusion extends CrumbExclusion { + + @Override + public boolean process(HttpServletRequest request, HttpServletResponse response, FilterChain chain) + throws IOException, ServletException { + Jenkins j = Jenkins.getInstanceOrNull(); + if (j != null) { + SecurityRealm sr = j.getSecurityRealm(); + if (sr instanceof OicSecurityRealm) { + if ("/securityRealm/finishLogin".equals(request.getPathInfo())) { + chain.doFilter(request, response); + return true; + } + } + } + return false; + } +} diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSession.java b/src/main/java/org/jenkinsci/plugins/oic/OicSession.java index d4c78f66..263d1b0d 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSession.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSession.java @@ -38,12 +38,14 @@ import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; +import java.util.Map; import java.util.UUID; import javax.servlet.http.HttpSession; import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.Stapler; import org.kohsuke.stapler.StaplerRequest; +import org.springframework.web.util.UriComponentsBuilder; import static org.jenkinsci.plugins.oic.OicSecurityRealm.ensureStateAttribute; @@ -192,6 +194,14 @@ public HttpResponse finishLogin(StaplerRequest request, AuthorizationCodeFlow fl StringBuffer buf = request.getRequestURL(); if (request.getQueryString() != null) { buf.append('?').append(request.getQueryString()); + } else { + // some providers ADFS! post data using a form rather than the queryString. + Map parameterMap = request.getParameterMap(); + UriComponentsBuilder queryBuilder = UriComponentsBuilder.fromHttpUrl(buf.toString()); + for (Map.Entry entry : parameterMap.entrySet()) { + queryBuilder.queryParam(entry.getKey(), (Object[]) entry.getValue()); + } + buf = new StringBuffer(queryBuilder.build().toUriString()); } AuthorizationCodeResponseUrl responseUrl = new AuthorizationCodeResponseUrl(buf.toString()); if (!state.equals(responseUrl.getState())) { diff --git a/src/test/java/org/jenkinsci/plugins/oic/OicCrumbExclusionTest.java b/src/test/java/org/jenkinsci/plugins/oic/OicCrumbExclusionTest.java new file mode 100644 index 00000000..a6f0ad72 --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/oic/OicCrumbExclusionTest.java @@ -0,0 +1,101 @@ +package org.jenkinsci.plugins.oic; + +import javax.servlet.FilterChain; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import jenkins.model.Jenkins; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class OicCrumbExclusionTest { + + @Mock + Jenkins jenkins; + + @Mock + MockedStatic staticJenkins; + + @Mock + OicSecurityRealm oicSecurityRealm; + + @Mock + FilterChain chain; + + @Mock + HttpServletRequest request; + + @Mock + HttpServletResponse response; + + private void withJenkins() { + staticJenkins.when(Jenkins::getInstanceOrNull).thenReturn(jenkins); + } + + private void withRequestPath(String path) { + lenient().when(request.getPathInfo()).thenReturn(path); + } + + private void withOicSecurityRealm() { + when(jenkins.getSecurityRealm()).thenReturn(oicSecurityRealm); + } + + @Test + void exclusion_applies_when_realm_is_OIC_and_path_is_finishLogin() throws Exception { + withJenkins(); + withRequestPath("/securityRealm/finishLogin"); + withOicSecurityRealm(); + + OicCrumbExclusion oicCrumbExclusion = new OicCrumbExclusion(); + assertTrue("path should be excluded", oicCrumbExclusion.process(request, response, chain)); + Mockito.verify(chain, times(1)).doFilter(request, response); + } + + @Test + void exclusion_does_not_apply_when_realm_is_OIC_and_path_is_not_finishLogin() throws Exception { + withJenkins(); + withRequestPath("/securityRealm/anything"); + withOicSecurityRealm(); + + OicCrumbExclusion oicCrumbExclusion = new OicCrumbExclusion(); + assertFalse("path should not be excluded", oicCrumbExclusion.process(request, response, chain)); + Mockito.verify(chain, times(0)).doFilter(request, response); + } + + @Test + void exclusion_does_not_apply_when_realm_is_not_OIC_and_path_is_finishLogin() throws Exception { + withJenkins(); + withRequestPath("/securityRealm/finishLogin"); + + OicCrumbExclusion oicCrumbExclusion = new OicCrumbExclusion(); + assertFalse("path should not be excluded", oicCrumbExclusion.process(request, response, chain)); + Mockito.verify(chain, times(0)).doFilter(request, response); + } + + @Test + void exclusion_does_not_apply_when_realm_is_not_OIC_and_path_is_not_finishLogin() throws Exception { + withJenkins(); + withRequestPath("/securityRealm/anything"); + + OicCrumbExclusion oicCrumbExclusion = new OicCrumbExclusion(); + assertFalse("path should not be excluded", oicCrumbExclusion.process(request, response, chain)); + Mockito.verify(chain, times(0)).doFilter(request, response); + } + + @Test + void exclusion_does_not_apply_when_jenkins_is_not_set() throws Exception { + OicCrumbExclusion oicCrumbExclusion = new OicCrumbExclusion(); + assertFalse("path should not be excluded", oicCrumbExclusion.process(request, response, chain)); + Mockito.verify(chain, times(0)).doFilter(request, response); + } +} From b2f787f576187763da11c412dfc7b7a77fbd0f8c Mon Sep 17 00:00:00 2001 From: James Nord Date: Wed, 14 Aug 2024 17:24:25 +0100 Subject: [PATCH 3/4] rudementary ADFS Docs --- docs/configuration/ADFS.md | 26 ++++++++++++++++++++++++++ docs/configuration/README.md | 1 + 2 files changed, 27 insertions(+) create mode 100644 docs/configuration/ADFS.md diff --git a/docs/configuration/ADFS.md b/docs/configuration/ADFS.md new file mode 100644 index 00000000..25e0fdb9 --- /dev/null +++ b/docs/configuration/ADFS.md @@ -0,0 +1,26 @@ +# ADFS Provider + +[ADFS][1] can be used as as an OpenID Connect identity provider. + +## Provider configuration + +[This][2] stack overflow step though is a great resource, followed by [This IBM resource][3] for granting the correct permissions. + +Where the IBM resource adds 2 individual permissions, 3 are needed and can be performed in one command - e.g. +`Set-AdfsApplicationPermission -TargetIdentifier fe56f061-c689-45e8-af8d-b8fdf5d1e60f -AddScope 'openid','aza','allatclaims'` + +Extra claims (for example users display name) can be added using a similar approach to the groups. + +## Plugin configuration + +ADFS provides a well known configuration endpoint which can be used for automating endpoint configuration. +It also supports PKCE verification for additional security. + +### User information + +Without any extra claims, the user field should be set to `upn` + + +[1]: https://learn.microsoft.com/en-us/windows-server/identity/ad-fs/ad-fs-overview +[2]: https://stackoverflow.com/questions/55494354/user-groups-as-claims-through-openid-connect-over-adfs/55570286#55570286 +[3]: https://community.ibm.com/community/user/security/blogs/laurent-lapiquionne1/2020/07/21/how-to-configure-igi-service-center-to-authent diff --git a/docs/configuration/README.md b/docs/configuration/README.md index 93da9510..3ccd0121 100644 --- a/docs/configuration/README.md +++ b/docs/configuration/README.md @@ -9,6 +9,7 @@ There are specifics instructions for well known providers: * [Google Provider](GOOGLE.md) * [Gitlab Provider](GITLAB.md) +* [Microsoft AD FS](ADFS.md) This page contains the reference of plugin's configuration. From a08b47ea7314bd587fe62319458f335e50facfc0 Mon Sep 17 00:00:00 2001 From: James Nord Date: Thu, 15 Aug 2024 13:24:54 +0100 Subject: [PATCH 4/4] Move extracting form parameters to a helper function and introduce unit test --- .../org/jenkinsci/plugins/oic/OicSession.java | 26 +++++++++++----- .../jenkinsci/plugins/oic/OicSessionTest.java | 30 ++++++++++++++++--- 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSession.java b/src/main/java/org/jenkinsci/plugins/oic/OicSession.java index 263d1b0d..66d71649 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSession.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSession.java @@ -41,6 +41,8 @@ import java.util.Map; import java.util.UUID; import javax.servlet.http.HttpSession; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.DoNotUse; import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.Stapler; @@ -191,19 +193,16 @@ public HttpResponse commenceLogin(AuthorizationCodeFlow flow) { * @return an {@link HttpResponse} */ public HttpResponse finishLogin(StaplerRequest request, AuthorizationCodeFlow flow) throws IOException { - StringBuffer buf = request.getRequestURL(); + final String requestURL; if (request.getQueryString() != null) { + StringBuffer buf = request.getRequestURL(); buf.append('?').append(request.getQueryString()); + requestURL = buf.toString(); } else { // some providers ADFS! post data using a form rather than the queryString. - Map parameterMap = request.getParameterMap(); - UriComponentsBuilder queryBuilder = UriComponentsBuilder.fromHttpUrl(buf.toString()); - for (Map.Entry entry : parameterMap.entrySet()) { - queryBuilder.queryParam(entry.getKey(), (Object[]) entry.getValue()); - } - buf = new StringBuffer(queryBuilder.build().toUriString()); + requestURL = convertFormToQueryParameters(request); } - AuthorizationCodeResponseUrl responseUrl = new AuthorizationCodeResponseUrl(buf.toString()); + AuthorizationCodeResponseUrl responseUrl = new AuthorizationCodeResponseUrl(requestURL); if (!state.equals(responseUrl.getState())) { return new Failure("State is invalid"); } @@ -226,6 +225,17 @@ public HttpResponse finishLogin(StaplerRequest request, AuthorizationCodeFlow fl return onSuccess(code, flow); } + @VisibleForTesting + @Restricted(DoNotUse.class) + protected static String convertFormToQueryParameters(StaplerRequest request) { + Map parameterMap = request.getParameterMap(); + UriComponentsBuilder queryBuilder = + UriComponentsBuilder.fromHttpUrl(request.getRequestURL().toString()); + for (Map.Entry entry : parameterMap.entrySet()) { + queryBuilder.queryParam(entry.getKey(), (Object[]) entry.getValue()); + } + return queryBuilder.build().toUriString(); + } /** * Where was the user trying to navigate to when they had to login? * diff --git a/src/test/java/org/jenkinsci/plugins/oic/OicSessionTest.java b/src/test/java/org/jenkinsci/plugins/oic/OicSessionTest.java index aceff857..21c91ca8 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/OicSessionTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/OicSessionTest.java @@ -2,15 +2,20 @@ import com.google.api.client.auth.oauth2.AuthorizationCodeFlow; import java.io.IOException; +import java.util.SortedMap; +import java.util.TreeMap; import jenkins.model.Jenkins; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.WithoutJenkins; import org.kohsuke.stapler.HttpResponse; +import org.kohsuke.stapler.StaplerRequest; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class OicSessionTest { @@ -21,7 +26,6 @@ public class OicSessionTest { private static final String from = "fromAddy"; - @Before public void init() throws IOException { TestRealm realm = new TestRealm.Builder("http://localhost/") .WithMinimalDefaults().WithScopes("openid").build(); @@ -44,12 +48,30 @@ private String buildOAuthRedirectUrl() throws NullPointerException { } @Test - public void getFrom() { + public void getFrom() throws Exception { + init(); assertEquals(from, session.getFrom()); } @Test - public void getState() { + public void getState() throws Exception { + init(); assertNotEquals("", session.getState()); } + + @Test + @WithoutJenkins + public void testFormToQueryParameters() { + StaplerRequest sr = mock(StaplerRequest.class); + when(sr.getRequestURL()) + .thenReturn(new StringBuffer("http://domain.invalid/jenkins/securityRealm/finishLogin")); + SortedMap parametersMap = new TreeMap<>(); + parametersMap.put("param1", new String[] {"p1k1"}); + parametersMap.put("param2", new String[] {"p2k1", "p2k2"}); + when(sr.getParameterMap()).thenReturn(parametersMap); + String converted = OicSession.convertFormToQueryParameters(sr); + assertEquals( + "http://domain.invalid/jenkins/securityRealm/finishLogin?param1=p1k1¶m2=p2k1¶m2=p2k2", + converted); + } }