Merge "SimpleManualPermissionEnforcementDetector: conditional ERROR level incident"

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()