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/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..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 @@ -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 @@ -88,8 +97,51 @@ 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 + * * @return EnforcePermissionFix if the called method is annotated with @PermissionMethod, else null */ fun fromCallExpression( @@ -111,6 +163,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 @@ -172,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. @@ -180,7 +234,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..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,8 +39,8 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { interfaceName: String, body: UBlockExpression ) { - val enforcePermissionFix = accumulateSimplePermissionCheckFixes(body, context) ?: return - val lintFix = enforcePermissionFix.toLintFix(context.getLocation(node)) + val enforcePermissionFix = EnforcePermissionFix.fromBlockExpression(context, body) ?: return + val lintFix = enforcePermissionFix.toLintFix(context, node) val message = "$interfaceName permission check ${ if (enforcePermissionFix.errorLevel) "should" else "can" @@ -54,68 +48,19 @@ 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) } - /** - * 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 = """ @@ -142,7 +87,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..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 @@ -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() { @@ -389,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"); } @@ -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(); """ ) } @@ -433,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"); } @@ -463,6 +472,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") @@ -14 +15 - helper(); + + test_enforcePermission(); """ ) } @@ -477,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"); @@ -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(); """ ) } @@ -525,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"); } @@ -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,106 @@ 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 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() { @@ -685,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 { @@ -718,6 +760,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { + @android.annotation.EnforcePermission(anyOf={"BAZ", "BUZZ", "FOO", "BAR"}) @@ -17 +18 - helperHelper(); + + test_enforcePermission(); """ ) } @@ -733,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 @@ -761,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