diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionFix.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionFix.kt index 9ef6da14877e3..485765ba90909 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionFix.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionFix.kt @@ -20,6 +20,7 @@ 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.android.tools.lint.detector.api.UastLintUtils.Companion.getAnnotationBooleanValue +import com.android.tools.lint.detector.api.UastLintUtils.Companion.getAnnotationStringValues import com.android.tools.lint.detector.api.findSelector import com.android.tools.lint.detector.api.getUMethod import com.google.android.lint.findCallExpression @@ -32,6 +33,8 @@ import org.jetbrains.kotlin.psi.psiUtil.parameterIndex import org.jetbrains.uast.UBinaryExpression import org.jetbrains.uast.UBlockExpression import org.jetbrains.uast.UCallExpression +import org.jetbrains.uast.UExpression +import org.jetbrains.uast.UExpressionList import org.jetbrains.uast.UIfExpression import org.jetbrains.uast.UThrowExpression import org.jetbrains.uast.UastBinaryOperator @@ -41,16 +44,12 @@ import org.jetbrains.uast.visitor.AbstractUastVisitor /** * Helper class that facilitates the creation of lint auto fixes - * - * Handles "Single" permission checks that should be migrated to @EnforcePermission(...), as well as consecutive checks - * that should be migrated to @EnforcePermission(allOf={...}) - * - * TODO: handle anyOf style annotations */ data class EnforcePermissionFix( val locations: List, val permissionNames: List, val errorLevel: Boolean, + val anyOf: Boolean, ) { fun toLintFix(annotationLocation: Location): LintFix { val removeFixes = this.locations.map { @@ -76,8 +75,13 @@ data class EnforcePermissionFix( get() { val quotedPermissions = permissionNames.joinToString(", ") { """"$it"""" } + val attributeName = + if (permissionNames.size > 1) { + if (anyOf) "anyOf" else "allOf" + } else null + val annotationParameter = - if (permissionNames.size > 1) "allOf={$quotedPermissions}" + if (attributeName != null) "$attributeName={$quotedPermissions}" else quotedPermissions return "@$ANNOTATION_ENFORCE_PERMISSION($annotationParameter)" @@ -96,10 +100,12 @@ data class EnforcePermissionFix( val annotation = getPermissionMethodAnnotation(method) ?: return null val returnsVoid = method.returnType == PsiType.VOID val orSelf = getAnnotationBooleanValue(annotation, "orSelf") ?: false + val anyOf = getAnnotationBooleanValue(annotation, "anyOf") ?: false return EnforcePermissionFix( listOf(getPermissionCheckLocation(context, callExpression)), getPermissionCheckValues(callExpression), - errorLevel = isErrorLevel(throws = returnsVoid, orSelf = orSelf) + errorLevel = isErrorLevel(throws = returnsVoid, orSelf = orSelf), + anyOf, ) } @@ -155,21 +161,31 @@ data class EnforcePermissionFix( } val orSelf = getAnnotationBooleanValue(permissionMethodAnnotation, "orSelf") ?: false + val anyOf = getAnnotationBooleanValue(permissionMethodAnnotation, "anyOf") ?: false return EnforcePermissionFix( listOf(context.getLocation(ifExpression)), getPermissionCheckValues(callExpression), errorLevel = isErrorLevel(throws = true, orSelf = orSelf), + anyOf = anyOf ) } - fun compose(individuals: List): EnforcePermissionFix = - EnforcePermissionFix( - individuals.flatMap { it.locations }, - individuals.flatMap { it.permissionNames }, - errorLevel = individuals.all(EnforcePermissionFix::errorLevel) + fun compose(individuals: List): EnforcePermissionFix { + val anyOfs = individuals.filter(EnforcePermissionFix::anyOf) + // anyOf/allOf should be consistent. If we encounter some @PermissionMethods that are anyOf + // and others that aren't, we don't know what to do. + if (anyOfs.isNotEmpty() && anyOfs.size < individuals.size) { + throw AnyOfAllOfException() + } + return EnforcePermissionFix( + individuals.flatMap(EnforcePermissionFix::locations), + individuals.flatMap(EnforcePermissionFix::permissionNames), + errorLevel = individuals.all(EnforcePermissionFix::errorLevel), + anyOf = anyOfs.isNotEmpty() ) + } /** * Given a permission check, get its proper location @@ -195,6 +211,7 @@ data class EnforcePermissionFix( * and pull out the permission value(s) being used. Also evaluates nested calls * to @PermissionMethod(s) in the given method's body. */ + @Throws(AnyOfAllOfException::class) private fun getPermissionCheckValues( callExpression: UCallExpression ): List { @@ -204,38 +221,94 @@ data class EnforcePermissionFix( val visitedCalls = mutableSetOf() // don't visit the same call twice val bfsQueue = ArrayDeque(listOf(callExpression)) - // Breadth First Search - evalutaing nested @PermissionMethod(s) in the available + var anyOfAllOfState: AnyOfAllOfState = AnyOfAllOfState.INITIAL + + // Bread First Search - evaluating nested @PermissionMethod(s) in the available // source code for @PermissionName(s). while (bfsQueue.isNotEmpty()) { - val current = bfsQueue.removeFirst() - visitedCalls.add(current) - result.addAll(findPermissions(current)) + val currentCallExpression = bfsQueue.removeFirst() + visitedCalls.add(currentCallExpression) + val currentPermissions = findPermissions(currentCallExpression) + result.addAll(currentPermissions) - current.resolve()?.getUMethod()?.accept(object : AbstractUastVisitor() { - override fun visitCallExpression(node: UCallExpression): Boolean { - if (isPermissionMethodCall(node) && node !in visitedCalls) { - bfsQueue.add(node) - } - return false - } - }) + val currentAnnotation = getPermissionMethodAnnotation( + currentCallExpression.resolve()?.getUMethod()) + val currentAnyOf = getAnnotationBooleanValue(currentAnnotation, "anyOf") ?: false + + // anyOf/allOf should be consistent. If we encounter a nesting of @PermissionMethods + // where we start in an anyOf state and switch to allOf, or vice versa, + // we don't know what to do. + if (anyOfAllOfState == AnyOfAllOfState.INITIAL) { + if (currentAnyOf) anyOfAllOfState = AnyOfAllOfState.ANY_OF + else if (result.isNotEmpty()) anyOfAllOfState = AnyOfAllOfState.ALL_OF + } + + if (anyOfAllOfState == AnyOfAllOfState.ALL_OF && currentAnyOf) { + throw AnyOfAllOfException() + } + + if (anyOfAllOfState == AnyOfAllOfState.ANY_OF && + !currentAnyOf && currentPermissions.size > 1) { + throw AnyOfAllOfException() + } + + currentCallExpression.resolve()?.getUMethod() + ?.accept(PermissionCheckValuesVisitor(visitedCalls, bfsQueue)) } return result.toList() } + private enum class AnyOfAllOfState { + INITIAL, + ANY_OF, + ALL_OF + } + + /** + * Adds visited permission method calls to the provided + * queue in support of the BFS traversal happening while + * this is used + */ + private class PermissionCheckValuesVisitor( + val visitedCalls: Set, + val bfsQueue: ArrayDeque + ) : AbstractUastVisitor() { + override fun visitCallExpression(node: UCallExpression): Boolean { + if (isPermissionMethodCall(node) && node !in visitedCalls) { + bfsQueue.add(node) + } + return false + } + } + private fun findPermissions( callExpression: UCallExpression, ): List { - val indices = callExpression.resolve()?.getUMethod() - ?.uastParameters - ?.filter(::hasPermissionNameAnnotation) - ?.mapNotNull { it.sourcePsi?.parameterIndex() } - ?: emptyList() + val annotation = getPermissionMethodAnnotation(callExpression.resolve()?.getUMethod()) - return indices.mapNotNull { - callExpression.getArgumentForParameter(it)?.evaluateString() - } + val hardCodedPermissions = (getAnnotationStringValues(annotation, "value") + ?: emptyArray()) + .toList() + + val indices = callExpression.resolve()?.getUMethod() + ?.uastParameters + ?.filter(::hasPermissionNameAnnotation) + ?.mapNotNull { it.sourcePsi?.parameterIndex() } + ?: emptyList() + + val argPermissions = indices + .flatMap { i -> + when (val argument = callExpression.getArgumentForParameter(i)) { + null -> listOf(null) + is UExpressionList -> // varargs e.g. someMethod(String...) + argument.expressions.map(UExpression::evaluateString) + else -> listOf(argument.evaluateString()) + } + } + .filterNotNull() + + return hardCodedPermissions + argPermissions } /** @@ -247,3 +320,11 @@ data class EnforcePermissionFix( private fun isErrorLevel(throws: Boolean, orSelf: Boolean): Boolean = throws && orSelf } } +/** + * anyOf/allOf @PermissionMethods must be consistent to apply @EnforcePermission - + * meaning if we encounter some @PermissionMethods that are anyOf, and others are allOf, + * we don't know which to apply. + */ +class AnyOfAllOfException : Exception() { + override val message: String = "anyOf/allOf permission methods cannot be mixed" +} diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt index 46517e1166a68..11a283af30495 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetector.kt @@ -79,22 +79,28 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { * as some other business logic is happening that prevents an automated fix. */ private fun accumulateSimplePermissionCheckFixes( - methodBody: UBlockExpression, - context: JavaContext - ): - EnforcePermissionFix? { - val singleFixes = mutableListOf() - for (expression in methodBody.expressions) { - singleFixes.add(getPermissionCheckFix(expression.skipParenthesizedExprDown(), context) - ?: break) - } - return when (singleFixes.size) { - 0 -> null - 1 -> singleFixes[0] - else -> EnforcePermissionFix.compose(singleFixes) + methodBody: UBlockExpression, + context: JavaContext + ): EnforcePermissionFix? { + try { + val singleFixes = mutableListOf() + for (expression in methodBody.expressions) { + val fix = getPermissionCheckFix( + expression.skipParenthesizedExprDown(), + context) ?: break + singleFixes.add(fix) + } + return when (singleFixes.size) { + 0 -> null + 1 -> singleFixes[0] + else -> EnforcePermissionFix.compose(singleFixes) + } + } catch (e: AnyOfAllOfException) { + return null } } + /** * If an expression boils down to a permission check, return * the helper for creating a lint auto fix to @EnforcePermission diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt index 5ab964d8efdd3..2ac550bd468aa 100644 --- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/SimpleManualPermissionEnforcementDetectorTest.kt @@ -675,6 +675,118 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .expectClean() } + fun testAnyOf_hardCodedAndVarArgs() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.content.pm.PermissionMethod(anyOf = true) + private void helperHelper() { + helper("FOO", "BAR"); + } + + @android.content.pm.PermissionMethod(anyOf = true, value = {"BAZ", "BUZZ"}) + private void helper(@android.content.pm.PermissionName String... extraPermissions) {} + + @Override + public void test() throws android.os.RemoteException { + helperHelper(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:17: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + helperHelper(); + ~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 17: Annotate with @EnforcePermission: + @@ -15 +15 + + @android.annotation.EnforcePermission(anyOf={"BAZ", "BUZZ", "FOO", "BAR"}) + @@ -17 +18 + - helperHelper(); + """ + ) + } + + + fun testAnyOfAllOf_mixedConsecutiveCalls_ignored() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.content.pm.PermissionMethod + private void allOfhelper() { + mContext.enforceCallingOrSelfPermission("FOO"); + mContext.enforceCallingOrSelfPermission("BAR"); + } + + @android.content.pm.PermissionMethod(anyOf = true, permissions = {"BAZ", "BUZZ"}) + private void anyOfHelper() {} + + @Override + public void test() throws android.os.RemoteException { + allOfhelper(); + anyOfHelper(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testAnyOfAllOf_mixedNestedCalls_ignored() { + lint().files( + java( + """ + import android.content.Context; + import android.content.pm.PermissionName;import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.content.pm.PermissionMethod(anyOf = true) + private void anyOfCheck(@PermissionName String... permissions) { + allOfCheck("BAZ", "BUZZ"); + } + + @android.content.pm.PermissionMethod + private void allOfCheck(@PermissionName String... permissions) {} + + @Override + public void test() throws android.os.RemoteException { + anyOfCheck("FOO", "BAR"); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + companion object { val stubs = arrayOf( aidlStub,