diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/AidlImplementationDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/AidlImplementationDetector.kt index 227cdcdc2fec2..ab6d871d6ea67 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/AidlImplementationDetector.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/AidlImplementationDetector.kt @@ -36,7 +36,7 @@ abstract class AidlImplementationDetector : Detector(), SourceCodeScanner { private inner class AidlStubHandler(val context: JavaContext) : UElementHandler() { override fun visitMethod(node: UMethod) { - val interfaceName = getContainingAidlInterface(node) + val interfaceName = getContainingAidlInterface(context, node) .takeUnless(EXCLUDED_CPP_INTERFACES::contains) ?: return val body = (node.uastBody as? UBlockExpression) ?: return visitAidlMethod(context, node, interfaceName, body) diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt index 8ee3763e50794..72de00f885ba2 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt @@ -26,6 +26,7 @@ val AIDL_PERMISSION_ANNOTATIONS = listOf( ANNOTATION_PERMISSION_MANUALLY_ENFORCED ) +const val BINDER_CLASS = "android.os.Binder" const val IINTERFACE_INTERFACE = "android.os.IInterface" /** diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt index 2665b3c9ec20b..0baac2c7aacf6 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt @@ -38,6 +38,7 @@ import com.intellij.psi.PsiMethod import org.jetbrains.uast.UAnnotation import org.jetbrains.uast.UElement import org.jetbrains.uast.UMethod +import org.jetbrains.uast.toUElement /** * Lint Detector that ensures that any method overriding a method annotated @@ -55,9 +56,6 @@ import org.jetbrains.uast.UMethod */ class EnforcePermissionDetector : Detector(), SourceCodeScanner { - val BINDER_CLASS = "android.os.Binder" - val JAVA_OBJECT = "java.lang.Object" - override fun applicableAnnotations(): List { return listOf(ANNOTATION_ENFORCE_PERMISSION) } @@ -123,6 +121,11 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { overriddenMethod: PsiMethod, checkEquivalence: Boolean = true ) { + // If method is not from a Stub subclass, this method shouldn't use @EP at all. + // This is handled by EnforcePermissionHelperDetector. + if (!isContainedInSubclassOfStub(context, overridingMethod.toUElement() as? UMethod)) { + return + } val overridingAnnotation = overridingMethod.getAnnotation(ANNOTATION_ENFORCE_PERMISSION) val overriddenAnnotation = overriddenMethod.getAnnotation(ANNOTATION_ENFORCE_PERMISSION) val location = context.getLocation(element) @@ -131,13 +134,6 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { val overridingName = "${overridingClass.name}.${overridingMethod.name}" val overriddenName = "${overriddenClass.name}.${overriddenMethod.name}" if (overridingAnnotation == null) { - if (shouldIgnoreGeneratedMethod( - context, - overriddenClass = overriddenClass, - overridingClass = overridingClass) - ) { - return - } val msg = "The method $overridingName overrides the method $overriddenName which " + "is annotated with @EnforcePermission. The same annotation must be used " + "on $overridingName" @@ -177,54 +173,19 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { if (node.qualifiedName != ANNOTATION_ENFORCE_PERMISSION) { return } - val method = node.uastParent as? UMethod - if (method != null) { - val overridingMethod = method as PsiMethod - val parents = overridingMethod.findSuperMethods() - for (overriddenMethod in parents) { - // The equivalence check can be skipped, if both methods are - // annotated, it will be verified by visitAnnotationUsage. - compareMethods(context, method, overridingMethod, - overriddenMethod, checkEquivalence = false) - } + val method = node.uastParent as? UMethod ?: return + val overridingMethod = method as PsiMethod + val parents = overridingMethod.findSuperMethods() + for (overriddenMethod in parents) { + // The equivalence check can be skipped, if both methods are + // annotated, it will be verified by visitAnnotationUsage. + compareMethods(context, method, overridingMethod, + overriddenMethod, checkEquivalence = false) } } } } - /** - * since this lint runs globally, it will also run against generated - * test code e.g. - * system/tools/aidl/tests/golden_output/aidl-test-interface-permission-java-source/gen/android/aidl/tests/permission/IProtected.java - * system/tools/aidl/tests/golden_output/aidl-test-interface-permission-java-source/gen/android/aidl/tests/permission/IProtectedInterface.java - * we do not want to report errors against generated `Stub` and `Proxy` classes in those files - */ - private fun shouldIgnoreGeneratedMethod( - context: JavaContext, - overriddenClass: PsiClass, - overridingClass: PsiClass, - - ): Boolean { - if (isInterfaceAndExtendsIInterface(overriddenClass) && - context.evaluator.isStatic(overridingClass)) { - if (overridingClass.name == "Default") return true - if (overridingClass.name == "Proxy") { - val shouldBeStub = overridingClass.parent as? PsiClass ?: return false - return shouldBeStub.name == "Stub" && - context.evaluator.isAbstract(shouldBeStub) && - context.evaluator.isStatic(shouldBeStub) && - shouldBeStub.extendsList?.referenceElements - ?.any { it.qualifiedName == BINDER_CLASS } == true - } - } - return false - } - - private fun isInterfaceAndExtendsIInterface(overriddenClass: PsiClass): Boolean = - overriddenClass.isInterface && - overriddenClass.extendsList?.referenceElements - ?.any { it.qualifiedName == IINTERFACE_INTERFACE } == true - companion object { val EXPLANATION = """ The @EnforcePermission annotation is used to indicate that the underlying binder code diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt index 268c565b65ffe..b65c0fc6a7db7 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt @@ -45,8 +45,19 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner { if (context.evaluator.isAbstract(node)) return if (!node.hasAnnotation(ANNOTATION_ENFORCE_PERMISSION)) return + if (!isContainedInSubclassOfStub(context, node)) { + context.report( + ISSUE_MISUSING_ENFORCE_PERMISSION, + node, + context.getLocation(node), + "The class of ${node.name} does not inherit from an AIDL generated Stub class" + ) + return + } + val targetExpression = "${node.name}$HELPER_SUFFIX()" - val message = "Method must start with $targetExpression or super.${node.name}()" + val message = + "Method must start with $targetExpression or super.${node.name}(), if applicable" val firstExpression = (node.uastBody as? UBlockExpression) ?.expressions?.firstOrNull() @@ -99,16 +110,17 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner { private const val HELPER_SUFFIX = "_enforcePermission" private const val EXPLANATION = """ - When @EnforcePermission is applied, the AIDL compiler generates a Stub method to do the - permission check called yourMethodName$HELPER_SUFFIX. + The @EnforcePermission annotation can only be used on methods whose class extends from + the Stub class generated by the AIDL compiler. When @EnforcePermission is applied, the + AIDL compiler generates a Stub method to do the permission check called yourMethodName$HELPER_SUFFIX. yourMethodName$HELPER_SUFFIX must be executed before any other operation. To do that, you can - either call it directly or indirectly via super.yourMethodName(). + either call it directly, or call it indirectly via super.yourMethodName(). """ val ISSUE_ENFORCE_PERMISSION_HELPER: Issue = Issue.create( id = "MissingEnforcePermissionHelper", - briefDescription = """Missing permission-enforcing method call in AIDL method + briefDescription = """Missing permission-enforcing method call in AIDL method |annotated with @EnforcePermission""".trimMargin(), explanation = EXPLANATION, category = Category.SECURITY, @@ -120,6 +132,19 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner { ) ) + val ISSUE_MISUSING_ENFORCE_PERMISSION: Issue = Issue.create( + id = "MisusingEnforcePermissionAnnotation", + briefDescription = "@EnforcePermission cannot be used here", + explanation = EXPLANATION, + category = Category.SECURITY, + priority = 6, + severity = Severity.ERROR, + implementation = Implementation( + EnforcePermissionDetector::class.java, + Scope.JAVA_FILE_SCOPE + ) + ) + /** * handles an edge case with UDeclarationsExpression, where sourcePsi is null, * resulting in an incorrect Location if used directly diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt index 250ca78bae5e6..2239ea169632d 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt @@ -16,17 +16,18 @@ package com.google.android.lint.aidl -import com.google.android.lint.CLASS_STUB -import com.intellij.psi.PsiAnonymousClass +import com.android.tools.lint.detector.api.JavaContext +import com.intellij.psi.PsiClass +import com.intellij.psi.PsiReferenceList import org.jetbrains.uast.UMethod /** * Given a UMethod, determine if this method is - * an entrypoint to an interface generated by AIDL, - * returning the interface name if so + * the entrypoint to an interface generated by AIDL, + * returning the interface name if so, otherwise returning null */ -fun getContainingAidlInterface(node: UMethod): String? { - if (!isInClassCalledStub(node)) return null +fun getContainingAidlInterface(context: JavaContext, node: UMethod): String? { + if (!isContainedInSubclassOfStub(context, node)) return null for (superMethod in node.findSuperMethods()) { for (extendsInterface in superMethod.containingClass?.extendsList?.referenceElements ?: continue) { @@ -38,11 +39,33 @@ fun getContainingAidlInterface(node: UMethod): String? { return null } -private fun isInClassCalledStub(node: UMethod): Boolean { - (node.containingClass as? PsiAnonymousClass)?.let { - return it.baseClassReference.referenceName == CLASS_STUB +fun isContainedInSubclassOfStub(context: JavaContext, node: UMethod?): Boolean { + var superClass = node?.containingClass?.superClass + while (superClass != null) { + if (isStub(context, superClass)) return true + superClass = superClass.superClass } - return node.containingClass?.extendsList?.referenceElements?.any { - it.referenceName == CLASS_STUB - } ?: false + return false } + +fun isStub(context: JavaContext, psiClass: PsiClass?): Boolean { + if (psiClass == null) return false + if (psiClass.name != "Stub") return false + if (!context.evaluator.isStatic(psiClass)) return false + if (!context.evaluator.isAbstract(psiClass)) return false + + if (!hasSingleAncestor(psiClass.extendsList, BINDER_CLASS)) return false + + val parent = psiClass.parent as? PsiClass ?: return false + if (!hasSingleAncestor(parent.extendsList, IINTERFACE_INTERFACE)) return false + + val parentName = parent.qualifiedName ?: return false + if (!hasSingleAncestor(psiClass.implementsList, parentName)) return false + + return true +} + +private fun hasSingleAncestor(references: PsiReferenceList?, qualifiedName: String) = + references != null && + references.referenceElements.size == 1 && + references.referenceElements[0].qualifiedName == qualifiedName diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorTest.kt index 4ed68a8791ed8..f7560a712f707 100644 --- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorTest.kt +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorTest.kt @@ -326,7 +326,7 @@ class EnforcePermissionDetectorTest : LintDetectorTest() { // A service with permission annotation on the method. private val interfaceIFooMethodStub: TestFile = java( """ - public interface IFooMethod { + public interface IFooMethod extends android.os.IInterface { public static abstract class Stub extends android.os.Binder implements IFooMethod { @Override @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) @@ -361,7 +361,7 @@ class EnforcePermissionDetectorTest : LintDetectorTest() { // A service without any permission annotation. private val interfaceIBarStub: TestFile = java( """ - public interface IBar { + public interface IBar extends android.os.IInterface { public static abstract class Stub extends android.os.Binder implements IBar { @Override public void testMethod() {} diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt index df7ebd7e1e7aa..10a6e1da91dc4 100644 --- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt @@ -22,7 +22,9 @@ import com.android.tools.lint.checks.infrastructure.TestLintTask class EnforcePermissionHelperDetectorTest : LintDetectorTest() { override fun getDetector() = EnforcePermissionHelperDetector() override fun getIssues() = listOf( - EnforcePermissionHelperDetector.ISSUE_ENFORCE_PERMISSION_HELPER) + EnforcePermissionHelperDetector.ISSUE_ENFORCE_PERMISSION_HELPER, + EnforcePermissionHelperDetector.ISSUE_MISUSING_ENFORCE_PERMISSION + ) override fun lint(): TestLintTask = super.lint().allowMissingSdk() @@ -47,7 +49,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] + src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] @Override ^ 1 errors, 0 warnings @@ -55,9 +57,9 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { ) .expectFixDiffs( """ - Autofix for src/Foo.java line 5: Replace with super.test_enforcePermission();...: + Autofix for src/Foo.java line 5: Replace with test_enforcePermission();...: @@ -8 +8 - + super.test_enforcePermission(); + + test_enforcePermission(); + """ ) @@ -85,7 +87,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] + src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] @Override ^ 1 errors, 0 warnings @@ -93,9 +95,9 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { ) .expectFixDiffs( """ - Autofix for src/Foo.java line 5: Replace with super.test_enforcePermission();...: + Autofix for src/Foo.java line 5: Replace with test_enforcePermission();...: @@ -8 +8 - + super.test_enforcePermission(); + + test_enforcePermission(); + """ ) @@ -120,7 +122,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] + src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] @Override ^ 1 errors, 0 warnings @@ -172,29 +174,29 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .expectClean() } - fun testInterfaceDefaultMethod_wouldStillReport() { + fun testInterfaceDefaultMethod_notStubAncestor_error() { lint().files( - java( - """ - public interface IProtected extends android.os.IInterface { - @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - default void PermissionProtected() throws android.os.RemoteException { - String foo = "bar"; - } + java( + """ + public interface IProtected extends android.os.IInterface { + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + default void PermissionProtected() throws android.os.RemoteException { + String foo = "bar"; } - """ - ).indented(), - *stubs + } + """ + ).indented(), + *stubs ) - .run() - .expect( - """ - src/IProtected.java:2: Error: Method must start with super.PermissionProtected_enforcePermission() or super.PermissionProtected() [MissingEnforcePermissionHelper] - @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - ^ - 1 errors, 0 warnings - """ - ) + .run() + .expect( + """ + src/IProtected.java:2: Error: The class of PermissionProtected does not inherit from an AIDL generated Stub class [MisusingEnforcePermissionAnnotation] + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + ^ + 1 errors, 0 warnings + """ + ) } fun testInheritance_callSuper_okay() { @@ -343,7 +345,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .run() .expect( """ - src/test/Bar.java:4: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] + src/test/Bar.java:4: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] @Override ^ 1 errors, 0 warnings @@ -399,7 +401,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .run() .expect( """ - src/test/Baz.java:4: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] + src/test/Baz.java:4: Error: Method must start with test_enforcePermission() or super.test(), if applicable [MissingEnforcePermissionHelper] @Override ^ 1 errors, 0 warnings @@ -407,6 +409,31 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { ) } + fun testRandomClass_notStubAncestor_error() { + lint().files( + java( + """ + public class Foo { + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + void PermissionProtected() throws android.os.RemoteException { + String foo = "bar"; + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:2: Error: The class of PermissionProtected does not inherit from an AIDL generated Stub class [MisusingEnforcePermissionAnnotation] + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + ^ + 1 errors, 0 warnings + """ + ) + } + companion object { val stubs = arrayOf(aidlStub, contextStub, binderStub) }