Merge "EnforcePermissionHelperDetector: allow ancestors to call super instead of helper" (e023e7d4) · Commits · e / os / android_frameworks_base

tools/lint/global/checks/src/main/java/com/google/android/lint/aidl/EnforcePermissionHelperDetector.kt

+21 −17

Original line number	Diff line number	Diff line
		@@ -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<Class<out UElement?>> =
		@@ -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()
		val firstExpressionSource = firstExpression.skipParenthesizedExprDown()
		.asSourceString()
		.filterNot(Char::isWhitespace)

		if (firstExpressionSource != targetExpression) {
		if (firstExpressionSource != targetExpression &&
		firstExpressionSource != "super.$targetExpression") {
		// calling super.<methodName>() is also legal
		val directSuper = context.evaluator.getSuperMethod(node)
		val firstCall = findCallExpression(firstExpression)?.resolve()
		if (directSuper != null && firstCall == directSuper) return

		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(

tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/EnforcePermissionHelperDetectorTest.kt

+236 −4

Original line number	Diff line number	Diff line
		@@ -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)
		}

tools/lint/global/checks/src/test/java/com/google/android/lint/aidl/Stubs.kt

+3 −1

Original line number	Diff line number	Diff line
		@@ -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;
		}
		"""