From 6d990c4494e8423d5f9de493b61bbb8cda2848b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 20:59:35 +0100 Subject: [PATCH 01/15] Fix Javadoc --- .../jenkinsci/plugins/oic/OicSecurityRealm.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 45ece3e9..eeb85e53 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -172,23 +172,23 @@ ClientAuthenticationMethod toClientAuthenticationMethod() { @Deprecated private transient String wellKnownOpenIDConfigurationUrl; - /** @deprecated see {@link OicServerConfiguration#getTokenServerUrl()} */ + /** @deprecated see {@link OicServerManualConfiguration#getTokenServerUrl()} */ @Deprecated private transient String tokenServerUrl; - /** @deprecated see {@link OicServerConfiguration#getJwksServerUrl()} */ + /** @deprecated see {@link OicServerManualConfiguration#getJwksServerUrl()} */ @Deprecated private transient String jwksServerUrl; - /** @deprecated see {@link OicServerConfiguration#getTokenAuthMethod()} */ + /** @deprecated see {@link OicServerManualConfiguration#getTokenAuthMethod()} */ @Deprecated private transient TokenAuthMethod tokenAuthMethod; - /** @deprecated see {@link OicServerConfiguration#getAuthorizationServerUrl()} */ + /** @deprecated see {@link OicServerManualConfiguration#getAuthorizationServerUrl()} */ @Deprecated private transient String authorizationServerUrl; - /** @deprecated see {@link OicServerConfiguration#getUserInfoServerUrl()} */ + /** @deprecated see {@link OicServerManualConfiguration#getUserInfoServerUrl()} */ @Deprecated private transient String userInfoServerUrl; @@ -206,14 +206,14 @@ ClientAuthenticationMethod toClientAuthenticationMethod() { private transient String simpleGroupsFieldName = null; private transient String nestedGroupFieldName = null; - /** @deprecated see {@link OicServerConfiguration#getScopes()} */ + /** @deprecated see {@link OicServerManualConfiguration#getScopes()} */ @Deprecated private transient String scopes = null; private final boolean disableSslVerification; private boolean logoutFromOpenidProvider = true; - /** @deprecated see {@link OicServerConfiguration#getEndSessionUrl()} */ + /** @deprecated see {@link OicServerManualConfiguration#getEndSessionUrl()} */ @Deprecated private transient String endSessionEndpoint = null; From cf6307f1bdcd30a7552a08ff73031f589c034f64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 21:01:25 +0100 Subject: [PATCH 02/15] Fix typos --- docs/configuration/README.md | 2 +- .../jenkinsci/plugins/oic/OicSecurityRealm.java | 14 +++++++------- .../plugins/oic/OicServerManualConfiguration.java | 13 ++++++------- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/docs/configuration/README.md b/docs/configuration/README.md index 53c2a839..b39e56ea 100644 --- a/docs/configuration/README.md +++ b/docs/configuration/README.md @@ -21,7 +21,7 @@ which will also help discovering your settings From 1.5 and onward the well known configuration location may be used to populate the configuration simplifying the configuration greatly. -The switch between modes is controled by the `serverConfiguration` field +The switch between modes is controlled by the `serverConfiguration` field | field | format | description | |----------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------| diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index eeb85e53..2f9c5a94 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -282,7 +282,7 @@ ClientAuthenticationMethod toClientAuthenticationMethod() { SystemProperties.getBoolean(OicSecurityRealm.class.getName() + ".checkNonceInRefreshFlow", false); /** old field that had an '/' implicitly added at the end, - * transient because we no longer want to have this value stored + * transient because we no longer want to have this value stored, * but it's still needed for backwards compatibility */ @Deprecated private transient String endSessionUrl; @@ -908,7 +908,7 @@ public Authentication authenticate(Authentication authentication) throws Authent * Validate post-login redirect URL * * For security reasons, the login must not redirect outside Jenkins - * realm. For useablility reason, the logout page should redirect to + * realm. For usability reason, the logout page should redirect to * root url. */ protected String getValidRedirectUrl(String url) { @@ -933,7 +933,7 @@ protected String getValidRedirectUrl(String url) { } /** - * Handles the the securityRealm/commenceLogin resource and sends the user off to the IdP + * Handles the securityRealm/commenceLogin resource and sends the user off to the IdP * @param from the relative URL to the page that the user has just come from * @param referer the HTTP referer header (where to redirect the user back to after login has finished) * @throws URISyntaxException if the provided data is invalid @@ -1110,7 +1110,7 @@ private List ensureString(Object field) { LOGGER.warning("userInfo did not contain a valid group field content, got null"); return Collections.emptyList(); } else if (field instanceof String) { - // if its a String, the original value was not a json array. + // if it's a String, the original value was not a json array. // We try to convert the string to list based on comma while ignoring whitespaces and square brackets. // Example value "[demo-user-group, demo-test-group, demo-admin-group]" String sField = (String) field; @@ -1129,7 +1129,7 @@ private List ensureString(Object field) { if (group instanceof String) { result.add(group.toString()); } else if (group instanceof Map) { - // if its a Map, we use the nestedGroupFieldName to grab the groups + // if it's a Map, we use the nestedGroupFieldName to grab the groups Map groupMap = (Map) group; if (nestedGroupFieldName != null && groupMap.keySet().contains(nestedGroupFieldName)) { result.add(groupMap.get(nestedGroupFieldName)); @@ -1444,8 +1444,8 @@ private boolean refreshExpiredToken( User u = User.get2(a); LOGGER.log( Level.FINE, - "Token refresh. Current Authentitcation principal: " + a.getName() + " user id:" - + (u == null ? "null user" : u.getId()) + " newly retreived username would have been: " + "Token refresh. Current Authentication principal: " + a.getName() + " user id:" + + (u == null ? "null user" : u.getId()) + " newly retrieved username would have been: " + username); } username = expectedUsername; diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java b/src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java index c11577f7..9849bd3e 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java @@ -153,17 +153,16 @@ public OIDCProviderMetadata toProviderMetadata() { // should really be a UI option, but was not previously // server is mandated to support HS256 but if we do not specify things that it produces // then they would never be checked. - // rather we just say "I support anything, and let the check for the specific ones fail and fall through - ArrayList allAlgorthms = new ArrayList<>(); - allAlgorthms.addAll(JWSAlgorithm.Family.HMAC_SHA); + // rather we just say "I support anything, and let the check for the specific ones fail and fall through" + ArrayList allAlgorithms = new ArrayList<>(JWSAlgorithm.Family.HMAC_SHA); if (FIPS140.useCompliantAlgorithms()) { // In FIPS-140 Family.ED is not supported - allAlgorthms.addAll(JWSAlgorithm.Family.RSA); - allAlgorthms.addAll(JWSAlgorithm.Family.EC); + allAlgorithms.addAll(JWSAlgorithm.Family.RSA); + allAlgorithms.addAll(JWSAlgorithm.Family.EC); } else { - allAlgorthms.addAll(JWSAlgorithm.Family.SIGNATURE); + allAlgorithms.addAll(JWSAlgorithm.Family.SIGNATURE); } - providerMetadata.setIDTokenJWSAlgs(allAlgorthms); + providerMetadata.setIDTokenJWSAlgs(allAlgorithms); return providerMetadata; } catch (URISyntaxException e) { throw new IllegalStateException("could not create provider metadata", e); From 4da425c3818bc43b4c769ba1694b9e4e18adbc37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 21:01:47 +0100 Subject: [PATCH 03/15] Remove unused method --- src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 2f9c5a94..6cdc2264 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -737,10 +737,6 @@ protected static Expression compileJMESPath(String str, String logCommen return null; } - private Object applyJMESPath(Expression expression, Object map) { - return expression.search(map); - } - @DataBoundSetter public void setGroupsFieldName(String groupsFieldName) { this.groupsFieldName = Util.fixEmptyAndTrim(groupsFieldName); From 4107d5a76e33be30985b9adaf75b914c7b4332fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 21:02:31 +0100 Subject: [PATCH 04/15] Do not access static member compileJMESPath via instance reference --- src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 6cdc2264..fca3fa47 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -740,7 +740,7 @@ protected static Expression compileJMESPath(String str, String logCommen @DataBoundSetter public void setGroupsFieldName(String groupsFieldName) { this.groupsFieldName = Util.fixEmptyAndTrim(groupsFieldName); - this.groupsFieldExpr = this.compileJMESPath(this.groupsFieldName, "groups field"); + this.groupsFieldExpr = compileJMESPath(this.groupsFieldName, "groups field"); } @DataBoundSetter From 9fd18664cf8fe3af323bc975ea6456e6ecfc4fa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 21:02:46 +0100 Subject: [PATCH 05/15] Fix typo --- src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index fca3fa47..478d1ed5 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -669,9 +669,9 @@ private void filterJwsAlgorithms(@NonNull OIDCProviderMetadata oidcProviderMetad } if (oidcProviderMetadata.getClientRegistrationAuthnJWSAlgs() != null) { - List clientRegisterationAuth = OicAlgorithmValidatorFIPS140.getFipsCompliantJWSAlgorithm( + List clientRegistrationAuth = OicAlgorithmValidatorFIPS140.getFipsCompliantJWSAlgorithm( oidcProviderMetadata.getClientRegistrationAuthnJWSAlgs()); - oidcProviderMetadata.setClientRegistrationAuthnJWSAlgs(clientRegisterationAuth); + oidcProviderMetadata.setClientRegistrationAuthnJWSAlgs(clientRegistrationAuth); } } From df31734196535b091ae63e5de8c0c9a51b0aecaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 21:03:11 +0100 Subject: [PATCH 06/15] Remove duplicate check --- .../java/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 478d1ed5..550defed 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -573,12 +573,6 @@ private void filterEncryptionMethods(@NonNull OIDCProviderMetadata oidcProviderM oidcProviderMetadata.setUserInfoJWEEncs(userInfoJWEEncs); } - if (oidcProviderMetadata.getRequestObjectJWEEncs() != null) { - List requestObjectJweEncs = OicAlgorithmValidatorFIPS140.getFipsCompliantEncryptionMethod( - oidcProviderMetadata.getRequestObjectJWEEncs()); - oidcProviderMetadata.setRequestObjectJWEEncs(requestObjectJweEncs); - } - if (oidcProviderMetadata.getAuthorizationJWEEncs() != null) { List authJweEncs = OicAlgorithmValidatorFIPS140.getFipsCompliantEncryptionMethod( oidcProviderMetadata.getAuthorizationJWEEncs()); From 1a406c13d4d2073d10f3f42012e0625c6dac8743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 21:03:38 +0100 Subject: [PATCH 07/15] Remove unnecessary return statement in 'void' method --- src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 550defed..21fa5207 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -946,7 +946,6 @@ public void doCommenceLogin(@QueryParameter String from, @Header("Referer") fina // store the redirect url for after the login. sessionStore.set(webContext, SESSION_POST_LOGIN_REDIRECT_URL_KEY, redirectOnFinish); JEEHttpActionAdapter.INSTANCE.adapt(redirectionAction, webContext); - return; } @SuppressFBWarnings( @@ -1300,7 +1299,6 @@ public void doFinishLogin(StaplerRequest request, StaplerResponse response) thro } catch (HttpAction e) { // this may be an OK flow for logout login is handled upstream. JEEHttpActionAdapter.INSTANCE.adapt(e, webContext); - return; } } From b0a515f850627b20673970dc9208a42d8b122b7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 21:04:45 +0100 Subject: [PATCH 08/15] Condition 'field != null' already covered by subsequent condition 'field instanceof String' --- src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 21fa5207..36b15191 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -1022,7 +1022,7 @@ private String determineStringField(Expression fieldExpr, JWT idToken, M if (fieldExpr != null) { if (userInfo != null) { Object field = fieldExpr.search(userInfo); - if (field != null && field instanceof String) { + if (field instanceof String) { String fieldValue = Util.fixEmptyAndTrim((String) field); if (fieldValue != null) { return fieldValue; From a3f1467e8c9009fe11ebba939e0d31208aadc351 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 21:05:05 +0100 Subject: [PATCH 09/15] Simplify statement --- .../java/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 36b15191..428bd5ea 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -1030,11 +1030,8 @@ private String determineStringField(Expression fieldExpr, JWT idToken, M } } if (idToken != null) { - String fieldValue = Util.fixEmptyAndTrim( + return Util.fixEmptyAndTrim( getStringField(idToken.getJWTClaimsSet().getClaims(), fieldExpr)); - if (fieldValue != null) { - return fieldValue; - } } } return null; From 8884688c47dfc8b93e4c2e74630555880622f210 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Sat, 14 Dec 2024 21:06:29 +0100 Subject: [PATCH 10/15] Remove superfluous log statement --- src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 428bd5ea..646bf594 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -1053,7 +1053,7 @@ private List determineAuthorities(JWT idToken, Map { + @NonNull + @Override + public String getDisplayName() { + return "Query Parameter Configuration"; + } + + @POST + public FormValidation doCheckQueryParamName(@QueryParameter String queryParamName) { + Jenkins.get().checkPermission(Jenkins.ADMINISTER); + if (Util.fixEmptyAndTrim(queryParamName) == null) { + return FormValidation.error(Messages.OicQueryParameterConfiguration_QueryParameterNameRequired()); + } + return FormValidation.ok(); + } + } +} diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 9ea95563..b566ddca 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -71,13 +71,17 @@ import java.util.ArrayList; import java.util.Base64; import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Random; +import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; import java.util.regex.Pattern; +import java.util.stream.Collectors; import javax.annotation.PostConstruct; import javax.servlet.Filter; import javax.servlet.FilterChain; @@ -305,6 +309,9 @@ ClientAuthenticationMethod toClientAuthenticationMethod() { */ private transient ProxyAwareResourceRetriever proxyAwareResourceRetriever; + private List loginQueryParamNameValuePairs; + private List logoutQueryParamNameValuePairs; + @DataBoundConstructor public OicSecurityRealm( String clientId, @@ -357,6 +364,9 @@ protected Object readResolve() throws ObjectStreamException { // ensure escapeHatchSecret is encrypted this.setEscapeHatchSecret(this.escapeHatchSecret); + this.setLoginQueryParamNameValuePairs(this.loginQueryParamNameValuePairs); + this.setLogoutQueryParamNameValuePairs(this.logoutQueryParamNameValuePairs); + // validate this option in FIPS env or not try { this.setEscapeHatchEnabled(this.escapeHatchEnabled); @@ -397,6 +407,24 @@ protected Object readResolve() throws ObjectStreamException { return this; } + @DataBoundSetter + public void setLoginQueryParamNameValuePairs(List values) { + this.loginQueryParamNameValuePairs = values; + } + + public List getLoginQueryParamNameValuePairs() { + return loginQueryParamNameValuePairs; + } + + @DataBoundSetter + public void setLogoutQueryParamNameValuePairs(List values) { + this.logoutQueryParamNameValuePairs = values; + } + + public List getLogoutQueryParamNameValuePairs() { + return logoutQueryParamNameValuePairs; + } + public String getClientId() { return clientId; } @@ -505,7 +533,7 @@ ProxyAwareResourceRetriever getResourceRetriever() { return proxyAwareResourceRetriever; } - private OidcConfiguration buildOidcConfiguration() { + private OidcConfiguration buildOidcConfiguration(boolean addCustomLoginParams) { // TODO cache this and use the well known if available. OidcConfiguration conf = new CustomOidcConfiguration(this.isDisableSslVerification()); conf.setClientId(clientId); @@ -534,9 +562,37 @@ private OidcConfiguration buildOidcConfiguration() { if (this.isPkceEnabled()) { conf.setPkceMethod(CodeChallengeMethod.S256); } + if (addCustomLoginParams && loginQueryParamNameValuePairs != null && !loginQueryParamNameValuePairs.isEmpty()) { + Set forbiddenKeys = Set.of( + OidcConfiguration.SCOPE, + OidcConfiguration.RESPONSE_TYPE, + OidcConfiguration.RESPONSE_MODE, + OidcConfiguration.REDIRECT_URI, + OidcConfiguration.CLIENT_ID, + OidcConfiguration.STATE, + OidcConfiguration.MAX_AGE, + OidcConfiguration.PROMPT, + OidcConfiguration.NONCE, + OidcConfiguration.CODE_CHALLENGE, + OidcConfiguration.CODE_CHALLENGE_METHOD); + Map customParameterMap = + getCustomParametersMap(loginQueryParamNameValuePairs, forbiddenKeys); + LOGGER.info("Append the following custom parameters to the authorize endpoint: " + customParameterMap); + customParameterMap.forEach(conf::addCustomParam); + } return conf; } + Map getCustomParametersMap( + List queryParamNameValuePairs, Set forbiddenKeys) { + return queryParamNameValuePairs.stream() + .filter(c -> Util.fixEmptyAndTrim(c.getQueryParamName()) != null) + .filter(c -> !forbiddenKeys.contains(c.getQueryParamNameDecoded())) + .collect(Collectors.toMap( + OicQueryParameterConfiguration::getQueryParamNameDecoded, + OicQueryParameterConfiguration::getQueryParamValueDecoded)); + } + // Visible for testing @Restricted(NoExternalUse.class) protected void filterNonFIPS140CompliantAlgorithms(@NonNull OIDCProviderMetadata oidcProviderMetadata) { @@ -670,8 +726,8 @@ private void filterJwsAlgorithms(@NonNull OIDCProviderMetadata oidcProviderMetad } @Restricted(NoExternalUse.class) // exposed for testing only - protected OidcClient buildOidcClient() { - OidcConfiguration oidcConfiguration = buildOidcConfiguration(); + protected OidcClient buildOidcClient(boolean addCustomLoginParams) { + OidcConfiguration oidcConfiguration = buildOidcConfiguration(addCustomLoginParams); OidcClient client = new OidcClient(oidcConfiguration); // add the extra settings for the client... client.setCallbackUrl(buildOAuthRedirectUrl()); @@ -932,7 +988,7 @@ protected String getValidRedirectUrl(String url) { public void doCommenceLogin(@QueryParameter String from, @Header("Referer") final String referer) throws URISyntaxException { - OidcClient client = buildOidcClient(); + OidcClient client = buildOidcClient(true); // add the extra params for the client... final String redirectOnFinish = getValidRedirectUrl(from != null ? from : referer); @@ -1172,7 +1228,7 @@ public String getPostLogOutUrl2(StaplerRequest req, Authentication auth) { @VisibleForTesting Object getStateAttribute(HttpSession session) { // return null; - OidcClient client = buildOidcClient(); + OidcClient client = buildOidcClient(false); WebContext webContext = JEEContextFactory.INSTANCE.newContext(Stapler.getCurrentRequest(), Stapler.getCurrentResponse()); SessionStore sessionStore = JEESessionStoreFactory.INSTANCE.newSessionStore(); @@ -1183,22 +1239,49 @@ Object getStateAttribute(HttpSession session) { } @CheckForNull - private String maybeOpenIdLogoutEndpoint(String idToken, String state, String postLogoutRedirectUrl) { + String maybeOpenIdLogoutEndpoint(String idToken, String state, String postLogoutRedirectUrl) { final URI url = serverConfiguration.toProviderMetadata().getEndSessionEndpointURI(); if (this.logoutFromOpenidProvider && url != null) { - StringBuilder openidLogoutEndpoint = new StringBuilder(url.toString()); - + Map segmentsMap = new HashMap<>(); + Set segmentsSet = new HashSet<>(); if (!Strings.isNullOrEmpty(idToken)) { - openidLogoutEndpoint.append("?id_token_hint=").append(idToken).append("&"); - } else { - openidLogoutEndpoint.append("?"); + segmentsMap.put("id_token_hint", idToken); + } + if (!Strings.isNullOrEmpty(state) && !"null".equals(state)) { + segmentsMap.put("state", state); } - openidLogoutEndpoint.append("state=").append(state); - if (postLogoutRedirectUrl != null) { - openidLogoutEndpoint - .append("&post_logout_redirect_uri=") - .append(URLEncoder.encode(postLogoutRedirectUrl, StandardCharsets.UTF_8)); + segmentsMap.put( + "post_logout_redirect_uri", URLEncoder.encode(postLogoutRedirectUrl, StandardCharsets.UTF_8)); + } + Set forbiddenKeys = Set.of("id_token_hint", "state", "post_logout_redirect_uri"); + if (logoutQueryParamNameValuePairs != null && !logoutQueryParamNameValuePairs.isEmpty()) { + Map customParameterMap = + getCustomParametersMap(logoutQueryParamNameValuePairs, forbiddenKeys); + LOGGER.info("Append the following custom parameters to the logout endpoint: " + customParameterMap); + + customParameterMap.forEach((k, v) -> { + String key = k.trim(); + String value = v.trim(); + if (value.isEmpty()) { + segmentsSet.add(key); + } else { + segmentsMap.put(key, value); + } + }); + } + + StringBuilder openidLogoutEndpoint = new StringBuilder(url.toString()); + String concatChar = openidLogoutEndpoint.toString().contains("?") ? "&" : "?"; + if (!segmentsMap.isEmpty()) { + String joinedString = segmentsMap.entrySet().stream() + .map(entry -> entry.getKey() + "=" + entry.getValue()) + .collect(Collectors.joining("&")); + openidLogoutEndpoint.append(concatChar).append(joinedString); + concatChar = "&"; + } + if (!segmentsSet.isEmpty()) { + openidLogoutEndpoint.append(concatChar).append(String.join("&", segmentsSet)); } return openidLogoutEndpoint.toString(); } @@ -1243,7 +1326,7 @@ private String buildOAuthRedirectUrl() throws NullPointerException { * @throws ParseException if the JWT (or other response) could not be parsed. */ public void doFinishLogin(StaplerRequest request, StaplerResponse response) throws IOException, ParseException { - OidcClient client = buildOidcClient(); + OidcClient client = buildOidcClient(false); WebContext webContext = JEEContextFactory.INSTANCE.newContext(request, response); SessionStore sessionStore = JEESessionStoreFactory.INSTANCE.newSessionStore(); @@ -1386,7 +1469,7 @@ private boolean refreshExpiredToken( WebContext webContext = JEEContextFactory.INSTANCE.newContext(httpRequest, httpResponse); SessionStore sessionStore = JEESessionStoreFactory.INSTANCE.newSessionStore(); - OidcClient client = buildOidcClient(); + OidcClient client = buildOidcClient(false); // PAC4J maintains the nonce even though servers should not respond with an id token containing the nonce // https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse // it SHOULD NOT have a nonce Claim, even when the ID Token issued at the time of the original authentication diff --git a/src/main/resources/org/jenkinsci/plugins/oic/Messages.properties b/src/main/resources/org/jenkinsci/plugins/oic/Messages.properties index f86d37fe..41c8027d 100644 --- a/src/main/resources/org/jenkinsci/plugins/oic/Messages.properties +++ b/src/main/resources/org/jenkinsci/plugins/oic/Messages.properties @@ -1,5 +1,8 @@ OicLogoutAction.OicLogout = Oic Logout +OicQueryParameterConfiguration.QueryParameterNameRequired = Query parameter name is required. +OicQueryParameterConfiguration.QueryParameterValueRequired = Query parameter value is required. + OicSecurityRealm.DisplayName = Login with Openid Connect OicSecurityRealm.CouldNotRefreshToken = Unable to refresh access token OicSecurityRealm.ClientIdRequired = Client id is required. diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration/config.jelly b/src/main/resources/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration/config.jelly new file mode 100644 index 00000000..5583a825 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration/config.jelly @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration/config.properties b/src/main/resources/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration/config.properties new file mode 100644 index 00000000..eeba9906 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration/config.properties @@ -0,0 +1,2 @@ +QueryParameterName=Query Parameter Name +QueryParameterValue=Query Parameter Value diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration/help.html b/src/main/resources/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration/help.html new file mode 100644 index 00000000..c1484ebd --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration/help.html @@ -0,0 +1,3 @@ +
+ Additional custom query parameters added to a URL. +
diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.jelly b/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.jelly index fed2da9c..4beb002d 100644 --- a/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.jelly @@ -26,6 +26,28 @@ + + + + +
+ + + + + +
+ + + diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.properties b/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.properties index fb8acda3..f5436878 100644 --- a/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.properties +++ b/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.properties @@ -14,6 +14,12 @@ EnablePKCE=Enable Proof Key for Code Exchange (PKCE) FullnameFieldName=Full name field name Group=Group GroupsFieldName=Groups field name +LoginLogoutQueryParametersTitle=Query Parameters for Login and Logout Endpoints +LoginLogoutQueryParamNameValuePairs.header=Query Parameter +LoginQueryParametersTitle=Query Parameters for Login Endpoint +LoginQueryParamNameValuePairs.add=Add Login Query Parameter +LogoutQueryParametersTitle=Query Parameters for Logout Endpoint +LogoutQueryParamNameValuePairs.add=Add Logout Query Parameter LogoutFromOpenIDProvider=Logout from OpenID Provider PostLogoutRedirectUrl=Post logout redirect URL Secret=Secret diff --git a/src/test/java/org/jenkinsci/plugins/oic/ConfigurationAsCodeTest.java b/src/test/java/org/jenkinsci/plugins/oic/ConfigurationAsCodeTest.java index d5de7619..e415e25b 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/ConfigurationAsCodeTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/ConfigurationAsCodeTest.java @@ -11,6 +11,7 @@ import io.jenkins.plugins.casc.model.CNode; import java.util.ArrayList; import java.util.List; +import java.util.stream.Collectors; import jenkins.model.Jenkins; import org.jenkinsci.plugins.oic.OicSecurityRealm.TokenAuthMethod; import org.junit.Rule; @@ -27,6 +28,7 @@ import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -68,6 +70,18 @@ public void testConfig() { assertTrue(oicSecurityRealm.isRootURLFromRequest()); assertEquals("http://localhost/jwks", serverConf.getJwksServerUrl()); assertFalse(oicSecurityRealm.isDisableTokenVerification()); + assertNotNull(oicSecurityRealm.getLoginQueryParamNameValuePairs()); + assertNotNull(oicSecurityRealm.getLogoutQueryParamNameValuePairs()); + assertEquals( + "loginkey1x\"xx@me=loginvalue1xxxx@you&?loginneu&/test==login?sunny%&/xx\"x", + oicSecurityRealm.getLoginQueryParamNameValuePairs().stream() + .map(config -> config.getQueryParamName() + "=" + config.getQueryParamValue()) + .collect(Collectors.joining("&"))); + assertEquals( + "logoutkey1x\"xx@me=logoutvalue1xxxx@you&?logoutneu&/test==logout?sunny%&/xx\"x", + oicSecurityRealm.getLogoutQueryParamNameValuePairs().stream() + .map(config -> config.getQueryParamName() + "=" + config.getQueryParamValue()) + .collect(Collectors.joining("&"))); } @Test @@ -120,8 +134,10 @@ public void testMinimal() throws Exception { assertEquals("sub", oicSecurityRealm.getUserNameField()); assertTrue(oicSecurityRealm.isLogoutFromOpenidProvider()); assertFalse(oicSecurityRealm.isRootURLFromRequest()); - assertEquals(null, serverConf.getJwksServerUrl()); + assertNull(serverConf.getJwksServerUrl()); assertFalse(oicSecurityRealm.isDisableTokenVerification()); + assertNull(oicSecurityRealm.getLoginQueryParamNameValuePairs()); + assertNull(oicSecurityRealm.getLogoutQueryParamNameValuePairs()); } @Rule(order = 0) @@ -163,6 +179,9 @@ public void testMinimalWellKnown() throws Exception { assertFalse(oicSecurityRealm.isDisableTokenVerification()); assertEquals(urlBase + "/well.known", serverConf.getWellKnownOpenIDConfigurationUrl()); + + assertNull(oicSecurityRealm.getLoginQueryParamNameValuePairs()); + assertNull(oicSecurityRealm.getLogoutQueryParamNameValuePairs()); } /** Class to setup WireMockRule for well known with stub and setting port in env variable diff --git a/src/test/java/org/jenkinsci/plugins/oic/OicQueryParameterConfigurationTest.java b/src/test/java/org/jenkinsci/plugins/oic/OicQueryParameterConfigurationTest.java new file mode 100644 index 00000000..616aaf36 --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/oic/OicQueryParameterConfigurationTest.java @@ -0,0 +1,81 @@ +package org.jenkinsci.plugins.oic; + +import hudson.Util; +import hudson.util.FormValidation; +import org.hamcrest.Matcher; +import org.jenkinsci.plugins.oic.OicQueryParameterConfiguration.DescriptorImpl; +import org.junit.ClassRule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.WithoutJenkins; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.hasProperty; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.jvnet.hudson.test.JenkinsMatchers.hasKind; + +public class OicQueryParameterConfigurationTest { + + @ClassRule + public static JenkinsRule jenkinsRule = new JenkinsRule(); + + @Test + @WithoutJenkins + public void testOicQueryParameterConfiguration() { + assertThrows(IllegalStateException.class, () -> new OicQueryParameterConfiguration("", "")); + } + + @Test + @WithoutJenkins + public void testQueryParameterDecoded() { + OicQueryParameterConfiguration configClean = new OicQueryParameterConfiguration("key-1", "value-1"); + assertEquals("key-1", configClean.getQueryParamName()); + assertEquals("key-1", configClean.getQueryParamNameDecoded()); + assertEquals("value-1", configClean.getQueryParamValue()); + assertEquals("value-1", configClean.getQueryParamValueDecoded()); + + OicQueryParameterConfiguration configEmptyValue = new OicQueryParameterConfiguration("key-2", ""); + assertEquals("key-2", configEmptyValue.getQueryParamName()); + assertEquals("key-2", configEmptyValue.getQueryParamNameDecoded()); + assertEquals("", configEmptyValue.getQueryParamValue()); + assertEquals("", configEmptyValue.getQueryParamValueDecoded()); + configEmptyValue.setQueryParamName(null); + assertNull(configEmptyValue.getQueryParamName()); + assertNull(configEmptyValue.getQueryParamNameDecoded()); + configEmptyValue.setQueryParamValue(null); + assertNull(configEmptyValue.getQueryParamValue()); + assertNull(configEmptyValue.getQueryParamValueDecoded()); + + OicQueryParameterConfiguration config = + new OicQueryParameterConfiguration("key-a\"b/c?d#e:f@g&h=i+j$+k,l", "value-a\"b/c?d#e:f@g&h=i+j$+k,l"); + assertEquals("key-a\"b/c?d#e:f@g&h=i+j$+k,l", config.getQueryParamName()); + assertEquals("key-a%22b%2Fc%3Fd%23e%3Af%40g%26h%3Di%2Bj%24%2Bk%2Cl", config.getQueryParamNameDecoded()); + assertEquals("value-a\"b/c?d#e:f@g&h=i+j$+k,l", config.getQueryParamValue()); + assertEquals("value-a%22b%2Fc%3Fd%23e%3Af%40g%26h%3Di%2Bj%24%2Bk%2Cl", config.getQueryParamValueDecoded()); + } + + @Test + public void testDoCheckQueryParamName() { + DescriptorImpl descriptor = getDescriptor(); + assertThat( + descriptor.doCheckQueryParamName(null), + allOf(hasKind(FormValidation.Kind.ERROR), withMessage("Query parameter name is required."))); + assertThat( + descriptor.doCheckQueryParamName(""), + allOf(hasKind(FormValidation.Kind.ERROR), withMessage("Query parameter name is required."))); + assertThat(descriptor.doCheckQueryParamName("test"), hasKind(FormValidation.Kind.OK)); + } + + private static DescriptorImpl getDescriptor() { + return (DescriptorImpl) jenkinsRule.jenkins.getDescriptor(OicQueryParameterConfiguration.class); + } + + private static Matcher withMessage(String message) { + // the FormValidation message will be escaped for HTML, so we escape what we expect. + return hasProperty("message", is(Util.escape(message))); + } +} diff --git a/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java b/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java index 32d859cc..b9906b7e 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java @@ -3,6 +3,10 @@ import com.github.tomakehurst.wiremock.core.WireMockConfiguration; import com.github.tomakehurst.wiremock.junit.WireMockRule; import hudson.util.Secret; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; import org.acegisecurity.AuthenticationManager; import org.acegisecurity.BadCredentialsException; import org.acegisecurity.GrantedAuthority; @@ -12,12 +16,14 @@ import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.WithoutJenkins; import org.springframework.security.crypto.bcrypt.BCrypt; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; public class OicSecurityRealmTest { @@ -142,4 +148,101 @@ public void testShouldCheckEscapeHatchWithHashedPassword() throws Exception { assertFalse(realm.doCheckEscapeHatch("otherUsername", escapeHatchPassword)); assertFalse(realm.doCheckEscapeHatch(escapeHatchUsername, "wrongPassword")); } + + @Test + @WithoutJenkins + public void testGetCustomLoginParameters() throws Exception { + TestRealm realm = + new TestRealm.Builder(wireMockRule).WithMinimalDefaults().build(); + Set forbiddenKeys = Set.of("forbidden-key"); + + Map unsortedMapExpected = Map.of( + "b", "%2C", + "%26test", "2%40%2B+%2C+%3F", + "a%2Ftest%23", "1", + "b%2B", "%24other%3Anew", + "d%3D", "2", + "e%3F", ""); + + OicQueryParameterConfiguration empty = new OicQueryParameterConfiguration("non-empty", ""); + empty.setQueryParamName(null); + empty.setQueryParamValue(null); + + Map unsortedMapResult = realm.getCustomParametersMap( + List.of( + new OicQueryParameterConfiguration("a/test#", "1"), + new OicQueryParameterConfiguration("b", ","), + new OicQueryParameterConfiguration("b+", "$other:new"), + new OicQueryParameterConfiguration("&test", " 2@+ , ?"), + new OicQueryParameterConfiguration("d=", " 2 "), + new OicQueryParameterConfiguration(" e? ", " "), + empty, + new OicQueryParameterConfiguration("forbidden-key", "test")), + forbiddenKeys); + assertEquals(new TreeMap<>(unsortedMapExpected), new TreeMap<>(unsortedMapResult)); + } + + @Test + @WithoutJenkins + public void testMaybeOpenIdLogoutEndpoint() throws Exception { + TestRealm realm = new TestRealm.Builder(wireMockRule) + .WithMinimalDefaults() + .WithLogout(Boolean.FALSE, "https://endpoint") + .build(); + assertNull(realm.maybeOpenIdLogoutEndpoint("my-id-token", null, "https://localhost")); + + realm = new TestRealm.Builder(wireMockRule) + .WithMinimalDefaults().WithLogout(Boolean.TRUE, null).build(); + assertNull(realm.maybeOpenIdLogoutEndpoint("my-id-token", null, "https://localhost")); + + realm = new TestRealm.Builder(wireMockRule) + .WithMinimalDefaults().WithLogout(Boolean.FALSE, null).build(); + assertNull(realm.maybeOpenIdLogoutEndpoint("my-id-token", null, "https://localhost")); + + realm = new TestRealm.Builder(wireMockRule) + .WithMinimalDefaults() + .WithLogout(Boolean.TRUE, "https://endpoint?query-param-1=test") + .build(); + assertEquals( + "https://endpoint?query-param-1=test&id_token_hint=my-id-token&post_logout_redirect_uri=https%3A%2F%2Flocalhost", + realm.maybeOpenIdLogoutEndpoint("my-id-token", null, "https://localhost")); + } + + @Test + @WithoutJenkins + public void testMaybeOpenIdLogoutEndpointWithNoCustomLogoutQueryParameters() throws Exception { + TestRealm realm = new TestRealm.Builder(wireMockRule) + .WithMinimalDefaults().WithLogout(true, "https://endpoint").build(); + assertEquals( + "https://endpoint?id_token_hint=my-id-token&post_logout_redirect_uri=https%3A%2F%2Flocalhost", + realm.maybeOpenIdLogoutEndpoint("my-id-token", "null", "https://localhost")); + assertEquals( + "https://endpoint?id_token_hint=my-id-token&post_logout_redirect_uri=https%3A%2F%2Flocalhost", + realm.maybeOpenIdLogoutEndpoint("my-id-token", null, "https://localhost")); + assertEquals( + "https://endpoint?id_token_hint=my-id-token&state=test&post_logout_redirect_uri=https%3A%2F%2Flocalhost", + realm.maybeOpenIdLogoutEndpoint("my-id-token", "test", "https://localhost")); + assertEquals("https://endpoint", realm.maybeOpenIdLogoutEndpoint(null, null, null)); + } + + @Test + @WithoutJenkins + public void testMaybeOpenIdLogoutEndpointWithCustomLogoutQueryParameters() throws Exception { + TestRealm realm = new TestRealm.Builder(wireMockRule) + .WithMinimalDefaults() + .WithLogoutQueryParameters(List.of( + new OicQueryParameterConfiguration("key1", " with-spaces "), + new OicQueryParameterConfiguration("param-only", ""), + new OicQueryParameterConfiguration("id_token_hint", "overwrite-test-1"), + new OicQueryParameterConfiguration("post_logout_redirect_uri", "overwrite-test-2"), + new OicQueryParameterConfiguration("state", "overwrite-test-3"))) + .WithLogout(true, "https://endpoint") + .build(); + String result = realm.maybeOpenIdLogoutEndpoint("my-id-token", "test", "https://localhost"); + assertNotNull(result); + assertFalse(result.contains("overwrite-test")); + assertEquals( + "https://endpoint?key1=with-spaces&id_token_hint=my-id-token&state=test&post_logout_redirect_uri=https%3A%2F%2Flocalhost¶m-only", + result); + } } diff --git a/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java b/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java index 4d2fcb34..f54dc880 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java @@ -374,6 +374,25 @@ public void testLoginWithAutoConfiguration() throws Exception { assertTestUserIsMemberOfTestGroups(user); } + @Test + public void testLoginWithCustomLoginParameters() throws Exception { + mockAuthorizationRedirectsToFinishLogin(); + mockTokenReturnsIdTokenWithGroup(); + mockUserInfoWithTestGroups(); + configureWellKnown(null, null); + jenkins.setSecurityRealm(new TestRealm.Builder(wireMockRule) + .WithMinimalDefaults() + .WithAutomanualconfigure(true) + .WithLoginQueryParameters(List.of( + new OicQueryParameterConfiguration("queryLoginParamName", "queryLoginParamValue"))) + .build()); + assertAnonymous(); + browseLoginPage(); + var user = assertTestUser(); + assertTestUserEmail(user); + assertTestUserIsMemberOfTestGroups(user); + } + @Test public void testLoginWithAutoConfiguration_WithNoScope() throws Exception { mockAuthorizationRedirectsToFinishLogin(); @@ -1000,7 +1019,28 @@ public void testLogoutShouldBeProviderURLWhenProviderLogoutConfigured() throws E logoutURL[0] = oicsr.getPostLogOutUrl2(Stapler.getCurrentRequest(), Jenkins.ANONYMOUS2); return null; }); - assertEquals("http://provider/logout?state=null", logoutURL[0]); + assertEquals("http://provider/logout", logoutURL[0]); + } + + @Test + public void testLogoutShouldBeProviderURLWhenProviderLogoutConfiguredWithAdditionalLogoutQueryParameters() + throws Exception { + final TestRealm oicsr = new TestRealm.Builder(wireMockRule) + .WithLogoutQueryParameters(List.of( + new OicQueryParameterConfiguration("hello", "world"), + new OicQueryParameterConfiguration("state", "test"), + new OicQueryParameterConfiguration("single", ""), + new OicQueryParameterConfiguration("id_token_hint", "other"))) + .WithLogout(Boolean.TRUE, "http://provider/logout") + .build(); + jenkins.setSecurityRealm(oicsr); + + String[] logoutURL = new String[1]; + jenkinsRule.executeOnServer(() -> { + logoutURL[0] = oicsr.getPostLogOutUrl2(Stapler.getCurrentRequest(), Jenkins.ANONYMOUS2); + return null; + }); + assertEquals("http://provider/logout?hello=world&single", logoutURL[0]); } @Test @@ -1018,7 +1058,7 @@ public void testLogoutShouldBeProviderURLWithRedirectWhenProviderLogoutConfigure return null; }); assertEquals( - "http://provider/logout?state=null&post_logout_redirect_uri=http%3A%2F%2Fsee.it%2F%3Fcat%26color%3Dwhite", + "http://provider/logout?post_logout_redirect_uri=http%3A%2F%2Fsee.it%2F%3Fcat%26color%3Dwhite", logoutURL[0]); } diff --git a/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java b/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java index eec1d58e..d1de39ad 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java +++ b/src/test/java/org/jenkinsci/plugins/oic/TestRealm.java @@ -8,6 +8,7 @@ import java.io.IOException; import java.io.ObjectStreamException; import java.text.ParseException; +import java.util.List; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; import org.pac4j.core.context.WebContext; @@ -40,6 +41,8 @@ public static class Builder { public String fullNameFieldName = FULL_NAME_FIELD; public String emailFieldName = null; public String scopes = null; + public List loginQueryParameters = null; + public List logoutQueryParameters = null; public String groupsFieldName = null; public boolean disableSslVerification = false; public Boolean logoutFromOpenidProvider = false; @@ -115,6 +118,16 @@ public Builder WithScopes(String scopes) { return this; } + public Builder WithLoginQueryParameters(List values) { + this.loginQueryParameters = values; + return this; + } + + public Builder WithLogoutQueryParameters(List values) { + this.logoutQueryParameters = values; + return this; + } + public Builder WithMinimalDefaults() { return this.WithEmailFieldName(EMAIL_FIELD).WithGroupsFieldName(GROUPS_FIELD); } @@ -196,6 +209,8 @@ public TestRealm(Builder builder) throws Exception { this.setEscapeHatchSecret(builder.escapeHatchSecret); this.setEscapeHatchGroup(builder.escapeHatchGroup); this.setDisableTokenVerification(builder.disableTokenValidation); + this.setLoginQueryParamNameValuePairs(builder.loginQueryParameters); + this.setLogoutQueryParamNameValuePairs(builder.logoutQueryParameters); // need to call the following method annotated with @PostConstruct and called // from readResolve and as such // is only called in regular use not code use. @@ -267,7 +282,7 @@ public void doFinishLogin(StaplerRequest request, StaplerResponse response) thro // only hack the nonce if the nonce is enabled WebContext webContext = JEEContextFactory.INSTANCE.newContext(request, response); SessionStore sessionStore = JEESessionStoreFactory.INSTANCE.newSessionStore(); - OidcClient oidcClient = buildOidcClient(); + OidcClient oidcClient = buildOidcClient(true); sessionStore.set(webContext, oidcClient.getNonceSessionAttributeName(), "nonce"); } super.doFinishLogin(request, response); diff --git a/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCode.yml b/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCode.yml index fbd70922..ea0b686c 100644 --- a/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCode.yml +++ b/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCode.yml @@ -25,3 +25,13 @@ jenkins: sendScopesInTokenRequest: true pkceEnabled: true nonceDisabled: true + loginQueryParamNameValuePairs: + - queryParamName: "loginkey1x\"xx@me" + queryParamValue: "loginvalue1xxxx@you" + - queryParamName: "?loginneu&/test=" + queryParamValue: "login?sunny%&/xx\"x" + logoutQueryParamNameValuePairs: + - queryParamName: "logoutkey1x\"xx@me" + queryParamValue: "logoutvalue1xxxx@you" + - queryParamName: "?logoutneu&/test=" + queryParamValue: "logout?sunny%&/xx\"x" diff --git a/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCodeExport.yml b/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCodeExport.yml index b8d603a2..6adcb677 100644 --- a/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCodeExport.yml +++ b/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCodeExport.yml @@ -6,6 +6,16 @@ escapeHatchGroup: "escapeHatchGroup" escapeHatchUsername: "escapeHatchUsername" fullNameFieldName: "fullNameFieldName" groupsFieldName: "groupsFieldName" +loginQueryParamNameValuePairs: +- queryParamName: "loginkey1x\"xx@me" + queryParamValue: "loginvalue1xxxx@you" +- queryParamName: "?loginneu&/test=" + queryParamValue: "login?sunny%&/xx\"x" +logoutQueryParamNameValuePairs: +- queryParamName: "logoutkey1x\"xx@me" + queryParamValue: "logoutvalue1xxxx@you" +- queryParamName: "?logoutneu&/test=" + queryParamValue: "logout?sunny%&/xx\"x" nonceDisabled: true pkceEnabled: true rootURLFromRequest: true From 4e5ab83d7ac43e5e509d024a4b8f7bea2a451fdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Wed, 25 Dec 2024 12:55:41 +0100 Subject: [PATCH 14/15] Fix FormValidation check when overwriting scopes --- .../plugins/oic/OicServerWellKnownConfiguration.java | 6 +++--- .../plugins/oic/OicServerWellKnownConfigurationTest.java | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java b/src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java index bbb329b9..504dfc96 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java @@ -219,12 +219,12 @@ public FormValidation doCheckWellKnownOpenIDConfigurationUrl( } @POST - public FormValidation doCheckOverrideScopes(@QueryParameter String overrideScopes) { + public FormValidation doCheckScopesOverride(@QueryParameter String scopesOverride) { Jenkins.get().checkPermission(Jenkins.ADMINISTER); - if (Util.fixEmptyAndTrim(overrideScopes) == null) { + if (Util.fixEmptyAndTrim(scopesOverride) == null) { return FormValidation.ok(); } - if (!overrideScopes.toLowerCase().contains("openid")) { + if (!scopesOverride.toLowerCase().contains("openid")) { return FormValidation.warning(Messages.OicSecurityRealm_RUSureOpenIdNotInScope()); } return FormValidation.ok(); diff --git a/src/test/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfigurationTest.java b/src/test/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfigurationTest.java index 3d1e103c..418bd5e9 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfigurationTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfigurationTest.java @@ -83,13 +83,13 @@ public void doCheckWellKnownOpenIDConfigurationUrl() throws IOException { public void doCheckOverrideScopes() throws IOException { DescriptorImpl descriptor = getDescriptor(); - assertThat(descriptor.doCheckOverrideScopes(null), hasKind(FormValidation.Kind.OK)); - assertThat(descriptor.doCheckOverrideScopes(""), hasKind(FormValidation.Kind.OK)); + assertThat(descriptor.doCheckScopesOverride(null), hasKind(FormValidation.Kind.OK)); + assertThat(descriptor.doCheckScopesOverride(""), hasKind(FormValidation.Kind.OK)); assertThat( - descriptor.doCheckOverrideScopes("openid email profile address phone offline_access"), + descriptor.doCheckScopesOverride("openid email profile address phone offline_access"), hasKind(FormValidation.Kind.OK)); assertThat( - descriptor.doCheckOverrideScopes("blah"), + descriptor.doCheckScopesOverride("blah"), allOf( hasKind(FormValidation.Kind.WARNING), withMessage("Are you sure you don't want to include 'openid' as a scope?"))); From bf28327b386809756cf387571f293d97b1e2455c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20M=C3=BCller?= Date: Wed, 25 Dec 2024 12:55:47 +0100 Subject: [PATCH 15/15] Extend test for OicServerManualConfigurationTest --- .../oic/OicServerManualConfigurationTest.java | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/test/java/org/jenkinsci/plugins/oic/OicServerManualConfigurationTest.java b/src/test/java/org/jenkinsci/plugins/oic/OicServerManualConfigurationTest.java index 8f2309f3..be7654f9 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/OicServerManualConfigurationTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/OicServerManualConfigurationTest.java @@ -1,20 +1,31 @@ package org.jenkinsci.plugins.oic; +import com.nimbusds.jose.JWSAlgorithm; +import com.nimbusds.openid.connect.sdk.op.OIDCProviderMetadata; import hudson.Util; +import hudson.model.Descriptor; import hudson.util.FormValidation; import java.io.IOException; +import java.net.URISyntaxException; +import jenkins.security.FIPS140; import org.hamcrest.Matcher; import org.jenkinsci.plugins.oic.OicServerManualConfiguration.DescriptorImpl; import org.junit.ClassRule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.WithoutJenkins; +import org.mockito.MockedStatic; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasProperty; import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import static org.jvnet.hudson.test.JenkinsMatchers.hasKind; +import static org.mockito.Mockito.mockStatic; public class OicServerManualConfigurationTest { @@ -91,6 +102,31 @@ public void doCheckEndSessionEndpoint() throws IOException { assertThat(descriptor.doCheckEndSessionUrl("http://localhost.jwks"), hasKind(FormValidation.Kind.OK)); } + @Test + @WithoutJenkins + public void testProviderMetadataWithFips() throws Descriptor.FormException { + OicServerManualConfiguration config = new OicServerManualConfiguration("issuer", "t-url", "a-url"); + try (MockedStatic fips140Mock = mockStatic(FIPS140.class)) { + JWSAlgorithm.Family ed = JWSAlgorithm.Family.ED; + JWSAlgorithm arbitraryEdAlgorithm = (JWSAlgorithm) ed.toArray()[0]; + + fips140Mock.when(FIPS140::useCompliantAlgorithms).thenReturn(true); + OIDCProviderMetadata data = config.toProviderMetadata(); + assertFalse(data.getIDTokenJWSAlgs().contains(arbitraryEdAlgorithm)); + + fips140Mock.when(FIPS140::useCompliantAlgorithms).thenReturn(false); + data = config.toProviderMetadata(); + assertTrue(data.getIDTokenJWSAlgs().contains(arbitraryEdAlgorithm)); + } + } + + @Test + @WithoutJenkins + public void testProviderMetadataWithInvalidURI() throws Descriptor.FormException, URISyntaxException { + OicServerManualConfiguration config = new OicServerManualConfiguration("issuer", "t-url", "inv%alid"); + assertThrows(IllegalStateException.class, () -> config.toProviderMetadata()); + } + private static DescriptorImpl getDescriptor() { return (DescriptorImpl) jenkinsRule.jenkins.getDescriptor(OicServerManualConfiguration.class); }