RemoveFabric command crashes server when removing accessing fabric

[bzbarsky-apple]

Problem

After #16098 we crash if we RemoveFabric and pass it the fabric index of the fabric the command is running on.

This happens because of this code in SessionManager::PrepareMessage:

        if (session->GetSecureSessionType() == SecureSession::Type::kCASE)
        {
            FabricInfo * fabric = mFabricTable->FindFabricWithIndex(session->GetFabricIndex());
            VerifyOrDie(fabric != nullptr);
            CryptoContext::BuildNonce(nonce, packetHeader.GetSecurityFlags(), messageCounter, fabric->GetNodeId());

when sending the response message for the RemoveFabric command. That VerifyOrDie dies, because fabric is in fact null by this point.

Proposed Solution

We need to either fix this code to get the node id from somewhere else (e.g. store it in the session instead of getting it from the fabric table every time) or change how exactly RemoveFabric works so that it manages to send the message before doing the actual fabric removal.... but then we may have situations where we claim to have removed the fabric when we actually have not.

@kghost @tcarmelveilleux @turon

[turon]

I'd lean toward storing the node id redundantly in the session context. That way we can support this "lingering session" use cases elegantly and decouple them from configuration updates that are occurring over the session.

[Damian-Nordic]

Note that the same assertion fails when the controller is subscribed to Leave/ShutDown events and the factory reset is triggered.

[kghost]

Discussed with Boris, we will achieve RemoveFabric in following actions:

1. Call OnFabricDeletedFromStorage
2. Send RemoveFabric response
3. Remove session associated with the fabric
4. Remove exchanges associated with the fabric
5. Call exchange->OnExchangeReleased to all exchange delegates
6. Remove the fabric object

Step 3-6 will be deferred to the next schedule

[mlepage-google]

I can verify I hit this too while testing something. :-)

[Damian-Nordic]

@kghost I agree with your solution, but I think you will need to make sure that the LEAVE event is generated prior to removing ACLs for the removed fabric. Otherwise, generating the report containing the LEAVE event will fail the permission check, right?

[bzbarsky-apple]

Fixed in #17815.