[JENKINS-4478] - Move TCP port out from under security #2900

Merged
merged 1 commit into from Jun 3, 2017

Conversation

5 participants
@ksenia-nenasheva
Contributor

ksenia-nenasheva commented May 25, 2017

Description

See JENKINS-4478.

Details:

  • Moved Agent section out from security checkbox
  • Added sections for "Markup Formatter" and "CSRF Protection"

2017-05-25 22 11 45

Changelog entries

Proposed changelog entries:

  • Entry 1: Move TCP port out from under security
@daniel-beck

This is so great, and many years overdue. Thanks for taking this on!

Has this been tested manually or via ATH to make sure no UI/form weirdness results?

@ksenia-nenasheva

This comment has been minimized.

Show comment
Hide comment
@ksenia-nenasheva

ksenia-nenasheva May 29, 2017

Contributor

It has been tested manually.

Contributor

ksenia-nenasheva commented May 29, 2017

It has been tested manually.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 29, 2017

Member

I would expect the change to break some ATH tests and its configuration engine. But it should not be a big deal afterwards, especially since we have plenty of time before the next LTS baseline

Member

oleg-nenashev commented May 29, 2017

I would expect the change to break some ATH tests and its configuration engine. But it should not be a big deal afterwards, especially since we have plenty of time before the next LTS baseline

@recena

This comment has been minimized.

Show comment
Hide comment
@recena

recena May 31, 2017

Contributor

@ksenia-nenasheva Please, for UI changes, provides a screenshot BEFORE / AFTER the changes.

Contributor

recena commented May 31, 2017

@ksenia-nenasheva Please, for UI changes, provides a screenshot BEFORE / AFTER the changes.

+
+ f.entry(title:_("Access Control")) {
+ table(style:"width:100%") {
+ f.descriptorRadioList(title:_("Security Realm"),varName:"realm", instance:app.securityRealm, descriptors:SecurityRealm.all())

This comment has been minimized.

@recena

recena May 31, 2017

Contributor

I don't understand this code formatting, why not?

f.descriptorRadioList(title: _("Security Realm"), varName: "realm", instance: app.securityRealm, descriptors:SecurityRealm.all())
@recena

recena May 31, 2017

Contributor

I don't understand this code formatting, why not?

f.descriptorRadioList(title: _("Security Realm"), varName: "realm", instance: app.securityRealm, descriptors:SecurityRealm.all())

This comment has been minimized.

@daniel-beck

daniel-beck May 31, 2017

Member

@recena Note that is was this weird before, and @ksenia-nenasheva just didn't change it.

@daniel-beck

daniel-beck May 31, 2017

Member

@recena Note that is was this weird before, and @ksenia-nenasheva just didn't change it.

This comment has been minimized.

@recena

recena Jun 1, 2017

Contributor

@daniel-beck The diff says that those lines were modified...

@recena

recena Jun 1, 2017

Contributor

@daniel-beck The diff says that those lines were modified...

This comment has been minimized.

@daniel-beck

daniel-beck Jun 1, 2017

Member

@recena The lines 67-76 were moved to 29-38 without modification.

Hence the answer for your question…

I don't understand this code formatting, why not [this other way]?

… is: "It has been that way before and left unchanged".

@daniel-beck

daniel-beck Jun 1, 2017

Member

@recena The lines 67-76 were moved to 29-38 without modification.

Hence the answer for your question…

I don't understand this code formatting, why not [this other way]?

… is: "It has been that way before and left unchanged".

This comment has been minimized.

@recena

recena Jun 1, 2017

Contributor

@daniel-beck If you move a block of code, and you can apply a better code-style format....why not? the diff will be as complex 😄

@recena

recena Jun 1, 2017

Contributor

@daniel-beck If you move a block of code, and you can apply a better code-style format....why not? the diff will be as complex 😄

This comment has been minimized.

@daniel-beck

daniel-beck Jun 1, 2017

Member

@recena Not saying it shouldn't have been done (it probably should, but not actively making it worse isn't reason to reject), but you asked "Why not use sane formatting?" and I provided the answer.

@daniel-beck

daniel-beck Jun 1, 2017

Member

@recena Not saying it shouldn't have been done (it probably should, but not actively making it worse isn't reason to reject), but you asked "Why not use sane formatting?" and I provided the answer.

+ }
+
+ f.entry(title:_("Access Control")) {
+ table(style:"width:100%") {

This comment has been minimized.

@recena

recena May 31, 2017

Contributor

Bad idea to use inline CSS style but certainly Jenkins project is full of them 😢

@recena

recena May 31, 2017

Contributor

Bad idea to use inline CSS style but certainly Jenkins project is full of them 😢

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 31, 2017

Member

I agree. Tables are also not the best practice. But it is a moved code anyway

@oleg-nenashev

oleg-nenashev May 31, 2017

Member

I agree. Tables are also not the best practice. But it is a moved code anyway

This comment has been minimized.

@recena

recena Jun 1, 2017

Contributor

@oleg-nenashev Tables are not good when are using in a wrong way 😄

@recena

recena Jun 1, 2017

Contributor

@oleg-nenashev Tables are not good when are using in a wrong way 😄

@recena

This comment has been minimized.

Show comment
Hide comment
@recena

recena May 31, 2017

Contributor

The UI change LGTM

Contributor

recena commented May 31, 2017

The UI change LGTM

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 31, 2017

Member

Does this fix JENKINS-40228?

Member

jglick commented May 31, 2017

Does this fix JENKINS-40228?

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 31, 2017

Member

Why not just get rid of the useSecurity checkbox altogether and have all the controls at top level? As of 2.0, security is enabled by default—you need to go out of your way to disable it. If you do so, you will just have selected some other insecure options for the security realm and authorization strategy; the checkbox adds no value that I can see.

Member

jglick commented May 31, 2017

Why not just get rid of the useSecurity checkbox altogether and have all the controls at top level? As of 2.0, security is enabled by default—you need to go out of your way to disable it. If you do so, you will just have selected some other insecure options for the security realm and authorization strategy; the checkbox adds no value that I can see.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Jun 1, 2017

Member

Why not just get rid of the useSecurity checkbox altogether and have all the controls at top level?

I would expect that to not be that simple. For example, people skipping the wizard and getting no security by default (e.g. in Puppet managed environments). Additionally, we may need to give SecurityRealm.NO_AUTHENTICATION an @Extension, and possibly adapt authn/authz option related methods in Jenkins (e.g. Jenkins#disableSecurity()).

Therefore I would prefer to log this as an issue, and leave it out of this PR.

Member

daniel-beck commented Jun 1, 2017

Why not just get rid of the useSecurity checkbox altogether and have all the controls at top level?

I would expect that to not be that simple. For example, people skipping the wizard and getting no security by default (e.g. in Puppet managed environments). Additionally, we may need to give SecurityRealm.NO_AUTHENTICATION an @Extension, and possibly adapt authn/authz option related methods in Jenkins (e.g. Jenkins#disableSecurity()).

Therefore I would prefer to log this as an issue, and leave it out of this PR.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jun 1, 2017

Member

@daniel-beck is it really that difficult? From a quick attempt,

diff --git a/core/src/main/java/hudson/security/GlobalSecurityConfiguration.java b/core/src/main/java/hudson/security/GlobalSecurityConfiguration.java
index b9c6fee0de..9fb74c7344 100644
--- a/core/src/main/java/hudson/security/GlobalSecurityConfiguration.java
+++ b/core/src/main/java/hudson/security/GlobalSecurityConfiguration.java
@@ -111,8 +111,7 @@ public class GlobalSecurityConfiguration extends ManagementLink implements Descr
         // for compatibility reasons, the actual value is stored in Jenkins
         Jenkins j = Jenkins.getInstance();
         j.checkPermission(Jenkins.ADMINISTER);
-        if (json.has("useSecurity")) {
-            JSONObject security = json.getJSONObject("useSecurity");
+            JSONObject security = json; // TODO inline
             j.setDisableRememberMe(security.optBoolean("disableRememberMe", false));
             j.setSecurityRealm(SecurityRealm.all().newInstanceFromRadioList(security, "realm"));
             j.setAuthorizationStrategy(AuthorizationStrategy.all().newInstanceFromRadioList(security, "authorization"));
@@ -135,9 +134,6 @@ public class GlobalSecurityConfiguration extends ManagementLink implements Descr
                 }
             }
             j.setAgentProtocols(agentProtocols);
-        } else {
-            j.disableSecurity();
-        }
 
         if (json.has("markupFormatter")) {
             j.setMarkupFormatter(req.bindJSON(MarkupFormatter.class, json.getJSONObject("markupFormatter")));
diff --git a/core/src/main/java/hudson/security/SecurityRealm.java b/core/src/main/java/hudson/security/SecurityRealm.java
index 6cb69906e0..bd6e5fabd8 100644
--- a/core/src/main/java/hudson/security/SecurityRealm.java
+++ b/core/src/main/java/hudson/security/SecurityRealm.java
@@ -69,6 +69,8 @@ import java.io.UnsupportedEncodingException;
 import java.util.List;
 import java.util.Map;
 import java.util.logging.Logger;
+import net.sf.json.JSONObject;
+import org.jenkinsci.Symbol;
 
 /**
  * Pluggable security realm that connects external user database to Hudson.
@@ -550,15 +552,6 @@ public abstract class SecurityRealm extends AbstractDescribableImpl<SecurityReal
         }
 
         /**
-         * This special instance is not configurable explicitly,
-         * so it doesn't have a descriptor.
-         */
-        @Override
-        public Descriptor<SecurityRealm> getDescriptor() {
-            return null;
-        }
-
-        /**
          * There's no group.
          */
         @Override
@@ -580,6 +573,23 @@ public abstract class SecurityRealm extends AbstractDescribableImpl<SecurityReal
         private Object readResolve() {
             return NO_AUTHENTICATION;
         }
+
+        @Extension
+        @Symbol("none")
+        public static class DescriptorImpl extends Descriptor<SecurityRealm> {
+
+            @Override
+            public String getDisplayName() {
+                return "None"; // TODO I18N
+            }
+
+            @Override
+            public SecurityRealm newInstance(StaplerRequest req, JSONObject formData) throws FormException {
+                return NO_AUTHENTICATION;
+            }
+
+        }
+
     }
 
     /**
diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
index f187137018..57fc1afec6 100644
--- a/core/src/main/java/jenkins/model/Jenkins.java
+++ b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2585,6 +2585,7 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
         this.disableRememberMe = disableRememberMe;
     }
 
+    @Deprecated
     public void disableSecurity() {
         useSecurity = null;
         setSecurityRealm(SecurityRealm.NO_AUTHENTICATION);
diff --git a/core/src/main/resources/hudson/security/GlobalSecurityConfiguration/index.groovy b/core/src/main/resources/hudson/security/GlobalSecurityConfiguration/index.groovy
index f082a3fce5..913d208583 100644
--- a/core/src/main/resources/hudson/security/GlobalSecurityConfiguration/index.groovy
+++ b/core/src/main/resources/hudson/security/GlobalSecurityConfiguration/index.groovy
@@ -25,7 +25,6 @@ l.layout(norefresh:true, permission:app.ADMINISTER, title:my.displayName, csscla
             set("instance",my);
             set("descriptor", my.descriptor);
 
-            f.optionalBlock( field:"useSecurity", title:_("Enable security"), checked:app.useSecurity) {
                 f.entry(title: _("TCP port for JNLP agents"), field: "slaveAgentPort") {
                     if (my.slaveAgentPortEnforced) {
                         if (my.slaveAgentPort == -1) {
@@ -74,7 +73,6 @@ l.layout(norefresh:true, permission:app.ADMINISTER, title:my.displayName, csscla
                         f.descriptorRadioList(title:_("Authorization"), varName:"authorization", instance:app.authorizationStrategy, descriptors:AuthorizationStrategy.all())
                     }
                 }
-            }
 
             f.dropdownDescriptorSelector(title:_("Markup Formatter"),descriptors: MarkupFormatterDescriptor.all(), field: 'markupFormatter')
 

seems to work.

Member

jglick commented Jun 1, 2017

@daniel-beck is it really that difficult? From a quick attempt,

diff --git a/core/src/main/java/hudson/security/GlobalSecurityConfiguration.java b/core/src/main/java/hudson/security/GlobalSecurityConfiguration.java
index b9c6fee0de..9fb74c7344 100644
--- a/core/src/main/java/hudson/security/GlobalSecurityConfiguration.java
+++ b/core/src/main/java/hudson/security/GlobalSecurityConfiguration.java
@@ -111,8 +111,7 @@ public class GlobalSecurityConfiguration extends ManagementLink implements Descr
         // for compatibility reasons, the actual value is stored in Jenkins
         Jenkins j = Jenkins.getInstance();
         j.checkPermission(Jenkins.ADMINISTER);
-        if (json.has("useSecurity")) {
-            JSONObject security = json.getJSONObject("useSecurity");
+            JSONObject security = json; // TODO inline
             j.setDisableRememberMe(security.optBoolean("disableRememberMe", false));
             j.setSecurityRealm(SecurityRealm.all().newInstanceFromRadioList(security, "realm"));
             j.setAuthorizationStrategy(AuthorizationStrategy.all().newInstanceFromRadioList(security, "authorization"));
@@ -135,9 +134,6 @@ public class GlobalSecurityConfiguration extends ManagementLink implements Descr
                 }
             }
             j.setAgentProtocols(agentProtocols);
-        } else {
-            j.disableSecurity();
-        }
 
         if (json.has("markupFormatter")) {
             j.setMarkupFormatter(req.bindJSON(MarkupFormatter.class, json.getJSONObject("markupFormatter")));
diff --git a/core/src/main/java/hudson/security/SecurityRealm.java b/core/src/main/java/hudson/security/SecurityRealm.java
index 6cb69906e0..bd6e5fabd8 100644
--- a/core/src/main/java/hudson/security/SecurityRealm.java
+++ b/core/src/main/java/hudson/security/SecurityRealm.java
@@ -69,6 +69,8 @@ import java.io.UnsupportedEncodingException;
 import java.util.List;
 import java.util.Map;
 import java.util.logging.Logger;
+import net.sf.json.JSONObject;
+import org.jenkinsci.Symbol;
 
 /**
  * Pluggable security realm that connects external user database to Hudson.
@@ -550,15 +552,6 @@ public abstract class SecurityRealm extends AbstractDescribableImpl<SecurityReal
         }
 
         /**
-         * This special instance is not configurable explicitly,
-         * so it doesn't have a descriptor.
-         */
-        @Override
-        public Descriptor<SecurityRealm> getDescriptor() {
-            return null;
-        }
-
-        /**
          * There's no group.
          */
         @Override
@@ -580,6 +573,23 @@ public abstract class SecurityRealm extends AbstractDescribableImpl<SecurityReal
         private Object readResolve() {
             return NO_AUTHENTICATION;
         }
+
+        @Extension
+        @Symbol("none")
+        public static class DescriptorImpl extends Descriptor<SecurityRealm> {
+
+            @Override
+            public String getDisplayName() {
+                return "None"; // TODO I18N
+            }
+
+            @Override
+            public SecurityRealm newInstance(StaplerRequest req, JSONObject formData) throws FormException {
+                return NO_AUTHENTICATION;
+            }
+
+        }
+
     }
 
     /**
diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
index f187137018..57fc1afec6 100644
--- a/core/src/main/java/jenkins/model/Jenkins.java
+++ b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2585,6 +2585,7 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
         this.disableRememberMe = disableRememberMe;
     }
 
+    @Deprecated
     public void disableSecurity() {
         useSecurity = null;
         setSecurityRealm(SecurityRealm.NO_AUTHENTICATION);
diff --git a/core/src/main/resources/hudson/security/GlobalSecurityConfiguration/index.groovy b/core/src/main/resources/hudson/security/GlobalSecurityConfiguration/index.groovy
index f082a3fce5..913d208583 100644
--- a/core/src/main/resources/hudson/security/GlobalSecurityConfiguration/index.groovy
+++ b/core/src/main/resources/hudson/security/GlobalSecurityConfiguration/index.groovy
@@ -25,7 +25,6 @@ l.layout(norefresh:true, permission:app.ADMINISTER, title:my.displayName, csscla
             set("instance",my);
             set("descriptor", my.descriptor);
 
-            f.optionalBlock( field:"useSecurity", title:_("Enable security"), checked:app.useSecurity) {
                 f.entry(title: _("TCP port for JNLP agents"), field: "slaveAgentPort") {
                     if (my.slaveAgentPortEnforced) {
                         if (my.slaveAgentPort == -1) {
@@ -74,7 +73,6 @@ l.layout(norefresh:true, permission:app.ADMINISTER, title:my.displayName, csscla
                         f.descriptorRadioList(title:_("Authorization"), varName:"authorization", instance:app.authorizationStrategy, descriptors:AuthorizationStrategy.all())
                     }
                 }
-            }
 
             f.dropdownDescriptorSelector(title:_("Markup Formatter"),descriptors: MarkupFormatterDescriptor.all(), field: 'markupFormatter')
 

seems to work.

@ksenia-nenasheva

This comment has been minimized.

Show comment
Hide comment
@ksenia-nenasheva

ksenia-nenasheva Jun 2, 2017

Contributor

Before:

  • Disable security:

2017-06-02 8 15 09

  • Enable security:

2017-06-02 8 14 42

After:
2017-05-25 22 11 45

Contributor

ksenia-nenasheva commented Jun 2, 2017

Before:

  • Disable security:

2017-06-02 8 15 09

  • Enable security:

2017-06-02 8 14 42

After:
2017-05-25 22 11 45

@ksenia-nenasheva

This comment has been minimized.

Show comment
Hide comment
@ksenia-nenasheva

ksenia-nenasheva Jun 2, 2017

Contributor

Hi @daniel-beck, @jglick
Yes, I could work to it. But I am not sure that it should be in this PR. I prefer different PRs for different bugs.
Could you please make a decision?

Contributor

ksenia-nenasheva commented Jun 2, 2017

Hi @daniel-beck, @jglick
Yes, I could work to it. But I am not sure that it should be in this PR. I prefer different PRs for different bugs.
Could you please make a decision?

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Jun 2, 2017

Member

@jglick @ksenia-nenasheva I would also prefer if the scope of this PR not be extended. This has merit with or without a followup change due to new section headers for JNLP port and CSRF protection.

Member

daniel-beck commented Jun 2, 2017

@jglick @ksenia-nenasheva I would also prefer if the scope of this PR not be extended. This has merit with or without a followup change due to new section headers for JNLP port and CSRF protection.

@oleg-nenashev

I agree with the merge in the current state. I am not 100% sure it qualifies for backporting (if somebody submits the manually-generated form in his configuration-as-code engine), but the change addresses what is required in JENKINS-4478.

After the merge we should reopen https://issues.jenkins-ci.org/browse/JENKINS-40228, which is not a full duplicate.

@daniel-beck daniel-beck merged commit 2228b39 into jenkinsci:master Jun 3, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment