Merge pull request #16 from mrmateo/bugfix/JENKINS-71337

[JENKINS-71337] Fix configuration not being saved.
jenkinsci · May 25, 2023 · 4b2efa8 · 4b2efa8
2 parents 5dda43f + 8e6c4ba
commit 4b2efa8
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 79 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/KeycloakSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/KeycloakSecurityRealm.java
@@ -38,6 +38,7 @@ of this software and associated documentation files (the "Software"), to deal
 import javax.servlet.http.HttpServletRequest;
 
 import com.google.common.base.Strings;
+import edu.umd.cs.findbugs.annotations.NonNull;
 import jenkins.security.SecurityListener;
 import org.acegisecurity.Authentication;
 import org.acegisecurity.AuthenticationException;
@@ -130,7 +131,19 @@ public class KeycloakSecurityRealm extends SecurityRealm {
 	 *             -
 	 */
 	@DataBoundConstructor
-	public KeycloakSecurityRealm() throws IOException {
+	public KeycloakSecurityRealm(String keycloakIdp, String keycloakJson, boolean keycloakValidate, boolean keycloakRespectAccessTokenTimeout) throws IOException {
+		super();
+		if (StringUtils.isEmpty(keycloakJson)) {
+			throw new IllegalArgumentException("Keycloak JSON is a mandatory item.");
+		}
+		setKeycloakIdp(keycloakIdp);
+		setKeycloakJson(keycloakJson);
+		setKeycloakValidate(keycloakValidate);
+		setKeycloakRespectAccessTokenTimeout(keycloakRespectAccessTokenTimeout);
+		createFilter();
+	}
+
+	protected KeycloakSecurityRealm() {
 		super();
 		createFilter();
 	}
@@ -375,21 +388,11 @@ public String getHelpFile() {
 		}
 
 		@Override
+		@NonNull
 		public String getDisplayName() {
 			return "Keycloak Authentication Plugin";
 		}
 
-		/**
-		 * Constructor
-		 */
-		public DescriptorImpl() {
-			super();
-		}
-
-		public DescriptorImpl(Class<? extends SecurityRealm> clazz) {
-			super(clazz);
-		}
-
 		/**
 		 * Validate keycloakJson
 		 * 
@@ -399,46 +402,25 @@ public DescriptorImpl(Class<? extends SecurityRealm> clazz) {
 		*/
 		public FormValidation doCheckKeycloakJson(@QueryParameter String value) throws ServletException {
 			try {
-				if (value != null && !value.isEmpty()) {
+				if (StringUtils.isNotEmpty(value)) {
 					JsonSerialization.readValue(value, AdapterConfig.class);
+				} else {
+					return FormValidation.error("Keycloak JSON is required.");
 				}
 			} catch (IOException ex) {
-				return FormValidation.error("Invalid adapter config");
+				return FormValidation.error("Issue parsing keycloak adapter json. JSON does not appear valid.");
 			}
 			return FormValidation.ok();
 		}
 
 		@Override
-		public boolean configure(StaplerRequest req, JSONObject json) throws hudson.model.Descriptor.FormException {
-			json = json.getJSONObject("keycloak");
-			// if json contains keycloakvalidate then keycloakvalidate is true
-			if (json.containsKey("keycloakValidate")) {
-				LOGGER.log(Level.FINE, "Keycloakvalidate set to true");
-				json.put("keycloakValidate", true);
-				JSONObject validate = json.getJSONObject("keycloakValidate");
-				if (validate.containsKey("keycloakRespectAccessTokenTimeout")) {
-					json.put("keycloakRespectAccessTokenTimeout", validate.getBoolean("keycloakRespectAccessTokenTimeout"));
-					LOGGER.log(Level.FINE, "Respect access token timeout is set to " + validate.getBoolean("keycloakRespectAccessTokenTimeout"));
-				}
-			} else {
-				json.put("keycloakValidate", false);
-				json.put("keycloakRespectAccessTokenTimeout", true);
+		public SecurityRealm newInstance(StaplerRequest request, JSONObject formData) throws FormException {
+			JSONObject keycloakJson = formData.getJSONObject("keycloak").getJSONObject("keycloakJson");
+			if (keycloakJson.isNullObject() || keycloakJson.isEmpty()) {
+				throw new Descriptor.FormException("Keycloak JSON is required.", "keycloakJson");
 			}
-			return super.configure(req, json);
+			return super.newInstance(request, formData);
 		}
-
-		@Restricted(NoExternalUse.class) // Only for loading in from legacy disk
-		@Deprecated
-		public transient String keycloakJson = "";
-		@Restricted(NoExternalUse.class) // Only for loading in from legacy disk
-		@Deprecated
-		public transient String keycloakIdp = "";
-		@Restricted(NoExternalUse.class) // Only for loading in from legacy disk
-		@Deprecated
-		public transient boolean keycloakValidate = false;
-		@Restricted(NoExternalUse.class) // Only for loading in from legacy disk
-		@Deprecated
-		public transient boolean keycloakRespectAccessTokenTimeout = true;
 	}
 
 	/**
@@ -456,7 +438,6 @@ public String getKeycloakJson() {
 	 * @param keycloakJson
 	 *            the configuration
 	 */
-	@DataBoundSetter
 	public void setKeycloakJson(String keycloakJson) {
 		this.keycloakJson = keycloakJson;
 	}
@@ -477,7 +458,6 @@ public boolean isKeycloakValidate() {
 	 * @param keycloakValidate
 	 *            {@link Boolean} if true authentication is checked on each request
 	 */
-	@DataBoundSetter
 	public void setKeycloakValidate(boolean keycloakValidate) {
 		this.keycloakValidate = keycloakValidate;
 	}
@@ -499,7 +479,6 @@ public boolean isKeycloakRespectAccessTokenTimeout() {
 	 *            {@link Boolean} whether the expiration of the access token should
 	 *            be checked or not before a token refresh
 	 */
-	@DataBoundSetter
 	public void setKeycloakRespectAccessTokenTimeout(boolean keycloakRespectAccessTokenTimeout) {
 		this.keycloakRespectAccessTokenTimeout = keycloakRespectAccessTokenTimeout;
 	}
@@ -519,7 +498,6 @@ public String getKeycloakIdp() {
 	 * @param keycloakIdp {@link String} the keycloak idp hint
 	 *
 	 */
-	@DataBoundSetter
 	public void setKeycloakIdp(String keycloakIdp) {
 		this.keycloakIdp = keycloakIdp;
 	}
@@ -670,19 +648,4 @@ public X509Certificate[] getCertificateChain() {
 		}
 	}
 
-	private Object readResolve() {
-		if (Strings.isNullOrEmpty(this.keycloakJson)) {
-			getDescriptor().load();
-			DescriptorImpl descriptor = ((DescriptorImpl) getDescriptor());
-			System.out.println("Moo -- " + descriptor.keycloakJson);
-			if (!Strings.isNullOrEmpty(descriptor.keycloakJson)) {
-				this.keycloakJson = descriptor.keycloakJson;
-				this.keycloakIdp = descriptor.keycloakIdp;
-				this.keycloakValidate = descriptor.keycloakValidate;
-				this.keycloakRespectAccessTokenTimeout = descriptor.keycloakRespectAccessTokenTimeout;
-			}
-		}
-		return this;
-	}
-
 }
diff --git a/src/main/resources/org/jenkinsci/plugins/KeycloakSecurityRealm/config.jelly b/src/main/resources/org/jenkinsci/plugins/KeycloakSecurityRealm/config.jelly
@@ -1,21 +1,16 @@
-<!--
-
--->
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
-	<f:section title="Global Keycloak Settings" name="keycloak" >
-		<f:entry title="Keycloak JSON"  field="keycloakJson" help="/plugin/keycloak/help/auth/keycloak-json-help.html">
-			<f:textarea />
+	<f:entry title="Keycloak JSON"  field="keycloakJson" help="/plugin/keycloak/help/auth/keycloak-json-help.html">
+		<f:textarea />
+	</f:entry>
+
+	<f:optionalBlock title="Validate Token on each request"  field="keycloakValidate" help="/plugin/keycloak/help/auth/keycloak-validate-help.html" inline="true">
+		<f:entry title="Keep login session open until access token times out?"  field="keycloakRespectAccessTokenTimeout" help="/plugin/keycloak/help/auth/keycloak-respectAccessTokenTimeout-help.html">
+			<f:checkbox/>
 		</f:entry>
-
-		<f:optionalBlock title="Validate Token on each request"  field="keycloakValidate" help="/plugin/keycloak/help/auth/keycloak-validate-help.html">
-			<f:entry title="Keep login session open until access token times out?"  field="keycloakRespectAccessTokenTimeout" help="/plugin/keycloak/help/auth/keycloak-respectAccessTokenTimeout-help.html">
-				<f:checkbox/>
-			</f:entry>
-		</f:optionalBlock>
-
-        <f:entry title="Use default IDP"  field="keycloakIdp" help="/plugin/keycloak/help/auth/keycloak-idp-help.html">
-            <f:textbox/>
-        </f:entry>
-	</f:section>
+	</f:optionalBlock>
+
+	<f:entry title="Use default IDP"  field="keycloakIdp" help="/plugin/keycloak/help/auth/keycloak-idp-help.html">
+		<f:textbox/>
+	</f:entry>
 </j:jelly>
diff --git a/src/test/java/org/jenkinsci/plugins/KeycloakSecurityRealmTest.java b/src/test/java/org/jenkinsci/plugins/KeycloakSecurityRealmTest.java
@@ -39,7 +39,7 @@ public void configure_keycloak() {
     public void export_casc_keycloak() throws Exception {
         KeycloakSecurityRealm ksr = new KeycloakSecurityRealm();
         ksr.setKeycloakJson("{\"realm\": \"master\",\"auth-server-url\": \"https://keycloak.example.com/auth/\",\"ssl-required\": \"external\",\"resource\": \"ci-example-com\",\"credentials\": {\"secret\": \"secret-secret-secret\"},\"confidential-port\": 0}");
-        Jenkins.getInstance().setSecurityRealm(ksr);
+		Jenkins.getInstanceOrNull().setSecurityRealm(ksr);
 
         ConfiguratorRegistry registry = ConfiguratorRegistry.get();
         ConfigurationContext context = new ConfigurationContext(registry);

diff --git a/src/test/resources/org/jenkinsci/plugins/KeycloakYamlExport.yaml b/src/test/resources/org/jenkinsci/plugins/KeycloakYamlExport.yaml
@@ -1,3 +1,5 @@
 keycloakJson: "{\"realm\": \"master\",\"auth-server-url\": \"https://keycloak.example.com/auth/\"\
   ,\"ssl-required\": \"external\",\"resource\": \"ci-example-com\",\"credentials\"\
   : {\"secret\": \"secret-secret-secret\"},\"confidential-port\": 0}"
+keycloakRespectAccessTokenTimeout: true
+keycloakValidate: false