EnforcePermissionHelperDetector: @EP can only be used in Stub ancestors

Add helpers to identify the generated Stub class as well as methods that
inherit from it. Use those helpers where relevant, including
EnforcePermissionHelperDetector.

Bug: 261974131
Test: EnforcePermissionHelperDetectorTest

Change-Id: I68e572e6a8558be966450fcbf75eabbdb7ac6ae7

7 files changed, 140 insertions, 103 deletions

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 227cdcdc2fec..ab6d871d6ea6 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 8ee3763e5079..72de00f885ba 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 2665b3c9ec20..0baac2c7aacf 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<String> {
         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 268c565b65ff..b65c0fc6a7db 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 250ca78bae5e..2239ea169632 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 4ed68a8791ed..f7560a712f70 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 df7ebd7e1e7a..10a6e1da91dc 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)
     }