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 9, 2022 · 1744422 · 1744422
1 parent f4901af
commit 1744422
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