Migrate PermissionAnnotationDetector to global lint checks
Cherry-picked from internal branch. Originally authored by
tmagirescu@google.com.
PermissionAnnotationDetector will no longer be a local lint check which
could optionally be added to build targets, but instead a global lint
check ran on every build. New AIDL Interfaces part of the system_server
process will need to use @EnforcePermission annotations. The detector
does not enforce old AIDL Interfaces to use the annotations. These are
included in the exemptAidlInterfaces set generated by
ExemptAidlInterfacesGenerator.
The CL removes the reference to the PermissionAnnotationDetector from
the Accessibility Service build targets. Instead the already-annotated
AIDL Interfaces are removed from exemptAidlInterfaces, achieving the
same effect.
Bug: 363248121
Test: PermissionAnnotationDetectorTest
Flag: EXEMPT lint check
Merged-In: I2cad77cbc7883087dd95b9558d3543fcb321bbc8
Change-Id: I47873ec83d8f26a97f1f6ff70a056899dd9c5f45
diff --git a/services/accessibility/Android.bp b/services/accessibility/Android.bp
index 7a99b60..8ea8c9a 100644
--- a/services/accessibility/Android.bp
+++ b/services/accessibility/Android.bp
@@ -19,11 +19,6 @@
defaults: [
"platform_service_defaults",
],
- lint: {
- error_checks: ["MissingPermissionAnnotation"],
- baseline_filename: "lint-baseline.xml",
-
- },
srcs: [
":services.accessibility-sources",
"//frameworks/base/packages/SettingsLib/RestrictedLockUtils:SettingsLibRestrictedLockUtilsSrc",
diff --git a/services/accessibility/java/com/android/server/accessibility/AccessibilityInputFilter.java b/services/accessibility/java/com/android/server/accessibility/AccessibilityInputFilter.java
index f9196f3..7549655 100644
--- a/services/accessibility/java/com/android/server/accessibility/AccessibilityInputFilter.java
+++ b/services/accessibility/java/com/android/server/accessibility/AccessibilityInputFilter.java
@@ -62,7 +62,6 @@
*
* 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 e10e87c..c9ec16e 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 @@
/**
* 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 f5af99e..b79563f 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 @@
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 5c64697..af753e5 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.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 @@
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 bce848a..0000000
--- 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 28eab8f..290e7be 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.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 @@
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 8777712..675a59e 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 @@
* 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 @@
"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
similarity index 92%
rename from tools/lint/framework/checks/src/main/java/com/google/android/lint/PermissionAnnotationDetector.kt
rename to tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/PermissionAnnotationDetector.kt
index 6b50cfd..d44c271 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 @@
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 @@
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 0000000..92d0829
--- /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 6ad223c..57c2e5a 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 @@
*/
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 @@
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