Make group session ephemeral (#15708)

* Make group session ephemeral * Resolve comments * Resolve comments * Resolve conflict
project-chip · Mar 3, 2022 · c275d1f · c275d1f
1 parent af158c6
commit c275d1f
Show file tree

Hide file tree

Showing 19 changed files with 152 additions and 184 deletions.
diff --git a/examples/chip-tool/commands/clusters/ClusterCommand.h b/examples/chip-tool/commands/clusters/ClusterCommand.h
@@ -126,15 +126,8 @@ class ClusterCommand : public ModelCommand, public chip::app::CommandSender::Cal
         VerifyOrReturnError(commandSender != nullptr, CHIP_ERROR_NO_MEMORY);
         ReturnErrorOnFailure(commandSender->AddRequestDataNoTimedCheck(commandPath, value, mTimedInteractionTimeoutMs));
 
-        chip::Optional<chip::SessionHandle> session =
-            exchangeManager->GetSessionManager()->CreateGroupSession(groupId, fabricIndex, senderNodeId);
-        if (!session.HasValue())
-        {
-            return CHIP_ERROR_NO_MEMORY;
-        }
-        CHIP_ERROR err = commandSender->SendGroupCommandRequest(session.Value());
-        exchangeManager->GetSessionManager()->RemoveGroupSession(session.Value()->AsGroupSession());
-        ReturnErrorOnFailure(err);
+        chip::Transport::OutgoingGroupSession session(groupId, fabricIndex, senderNodeId);
+        ReturnErrorOnFailure(commandSender->SendGroupCommandRequest(chip::SessionHandle(session)));
         commandSender.release();
 
         return CHIP_NO_ERROR;

diff --git a/examples/chip-tool/commands/clusters/WriteAttributeCommand.h b/examples/chip-tool/commands/clusters/WriteAttributeCommand.h
@@ -128,15 +128,8 @@ class WriteAttribute : public ModelCommand, public chip::app::WriteClient::Callb
         VerifyOrReturnError(writeClient != nullptr, CHIP_ERROR_NO_MEMORY);
         ReturnErrorOnFailure(writeClient->EncodeAttribute(attributePathParams, value, mDataVersion));
 
-        chip::Optional<chip::SessionHandle> session =
-            exchangeManager->GetSessionManager()->CreateGroupSession(groupId, fabricIndex, senderNodeId);
-        if (!session.HasValue())
-        {
-            return CHIP_ERROR_NO_MEMORY;
-        }
-        CHIP_ERROR err = writeClient->SendWriteRequest(session.Value());
-        exchangeManager->GetSessionManager()->RemoveGroupSession(session.Value()->AsGroupSession());
-        ReturnErrorOnFailure(err);
+        chip::Transport::OutgoingGroupSession session(groupId, fabricIndex, senderNodeId);
+        ReturnErrorOnFailure(writeClient->SendWriteRequest(chip::SessionHandle(session)));
         writeClient.release();
 
         return CHIP_NO_ERROR;

diff --git a/examples/chip-tool/commands/common/CommandInvoker.h b/examples/chip-tool/commands/common/CommandInvoker.h
@@ -117,19 +117,14 @@ class CommandInvoker final : public ResponseReceiver<typename RequestType::Respo
 
         ReturnErrorOnFailure(commandSender->AddRequestData(commandPath, aRequestData));
 
-        Optional<SessionHandle> session = exchangeManager->GetSessionManager()->CreateGroupSession(groupId, fabric, sourceNodeId);
-        if (!session.HasValue())
-        {
-            return CHIP_ERROR_NO_MEMORY;
-        }
+        Transport::OutgoingGroupSession session(groupId, fabric, sourceNodeId);
 
         // this (invoker) and commandSender will be deleted by the onDone call before the return of SendGroupCommandRequest
         // this (invoker) should not be used after the SendGroupCommandRequest call
-        ReturnErrorOnFailure(commandSender->SendGroupCommandRequest(session.Value()));
+        ReturnErrorOnFailure(commandSender->SendGroupCommandRequest(SessionHandle(session)));
 
         // this (invoker) and commandSender are already deleted and are not to be used
         commandSender.release();
-        exchangeManager->GetSessionManager()->RemoveGroupSession(session.Value()->AsGroupSession());
 
         return CHIP_NO_ERROR;
     }

diff --git a/examples/shell/shell_common/cmd_server.cpp b/examples/shell/shell_common/cmd_server.cpp
@@ -128,9 +128,15 @@ static bool PrintServerSession(void * context, SessionHandle & session)
         break;
     }
 
-    case Session::SessionType::kGroup: {
-        GroupSession * groupSession = session->AsGroupSession();
-        streamer_printf(streamer_get(), "session type=GROUP id=0x%04x fabricIdx=%d\r\n", groupSession->GetGroupId(),
+    case Session::SessionType::kGroupIncoming: {
+        IncomingGroupSession * groupSession = session->AsIncomingGroupSession();
+        streamer_printf(streamer_get(), "session type=GROUP INCOMING id=0x%04x fabricIdx=%d\r\n", groupSession->GetGroupId(),
+                        groupSession->GetFabricIndex());
+        break;
+    }
+    case Session::SessionType::kGroupOutgoing: {
+        OutgoingGroupSession * groupSession = session->AsOutgoingGroupSession();
+        streamer_printf(streamer_get(), "session type=GROUP OUTGOING id=0x%04x fabricIdx=%d\r\n", groupSession->GetGroupId(),
                         groupSession->GetFabricIndex());
         break;
     }

diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp
@@ -344,7 +344,7 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
     err = commandPath.GetCommandId(&commandId);
     SuccessOrExit(err);
 
-    groupId = mpExchangeCtx->GetSessionHandle()->AsGroupSession()->GetGroupId();
+    groupId = mpExchangeCtx->GetSessionHandle()->AsIncomingGroupSession()->GetGroupId();
     fabric  = GetAccessingFabricIndex();
 
     ChipLogDetail(DataManagement,

diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
@@ -295,7 +295,8 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut
     CHIP_ERROR err = CHIP_NO_ERROR;
 
     ReturnErrorCodeIf(mpExchangeCtx == nullptr, CHIP_ERROR_INTERNAL);
-    const Access::SubjectDescriptor subjectDescriptor = mpExchangeCtx->GetSessionHandle()->AsGroupSession()->GetSubjectDescriptor();
+    const Access::SubjectDescriptor subjectDescriptor =
+        mpExchangeCtx->GetSessionHandle()->AsIncomingGroupSession()->GetSubjectDescriptor();
 
     while (CHIP_NO_ERROR == (err = aAttributeDataIBsReader.Next()))
     {
@@ -329,7 +330,7 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut
         err = attributePath.GetListIndex(dataAttributePath);
         SuccessOrExit(err);
 
-        groupId = mpExchangeCtx->GetSessionHandle()->AsGroupSession()->GetGroupId();
+        groupId = mpExchangeCtx->GetSessionHandle()->AsIncomingGroupSession()->GetGroupId();
         fabric  = GetAccessingFabricIndex();
 
         err = element.GetData(&dataReader);

diff --git a/src/app/clusters/on-off-server/on-off-server.cpp b/src/app/clusters/on-off-server/on-off-server.cpp
@@ -316,7 +316,7 @@ bool OnOffServer::offWithEffectCommand(app::CommandHandler * commandObj, const a
             GroupId groupId = ZCL_SCENES_GLOBAL_SCENE_GROUP_ID;
             if (commandObj->GetExchangeContext()->IsGroupExchangeContext())
             {
-                groupId = commandObj->GetExchangeContext()->GetSessionHandle()->AsGroupSession()->GetGroupId();
+                groupId = commandObj->GetExchangeContext()->GetSessionHandle()->AsIncomingGroupSession()->GetGroupId();
             }
 
             emberAfScenesClusterStoreCurrentSceneCallback(fabric, endpoint, groupId, ZCL_SCENES_GLOBAL_SCENE_SCENE_ID);
@@ -383,7 +383,7 @@ bool OnOffServer::OnWithRecallGlobalSceneCommand(app::CommandHandler * commandOb
     GroupId groupId = ZCL_SCENES_GLOBAL_SCENE_GROUP_ID;
     if (commandObj->GetExchangeContext()->IsGroupExchangeContext())
     {
-        groupId = commandObj->GetExchangeContext()->GetSessionHandle()->AsGroupSession()->GetGroupId();
+        groupId = commandObj->GetExchangeContext()->GetSessionHandle()->AsIncomingGroupSession()->GetGroupId();
     }
 
     emberAfScenesClusterRecallSavedSceneCallback(fabric, endpoint, groupId, ZCL_SCENES_GLOBAL_SCENE_SCENE_ID);

diff --git a/src/controller/CHIPCluster.cpp b/src/controller/CHIPCluster.cpp
@@ -45,34 +45,10 @@ CHIP_ERROR ClusterBase::Associate(DeviceProxy * device, EndpointId endpoint)
 CHIP_ERROR ClusterBase::AssociateWithGroup(DeviceProxy * device, GroupId groupId)
 {
     // TODO Update this function to work in all possible conditions Issue #11850
-
-    CHIP_ERROR err = CHIP_NO_ERROR;
-
-    mDevice = device;
-    if (mDevice->GetSecureSession().HasValue())
-    {
-        // Local copy to preserve original SessionHandle for future Unicast communication.
-        Optional<SessionHandle> session = mDevice->GetExchangeManager()->GetSessionManager()->CreateGroupSession(
-            groupId, mDevice->GetSecureSession().Value()->GetFabricIndex(), mDevice->GetDeviceId());
-
-        // Sanity check
-        if (!session.HasValue() || !session.Value()->IsGroupSession())
-        {
-            err = CHIP_ERROR_INCORRECT_STATE;
-        }
-
-        mGroupSession.Grab(session.Value());
-    }
-    else
-    {
-        // something fishy is going on
-        err = CHIP_ERROR_INCORRECT_STATE;
-    }
-
-    // Set to 0 for now.
-    mEndpoint = 0;
-
-    return err;
+    mDevice   = device;
+    mEndpoint = 0; // Set to 0 for now.
+    mGroupId.SetValue(groupId);
+    return CHIP_NO_ERROR;
 }
 
 void ClusterBase::Dissociate()

diff --git a/src/controller/CHIPCluster.h b/src/controller/CHIPCluster.h
@@ -118,7 +118,6 @@ class DLL_EXPORT ClusterBase
                               const Optional<uint16_t> & aTimedWriteTimeoutMs, WriteResponseDoneCallback doneCb = nullptr,
                               const Optional<DataVersion> & aDataVersion = NullOptional)
     {
-        CHIP_ERROR err = CHIP_NO_ERROR;
         VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
         auto onSuccessCb = [context, successCb](const app::ConcreteAttributePath & aPath) {
@@ -142,21 +141,21 @@ class DLL_EXPORT ClusterBase
             }
         };
 
-        if (mGroupSession)
+        if (mGroupId.HasValue())
         {
-            err =
-                chip::Controller::WriteAttribute<AttrType>(mGroupSession.Get(), mEndpoint, clusterId, attributeId, requestData,
-                                                           onSuccessCb, onFailureCb, aTimedWriteTimeoutMs, onDoneCb, aDataVersion);
-            mDevice->GetExchangeManager()->GetSessionManager()->RemoveGroupSession(mGroupSession->AsGroupSession());
+            VerifyOrReturnError(mDevice->GetSecureSession().HasValue(), CHIP_ERROR_INCORRECT_STATE);
+            Transport::OutgoingGroupSession groupSession(mGroupId.Value(), mDevice->GetSecureSession().Value()->GetFabricIndex(),
+                                                         mDevice->GetDeviceId());
+            return chip::Controller::WriteAttribute<AttrType>(SessionHandle(groupSession), mEndpoint, clusterId, attributeId,
+                                                              requestData, onSuccessCb, onFailureCb, aTimedWriteTimeoutMs, onDoneCb,
+                                                              aDataVersion);
         }
         else
         {
-            err = chip::Controller::WriteAttribute<AttrType>(mDevice->GetSecureSession().Value(), mEndpoint, clusterId, attributeId,
-                                                             requestData, onSuccessCb, onFailureCb, aTimedWriteTimeoutMs, onDoneCb,
-                                                             aDataVersion);
+            return chip::Controller::WriteAttribute<AttrType>(mDevice->GetSecureSession().Value(), mEndpoint, clusterId,
+                                                              attributeId, requestData, onSuccessCb, onFailureCb,
+                                                              aTimedWriteTimeoutMs, onDoneCb, aDataVersion);
         }
-
-        return err;
     }
 
     template <typename AttributeInfo>
@@ -346,7 +345,7 @@ class DLL_EXPORT ClusterBase
     const ClusterId mClusterId;
     DeviceProxy * mDevice;
     EndpointId mEndpoint;
-    SessionHolder mGroupSession;
+    Optional<GroupId> mGroupId;
     Optional<System::Clock::Timeout> mTimeout;
 };
 

diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h
@@ -1204,17 +1204,6 @@
 #define CHIP_CONFIG_UNAUTHENTICATED_CONNECTION_POOL_SIZE 4
 #endif // CHIP_CONFIG_UNAUTHENTICATED_CONNECTION_POOL_SIZE
 
-/**
- * @def CHIP_CONFIG_GROUP_CONNECTION_POOL_SIZE
- *
- * @brief Define the size of the pool used for tracking CHIP groups.
- *        Given the ephemeral nature of groups session, no need to support
- *        a large pool size.
- */
-#ifndef CHIP_CONFIG_GROUP_CONNECTION_POOL_SIZE
-#define CHIP_CONFIG_GROUP_CONNECTION_POOL_SIZE 4
-#endif // CHIP_CONFIG_GROUP_CONNECTION_POOL_SIZE
-
 /**
  * @def CHIP_CONFIG_PEER_CONNECTION_POOL_SIZE
  *

diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h
@@ -79,10 +79,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
 
     bool IsEncryptionRequired() const { return mDispatch.IsEncryptionRequired(); }
 
-    bool IsGroupExchangeContext() const
-    {
-        return (mSession && mSession->GetSessionType() == Transport::Session::SessionType::kGroup);
-    }
+    bool IsGroupExchangeContext() const { return mSession && mSession->IsGroupSession(); }
 
     // Implement SessionReleaseDelegate
     void OnSessionReleased() override;

diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp
@@ -83,7 +83,7 @@ CHIP_ERROR MessagingContext::CreateSessionAliceToBob()
 
 CHIP_ERROR MessagingContext::CreateSessionBobToFriends()
 {
-    mSessionBobToFriends.Grab(mSessionManager.CreateGroupSession(GetFriendsGroupId(), mSrcFabricIndex, GetBobNodeId()).Value());
+    mSessionBobToFriends.Emplace(GetFriendsGroupId(), mSrcFabricIndex, GetBobNodeId());
     return CHIP_NO_ERROR;
 }
 
@@ -99,7 +99,7 @@ SessionHandle MessagingContext::GetSessionAliceToBob()
 
 SessionHandle MessagingContext::GetSessionBobToFriends()
 {
-    return mSessionBobToFriends.Get();
+    return SessionHandle(mSessionBobToFriends.Value());
 }
 
 void MessagingContext::ExpireSessionBobToAlice()
@@ -114,7 +114,7 @@ void MessagingContext::ExpireSessionAliceToBob()
 
 void MessagingContext::ExpireSessionBobToFriends()
 {
-    mSessionManager.RemoveGroupSession(mSessionBobToFriends.Get()->AsGroupSession());
+    mSessionBobToFriends.ClearValue();
 }
 
 Messaging::ExchangeContext * MessagingContext::NewUnauthenticatedExchangeToAlice(Messaging::ExchangeDelegate * delegate)

diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h
@@ -160,7 +160,7 @@ class MessagingContext : public PlatformMemoryUser
     SecurePairingUsingTestSecret mPairingBobToAlice;
     SessionHolder mSessionAliceToBob;
     SessionHolder mSessionBobToAlice;
-    SessionHolder mSessionBobToFriends;
+    Optional<Transport::OutgoingGroupSession> mSessionBobToFriends;
     FabricIndex mSrcFabricIndex  = 1;
     FabricIndex mDestFabricIndex = 1;
 };

diff --git a/src/protocols/secure_channel/MessageCounterManager.cpp b/src/protocols/secure_channel/MessageCounterManager.cpp
@@ -224,8 +224,7 @@ CHIP_ERROR MessageCounterManager::SendMsgCounterSyncResp(Messaging::ExchangeCont
     System::PacketBufferHandle msgBuf;
     VerifyOrDie(exchangeContext->HasSessionHandle());
 
-    VerifyOrReturnError(exchangeContext->GetSessionHandle()->GetSessionType() == Transport::Session::SessionType::kGroup,
-                        CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(exchangeContext->GetSessionHandle()->IsGroupSession(), CHIP_ERROR_INVALID_ARGUMENT);
 
     // NOTE: not currently implemented. When implementing, the following should be done:
     //    - allocate a new buffer: MessagePacketBuffer::New