From 0b428d5aa7858319e1bdf5c76ee181e4d7ca081c Mon Sep 17 00:00:00 2001 From: Philip Gregor Date: Thu, 16 May 2024 16:58:10 -0700 Subject: [PATCH] Addressed comments by andy31415 and tcarmelveilleux --- .../main/jni/cpp/core/MatterCastingPlayer-JNI.cpp | 3 +++ .../tv-casting-app/linux/simple-app-helper.cpp | 15 +++++++++------ .../tv-casting-common/core/CastingApp.cpp | 4 +--- .../tv-casting-common/core/CastingPlayer.cpp | 9 +++++++-- .../tv-casting-common/core/CastingPlayer.h | 3 ++- .../core/CommissionerDeclarationHandler.cpp | 6 +++--- .../core/CommissionerDeclarationHandler.h | 6 +++--- .../tv-casting-common/core/ConnectionCallbacks.h | 14 +++++++------- 8 files changed, 35 insertions(+), 25 deletions(-) diff --git a/examples/tv-casting-app/android/App/app/src/main/jni/cpp/core/MatterCastingPlayer-JNI.cpp b/examples/tv-casting-app/android/App/app/src/main/jni/cpp/core/MatterCastingPlayer-JNI.cpp index a8467effc6684b..d0409c6a27945c 100644 --- a/examples/tv-casting-app/android/App/app/src/main/jni/cpp/core/MatterCastingPlayer-JNI.cpp +++ b/examples/tv-casting-app/android/App/app/src/main/jni/cpp/core/MatterCastingPlayer-JNI.cpp @@ -101,6 +101,8 @@ JNI_METHOD(jobject, verifyOrEstablishConnection) "MatterCastingPlayer-JNI::verifyOrEstablishConnection() ConnectCallback() Connected to Casting Player " "with device ID: %s", playerPtr->GetId()); + // The Java jSuccessCallback is expecting a Void v callback parameter which translates to a nullptr. When calling the + // Java method from C++ via JNI, passing nullptr is equivalent to passing a Void object in Java. MatterCastingPlayerJNIMgr().mConnectionSuccessHandler.Handle(nullptr); } else @@ -118,6 +120,7 @@ JNI_METHOD(jobject, verifyOrEstablishConnection) matter::casting::core::ConnectionCallbacks connectionCallbacks; connectionCallbacks.mOnConnectionComplete = connectCallback; + // TODO: Verify why commissioningWindowTimeoutSec is a "unsigned long long int" type. Seems too big. castingPlayer->VerifyOrEstablishConnection(connectionCallbacks, static_cast(commissioningWindowTimeoutSec), idOptions); return support::convertMatterErrorFromCppToJava(CHIP_NO_ERROR); diff --git a/examples/tv-casting-app/linux/simple-app-helper.cpp b/examples/tv-casting-app/linux/simple-app-helper.cpp index 866c77231c30a7..4d67b7936c5974 100644 --- a/examples/tv-casting-app/linux/simple-app-helper.cpp +++ b/examples/tv-casting-app/linux/simple-app-helper.cpp @@ -35,7 +35,7 @@ const uint16_t kDesiredEndpointVendorId = 65521; DiscoveryDelegateImpl * DiscoveryDelegateImpl::_discoveryDelegateImpl = nullptr; -bool awaitingCommissionerPasscodeInput = false; +bool gAwaitingCommissionerPasscodeInput = false; std::shared_ptr targetCastingPlayer; DiscoveryDelegateImpl * DiscoveryDelegateImpl::GetInstance() @@ -206,13 +206,16 @@ CHIP_ERROR InitCommissionableDataProvider(LinuxCommissionableDataProvider & prov // properly to the commissioner later, PASE will succeed. } - // Default to minimum PBKDF iterations + // Default to the minimum PBKDF iterations (1000) for this example implementation. For TV devices and TV casting app production + // implementations, you should use a higher number of PBKDF iterations to enhance security. The default minimum iterations are + // not sufficient against brute-force and rainbow table attacks. uint32_t spake2pIterationCount = chip::Crypto::kSpake2p_Min_PBKDF_Iterations; if (options.spake2pIterations != 0) { spake2pIterationCount = options.spake2pIterations; } - ChipLogError(Support, "PASE PBKDF iterations set to %u", static_cast(spake2pIterationCount)); + ChipLogError(Support, "PASE PBKDF iterations set to %u. Should be set higher for a production app implementation.", + static_cast(spake2pIterationCount)); return provider.Init(options.spake2pVerifier, options.spake2pSalt, spake2pIterationCount, setupPasscode, options.payload.discriminator.GetLongValue()); @@ -278,7 +281,7 @@ void CommissionerDeclarationCallback(const chip::Transport::PeerAddress & source ChipLogProgress(AppServer, "Input 1245678 to use the defualt passcode."); ChipLogProgress(AppServer, "Example: cast setcommissionerpasscode 12345678"); ChipLogProgress(AppServer, "---- Awaiting user input ----"); - awaitingCommissionerPasscodeInput = true; + gAwaitingCommissionerPasscodeInput = true; } } @@ -375,10 +378,10 @@ CHIP_ERROR CommandHandler(int argc, char ** argv) } char * eptr; uint32_t passcode = (uint32_t) strtol(argv[1], &eptr, 10); - if (awaitingCommissionerPasscodeInput) + if (gAwaitingCommissionerPasscodeInput) { ChipLogProgress(AppServer, "CommandHandler() setcommissionerpasscode user enterd passcode: %d", passcode); - awaitingCommissionerPasscodeInput = false; + gAwaitingCommissionerPasscodeInput = false; // Per connectedhomeip/examples/platform/linux/LinuxCommissionableDataProvider.h: We don't support overriding the // passcode post-init (it is deprecated!). Therefore we need to initiate a new provider with the user entered diff --git a/examples/tv-casting-app/tv-casting-common/core/CastingApp.cpp b/examples/tv-casting-app/tv-casting-common/core/CastingApp.cpp index 347f181c297fd0..216d06b062b50e 100644 --- a/examples/tv-casting-app/tv-casting-common/core/CastingApp.cpp +++ b/examples/tv-casting-app/tv-casting-common/core/CastingApp.cpp @@ -90,9 +90,7 @@ CHIP_ERROR CastingApp::UpdateCommissionableDataProvider(chip::DeviceLayer::Commi { ChipLogProgress(Discovery, "CastingApp::UpdateCommissionableDataProvider()"); chip::DeviceLayer::SetCommissionableDataProvider(commissionableDataProvider); - CHIP_ERROR err = CHIP_NO_ERROR; - err = mAppParameters->SetCommissionableDataProvider(commissionableDataProvider); - return err; + return mAppParameters->SetCommissionableDataProvider(commissionableDataProvider); } void ReconnectHandler(CHIP_ERROR err, matter::casting::core::CastingPlayer * castingPlayer) diff --git a/examples/tv-casting-app/tv-casting-common/core/CastingPlayer.cpp b/examples/tv-casting-app/tv-casting-common/core/CastingPlayer.cpp index 8745139709efb9..b3075be95304d8 100644 --- a/examples/tv-casting-app/tv-casting-common/core/CastingPlayer.cpp +++ b/examples/tv-casting-app/tv-casting-common/core/CastingPlayer.cpp @@ -29,6 +29,7 @@ namespace core { CastingPlayer * CastingPlayer::mTargetCastingPlayer = nullptr; +// TODO: Verify why commissioningWindowTimeoutSec is a "unsigned long long int" type. Seems too big. void CastingPlayer::VerifyOrEstablishConnection(ConnectionCallbacks connectionCallbacks, unsigned long long int commissioningWindowTimeoutSec, IdentificationDeclarationOptions idOptions) @@ -225,12 +226,16 @@ void CastingPlayer::ContinueConnecting(ConnectionCallbacks connectionCallbacks, void CastingPlayer::resetState(CHIP_ERROR err) { + ChipLogProgress(AppServer, "CastingPlayer::resetState()"); support::ChipDeviceEventHandler::SetUdcStatus(false); mConnectionState = CASTING_PLAYER_NOT_CONNECTED; mCommissioningWindowTimeoutSec = kCommissioningWindowTimeoutSec; mTargetCastingPlayer = nullptr; - mOnCompleted(err, nullptr); - mOnCompleted = nullptr; + if (mOnCompleted) + { + mOnCompleted(err, nullptr); + mOnCompleted = nullptr; + } } void CastingPlayer::Disconnect() diff --git a/examples/tv-casting-app/tv-casting-common/core/CastingPlayer.h b/examples/tv-casting-app/tv-casting-common/core/CastingPlayer.h index 7e2c55fc269db1..14a55e045fcb83 100644 --- a/examples/tv-casting-app/tv-casting-common/core/CastingPlayer.h +++ b/examples/tv-casting-app/tv-casting-common/core/CastingPlayer.h @@ -224,7 +224,8 @@ class CastingPlayer : public std::enable_shared_from_this ConnectCallback mOnCompleted = {}; /** - * @brief resets this CastingPlayer's state and calls mOnCompleted with the CHIP_ERROR. + * @brief resets this CastingPlayer's state and calls mOnCompleted with the CHIP_ERROR. Also, after calling mOnCompleted, it + * clears mOnCompleted by setting it to a nullptr. */ void resetState(CHIP_ERROR err); diff --git a/examples/tv-casting-app/tv-casting-common/core/CommissionerDeclarationHandler.cpp b/examples/tv-casting-app/tv-casting-common/core/CommissionerDeclarationHandler.cpp index 18e9d39a827508..bb9ecc3c4c57d9 100644 --- a/examples/tv-casting-app/tv-casting-common/core/CommissionerDeclarationHandler.cpp +++ b/examples/tv-casting-app/tv-casting-common/core/CommissionerDeclarationHandler.cpp @@ -68,6 +68,6 @@ void CommissionerDeclarationHandler::SetCommissionerDeclarationCallback( mCmmissionerDeclarationCallback_ = std::move(callback); } -}; // namespace core -}; // namespace casting -}; // namespace matter +} // namespace core +} // namespace casting +} // namespace matter diff --git a/examples/tv-casting-app/tv-casting-common/core/CommissionerDeclarationHandler.h b/examples/tv-casting-app/tv-casting-common/core/CommissionerDeclarationHandler.h index 923045a4e490d7..ac4189267193c1 100644 --- a/examples/tv-casting-app/tv-casting-common/core/CommissionerDeclarationHandler.h +++ b/examples/tv-casting-app/tv-casting-common/core/CommissionerDeclarationHandler.h @@ -64,6 +64,6 @@ class CommissionerDeclarationHandler : public chip::Protocols::UserDirectedCommi ~CommissionerDeclarationHandler() {} }; -}; // namespace core -}; // namespace casting -}; // namespace matter +} // namespace core +} // namespace casting +} // namespace matter diff --git a/examples/tv-casting-app/tv-casting-common/core/ConnectionCallbacks.h b/examples/tv-casting-app/tv-casting-common/core/ConnectionCallbacks.h index 9c5095aa851380..8e0ce170171ba8 100644 --- a/examples/tv-casting-app/tv-casting-common/core/ConnectionCallbacks.h +++ b/examples/tv-casting-app/tv-casting-common/core/ConnectionCallbacks.h @@ -38,13 +38,13 @@ using ConnectCallback = std::function; +using CommissionerDeclarationCallback = std::function; /** - * @brief A container class for User Directed Commissioning (UDC) callbacks. + * @brief A container struct for User Directed Commissioning (UDC) callbacks. */ -class ConnectionCallbacks +struct ConnectionCallbacks { public: /** @@ -57,6 +57,6 @@ class ConnectionCallbacks CommissionerDeclarationCallback mCommissionerDeclarationCallback = nullptr; }; -}; // namespace core -}; // namespace casting -}; // namespace matter +} // namespace core +} // namespace casting +} // namespace matter