diff options
11 files changed, 263 insertions, 167 deletions
diff --git a/services/accessibility/Android.bp b/services/accessibility/Android.bp index 3d7ad0b94d1b..34b9a5db580f 100644 --- a/services/accessibility/Android.bp +++ b/services/accessibility/Android.bp @@ -19,11 +19,6 @@ java_library_static { defaults: [ "platform_service_defaults", ], - lint: { - error_checks: ["MissingPermissionAnnotation"], - baseline_filename: "lint-baseline.xml", - - }, srcs: [ ":services.accessibility-sources", ":statslog-accessibility-java-gen", diff --git a/services/accessibility/java/com/android/server/accessibility/AccessibilityInputFilter.java b/services/accessibility/java/com/android/server/accessibility/AccessibilityInputFilter.java index 1b2447e2c58a..617cca9d3075 100644 --- a/services/accessibility/java/com/android/server/accessibility/AccessibilityInputFilter.java +++ b/services/accessibility/java/com/android/server/accessibility/AccessibilityInputFilter.java @@ -67,7 +67,6 @@ import java.util.StringJoiner; * * NOTE: This class has to be created and poked only from the main thread. */ -@SuppressWarnings("MissingPermissionAnnotation") class AccessibilityInputFilter extends InputFilter implements EventStreamTransformation { private static final String TAG = AccessibilityInputFilter.class.getSimpleName(); diff --git a/services/accessibility/java/com/android/server/accessibility/FingerprintGestureDispatcher.java b/services/accessibility/java/com/android/server/accessibility/FingerprintGestureDispatcher.java index e10e87c51d59..c9ec16edc54e 100644 --- a/services/accessibility/java/com/android/server/accessibility/FingerprintGestureDispatcher.java +++ b/services/accessibility/java/com/android/server/accessibility/FingerprintGestureDispatcher.java @@ -33,7 +33,6 @@ import java.util.List; /** * Encapsulate fingerprint gesture logic */ -@SuppressWarnings("MissingPermissionAnnotation") public class FingerprintGestureDispatcher extends IFingerprintClientActiveCallback.Stub implements Handler.Callback{ private static final int MSG_REGISTER = 1; diff --git a/tools/lint/common/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt b/tools/lint/common/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt index f5af99ec39ac..b79563f740ee 100644 --- a/tools/lint/common/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt +++ b/tools/lint/common/src/main/java/com/google/android/lint/aidl/EnforcePermissionUtils.kt @@ -103,3 +103,26 @@ fun getHelperMethodFix( return fix.build() } + +/** + * PermissionAnnotationDetector uses this method to determine whether a specific file should be + * checked for unannotated methods. Only files located in directories whose paths begin with one + * of these prefixes will be considered. + */ +fun isSystemServicePath(context: JavaContext): Boolean { + val systemServicePathPrefixes = setOf( + "frameworks/base/services", + "frameworks/base/apex", + "frameworks/opt/wear", + "packages/modules" + ) + + val filePath = context.file.path + + // We perform `filePath.contains` instead of `filePath.startsWith` since getting the + // relative path of a source file is non-trivial. That is because `context.file.path` + // returns the path to where soong builds the file (i.e. /out/soong/...). Moreover, the + // logic to extract the relative path would need to consider several /out/soong/... + // locations patterns. + return systemServicePathPrefixes.any { filePath.contains(it) } +} diff --git a/tools/lint/framework/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt b/tools/lint/framework/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt index 5c6469706e18..af753e5963a3 100644 --- a/tools/lint/framework/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt +++ b/tools/lint/framework/checks/src/main/java/com/google/android/lint/AndroidFrameworkIssueRegistry.kt @@ -20,7 +20,6 @@ import com.android.tools.lint.client.api.IssueRegistry import com.android.tools.lint.client.api.Vendor import com.android.tools.lint.detector.api.CURRENT_API import com.google.android.lint.parcel.SaferParcelChecker -import com.google.android.lint.aidl.PermissionAnnotationDetector import com.google.auto.service.AutoService @AutoService(IssueRegistry::class) @@ -38,7 +37,6 @@ class AndroidFrameworkIssueRegistry : IssueRegistry() { SaferParcelChecker.ISSUE_UNSAFE_API_USAGE, // TODO: Currently crashes due to OOM issue // PackageVisibilityDetector.ISSUE_PACKAGE_NAME_NO_PACKAGE_VISIBILITY_FILTERS, - PermissionAnnotationDetector.ISSUE_MISSING_PERMISSION_ANNOTATION, PermissionMethodDetector.ISSUE_PERMISSION_METHOD_USAGE, PermissionMethodDetector.ISSUE_CAN_BE_PERMISSION_METHOD, FeatureAutomotiveDetector.ISSUE, diff --git a/tools/lint/framework/checks/src/test/java/com/google/android/lint/PermissionAnnotationDetectorTest.kt b/tools/lint/framework/checks/src/test/java/com/google/android/lint/PermissionAnnotationDetectorTest.kt deleted file mode 100644 index bce848a2e3a7..000000000000 --- a/tools/lint/framework/checks/src/test/java/com/google/android/lint/PermissionAnnotationDetectorTest.kt +++ /dev/null @@ -1,134 +0,0 @@ -/* - * Copyright (C) 2023 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.android.lint.aidl - -import com.android.tools.lint.checks.infrastructure.LintDetectorTest -import com.android.tools.lint.checks.infrastructure.TestFile -import com.android.tools.lint.checks.infrastructure.TestLintTask -import com.android.tools.lint.detector.api.Detector -import com.android.tools.lint.detector.api.Issue - -@Suppress("UnstableApiUsage") -class PermissionAnnotationDetectorTest : LintDetectorTest() { - override fun getDetector(): Detector = PermissionAnnotationDetector() - - override fun getIssues(): List<Issue> = listOf( - PermissionAnnotationDetector.ISSUE_MISSING_PERMISSION_ANNOTATION, - ) - - override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) - - /** No issue scenario */ - - fun testDoesNotDetectIssuesInCorrectScenario() { - lint().files( - java( - """ - public class Foo extends IFoo.Stub { - @Override - @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") - public void testMethod() { } - } - """ - ).indented(), - *stubs - ) - .run() - .expectClean() - } - - fun testMissingAnnotation() { - lint().files( - java( - """ - public class Bar extends IBar.Stub { - public void testMethod() { } - } - """ - ).indented(), - *stubs - ) - .run() - .expect( - """ - src/Bar.java:2: Error: The method testMethod is not permission-annotated. [MissingPermissionAnnotation] - public void testMethod() { } - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 1 errors, 0 warnings - """ - ) - } - - fun testNoIssueWhenExtendingWithAnotherSubclass() { - lint().files( - java( - """ - public class Foo extends IFoo.Stub { - @Override - @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - public void testMethod() { } - // not an AIDL method, just another method - public void someRandomMethod() { } - } - """).indented(), - java( - """ - public class Baz extends Bar { - @Override - public void someRandomMethod() { } - } - """).indented(), - *stubs - ) - .run() - .expectClean() - } - - /* Stubs */ - - // A service with permission annotation on the method. - private val interfaceIFoo: TestFile = java( - """ - public interface IFoo extends android.os.IInterface { - public static abstract class Stub extends android.os.Binder implements IFoo { - } - @Override - @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) - public void testMethod(); - @Override - @android.annotation.RequiresNoPermission - public void testMethodNoPermission(); - @Override - @android.annotation.PermissionManuallyEnforced - public void testMethodManual(); - } - """ - ).indented() - - // A service with no permission annotation. - private val interfaceIBar: TestFile = java( - """ - public interface IBar extends android.os.IInterface { - public static abstract class Stub extends android.os.Binder implements IBar { - } - public void testMethod(); - } - """ - ).indented() - - private val stubs = arrayOf(interfaceIFoo, interfaceIBar) -} diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt index 28eab8f62e74..290e7be9f6c4 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/AndroidGlobalIssueRegistry.kt @@ -20,6 +20,7 @@ import com.android.tools.lint.client.api.IssueRegistry import com.android.tools.lint.client.api.Vendor import com.android.tools.lint.detector.api.CURRENT_API import com.google.android.lint.aidl.EnforcePermissionDetector +import com.google.android.lint.aidl.PermissionAnnotationDetector import com.google.android.lint.aidl.SimpleManualPermissionEnforcementDetector import com.google.auto.service.AutoService @@ -31,6 +32,7 @@ class AndroidGlobalIssueRegistry : IssueRegistry() { EnforcePermissionDetector.ISSUE_MISMATCHING_ENFORCE_PERMISSION, EnforcePermissionDetector.ISSUE_ENFORCE_PERMISSION_HELPER, EnforcePermissionDetector.ISSUE_MISUSING_ENFORCE_PERMISSION, + PermissionAnnotationDetector.ISSUE_MISSING_PERMISSION_ANNOTATION, SimpleManualPermissionEnforcementDetector.ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT, ) diff --git a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/ExemptAidlInterfaces.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/ExemptAidlInterfaces.kt index 8777712b0f04..675a59e6ae3e 100644 --- a/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/ExemptAidlInterfaces.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/ExemptAidlInterfaces.kt @@ -20,12 +20,8 @@ package com.google.android.lint.aidl * The exemptAidlInterfaces set was generated by running ExemptAidlInterfacesGenerator on the * entire source tree. To reproduce the results, run generate-exempt-aidl-interfaces.sh * located in tools/lint/utils. - * - * TODO: b/363248121 - Use the exemptAidlInterfaces set inside PermissionAnnotationDetector when it - * gets migrated to a global lint check. */ val exemptAidlInterfaces = setOf( - "android.accessibilityservice.IAccessibilityServiceConnection", "android.accessibilityservice.IBrailleDisplayConnection", "android.accounts.IAccountAuthenticatorResponse", "android.accounts.IAccountManager", @@ -663,10 +659,6 @@ val exemptAidlInterfaces = setOf( "android.uwb.IUwbOemExtensionCallback", "android.uwb.IUwbRangingCallbacks", "android.uwb.IUwbVendorUciCallback", - "android.view.accessibility.IAccessibilityInteractionConnectionCallback", - "android.view.accessibility.IAccessibilityManager", - "android.view.accessibility.IMagnificationConnectionCallback", - "android.view.accessibility.IRemoteMagnificationAnimationCallback", "android.view.autofill.IAutoFillManager", "android.view.autofill.IAutofillWindowPresenter", "android.view.contentcapture.IContentCaptureManager", diff --git a/tools/lint/framework/checks/src/main/java/com/google/android/lint/PermissionAnnotationDetector.kt b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/PermissionAnnotationDetector.kt index 6b50cfd9e5ab..d44c271e734c 100644 --- a/tools/lint/framework/checks/src/main/java/com/google/android/lint/PermissionAnnotationDetector.kt +++ b/tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/PermissionAnnotationDetector.kt @@ -43,8 +43,14 @@ class PermissionAnnotationDetector : AidlImplementationDetector() { interfaceName: String, body: UBlockExpression ) { + if (!isSystemServicePath(context)) return + if (context.evaluator.isAbstract(node)) return + val fullyQualifiedInterfaceName = + getContainingAidlInterfaceQualified(context, node) ?: return + if (exemptAidlInterfaces.contains(fullyQualifiedInterfaceName)) return + if (AIDL_PERMISSION_ANNOTATIONS.any { node.hasAnnotation(it) }) return context.report( @@ -80,8 +86,7 @@ class PermissionAnnotationDetector : AidlImplementationDetector() { implementation = Implementation( PermissionAnnotationDetector::class.java, Scope.JAVA_FILE_SCOPE - ), - enabledByDefault = false + ) ) } } diff --git a/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/PermissionAnnotationDetectorTest.kt b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/PermissionAnnotationDetectorTest.kt new file mode 100644 index 000000000000..92d0829911bf --- /dev/null +++ b/tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/PermissionAnnotationDetectorTest.kt @@ -0,0 +1,230 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.android.lint.aidl + +import com.android.tools.lint.checks.infrastructure.LintDetectorTest +import com.android.tools.lint.checks.infrastructure.TestFile +import com.android.tools.lint.checks.infrastructure.TestLintTask +import com.android.tools.lint.detector.api.Detector +import com.android.tools.lint.detector.api.Issue + +@Suppress("UnstableApiUsage") +class PermissionAnnotationDetectorTest : LintDetectorTest() { + override fun getDetector(): Detector = + PermissionAnnotationDetector() + + override fun getIssues(): List<Issue> = listOf( + PermissionAnnotationDetector.ISSUE_MISSING_PERMISSION_ANNOTATION, + ) + + override fun lint(): TestLintTask = super.lint().allowMissingSdk(true) + + /** No issue scenario */ + + fun testDoesNotDetectIssuesInCorrectScenario() { + lint() + .files( + java( + createVisitedPath("Foo.java"), + """ + package com.android.server; + public class Foo extends IFoo.Stub { + @Override + @android.annotation.EnforcePermission("android.Manifest.permission.READ_CONTACTS") + public void testMethod() { } + } + """ + ) + .indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testMissingAnnotation() { + lint() + .files( + java( + createVisitedPath("Bar.java"), + """ + package com.android.server; + public class Bar extends IBar.Stub { + public void testMethod() { } + } + """ + ) + .indented(), + *stubs + ) + .run() + .expect( + """ + src/frameworks/base/services/java/com/android/server/Bar.java:3: Error: The method testMethod is not permission-annotated. [MissingPermissionAnnotation] + public void testMethod() { } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + 1 errors, 0 warnings + """ + ) + } + + fun testMissingAnnotationInIgnoredDirectory() { + lint() + .files( + java( + ignoredPath, + """ + package com.android.server; + public class Bar extends IBar.Stub { + public void testMethod() { } + } + """ + ) + .indented(), + *stubs + ) + .run() + .expectClean() + } + + // If this test fails, consider the following steps: + // 1. Pick the first entry (interface) from `exemptAidlInterfaces`. + // 2. Change `interfaceIExempted` to use that interface. + // 3. Change this test's class to extend the interface's Stub. + fun testMissingAnnotationAidlInterfaceExempted() { + lint() + .files( + java( + createVisitedPath("Bar.java"), + """ + package com.android.server; + public class Bar extends android.accessibilityservice.IBrailleDisplayConnection.Stub { + public void testMethod() { } + } + """ + ) + .indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testMissingAnnotationAidlInterfaceAbstractMethod() { + lint() + .files( + java( + createVisitedPath("Bar.java"), + """ + package com.android.server; + public abstract class Bar extends IBar.Stub { + public abstract void testMethod(); + } + """ + ) + .indented(), + *stubs + ) + .run() + .expectClean() + } + + fun testNoIssueWhenExtendingWithAnotherSubclass() { + lint() + .files( + java( + createVisitedPath("Foo.java"), + """ + package com.android.server; + public class Foo extends IFoo.Stub { + @Override + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + public void testMethod() { } + // not an AIDL method, just another method + public void someRandomMethod() { } + } + """ + ) + .indented(), + java( + createVisitedPath("Baz.java"), + """ + package com.android.server; + public class Baz extends Bar { + @Override + public void someRandomMethod() { } + } + """ + ) + .indented(), + *stubs + ) + .run() + .expectClean() + } + + /* Stubs */ + + // A service with permission annotation on the method. + private val interfaceIFoo: TestFile = java( + """ + public interface IFoo extends android.os.IInterface { + public static abstract class Stub extends android.os.Binder implements IFoo { + } + @Override + @android.annotation.EnforcePermission(android.Manifest.permission.READ_PHONE_STATE) + public void testMethod(); + @Override + @android.annotation.RequiresNoPermission + public void testMethodNoPermission(); + @Override + @android.annotation.PermissionManuallyEnforced + public void testMethodManual(); + } + """ + ).indented() + + // A service with no permission annotation. + private val interfaceIBar: TestFile = java( + """ + public interface IBar extends android.os.IInterface { + public static abstract class Stub extends android.os.Binder implements IBar { + } + public void testMethod(); + } + """ + ).indented() + + // A service whose AIDL Interface is exempted. + private val interfaceIExempted: TestFile = java( + """ + package android.accessibilityservice; + public interface IBrailleDisplayConnection extends android.os.IInterface { + public static abstract class Stub extends android.os.Binder implements IBrailleDisplayConnection { + } + public void testMethod(); + } + """ + ).indented() + + private val stubs = arrayOf(interfaceIFoo, interfaceIBar, interfaceIExempted) + + private fun createVisitedPath(filename: String) = + "src/frameworks/base/services/java/com/android/server/$filename" + + private val ignoredPath = "src/test/pkg/TestClass.java" +} diff --git a/tools/lint/utils/checks/src/main/java/com/google/android/lint/aidl/ExemptAidlInterfacesGenerator.kt b/tools/lint/utils/checks/src/main/java/com/google/android/lint/aidl/ExemptAidlInterfacesGenerator.kt index 6ad223c87a29..57c2e5aa9767 100644 --- a/tools/lint/utils/checks/src/main/java/com/google/android/lint/aidl/ExemptAidlInterfacesGenerator.kt +++ b/tools/lint/utils/checks/src/main/java/com/google/android/lint/aidl/ExemptAidlInterfacesGenerator.kt @@ -33,12 +33,6 @@ import org.jetbrains.uast.UMethod */ class ExemptAidlInterfacesGenerator : AidlImplementationDetector() { private val targetExemptAidlInterfaceNames = mutableSetOf<String>() - private val systemServicePathPrefixes = setOf( - "frameworks/base/services", - "frameworks/base/apex", - "frameworks/opt/wear", - "packages/modules" - ) // We could've improved performance by visiting classes rather than methods, however, this lint // check won't be run regularly, hence we've decided not to add extra overrides to @@ -49,14 +43,7 @@ class ExemptAidlInterfacesGenerator : AidlImplementationDetector() { interfaceName: String, body: UBlockExpression ) { - val filePath = context.file.path - - // We perform `filePath.contains` instead of `filePath.startsWith` since getting the - // relative path of a source file is non-trivial. That is because `context.file.path` - // returns the path to where soong builds the file (i.e. /out/soong/...). Moreover, the - // logic to extract the relative path would need to consider several /out/soong/... - // locations patterns. - if (systemServicePathPrefixes.none { filePath.contains(it) }) return + if (!isSystemServicePath(context)) return val fullyQualifiedInterfaceName = getContainingAidlInterfaceQualified(context, node) ?: return |