Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit eb133bf3 authored by Thiébaud Weksteen's avatar Thiébaud Weksteen
Browse files

Ignore superMethods from non-Stub parents 

In commit 9252e5ce, the logic did not ensure that the super method
belonged to the Stub class, and not any arbitrary method in a parent.

Refactor EnforcePermissionUtils by:
 - Removing isContainedInSubclassOfStub() in favour of containingStub(),
   which returns the Stub PsiClass. Document that this method does not
   mean that the argument is necessary an AIDL-generated method.
 - Update getContainingAidlInterface() to pass the PsiClass to
   findSuperMethod. This ensures that only the Stub class and its
   parents are considered.
 - Drop the check for IINTERFACE_INTERFACE. This is already verified in
   the inner call to isStub().

The same logic is applied manually to EnforcePermissionDetector, as each
condition in getContainingAidlInterface() raises a different error
message.

Add a test to confirm the behaviour of EnforcePermissionDetector.

Bug: 307433823
Test: atest --host AndroidGlobalLintCheckerTest
Test: atest --host AndroidFrameworkLintCheckerTest
Test: enforce_permission_counter
Change-Id: If791b6d8741e5db483589446484bb68061b67b70
parent 253698da

tools/lint/common/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt

+15 −17
Original line number Diff line number Diff line
@@ -24,33 +24,31 @@ import com.intellij.psi.PsiReferenceList 
import org.jetbrains.uast.UMethod



/**

 * Given a UMethod, determine if this method is

 * the entrypoint to an interface generated by AIDL,

 * returning the interface name if so, otherwise returning null

 * Given a UMethod, determine if this method is the entrypoint to an interface

 * generated by AIDL, returning the interface name if so, otherwise returning

 * 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) {

            if (extendsInterface.qualifiedName == IINTERFACE_INTERFACE) {

                return superMethod.containingClass?.name

            }

        }

    }

    return null

    val containingStub = containingStub(context, node) ?: return null

    val superMethod = node.findSuperMethods(containingStub)

    if (superMethod.isEmpty()) return null

    return containingStub.containingClass?.name

}



fun isContainedInSubclassOfStub(context: JavaContext, node: UMethod?): Boolean {

/* Returns the containing Stub class if any. This is not sufficient to infer

 * that the method itself extends an AIDL generated method. See

 * getContainingAidlInterface for that purpose.

 */

fun containingStub(context: JavaContext, node: UMethod?): PsiClass? {

    var superClass = node?.containingClass?.superClass

    while (superClass != null) {

        if (isStub(context, superClass)) return true

        if (isStub(context, superClass)) return superClass

        superClass = superClass.superClass

    }

    return false

    return null

}



fun isStub(context: JavaContext, psiClass: PsiClass?): Boolean {

private 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

tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionDetector.kt

+4 −3
Original line number Diff line number Diff line
@@ -168,7 +168,7 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { 
            annotationInfo.origin == AnnotationOrigin.METHOD) {

            /* Ignore implementations that are not a sub-class of Stub (i.e., Proxy). */

            val uMethod = element as? UMethod ?: return

            if (!isContainedInSubclassOfStub(context, uMethod)) {

            if (getContainingAidlInterface(context, uMethod) == null) {

                return

            }

            val overridingMethod = element.sourcePsi as PsiMethod
@@ -184,7 +184,8 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { 
            if (context.evaluator.isAbstract(node)) return

            if (!node.hasAnnotation(ANNOTATION_ENFORCE_PERMISSION)) return



            if (!isContainedInSubclassOfStub(context, node)) {

            val stubClass = containingStub(context, node)

            if (stubClass == null) {

                context.report(

                    ISSUE_MISUSING_ENFORCE_PERMISSION,

                    node,
@@ -196,7 +197,7 @@ class EnforcePermissionDetector : Detector(), SourceCodeScanner { 


            /* Check that we are connected to the super class */

            val overridingMethod = node as PsiMethod

            val parents = overridingMethod.findSuperMethods()

            val parents = overridingMethod.findSuperMethods(stubClass)

            if (parents.isEmpty()) {

                context.report(

                    ISSUE_MISUSING_ENFORCE_PERMISSION,

tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionDetectorTest.kt

+23 −0
Original line number Diff line number Diff line
@@ -176,6 +176,29 @@ class EnforcePermissionDetectorTest : LintDetectorTest() { 
                """.addLineContinuation())

    }



    fun testDetectNoIssuesAnnotationOnNonStubMethod() {

        lint().files(java(

            """

            package test.pkg;

            public class TestClass43 extends IFooMethod.Stub {

                public void aRegularMethodNotPartOfStub() {

                }

            }

            """).indented(), java(

            """

            package test.pkg;

            public class TestClass44 extends TestClass43 {

              @Override

              public void aRegularMethodNotPartOfStub() {

              }

            }

            """).indented(),

                *stubs

        )

        .run()

        .expectClean()

    }



    fun testDetectIssuesEmptyAnnotationOnMethod() {

        lint().files(java(

            """