From d7cbe7d90a597c1aff37561e3825942811222b77 Mon Sep 17 00:00:00 2001 From: mattgilbride Date: Thu, 29 Dec 2022 17:47:05 +0000 Subject: [PATCH] EnforcePermissionHelperDetector: allow ancestors to call super instead of helper Bug: 261976627 Test: EnforcePermissionHelperDetectorTest Change-Id: I006e8f391e2d97bc568db59d26ece704f12e1b2e --- .../aidl/EnforcePermissionHelperDetector.kt | 38 +-- .../EnforcePermissionHelperDetectorTest.kt | 240 +++++++++++++++++- .../com/google/android/lint/aidl/Stubs.kt | 4 +- 3 files changed, 260 insertions(+), 22 deletions(-) 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 c1e47e906e224..268c565b65ffe 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 @@ -25,12 +25,14 @@ 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.android.tools.lint.detector.api.SourceCodeScanner +import com.google.android.lint.findCallExpression import com.intellij.psi.PsiElement import org.jetbrains.uast.UBlockExpression import org.jetbrains.uast.UDeclarationsExpression import org.jetbrains.uast.UElement import org.jetbrains.uast.UExpression import org.jetbrains.uast.UMethod +import org.jetbrains.uast.skipParenthesizedExprDown class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner { override fun getApplicableUastTypes(): List> = @@ -43,34 +45,35 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner { if (context.evaluator.isAbstract(node)) return if (!node.hasAnnotation(ANNOTATION_ENFORCE_PERMISSION)) return - val targetExpression = "super.${node.name}$HELPER_SUFFIX()" + val targetExpression = "${node.name}$HELPER_SUFFIX()" + val message = "Method must start with $targetExpression or super.${node.name}()" - val body = node.uastBody as? UBlockExpression - if (body == null) { - context.report( - ISSUE_ENFORCE_PERMISSION_HELPER, - context.getLocation(node), - "Method must start with $targetExpression", - ) - return - } + val firstExpression = (node.uastBody as? UBlockExpression) + ?.expressions?.firstOrNull() - val firstExpression = body.expressions.firstOrNull() if (firstExpression == null) { context.report( ISSUE_ENFORCE_PERMISSION_HELPER, context.getLocation(node), - "Method must start with $targetExpression", + message, ) return } - val firstExpressionSource = firstExpression.asSourceString() - .filterNot(Char::isWhitespace) + val firstExpressionSource = firstExpression.skipParenthesizedExprDown() + .asSourceString() + .filterNot(Char::isWhitespace) + + if (firstExpressionSource != targetExpression && + firstExpressionSource != "super.$targetExpression") { + // calling super.() is also legal + val directSuper = context.evaluator.getSuperMethod(node) + val firstCall = findCallExpression(firstExpression)?.resolve() + if (directSuper != null && firstCall == directSuper) return - if (firstExpressionSource != targetExpression) { val locationTarget = getLocationTarget(firstExpression) val expressionLocation = context.getLocation(locationTarget) + val indent = " ".repeat(expressionLocation.start?.column ?: 0) val fix = fix() @@ -85,7 +88,7 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner { context.report( ISSUE_ENFORCE_PERMISSION_HELPER, context.getLocation(node), - "Method must start with $targetExpression", + message, fix ) } @@ -99,7 +102,8 @@ class EnforcePermissionHelperDetector : Detector(), SourceCodeScanner { When @EnforcePermission is applied, the AIDL compiler generates a Stub method to do the permission check called yourMethodName$HELPER_SUFFIX. - You must call this method as the first expression in your implementation. + yourMethodName$HELPER_SUFFIX must be executed before any other operation. To do that, you can + either call it directly or indirectly via super.yourMethodName(). """ val ISSUE_ENFORCE_PERMISSION_HELPER: Issue = Issue.create( diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt index 4799184b0e239..df7ebd7e1e7aa 100644 --- a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt @@ -47,7 +47,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:5: Error: Method must start with super.test_enforcePermission() [MissingEnforcePermissionHelper] + src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] @Override ^ 1 errors, 0 warnings @@ -85,7 +85,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:5: Error: Method must start with super.test_enforcePermission() [MissingEnforcePermissionHelper] + src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] @Override ^ 1 errors, 0 warnings @@ -120,7 +120,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .run() .expect( """ - src/Foo.java:5: Error: Method must start with super.test_enforcePermission() [MissingEnforcePermissionHelper] + src/Foo.java:5: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] @Override ^ 1 errors, 0 warnings @@ -150,6 +150,28 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .expectClean() } + fun testHelperWithoutSuperPrefix_Okay() { + lint().files( + java( + """ + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + test_enforcePermission(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + fun testInterfaceDefaultMethod_wouldStillReport() { lint().files( java( @@ -167,7 +189,7 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { .run() .expect( """ - src/IProtected.java:2: Error: Method must start with super.PermissionProtected_enforcePermission() [MissingEnforcePermissionHelper] + src/IProtected.java:2: Error: Method must start with super.PermissionProtected_enforcePermission() or super.PermissionProtected() [MissingEnforcePermissionHelper] @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) ^ 1 errors, 0 warnings @@ -175,6 +197,216 @@ class EnforcePermissionHelperDetectorTest : LintDetectorTest() { ) } + fun testInheritance_callSuper_okay() { + lint().files( + java( + """ + package test; + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Foo; + public class Bar extends Foo { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Bar; + public class Baz extends Bar { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testInheritance_callHelper_okay() { + lint().files( + java( + """ + package test; + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Foo; + public class Bar extends Foo { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Bar; + public class Baz extends Bar { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testInheritance_missingCallInChain_error() { + lint().files( + java( + """ + package test; + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Foo; + public class Bar extends Foo { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + doStuff(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Bar; + public class Baz extends Bar { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/test/Bar.java:4: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] + @Override + ^ + 1 errors, 0 warnings + """ + ) + } + + fun testInheritance_missingCall_error() { + lint().files( + java( + """ + package test; + import android.content.Context; + import android.test.ITest; + public class Foo extends ITest.Stub { + private Context mContext; + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test_enforcePermission(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Foo; + public class Bar extends Foo { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + super.test(); + } + } + """ + ).indented(), + java( + """ + package test; + import test.Bar; + public class Baz extends Bar { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void test() throws android.os.RemoteException { + doStuff(); + } + } + """ + ).indented(), + *stubs + ) + .run() + .expect( + """ + src/test/Baz.java:4: Error: Method must start with test_enforcePermission() or super.test() [MissingEnforcePermissionHelper] + @Override + ^ + 1 errors, 0 warnings + """ + ) + } + companion object { val stubs = arrayOf(aidlStub, contextStub, binderStub) } 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 362ac61ff6e8f..f6e58da78e662 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 @@ -7,7 +7,9 @@ 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 {} + 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; } """