diff options
5 files changed, 269 insertions, 33 deletions
diff --git a/tools/lint/common/src/main/java/com/google/android/lint/PermissionMethodUtils.kt b/tools/lint/common/src/main/java/com/google/android/lint/PermissionMethodUtils.kt index 720f8356f050..01575963951b 100644 --- a/tools/lint/common/src/main/java/com/google/android/lint/PermissionMethodUtils.kt +++ b/tools/lint/common/src/main/java/com/google/android/lint/PermissionMethodUtils.kt @@ -17,6 +17,7 @@ package com.google.android.lint import com.android.tools.lint.detector.api.getUMethod +import org.jetbrains.uast.UAnnotation import org.jetbrains.uast.UCallExpression import org.jetbrains.uast.UMethod import org.jetbrains.uast.UParameter @@ -26,10 +27,11 @@ fun isPermissionMethodCall(callExpression: UCallExpression): Boolean { return hasPermissionMethodAnnotation(method) } -fun hasPermissionMethodAnnotation(method: UMethod): Boolean = method.annotations - .any { - it.hasQualifiedName(ANNOTATION_PERMISSION_METHOD) - } +fun hasPermissionMethodAnnotation(method: UMethod): Boolean = + getPermissionMethodAnnotation(method) != null + +fun getPermissionMethodAnnotation(method: UMethod?): UAnnotation? = method?.uAnnotations + ?.firstOrNull { it.qualifiedName == ANNOTATION_PERMISSION_METHOD } fun hasPermissionNameAnnotation(parameter: UParameter) = parameter.annotations.any { it.hasQualifiedName(ANNOTATION_PERMISSION_NAME) 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 5c177d59e349..ee7dd62aaa36 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 @@ -19,9 +19,12 @@ 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.android.tools.lint.detector.api.UastLintUtils.Companion.getAnnotationBooleanValue import com.android.tools.lint.detector.api.getUMethod +import com.google.android.lint.getPermissionMethodAnnotation import com.google.android.lint.hasPermissionNameAnnotation import com.google.android.lint.isPermissionMethodCall +import com.intellij.psi.PsiType import org.jetbrains.kotlin.psi.psiUtil.parameterIndex import org.jetbrains.uast.UCallExpression import org.jetbrains.uast.evaluateString @@ -37,7 +40,8 @@ import org.jetbrains.uast.visitor.AbstractUastVisitor */ data class EnforcePermissionFix( val locations: List<Location>, - val permissionNames: List<String> + val permissionNames: List<String>, + val errorLevel: Boolean, ) { fun toLintFix(annotationLocation: Location): LintFix { val removeFixes = this.locations.map { @@ -78,19 +82,28 @@ data class EnforcePermissionFix( fun fromCallExpression( context: JavaContext, callExpression: UCallExpression - ): EnforcePermissionFix? = - if (isPermissionMethodCall(callExpression)) { - EnforcePermissionFix( + ): EnforcePermissionFix? { + val method = callExpression.resolve()?.getUMethod() ?: return null + val annotation = getPermissionMethodAnnotation(method) ?: return null + val enforces = method.returnType == PsiType.VOID + val orSelf = getAnnotationBooleanValue(annotation, "orSelf") ?: false + return EnforcePermissionFix( listOf(getPermissionCheckLocation(context, callExpression)), - getPermissionCheckValues(callExpression) - ) - } else null + getPermissionCheckValues(callExpression), + // If we detect that the PermissionMethod enforces that permission is granted, + // AND is of the "orSelf" variety, we are very confident that this is a behavior + // preserving migration to @EnforcePermission. Thus, the incident should be ERROR + // level. + errorLevel = enforces && orSelf + ) + } fun compose(individuals: List<EnforcePermissionFix>): EnforcePermissionFix = EnforcePermissionFix( individuals.flatMap { it.locations }, - individuals.flatMap { it.permissionNames } + individuals.flatMap { it.permissionNames }, + errorLevel = individuals.all(EnforcePermissionFix::errorLevel) ) /** 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 8ff813e4663e..9999a0ba7e06 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 @@ -18,6 +18,7 @@ package com.google.android.lint.aidl import com.android.tools.lint.detector.api.Category import com.android.tools.lint.detector.api.Implementation +import com.android.tools.lint.detector.api.Incident import com.android.tools.lint.detector.api.Issue import com.android.tools.lint.detector.api.JavaContext import com.android.tools.lint.detector.api.Scope @@ -48,14 +49,22 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { val enforcePermissionFix = accumulateSimplePermissionCheckFixes(body, context) ?: return val lintFix = enforcePermissionFix.toLintFix(context.getLocation(node)) val message = - "$interfaceName permission check can be converted to @EnforcePermission annotation" + "$interfaceName permission check ${ + if (enforcePermissionFix.errorLevel) "should" else "can" + } be converted to @EnforcePermission annotation" - context.report( + val incident = Incident( ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT, enforcePermissionFix.locations.last(), message, lintFix ) + + if (enforcePermissionFix.errorLevel) { + incident.overrideSeverity(Severity.ERROR) + } + + context.report(incident) } /** 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 50d5081c4b0d..bdf9c897b946 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: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:7: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 0 errors, 1 warnings + 1 errors, 0 warnings """ ) .expectFixDiffs( @@ -68,6 +68,80 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { ) } + fun testClass_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 { + mContext.enforceCallingPermission("android.permission.READ_CONTACTS", "foo"); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.enforceCallingPermission("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 + - mContext.enforceCallingPermission("android.permission.READ_CONTACTS", "foo"); + """ + ) + } + + fun testClass_enforcesFalse_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 { + mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + mContext.checkCallingOrSelfPermission("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 + - mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + """ + ) + } + fun testAnonClass() { lint().files( java( @@ -91,10 +165,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:8: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:8: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission( ^ - 0 errors, 1 warnings + 1 errors, 0 warnings """ ) .expectFixDiffs( @@ -131,10 +205,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:8: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:8: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission(android.Manifest.permission.READ_CONTACTS, "foo"); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 0 errors, 1 warnings + 1 errors, 0 warnings """ ) .expectFixDiffs( @@ -173,10 +247,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:10: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission( ^ - 0 errors, 1 warnings + 1 errors, 0 warnings """ ) .expectFixDiffs( @@ -193,6 +267,96 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { ) } + 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 + ) + .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"); + """ + ) + } + + 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 + ) + .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"); + """ + ) + } + fun testPrecedingExpressions() { lint().files( java( @@ -225,7 +389,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod + @android.content.pm.PermissionMethod(orSelf = true) private void helper() { mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); } @@ -242,10 +406,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:14: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:14: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] helper(); ~~~~~~~~~ - 0 errors, 1 warnings + 1 errors, 0 warnings """ ) .expectFixDiffs( @@ -259,6 +423,50 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { ) } + fun testPermissionHelper_orSelfNotBubbledUp_warning() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + + public class Foo extends ITest.Stub { + private Context mContext; + + @android.content.pm.PermissionMethod + private void helper() { + mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); + } + + @Override + public void test() throws android.os.RemoteException { + helper(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/Foo.java:14: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + helper(); + ~~~~~~~~~ + 0 errors, 1 warnings + """ + ) + .expectFixDiffs( + """ + Fix for src/Foo.java line 14: Annotate with @EnforcePermission: + @@ -12 +12 + + @android.annotation.EnforcePermission("android.permission.READ_CONTACTS") + @@ -14 +15 + - helper(); + """ + ) + } + fun testPermissionHelperAllOf() { lint().files( java( @@ -269,7 +477,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod + @android.content.pm.PermissionMethod(orSelf = true) private void helper() { mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); mContext.enforceCallingOrSelfPermission("android.permission.WRITE_CONTACTS", "foo"); @@ -288,10 +496,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:16: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:16: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] mContext.enforceCallingOrSelfPermission("FOO", "foo"); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 0 errors, 1 warnings + 1 errors, 0 warnings """ ) .expectFixDiffs( @@ -317,12 +525,12 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { public class Foo extends ITest.Stub { private Context mContext; - @android.content.pm.PermissionMethod + @android.content.pm.PermissionMethod(orSelf = true) private void helperHelper() { helper("android.permission.WRITE_CONTACTS"); } - @android.content.pm.PermissionMethod + @android.content.pm.PermissionMethod(orSelf = true) private void helper(@android.content.pm.PermissionName String extraPermission) { mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo"); } @@ -339,10 +547,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:19: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] + src/Foo.java:19: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement] helperHelper(); ~~~~~~~~~~~~~~~ - 0 errors, 1 warnings + 1 errors, 0 warnings """ ) .expectFixDiffs( 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 bd6b1952847c..5ac8a0ba2604 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 @@ -17,8 +17,12 @@ val contextStub: TestFile = java( """ package android.content; public class Context { - @android.content.pm.PermissionMethod + @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) {} } """ ).indented() |