Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d27d379e authored by mattgilbride's avatar mattgilbride
Browse files

EnforcePermissionFix: refactor for greater separation of concern and reuse 

Move more logic for identifying/creating an instance of
EnforcePermissionFix from the related detector to the class itself.

These changes also make it easier to consume this code from experimental
projects.

This change also enables AndroidGlobalLintCheckerTest, and adds/fixes
test cases.  AndroidGlobalLintCheckerTest will now run as part of
presubmit.

Bug: 232058525
Bug: 240445172
Test: atest --host AndroidGlobalLintCheckerTest

Change-Id: Ice19df391e9a308f3d2a6ebf2b5ba2f8ad5bb00b
parent 003fbc74

tools/lint/global/Android.bp

+14 −6
Original line number Diff line number Diff line
@@ -38,12 +38,6 @@ java_library_host { 


java_test_host {

    name: "AndroidGlobalLintCheckerTest",

    // TODO(b/239881504): Since this test was written, Android

    // Lint was updated, and now includes classes that were

    // compiled for java 15. The soong build doesn't support

    // java 15 yet, so we can't compile against "lint". Disable

    // the test until java 15 is supported.

    enabled: false,

    srcs: ["checks/src/test/java/**/*.kt"],

    static_libs: [

        "AndroidGlobalLintChecker",
@@ -53,5 +47,19 @@ java_test_host { 
    ],

    test_options: {

        unit_test: true,

        tradefed_options: [

            {

                // lint bundles in some classes that were built with older versions

                // of libraries, and no longer load. Since tradefed tries to load

                // all classes in the jar to look for tests, it crashes loading them.

                // Exclude these classes from tradefed's search.

                name: "exclude-paths",

                value: "org/apache",

            },

            {

                name: "exclude-paths",

                value: "META-INF",

            },

        ],

    },

}

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

+44 −1
Original line number Diff line number Diff line
@@ -97,6 +97,48 @@ data class EnforcePermissionFix( 
        }



    companion object {

        /**

         * Walks the expressions in a block, looking for simple permission checks.

         *

         * As soon as something other than a permission check is encountered, stop looking,

         * as some other business logic is happening that prevents an automated fix.

         */

        fun fromBlockExpression(

            context: JavaContext,

            blockExpression: UBlockExpression

        ): EnforcePermissionFix? {

            try {

                val singleFixes = mutableListOf<EnforcePermissionFix>()

                for (expression in blockExpression.expressions) {

                    val fix = fromExpression(context, expression) ?: break

                    singleFixes.add(fix)

                }

                return compose(singleFixes)

            } catch (e: AnyOfAllOfException) {

                return null

            }

        }



        /**

         * Conditionally constructs EnforcePermissionFix from any UExpression

         *

         * @return EnforcePermissionFix if the expression boils down to a permission check,

         * else null

         */

        fun fromExpression(

            context: JavaContext,

            expression: UExpression

        ): EnforcePermissionFix? {

            val trimmedExpression = expression.skipParenthesizedExprDown()

            if (trimmedExpression is UIfExpression) {

                return fromIfExpression(context, trimmedExpression)

            }

            findCallExpression(trimmedExpression)?.let {

                return fromCallExpression(context, it)

            }

            return null

        }



        /**

         * Conditionally constructs EnforcePermissionFix from a UCallExpression

         *
@@ -183,7 +225,8 @@ data class EnforcePermissionFix( 
        }





        fun compose(individuals: List<EnforcePermissionFix>): EnforcePermissionFix {

        fun compose(individuals: List<EnforcePermissionFix>): EnforcePermissionFix? {

            if (individuals.isEmpty()) return null

            val anyOfs = individuals.filter(EnforcePermissionFix::anyOf)

            // anyOf/allOf should be consistent.  If we encounter some @PermissionMethods that are anyOf

            // and others that aren't, we don't know what to do.

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

+1 −57
Original line number Diff line number Diff line
@@ -23,19 +23,13 @@ import com.android.tools.lint.detector.api.Issue 
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.google.android.lint.findCallExpression

import org.jetbrains.uast.UBlockExpression

import org.jetbrains.uast.UElement

import org.jetbrains.uast.UIfExpression

import org.jetbrains.uast.UMethod

import org.jetbrains.uast.skipParenthesizedExprDown



/**

 * Looks for methods implementing generated AIDL interface stubs

 * that can have simple permission checks migrated to

 * @EnforcePermission annotations

 *

 * TODO: b/242564870 (enable parse and autoFix of .aidl files)

 */

@Suppress("UnstableApiUsage")

class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() {
@@ -45,7 +39,7 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { 
            interfaceName: String,

            body: UBlockExpression

    ) {

        val enforcePermissionFix = accumulateSimplePermissionCheckFixes(body, context) ?: return

        val enforcePermissionFix = EnforcePermissionFix.fromBlockExpression(context, body) ?: return

        val lintFix = enforcePermissionFix.toLintFix(context, node)

        val message =

                "$interfaceName permission check ${
@@ -67,56 +61,6 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() { 
        context.report(incident)

    }



    /**

     * Walk the expressions in the method, looking for simple permission checks.

     *

     * If a single permission check is found at the beginning of the method,

     * this should be migrated to @EnforcePermission(value).

     *

     * If multiple consecutive permission checks are found,

     * these should be migrated to @EnforcePermission(allOf={value1, value2, ...})

     *

     * As soon as something other than a permission check is encountered, stop looking,

     * as some other business logic is happening that prevents an automated fix.

     */

    private fun accumulateSimplePermissionCheckFixes(

                methodBody: UBlockExpression,

                context: JavaContext

        ): EnforcePermissionFix? {

        try {

            val singleFixes = mutableListOf<EnforcePermissionFix>()

            for (expression in methodBody.expressions) {

                val fix = getPermissionCheckFix(

                        expression.skipParenthesizedExprDown(),

                        context) ?: break

                singleFixes.add(fix)

            }

            return when (singleFixes.size) {

                0 -> null

                1 -> singleFixes[0]

                else -> EnforcePermissionFix.compose(singleFixes)

            }

        } catch (e: AnyOfAllOfException) {

            return null

        }

    }





    /**

     * If an expression boils down to a permission check, return

     * the helper for creating a lint auto fix to @EnforcePermission

     */

    private fun getPermissionCheckFix(startingExpression: UElement?, context: JavaContext):

            EnforcePermissionFix? {

        if (startingExpression is UIfExpression) {

            return EnforcePermissionFix.fromIfExpression(context, startingExpression)

        }

        findCallExpression(startingExpression)?.let {

            return EnforcePermissionFix.fromCallExpression(context, it)

        }

        return null

    }



    companion object {



        private val EXPLANATION = """

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

+43 −14
Original line number Diff line number Diff line
@@ -397,7 +397,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { 
                public class Foo extends ITest.Stub {

                    private Context mContext;



                    @android.content.pm.PermissionMethod(orSelf = true)

                    @android.annotation.PermissionMethod(orSelf = true)

                    private void helper() {

                        mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");

                    }
@@ -442,7 +442,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { 
                    public class Foo extends ITest.Stub {

                        private Context mContext;



                        @android.content.pm.PermissionMethod

                        @android.annotation.PermissionMethod

                    private void helper() {

                        mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");

                    }
@@ -487,7 +487,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { 
                public class Foo extends ITest.Stub {

                    private Context mContext;



                    @android.content.pm.PermissionMethod(orSelf = true)

                    @android.annotation.PermissionMethod(orSelf = true)

                    private void helper() {

                        mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");

                        mContext.enforceCallingOrSelfPermission("android.permission.WRITE_CONTACTS", "foo");
@@ -536,13 +536,13 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { 
                public class Foo extends ITest.Stub {

                    private Context mContext;



                    @android.content.pm.PermissionMethod(orSelf = true)

                    @android.annotation.PermissionMethod(orSelf = true)

                    private void helperHelper() {

                        helper("android.permission.WRITE_CONTACTS");

                    }



                    @android.content.pm.PermissionMethod(orSelf = true)

                    private void helper(@android.content.pm.PermissionName String extraPermission) {

                    @android.annotation.PermissionMethod(orSelf = true)

                    private void helper(@android.annotation.PermissionName String extraPermission) {

                        mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");

                    }
@@ -689,6 +689,34 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { 
            .expectClean()

    }



    fun testIfExpression_inlinedWithSideEffect_ignored() {

        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 {

                        if (somethingElse() && mContext.checkCallingPermission("android.permission.READ_CONTACTS", "foo")

                                != PackageManager.PERMISSION_GRANTED) {

                            throw new SecurityException("yikes!");

                        }

                    }



                    private boolean somethingElse() {

                        return true;

                    }

                }

                """

            ).indented(),

            *stubs

        )

            .run()

            .expectClean()

    }



    fun testAnyOf_hardCodedAndVarArgs() {

        lint().files(

                java(
@@ -699,13 +727,13 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { 
                    public class Foo extends ITest.Stub {

                        private Context mContext;



                        @android.content.pm.PermissionMethod(anyOf = true)

                        @android.annotation.PermissionMethod(anyOf = true)

                        private void helperHelper() {

                            helper("FOO", "BAR");

                        }



                        @android.content.pm.PermissionMethod(anyOf = true, value = {"BAZ", "BUZZ"})

                        private void helper(@android.content.pm.PermissionName String... extraPermissions) {}

                        @android.annotation.PermissionMethod(anyOf = true, value = {"BAZ", "BUZZ"})

                        private void helper(@android.annotation.PermissionName String... extraPermissions) {}



                        @Override

                        public void test() throws android.os.RemoteException {
@@ -748,13 +776,13 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { 
                    public class Foo extends ITest.Stub {

                        private Context mContext;



                        @android.content.pm.PermissionMethod

                        @android.annotation.PermissionMethod

                        private void allOfhelper() {

                            mContext.enforceCallingOrSelfPermission("FOO");

                            mContext.enforceCallingOrSelfPermission("BAR");

                        }



                        @android.content.pm.PermissionMethod(anyOf = true, permissions = {"BAZ", "BUZZ"})

                        @android.annotation.PermissionMethod(anyOf = true, permissions = {"BAZ", "BUZZ"})

                        private void anyOfHelper() {}



                        @Override
@@ -776,17 +804,18 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() { 
                java(

                    """

                    import android.content.Context;

                    import android.content.pm.PermissionName;import android.test.ITest;

                    import android.annotation.PermissionName;

                    import android.test.ITest;



                    public class Foo extends ITest.Stub {

                        private Context mContext;



                        @android.content.pm.PermissionMethod(anyOf = true)

                        @android.annotation.PermissionMethod(anyOf = true)

                        private void anyOfCheck(@PermissionName String... permissions) {

                            allOfCheck("BAZ", "BUZZ");

                        }



                        @android.content.pm.PermissionMethod

                        @android.annotation.PermissionMethod

                        private void allOfCheck(@PermissionName String... permissions) {}



                        @Override

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

+47 −47
Original line number Diff line number Diff line
@@ -19,14 +19,14 @@ val contextStub: TestFile = java( 
    """

    package android.content;

    public class Context {

            @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) {}

            @android.content.pm.PermissionMethod

            public int checkCallingPermission(@android.content.pm.PermissionName String permission, String message) {}

        @android.annotation.PermissionMethod(orSelf = true)

        public void enforceCallingOrSelfPermission(@android.annotation.PermissionName String permission, String message) {}

        @android.annotation.PermissionMethod

        public void enforceCallingPermission(@android.annotation.PermissionName String permission, String message) {}

        @android.annotation.PermissionMethod(orSelf = true)

        public int checkCallingOrSelfPermission(@android.annotation.PermissionName String permission, String message) {}

        @android.annotation.PermissionMethod

        public int checkCallingPermission(@android.annotation.PermissionName String permission, String message) {}

    }

    """

).indented()
@@ -42,7 +42,7 @@ val binderStub: TestFile = java( 


val permissionMethodStub: TestFile = java(

    """

        package android.content.pm;

    package android.annotation;



    import static java.lang.annotation.ElementType.METHOD;

    import static java.lang.annotation.RetentionPolicy.CLASS;
@@ -58,7 +58,7 @@ val permissionMethodStub: TestFile = java( 


val permissionNameStub: TestFile = java(

    """

        package android.content.pm;

    package android.annotation;



    import static java.lang.annotation.ElementType.FIELD;

    import static java.lang.annotation.ElementType.LOCAL_VARIABLE;