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

Commit 003fbc74 authored by mattgilbride's avatar mattgilbride
Browse files

SimpleManualPermissionEnforcementDetector: suggest calling helper method and turn on 

The previous fix only suggested removing the manual permission
enforcement. Suggest replacing with the required helper method call.

Also turn this detector on, but reporting all incidents at WARNING level.
This will surface the call sites that need migration in the errorprone
build without actually causing failure. Once all call sites that would
cause an ERROR level incident have been migrated, we can turn that
functionality back on (b/265014041).

Bug: 261976627
Test: SimpleManualPermissionEnforcementDetectorTest

Change-Id: I9e2e6d1981d9a8dc59b9c8078d7ce80b1d0b10ab
parent 7535df42

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

+2 −0
Original line number Diff line number Diff line
@@ -29,6 +29,8 @@ val AIDL_PERMISSION_ANNOTATIONS = listOf( 
const val BINDER_CLASS = "android.os.Binder"

const val IINTERFACE_INTERFACE = "android.os.IInterface"



const val AIDL_PERMISSION_HELPER_SUFFIX = "_enforcePermission"



/**

 * If a non java (e.g. c++) backend is enabled, the @EnforcePermission

 * annotation cannot be used.  At time of writing, the mechanism

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

+25 −14
Original line number Diff line number Diff line
@@ -36,6 +36,7 @@ import org.jetbrains.uast.UCallExpression 
import org.jetbrains.uast.UExpression

import org.jetbrains.uast.UExpressionList

import org.jetbrains.uast.UIfExpression

import org.jetbrains.uast.UMethod

import org.jetbrains.uast.UThrowExpression

import org.jetbrains.uast.UastBinaryOperator

import org.jetbrains.uast.evaluateString
@@ -46,29 +47,37 @@ import org.jetbrains.uast.visitor.AbstractUastVisitor 
 * Helper class that facilitates the creation of lint auto fixes

 */

data class EnforcePermissionFix(

    val locations: List<Location>,

    val manualCheckLocations: List<Location>,

    val permissionNames: List<String>,

    val errorLevel: Boolean,

    val anyOf: Boolean,

) {

    fun toLintFix(annotationLocation: Location): LintFix {

        val removeFixes = this.locations.map {

    fun toLintFix(context: JavaContext, node: UMethod): LintFix {

        val methodLocation = context.getLocation(node)

        val replaceOrRemoveFixes = manualCheckLocations.mapIndexed { index, manualCheckLocation ->

            if (index == 0) {

                // Replace the first manual check with a call to the helper method

                getHelperMethodFix(node, manualCheckLocation, false)

            } else {

                // Remove all subsequent manual checks

                LintFix.create()

                    .replace()

                    .reformat(true)

                .range(it)

                    .range(manualCheckLocation)

                    .with("")

                    .autoFix()

                    .build()

            }

        }



        // Annotate the method with @EnforcePermission(...)

        val annotateFix = LintFix.create()

            .annotate(this.annotation)

            .range(annotationLocation)

            .annotate(annotation)

            .range(methodLocation)

            .autoFix()

            .build()



        return LintFix.create().composite(annotateFix, *removeFixes.toTypedArray())

        return LintFix.create().composite(annotateFix, *replaceOrRemoveFixes.toTypedArray())

    }



    private val annotation: String
@@ -90,6 +99,7 @@ data class EnforcePermissionFix( 
    companion object {

        /**

         * Conditionally constructs EnforcePermissionFix from a UCallExpression

         *

         * @return EnforcePermissionFix if the called method is annotated with @PermissionMethod, else null

         */

        fun fromCallExpression(
@@ -111,6 +121,7 @@ data class EnforcePermissionFix( 


        /**

         * Conditionally constructs EnforcePermissionFix from a UCallExpression

         *

         * @return EnforcePermissionFix IF AND ONLY IF:

         * * The condition of the if statement compares the return value of a

         *   PermissionMethod to one of the PackageManager.PermissionResult values
@@ -180,7 +191,7 @@ data class EnforcePermissionFix( 
                throw AnyOfAllOfException()

            }

            return EnforcePermissionFix(

                    individuals.flatMap(EnforcePermissionFix::locations),

                    individuals.flatMap(EnforcePermissionFix::manualCheckLocations),

                    individuals.flatMap(EnforcePermissionFix::permissionNames),

                    errorLevel = individuals.all(EnforcePermissionFix::errorLevel),

                    anyOf = anyOfs.isNotEmpty()

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

+2 −13
Original line number Diff line number Diff line
@@ -55,7 +55,7 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner { 
                return

            }



            val targetExpression = "${node.name}$HELPER_SUFFIX()"

            val targetExpression = getHelperMethodCallSourceString(node)

            val message =

                "Method must start with $targetExpression or super.${node.name}(), if applicable"
@@ -85,22 +85,11 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner { 
                val locationTarget = getLocationTarget(firstExpression)

                val expressionLocation = context.getLocation(locationTarget)



                val indent = " ".repeat(expressionLocation.start?.column ?: 0)



                val fix = fix()

                    .replace()

                    .range(expressionLocation)

                    .beginning()

                    .with("$targetExpression;\n\n$indent")

                    .reformat(true)

                    .autoFix()

                    .build()



                context.report(

                    ISSUE_ENFORCE_PERMISSION_HELPER,

                    context.getLocation(node),

                    message,

                    fix

                    getHelperMethodFix(node, expressionLocation),

                )

            }

        }

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

+25 −0
Original line number Diff line number Diff line
@@ -17,6 +17,8 @@ 
package com.google.android.lint.aidl



import com.android.tools.lint.detector.api.JavaContext

import com.android.tools.lint.detector.api.LintFix

import com.android.tools.lint.detector.api.Location

import com.intellij.psi.PsiClass

import com.intellij.psi.PsiReferenceList

import org.jetbrains.uast.UMethod
@@ -69,3 +71,26 @@ private fun hasSingleAncestor(references: PsiReferenceList?, qualifiedName: Stri 
        references != null &&

                references.referenceElements.size == 1 &&

                references.referenceElements[0].qualifiedName == qualifiedName



fun getHelperMethodCallSourceString(node: UMethod) = "${node.name}$AIDL_PERMISSION_HELPER_SUFFIX()"



fun getHelperMethodFix(

    node: UMethod,

    manualCheckLocation: Location,

    prepend: Boolean = true

): LintFix {

    val helperMethodSource = getHelperMethodCallSourceString(node)

    val indent = " ".repeat(manualCheckLocation.start?.column ?: 0)

    val newText = "$helperMethodSource;${if (prepend) "\n\n$indent" else ""}"



    val fix = LintFix.create()

            .replace()

            .range(manualCheckLocation)

            .with(newText)

            .reformat(true)

            .autoFix()



    if (prepend) fix.beginning()



    return fix.build()

}

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

+6 −6
Original line number Diff line number Diff line
@@ -46,7 +46,7 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { 
            body: UBlockExpression

    ) {

        val enforcePermissionFix = accumulateSimplePermissionCheckFixes(body, context) ?: return

        val lintFix = enforcePermissionFix.toLintFix(context.getLocation(node))

        val lintFix = enforcePermissionFix.toLintFix(context, node)

        val message =

                "$interfaceName permission check ${

                    if (enforcePermissionFix.errorLevel) "should" else "can"
@@ -54,14 +54,15 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { 


        val incident = Incident(

                ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT,

                enforcePermissionFix.locations.last(),

                enforcePermissionFix.manualCheckLocations.last(),

                message,

                lintFix

        )



        if (enforcePermissionFix.errorLevel) {

            incident.overrideSeverity(Severity.ERROR)

        }

        // TODO(b/265014041): turn on errors once all code that would cause one is fixed

        // if (enforcePermissionFix.errorLevel) {

        //     incident.overrideSeverity(Severity.ERROR)

        // }



        context.report(incident)

    }
@@ -142,7 +143,6 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { 
                        SimpleManualPermissionEnforcementDetector::class.java,

                        Scope.JAVA_FILE_SCOPE

                ),

                enabledByDefault = false, // TODO: enable once b/241171714 is resolved

        )

    }

}