mountetna / monoetna

mono-repository version of etna projects
GNU General Public License v2.0
1 stars 0 forks source link

Magma does not censor updates for unattached records #401

Open graft opened 3 years ago

graft commented 3 years ago

It is possible to make updates to restricted records without 'restricted' privileges.

This issue was discovered in the following case:

If your updates are made to a table attribute, i.e., to a table lacking an identifier. When such an update is made, Magma uses "temp ids" to keep track of the records-to-be-created. However, the censor is unable to look up these non-existent records and therefore excludes them from its restriction check. The records escape censoring, and the update may be allowed improperly.

In fact, however, I think this issue might extend even further than this, allowing any data to be attached regardless of whether it is temp or not; the censor works by making two comparisons, first computing unrestricted_identifiers using restrict: true, and then computing existing_identifiers using restrict: false, and then finding restricted_identifiers as the difference of these two sets (which must be empty).

However, if there is data that is not yet attached, the record_names for this data will not be in either set, and thus can't turn up in "restricted_identifiers".

To fix, the censor needs to take account of un-inserted data. In this case it can proceed by looking at the attachment point (that is, the parent) of the inserted records and checking if those are restricted instead. This might also get complex if you are inserting a tree of data with unattached records at several points in the hierarchy (e.g., i am inserting a new record and its new parent at the same time).

corps commented 3 years ago

So... there actually is an even simpler solution, that is also more robust. We just need to do the insert before the censor, in an open transaction, then revert the transaction on censor failure.

Currently we censor before upsert, but we could actually open the transaction from the top of dispatch records, let the upsert happen, and then fully validate the entire tree using our current approach with magma queries.