From 003fbc74f4a7a508ffb0746a4dfe4f9e52100413 Mon Sep 17 00:00:00 2001 From: mattgilbride Date: Thu, 29 Dec 2022 19:16:22 +0000 Subject: [PATCH 1/2] 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 --- .../com/google/android/lint/aidl/Constants.kt | 2 + .../android/lint/aidl/EnforcePermissionFix.kt | 39 ++- .../aidl/EnforcePermissionHelperDetector.kt | 15 +- .../lint/aidl/EnforcePermissionUtils.kt | 25 ++ ...mpleManualPermissionEnforcementDetector.kt | 12 +- ...ManualPermissionEnforcementDetectorTest.kt | 317 +++++++++--------- 6 files changed, 226 insertions(+), 184 deletions(-) diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt index 72de00f885ba2..dcfbe953f955b 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/Constants.kt @@ -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 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 485765ba90909..39d6eb22ef20b 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 @@ -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, + val manualCheckLocations: List, val permissionNames: List, val errorLevel: Boolean, val anyOf: Boolean, ) { - fun toLintFix(annotationLocation: Location): LintFix { - val removeFixes = this.locations.map { - LintFix.create() - .replace() - .reformat(true) - .range(it) - .with("") - .autoFix() - .build() + 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(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() diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt index b65c0fc6a7db7..df13af516514c 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt @@ -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), ) } } diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt index 2239ea169632d..d41fee3fc0dc8 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt @@ -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() +} 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 11a283af30495..44afb1a7e60b2 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 @@ -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 ) } } 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 2ac550bd468aa..c4dfa04c8d71a 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 @@ -51,10 +51,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:7: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:7: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 1 errors, 0 warnings + 0 errors, 1 warnings """ ) .expectFixDiffs( @@ -64,6 +64,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") @@ -7 +8 - mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + + test_enforcePermission(); """ ) } @@ -101,6 +102,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") @@ -7 +8 - mContext.enforceCallingPermission("android.permission.READ_CONTACTS", "foo"); + + test_enforcePermission(); """ ) } @@ -138,6 +140,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") @@ -7 +8 - mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + + test_enforcePermission(); """ ) } @@ -165,10 +168,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:8: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:8: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission( ^ - 1 errors, 0 warnings + 0 errors, 1 warnings """ ) .expectFixDiffs( @@ -179,6 +182,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { @@ -8 +9 - mContext.enforceCallingOrSelfPermission( - "android.permission.READ_CONTACTS", "foo"); + + test_enforcePermission(); """ ) } @@ -205,19 +209,20 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:8: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:8: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission(android.Manifest.permission.READ_CONTACTS, "foo"); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 1 errors, 0 warnings + 0 errors, 1 warnings """ ) .expectFixDiffs( """ - Fix for src/Foo.java line 7: Annotate with @EnforcePermission: + Fix for src/Foo.java line 8: Annotate with @EnforcePermission: @@ -6 +6 + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") @@ -8 +9 - mContext.enforceCallingOrSelfPermission(android.Manifest.permission.READ_CONTACTS, "foo"); + + test_enforcePermission(); """ ) } @@ -247,10 +252,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:10: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:10: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission( ^ - 1 errors, 0 warnings + 0 errors, 1 warnings """ ) .expectFixDiffs( @@ -263,98 +268,101 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { - "android.permission.READ_CONTACTS", "foo"); - mContext.enforceCallingOrSelfPermission( - "android.permission.WRITE_CONTACTS", "foo"); + + test_enforcePermission(); """ ) } fun testAllOf_mixedOrSelf_warning() { lint().files( - java( - """ - import android.content.Context; - import android.test.ITest; - public class Foo { - private Context mContext; - private ITest itest = new ITest.Stub() { - @Override - public void test() throws android.os.RemoteException { - mContext.enforceCallingOrSelfPermission( - "android.permission.READ_CONTACTS", "foo"); - mContext.enforceCallingPermission( - "android.permission.WRITE_CONTACTS", "foo"); - } - }; - } - """ - ).indented(), - *stubs + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo { + private Context mContext; + private ITest itest = new ITest.Stub() { + @Override + public void test() throws android.os.RemoteException { + mContext.enforceCallingOrSelfPermission( + "android.permission.READ_CONTACTS", "foo"); + mContext.enforceCallingPermission( + "android.permission.WRITE_CONTACTS", "foo"); + } + }; + } + """ + ).indented(), + *stubs ) - .run() - .expect( - """ - src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] - mContext.enforceCallingPermission( - ^ - 0 errors, 1 warnings - """ - ) - .expectFixDiffs( - """ - Fix for src/Foo.java line 10: Annotate with @EnforcePermission: - @@ -6 +6 - + @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS"}) - @@ -8 +9 - - mContext.enforceCallingOrSelfPermission( - - "android.permission.READ_CONTACTS", "foo"); - - mContext.enforceCallingPermission( - - "android.permission.WRITE_CONTACTS", "foo"); - """ - ) + .run() + .expect( + """ + src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.enforceCallingPermission( + ^ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 10: Annotate with @EnforcePermission: + @@ -6 +6 + + @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS"}) + @@ -8 +9 + - mContext.enforceCallingOrSelfPermission( + - "android.permission.READ_CONTACTS", "foo"); + - mContext.enforceCallingPermission( + - "android.permission.WRITE_CONTACTS", "foo"); + + test_enforcePermission(); + """ + ) } fun testAllOf_mixedEnforces_warning() { lint().files( - java( - """ - import android.content.Context; - import android.test.ITest; - public class Foo { - private Context mContext; - private ITest itest = new ITest.Stub() { - @Override - public void test() throws android.os.RemoteException { - mContext.enforceCallingOrSelfPermission( - "android.permission.READ_CONTACTS", "foo"); - mContext.checkCallingOrSelfPermission( - "android.permission.WRITE_CONTACTS", "foo"); - } - }; - } - """ - ).indented(), - *stubs + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo { + private Context mContext; + private ITest itest = new ITest.Stub() { + @Override + public void test() throws android.os.RemoteException { + mContext.enforceCallingOrSelfPermission( + "android.permission.READ_CONTACTS", "foo"); + mContext.checkCallingOrSelfPermission( + "android.permission.WRITE_CONTACTS", "foo"); + } + }; + } + """ + ).indented(), + *stubs ) - .run() - .expect( - """ - src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] - mContext.checkCallingOrSelfPermission( - ^ - 0 errors, 1 warnings - """ - ) - .expectFixDiffs( - """ - Fix for src/Foo.java line 10: Annotate with @EnforcePermission: - @@ -6 +6 - + @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS"}) - @@ -8 +9 - - mContext.enforceCallingOrSelfPermission( - - "android.permission.READ_CONTACTS", "foo"); - - mContext.checkCallingOrSelfPermission( - - "android.permission.WRITE_CONTACTS", "foo"); - """ - ) + .run() + .expect( + """ + src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.checkCallingOrSelfPermission( + ^ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 10: Annotate with @EnforcePermission: + @@ -6 +6 + + @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS"}) + @@ -8 +9 + - mContext.enforceCallingOrSelfPermission( + - "android.permission.READ_CONTACTS", "foo"); + - mContext.checkCallingOrSelfPermission( + - "android.permission.WRITE_CONTACTS", "foo"); + + test_enforcePermission(); + """ + ) } fun testPrecedingExpressions() { @@ -406,10 +414,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:14: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:14: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] helper(); ~~~~~~~~~ - 1 errors, 0 warnings + 0 errors, 1 warnings """ ) .expectFixDiffs( @@ -419,6 +427,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") @@ -14 +15 - helper(); + + test_enforcePermission(); """ ) } @@ -463,6 +472,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") @@ -14 +15 - helper(); + + test_enforcePermission(); """ ) } @@ -496,10 +506,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:16: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:16: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission("FOO", "foo"); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 1 errors, 0 warnings + 0 errors, 1 warnings """ ) .expectFixDiffs( @@ -510,6 +520,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { @@ -15 +16 - helper(); - mContext.enforceCallingOrSelfPermission("FOO", "foo"); + + test_enforcePermission(); """ ) } @@ -547,10 +558,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:19: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:19: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] helperHelper(); ~~~~~~~~~~~~~~~ - 1 errors, 0 warnings + 0 errors, 1 warnings """ ) .expectFixDiffs( @@ -560,6 +571,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + @android.annotation.EnforcePermission(allOf={"android.permission.WRITE_CONTACTS", "android.permission.READ_CONTACTS"}) @@ -19 +20 - helperHelper(); + + test_enforcePermission(); """ ) } @@ -587,10 +599,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:7: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:7: Warning: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] if (mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo") ^ - 1 errors, 0 warnings + 0 errors, 1 warnings """ ) .expectFixDiffs( @@ -603,76 +615,78 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { - != PackageManager.PERMISSION_GRANTED) { - throw new SecurityException("yikes!"); - } + + test_enforcePermission(); """ ) } fun testIfExpression_orSelfFalse_warning() { 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 (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") - != PackageManager.PERMISSION_GRANTED) { - throw new SecurityException("yikes!"); - } + 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 (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + != PackageManager.PERMISSION_GRANTED) { + throw new SecurityException("yikes!"); } } - """ - ).indented(), - *stubs + } + """ + ).indented(), + *stubs ) - .run() - .expect( - """ - src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] - if (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") - ^ - 0 errors, 1 warnings - """ - ) - .expectFixDiffs( - """ - Fix for src/Foo.java line 7: Annotate with @EnforcePermission: - @@ -5 +5 - + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") - @@ -7 +8 - - if (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") - - != PackageManager.PERMISSION_GRANTED) { - - throw new SecurityException("yikes!"); - - } - """ - ) + .run() + .expect( + """ + src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + if (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + ^ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 7: Annotate with @EnforcePermission: + @@ -5 +5 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -7 +8 + - if (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + - != PackageManager.PERMISSION_GRANTED) { + - throw new SecurityException("yikes!"); + - } + + test_enforcePermission(); + """ + ) } fun testIfExpression_otherSideEffect_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 (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") - != PackageManager.PERMISSION_GRANTED) { - doSomethingElse(); - throw new SecurityException("yikes!"); - } + 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 (mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo") + != PackageManager.PERMISSION_GRANTED) { + doSomethingElse(); + throw new SecurityException("yikes!"); } } - """ - ).indented(), - *stubs + } + """ + ).indented(), + *stubs ) - .run() - .expectClean() + .run() + .expectClean() } fun testAnyOf_hardCodedAndVarArgs() { @@ -718,6 +732,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + @android.annotation.EnforcePermission(anyOf={"BAZ", "BUZZ", "FOO", "BAR"}) @@ -17 +18 - helperHelper(); + + test_enforcePermission(); """ ) } From d27d379efd63040e8d2bd686ce35d21bb61f928a Mon Sep 17 00:00:00 2001 From: mattgilbride Date: Mon, 2 Jan 2023 20:24:55 +0000 Subject: [PATCH 2/2] 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