Merge "SimpleManualPermissionEnforcementDetector: conditional ERROR level incident"
This commit is contained in:
Matt Gilbride
committed by
Android (Google) Code Review
Android (Google) Code Review
commit
73a89acae7
@@ -17,6 +17,7 @@
|
||||
package com.google.android.lint
|
||||
|
||||
import com.android.tools.lint.detector.api.getUMethod
|
||||
import org.jetbrains.uast.UAnnotation
|
||||
import org.jetbrains.uast.UCallExpression
|
||||
import org.jetbrains.uast.UMethod
|
||||
import org.jetbrains.uast.UParameter
|
||||
@@ -26,10 +27,11 @@ fun isPermissionMethodCall(callExpression: UCallExpression): Boolean {
|
||||
return hasPermissionMethodAnnotation(method)
|
||||
}
|
||||
|
||||
fun hasPermissionMethodAnnotation(method: UMethod): Boolean = method.annotations
|
||||
.any {
|
||||
it.hasQualifiedName(ANNOTATION_PERMISSION_METHOD)
|
||||
}
|
||||
fun hasPermissionMethodAnnotation(method: UMethod): Boolean =
|
||||
getPermissionMethodAnnotation(method) != null
|
||||
|
||||
fun getPermissionMethodAnnotation(method: UMethod?): UAnnotation? = method?.uAnnotations
|
||||
?.firstOrNull { it.qualifiedName == ANNOTATION_PERMISSION_METHOD }
|
||||
|
||||
fun hasPermissionNameAnnotation(parameter: UParameter) = parameter.annotations.any {
|
||||
it.hasQualifiedName(ANNOTATION_PERMISSION_NAME)
|
||||
|
||||
@@ -19,9 +19,12 @@ package com.google.android.lint.aidl
|
||||
import com.android.tools.lint.detector.api.JavaContext
|
||||
import com.android.tools.lint.detector.api.LintFix
|
||||
import com.android.tools.lint.detector.api.Location
|
||||
import com.android.tools.lint.detector.api.UastLintUtils.Companion.getAnnotationBooleanValue
|
||||
import com.android.tools.lint.detector.api.getUMethod
|
||||
import com.google.android.lint.getPermissionMethodAnnotation
|
||||
import com.google.android.lint.hasPermissionNameAnnotation
|
||||
import com.google.android.lint.isPermissionMethodCall
|
||||
import com.intellij.psi.PsiType
|
||||
import org.jetbrains.kotlin.psi.psiUtil.parameterIndex
|
||||
import org.jetbrains.uast.UCallExpression
|
||||
import org.jetbrains.uast.evaluateString
|
||||
@@ -37,7 +40,8 @@ import org.jetbrains.uast.visitor.AbstractUastVisitor
|
||||
*/
|
||||
data class EnforcePermissionFix(
|
||||
val locations: List<Location>,
|
||||
val permissionNames: List<String>
|
||||
val permissionNames: List<String>,
|
||||
val errorLevel: Boolean,
|
||||
) {
|
||||
fun toLintFix(annotationLocation: Location): LintFix {
|
||||
val removeFixes = this.locations.map {
|
||||
@@ -78,19 +82,28 @@ data class EnforcePermissionFix(
|
||||
fun fromCallExpression(
|
||||
context: JavaContext,
|
||||
callExpression: UCallExpression
|
||||
): EnforcePermissionFix? =
|
||||
if (isPermissionMethodCall(callExpression)) {
|
||||
EnforcePermissionFix(
|
||||
): EnforcePermissionFix? {
|
||||
val method = callExpression.resolve()?.getUMethod() ?: return null
|
||||
val annotation = getPermissionMethodAnnotation(method) ?: return null
|
||||
val enforces = method.returnType == PsiType.VOID
|
||||
val orSelf = getAnnotationBooleanValue(annotation, "orSelf") ?: false
|
||||
return EnforcePermissionFix(
|
||||
listOf(getPermissionCheckLocation(context, callExpression)),
|
||||
getPermissionCheckValues(callExpression)
|
||||
)
|
||||
} else null
|
||||
getPermissionCheckValues(callExpression),
|
||||
// If we detect that the PermissionMethod enforces that permission is granted,
|
||||
// AND is of the "orSelf" variety, we are very confident that this is a behavior
|
||||
// preserving migration to @EnforcePermission. Thus, the incident should be ERROR
|
||||
// level.
|
||||
errorLevel = enforces && orSelf
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
fun compose(individuals: List<EnforcePermissionFix>): EnforcePermissionFix =
|
||||
EnforcePermissionFix(
|
||||
individuals.flatMap { it.locations },
|
||||
individuals.flatMap { it.permissionNames }
|
||||
individuals.flatMap { it.permissionNames },
|
||||
errorLevel = individuals.all(EnforcePermissionFix::errorLevel)
|
||||
)
|
||||
|
||||
/**
|
||||
|
||||
@@ -18,6 +18,7 @@ package com.google.android.lint.aidl
|
||||
|
||||
import com.android.tools.lint.detector.api.Category
|
||||
import com.android.tools.lint.detector.api.Implementation
|
||||
import com.android.tools.lint.detector.api.Incident
|
||||
import com.android.tools.lint.detector.api.Issue
|
||||
import com.android.tools.lint.detector.api.JavaContext
|
||||
import com.android.tools.lint.detector.api.Scope
|
||||
@@ -48,14 +49,22 @@ class SimpleManualPermissionEnforcementDetector : AidlImplementationDetector() {
|
||||
val enforcePermissionFix = accumulateSimplePermissionCheckFixes(body, context) ?: return
|
||||
val lintFix = enforcePermissionFix.toLintFix(context.getLocation(node))
|
||||
val message =
|
||||
"$interfaceName permission check can be converted to @EnforcePermission annotation"
|
||||
"$interfaceName permission check ${
|
||||
if (enforcePermissionFix.errorLevel) "should" else "can"
|
||||
} be converted to @EnforcePermission annotation"
|
||||
|
||||
context.report(
|
||||
val incident = Incident(
|
||||
ISSUE_SIMPLE_MANUAL_PERMISSION_ENFORCEMENT,
|
||||
enforcePermissionFix.locations.last(),
|
||||
message,
|
||||
lintFix
|
||||
)
|
||||
|
||||
if (enforcePermissionFix.errorLevel) {
|
||||
incident.overrideSeverity(Severity.ERROR)
|
||||
}
|
||||
|
||||
context.report(incident)
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -51,10 +51,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
src/Foo.java:7: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");
|
||||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||||
0 errors, 1 warnings
|
||||
1 errors, 0 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
@@ -68,6 +68,80 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
)
|
||||
}
|
||||
|
||||
fun testClass_orSelfFalse_warning() {
|
||||
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 {
|
||||
mContext.enforceCallingPermission("android.permission.READ_CONTACTS", "foo");
|
||||
}
|
||||
}
|
||||
"""
|
||||
).indented(),
|
||||
*stubs
|
||||
)
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
mContext.enforceCallingPermission("android.permission.READ_CONTACTS", "foo");
|
||||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||||
0 errors, 1 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
"""
|
||||
Fix for src/Foo.java line 7: Annotate with @EnforcePermission:
|
||||
@@ -5 +5
|
||||
+ @android.annotation.EnforcePermission("android.permission.READ_CONTACTS")
|
||||
@@ -7 +8
|
||||
- mContext.enforceCallingPermission("android.permission.READ_CONTACTS", "foo");
|
||||
"""
|
||||
)
|
||||
}
|
||||
|
||||
fun testClass_enforcesFalse_warning() {
|
||||
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 {
|
||||
mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");
|
||||
}
|
||||
}
|
||||
"""
|
||||
).indented(),
|
||||
*stubs
|
||||
)
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:7: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");
|
||||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||||
0 errors, 1 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
"""
|
||||
Fix for src/Foo.java line 7: Annotate with @EnforcePermission:
|
||||
@@ -5 +5
|
||||
+ @android.annotation.EnforcePermission("android.permission.READ_CONTACTS")
|
||||
@@ -7 +8
|
||||
- mContext.checkCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");
|
||||
"""
|
||||
)
|
||||
}
|
||||
|
||||
fun testAnonClass() {
|
||||
lint().files(
|
||||
java(
|
||||
@@ -91,10 +165,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:8: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
src/Foo.java:8: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
mContext.enforceCallingOrSelfPermission(
|
||||
^
|
||||
0 errors, 1 warnings
|
||||
1 errors, 0 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
@@ -131,10 +205,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:8: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
src/Foo.java:8: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
mContext.enforceCallingOrSelfPermission(android.Manifest.permission.READ_CONTACTS, "foo");
|
||||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||||
0 errors, 1 warnings
|
||||
1 errors, 0 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
@@ -173,10 +247,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
src/Foo.java:10: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
mContext.enforceCallingOrSelfPermission(
|
||||
^
|
||||
0 errors, 1 warnings
|
||||
1 errors, 0 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
@@ -193,6 +267,96 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
)
|
||||
}
|
||||
|
||||
fun testAllOf_mixedOrSelf_warning() {
|
||||
lint().files(
|
||||
java(
|
||||
"""
|
||||
import android.content.Context;
|
||||
import android.test.ITest;
|
||||
public class Foo {
|
||||
private Context mContext;
|
||||
private ITest itest = new ITest.Stub() {
|
||||
@Override
|
||||
public void test() throws android.os.RemoteException {
|
||||
mContext.enforceCallingOrSelfPermission(
|
||||
"android.permission.READ_CONTACTS", "foo");
|
||||
mContext.enforceCallingPermission(
|
||||
"android.permission.WRITE_CONTACTS", "foo");
|
||||
}
|
||||
};
|
||||
}
|
||||
"""
|
||||
).indented(),
|
||||
*stubs
|
||||
)
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
mContext.enforceCallingPermission(
|
||||
^
|
||||
0 errors, 1 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
"""
|
||||
Fix for src/Foo.java line 10: Annotate with @EnforcePermission:
|
||||
@@ -6 +6
|
||||
+ @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS"})
|
||||
@@ -8 +9
|
||||
- mContext.enforceCallingOrSelfPermission(
|
||||
- "android.permission.READ_CONTACTS", "foo");
|
||||
- mContext.enforceCallingPermission(
|
||||
- "android.permission.WRITE_CONTACTS", "foo");
|
||||
"""
|
||||
)
|
||||
}
|
||||
|
||||
fun testAllOf_mixedEnforces_warning() {
|
||||
lint().files(
|
||||
java(
|
||||
"""
|
||||
import android.content.Context;
|
||||
import android.test.ITest;
|
||||
public class Foo {
|
||||
private Context mContext;
|
||||
private ITest itest = new ITest.Stub() {
|
||||
@Override
|
||||
public void test() throws android.os.RemoteException {
|
||||
mContext.enforceCallingOrSelfPermission(
|
||||
"android.permission.READ_CONTACTS", "foo");
|
||||
mContext.checkCallingOrSelfPermission(
|
||||
"android.permission.WRITE_CONTACTS", "foo");
|
||||
}
|
||||
};
|
||||
}
|
||||
"""
|
||||
).indented(),
|
||||
*stubs
|
||||
)
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:10: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
mContext.checkCallingOrSelfPermission(
|
||||
^
|
||||
0 errors, 1 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
"""
|
||||
Fix for src/Foo.java line 10: Annotate with @EnforcePermission:
|
||||
@@ -6 +6
|
||||
+ @android.annotation.EnforcePermission(allOf={"android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS"})
|
||||
@@ -8 +9
|
||||
- mContext.enforceCallingOrSelfPermission(
|
||||
- "android.permission.READ_CONTACTS", "foo");
|
||||
- mContext.checkCallingOrSelfPermission(
|
||||
- "android.permission.WRITE_CONTACTS", "foo");
|
||||
"""
|
||||
)
|
||||
}
|
||||
|
||||
fun testPrecedingExpressions() {
|
||||
lint().files(
|
||||
java(
|
||||
@@ -225,7 +389,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
public class Foo extends ITest.Stub {
|
||||
private Context mContext;
|
||||
|
||||
@android.content.pm.PermissionMethod
|
||||
@android.content.pm.PermissionMethod(orSelf = true)
|
||||
private void helper() {
|
||||
mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");
|
||||
}
|
||||
@@ -242,10 +406,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:14: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
src/Foo.java:14: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
helper();
|
||||
~~~~~~~~~
|
||||
0 errors, 1 warnings
|
||||
1 errors, 0 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
@@ -259,6 +423,50 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
)
|
||||
}
|
||||
|
||||
fun testPermissionHelper_orSelfNotBubbledUp_warning() {
|
||||
lint().files(
|
||||
java(
|
||||
"""
|
||||
import android.content.Context;
|
||||
import android.test.ITest;
|
||||
|
||||
public class Foo extends ITest.Stub {
|
||||
private Context mContext;
|
||||
|
||||
@android.content.pm.PermissionMethod
|
||||
private void helper() {
|
||||
mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");
|
||||
}
|
||||
|
||||
@Override
|
||||
public void test() throws android.os.RemoteException {
|
||||
helper();
|
||||
}
|
||||
}
|
||||
"""
|
||||
).indented(),
|
||||
*stubs
|
||||
)
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:14: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
helper();
|
||||
~~~~~~~~~
|
||||
0 errors, 1 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
"""
|
||||
Fix for src/Foo.java line 14: Annotate with @EnforcePermission:
|
||||
@@ -12 +12
|
||||
+ @android.annotation.EnforcePermission("android.permission.READ_CONTACTS")
|
||||
@@ -14 +15
|
||||
- helper();
|
||||
"""
|
||||
)
|
||||
}
|
||||
|
||||
fun testPermissionHelperAllOf() {
|
||||
lint().files(
|
||||
java(
|
||||
@@ -269,7 +477,7 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
public class Foo extends ITest.Stub {
|
||||
private Context mContext;
|
||||
|
||||
@android.content.pm.PermissionMethod
|
||||
@android.content.pm.PermissionMethod(orSelf = true)
|
||||
private void helper() {
|
||||
mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");
|
||||
mContext.enforceCallingOrSelfPermission("android.permission.WRITE_CONTACTS", "foo");
|
||||
@@ -288,10 +496,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:16: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
src/Foo.java:16: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
mContext.enforceCallingOrSelfPermission("FOO", "foo");
|
||||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||||
0 errors, 1 warnings
|
||||
1 errors, 0 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
@@ -317,12 +525,12 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
public class Foo extends ITest.Stub {
|
||||
private Context mContext;
|
||||
|
||||
@android.content.pm.PermissionMethod
|
||||
@android.content.pm.PermissionMethod(orSelf = true)
|
||||
private void helperHelper() {
|
||||
helper("android.permission.WRITE_CONTACTS");
|
||||
}
|
||||
|
||||
@android.content.pm.PermissionMethod
|
||||
@android.content.pm.PermissionMethod(orSelf = true)
|
||||
private void helper(@android.content.pm.PermissionName String extraPermission) {
|
||||
mContext.enforceCallingOrSelfPermission("android.permission.READ_CONTACTS", "foo");
|
||||
}
|
||||
@@ -339,10 +547,10 @@ class SimpleManualPermissionEnforcementDetectorTest : LintDetectorTest() {
|
||||
.run()
|
||||
.expect(
|
||||
"""
|
||||
src/Foo.java:19: Warning: ITest permission check can be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
src/Foo.java:19: Error: ITest permission check should be converted to @EnforcePermission annotation [SimpleManualPermissionEnforcement]
|
||||
helperHelper();
|
||||
~~~~~~~~~~~~~~~
|
||||
0 errors, 1 warnings
|
||||
1 errors, 0 warnings
|
||||
"""
|
||||
)
|
||||
.expectFixDiffs(
|
||||
|
||||
@@ -17,8 +17,12 @@ val contextStub: TestFile = java(
|
||||
"""
|
||||
package android.content;
|
||||
public class Context {
|
||||
@android.content.pm.PermissionMethod
|
||||
@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) {}
|
||||
}
|
||||
"""
|
||||
).indented()
|
||||
|
||||
Reference in New Issue
Block a user