From fd87a60be88d3d2d3ebc37a2ff1b86ec1489efe8 Mon Sep 17 00:00:00 2001 From: atrost Date: Mon, 2 Mar 2020 17:09:23 +0000 Subject: [PATCH] Add support for LoggingOnly changes Throw an exception if someone tries to add an override for a logging only change. Incorporate the restriction in the OverrideValidator. Test: change one change to be logging only, flash device, adb shell dumpsys platform_compat Test: atest com.android.server.compat.CompatConfigTest Test: atest com.android.server.compat.OverrideValidatorImplTest Bug: 148009004 Change-Id: I379c63f8b5c54500d9066be9363a186efd55d200 --- .../compat/CompatibilityChangeInfo.java | 10 ++++++- .../internal/compat/OverrideAllowedState.java | 11 ++++++- .../android/server/compat/CompatChange.java | 15 +++++++--- .../android/server/compat/CompatConfig.java | 14 +++++++++ .../server/compat/OverrideValidatorImpl.java | 12 ++++---- services/core/xsd/platform-compat-config.xsd | 1 + .../xsd/platform-compat-schema/current.txt | 2 ++ .../server/compat/CompatConfigBuilder.java | 25 +++++++++------- .../server/compat/CompatConfigTest.java | 11 +++++++ .../compat/OverrideValidatorImplTest.java | 30 ++++++++++++++++--- 10 files changed, 106 insertions(+), 25 deletions(-) diff --git a/core/java/com/android/internal/compat/CompatibilityChangeInfo.java b/core/java/com/android/internal/compat/CompatibilityChangeInfo.java index 16628d71712cb..ab890d277f43e 100644 --- a/core/java/com/android/internal/compat/CompatibilityChangeInfo.java +++ b/core/java/com/android/internal/compat/CompatibilityChangeInfo.java @@ -30,6 +30,7 @@ public class CompatibilityChangeInfo implements Parcelable { private final @Nullable String mName; private final int mEnableAfterTargetSdk; private final boolean mDisabled; + private final boolean mLoggingOnly; private final @Nullable String mDescription; public long getId() { @@ -49,17 +50,22 @@ public class CompatibilityChangeInfo implements Parcelable { return mDisabled; } + public boolean getLoggingOnly() { + return mLoggingOnly; + } + public String getDescription() { return mDescription; } public CompatibilityChangeInfo( Long changeId, String name, int enableAfterTargetSdk, boolean disabled, - String description) { + boolean loggingOnly, String description) { this.mChangeId = changeId; this.mName = name; this.mEnableAfterTargetSdk = enableAfterTargetSdk; this.mDisabled = disabled; + this.mLoggingOnly = loggingOnly; this.mDescription = description; } @@ -68,6 +74,7 @@ public class CompatibilityChangeInfo implements Parcelable { mName = in.readString(); mEnableAfterTargetSdk = in.readInt(); mDisabled = in.readBoolean(); + mLoggingOnly = in.readBoolean(); mDescription = in.readString(); } @@ -82,6 +89,7 @@ public class CompatibilityChangeInfo implements Parcelable { dest.writeString(mName); dest.writeInt(mEnableAfterTargetSdk); dest.writeBoolean(mDisabled); + dest.writeBoolean(mLoggingOnly); dest.writeString(mDescription); } diff --git a/core/java/com/android/internal/compat/OverrideAllowedState.java b/core/java/com/android/internal/compat/OverrideAllowedState.java index 56216c2510708..bed41b37ac816 100644 --- a/core/java/com/android/internal/compat/OverrideAllowedState.java +++ b/core/java/com/android/internal/compat/OverrideAllowedState.java @@ -33,7 +33,8 @@ public final class OverrideAllowedState implements Parcelable { DISABLED_NOT_DEBUGGABLE, DISABLED_NON_TARGET_SDK, DISABLED_TARGET_SDK_TOO_HIGH, - PACKAGE_DOES_NOT_EXIST + PACKAGE_DOES_NOT_EXIST, + LOGGING_ONLY_CHANGE }) @Retention(RetentionPolicy.SOURCE) public @interface State { @@ -60,6 +61,10 @@ public final class OverrideAllowedState implements Parcelable { * Package does not exist. */ public static final int PACKAGE_DOES_NOT_EXIST = 4; + /** + * Change is marked as logging only, and cannot be toggled. + */ + public static final int LOGGING_ONLY_CHANGE = 5; @State public final int state; @@ -118,6 +123,10 @@ public final class OverrideAllowedState implements Parcelable { "Cannot override %1$d for %2$s because the package does not exist, and " + "the change is targetSdk gated.", changeId, packageName)); + case LOGGING_ONLY_CHANGE: + throw new SecurityException(String.format( + "Cannot override %1$d because it is marked as a logging-only change.", + changeId)); } } diff --git a/services/core/java/com/android/server/compat/CompatChange.java b/services/core/java/com/android/server/compat/CompatChange.java index 8687f35d745e5..7bdeb5969f1f9 100644 --- a/services/core/java/com/android/server/compat/CompatChange.java +++ b/services/core/java/com/android/server/compat/CompatChange.java @@ -63,7 +63,7 @@ public final class CompatChange extends CompatibilityChangeInfo { private Map mPackageOverrides; public CompatChange(long changeId) { - this(changeId, null, -1, false, null); + this(changeId, null, -1, false, false, null); } /** @@ -74,8 +74,8 @@ public final class CompatChange extends CompatibilityChangeInfo { * @param disabled If {@code true}, overrides any {@code enableAfterTargetSdk} set. */ public CompatChange(long changeId, @Nullable String name, int enableAfterTargetSdk, - boolean disabled, String description) { - super(changeId, name, enableAfterTargetSdk, disabled, description); + boolean disabled, boolean loggingOnly, String description) { + super(changeId, name, enableAfterTargetSdk, disabled, loggingOnly, description); } /** @@ -83,7 +83,7 @@ public final class CompatChange extends CompatibilityChangeInfo { */ public CompatChange(Change change) { super(change.getId(), change.getName(), change.getEnableAfterTargetSdk(), - change.getDisabled(), change.getDescription()); + change.getDisabled(), change.getLoggingOnly(), change.getDescription()); } void registerListener(ChangeListener listener) { @@ -105,6 +105,10 @@ public final class CompatChange extends CompatibilityChangeInfo { * @param enabled Whether or not to enable the change. */ void addPackageOverride(String pname, boolean enabled) { + if (getLoggingOnly()) { + throw new IllegalArgumentException( + "Can't add overrides for a logging only change " + toString()); + } if (mPackageOverrides == null) { mPackageOverrides = new HashMap<>(); } @@ -160,6 +164,9 @@ public final class CompatChange extends CompatibilityChangeInfo { if (getDisabled()) { sb.append("; disabled"); } + if (getLoggingOnly()) { + sb.append("; loggingOnly"); + } if (mPackageOverrides != null && mPackageOverrides.size() > 0) { sb.append("; packageOverrides=").append(mPackageOverrides); } diff --git a/services/core/java/com/android/server/compat/CompatConfig.java b/services/core/java/com/android/server/compat/CompatConfig.java index 441d9d9f380ef..bfc2f82d586d2 100644 --- a/services/core/java/com/android/server/compat/CompatConfig.java +++ b/services/core/java/com/android/server/compat/CompatConfig.java @@ -205,6 +205,19 @@ final class CompatConfig { } } + /** + * Returns whether the change is marked as logging only. + */ + boolean isLoggingOnly(long changeId) { + synchronized (mChanges) { + CompatChange c = mChanges.get(changeId); + if (c == null) { + return false; + } + return c.getLoggingOnly(); + } + } + /** * Removes an override previously added via {@link #addOverride(long, String, boolean)}. This * restores the default behaviour for the given change and app, once any app processes have been @@ -365,6 +378,7 @@ final class CompatConfig { change.getName(), change.getEnableAfterTargetSdk(), change.getDisabled(), + change.getLoggingOnly(), change.getDescription()); } return changeInfos; diff --git a/services/core/java/com/android/server/compat/OverrideValidatorImpl.java b/services/core/java/com/android/server/compat/OverrideValidatorImpl.java index 4bf606e801f9b..9e18c74265d7e 100644 --- a/services/core/java/com/android/server/compat/OverrideValidatorImpl.java +++ b/services/core/java/com/android/server/compat/OverrideValidatorImpl.java @@ -20,6 +20,7 @@ import static com.android.internal.compat.OverrideAllowedState.ALLOWED; import static com.android.internal.compat.OverrideAllowedState.DISABLED_NON_TARGET_SDK; import static com.android.internal.compat.OverrideAllowedState.DISABLED_NOT_DEBUGGABLE; import static com.android.internal.compat.OverrideAllowedState.DISABLED_TARGET_SDK_TOO_HIGH; +import static com.android.internal.compat.OverrideAllowedState.LOGGING_ONLY_CHANGE; import static com.android.internal.compat.OverrideAllowedState.PACKAGE_DOES_NOT_EXIST; import android.content.Context; @@ -51,12 +52,13 @@ public class OverrideValidatorImpl extends IOverrideValidator.Stub { @Override public OverrideAllowedState getOverrideAllowedState(long changeId, String packageName) { - boolean debuggableBuild = false; - boolean finalBuild = false; - int minTargetSdk = mCompatConfig.minTargetSdkForChangeId(changeId); + if (mCompatConfig.isLoggingOnly(changeId)) { + return new OverrideAllowedState(LOGGING_ONLY_CHANGE, -1, -1); + } - debuggableBuild = mAndroidBuildClassifier.isDebuggableBuild(); - finalBuild = mAndroidBuildClassifier.isFinalBuild(); + boolean debuggableBuild = mAndroidBuildClassifier.isDebuggableBuild(); + boolean finalBuild = mAndroidBuildClassifier.isFinalBuild(); + int minTargetSdk = mCompatConfig.minTargetSdkForChangeId(changeId); // Allow any override for userdebug or eng builds. if (debuggableBuild) { diff --git a/services/core/xsd/platform-compat-config.xsd b/services/core/xsd/platform-compat-config.xsd index a70568f5911a2..5a4c682f52eb1 100644 --- a/services/core/xsd/platform-compat-config.xsd +++ b/services/core/xsd/platform-compat-config.xsd @@ -27,6 +27,7 @@ + diff --git a/services/core/xsd/platform-compat-schema/current.txt b/services/core/xsd/platform-compat-schema/current.txt index 3a33f63361a53..7def58d56a261 100644 --- a/services/core/xsd/platform-compat-schema/current.txt +++ b/services/core/xsd/platform-compat-schema/current.txt @@ -7,12 +7,14 @@ package com.android.server.compat.config { method public boolean getDisabled(); method public int getEnableAfterTargetSdk(); method public long getId(); + method public boolean getLoggingOnly(); method public String getName(); method public String getValue(); method public void setDescription(String); method public void setDisabled(boolean); method public void setEnableAfterTargetSdk(int); method public void setId(long); + method public void setLoggingOnly(boolean); method public void setName(String); method public void setValue(String); } diff --git a/services/tests/servicestests/src/com/android/server/compat/CompatConfigBuilder.java b/services/tests/servicestests/src/com/android/server/compat/CompatConfigBuilder.java index 328c71dbc7dbf..2cbe7be8ac33d 100644 --- a/services/tests/servicestests/src/com/android/server/compat/CompatConfigBuilder.java +++ b/services/tests/servicestests/src/com/android/server/compat/CompatConfigBuilder.java @@ -40,52 +40,57 @@ class CompatConfigBuilder { } CompatConfigBuilder addTargetSdkChangeWithId(int sdk, long id) { - mChanges.add(new CompatChange(id, "", sdk, false, "")); + mChanges.add(new CompatChange(id, "", sdk, false, false, "")); return this; } CompatConfigBuilder addTargetSdkDisabledChangeWithId(int sdk, long id) { - mChanges.add(new CompatChange(id, "", sdk, true, "")); + mChanges.add(new CompatChange(id, "", sdk, true, false, "")); return this; } CompatConfigBuilder addTargetSdkChangeWithIdAndName(int sdk, long id, String name) { - mChanges.add(new CompatChange(id, name, sdk, false, "")); + mChanges.add(new CompatChange(id, name, sdk, false, false, "")); return this; } CompatConfigBuilder addTargetSdkChangeWithIdAndDescription(int sdk, long id, String description) { - mChanges.add(new CompatChange(id, "", sdk, false, description)); + mChanges.add(new CompatChange(id, "", sdk, false, false, description)); return this; } CompatConfigBuilder addEnabledChangeWithId(long id) { - mChanges.add(new CompatChange(id, "", -1, false, "")); + mChanges.add(new CompatChange(id, "", -1, false, false, "")); return this; } CompatConfigBuilder addEnabledChangeWithIdAndName(long id, String name) { - mChanges.add(new CompatChange(id, name, -1, false, "")); + mChanges.add(new CompatChange(id, name, -1, false, false, "")); return this; } CompatConfigBuilder addEnabledChangeWithIdAndDescription(long id, String description) { - mChanges.add(new CompatChange(id, "", -1, false, description)); + mChanges.add(new CompatChange(id, "", -1, false, false, description)); return this; } CompatConfigBuilder addDisabledChangeWithId(long id) { - mChanges.add(new CompatChange(id, "", -1, true, "")); + mChanges.add(new CompatChange(id, "", -1, true, false, "")); return this; } CompatConfigBuilder addDisabledChangeWithIdAndName(long id, String name) { - mChanges.add(new CompatChange(id, name, -1, true, "")); + mChanges.add(new CompatChange(id, name, -1, true, false, "")); return this; } CompatConfigBuilder addDisabledChangeWithIdAndDescription(long id, String description) { - mChanges.add(new CompatChange(id, "", -1, true, description)); + mChanges.add(new CompatChange(id, "", -1, true, false, description)); + return this; + } + + CompatConfigBuilder addLoggingOnlyChangeWithId(long id) { + mChanges.add(new CompatChange(id, "", -1, false, true, "")); return this; } diff --git a/services/tests/servicestests/src/com/android/server/compat/CompatConfigTest.java b/services/tests/servicestests/src/com/android/server/compat/CompatConfigTest.java index 44f4ccf69cdd3..eb2dd64610710 100644 --- a/services/tests/servicestests/src/com/android/server/compat/CompatConfigTest.java +++ b/services/tests/servicestests/src/com/android/server/compat/CompatConfigTest.java @@ -213,6 +213,17 @@ public class CompatConfigTest { assertThat(compatConfig.isChangeEnabled(1234L, applicationInfo)).isFalse(); } + @Test + public void testLoggingOnlyChangePreventAddOverride() throws Exception { + CompatConfig compatConfig = CompatConfigBuilder.create(mBuildClassifier, mContext) + .addLoggingOnlyChangeWithId(1234L) + .build(); + + assertThrows(SecurityException.class, + () -> compatConfig.addOverride(1234L, "com.some.package", true) + ); + } + @Test public void testPreventRemoveOverride() throws Exception { CompatConfig compatConfig = CompatConfigBuilder.create(mBuildClassifier, mContext) diff --git a/services/tests/servicestests/src/com/android/server/compat/OverrideValidatorImplTest.java b/services/tests/servicestests/src/com/android/server/compat/OverrideValidatorImplTest.java index b14291b341951..16291b256c449 100644 --- a/services/tests/servicestests/src/com/android/server/compat/OverrideValidatorImplTest.java +++ b/services/tests/servicestests/src/com/android/server/compat/OverrideValidatorImplTest.java @@ -20,6 +20,7 @@ import static com.android.internal.compat.OverrideAllowedState.ALLOWED; import static com.android.internal.compat.OverrideAllowedState.DISABLED_NON_TARGET_SDK; import static com.android.internal.compat.OverrideAllowedState.DISABLED_NOT_DEBUGGABLE; import static com.android.internal.compat.OverrideAllowedState.DISABLED_TARGET_SDK_TOO_HIGH; +import static com.android.internal.compat.OverrideAllowedState.LOGGING_ONLY_CHANGE; import static com.google.common.truth.Truth.assertThat; @@ -78,6 +79,7 @@ public class OverrideValidatorImplTest { @Before public void setUp() throws Exception { MockitoAnnotations.initMocks(this); + android.app.compat.ChangeIdStateCache.disable(); when(mContext.getPackageManager()).thenReturn(mPackageManager); } @@ -89,7 +91,8 @@ public class OverrideValidatorImplTest { .addTargetSdkChangeWithId(TARGET_SDK, 2) .addTargetSdkChangeWithId(TARGET_SDK_AFTER, 3) .addEnabledChangeWithId(4) - .addDisabledChangeWithId(5).build(); + .addDisabledChangeWithId(5) + .addLoggingOnlyChangeWithId(6).build(); IOverrideValidator overrideValidator = config.getOverrideValidator(); when(mPackageManager.getApplicationInfo(eq(PACKAGE_NAME), anyInt())) .thenReturn(ApplicationInfoBuilder.create() @@ -107,6 +110,8 @@ public class OverrideValidatorImplTest { overrideValidator.getOverrideAllowedState(4, PACKAGE_NAME); OverrideAllowedState stateDisabledChange = overrideValidator.getOverrideAllowedState(5, PACKAGE_NAME); + OverrideAllowedState stateDLoggingOnlyChange = + overrideValidator.getOverrideAllowedState(6, PACKAGE_NAME); assertThat(stateTargetSdkLessChange) .isEqualTo(new OverrideAllowedState(ALLOWED, -1, -1)); @@ -118,6 +123,8 @@ public class OverrideValidatorImplTest { .isEqualTo(new OverrideAllowedState(ALLOWED, -1, -1)); assertThat(stateDisabledChange) .isEqualTo(new OverrideAllowedState(ALLOWED, -1, -1)); + assertThat(stateDLoggingOnlyChange) + .isEqualTo(new OverrideAllowedState(LOGGING_ONLY_CHANGE, -1, -1)); } @Test @@ -128,7 +135,8 @@ public class OverrideValidatorImplTest { .addTargetSdkChangeWithId(TARGET_SDK, 2) .addTargetSdkChangeWithId(TARGET_SDK_AFTER, 3) .addEnabledChangeWithId(4) - .addDisabledChangeWithId(5).build(); + .addDisabledChangeWithId(5) + .addLoggingOnlyChangeWithId(6).build(); IOverrideValidator overrideValidator = config.getOverrideValidator(); when(mPackageManager.getApplicationInfo(eq(PACKAGE_NAME), anyInt())) .thenReturn(ApplicationInfoBuilder.create() @@ -145,6 +153,8 @@ public class OverrideValidatorImplTest { overrideValidator.getOverrideAllowedState(4, PACKAGE_NAME); OverrideAllowedState stateDisabledChange = overrideValidator.getOverrideAllowedState(5, PACKAGE_NAME); + OverrideAllowedState stateDLoggingOnlyChange = + overrideValidator.getOverrideAllowedState(6, PACKAGE_NAME); assertThat(stateTargetSdkLessChange) .isEqualTo(new OverrideAllowedState(ALLOWED, -1, -1)); @@ -156,6 +166,8 @@ public class OverrideValidatorImplTest { .isEqualTo(new OverrideAllowedState(ALLOWED, -1, -1)); assertThat(stateDisabledChange) .isEqualTo(new OverrideAllowedState(ALLOWED, -1, -1)); + assertThat(stateDLoggingOnlyChange) + .isEqualTo(new OverrideAllowedState(LOGGING_ONLY_CHANGE, -1, -1)); } @Test @@ -232,7 +244,8 @@ public class OverrideValidatorImplTest { .addTargetSdkChangeWithId(TARGET_SDK, 2) .addTargetSdkChangeWithId(TARGET_SDK_AFTER, 3) .addEnabledChangeWithId(4) - .addDisabledChangeWithId(5).build(); + .addDisabledChangeWithId(5) + .addLoggingOnlyChangeWithId(6).build(); IOverrideValidator overrideValidator = config.getOverrideValidator(); when(mPackageManager.getApplicationInfo(eq(PACKAGE_NAME), anyInt())) .thenReturn(ApplicationInfoBuilder.create() @@ -249,6 +262,8 @@ public class OverrideValidatorImplTest { overrideValidator.getOverrideAllowedState(4, PACKAGE_NAME); OverrideAllowedState stateDisabledChange = overrideValidator.getOverrideAllowedState(5, PACKAGE_NAME); + OverrideAllowedState stateDLoggingOnlyChange = + overrideValidator.getOverrideAllowedState(6, PACKAGE_NAME); assertThat(stateTargetSdkLessChange) .isEqualTo(new OverrideAllowedState(DISABLED_NOT_DEBUGGABLE, -1, -1)); @@ -260,6 +275,8 @@ public class OverrideValidatorImplTest { .isEqualTo(new OverrideAllowedState(DISABLED_NOT_DEBUGGABLE, -1, -1)); assertThat(stateDisabledChange) .isEqualTo(new OverrideAllowedState(DISABLED_NOT_DEBUGGABLE, -1, -1)); + assertThat(stateDLoggingOnlyChange) + .isEqualTo(new OverrideAllowedState(LOGGING_ONLY_CHANGE, -1, -1)); } @Test @@ -351,7 +368,8 @@ public class OverrideValidatorImplTest { .addTargetSdkChangeWithId(TARGET_SDK, 2) .addTargetSdkChangeWithId(TARGET_SDK_AFTER, 3) .addEnabledChangeWithId(4) - .addDisabledChangeWithId(5).build(); + .addDisabledChangeWithId(5) + .addLoggingOnlyChangeWithId(6).build(); IOverrideValidator overrideValidator = config.getOverrideValidator(); when(mPackageManager.getApplicationInfo(eq(PACKAGE_NAME), anyInt())) .thenReturn(ApplicationInfoBuilder.create() @@ -368,6 +386,8 @@ public class OverrideValidatorImplTest { overrideValidator.getOverrideAllowedState(4, PACKAGE_NAME); OverrideAllowedState stateDisabledChange = overrideValidator.getOverrideAllowedState(5, PACKAGE_NAME); + OverrideAllowedState stateDLoggingOnlyChange = + overrideValidator.getOverrideAllowedState(6, PACKAGE_NAME); assertThat(stateTargetSdkLessChange) .isEqualTo(new OverrideAllowedState(DISABLED_NOT_DEBUGGABLE, -1, -1)); @@ -379,5 +399,7 @@ public class OverrideValidatorImplTest { .isEqualTo(new OverrideAllowedState(DISABLED_NOT_DEBUGGABLE, -1, -1)); assertThat(stateDisabledChange) .isEqualTo(new OverrideAllowedState(DISABLED_NOT_DEBUGGABLE, -1, -1)); + assertThat(stateDLoggingOnlyChange) + .isEqualTo(new OverrideAllowedState(LOGGING_ONLY_CHANGE, -1, -1)); } }