Skip to content

Commit

Permalink
[opcreds] Fix LEAVE event on RemoveFabric (#18434)
Browse files Browse the repository at this point in the history
* [opcreds] Fix LEAVE event on RemoveFabric

LEAVE event is not emitted on RemoveFabric or factory reset.
There are two reasons for that:
- Removing a fabric causes closing all active ReadHandlers
  for a given fabric, so we must flush the event log before
  that happens.
- There are two listeners for removing a fabric; the one
  that removes ACLs for a given fabric index is executed
  prior to the other one that generates LEAVE event. That
  causes "Accces control: denied" error.

Fix both issues.

* Add comment
  • Loading branch information
Damian-Nordic authored and pull[bot] committed Jul 22, 2023
1 parent c0ec70e commit 1121912
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -364,8 +364,7 @@ class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate
static_cast<unsigned>(fabricIndex));
fabricListChanged();

// The Leave event SHOULD be emitted by a Node prior to permanently
// leaving the Fabric.
// The Leave event SHOULD be emitted by a Node prior to permanently leaving the Fabric.
for (auto endpoint : EnabledEndpointsWithServerCluster(Basic::Id))
{
// If Basic cluster is implemented on this endpoint
Expand All @@ -377,6 +376,26 @@ class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate
ChipLogError(Zcl, "OpCredsFabricTableDelegate: Failed to record Leave event");
}
}

// Try to send the queued events as soon as possible. If the just emitted leave event won't
// be sent this time, it will likely not be delivered at all for the following reasons:
// - removing the fabric expires all associated ReadHandlers, so all subscriptions to
// the leave event will be cancelled.
// - removing the fabric removes all associated access control entries, so generating
// subsequent reports containing the leave event will fail the access control check.
InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleUrgentEventDeliverySync();
EventManagement::GetInstance().FabricRemoved(fabricIndex);

// Remove access control entries in reverse order (it could be any order, but reverse order
// will cause less churn in persistent storage).
size_t aclCount = 0;
if (Access::GetAccessControl().GetEntryCount(fabricIndex, aclCount) == CHIP_NO_ERROR)
{
while (aclCount)
{
(void) Access::GetAccessControl().DeleteEntry(nullptr, fabricIndex, --aclCount);
}
}
}

// Gets called when a fabric is loaded into the FabricTable from storage
Expand Down
15 changes: 0 additions & 15 deletions src/app/server/Server.h
Original file line number Diff line number Diff line change
Expand Up @@ -333,21 +333,6 @@ class Server
static_cast<unsigned>(fabricIndex), err.Format());
}
}

{
// Remove access control entries in reverse order. (It could be
// any order, but reverse order will cause less churn in
// persistent storage.)
size_t count = 0;
if (Access::GetAccessControl().GetEntryCount(fabricIndex, count) == CHIP_NO_ERROR)
{
while (count)
{
(void) Access::GetAccessControl().DeleteEntry(nullptr, fabricIndex, --count);
}
}
}
app::EventManagement::GetInstance().FabricRemoved(fabricIndex);
};

void OnFabricRetrievedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
Expand Down

0 comments on commit 1121912

Please sign in to comment.