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