checkstyle security issue - checking for protected classes

hbyk3344 · Dec 11, 2014 · 48879cc · 48879cc
1 parent f24ebde
commit 48879cc
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 34 deletions.
diff --git a/...app/src/test/java/org/jasig/cas/services/web/support/RegisteredServiceValidatorTests.java b/...app/src/test/java/org/jasig/cas/services/web/support/RegisteredServiceValidatorTests.java
@@ -18,16 +18,6 @@
  */
 package org.jasig.cas.services.web.support;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
 import org.jasig.cas.authentication.principal.Service;
 import org.jasig.cas.services.RegisteredService;
 import org.jasig.cas.services.RegisteredServiceImpl;
@@ -38,6 +28,14 @@
 import org.springframework.validation.BindException;
 import org.springframework.validation.Validator;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.junit.Assert.*;
+
 /**
  *
  * @author Scott Battaglia
@@ -117,7 +115,7 @@ protected void checkId(final boolean exists, final int expectedErrors, final Str
 
     }
 
-    protected class TestServicesManager implements ServicesManager {
+    private static class TestServicesManager implements ServicesManager {
 
         private final boolean returnValue;
 

diff --git a/cas-server-core/src/test/java/org/jasig/cas/services/DefaultServicesManagerImplTests.java b/cas-server-core/src/test/java/org/jasig/cas/services/DefaultServicesManagerImplTests.java
@@ -18,15 +18,15 @@
  */
 package org.jasig.cas.services;
 
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
-
 import org.jasig.cas.authentication.principal.Principal;
 import org.jasig.cas.authentication.principal.Service;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+
 import static org.junit.Assert.*;
 
 /**
@@ -211,7 +211,7 @@ public void verifyEvaluationOrderOfServices() {
 
     }
 
-    protected class SimpleService implements Service {
+    private static class SimpleService implements Service {
 
         /**
          * Comment for <code>serialVersionUID</code>.

diff --git a/...rver-core/src/test/java/org/jasig/cas/ticket/registry/DistributedTicketRegistryTests.java b/...rver-core/src/test/java/org/jasig/cas/ticket/registry/DistributedTicketRegistryTests.java
@@ -39,15 +39,19 @@
  * @since 3.1
  *
  */
-public class DistributedTicketRegistryTests {
+public final class DistributedTicketRegistryTests {
 
     private TestDistributedTicketRegistry ticketRegistry;
 
     private boolean wasTicketUpdated;
 
+    public void setWasTicketUpdated(final boolean wasTicketUpdated) {
+        this.wasTicketUpdated = wasTicketUpdated;
+    }
+
     @Before
     public void setUp() throws Exception {
-        this.ticketRegistry = new TestDistributedTicketRegistry();
+        this.ticketRegistry = new TestDistributedTicketRegistry(this);
         this.wasTicketUpdated = false;
     }
 
@@ -114,12 +118,16 @@ public void verifyTicketDoesntExist() {
         assertNull(this.ticketRegistry.getTicket("fdfas"));
     }
 
-    protected class TestDistributedTicketRegistry extends AbstractDistributedTicketRegistry {
-
+    private static class TestDistributedTicketRegistry extends AbstractDistributedTicketRegistry {
+        private final DistributedTicketRegistryTests parent;
         private Map<String, Ticket> tickets = new HashMap<String, Ticket>();
 
+        public TestDistributedTicketRegistry(final DistributedTicketRegistryTests parent) {
+            this.parent = parent;
+        }
+
         protected void updateTicket(final Ticket ticket) {
-            DistributedTicketRegistryTests.this.wasTicketUpdated = true;
+            this.parent.setWasTicketUpdated(true);
         }
 
         public void addTicket(final Ticket ticket) {

diff --git a/...a/org/jasig/cas/support/saml/authentication/SamlAuthenticationMetaDataPopulatorTests.java b/...a/org/jasig/cas/support/saml/authentication/SamlAuthenticationMetaDataPopulatorTests.java
@@ -18,25 +18,26 @@
  */
 package org.jasig.cas.support.saml.authentication;
 
-import static org.junit.Assert.*;
-
-import java.util.HashMap;
-import java.util.Map;
-
 import org.jasig.cas.TestUtils;
 import org.jasig.cas.authentication.Authentication;
 import org.jasig.cas.authentication.AuthenticationBuilder;
 import org.jasig.cas.authentication.AuthenticationHandler;
 import org.jasig.cas.authentication.BasicCredentialMetaData;
+import org.jasig.cas.authentication.Credential;
 import org.jasig.cas.authentication.CredentialMetaData;
 import org.jasig.cas.authentication.HandlerResult;
 import org.jasig.cas.authentication.UsernamePasswordCredential;
 import org.jasig.cas.authentication.handler.support.SimpleTestUsernamePasswordAuthenticationHandler;
-import org.jasig.cas.authentication.Credential;
 import org.jasig.cas.authentication.principal.Principal;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
 /**
  * @author Scott Battaglia
  * @since 3.1
@@ -91,7 +92,7 @@ public void verifyAuthenticationTypeFoundCustom() {
                 auth.getAttributes().get(SamlAuthenticationMetaDataPopulator.ATTRIBUTE_AUTHENTICATION_METHOD));
     }
 
-    protected class CustomCredential implements Credential {
+    private static class CustomCredential implements Credential {
 
         public String getId() {
             return "nobody";

diff --git a/.../jasig/cas/adaptors/x509/authentication/handler/support/ResourceCRLRevocationChecker.java b/.../jasig/cas/adaptors/x509/authentication/handler/support/ResourceCRLRevocationChecker.java
@@ -81,7 +81,7 @@ public ResourceCRLRevocationChecker(final Resource crl) {
      * at least one non-null element.
      */
     public ResourceCRLRevocationChecker(final Resource[] crls) {
-        this.fetcher = new CRLFetcher(crls);
+        this.fetcher = new CRLFetcher(crls, this);
     }
 
     /**
@@ -153,7 +153,8 @@ protected void finalize() throws Throwable {
     /**
      * Handles details of fetching CRL data from resources.
      */
-    protected class CRLFetcher {
+    private static class CRLFetcher {
+        private final ResourceCRLRevocationChecker crlRevocationChecker;
         /** Logger instance. */
         private final Logger logger = LoggerFactory.getLogger(getClass());
 
@@ -165,8 +166,9 @@ protected class CRLFetcher {
          *
          * @param crls Resources containing CRL data.  MUST NOT be null and MUST have
          * at least one non-null element.
+         * @param crlRevocationChecker the crl revocation checker
          */
-        public CRLFetcher(final Resource[] crls) {
+        public CRLFetcher(final Resource[] crls, final ResourceCRLRevocationChecker crlRevocationChecker) {
             if (crls == null) {
                 throw new IllegalArgumentException("CRL resources cannot be null.");
             }
@@ -179,6 +181,7 @@ public CRLFetcher(final Resource[] crls) {
             if (this.resources.isEmpty()) {
                 throw new IllegalArgumentException("Must provide at least one non-null CRL resource.");
             }
+            this.crlRevocationChecker = crlRevocationChecker;
         }
 
         /**
@@ -191,7 +194,7 @@ public void fetch(final boolean throwOnError) {
             for (Resource r : this.resources) {
                 logger.debug("Fetching CRL data from {}", r);
                 try {
-                    addCrl(CertUtils.fetchCRL(r));
+                    this.crlRevocationChecker.addCrl(CertUtils.fetchCRL(r));
                 } catch (final Exception e) {
                     if (throwOnError) {
                         throw new RuntimeException("Error fetching CRL from " + r, e);

diff --git a/checkstyle-rules.xml b/checkstyle-rules.xml
@@ -348,8 +348,8 @@ Checkstyle configuration based on Sun's conventions, compliant with CAS coding c
   <module name="RegexpSingleline">
     <metadata name="net.sf.eclipsecs.core.comment" value="Non-static inner class"/>
     <property name="severity" value="error"/>
-    <property name="format" value="\s+(private|public)*\s+(abstract\s)*class\s+\w+"/>
-    <property name="message" value="Non-static inner classes are a security compromise. Consider static instead"/>
+    <property name="format" value="\s+(private|public|protected)*\s+(abstract\s)*class\s+\w+"/>
+    <property name="message" value="Non-static nested classes are a security compromise. Consider using a static class instead"/>
   </module>
   <module name="RegexpSingleline">
     <metadata name="net.sf.eclipsecs.core.comment" value="Usage of java.util.Random"/>