M-ACL: switch from UnsupportedAccess to AccessRestricted (#35263)

* Implement new AccessRestricted error code * [NXP][Zephyr] Provide AP band in connection request parameters (#35181) Signed-off-by: Axel Le Bourhis <axel.lebourhis@nxp.com> * added missed ERROR_CODES.md update * fixed build issue * restyled * fix return type in CodegenDataModelProvider_Write.cpp * fix review comments --------- Signed-off-by: Axel Le Bourhis <axel.lebourhis@nxp.com> Co-authored-by: Axel Le Bourhis <45206070+axelnxp@users.noreply.github.com>
project-chip · Sep 25, 2024 · 2077649 · 2077649
1 parent 390c66c
commit 2077649
Show file tree

Hide file tree

Showing 18 changed files with 55 additions and 51 deletions.
diff --git a/docs/ERROR_CODES.md b/docs/ERROR_CODES.md
@@ -118,6 +118,7 @@ This file was **AUTOMATICALLY** generated by
 | 165 | 0xA5 | `CHIP_ERROR_ACCESS_DENIED` |
 | 166 | 0xA6 | `CHIP_ERROR_UNKNOWN_RESOURCE_ID` |
 | 167 | 0xA7 | `CHIP_ERROR_VERSION_MISMATCH` |
+| 168 | 0xA8 | `CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL` |
 | 171 | 0xAB | `CHIP_ERROR_EVENT_ID_FOUND` |
 | 172 | 0xAC | `CHIP_ERROR_INTERNAL` |
 | 173 | 0xAD | `CHIP_ERROR_OPEN_FAILED` |
@@ -252,6 +253,7 @@ This file was **AUTOMATICALLY** generated by
 | 1426 | 0x592 | `DATA_VERSION_MISMATCH` |
 | 1428 | 0x594 | `TIMEOUT` |
 | 1436 | 0x59C | `BUSY` |
+| 1437 | 0x59D | `ACCESS_RESTRICTED` |
 | 1475 | 0x5C3 | `UNSUPPORTED_CLUSTER` |
 | 1477 | 0x5C5 | `NO_UPSTREAM_SUBSCRIPTION` |
 | 1478 | 0x5C6 | `NEEDS_TIMED_INTERACTION` |

diff --git a/src/access/AccessControl.cpp b/src/access/AccessControl.cpp
@@ -538,12 +538,7 @@ CHIP_ERROR AccessControl::CheckARL(const SubjectDescriptor & subjectDescriptor,
  if (result != CHIP_NO_ERROR)
  {
  ChipLogProgress(DataManagement, "AccessControl: %s",
-#if 0
- // TODO(#35177): new error code coming when access check plumbing are fixed in callers
  (result == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL) ? "denied (restricted)" : "denied (restriction error)");
-#else
- (result == CHIP_ERROR_ACCESS_DENIED) ? "denied (restricted)" : "denied (restriction error)");
-#endif
  return result;
  }
 

diff --git a/src/access/AccessRestrictionProvider.cpp b/src/access/AccessRestrictionProvider.cpp
@@ -197,45 +197,25 @@ CHIP_ERROR AccessRestrictionProvider::DoCheck(const std::vector<Entry> & entries
  if (requestPath.requestType == RequestType::kAttributeReadRequest ||
  requestPath.requestType == RequestType::kAttributeWriteRequest)
  {
-#if 0
- // TODO(#35177): use new ARL error code when access checks are fixed
  return CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL;
-#else
- return CHIP_ERROR_ACCESS_DENIED;
-#endif
  }
  break;
  case Type::kAttributeWriteForbidden:
  if (requestPath.requestType == RequestType::kAttributeWriteRequest)
  {
-#if 0
- // TODO(#35177): use new ARL error code when access checks are fixed
  return CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL;
-#else
- return CHIP_ERROR_ACCESS_DENIED;
-#endif
  }
  break;
  case Type::kCommandForbidden:
  if (requestPath.requestType == RequestType::kCommandInvokeRequest)
  {
-#if 0
- // TODO(#35177): use new ARL error code when access checks are fixed
  return CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL;
-#else
- return CHIP_ERROR_ACCESS_DENIED;
-#endif
  }
  break;
  case Type::kEventForbidden:
  if (requestPath.requestType == RequestType::kEventReadRequest)
  {
-#if 0
- // TODO(#35177): use new ARL error code when access checks are fixed
  return CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL;
-#else
- return CHIP_ERROR_ACCESS_DENIED;
-#endif
  }
  break;
  }

diff --git a/src/access/tests/TestAccessRestrictionProvider.cpp b/src/access/tests/TestAccessRestrictionProvider.cpp
@@ -174,7 +174,7 @@ void RunChecks(const CheckData * checkData, size_t count)
 {
  for (size_t i = 0; i < count; i++)
  {
- CHIP_ERROR expectedResult = checkData[i].allow ? CHIP_NO_ERROR : CHIP_ERROR_ACCESS_DENIED;
+ CHIP_ERROR expectedResult = checkData[i].allow ? CHIP_NO_ERROR : CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL;
  EXPECT_EQ(accessControl.Check(checkData[i].subjectDescriptor, checkData[i].requestPath, checkData[i].privilege),
  expectedResult);
  }

diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp
@@ -410,12 +410,13 @@ Status CommandHandlerImpl::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
  err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege);
  if (err != CHIP_NO_ERROR)
  {
- if (err != CHIP_ERROR_ACCESS_DENIED)
+ if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL))
  {
  return FallibleAddStatus(concretePath, Status::Failure) != CHIP_NO_ERROR ? Status::Failure : Status::Success;
  }
  // TODO: when wildcard invokes are supported, handle them to discard rather than fail with status
- return FallibleAddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR ? Status::Failure : Status::Success;
+ Status status = err == CHIP_ERROR_ACCESS_DENIED ? Status::UnsupportedAccess : Status::AccessRestricted;
+ return FallibleAddStatus(concretePath, status) != CHIP_NO_ERROR ? Status::Failure : Status::Success;
  }
  }
 

diff --git a/src/app/EventManagement.cpp b/src/app/EventManagement.cpp
@@ -563,7 +563,9 @@ CHIP_ERROR EventManagement::CheckEventContext(EventLoadOutContext * eventLoadOut
  Access::GetAccessControl().Check(eventLoadOutContext->mSubjectDescriptor, requestPath, requestPrivilege);
  if (accessControlError != CHIP_NO_ERROR)
  {
- ReturnErrorCodeIf(accessControlError != CHIP_ERROR_ACCESS_DENIED, accessControlError);
+ ReturnErrorCodeIf((accessControlError != CHIP_ERROR_ACCESS_DENIED) &&
+ (accessControlError != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL),
+ accessControlError);
  ret = CHIP_ERROR_UNEXPECTED_EVENT;
  }
 

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp
@@ -281,15 +281,17 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data
  RequiredPrivilege::ForReadAttribute(request.path));
  if (err != CHIP_NO_ERROR)
  {
- ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
+ ReturnErrorCodeIf((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);
 
  // Implementation of 8.4.3.2 of the spec for path expansion
  if (request.path.mExpanded)
  {
  return CHIP_NO_ERROR;
  }
- // access denied has a specific code for IM
- return CHIP_IM_GLOBAL_STATUS(UnsupportedAccess);
+
+ // access denied and access restricted have specific codes for IM
+ return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)
+ : CHIP_IM_GLOBAL_STATUS(AccessRestricted);
  }
  }
 

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp
@@ -287,10 +287,10 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat
 
  if (err != CHIP_NO_ERROR)
  {
- ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
+ ReturnErrorCodeIf((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);
 
  // TODO: when wildcard/group writes are supported, handle them to discard rather than fail with status
- return Status::UnsupportedAccess;
+ return err == CHIP_ERROR_ACCESS_DENIED ? Status::UnsupportedAccess : Status::AccessRestricted;
  }
  }
 

diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -346,23 +346,26 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool &
  Access::Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path);
 
  err = Access::GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege);
- if (err != CHIP_ERROR_ACCESS_DENIED)
+ if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL))
  {
  ReturnErrorOnFailure(err);
  }
  else
  {
  TLV::TLVWriter checkpoint = aWriter;
- err = EventReportIB::ConstructEventStatusIB(aWriter, path, StatusIB(Status::UnsupportedAccess));
+ err = EventReportIB::ConstructEventStatusIB(aWriter, path,
+ err == CHIP_ERROR_ACCESS_DENIED ? StatusIB(Status::UnsupportedAccess)
+ : StatusIB(Status::AccessRestricted));
+
  if (err != CHIP_NO_ERROR)
  {
  aWriter = checkpoint;
  break;
  }
  aHasEncodedData = true;
- ChipLogDetail(InteractionModel, "Access to event (%u, " ChipLogFormatMEI ", " ChipLogFormatMEI ") denied by ACL",
+ ChipLogDetail(InteractionModel, "Access to event (%u, " ChipLogFormatMEI ", " ChipLogFormatMEI ") denied by %s",
  current->mValue.mEndpointId, ChipLogValueMEI(current->mValue.mClusterId),
- ChipLogValueMEI(current->mValue.mEventId));
+ ChipLogValueMEI(current->mValue.mEventId), err == CHIP_ERROR_ACCESS_DENIED ? "ACL" : "ARL");
  }
  current = current->mpNext;
  }

diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp
@@ -302,12 +302,13 @@ CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, b
  CHIP_ERROR err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, requestPrivilege);
  if (err != CHIP_NO_ERROR)
  {
- ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
+ ReturnErrorCodeIf((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);
  if (aPath.mExpanded)
  {
  return CHIP_NO_ERROR;
  }
- return CHIP_IM_GLOBAL_STATUS(UnsupportedAccess);
+ return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)
+ : CHIP_IM_GLOBAL_STATUS(AccessRestricted);
  }
  }
 
@@ -701,9 +702,12 @@ CHIP_ERROR WriteSingleClusterData(const SubjectDescriptor & aSubjectDescriptor,
  }
  if (err != CHIP_NO_ERROR)
  {
- ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
+ ReturnErrorCodeIf((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);
  // TODO: when wildcard/group writes are supported, handle them to discard rather than fail with status
- return apWriteHandler->AddStatus(aPath, Protocols::InteractionModel::Status::UnsupportedAccess);
+ return apWriteHandler->AddStatus(aPath,
+ err == CHIP_ERROR_ACCESS_DENIED
+ ? Protocols::InteractionModel::Status::UnsupportedAccess
+ : Protocols::InteractionModel::Status::AccessRestricted);
  }
  apWriteHandler->CacheACLCheckResult({ aPath, requestPrivilege });
  }

diff --git a/src/controller/java/src/chip/devicecontroller/model/Status.java b/src/controller/java/src/chip/devicecontroller/model/Status.java
@@ -57,6 +57,7 @@ public enum Code {
  Reserved99(0x99),
  Reserved9a(0x9a),
  Busy(0x9c),
+ AccessRestricted(0x9d),
  Deprecatedc0(0xc0),
  Deprecatedc1(0xc1),
  Deprecatedc2(0xc2),

diff --git a/src/controller/java/src/matter/controller/model/Status.kt b/src/controller/java/src/matter/controller/model/Status.kt
@@ -57,6 +57,7 @@ data class Status(val status: Int, val clusterStatus: Int?) {
  RESERVED99(0X99),
  RESERVED9A(0X9A),
  BUSY(0X9C),
+ ACCESS_RESTRICTED(0x9D),
  DEPRECATEDC0(0XC0),
  DEPRECATEDC1(0XC1),
  DEPRECATEDC2(0XC2),

diff --git a/src/controller/python/chip/interaction_model/__init__.py b/src/controller/python/chip/interaction_model/__init__.py
@@ -73,6 +73,7 @@ class Status(enum.IntEnum):
  Reserved99 = 0x99
  Reserved9a = 0x9a
  Busy = 0x9c
+ AccessRestricted = 0x9d
  Deprecatedc0 = 0xc0
  Deprecatedc1 = 0xc1
  Deprecatedc2 = 0xc2

diff --git a/src/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp
@@ -362,6 +362,9 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err)
  case CHIP_ERROR_VERSION_MISMATCH.AsInteger():
  desc = "Version mismatch";
  break;
+ case CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL.AsInteger():
+ desc = "The CHIP message's access is restricted by ARL";
+ break;
  case CHIP_EVENT_ID_FOUND.AsInteger():
  desc = "Event ID matching criteria was found";
  break;

diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h
@@ -1484,7 +1484,14 @@ using CHIP_ERROR = ::chip::ChipError;
  */
 #define CHIP_ERROR_VERSION_MISMATCH CHIP_CORE_ERROR(0xa7)
 
-// AVAILABLE: 0xa8
+/**
+ * @def CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL
+ *
+ * @brief
+ * The CHIP message is not granted access for further processing due to Access Restriction List.
+ */
+#define CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL CHIP_CORE_ERROR(0xa8)
+
 // AVAILABLE: 0xa9
 // AVAILABLE: 0xaa
 

diff --git a/src/lib/core/tests/TestCHIPErrorStr.cpp b/src/lib/core/tests/TestCHIPErrorStr.cpp
@@ -140,6 +140,7 @@ static const CHIP_ERROR kTestElements[] =
  CHIP_ERROR_ACCESS_DENIED,
  CHIP_ERROR_UNKNOWN_RESOURCE_ID,
  CHIP_ERROR_VERSION_MISMATCH,
+ CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL,
  CHIP_EVENT_ID_FOUND,
  CHIP_ERROR_INTERNAL,
  CHIP_ERROR_OPEN_FAILED,

diff --git a/src/protocols/interaction_model/StatusCodeList.h b/src/protocols/interaction_model/StatusCodeList.h
@@ -61,6 +61,7 @@ CHIP_IM_STATUS_CODE(Reserved98 , Reserved98 , 0x98)
 CHIP_IM_STATUS_CODE(Reserved99 , Reserved99 , 0x99)
 CHIP_IM_STATUS_CODE(Reserved9a , Reserved9a , 0x9a)
 CHIP_IM_STATUS_CODE(Busy , BUSY , 0x9c)
+CHIP_IM_STATUS_CODE(AccessRestricted , ACCESS_RESTRICTED , 0x9d)
 CHIP_IM_STATUS_CODE(Deprecatedc0 , Deprecatedc0 , 0xc0)
 CHIP_IM_STATUS_CODE(Deprecatedc1 , Deprecatedc1 , 0xc1)
 CHIP_IM_STATUS_CODE(Deprecatedc2 , Deprecatedc2 , 0xc2)

diff --git a/src/python_testing/TC_ACL_2_11.py b/src/python_testing/TC_ACL_2_11.py
@@ -73,9 +73,9 @@ def steps_TC_ACL_2_11(self) -> list[TestStep]:
  TestStep(2, "TH1 reads DUT Endpoint 0 AccessControl cluster CommissioningARL attribute"),
  TestStep(3, "TH1 reads DUT Endpoint 0 AccessControl cluster ARL attribute"),
  TestStep(4, "For each entry in ARL, iterate over each restriction and attempt access the restriction's ID on the Endpoint and Cluster in the ARL entry.",
- "If the restriction is Type AttributeAccessForbidden, read the restriction's attribute ID and verify the response is UNSUPPORTED_ACCESS."
- "If the restriction is Type AttributeWriteForbidden, write restriction's the attribute ID and verify the response is UNSUPPORTED_ACCESS."
- "If the restriction is Type CommandForbidden, invoke the restriction's command ID and verify the response is UNSUPPORTED_ACCESS."),
+ "If the restriction is Type AttributeAccessForbidden, read the restriction's attribute ID and verify the response is ACCESS_RESTRICTED."
+ "If the restriction is Type AttributeWriteForbidden, write restriction's the attribute ID and verify the response is ACCESS_RESTRICTED."
+ "If the restriction is Type CommandForbidden, invoke the restriction's command ID and verify the response is ACCESS_RESTRICTED."),
  TestStep(5, "TH1 sends DUT Endpoint 0 AccessControl cluster command ReviewFabricRestrictions"),
  TestStep(6, "Wait for up to 1 hour. Follow instructions provided by device maker to remove all access restrictions",
  "AccessRestrictionReviewUpdate event is received"),
@@ -119,15 +119,15 @@ async def test_TC_ACL_2_11(self):
  command = ALL_ACCEPTED_COMMANDS[C1][ID1]
 
  if restriction_type == AccessControl.Enums.AccessRestrictionTypeEnum.kAttributeAccessForbidden:
- await self.read_single_attribute_expect_error(cluster=cluster, attribute=attribute, error=Status.UnsupportedAccess, endpoint=E1)
+ await self.read_single_attribute_expect_error(cluster=cluster, attribute=attribute, error=Status.AccessRestricted, endpoint=E1)
  elif restriction_type == AccessControl.Enums.AccessRestrictionTypeEnum.kAttributeWriteForbidden:
  status = await self.write_single_attribute(attribute_value=attribute, endpoint_id=E1)
- asserts.assert_equal(status, Status.UnsupportedAccess,
- f"Failed to verify UNSUPPORTED_ACCESS when writing to Attribute {ID1} Cluster {C1} Endpoint {E1}")
+ asserts.assert_equal(status, Status.AccessRestricted,
+ f"Failed to verify ACCESS_RESTRICTED when writing to Attribute {ID1} Cluster {C1} Endpoint {E1}")
  elif restriction_type == AccessControl.Enums.AccessRestrictionTypeEnum.kCommandForbidden:
  result = await self.send_single_cmd(cmd=command, endpoint=E1)
- asserts.assert_equal(result.status, Status.UnsupportedAccess,
- f"Failed to verify UNSUPPORTED_ACCESS when sending command {ID1} to Cluster {C1} Endpoint {E1}")
+ asserts.assert_equal(result.status, Status.AccessRestricted,
+ f"Failed to verify ACCESS_RESTRICTED when sending command {ID1} to Cluster {C1} Endpoint {E1}")
 
  # Belongs to step 6, but needs to be subscribed before executing step 5: begin
  arru_queue = queue.Queue()