From 8c380f35e2912eed0a7c618a287007cd1a428dff Mon Sep 17 00:00:00 2001 From: Steve Arch Date: Mon, 1 Oct 2018 13:39:13 +0100 Subject: [PATCH 1/9] typo --- .../src/main/java/org/kohsuke/accmod/AccessRestriction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/access-modifier-annotation/src/main/java/org/kohsuke/accmod/AccessRestriction.java b/access-modifier-annotation/src/main/java/org/kohsuke/accmod/AccessRestriction.java index 1f9fa6a..1509196 100644 --- a/access-modifier-annotation/src/main/java/org/kohsuke/accmod/AccessRestriction.java +++ b/access-modifier-annotation/src/main/java/org/kohsuke/accmod/AccessRestriction.java @@ -38,7 +38,7 @@ *

* Single execution of the enforcement check would create at most one instance * of a given {@link AccessRestriction} type, so instance fields can be used to store - * heavy-weight objects or other indicies that you might need for implementing + * heavy-weight objects or other indices that you might need for implementing * access control checks. * * @author Kohsuke Kawaguchi From c8cee8aadcfe2f7e268a6d5d7ed670aa8c33f9db Mon Sep 17 00:00:00 2001 From: Steve Arch Date: Tue, 2 Oct 2018 10:31:03 +0100 Subject: [PATCH 2/9] JENKINS-53675 - extracted anonymous inner classes into proper classes --- .../java/org/kohsuke/accmod/impl/Checker.java | 248 ++++++++++-------- 1 file changed, 132 insertions(+), 116 deletions(-) diff --git a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java index bb504d1..7e1c6a5 100644 --- a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java +++ b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java @@ -228,122 +228,7 @@ public void checkClass(File clazz) throws IOException { FileInputStream in = new FileInputStream(clazz); try { ClassReader cr = new ClassReader(in); - cr.accept(new ClassVisitor(Opcodes.ASM5) { - private String className; - private String methodName,methodDesc; - private int line; - - @Override - public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) { - this.className = name; - - if (isSynthetic(access)) { - return; - } - - if (superName != null) { - for (Restrictions r : getRestrictions(superName)) { - r.usedAsSuperType(currentLocation, errorListener); - } - } - - if (interfaces != null) { - for (String intf : interfaces) { - for (Restrictions r : getRestrictions(intf)) { - r.usedAsInterface(currentLocation, errorListener); - } - } - } - } - - @Override - public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) { - this.methodName = name; - this.methodDesc = desc; - - if (isSynthetic(access)) { - return null; - } - - return new MethodVisitor(Opcodes.ASM5) { - @Override - public void visitLineNumber(int _line, Label start) { - line = _line; - } - - public void visitTypeInsn(int opcode, String type) { - switch (opcode) { - case Opcodes.NEW: - for (Restrictions r : getRestrictions(type)) { - r.instantiated(currentLocation, errorListener); - } - } - } - - @Override - public void visitMethodInsn(int opcode, String owner, String name, String desc, boolean itf) { - for (Restrictions r : getRestrictions(owner + '.' + name + desc)) { - r.invoked(currentLocation, errorListener); - } - } - - @Override - public void visitFieldInsn(int opcode, String owner, String name, String desc) { - Iterable rs = getRestrictions(owner + '.' + name); - switch (opcode) { - case Opcodes.GETSTATIC: - case Opcodes.GETFIELD: - for (Restrictions r : rs) { - r.read(currentLocation, errorListener); - } - break; - case Opcodes.PUTSTATIC: - case Opcodes.PUTFIELD: - for (Restrictions r : rs) { - r.written(currentLocation, errorListener); - } - break; - } - super.visitFieldInsn(opcode, owner, name, desc); - } - }; - } - - - /** - * Constant that represents the current location. - */ - private final Location currentLocation = new Location() { - public String getClassName() { - return className.replace('/','.'); - } - - public String getMethodName() { - return methodName; - } - - public String getMethodDescriptor() { - return methodDesc; - } - - public int getLineNumber() { - return line; - } - - public String toString() { - return className+':'+line; - } - - public ClassLoader getDependencyClassLoader() { - return dependencies; - } - - @Override - public String getProperty(String key) { - return properties.getProperty(key); - } - }; - }, SKIP_FRAMES); + cr.accept(new MyClassVisitor(), SKIP_FRAMES); } finally { in.close(); } @@ -387,4 +272,135 @@ private Iterable getRestrictions(String keyName) { private static boolean isSynthetic(int access) { return (access & Opcodes.ACC_SYNTHETIC) != 0; } + + private class MyClassVisitor extends ClassVisitor { + private String className; + private String methodName, methodDesc; + private int line; + + public MyClassVisitor() { + super(Opcodes.ASM5); + } + + @Override + public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) { + this.className = name; + + if (isSynthetic(access)) { + return; + } + + if (superName != null) { + for (Restrictions r : getRestrictions(superName)) { + r.usedAsSuperType(currentLocation, errorListener); + } + } + + if (interfaces != null) { + for (String intf : interfaces) { + for (Restrictions r : getRestrictions(intf)) { + r.usedAsInterface(currentLocation, errorListener); + } + } + } + } + + @Override + public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) { + this.methodName = name; + this.methodDesc = desc; + + if (isSynthetic(access)) { + return null; + } + + return new MyMethodVisitor(currentLocation); + } + + + /** + * Constant that represents the current location. + */ + private final Location currentLocation = new Location() { + public String getClassName() { + return className.replace('/','.'); + } + + public String getMethodName() { + return methodName; + } + + public String getMethodDescriptor() { + return methodDesc; + } + + public int getLineNumber() { + return line; + } + + public String toString() { + return className+':'+line; + } + + public ClassLoader getDependencyClassLoader() { + return dependencies; + } + + @Override + public String getProperty(String key) { + return properties.getProperty(key); + } + }; + } + + private class MyMethodVisitor extends MethodVisitor { + + private Location currentLocation; + + public MyMethodVisitor(Location currentLocation) { + super(Opcodes.ASM5); + this.currentLocation = currentLocation; + } + +// @Override +// public void visitLineNumber(int _line, Label start) { + // line = _line; FIXME +// } + + public void visitTypeInsn(int opcode, String type) { + switch (opcode) { + case Opcodes.NEW: + for (Restrictions r : getRestrictions(type)) { + r.instantiated(currentLocation, errorListener); + } + } + } + + @Override + public void visitMethodInsn(int opcode, String owner, String name, String desc, boolean itf) { + for (Restrictions r : getRestrictions(owner + '.' + name + desc)) { + r.invoked(currentLocation, errorListener); + } + } + + @Override + public void visitFieldInsn(int opcode, String owner, String name, String desc) { + Iterable rs = getRestrictions(owner + '.' + name); + switch (opcode) { + case Opcodes.GETSTATIC: + case Opcodes.GETFIELD: + for (Restrictions r : rs) { + r.read(currentLocation, errorListener); + } + break; + case Opcodes.PUTSTATIC: + case Opcodes.PUTFIELD: + for (Restrictions r : rs) { + r.written(currentLocation, errorListener); + } + break; + } + super.visitFieldInsn(opcode, owner, name, desc); + } + } } From d55f1e1ecc51c767284d1a73ef06c887f981bb19 Mon Sep 17 00:00:00 2001 From: Steve Arch Date: Tue, 2 Oct 2018 13:49:12 +0100 Subject: [PATCH 3/9] JENKINS-53675 - Added @DisableRestriction annotation This allows you to disable @Restricted class/methods at the point of use --- .../disable/DisableRestriction.java | 56 +++++++ .../api/ApiWithRestrictedMethodAndField.java | 13 ++ .../api/src/main/java/api/RestrictedApi.java | 12 ++ .../main/java/api/RestrictedInterface.java | 8 + .../caller/src/main/java/caller/Caller.java | 36 +++++ .../caller/CallerDisabledAtClassLevel.java | 32 ++++ .../java/caller/RestrictedApiSubclass.java | 8 + .../RestrictedInterfaceImplementation.java | 8 + .../disable-restrictions/invoker.properties | 2 + .../src/it/disable-restrictions/pom.xml | 17 ++ .../java/org/kohsuke/accmod/impl/Checker.java | 147 ++++++++++++++---- .../org/kohsuke/accmod/impl/EnforcerMojo.java | 2 +- 12 files changed, 306 insertions(+), 35 deletions(-) create mode 100644 access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java create mode 100644 access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/ApiWithRestrictedMethodAndField.java create mode 100644 access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/RestrictedApi.java create mode 100644 access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/RestrictedInterface.java create mode 100644 access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/Caller.java create mode 100644 access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/CallerDisabledAtClassLevel.java create mode 100644 access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedApiSubclass.java create mode 100644 access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedInterfaceImplementation.java create mode 100644 access-modifier-checker/src/it/disable-restrictions/invoker.properties create mode 100644 access-modifier-checker/src/it/disable-restrictions/pom.xml diff --git a/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java b/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java new file mode 100644 index 0000000..37bcb15 --- /dev/null +++ b/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java @@ -0,0 +1,56 @@ +/* + * The MIT License + * + * Copyright (c) 2010, Kohsuke Kawaguchi + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package org.kohsuke.accmod.restrictions.disable; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.Target; +import org.jvnet.hudson.annotation_indexer.Indexed; +import org.kohsuke.accmod.AccessRestriction; + +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +/** + * Indicates that a particular element is really deprecated and that the access to it + * is subject to the additional restrictions. + * + *

+ * These annotations and restrictions introduced by them are enforced by the + * "access-modifier-checker" mojo. + * + * @author Kohsuke Kawaguchi + */ +@Retention(RUNTIME) +@Documented +//@Indexed +@Target({ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.TYPE}) +public @interface DisableRestriction { + /** + * Kind of access that are restricted. + * If multiple values are specified, those restrictions are OR-ed — thus if an use + * violates any of the restrictions, it'll be considered as an error. + */ + Class[] value(); +} diff --git a/access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/ApiWithRestrictedMethodAndField.java b/access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/ApiWithRestrictedMethodAndField.java new file mode 100644 index 0000000..82faef5 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/ApiWithRestrictedMethodAndField.java @@ -0,0 +1,13 @@ +package api; + +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +public class ApiWithRestrictedMethodAndField { + + @Restricted(NoExternalUse.class) + public String field; + + @Restricted(NoExternalUse.class) + public static void notReallyPublic() {} +} diff --git a/access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/RestrictedApi.java b/access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/RestrictedApi.java new file mode 100644 index 0000000..ef23039 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/RestrictedApi.java @@ -0,0 +1,12 @@ +package api; + +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +@Restricted(NoExternalUse.class) +public class RestrictedApi { + + public String field; + + public void doNotUse() {} +} diff --git a/access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/RestrictedInterface.java b/access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/RestrictedInterface.java new file mode 100644 index 0000000..c51fd14 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/api/src/main/java/api/RestrictedInterface.java @@ -0,0 +1,8 @@ +package api; + +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +@Restricted(NoExternalUse.class) +public interface RestrictedInterface { +} diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/Caller.java b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/Caller.java new file mode 100644 index 0000000..e5ac9ef --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/Caller.java @@ -0,0 +1,36 @@ +package caller; + +import api.ApiWithRestrictedMethodAndField; +import api.RestrictedApi; +import org.kohsuke.accmod.restrictions.disable.DisableRestriction; + +public class Caller extends ApiWithRestrictedMethodAndField { // This is fine, ApiWithRestrictedMethodAndField itself is not restricted + + private RestrictedApi restrictedApi; + + @DisableRestriction(ApiWithRestrictedMethodAndField.class) + public Caller() { + ApiWithRestrictedMethodAndField.notReallyPublic(); // illegal but check disabled at the method level + } + + @DisableRestriction({RestrictedApi.class, ApiWithRestrictedMethodAndField.class}) + private void invalidFieldUse() { + restrictedApi.field = null; + super.field = null; + } + + @DisableRestriction(ApiWithRestrictedMethodAndField.class) + public void callerMethod() { + ApiWithRestrictedMethodAndField.notReallyPublic(); // illegal but check disabled at the method level + } + + @DisableRestriction(RestrictedApi.class) + public void methodWithRestrictedParameter(RestrictedApi api) { + api.doNotUse(); // illegal but check disabled at the method level + } + + @DisableRestriction(RestrictedApi.class) + public RestrictedApi getRestrictedApi() { + return new RestrictedApi(); // illegal but check disabled at the method level + } +} diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/CallerDisabledAtClassLevel.java b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/CallerDisabledAtClassLevel.java new file mode 100644 index 0000000..8b4f0cb --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/CallerDisabledAtClassLevel.java @@ -0,0 +1,32 @@ +package caller; + +import api.ApiWithRestrictedMethodAndField; +import api.RestrictedApi; +import org.kohsuke.accmod.restrictions.disable.DisableRestriction; + +@DisableRestriction( {ApiWithRestrictedMethodAndField.class, RestrictedApi.class}) +public class CallerDisabledAtClassLevel extends RestrictedApi { + + private RestrictedApi restrictedApi; + + public CallerDisabledAtClassLevel() { + ApiWithRestrictedMethodAndField.notReallyPublic(); // illegal but check disabled at the class level + } + + private void invalidFieldUse() { + restrictedApi.field = null; + super.field = null; + } + + public void callerMethod() { + ApiWithRestrictedMethodAndField.notReallyPublic(); // illegal but check disabled at the class level + } + + public void methodWithRestrictedParameter(RestrictedApi api) { + api.doNotUse(); // illegal but check disabled at the class level + } + + public RestrictedApi getRestrictedApi() { + return new RestrictedApi(); // illegal but check disabled at the class level + } +} \ No newline at end of file diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedApiSubclass.java b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedApiSubclass.java new file mode 100644 index 0000000..1eea69c --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedApiSubclass.java @@ -0,0 +1,8 @@ +package caller; + +import api.RestrictedApi; +import org.kohsuke.accmod.restrictions.disable.DisableRestriction; + +@DisableRestriction(RestrictedApi.class) +public class RestrictedApiSubclass extends RestrictedApi { +} diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedInterfaceImplementation.java b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedInterfaceImplementation.java new file mode 100644 index 0000000..cffc8cd --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedInterfaceImplementation.java @@ -0,0 +1,8 @@ +package caller; + +import api.RestrictedInterface; +import org.kohsuke.accmod.restrictions.disable.DisableRestriction; + +@DisableRestriction(RestrictedInterface.class) +public class RestrictedInterfaceImplementation implements RestrictedInterface { +} diff --git a/access-modifier-checker/src/it/disable-restrictions/invoker.properties b/access-modifier-checker/src/it/disable-restrictions/invoker.properties new file mode 100644 index 0000000..5c363f7 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/invoker.properties @@ -0,0 +1,2 @@ +invoker.goals=clean package +invoker.buildResult = success diff --git a/access-modifier-checker/src/it/disable-restrictions/pom.xml b/access-modifier-checker/src/it/disable-restrictions/pom.xml new file mode 100644 index 0000000..dd461ad --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/pom.xml @@ -0,0 +1,17 @@ + + + 4.0.0 + test + no-external-use + 1.0-SNAPSHOT + pom + + UTF-8 + 1.7 + 1.7 + + + api + caller + + \ No newline at end of file diff --git a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java index 7e1c6a5..4d6cd55 100644 --- a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java +++ b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java @@ -23,6 +23,9 @@ */ package org.kohsuke.accmod.impl; +import java.util.HashSet; +import java.util.Set; +import org.apache.maven.plugin.logging.Log; import org.kohsuke.accmod.AccessRestriction; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.impl.Restrictions.Parser; @@ -85,12 +88,17 @@ public class Checker { private final AccessRestrictionFactory factory; + private final Log log; - Checker(ClassLoader dependencies, ErrorListener errorListener, Properties properties) throws IOException { + private int line; + + Checker(ClassLoader dependencies, ErrorListener errorListener, Properties properties, + Log log) throws IOException { this.dependencies = dependencies; this.errorListener = errorListener; this.properties = properties; this.factory = new AccessRestrictionFactory(dependencies); + this.log = log; // load access restrictions loadAccessRestrictions(); @@ -228,7 +236,7 @@ public void checkClass(File clazz) throws IOException { FileInputStream in = new FileInputStream(clazz); try { ClassReader cr = new ClassReader(in); - cr.accept(new MyClassVisitor(), SKIP_FRAMES); + cr.accept(new RestrictedClassVisitor(), SKIP_FRAMES); } finally { in.close(); } @@ -273,12 +281,18 @@ private static boolean isSynthetic(int access) { return (access & Opcodes.ACC_SYNTHETIC) != 0; } - private class MyClassVisitor extends ClassVisitor { + private class RestrictedClassVisitor extends ClassVisitor { private String className; private String methodName, methodDesc; - private int line; + private String superName; + private String[] interfaces; + private RestrictedAnnotationVisitor annotationVisitor = new RestrictedAnnotationVisitor(); + + boolean isSkippedType(Type type) { + return annotationVisitor.getSkippedTypes().contains(type); + } - public MyClassVisitor() { + public RestrictedClassVisitor() { super(Opcodes.ASM5); } @@ -290,16 +304,24 @@ public void visit(int version, int access, String name, String signature, String return; } - if (superName != null) { + this.superName = superName; + this.interfaces = interfaces; + } + + @Override + public void visitEnd() { + // We need to do this in visitEnd so that we have parsed the annotations _before_ doing these checks + if (superName != null && !isSkippedType(Type.getObjectType(superName))) { for (Restrictions r : getRestrictions(superName)) { r.usedAsSuperType(currentLocation, errorListener); } } - if (interfaces != null) { for (String intf : interfaces) { - for (Restrictions r : getRestrictions(intf)) { - r.usedAsInterface(currentLocation, errorListener); + if(!isSkippedType((Type.getObjectType(intf)))) { + for (Restrictions r : getRestrictions(intf)) { + r.usedAsInterface(currentLocation, errorListener); + } } } } @@ -314,9 +336,13 @@ public MethodVisitor visitMethod(int access, String name, String desc, String si return null; } - return new MyMethodVisitor(currentLocation); + return new RestrictedMethodVisitor(currentLocation, annotationVisitor.getSkippedTypes()); } + @Override + public AnnotationVisitor visitAnnotation(String desc, boolean visible) { + return annotationVisitor; + } /** * Constant that represents the current location. @@ -353,54 +379,107 @@ public String getProperty(String key) { }; } - private class MyMethodVisitor extends MethodVisitor { + private class RestrictedMethodVisitor extends MethodVisitor { + private final Set skippedTypesFromParent; private Location currentLocation; + private RestrictedAnnotationVisitor annotationVisitor = new RestrictedAnnotationVisitor(); + + private boolean isSkippedType(Type t) { + Set allSkippedTypes = new HashSet<>(skippedTypesFromParent); + allSkippedTypes.addAll(annotationVisitor.getSkippedTypes()); + return allSkippedTypes.contains(t); + } - public MyMethodVisitor(Location currentLocation) { + public RestrictedMethodVisitor(Location currentLocation, Set skippedTypes) { super(Opcodes.ASM5); + log.debug(String.format("New method visitor at %s#%s", + currentLocation.getClassName(), currentLocation.getMethodName())); this.currentLocation = currentLocation; + this.skippedTypesFromParent = skippedTypes; } -// @Override -// public void visitLineNumber(int _line, Label start) { - // line = _line; FIXME -// } + @Override + public void visitLineNumber(int _line, Label start) { + line = _line; + } public void visitTypeInsn(int opcode, String type) { switch (opcode) { case Opcodes.NEW: - for (Restrictions r : getRestrictions(type)) { - r.instantiated(currentLocation, errorListener); + if (!isSkippedType(Type.getObjectType(type))) { + for (Restrictions r : getRestrictions(type)) { + r.instantiated(currentLocation, errorListener); + } } } } @Override public void visitMethodInsn(int opcode, String owner, String name, String desc, boolean itf) { - for (Restrictions r : getRestrictions(owner + '.' + name + desc)) { - r.invoked(currentLocation, errorListener); + log.debug(String.format("Visiting method %s#%s", owner, name)); + if (!isSkippedType(Type.getObjectType(owner))) { + for (Restrictions r : getRestrictions(owner + '.' + name + desc)) { + r.invoked(currentLocation, errorListener); + } } } @Override public void visitFieldInsn(int opcode, String owner, String name, String desc) { - Iterable rs = getRestrictions(owner + '.' + name); - switch (opcode) { - case Opcodes.GETSTATIC: - case Opcodes.GETFIELD: - for (Restrictions r : rs) { - r.read(currentLocation, errorListener); - } - break; - case Opcodes.PUTSTATIC: - case Opcodes.PUTFIELD: - for (Restrictions r : rs) { - r.written(currentLocation, errorListener); + log.debug(String.format("Visiting field '%s %s' in type %s", desc, name, owner)); + + if(!isSkippedType(Type.getObjectType(owner))) { + Iterable rs = getRestrictions(owner + '.' + name); + switch (opcode) { + case Opcodes.GETSTATIC: + case Opcodes.GETFIELD: + for (Restrictions r : rs) { + r.read(currentLocation, errorListener); + } + break; + case Opcodes.PUTSTATIC: + case Opcodes.PUTFIELD: + for (Restrictions r : rs) { + r.written(currentLocation, errorListener); + } + break; } - break; + super.visitFieldInsn(opcode, owner, name, desc); } - super.visitFieldInsn(opcode, owner, name, desc); + } + + @Override + public AnnotationVisitor visitAnnotation(String desc, boolean visible) { + return annotationVisitor; + } + } + + private class RestrictedAnnotationVisitor extends AnnotationVisitor { + + private Set skippedRestrictedClasses = new HashSet<>(); + + public RestrictedAnnotationVisitor() { + super(Opcodes.ASM5); + } + + public Set getSkippedTypes() { + return skippedRestrictedClasses; + } + + @Override + public AnnotationVisitor visitArray(String name) { + return new AnnotationVisitor(Opcodes.ASM5) { + + @Override + public void visit(String name, Object value) { + Type type = (Type) value; + log.debug(String.format("Skipping @%s class: %s", + Restricted.class.getSimpleName(), type.getClassName())); + skippedRestrictedClasses.add(type); + super.visit(name, value); + } + }; } } } diff --git a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/EnforcerMojo.java b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/EnforcerMojo.java index 55e5c83..ca769b4 100644 --- a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/EnforcerMojo.java +++ b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/EnforcerMojo.java @@ -84,7 +84,7 @@ public void onError(Throwable t, Location loc, String msg) { public void onWarning(Throwable t, Location loc, String msg) { getLog().warn(loc+" "+msg,t); } - }, properties != null ? properties : new Properties()); + }, properties != null ? properties : new Properties(), getLog()); {// if there's restriction list in the inspected module itself, load it as well InputStream self = null; From 48260fbef0484213a473c7a035e55aaca87214b4 Mon Sep 17 00:00:00 2001 From: Steve Arch Date: Tue, 2 Oct 2018 14:57:39 +0100 Subject: [PATCH 4/9] JENKINS-53675 Fixed intergation-test pom files --- access-modifier-checker/src/it/disable-restrictions/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/access-modifier-checker/src/it/disable-restrictions/pom.xml b/access-modifier-checker/src/it/disable-restrictions/pom.xml index dd461ad..3140ece 100644 --- a/access-modifier-checker/src/it/disable-restrictions/pom.xml +++ b/access-modifier-checker/src/it/disable-restrictions/pom.xml @@ -2,7 +2,7 @@ 4.0.0 test - no-external-use + disable-restrictions 1.0-SNAPSHOT pom From 5aebb1c988831ed05f4b6049676ee30966c1c8f8 Mon Sep 17 00:00:00 2001 From: Steve Arch Date: Tue, 2 Oct 2018 16:46:00 +0100 Subject: [PATCH 5/9] JENKINS-53675 Added missing pom files --- .../src/it/disable-restrictions/api/pom.xml | 17 ++++++++++ .../it/disable-restrictions/caller/pom.xml | 33 +++++++++++++++++++ .../src/it/disable-restrictions/pom.xml | 28 ++++++++-------- 3 files changed, 64 insertions(+), 14 deletions(-) create mode 100644 access-modifier-checker/src/it/disable-restrictions/api/pom.xml create mode 100644 access-modifier-checker/src/it/disable-restrictions/caller/pom.xml diff --git a/access-modifier-checker/src/it/disable-restrictions/api/pom.xml b/access-modifier-checker/src/it/disable-restrictions/api/pom.xml new file mode 100644 index 0000000..bd06be5 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/api/pom.xml @@ -0,0 +1,17 @@ + + + 4.0.0 + + test + disable-restrictions + 1.0-SNAPSHOT + + api + + + org.kohsuke + access-modifier-annotation + @project.version@ + + + \ No newline at end of file diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/pom.xml b/access-modifier-checker/src/it/disable-restrictions/caller/pom.xml new file mode 100644 index 0000000..c75f1a0 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions/caller/pom.xml @@ -0,0 +1,33 @@ + + + 4.0.0 + + test + disable-restrictions + 1.0-SNAPSHOT + + caller + + + ${project.groupId} + api + ${project.version} + + + + + + org.kohsuke + access-modifier-checker + @project.version@ + + + + enforce + + + + + + + \ No newline at end of file diff --git a/access-modifier-checker/src/it/disable-restrictions/pom.xml b/access-modifier-checker/src/it/disable-restrictions/pom.xml index 3140ece..c03dd1e 100644 --- a/access-modifier-checker/src/it/disable-restrictions/pom.xml +++ b/access-modifier-checker/src/it/disable-restrictions/pom.xml @@ -1,17 +1,17 @@ - 4.0.0 - test - disable-restrictions - 1.0-SNAPSHOT - pom - - UTF-8 - 1.7 - 1.7 - - - api - caller - + 4.0.0 + test + disable-restrictions + 1.0-SNAPSHOT + pom + + UTF-8 + 1.7 + 1.7 + + + api + caller + \ No newline at end of file From 74e309dd06bbb239370aa7771e02d18438849dd0 Mon Sep 17 00:00:00 2001 From: Steve Arch Date: Tue, 2 Oct 2018 17:04:15 +0100 Subject: [PATCH 6/9] JENKINS-53675 Pushed the skippedTypes into the getRestrictions Method --- .../java/org/kohsuke/accmod/impl/Checker.java | 74 +++++++++---------- 1 file changed, 35 insertions(+), 39 deletions(-) diff --git a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java index 4d6cd55..bd4a066 100644 --- a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java +++ b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java @@ -242,11 +242,11 @@ public void checkClass(File clazz) throws IOException { } } - private Iterable getRestrictions(String keyName) { + private Iterable getRestrictions(String keyName, Set skippedTypes) { List rs = new ArrayList<>(); Restrictions r = restrictions.get(keyName); - if (r != null) { - rs.add(r); + if (r != null && skippedTypes.isEmpty()) { + rs.add(r); // Don't get it from the cache if we have types that we want to skip } int idx = Integer.MAX_VALUE; while (true) { @@ -259,6 +259,10 @@ private Iterable getRestrictions(String keyName) { } idx = newIdx; keyName = keyName.substring(0, idx); + if(skippedTypes.contains(Type.getObjectType(keyName))) { + // We have hit a type that should be skipped - do not add it to the restrictions + break; + } r = restrictions.get(keyName); if (r != null) { Collection applicable = new ArrayList<>(); @@ -288,8 +292,8 @@ private class RestrictedClassVisitor extends ClassVisitor { private String[] interfaces; private RestrictedAnnotationVisitor annotationVisitor = new RestrictedAnnotationVisitor(); - boolean isSkippedType(Type type) { - return annotationVisitor.getSkippedTypes().contains(type); + private Set getSkippedTypes() { + return annotationVisitor.getSkippedTypes(); } public RestrictedClassVisitor() { @@ -311,17 +315,15 @@ public void visit(int version, int access, String name, String signature, String @Override public void visitEnd() { // We need to do this in visitEnd so that we have parsed the annotations _before_ doing these checks - if (superName != null && !isSkippedType(Type.getObjectType(superName))) { - for (Restrictions r : getRestrictions(superName)) { + if (superName != null) { + for (Restrictions r : getRestrictions(superName, getSkippedTypes())) { r.usedAsSuperType(currentLocation, errorListener); } } if (interfaces != null) { for (String intf : interfaces) { - if(!isSkippedType((Type.getObjectType(intf)))) { - for (Restrictions r : getRestrictions(intf)) { - r.usedAsInterface(currentLocation, errorListener); - } + for (Restrictions r : getRestrictions(intf, getSkippedTypes())) { + r.usedAsInterface(currentLocation, errorListener); } } } @@ -385,10 +387,10 @@ private class RestrictedMethodVisitor extends MethodVisitor { private Location currentLocation; private RestrictedAnnotationVisitor annotationVisitor = new RestrictedAnnotationVisitor(); - private boolean isSkippedType(Type t) { + private Set getSkippedTypes() { Set allSkippedTypes = new HashSet<>(skippedTypesFromParent); allSkippedTypes.addAll(annotationVisitor.getSkippedTypes()); - return allSkippedTypes.contains(t); + return allSkippedTypes; } public RestrictedMethodVisitor(Location currentLocation, Set skippedTypes) { @@ -407,10 +409,8 @@ public void visitLineNumber(int _line, Label start) { public void visitTypeInsn(int opcode, String type) { switch (opcode) { case Opcodes.NEW: - if (!isSkippedType(Type.getObjectType(type))) { - for (Restrictions r : getRestrictions(type)) { - r.instantiated(currentLocation, errorListener); - } + for (Restrictions r : getRestrictions(type, getSkippedTypes())) { + r.instantiated(currentLocation, errorListener); } } } @@ -418,10 +418,8 @@ public void visitTypeInsn(int opcode, String type) { @Override public void visitMethodInsn(int opcode, String owner, String name, String desc, boolean itf) { log.debug(String.format("Visiting method %s#%s", owner, name)); - if (!isSkippedType(Type.getObjectType(owner))) { - for (Restrictions r : getRestrictions(owner + '.' + name + desc)) { - r.invoked(currentLocation, errorListener); - } + for (Restrictions r : getRestrictions(owner + '.' + name + desc, getSkippedTypes())) { + r.invoked(currentLocation, errorListener); } } @@ -429,24 +427,22 @@ public void visitMethodInsn(int opcode, String owner, String name, String desc, public void visitFieldInsn(int opcode, String owner, String name, String desc) { log.debug(String.format("Visiting field '%s %s' in type %s", desc, name, owner)); - if(!isSkippedType(Type.getObjectType(owner))) { - Iterable rs = getRestrictions(owner + '.' + name); - switch (opcode) { - case Opcodes.GETSTATIC: - case Opcodes.GETFIELD: - for (Restrictions r : rs) { - r.read(currentLocation, errorListener); - } - break; - case Opcodes.PUTSTATIC: - case Opcodes.PUTFIELD: - for (Restrictions r : rs) { - r.written(currentLocation, errorListener); - } - break; - } - super.visitFieldInsn(opcode, owner, name, desc); + Iterable rs = getRestrictions(owner + '.' + name, getSkippedTypes()); + switch (opcode) { + case Opcodes.GETSTATIC: + case Opcodes.GETFIELD: + for (Restrictions r : rs) { + r.read(currentLocation, errorListener); + } + break; + case Opcodes.PUTSTATIC: + case Opcodes.PUTFIELD: + for (Restrictions r : rs) { + r.written(currentLocation, errorListener); + } + break; } + super.visitFieldInsn(opcode, owner, name, desc); } @Override @@ -473,7 +469,7 @@ public AnnotationVisitor visitArray(String name) { @Override public void visit(String name, Object value) { - Type type = (Type) value; + Type type = value instanceof Type ? (Type) value : Type.getType(value.getClass()); log.debug(String.format("Skipping @%s class: %s", Restricted.class.getSimpleName(), type.getClassName())); skippedRestrictedClasses.add(type); From 1ac046ec9662cce8284bba285cbffa0d01df2b88 Mon Sep 17 00:00:00 2001 From: Steve Arch Date: Tue, 2 Oct 2018 17:09:11 +0100 Subject: [PATCH 7/9] JENKINS-53675 Corrected javadoc and headers --- .../disable/DisableRestriction.java | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java b/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java index 37bcb15..a7d9f2f 100644 --- a/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java +++ b/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java @@ -1,7 +1,7 @@ /* * The MIT License * - * Copyright (c) 2010, Kohsuke Kawaguchi + * Copyright (c) 2018, Steve Arch * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -29,28 +29,22 @@ import java.lang.annotation.Target; import org.jvnet.hudson.annotation_indexer.Indexed; import org.kohsuke.accmod.AccessRestriction; +import org.kohsuke.accmod.Restricted; import static java.lang.annotation.RetentionPolicy.RUNTIME; /** - * Indicates that a particular element is really deprecated and that the access to it - * is subject to the additional restrictions. + * Indicates that certain classes annotated with {@link Restricted} annotations should be skipped during the + * access-modifier-check. * - *

- * These annotations and restrictions introduced by them are enforced by the - * "access-modifier-checker" mojo. - * - * @author Kohsuke Kawaguchi + * @author Steve Arch */ @Retention(RUNTIME) @Documented -//@Indexed @Target({ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.TYPE}) public @interface DisableRestriction { /** - * Kind of access that are restricted. - * If multiple values are specified, those restrictions are OR-ed — thus if an use - * violates any of the restrictions, it'll be considered as an error. + * The classes that are marked as {@link Restricted} that should be skipped from the scan. */ Class[] value(); } From 0a68c69ce027920427dfe48571d18d770d38bbc3 Mon Sep 17 00:00:00 2001 From: Steve Arch Date: Wed, 3 Oct 2018 10:48:56 +0100 Subject: [PATCH 8/9] JENKINS-53675 Actually check that we are using the DisableRestritions annotation --- .../api/pom.xml | 17 +++++++++ .../api/ApiWithRestrictedMethodAndField.java | 13 +++++++ .../api/src/main/java/api/RestrictedApi.java | 12 ++++++ .../main/java/api/RestrictedInterface.java | 8 ++++ .../caller/pom.xml | 33 ++++++++++++++++ .../caller/src/main/java/caller/Caller.java | 11 ++++++ .../caller/CallerDisabledAtClassLevel.java | 12 ++++++ .../src/main/java/caller/WrongAnnotation.java | 38 +++++++++++++++++++ .../invoker.properties | 2 + .../pom.xml | 17 +++++++++ .../postbuild.groovy | 2 + .../java/org/kohsuke/accmod/impl/Checker.java | 10 +++-- 12 files changed, 172 insertions(+), 3 deletions(-) create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/pom.xml create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/ApiWithRestrictedMethodAndField.java create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/RestrictedApi.java create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/RestrictedInterface.java create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/pom.xml create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/Caller.java create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/CallerDisabledAtClassLevel.java create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/WrongAnnotation.java create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/invoker.properties create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/pom.xml create mode 100644 access-modifier-checker/src/it/disable-restrictions-wrong-annotation/postbuild.groovy diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/pom.xml b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/pom.xml new file mode 100644 index 0000000..be7ee62 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/pom.xml @@ -0,0 +1,17 @@ + + + 4.0.0 + + test + disable-restrictions-wrong-annotation + 1.0-SNAPSHOT + + api + + + org.kohsuke + access-modifier-annotation + @project.version@ + + + \ No newline at end of file diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/ApiWithRestrictedMethodAndField.java b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/ApiWithRestrictedMethodAndField.java new file mode 100644 index 0000000..82faef5 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/ApiWithRestrictedMethodAndField.java @@ -0,0 +1,13 @@ +package api; + +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +public class ApiWithRestrictedMethodAndField { + + @Restricted(NoExternalUse.class) + public String field; + + @Restricted(NoExternalUse.class) + public static void notReallyPublic() {} +} diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/RestrictedApi.java b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/RestrictedApi.java new file mode 100644 index 0000000..ef23039 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/RestrictedApi.java @@ -0,0 +1,12 @@ +package api; + +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +@Restricted(NoExternalUse.class) +public class RestrictedApi { + + public String field; + + public void doNotUse() {} +} diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/RestrictedInterface.java b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/RestrictedInterface.java new file mode 100644 index 0000000..c51fd14 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/api/src/main/java/api/RestrictedInterface.java @@ -0,0 +1,8 @@ +package api; + +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +@Restricted(NoExternalUse.class) +public interface RestrictedInterface { +} diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/pom.xml b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/pom.xml new file mode 100644 index 0000000..55c0989 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/pom.xml @@ -0,0 +1,33 @@ + + + 4.0.0 + + test + disable-restrictions-wrong-annotation + 1.0-SNAPSHOT + + caller + + + ${project.groupId} + api + ${project.version} + + + + + + org.kohsuke + access-modifier-checker + @project.version@ + + + + enforce + + + + + + + \ No newline at end of file diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/Caller.java b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/Caller.java new file mode 100644 index 0000000..6591965 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/Caller.java @@ -0,0 +1,11 @@ +package caller; + +import api.ApiWithRestrictedMethodAndField; + +public class Caller { + + @WrongAnnotation(ApiWithRestrictedMethodAndField.class) + public Caller() { + ApiWithRestrictedMethodAndField.notReallyPublic(); // illegal + } +} diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/CallerDisabledAtClassLevel.java b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/CallerDisabledAtClassLevel.java new file mode 100644 index 0000000..f1a6662 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/CallerDisabledAtClassLevel.java @@ -0,0 +1,12 @@ +package caller; + +import api.ApiWithRestrictedMethodAndField; +import api.RestrictedApi; +import org.kohsuke.accmod.restrictions.disable.DisableRestriction; + +@WrongAnnotation(ApiWithRestrictedMethodAndField.class) +public class CallerDisabledAtClassLevel { + public CallerDisabledAtClassLevel() { + ApiWithRestrictedMethodAndField.notReallyPublic(); // illegal + } +} \ No newline at end of file diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/WrongAnnotation.java b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/WrongAnnotation.java new file mode 100644 index 0000000..3ff929f --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/WrongAnnotation.java @@ -0,0 +1,38 @@ +/* + * The MIT License + * + * Copyright (c) 2018, Steve Arch + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package caller; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +@Retention(RUNTIME) +@Documented +@Target({ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.TYPE}) +public @interface WrongAnnotation { + Class[] value(); +} diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/invoker.properties b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/invoker.properties new file mode 100644 index 0000000..8b87cc7 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/invoker.properties @@ -0,0 +1,2 @@ +invoker.goals=clean package +invoker.buildResult = failure diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/pom.xml b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/pom.xml new file mode 100644 index 0000000..4886872 --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/pom.xml @@ -0,0 +1,17 @@ + + + 4.0.0 + test + disable-restrictions-wrong-annotation + 1.0-SNAPSHOT + pom + + UTF-8 + 1.7 + 1.7 + + + api + caller + + \ No newline at end of file diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/postbuild.groovy b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/postbuild.groovy new file mode 100644 index 0000000..598dbea --- /dev/null +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/postbuild.groovy @@ -0,0 +1,2 @@ +assert new File(basedir, 'build.log').text.contains('[ERROR] caller/Caller:9 api/ApiWithRestrictedMethodAndField.notReallyPublic()V must not be used') +assert new File(basedir, 'build.log').text.contains('[ERROR] caller/CallerDisabledAtClassLevel:10 api/ApiWithRestrictedMethodAndField.notReallyPublic()V must not be used') diff --git a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java index bd4a066..c58c73e 100644 --- a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java +++ b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java @@ -29,6 +29,7 @@ import org.kohsuke.accmod.AccessRestriction; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.impl.Restrictions.Parser; +import org.kohsuke.accmod.restrictions.disable.DisableRestriction; import org.objectweb.asm.AnnotationVisitor; import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassVisitor; @@ -343,8 +344,9 @@ public MethodVisitor visitMethod(int access, String name, String desc, String si @Override public AnnotationVisitor visitAnnotation(String desc, boolean visible) { - return annotationVisitor; - } + return Type.getType(DisableRestriction.class).equals(Type.getType(desc)) + ? annotationVisitor + : super.visitAnnotation(desc, visible); } /** * Constant that represents the current location. @@ -447,7 +449,9 @@ public void visitFieldInsn(int opcode, String owner, String name, String desc) { @Override public AnnotationVisitor visitAnnotation(String desc, boolean visible) { - return annotationVisitor; + return Type.getType(DisableRestriction.class).equals(Type.getType(desc)) + ? annotationVisitor + : super.visitAnnotation(desc, visible); } } From 00c1a05b2ddba897d2010f3fac4a090d01c3539f Mon Sep 17 00:00:00 2001 From: Steve Arch Date: Fri, 5 Oct 2018 10:21:42 +0100 Subject: [PATCH 9/9] JENKINS-53675 Moved and renamed the suppression annotation SuppressRestrictedWarnings mirrors java.lang.SuppressWarnings naming Moved to a separate package makes it less-available to users and puts more barriers in for use. This makes the user stop-and-think about what they are doing, rather than just turning the warning off. --- access-modifier-checker/pom.xml | 5 ++++ .../caller/CallerDisabledAtClassLevel.java | 2 -- .../postbuild.groovy | 2 +- .../it/disable-restrictions/caller/pom.xml | 5 ++++ .../caller/src/main/java/caller/Caller.java | 12 ++++---- .../caller/CallerDisabledAtClassLevel.java | 4 +-- .../java/caller/RestrictedApiSubclass.java | 4 +-- .../RestrictedInterfaceImplementation.java | 4 +-- .../java/org/kohsuke/accmod/impl/Checker.java | 6 ++-- access-modifier-suppressions/pom.xml | 29 +++++++++++++++++++ .../SuppressRestrictedWarnings.java | 13 +++++---- pom.xml | 1 + 12 files changed, 63 insertions(+), 24 deletions(-) create mode 100644 access-modifier-suppressions/pom.xml rename access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java => access-modifier-suppressions/src/main/java/org/kohsuke/accmod/restrictions/suppressions/SuppressRestrictedWarnings.java (81%) diff --git a/access-modifier-checker/pom.xml b/access-modifier-checker/pom.xml index fe0278b..3b2e3f1 100644 --- a/access-modifier-checker/pom.xml +++ b/access-modifier-checker/pom.xml @@ -22,6 +22,11 @@ access-modifier-annotation ${project.version} + + ${project.groupId} + access-modifier-suppressions + ${project.version} + org.ow2.asm asm-debug-all diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/CallerDisabledAtClassLevel.java b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/CallerDisabledAtClassLevel.java index f1a6662..b912657 100644 --- a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/CallerDisabledAtClassLevel.java +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/caller/src/main/java/caller/CallerDisabledAtClassLevel.java @@ -1,8 +1,6 @@ package caller; import api.ApiWithRestrictedMethodAndField; -import api.RestrictedApi; -import org.kohsuke.accmod.restrictions.disable.DisableRestriction; @WrongAnnotation(ApiWithRestrictedMethodAndField.class) public class CallerDisabledAtClassLevel { diff --git a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/postbuild.groovy b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/postbuild.groovy index 598dbea..ae75e56 100644 --- a/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/postbuild.groovy +++ b/access-modifier-checker/src/it/disable-restrictions-wrong-annotation/postbuild.groovy @@ -1,2 +1,2 @@ assert new File(basedir, 'build.log').text.contains('[ERROR] caller/Caller:9 api/ApiWithRestrictedMethodAndField.notReallyPublic()V must not be used') -assert new File(basedir, 'build.log').text.contains('[ERROR] caller/CallerDisabledAtClassLevel:10 api/ApiWithRestrictedMethodAndField.notReallyPublic()V must not be used') +assert new File(basedir, 'build.log').text.contains('[ERROR] caller/CallerDisabledAtClassLevel:8 api/ApiWithRestrictedMethodAndField.notReallyPublic()V must not be used') diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/pom.xml b/access-modifier-checker/src/it/disable-restrictions/caller/pom.xml index c75f1a0..95d97b3 100644 --- a/access-modifier-checker/src/it/disable-restrictions/caller/pom.xml +++ b/access-modifier-checker/src/it/disable-restrictions/caller/pom.xml @@ -13,6 +13,11 @@ api ${project.version} + + org.kohsuke + access-modifier-suppressions + @project.version@ + diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/Caller.java b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/Caller.java index e5ac9ef..040d5f9 100644 --- a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/Caller.java +++ b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/Caller.java @@ -2,34 +2,34 @@ import api.ApiWithRestrictedMethodAndField; import api.RestrictedApi; -import org.kohsuke.accmod.restrictions.disable.DisableRestriction; +import org.kohsuke.accmod.restrictions.suppressions.SuppressRestrictedWarnings; public class Caller extends ApiWithRestrictedMethodAndField { // This is fine, ApiWithRestrictedMethodAndField itself is not restricted private RestrictedApi restrictedApi; - @DisableRestriction(ApiWithRestrictedMethodAndField.class) + @SuppressRestrictedWarnings(ApiWithRestrictedMethodAndField.class) public Caller() { ApiWithRestrictedMethodAndField.notReallyPublic(); // illegal but check disabled at the method level } - @DisableRestriction({RestrictedApi.class, ApiWithRestrictedMethodAndField.class}) + @SuppressRestrictedWarnings({RestrictedApi.class, ApiWithRestrictedMethodAndField.class}) private void invalidFieldUse() { restrictedApi.field = null; super.field = null; } - @DisableRestriction(ApiWithRestrictedMethodAndField.class) + @SuppressRestrictedWarnings(ApiWithRestrictedMethodAndField.class) public void callerMethod() { ApiWithRestrictedMethodAndField.notReallyPublic(); // illegal but check disabled at the method level } - @DisableRestriction(RestrictedApi.class) + @SuppressRestrictedWarnings(RestrictedApi.class) public void methodWithRestrictedParameter(RestrictedApi api) { api.doNotUse(); // illegal but check disabled at the method level } - @DisableRestriction(RestrictedApi.class) + @SuppressRestrictedWarnings(RestrictedApi.class) public RestrictedApi getRestrictedApi() { return new RestrictedApi(); // illegal but check disabled at the method level } diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/CallerDisabledAtClassLevel.java b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/CallerDisabledAtClassLevel.java index 8b4f0cb..1314fbc 100644 --- a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/CallerDisabledAtClassLevel.java +++ b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/CallerDisabledAtClassLevel.java @@ -2,9 +2,9 @@ import api.ApiWithRestrictedMethodAndField; import api.RestrictedApi; -import org.kohsuke.accmod.restrictions.disable.DisableRestriction; +import org.kohsuke.accmod.restrictions.suppressions.SuppressRestrictedWarnings; -@DisableRestriction( {ApiWithRestrictedMethodAndField.class, RestrictedApi.class}) +@SuppressRestrictedWarnings( {ApiWithRestrictedMethodAndField.class, RestrictedApi.class}) public class CallerDisabledAtClassLevel extends RestrictedApi { private RestrictedApi restrictedApi; diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedApiSubclass.java b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedApiSubclass.java index 1eea69c..f03ff47 100644 --- a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedApiSubclass.java +++ b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedApiSubclass.java @@ -1,8 +1,8 @@ package caller; import api.RestrictedApi; -import org.kohsuke.accmod.restrictions.disable.DisableRestriction; +import org.kohsuke.accmod.restrictions.suppressions.SuppressRestrictedWarnings; -@DisableRestriction(RestrictedApi.class) +@SuppressRestrictedWarnings(RestrictedApi.class) public class RestrictedApiSubclass extends RestrictedApi { } diff --git a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedInterfaceImplementation.java b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedInterfaceImplementation.java index cffc8cd..d09afcc 100644 --- a/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedInterfaceImplementation.java +++ b/access-modifier-checker/src/it/disable-restrictions/caller/src/main/java/caller/RestrictedInterfaceImplementation.java @@ -1,8 +1,8 @@ package caller; import api.RestrictedInterface; -import org.kohsuke.accmod.restrictions.disable.DisableRestriction; +import org.kohsuke.accmod.restrictions.suppressions.SuppressRestrictedWarnings; -@DisableRestriction(RestrictedInterface.class) +@SuppressRestrictedWarnings(RestrictedInterface.class) public class RestrictedInterfaceImplementation implements RestrictedInterface { } diff --git a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java index c58c73e..42160c8 100644 --- a/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java +++ b/access-modifier-checker/src/main/java/org/kohsuke/accmod/impl/Checker.java @@ -29,7 +29,7 @@ import org.kohsuke.accmod.AccessRestriction; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.impl.Restrictions.Parser; -import org.kohsuke.accmod.restrictions.disable.DisableRestriction; +import org.kohsuke.accmod.restrictions.suppressions.SuppressRestrictedWarnings; import org.objectweb.asm.AnnotationVisitor; import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassVisitor; @@ -344,7 +344,7 @@ public MethodVisitor visitMethod(int access, String name, String desc, String si @Override public AnnotationVisitor visitAnnotation(String desc, boolean visible) { - return Type.getType(DisableRestriction.class).equals(Type.getType(desc)) + return Type.getType(SuppressRestrictedWarnings.class).equals(Type.getType(desc)) ? annotationVisitor : super.visitAnnotation(desc, visible); } @@ -449,7 +449,7 @@ public void visitFieldInsn(int opcode, String owner, String name, String desc) { @Override public AnnotationVisitor visitAnnotation(String desc, boolean visible) { - return Type.getType(DisableRestriction.class).equals(Type.getType(desc)) + return Type.getType(SuppressRestrictedWarnings.class).equals(Type.getType(desc)) ? annotationVisitor : super.visitAnnotation(desc, visible); } diff --git a/access-modifier-suppressions/pom.xml b/access-modifier-suppressions/pom.xml new file mode 100644 index 0000000..aded61f --- /dev/null +++ b/access-modifier-suppressions/pom.xml @@ -0,0 +1,29 @@ + + + 4.0.0 + + access-modifier + org.kohsuke + 1.16-SNAPSHOT + + access-modifier-suppressions + + Suppression for Access Modifier annotations + This module allows you to enable suppressions for turning off warnings about Restricted APIs. + !!!WARNING!!! + Classes are marked as @Restricted for a reason and this module should not be used lightly! It implies that the + author does not intend for them to be used outside their defined scope and as such they may be + changed/modified/removed at any stage without warning. A simple upgrade of the dependency may break your module. Use + at your own risk. + You should try to not use @Restricted classes in the first place, but if you _must_ use them, this is a less-brutal + approach than just disabling the access-modifier-checker entirely + + + + + org.kohsuke + access-modifier-annotation + ${project.version} + + + diff --git a/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java b/access-modifier-suppressions/src/main/java/org/kohsuke/accmod/restrictions/suppressions/SuppressRestrictedWarnings.java similarity index 81% rename from access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java rename to access-modifier-suppressions/src/main/java/org/kohsuke/accmod/restrictions/suppressions/SuppressRestrictedWarnings.java index a7d9f2f..b2f2224 100644 --- a/access-modifier-annotation/src/main/java/org/kohsuke/accmod/restrictions/disable/DisableRestriction.java +++ b/access-modifier-suppressions/src/main/java/org/kohsuke/accmod/restrictions/suppressions/SuppressRestrictedWarnings.java @@ -21,28 +21,29 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN * THE SOFTWARE. */ -package org.kohsuke.accmod.restrictions.disable; +package org.kohsuke.accmod.restrictions.suppressions; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.Target; -import org.jvnet.hudson.annotation_indexer.Indexed; -import org.kohsuke.accmod.AccessRestriction; import org.kohsuke.accmod.Restricted; import static java.lang.annotation.RetentionPolicy.RUNTIME; /** - * Indicates that certain classes annotated with {@link Restricted} annotations should be skipped during the - * access-modifier-check. + *

Indicates that certain classes annotated with {@link Restricted} annotations should be skipped during the + * access-modifier-check.

+ * + *

Warning! Classes are markes as {@link Restricted} for a reason! Do not use these suppressions lightly. + * Use at your own risk

* * @author Steve Arch */ @Retention(RUNTIME) @Documented @Target({ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.TYPE}) -public @interface DisableRestriction { +public @interface SuppressRestrictedWarnings { /** * The classes that are marked as {@link Restricted} that should be skipped from the scan. */ diff --git a/pom.xml b/pom.xml index a851974..2ca0fdc 100644 --- a/pom.xml +++ b/pom.xml @@ -18,6 +18,7 @@ access-modifier-annotation access-modifier-checker + access-modifier-suppressions