Make FabricIndex be a common thing across all sessions. (#14904)

* Move fabric index into session.h instead of SecureSession * Remove the cast to securesession when getting the fabric index * Make group sessions use the common per-session fabric index * Restyle * Address code review comments * Use undefined fabric index as an invalid marker
project-chip · Feb 20, 2024 · 4025024 · 4025024
1 parent 6d5c3ef
commit 4025024
Show file tree

Hide file tree

Showing 14 changed files with 27 additions and 45 deletions.
diff --git a/examples/chip-tool/commands/common/CommandInvoker.h b/examples/chip-tool/commands/common/CommandInvoker.h
@@ -235,8 +235,7 @@ CHIP_ERROR InvokeGroupCommand(DeviceProxy * aDevice, void * aContext,
  //
  // We assume the aDevice already has a Case session which is way we can use he established Secure Session
  ReturnErrorOnFailure(invoker->InvokeGroupCommand(aDevice->GetExchangeManager(),
- aDevice->GetSecureSession().Value()->AsSecureSession()->GetFabricIndex(),
- groupId, aRequestData));
+ aDevice->GetSecureSession().Value()->GetFabricIndex(), groupId, aRequestData));
 
  // invoker is already deleted and is not to be used
  invoker.release();

diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp
@@ -566,17 +566,7 @@ TLV::TLVWriter * CommandHandler::GetCommandDataIBTLVWriter()
 
 FabricIndex CommandHandler::GetAccessingFabricIndex() const
 {
- FabricIndex fabric = kUndefinedFabricIndex;
- if (mpExchangeCtx->GetSessionHandle()->IsGroupSession())
- {
- fabric = mpExchangeCtx->GetSessionHandle()->AsGroupSession()->GetFabricIndex();
- }
- else
- {
- fabric = mpExchangeCtx->GetSessionHandle()->AsSecureSession()->GetFabricIndex();
- }
-
- return fabric;
+ return mpExchangeCtx->GetSessionHandle()->GetFabricIndex();
 }
 
 CommandHandler * CommandHandler::Handle::Get()

diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -276,7 +276,7 @@ CHIP_ERROR InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeConte
  ChipLogProgress(InteractionModel,
  "Deleting previous subscription from NodeId: " ChipLogFormatX64 ", FabricIndex: %" PRIu8,
  ChipLogValueX64(apExchangeContext->GetSessionHandle()->AsSecureSession()->GetPeerNodeId()),
- apExchangeContext->GetSessionHandle()->AsSecureSession()->GetFabricIndex());
+ apExchangeContext->GetSessionHandle()->GetFabricIndex());
  mReadHandlers.ReleaseObject(handler);
  }
 

diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
@@ -258,7 +258,7 @@ CHIP_ERROR ReadClient::SendReadRequest(ReadPrepareParams & aReadPrepareParams)
  Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse)));
 
  mPeerNodeId = aReadPrepareParams.mSessionHolder->AsSecureSession()->GetPeerNodeId();
- mFabricIndex = aReadPrepareParams.mSessionHolder->AsSecureSession()->GetFabricIndex();
+ mFabricIndex = aReadPrepareParams.mSessionHolder->GetFabricIndex();
 
  MoveToState(ClientState::AwaitingInitialReport);
 
@@ -801,7 +801,7 @@ CHIP_ERROR ReadClient::SendSubscribeRequest(ReadPrepareParams & aReadPreparePara
  Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse)));
 
  mPeerNodeId = aReadPrepareParams.mSessionHolder->AsSecureSession()->GetPeerNodeId();
- mFabricIndex = aReadPrepareParams.mSessionHolder->AsSecureSession()->GetFabricIndex();
+ mFabricIndex = aReadPrepareParams.mSessionHolder->GetFabricIndex();
 
  MoveToState(ClientState::AwaitingInitialReport);
 

diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp
@@ -261,7 +261,7 @@ bool ReadHandler::IsFromSubscriber(Messaging::ExchangeContext & apExchangeContex
 {
  return (IsType(InteractionType::Subscribe) &&
  GetInitiatorNodeId() == apExchangeContext.GetSessionHandle()->AsSecureSession()->GetPeerNodeId() &&
- GetAccessingFabricIndex() == apExchangeContext.GetSessionHandle()->AsSecureSession()->GetFabricIndex());
+ GetAccessingFabricIndex() == apExchangeContext.GetSessionHandle()->GetFabricIndex());
 }
 
 CHIP_ERROR ReadHandler::OnUnknownMsgType(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,

diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
@@ -479,17 +479,7 @@ CHIP_ERROR WriteHandler::AddStatus(const ConcreteAttributePath & aPath, const St
 
 FabricIndex WriteHandler::GetAccessingFabricIndex() const
 {
- FabricIndex fabric = kUndefinedFabricIndex;
- if (mpExchangeCtx->GetSessionHandle()->IsGroupSession())
- {
- fabric = mpExchangeCtx->GetSessionHandle()->AsGroupSession()->GetFabricIndex();
- }
- else
- {
- fabric = mpExchangeCtx->GetSessionHandle()->AsSecureSession()->GetFabricIndex();
- }
-
- return fabric;
+ return mpExchangeCtx->GetSessionHandle()->GetFabricIndex();
 }
 
 const char * WriteHandler::GetStateStr() const

diff --git a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp
@@ -150,7 +150,7 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback(
  * Once bindings are implemented, this may no longer be needed.
  */
  SessionHandle handle = commandObj->GetExchangeContext()->GetSessionHandle();
- server->SetFabricIndex(handle->AsSecureSession()->GetFabricIndex());
+ server->SetFabricIndex(handle->GetFabricIndex());
  server->SetPeerNodeId(handle->AsSecureSession()->GetPeerNodeId());
 
  CheckSuccess(server->CommissioningComplete(), Failure);

diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
@@ -351,7 +351,7 @@ class FabricCleanupExchangeDelegate : public chip::Messaging::ExchangeDelegate
  void OnResponseTimeout(chip::Messaging::ExchangeContext * ec) override {}
  void OnExchangeClosing(chip::Messaging::ExchangeContext * ec) override
  {
- FabricIndex currentFabricIndex = ec->GetSessionHandle()->AsSecureSession()->GetFabricIndex();
+ FabricIndex currentFabricIndex = ec->GetSessionHandle()->GetFabricIndex();
  ec->GetExchangeMgr()->GetSessionManager()->ExpireAllPairingsForFabric(currentFabricIndex);
  }
 };

diff --git a/src/controller/CHIPCluster.cpp b/src/controller/CHIPCluster.cpp
@@ -53,7 +53,7 @@ CHIP_ERROR ClusterBase::AssociateWithGroup(DeviceProxy * device, GroupId groupId
  {
  // Local copy to preserve original SessionHandle for future Unicast communication.
  Optional<SessionHandle> session = mDevice->GetExchangeManager()->GetSessionManager()->CreateGroupSession(
- groupId, mDevice->GetSecureSession().Value()->AsSecureSession()->GetFabricIndex());
+ groupId, mDevice->GetSecureSession().Value()->GetFabricIndex());
  // Sanity check
  if (!session.HasValue() || !session.Value()->IsGroupSession())
  {

diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h
@@ -46,6 +46,8 @@ static constexpr FabricIndex kMinValidFabricIndex = 1;
 static constexpr FabricIndex kMaxValidFabricIndex = std::min<FabricIndex>(UINT8_MAX - 1, CHIP_CONFIG_MAX_FABRICS);
 static constexpr uint8_t kFabricLabelMaxLengthInBytes = 32;
 
+static_assert(kUndefinedFabricIndex < chip::kMinValidFabricIndex, "Undefined fabric index should not be valid");
+
 // KVS store is sensitive to length of key strings, based on the underlying
 // platform. Keeping them short.
 constexpr char kFabricTableKeyPrefix[] = "Fabric";

diff --git a/src/transport/GroupSession.h b/src/transport/GroupSession.h
@@ -27,7 +27,7 @@ namespace Transport {
 class GroupSession : public Session
 {
 public:
- GroupSession(GroupId group, FabricIndex fabricIndex) : mGroupId(group), mFabricIndex(fabricIndex) {}
+ GroupSession(GroupId group, FabricIndex fabricIndex) : mGroupId(group) { SetFabricIndex(fabricIndex); }
  ~GroupSession() { NotifySessionReleased(); }
 
  Session::SessionType GetSessionType() const override { return Session::SessionType::kGroup; }
@@ -59,11 +59,9 @@ class GroupSession : public Session
  }
 
  GroupId GetGroupId() const { return mGroupId; }
- FabricIndex GetFabricIndex() const { return mFabricIndex; }
 
 private:
  const GroupId mGroupId;
- const FabricIndex mFabricIndex;
 };
 
 /*

diff --git a/src/transport/SecureSession.cpp b/src/transport/SecureSession.cpp
@@ -28,13 +28,13 @@ Access::SubjectDescriptor SecureSession::GetSubjectDescriptor() const
  subjectDescriptor.authMode = Access::AuthMode::kCase;
  subjectDescriptor.subject = mPeerNodeId;
  subjectDescriptor.cats = mPeerCATs;
- subjectDescriptor.fabricIndex = mFabric;
+ subjectDescriptor.fabricIndex = GetFabricIndex();
  }
  else if (IsPAKEKeyId(mPeerNodeId))
  {
  subjectDescriptor.authMode = Access::AuthMode::kPase;
  subjectDescriptor.subject = mPeerNodeId;
- subjectDescriptor.fabricIndex = mFabric;
+ subjectDescriptor.fabricIndex = GetFabricIndex();
  }
  else
  {

diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h
@@ -67,8 +67,10 @@ class SecureSession : public Session
  FabricIndex fabric, const ReliableMessageProtocolConfig & config) :
  mSecureSessionType(secureSessionType),
  mPeerNodeId(peerNodeId), mPeerCATs(peerCATs), mLocalSessionId(localSessionId), mPeerSessionId(peerSessionId),
- mFabric(fabric), mLastActivityTime(System::SystemClock().GetMonotonicTimestamp()), mMRPConfig(config)
- {}
+ mLastActivityTime(System::SystemClock().GetMonotonicTimestamp()), mMRPConfig(config)
+ {
+ SetFabricIndex(fabric);
+ }
  ~SecureSession() { NotifySessionReleased(); }
 
  SecureSession(SecureSession &&) = delete;
@@ -112,7 +114,6 @@ class SecureSession : public Session
 
  uint16_t GetLocalSessionId() const { return mLocalSessionId; }
  uint16_t GetPeerSessionId() const { return mPeerSessionId; }
- FabricIndex GetFabricIndex() const { return mFabric; }
 
  // Should only be called for PASE sessions, which start with undefined fabric,
  // to migrate to a newly commissioned fabric after successful
@@ -123,10 +124,10 @@ class SecureSession : public Session
  // TODO(#13711): this check won't work until the issue is addressed
  if (mSecureSessionType == Type::kPASE)
  {
- mFabric = fabricIndex;
+ SetFabricIndex(fabricIndex);
  }
 #else
- mFabric = fabricIndex;
+ SetFabricIndex(fabricIndex);
 #endif
  return CHIP_NO_ERROR;
  }
@@ -145,10 +146,6 @@ class SecureSession : public Session
  const uint16_t mLocalSessionId;
  const uint16_t mPeerSessionId;
 
- // PASE sessions start with undefined fabric, but are migrated to a newly
- // commissioned fabric after successful OperationalCredentialsCluster::AddNOC
- FabricIndex mFabric;
-
  PeerAddress mPeerAddress;
  System::Clock::Timestamp mLastActivityTime;
  ReliableMessageProtocolConfig mMRPConfig;

diff --git a/src/transport/Session.h b/src/transport/Session.h
@@ -16,6 +16,7 @@
 
 #pragma once
 
+#include <credentials/FabricTable.h>
 #include <lib/core/CHIPConfig.h>
 #include <messaging/ReliableMessageProtocolConfig.h>
 #include <transport/SessionHolder.h>
@@ -67,6 +68,8 @@ class Session
  virtual const ReliableMessageProtocolConfig & GetMRPConfig() const = 0;
  virtual System::Clock::Milliseconds32 GetAckTimeout() const = 0;
 
+ FabricIndex GetFabricIndex() const { return mFabricIndex; }
+
  SecureSession * AsSecureSession();
  UnauthenticatedSession * AsUnauthenticatedSession();
  GroupSession * AsGroupSession();
@@ -85,8 +88,11 @@ class Session
  }
  }
 
+ void SetFabricIndex(FabricIndex index) { mFabricIndex = index; }
+
 private:
  IntrusiveList<SessionHolder> mHolders;
+ FabricIndex mFabricIndex = kUndefinedFabricIndex;
 };
 
 } // namespace Transport