Updated DefaultVehicleHal to check VehicleAreaConfig.access
Bug: 290801790
Test: atest DefaultVehicleHalTest
Change-Id: I3b7c6c34835ba0ad29a495ed60cd3244eea93576
diff --git a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
index 9a42180..f7a71b4 100644
--- a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
+++ b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
@@ -191,6 +191,10 @@
const std::vector<aidl::android::hardware::automotive::vehicle::SubscribeOptions>&
options);
+ VhalResult<void> checkPermissionHelper(
+ const aidl::android::hardware::automotive::vehicle::VehiclePropValue& value,
+ aidl::android::hardware::automotive::vehicle::VehiclePropertyAccess accessToTest) const;
+
VhalResult<void> checkReadPermission(
const aidl::android::hardware::automotive::vehicle::VehiclePropValue& value) const;
diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
index d85cc09..847f3b8 100644
--- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
@@ -605,6 +605,23 @@
return vectorToStableLargeParcelable(std::move(configs), output);
}
+bool hasRequiredAccess(VehiclePropertyAccess access, VehiclePropertyAccess requiredAccess) {
+ return access == requiredAccess || access == VehiclePropertyAccess::READ_WRITE;
+}
+
+bool areaConfigsHaveRequiredAccess(const std::vector<VehicleAreaConfig>& areaConfigs,
+ VehiclePropertyAccess requiredAccess) {
+ if (areaConfigs.empty()) {
+ return false;
+ }
+ for (VehicleAreaConfig areaConfig : areaConfigs) {
+ if (!hasRequiredAccess(areaConfig.access, requiredAccess)) {
+ return false;
+ }
+ }
+ return true;
+}
+
VhalResult<void> DefaultVehicleHal::checkSubscribeOptions(
const std::vector<SubscribeOptions>& options) {
for (const auto& option : options) {
@@ -614,6 +631,26 @@
<< StringPrintf("no config for property, ID: %" PRId32, propId);
}
const VehiclePropConfig& config = mConfigsByPropId[propId];
+ std::vector<VehicleAreaConfig> areaConfigs;
+ if (option.areaIds.empty()) {
+ areaConfigs = config.areaConfigs;
+ } else {
+ std::unordered_map<int, VehicleAreaConfig> areaConfigByAreaId;
+ for (const VehicleAreaConfig& areaConfig : config.areaConfigs) {
+ areaConfigByAreaId.emplace(areaConfig.areaId, areaConfig);
+ }
+ for (int areaId : option.areaIds) {
+ auto it = areaConfigByAreaId.find(areaId);
+ if (it != areaConfigByAreaId.end()) {
+ areaConfigs.push_back(it->second);
+ } else if (areaId != 0 || !areaConfigByAreaId.empty()) {
+ return StatusError(StatusCode::INVALID_ARG)
+ << StringPrintf("invalid area ID: %" PRId32 " for prop ID: %" PRId32
+ ", not listed in config",
+ areaId, propId);
+ }
+ }
+ }
if (config.changeMode != VehiclePropertyChangeMode::ON_CHANGE &&
config.changeMode != VehiclePropertyChangeMode::CONTINUOUS) {
@@ -621,8 +658,9 @@
<< "only support subscribing to ON_CHANGE or CONTINUOUS property";
}
- if (config.access != VehiclePropertyAccess::READ &&
- config.access != VehiclePropertyAccess::READ_WRITE) {
+ // Either VehiclePropConfig.access or VehicleAreaConfig.access will be specified
+ if (!hasRequiredAccess(config.access, VehiclePropertyAccess::READ) &&
+ !areaConfigsHaveRequiredAccess(areaConfigs, VehiclePropertyAccess::READ)) {
return StatusError(StatusCode::ACCESS_DENIED)
<< StringPrintf("Property %" PRId32 " has no read access", propId);
}
@@ -644,20 +682,6 @@
<< "invalid sample rate: " << sampleRateHz << " HZ";
}
}
-
- if (isGlobalProp(propId)) {
- continue;
- }
-
- // Non-global property.
- for (int32_t areaId : option.areaIds) {
- if (auto areaConfig = getAreaConfig(propId, areaId, config); areaConfig == nullptr) {
- return StatusError(StatusCode::INVALID_ARG)
- << StringPrintf("invalid area ID: %" PRId32 " for prop ID: %" PRId32
- ", not listed in config",
- areaId, propId);
- }
- }
}
return {};
}
@@ -776,36 +800,42 @@
return mVehicleHardware.get();
}
-VhalResult<void> DefaultVehicleHal::checkWritePermission(const VehiclePropValue& value) const {
+VhalResult<void> DefaultVehicleHal::checkPermissionHelper(
+ const VehiclePropValue& value, VehiclePropertyAccess accessToTest) const {
+ static const std::unordered_set<VehiclePropertyAccess> validAccesses = {
+ VehiclePropertyAccess::WRITE, VehiclePropertyAccess::READ,
+ VehiclePropertyAccess::READ_WRITE};
+ if (validAccesses.find(accessToTest) == validAccesses.end()) {
+ return StatusError(StatusCode::INVALID_ARG)
+ << "checkPermissionHelper parameter is an invalid access type";
+ }
+
int32_t propId = value.prop;
auto result = getConfig(propId);
if (!result.ok()) {
return StatusError(StatusCode::INVALID_ARG) << getErrorMsg(result);
}
const VehiclePropConfig* config = result.value();
+ const VehicleAreaConfig* areaConfig = getAreaConfig(value, *config);
- if (config->access != VehiclePropertyAccess::WRITE &&
- config->access != VehiclePropertyAccess::READ_WRITE) {
+ if (areaConfig == nullptr && !isGlobalProp(propId)) {
+ return StatusError(StatusCode::INVALID_ARG) << "no config for area ID: " << value.areaId;
+ }
+ if (!hasRequiredAccess(config->access, accessToTest) &&
+ (areaConfig == nullptr || !hasRequiredAccess(areaConfig->access, accessToTest))) {
return StatusError(StatusCode::ACCESS_DENIED)
- << StringPrintf("Property %" PRId32 " has no write access", propId);
+ << StringPrintf("Property %" PRId32 " does not have the following access: %" PRId32,
+ propId, accessToTest);
}
return {};
}
-VhalResult<void> DefaultVehicleHal::checkReadPermission(const VehiclePropValue& value) const {
- int32_t propId = value.prop;
- auto result = getConfig(propId);
- if (!result.ok()) {
- return StatusError(StatusCode::INVALID_ARG) << getErrorMsg(result);
- }
- const VehiclePropConfig* config = result.value();
+VhalResult<void> DefaultVehicleHal::checkWritePermission(const VehiclePropValue& value) const {
+ return checkPermissionHelper(value, VehiclePropertyAccess::WRITE);
+}
- if (config->access != VehiclePropertyAccess::READ &&
- config->access != VehiclePropertyAccess::READ_WRITE) {
- return StatusError(StatusCode::ACCESS_DENIED)
- << StringPrintf("Property %" PRId32 " has no read access", propId);
- }
- return {};
+VhalResult<void> DefaultVehicleHal::checkReadPermission(const VehiclePropValue& value) const {
+ return checkPermissionHelper(value, VehiclePropertyAccess::READ);
}
void DefaultVehicleHal::checkHealth(IVehicleHardware* vehicleHardware,
diff --git a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp
index 7195d97..63964ef 100644
--- a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp
@@ -100,6 +100,10 @@
constexpr int32_t WRITE_ONLY_PROP = 10007 + 0x10000000 + 0x01000000 + 0x00400000;
// VehiclePropertyGroup:SYSTEM,VehicleArea:GLOBAL,VehiclePropertyType:INT32
constexpr int32_t GLOBAL_CONTINUOUS_PROP_NO_VUR = 10008 + 0x10000000 + 0x01000000 + 0x00400000;
+// VehiclePropertyGroup:SYSTEM,VehicleArea:GLOBAL,VehiclePropertyType:INT32
+constexpr int32_t GLOBAL_NONE_ACCESS_PROP = 10009 + 0x10000000 + 0x01000000 + 0x00400000;
+// VehiclePropertyGroup:SYSTEM,VehicleArea:WINDOW,VehiclePropertyType:INT32
+constexpr int32_t AREA_NONE_ACCESS_PROP = 10010 + 0x10000000 + 0x03000000 + 0x00400000;
int32_t testInt32VecProp(size_t i) {
// VehiclePropertyGroup:SYSTEM,VehicleArea:GLOBAL,VehiclePropertyType:INT32_VEC
@@ -175,6 +179,26 @@
.value.int32Values = {0},
},
.expectedStatus = StatusCode::ACCESS_DENIED,
+ },
+ {
+ .name = "none_access",
+ .request =
+ {
+ .prop = GLOBAL_NONE_ACCESS_PROP,
+ .value.int32Values = {0},
+ },
+ .expectedStatus = StatusCode::ACCESS_DENIED,
+ },
+ {
+ .name = "none_area_access",
+ .request =
+ {
+ .prop = AREA_NONE_ACCESS_PROP,
+ .value.int32Values = {0},
+ // Only ROW_1_LEFT is allowed.
+ .areaId = toInt(VehicleAreaWindow::ROW_1_RIGHT),
+ },
+ .expectedStatus = StatusCode::ACCESS_DENIED,
}};
}
@@ -228,11 +252,11 @@
for (size_t i = 0; i < 10000; i++) {
testConfigs.push_back(VehiclePropConfig{
.prop = testInt32VecProp(i),
- .access = VehiclePropertyAccess::READ_WRITE,
.areaConfigs =
{
{
.areaId = 0,
+ .access = VehiclePropertyAccess::READ_WRITE,
.minInt32Value = 0,
.maxInt32Value = 100,
},
@@ -242,9 +266,9 @@
// A property with area config.
testConfigs.push_back(
VehiclePropConfig{.prop = INT32_WINDOW_PROP,
- .access = VehiclePropertyAccess::READ_WRITE,
.areaConfigs = {{
.areaId = toInt(VehicleAreaWindow::ROW_1_LEFT),
+ .access = VehiclePropertyAccess::READ_WRITE,
.minInt32Value = 0,
.maxInt32Value = 100,
}}});
@@ -275,18 +299,19 @@
// A per-area on-change property.
testConfigs.push_back(VehiclePropConfig{
.prop = AREA_ON_CHANGE_PROP,
- .access = VehiclePropertyAccess::READ_WRITE,
.changeMode = VehiclePropertyChangeMode::ON_CHANGE,
.areaConfigs =
{
{
.areaId = toInt(VehicleAreaWindow::ROW_1_LEFT),
+ .access = VehiclePropertyAccess::READ_WRITE,
.minInt32Value = 0,
.maxInt32Value = 100,
},
{
.areaId = toInt(VehicleAreaWindow::ROW_1_RIGHT),
+ .access = VehiclePropertyAccess::READ,
.minInt32Value = 0,
.maxInt32Value = 100,
},
@@ -295,7 +320,6 @@
// A per-area continuous property.
testConfigs.push_back(VehiclePropConfig{
.prop = AREA_CONTINUOUS_PROP,
- .access = VehiclePropertyAccess::READ_WRITE,
.changeMode = VehiclePropertyChangeMode::CONTINUOUS,
.minSampleRate = 0.0,
.maxSampleRate = 1000.0,
@@ -304,12 +328,14 @@
{
.areaId = toInt(VehicleAreaWindow::ROW_1_LEFT),
+ .access = VehiclePropertyAccess::READ_WRITE,
.minInt32Value = 0,
.maxInt32Value = 100,
.supportVariableUpdateRate = true,
},
{
.areaId = toInt(VehicleAreaWindow::ROW_1_RIGHT),
+ .access = VehiclePropertyAccess::READ_WRITE,
.minInt32Value = 0,
.maxInt32Value = 100,
.supportVariableUpdateRate = false,
@@ -332,6 +358,37 @@
.minSampleRate = 0.0,
.maxSampleRate = 1000.0,
});
+ // Global access set to NONE
+ testConfigs.push_back(VehiclePropConfig{
+ .prop = GLOBAL_NONE_ACCESS_PROP,
+ .access = VehiclePropertyAccess::NONE,
+ .changeMode = VehiclePropertyChangeMode::CONTINUOUS,
+ .minSampleRate = 0.0,
+ .maxSampleRate = 100.0,
+ });
+ // Area access set to NONE
+ testConfigs.push_back(VehiclePropConfig{
+ .prop = AREA_NONE_ACCESS_PROP,
+ .changeMode = VehiclePropertyChangeMode::CONTINUOUS,
+ .minSampleRate = 0.0,
+ .maxSampleRate = 1000.0,
+ .areaConfigs =
+ {
+ {
+
+ .areaId = toInt(VehicleAreaWindow::ROW_1_LEFT),
+ .access = VehiclePropertyAccess::NONE,
+ .minInt32Value = 0,
+ .maxInt32Value = 100,
+ },
+ {
+ .areaId = toInt(VehicleAreaWindow::ROW_1_RIGHT),
+ .access = VehiclePropertyAccess::NONE,
+ .minInt32Value = 0,
+ .maxInt32Value = 100,
+ },
+ },
+ });
// Register the heartbeat event property.
testConfigs.push_back(VehiclePropConfig{
.prop = toInt(VehicleProperty::VHAL_HEARTBEAT),
@@ -673,6 +730,21 @@
.prop = WRITE_ONLY_PROP,
},
},
+ {
+ .requestId = 1,
+ .prop =
+ {
+ .prop = GLOBAL_NONE_ACCESS_PROP,
+ },
+ },
+ {
+ .requestId = 2,
+ .prop =
+ {
+ .prop = AREA_NONE_ACCESS_PROP,
+ .areaId = toInt(VehicleAreaWindow::ROW_1_LEFT),
+ },
+ },
},
};
@@ -690,6 +762,14 @@
.requestId = 0,
.status = StatusCode::ACCESS_DENIED,
},
+ {
+ .requestId = 1,
+ .status = StatusCode::ACCESS_DENIED,
+ },
+ {
+ .requestId = 2,
+ .status = StatusCode::ACCESS_DENIED,
+ },
}))
<< "expect to get ACCESS_DENIED status if no read permission";
}
@@ -1316,8 +1396,8 @@
auto maybeResults = getCallback()->nextOnPropertyEventResults();
ASSERT_TRUE(maybeResults.has_value()) << "no results in callback";
- ASSERT_THAT(maybeResults.value().payloads, UnorderedElementsAre(testValue1, testValue2))
- << "results mismatch, expect two on-change events for all updated areas";
+ ASSERT_THAT(maybeResults.value().payloads, UnorderedElementsAre(testValue1))
+ << "results mismatch, expect one on-change events for all updated areas";
ASSERT_FALSE(getCallback()->nextOnPropertyEventResults().has_value())
<< "more results than expected";
}
@@ -1627,6 +1707,27 @@
ASSERT_EQ(status.getServiceSpecificError(), toInt(StatusCode::ACCESS_DENIED));
}
+TEST_F(DefaultVehicleHalTest, testSubscribeGlobalNoneAccess) {
+ std::vector<SubscribeOptions> options = {{
+ .propId = GLOBAL_NONE_ACCESS_PROP,
+ }};
+
+ auto status = getClient()->subscribe(getCallbackClient(), options, 0);
+
+ ASSERT_FALSE(status.isOk()) << "subscribe to a property with NONE global access must fail";
+ ASSERT_EQ(status.getServiceSpecificError(), toInt(StatusCode::ACCESS_DENIED));
+}
+
+TEST_F(DefaultVehicleHalTest, testSubscribeAreaNoneAccess) {
+ std::vector<SubscribeOptions> options = {
+ {.propId = AREA_NONE_ACCESS_PROP, .areaIds = {toInt(VehicleAreaWindow::ROW_1_LEFT)}}};
+
+ auto status = getClient()->subscribe(getCallbackClient(), options, 0);
+
+ ASSERT_FALSE(status.isOk()) << "subscribe to a property with NONE area access must fail";
+ ASSERT_EQ(status.getServiceSpecificError(), toInt(StatusCode::ACCESS_DENIED));
+}
+
TEST_F(DefaultVehicleHalTest, testUnsubscribeFailure) {
auto status = getClient()->unsubscribe(getCallbackClient(),
std::vector<int32_t>({GLOBAL_ON_CHANGE_PROP}));
@@ -1881,7 +1982,7 @@
};
SetValueResult result3 = {
.requestId = 3,
- .status = StatusCode::OK,
+ .status = StatusCode::ACCESS_DENIED,
};
// Prepare the responses
for (int i = 0; i < 2; i++) {
@@ -1907,9 +2008,8 @@
auto maybeResults = getCallback()->nextOnPropertyEventResults();
ASSERT_TRUE(maybeResults.has_value()) << "no results in callback";
- ASSERT_THAT(maybeResults.value().payloads,
- UnorderedElementsAre(testValue1, testValue2, testValue3))
- << "results mismatch, expect 3 batched on change events";
+ ASSERT_THAT(maybeResults.value().payloads, UnorderedElementsAre(testValue1, testValue2))
+ << "results mismatch, expect 2 batched on change events";
ASSERT_FALSE(getCallback()->nextOnPropertyEventResults().has_value())
<< "more results than expected";
}