HandleLogin in ContentAppPlatform::ManageClientAccess

mthiesc · mthiesc · commit 2b99f246e069 · 2024-07-04T02:04:29.000+02:00
diff --git a/examples/tv-app/android/include/account-login/AccountLoginManager.cpp b/examples/tv-app/android/include/account-login/AccountLoginManager.cpp
@@ -58,7 +58,7 @@ AccountLoginManager::AccountLoginManager(ContentAppCommandDelegate * commandDele
     CopyString(mSetupPin, sizeof(mSetupPin), setupPin);
 }
 
-bool AccountLoginManager::HandleLogin(const CharSpan & tempAccountIdentifier, const CharSpan & setupPin,
+bool AccountLoginManager::HandleLogin(const CharSpan & tempAccountId, const CharSpan & setupPIN,
                                       const chip::Optional<chip::NodeId> & nodeId)
 {
     ChipLogProgress(DeviceLayer, "AccountLoginManager::HandleLogin called for endpoint %d", mEndpointId);
@@ -76,7 +76,7 @@ bool AccountLoginManager::HandleLogin(const CharSpan & tempAccountIdentifier, co
 
     Json::Value response;
     bool commandHandled = true;
-    AccountLogin::Commands::Login::Type cmd = { tempAccountIdentifier, setupPin, nodeId };
+    AccountLogin::Commands::Login::Type cmd = { tempAccountId, setupPIN, nodeId };
 
     // Deliberately ignore returned status
     mCommandDelegate->InvokeCommand(mEndpointId, AccountLogin::Id,
diff --git a/examples/tv-app/android/java/TVApp-JNI.cpp b/examples/tv-app/android/java/TVApp-JNI.cpp
@@ -252,13 +252,13 @@ SampleTvAppInstallationService gSampleTvAppInstallationService;
 
 class MyPostCommissioningListener : public PostCommissioningListener
 {
-    void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, Messaging::ExchangeManager & exchangeMgr,
-                                const SessionHandle & sessionHandle) override
+    void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, std::string rotatingId, uint32_t passcode,
+                                Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle) override
     {
         // read current binding list
         chip::Controller::ClusterBase cluster(exchangeMgr, sessionHandle, kTargetBindingClusterEndpointId);
 
-        cacheContext(vendorId, productId, nodeId, exchangeMgr, sessionHandle);
+        cacheContext(vendorId, productId, nodeId, std::move(rotatingId), passcode, exchangeMgr, sessionHandle);
 
         CHIP_ERROR err =
             cluster.ReadAttribute<Binding::Attributes::Binding::TypeInfo>(this, OnReadSuccessResponse, OnReadFailureResponse);
@@ -342,17 +342,19 @@ class MyPostCommissioningListener : public PostCommissioningListener
 
         Optional<SessionHandle> opt   = mSecureSession.Get();
         SessionHandle & sessionHandle = opt.Value();
-        ContentAppPlatform::GetInstance().ManageClientAccess(*mExchangeMgr, sessionHandle, mVendorId, mProductId, localNodeId,
+        ContentAppPlatform::GetInstance().ManageClientAccess(*mExchangeMgr, sessionHandle, mVendorId, mProductId, localNodeId, mRotatingId, mPasscode,
                                                              bindings, OnSuccessResponse, OnFailureResponse);
         clearContext();
     }
 
-    void cacheContext(uint16_t vendorId, uint16_t productId, NodeId nodeId, Messaging::ExchangeManager & exchangeMgr,
+    void cacheContext(uint16_t vendorId, uint16_t productId, NodeId nodeId, std::string rotatingId, uint32_t passcode, Messaging::ExchangeManager & exchangeMgr,
                       const SessionHandle & sessionHandle)
     {
         mVendorId    = vendorId;
         mProductId   = productId;
         mNodeId      = nodeId;
+        mRotatingId  = std::move(rotatingId);
+        mPasscode    = passcode;
         mExchangeMgr = &exchangeMgr;
         mSecureSession.ShiftToSession(sessionHandle);
     }
@@ -368,6 +370,8 @@ class MyPostCommissioningListener : public PostCommissioningListener
     uint16_t mVendorId                        = 0;
     uint16_t mProductId                       = 0;
     NodeId mNodeId                            = 0;
+    std::string mRotatingId;
+    uint32_t mPasscode                         = 0;
     Messaging::ExchangeManager * mExchangeMgr = nullptr;
     SessionHolder mSecureSession;
 };
diff --git a/examples/tv-app/tv-common/src/AppTv.cpp b/examples/tv-app/tv-common/src/AppTv.cpp
@@ -158,13 +158,13 @@ MyAppInstallationService gMyAppInstallationService;
 
 class MyPostCommissioningListener : public PostCommissioningListener
 {
-    void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, Messaging::ExchangeManager & exchangeMgr,
-                                const SessionHandle & sessionHandle) override
+    void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, std::string rotatingId, uint32_t passcode,
+                                Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle) override
     {
         // read current binding list
         chip::Controller::ClusterBase cluster(exchangeMgr, sessionHandle, kTargetBindingClusterEndpointId);
 
-        cacheContext(vendorId, productId, nodeId, exchangeMgr, sessionHandle);
+        cacheContext(vendorId, productId, nodeId, std::move(rotatingId), passcode, exchangeMgr, sessionHandle);
 
         CHIP_ERROR err =
             cluster.ReadAttribute<Binding::Attributes::Binding::TypeInfo>(this, OnReadSuccessResponse, OnReadFailureResponse);
@@ -248,17 +248,19 @@ class MyPostCommissioningListener : public PostCommissioningListener
 
         Optional<SessionHandle> opt   = mSecureSession.Get();
         SessionHandle & sessionHandle = opt.Value();
-        ContentAppPlatform::GetInstance().ManageClientAccess(*mExchangeMgr, sessionHandle, mVendorId, mProductId, localNodeId,
+        ContentAppPlatform::GetInstance().ManageClientAccess(*mExchangeMgr, sessionHandle, mVendorId, mProductId, localNodeId, rotatingId, passcode,
                                                              bindings, OnSuccessResponse, OnFailureResponse);
         clearContext();
     }
 
-    void cacheContext(uint16_t vendorId, uint16_t productId, NodeId nodeId, Messaging::ExchangeManager & exchangeMgr,
-                      const SessionHandle & sessionHandle)
+    void cacheContext(uint16_t vendorId, uint16_t productId, NodeId nodeId, std::string rotatingId, uint32_t passcode,
+                      Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle)
     {
         mVendorId    = vendorId;
         mProductId   = productId;
         mNodeId      = nodeId;
+        mRotatingId  = std::move(rotatingId);
+        mPassocde    = passcode;
         mExchangeMgr = &exchangeMgr;
         mSecureSession.ShiftToSession(sessionHandle);
     }
@@ -274,6 +276,8 @@ class MyPostCommissioningListener : public PostCommissioningListener
     uint16_t mVendorId                        = 0;
     uint16_t mProductId                       = 0;
     NodeId mNodeId                            = 0;
+    std::string rotatingId;
+    uint32_t passcode                         = 0;
     Messaging::ExchangeManager * mExchangeMgr = nullptr;
     SessionHolder mSecureSession;
 };
diff --git a/src/app/app-platform/ContentAppPlatform.cpp b/src/app/app-platform/ContentAppPlatform.cpp
@@ -609,6 +609,7 @@ CHIP_ERROR ContentAppPlatform::GetACLEntryIndex(size_t * foundIndex, FabricIndex
 // and create bindings on the given client so that it knows what it has access to.
 CHIP_ERROR ContentAppPlatform::ManageClientAccess(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle,
                                                   uint16_t targetVendorId, uint16_t targetProductId, NodeId localNodeId,
+                                                  const std::string& rotatingId, uint32_t passcode,
                                                   std::vector<Binding::Structs::TargetStruct::Type> bindings,
                                                   Controller::WriteResponseSuccessCallback successCb,
                                                   Controller::WriteResponseFailureCallback failureCb)
@@ -755,6 +756,19 @@ CHIP_ERROR ContentAppPlatform::ManageClientAccess(Messaging::ExchangeManager & e
             {
                 // notify content app about this nodeId
                 app->AddClientNode(subjectNodeId);
+
+                auto tempAccountId = CharSpan::fromCharString(rotatingId.c_str());
+                auto setupPIN = CharSpan::fromCharString(std::to_string(passcode).c_str());
+                auto nodeId = MakeOptional(subjectNodeId); // Q: Is subjectNodeId correct NodeId to use?
+
+                // send login command to content app
+                if (!tempAccountId.empty() && !setupPIN.empty()) {
+                    // Q: Should we only do this if (commissioner) passcode is not 0, e.g when user was shown PIN prompt?
+                    auto status = app->GetAccountLoginDelegate()->HandleLogin(tempAccountId, setupPIN, nodeId);
+                    ChipLogProgress(Controller, "AccountLogin::Login command sent and returned with status: %d", status);
+                } else {
+                    ChipLogError(Controller, "Failed to get tempAccountId or setupPIN to set AccountLogin::Login command");
+                }
             }
         }
     }
diff --git a/src/app/app-platform/ContentAppPlatform.h b/src/app/app-platform/ContentAppPlatform.h
@@ -176,6 +176,8 @@ class DLL_EXPORT ContentAppPlatform
      * @param[in] targetVendorId  Vendor ID for the target device.
      * @param[in] targetProductId Product ID for the target device.
      * @param[in] localNodeId     The NodeId for the local device.
+     * @param[in] rotatingId      The rotating account ID to handle account login.
+     * @param[in] passcode        The passcode to handle account login.
      * @param[in] bindings        Any additional bindings to include. This may include current bindings.
      * @param[in] successCb       The function to be called on success of adding the binding.
      * @param[in] failureCb       The function to be called on failure of adding the binding.
@@ -184,6 +186,7 @@ class DLL_EXPORT ContentAppPlatform
      */
     CHIP_ERROR ManageClientAccess(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle, uint16_t targetVendorId,
                                   uint16_t targetProductId, NodeId localNodeId,
+                                  const std::string& rotatingId, uint32_t passcode,
                                   std::vector<app::Clusters::Binding::Structs::TargetStruct::Type> bindings,
                                   Controller::WriteResponseSuccessCallback successCb,
                                   Controller::WriteResponseFailureCallback failureCb);
diff --git a/src/controller/CommissionerDiscoveryController.cpp b/src/controller/CommissionerDiscoveryController.cpp
@@ -32,6 +32,33 @@
 
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY
 
+namespace {
+
+bool fillRotatingIdBuffer(UDCClientState* client, char[] buffer) {
+    auto rotatingIdLength = client->GetRotatingIdLength();
+    auto hexBufferByteCount = 2 * rotatingIdLength;
+    if (sizeof(buffer) < hexBufferByteCount) {
+        return false;
+    }
+    auto err = Encoding::BytesToUppercaseHexBuffer(client->GetRotatingId(), rotatingIdLength, buffer, sizeof(buffer));
+    return err == CHIP_NO_ERROR;
+}
+
+// Returned CharSpan valid lifetime equals buffer lifetime.
+CharSpan getRotatingIdSpan(UDCClientState* client, char[] buffer) {
+    auto ok = fillRotatingIdBuffer(client, buffer);
+    return ok ? { buffer, hexBufferByteCount } : {};
+}
+
+// Allocates memory for rotating ID string and copies to string
+std::string getRotatingIdString(UDCClientState* client) {
+    auto buffer = char[Dnssd::kMaxRotatingIdLen * 2];
+    auto ok = fillRotatingIdBuffer(client, buffer);
+    return ok ? { buffer, hexBufferByteCount } : {};
+}
+
+}
+
 using namespace ::chip;
 using namespace chip::Protocols::UserDirectedCommissioning;
 
@@ -246,15 +273,11 @@ void CommissionerDiscoveryController::InternalOk()
     }
 
     char rotatingIdBuffer[Dnssd::kMaxRotatingIdLen * 2];
-    size_t rotatingIdLength = client->GetRotatingIdLength();
-    CHIP_ERROR err =
-        Encoding::BytesToUppercaseHexBuffer(client->GetRotatingId(), rotatingIdLength, rotatingIdBuffer, sizeof(rotatingIdBuffer));
-    if (err != CHIP_NO_ERROR)
-    {
+    CharSpan rotatingIdSpan = getRotatingIdSpan(client, rotatingIdBuffer);
+    if (rotatingIdSpan.empty()) {
         ChipLogError(AppServer, "UX InternalOk: could not convert rotating id to hex");
         return;
     }
-    CharSpan rotatingIdSpan(rotatingIdBuffer, 2 * rotatingIdLength);
 
     uint8_t targetAppCount = client->GetNumTargetAppInfos();
 
@@ -406,7 +429,6 @@ void CommissionerDiscoveryController::InternalHandleContentAppPasscodeResponse()
     ValidateSession();
     uint32_t passcode = mPasscode;
 
-    // Q: Seems like getting rotating ID is done twice - once here and once in InternalOk. Is this necessary or could it be cached?
     if (mUdcServer == nullptr)
     {
         ChipLogError(AppServer, "UX Ok - HandleContentAppPasscodeResponse: no udc server");
@@ -432,18 +454,11 @@ void CommissionerDiscoveryController::InternalHandleContentAppPasscodeResponse()
         if (passcode == 0 && client->GetCommissionerPasscode() && client->GetCdPort() != 0)
         {
             char rotatingIdBuffer[Dnssd::kMaxRotatingIdLen * 2];
-            size_t rotatingIdLength = client->GetRotatingIdLength();
-            CHIP_ERROR err = Encoding::BytesToUppercaseHexBuffer(client->GetRotatingId(), rotatingIdLength, rotatingIdBuffer,
-                                                                 sizeof(rotatingIdBuffer));
-            if (err != CHIP_NO_ERROR)
-            {
+            CharSpan rotatingIdSpan = getRotatingIdSpan(client, rotatingIdBuffer);
+            if (rotatingIdSpan.empty()) {
                 ChipLogError(AppServer, "UX Ok - HandleContentAppPasscodeResponse: could not convert rotating id to hex");
                 return;
             }
-            CharSpan rotatingIdSpan(rotatingIdBuffer, 2 * rotatingIdLength);
-
-            // Store rotating ID as tempAccountIdentifier to use in AccountLogin::Login payload later.
-            mTempAccountIdentifier = rotatingIdSpan;
 
             // first step of commissioner passcode
             ChipLogError(AppServer, "UX Ok: commissioner passcode, sending CDC");
@@ -460,9 +475,6 @@ void CommissionerDiscoveryController::InternalHandleContentAppPasscodeResponse()
                     cd, Transport::PeerAddress::UDP(client->GetPeerAddress().GetIPAddress(), client->GetCdPort()));
                 return;
             }
-            // Store commissioner passcode as setup PIN (string/charspan).
-            // If this PIN is not empty it signals that user prompt was shown to enter PIN and AccountLogin::Login command has to be sent.
-            mCommissionerSetupPin = CharSpan::fromCharString(std::to_string(passcode));
 
             client->SetCachedCommissionerPasscode(passcode);
             client->SetUDCClientProcessingState(UDCClientProcessingState::kWaitingForCommissionerPasscodeReady);
@@ -616,26 +628,22 @@ void CommissionerDiscoveryController::CommissioningSucceeded(uint16_t vendorId,
     mProductId = productId;
     mNodeId    = nodeId;
 
-    // Send AccountLogin::Login command if user was prompted to enter setup PIN manually.
-    // Q: Is this the correct place to have this logic?
-    // Q: Is there an easier way call AccountLoginDelegate from here?
-    if (!mCommissionerSetupPIN.empty()) {
-        ChipLogProgress(AppServer, "UX ComissioningSucceeded with setupPIN prompt flow");
-        auto app = ContentAppPlatform::GetInstance().LoadContentAppByClient(vendorId, productId);
-        if (app == nullptr) {
-            ChipLogError(AppServer, "UX ComissioningSucceeded with setupPIN prompt flow: Failed to get ContentApp");
-            // Q: Any action to take?
-        } else {
-            auto status = app->GetAccountLoginDelegate()->HandleLogin(mTempAccountIdentifier, mCommissionerSetupPin, {mNodeId});
-            ChipLogProgress(AppServer, "UX ComissioningSucceeded with setupPIN prompt flow: HandleLogin response status: %d", status);
-            // Q: Any action to take here if status is true/false?
-        }
+    auto rotatingId = std::string{};
+    auto passcode = uint32_t{ 0 };
+
+    auto client = GetUDCClientState();
+    if (client != nullptr)
+    {
+        // Get rotating ID and cached (commissioner?) passcode to handle AccountLogin
+        rotatingId = getRotatingIdString(client);
+        // Q: Should we use mPasscode, client->GetCommissionerPasscode, or client->GetCachedCommissionerPasscode?
+        passcode = std::to_string(client->GetCachedCommissionerPasscode());
     }
 
     if (mPostCommissioningListener != nullptr)
     {
         ChipLogDetail(Controller, "CommissionerDiscoveryController calling listener");
-        mPostCommissioningListener->CommissioningCompleted(vendorId, productId, nodeId, exchangeMgr, sessionHandle);
+        mPostCommissioningListener->CommissioningCompleted(vendorId, productId, nodeId, std::move(rotatingId), passcode, exchangeMgr, sessionHandle);
     }
     else
     {
diff --git a/src/controller/CommissionerDiscoveryController.h b/src/controller/CommissionerDiscoveryController.h
@@ -38,7 +38,6 @@
 
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY
 
-using chip::CharSpan;
 using chip::NodeId;
 using chip::OperationalSessionSetup;
 using chip::Protocols::UserDirectedCommissioning::UDCClientState;
@@ -233,11 +232,14 @@ class DLL_EXPORT PostCommissioningListener
      *  @param[in]    vendorId           The vendorid from the DAC of the new node.
      *  @param[in]    productId          The productid from the DAC of the new node.
      *  @param[in]    nodeId             The node id for the newly commissioned node.
+     *  @param[in]    rotatingId         The rotating ID to handle account login.
+     *  @param[in]    passcode           The passcode to handle account login.
      *  @param[in]    exchangeMgr        The exchange manager to be used to get an exchange context.
      *  @param[in]    sessionHandle      A reference to an established session.
      *
      */
     virtual void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId,
+                                        std::string rotatingId, uint32_t passcode,
                                         chip::Messaging::ExchangeManager & exchangeMgr,
                                         const chip::SessionHandle & sessionHandle) = 0;
 
@@ -452,8 +454,6 @@ class CommissionerDiscoveryController : public chip::Protocols::UserDirectedComm
     uint16_t mProductId = 0;
     NodeId mNodeId      = 0;
     uint32_t mPasscode  = 0;
-    CharSpan mTempAccountIdentifier;
-    CharSpan mCommissionerSetupPin;
 
     UserDirectedCommissioningServer * mUdcServer           = nullptr;
     UserPrompter * mUserPrompter                           = nullptr;

Original file line number	Diff line number	Diff line change
`@@ -58,7 +58,7 @@ AccountLoginManager::AccountLoginManager(ContentAppCommandDelegate * commandDele`
`58`	`58`	`CopyString(mSetupPin, sizeof(mSetupPin), setupPin);`
`59`	`59`	`}`
`60`	`60`
`61`		`-bool AccountLoginManager::HandleLogin(const CharSpan & tempAccountIdentifier, const CharSpan & setupPin,`
	`61`	`+bool AccountLoginManager::HandleLogin(const CharSpan & tempAccountId, const CharSpan & setupPIN,`
`62`	`62`	`const chip::Optional<chip::NodeId> & nodeId)`
`63`	`63`	`{`
`64`	`64`	`ChipLogProgress(DeviceLayer, "AccountLoginManager::HandleLogin called for endpoint %d", mEndpointId);`
`@@ -76,7 +76,7 @@ bool AccountLoginManager::HandleLogin(const CharSpan & tempAccountIdentifier, co`
`76`	`76`
`77`	`77`	`Json::Value response;`
`78`	`78`	`bool commandHandled = true;`
`79`		`- AccountLogin::Commands::Login::Type cmd = { tempAccountIdentifier, setupPin, nodeId };`
	`79`	`+ AccountLogin::Commands::Login::Type cmd = { tempAccountId, setupPIN, nodeId };`
`80`	`80`
`81`	`81`	`// Deliberately ignore returned status`
`82`	`82`	`mCommandDelegate->InvokeCommand(mEndpointId, AccountLogin::Id,`