From d27d379efd63040e8d2bd686ce35d21bb61f928a Mon Sep 17 00:00:00 2001 From: mattgilbride Date: Mon, 2 Jan 2023 20:24:55 +0000 Subject: [PATCH] EnforcePermissionFix: refactor for greater separation of concern and reuse Move more logic for identifying/creating an instance of EnforcePermissionFix from the related detector to the class itself. These changes also make it easier to consume this code from experimental projects. This change also enables AndroidGlobalLintCheckerTest, and adds/fixes test cases. AndroidGlobalLintCheckerTest will now run as part of presubmit. Bug: 232058525 Bug: 240445172 Test: atest --host AndroidGlobalLintCheckerTest Change-Id: Ice19df391e9a308f3d2a6ebf2b5ba2f8ad5bb00b --- tools/lint/global/Android.bp | 20 ++-- .../android/lint/aidl/EnforcePermissionFix.kt | 45 ++++++++- ...mpleManualPermissionEnforcementDetector.kt | 58 +----------- ...ManualPermissionEnforcementDetectorTest.kt | 57 ++++++++--- .../com/google/android/lint/aidl/Stubs.kt | 94 +++++++++---------- 5 files changed, 149 insertions(+), 125 deletions(-) diff --git a/tools/lint/global/Android.bp b/tools/lint/global/Android.bp index 3756abea23301..bedb7bd78a29f 100644 --- a/tools/lint/global/Android.bp +++ b/tools/lint/global/Android.bp @@ -38,12 +38,6 @@ java_library_host { java_test_host { name: "AndroidGlobalLintCheckerTest", - // TODO(b/239881504): Since this test was written, Android - // Lint was updated, and now includes classes that were - // compiled for java 15. The soong build doesn't support - // java 15 yet, so we can't compile against "lint". Disable - // the test until java 15 is supported. - enabled: false, srcs: ["checks/src/test/java/**/*.kt"], static_libs: [ "AndroidGlobalLintChecker", @@ -53,5 +47,19 @@ java_test_host { ], test_options: { unit_test: true, + tradefed_options: [ + { + // lint bundles in some classes that were built with older versions + // of libraries, and no longer load. Since tradefed tries to load + // all classes in the jar to look for tests, it crashes loading them. + // Exclude these classes from tradefed's search. + name: "exclude-paths", + value: "org/apache", + }, + { + name: "exclude-paths", + value: "META-INF", + }, + ], }, } 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 39d6eb22ef20b..25d208db14ec8 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 @@ -97,6 +97,48 @@ data class EnforcePermissionFix( } companion object { + /** + * Walks the expressions in a block, looking for simple permission checks. + * + * As soon as something other than a permission check is encountered, stop looking, + * as some other business logic is happening that prevents an automated fix. + */ + fun fromBlockExpression( + context: JavaContext, + blockExpression: UBlockExpression + ): EnforcePermissionFix? { + try { + val singleFixes = mutableListOf() + for (expression in blockExpression.expressions) { + val fix = fromExpression(context, expression) ?: break + singleFixes.add(fix) + } + return compose(singleFixes) + } catch (e: AnyOfAllOfException) { + return null + } + } + + /** + * Conditionally constructs EnforcePermissionFix from any UExpression + * + * @return EnforcePermissionFix if the expression boils down to a permission check, + * else null + */ + fun fromExpression( + context: JavaContext, + expression: UExpression + ): EnforcePermissionFix? { + val trimmedExpression = expression.skipParenthesizedExprDown() + if (trimmedExpression is UIfExpression) { + return fromIfExpression(context, trimmedExpression) + } + findCallExpression(trimmedExpression)?.let { + return fromCallExpression(context, it) + } + return null + } + /** * Conditionally constructs EnforcePermissionFix from a UCallExpression * @@ -183,7 +225,8 @@ data class EnforcePermissionFix( } - fun compose(individuals: List): EnforcePermissionFix { + fun compose(individuals: List): EnforcePermissionFix? { + if (individuals.isEmpty()) return null 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. 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 44afb1a7e60b2..c7be36efd9914 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 @@ -23,19 +23,13 @@ import com.android.tools.lint.detector.api.Issue import com.android.tools.lint.detector.api.JavaContext import com.android.tools.lint.detector.api.Scope import com.android.tools.lint.detector.api.Severity -import com.google.android.lint.findCallExpression import org.jetbrains.uast.UBlockExpression -import org.jetbrains.uast.UElement -import org.jetbrains.uast.UIfExpression import org.jetbrains.uast.UMethod -import org.jetbrains.uast.skipParenthesizedExprDown /** * Looks for methods implementing generated AIDL interface stubs * that can have simple permission checks migrated to * @EnforcePermission annotations - * - * TODO: b/242564870 (enable parse and autoFix of .aidl files) */ @Suppress("UnstableApiUsage") class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { @@ -45,7 +39,7 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { interfaceName: String, body: UBlockExpression ) { - val enforcePermissionFix = accumulateSimplePermissionCheckFixes(body, context) ?: return + val enforcePermissionFix = EnforcePermissionFix.fromBlockExpression(context, body) ?: return val lintFix = enforcePermissionFix.toLintFix(context, node) val message = "$interfaceName permission check ${ @@ -67,56 +61,6 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { context.report(incident) } - /** - * Walk the expressions in the method, looking for simple permission checks. - * - * If a single permission check is found at the beginning of the method, - * this should be migrated to @EnforcePermission(value). - * - * If multiple consecutive permission checks are found, - * these should be migrated to @EnforcePermission(allOf={value1, value2, ...}) - * - * As soon as something other than a permission check is encountered, stop looking, - * as some other business logic is happening that prevents an automated fix. - */ - private fun accumulateSimplePermissionCheckFixes( - 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 - */ - private fun getPermissionCheckFix(startingExpression: UElement?, context: JavaContext): - EnforcePermissionFix? { - if (startingExpression is UIfExpression) { - return EnforcePermissionFix.fromIfExpression(context, startingExpression) - } - findCallExpression(startingExpression)?.let { - return EnforcePermissionFix.fromCallExpression(context, it) - } - return null - } - companion object { private val EXPLANATION = """ 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 c4dfa04c8d71a..6b8e72cf92221 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 @@ -397,7 +397,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod(orSelf = true) + @android.annotation.PermissionMethod(orSelf = true) private void helper() { mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); } @@ -442,7 +442,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod + @android.annotation.PermissionMethod private void helper() { mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); } @@ -487,7 +487,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod(orSelf = true) + @android.annotation.PermissionMethod(orSelf = true) private void helper() { mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); mContext.enforceCallingOrSelfPermission("android.permission.WRITE_CONTACTS", "foo"); @@ -536,13 +536,13 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod(orSelf = true) + @android.annotation.PermissionMethod(orSelf = true) private void helperHelper() { helper("android.permission.WRITE_CONTACTS"); } - @android.content.pm.PermissionMethod(orSelf = true) - private void helper(@android.content.pm.PermissionName String extraPermission) { + @android.annotation.PermissionMethod(orSelf = true) + private void helper(@android.annotation.PermissionName String extraPermission) { mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); } @@ -689,6 +689,34 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .expectClean() } + fun testIfExpression_inlinedWithSideEffect_ignored() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + public void test() throws android.os.RemoteException { + if (somethingElse() && mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + != PackageManager.PERMISSION_GRANTED) { + throw new SecurityException("yikes!"); + } + } + + private boolean somethingElse() { + return true; + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + fun testAnyOf_hardCodedAndVarArgs() { lint().files( java( @@ -699,13 +727,13 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod(anyOf = true) + @android.annotation.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) {} + @android.annotation.PermissionMethod(anyOf = true, value = {"BAZ", "BUZZ"}) + private void helper(@android.annotation.PermissionName String... extraPermissions) {} @Override public void test() throws android.os.RemoteException { @@ -748,13 +776,13 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod + @android.annotation.PermissionMethod private void allOfhelper() { mContext.enforceCallingOrSelfPermission("FOO"); mContext.enforceCallingOrSelfPermission("BAR"); } - @android.content.pm.PermissionMethod(anyOf = true, permissions = {"BAZ", "BUZZ"}) + @android.annotation.PermissionMethod(anyOf = true, permissions = {"BAZ", "BUZZ"}) private void anyOfHelper() {} @Override @@ -776,17 +804,18 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { java( """ import android.content.Context; - import android.content.pm.PermissionName;import android.test.ITest; + import android.annotation.PermissionName; + import android.test.ITest; public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod(anyOf = true) + @android.annotation.PermissionMethod(anyOf = true) private void anyOfCheck(@PermissionName String... permissions) { allOfCheck("BAZ", "BUZZ"); } - @android.content.pm.PermissionMethod + @android.annotation.PermissionMethod private void allOfCheck(@PermissionName String... permissions) {} @Override diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt index f6e58da78e662..2ec8fddbb4e9e 100644 --- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt @@ -5,84 +5,84 @@ import com.android.tools.lint.checks.infrastructure.TestFile val aidlStub: TestFile = java( """ - package android.test; - public interface ITest extends android.os.IInterface { - public static abstract class Stub extends android.os.Binder implements android.test.ITest { - protected void test_enforcePermission() throws SecurityException {} - } - public void test() throws android.os.RemoteException; + package android.test; + public interface ITest extends android.os.IInterface { + public static abstract class Stub extends android.os.Binder implements android.test.ITest { + protected void test_enforcePermission() throws SecurityException {} } + public void test() throws android.os.RemoteException; + } """ ).indented() val contextStub: TestFile = java( """ - package android.content; - public class Context { - @android.content.pm.PermissionMethod(orSelf = true) - public void enforceCallingOrSelfPermission(@android.content.pm.PermissionName String permission, String message) {} - @android.content.pm.PermissionMethod - public void enforceCallingPermission(@android.content.pm.PermissionName String permission, String message) {} - @android.content.pm.PermissionMethod(orSelf = true) - public int checkCallingOrSelfPermission(@android.content.pm.PermissionName String permission, String message) {} - @android.content.pm.PermissionMethod - public int checkCallingPermission(@android.content.pm.PermissionName String permission, String message) {} - } + package android.content; + public class Context { + @android.annotation.PermissionMethod(orSelf = true) + public void enforceCallingOrSelfPermission(@android.annotation.PermissionName String permission, String message) {} + @android.annotation.PermissionMethod + public void enforceCallingPermission(@android.annotation.PermissionName String permission, String message) {} + @android.annotation.PermissionMethod(orSelf = true) + public int checkCallingOrSelfPermission(@android.annotation.PermissionName String permission, String message) {} + @android.annotation.PermissionMethod + public int checkCallingPermission(@android.annotation.PermissionName String permission, String message) {} + } """ ).indented() val binderStub: TestFile = java( """ - package android.os; - public class Binder { - public static int getCallingUid() {} - } + package android.os; + public class Binder { + public static int getCallingUid() {} + } """ ).indented() val permissionMethodStub: TestFile = java( -""" - package android.content.pm; + """ + package android.annotation; - import static java.lang.annotation.ElementType.METHOD; - import static java.lang.annotation.RetentionPolicy.CLASS; + import static java.lang.annotation.ElementType.METHOD; + import static java.lang.annotation.RetentionPolicy.CLASS; - import java.lang.annotation.Retention; - import java.lang.annotation.Target; + import java.lang.annotation.Retention; + import java.lang.annotation.Target; - @Retention(CLASS) - @Target({METHOD}) - public @interface PermissionMethod {} + @Retention(CLASS) + @Target({METHOD}) + public @interface PermissionMethod {} """ ).indented() val permissionNameStub: TestFile = java( -""" - package android.content.pm; + """ + package android.annotation; - import static java.lang.annotation.ElementType.FIELD; - import static java.lang.annotation.ElementType.LOCAL_VARIABLE; - import static java.lang.annotation.ElementType.METHOD; - import static java.lang.annotation.ElementType.PARAMETER; - import static java.lang.annotation.RetentionPolicy.CLASS; + import static java.lang.annotation.ElementType.FIELD; + import static java.lang.annotation.ElementType.LOCAL_VARIABLE; + import static java.lang.annotation.ElementType.METHOD; + import static java.lang.annotation.ElementType.PARAMETER; + import static java.lang.annotation.RetentionPolicy.CLASS; - import java.lang.annotation.Retention; - import java.lang.annotation.Target; + import java.lang.annotation.Retention; + import java.lang.annotation.Target; - @Retention(CLASS) - @Target({PARAMETER, METHOD, LOCAL_VARIABLE, FIELD}) - public @interface PermissionName {} + @Retention(CLASS) + @Target({PARAMETER, METHOD, LOCAL_VARIABLE, FIELD}) + public @interface PermissionName {} """ ).indented() val manifestStub: TestFile = java( """ - package android; + package android; - public final class Manifest { - public static final class permission { - public static final String READ_CONTACTS="android.permission.READ_CONTACTS"; - } + public final class Manifest { + public static final class permission { + public static final String READ_CONTACTS="android.permission.READ_CONTACTS"; } + } """.trimIndent() ) \ No newline at end of file