Tightening up OpType/FlatOp logic #1910

maackle · 2023-02-10T01:00:39Z

Summary

We have decided that StoreEntry validators should always get the full entry, even if it's private. This of course means that StoreEntry authorities will not get other agents' private CRUD or capability-related ops, unless they are the author.

StoreRecord and AgentActivity validators should never have the entry data in their op if it is private, even the author (for the sake of determinism).

The current codebase has some discrepancies in these areas, this PR fixes them.

Detail

The main change done here is to replace map_entry with two new functions,get_app_entry_type_for_record_authority and get_app_entry_type_for_entry_authority. map_entry was a one-size-fits all approach to making sense of Records with and without entry data, but really we need to distinguish between the StoreEntry case and other cases when it comes to private data.

While doing this, I noticed that a lot of validation is being done in the "flattening" process, which should already be handled during sys validation. For instance, it already should be invalid for a Create AgentPubKey record to contain app entry data. We don't want to lean on Op::flattened to catch these cases, so in some cases I simplified the validation that happens at this stage, and removed test cases accordingly.

There may be some other problems of this same nature in sys validation, which I am not handling here, namely that sys validation of private data is treated the same for StoreEntry ops and other ops, when those cases need to be handled differently.

TODO:

CHANGELOG(s) updated with appropriate info
Just before pressing the merge button, ensure new entries to CHANGELOG(s) are still under the UNRELEASED heading

maackle · 2023-02-14T22:28:27Z

crates/hdi/src/flat_op/flat_op_entry.rs

+    /// This operation stores the [`Entry`] for a CapGrant
+    CreateCapGrant {
+        /// The cap grant entry data.
+        entry: CapGrantEntry,
+        /// The [`Create`] action that creates this cap grant
+        action: Create,
+    },
+    /// This operation stores the [`Entry`] for a CapClaim
+    CreateCapClaim {
+        /// The cap claim entry data.
+        entry: CapClaimEntry,
+        /// The [`Create`] action that creates this cap claim
+        action: Create,
+    },
+    /// This operation updates the [`Entry`] for a CapGrant
+    UpdateCapGrant {
+        /// The hash of the [`Action`] that created the original [`crate::CapGrant`]
+        original_action_hash: ActionHash,
+        /// The hash of the original [`crate::CapGrant`]
+        original_entry_hash: EntryHash,
+        /// The [`Update`] action that updates the [`crate::CapGrant`]
+        action: Update,
+        /// The new entry to store
+        entry: CapGrantEntry,
+    },
+    /// This operation updates the [`Entry`] for a CapClaim
+    UpdateCapClaim {
+        /// The hash of the [`Action`] that created the original [`crate::CapClaim`]
+        original_action_hash: ActionHash,
+        /// The hash of the original [`crate::CapClaim`]
+        original_entry_hash: EntryHash,
+        /// The [`Update`] action that updates the [`crate::CapClaim`]
+        action: Update,
+        /// The new entry to store
+        entry: CapClaimEntry,
+    },


This is supposing we actually do want to validate the creation of (private) cap grants and claims

maackle · 2023-02-14T22:36:50Z

crates/hdi/tests/op/mod.rs

-#[test_case(s_record(Action::Create(c(EntryType::AgentPubKey)), RecordEntry::Hidden) => matches WasmErrorInner::Guest(_))]
-#[test_case(s_record(
-    Action::Create(c(EntryType::AgentPubKey)),
-    RecordEntry::NotApplicable) => matches WasmErrorInner::Guest(_) ; "Store Record: Agent key with not applicable")]
-#[test_case(s_record(Action::Create(c(EntryType::AgentPubKey)), RecordEntry::NotStored) => matches WasmErrorInner::Host(_))]
-#[test_case(s_record(Action::Create(c(EntryType::CapClaim)), RecordEntry::Present(e(A{}))) => matches WasmErrorInner::Guest(_))]
-#[test_case(s_record(Action::Create(c(EntryType::CapClaim)), RecordEntry::NotApplicable) => matches WasmErrorInner::Guest(_))]
-#[test_case(s_record(Action::Create(c(EntryType::CapClaim)), RecordEntry::NotStored) => matches WasmErrorInner::Guest(_))]
-#[test_case(s_record(Action::Create(c(EntryType::CapGrant)), RecordEntry::Present(e(A{}))) => matches WasmErrorInner::Guest(_))]
-#[test_case(s_record(Action::Create(c(EntryType::CapGrant)), RecordEntry::NotApplicable) => matches WasmErrorInner::Guest(_))]
-#[test_case(s_record(Action::Create(c(EntryType::CapGrant)), RecordEntry::NotStored) => matches WasmErrorInner::Guest(_))]


These cases (and all others that I removed in this PR) should all be handled by sys validation.

jost-s

LGTM as far as I understand the validation (I need another Q&A session with you soon!)

crates/holochain_state/src/scratch.rs

Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>

neonphog

Don't have a whole lot of context for reviewing this, but didn't see any red flags.

* OpType -> FlatOp * Reorg * Reorg * Move flat_op to hdi * Tightening up OpType/FlatOp logic (#1910) * Failing test * Fix test * WIP big refactor * Fix all compiler errors * Add FlatOps for storing cap entries * Fix private entry type logic * Test that sort of works * Test passes * Changelog * Removed needless test cases * Fix some warnings * More specific error * Relax test case * Remove dbg!s and backtraces * Apply suggestions from code review Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>

abe-njama · 2023-03-23T10:32:15Z

@maackle should we backport this fix into 0.1.x?

* OpType -> FlatOp * Reorg * Reorg * Move flat_op to hdi * Tightening up OpType/FlatOp logic (#1910) * Failing test * Fix test * WIP big refactor * Fix all compiler errors * Add FlatOps for storing cap entries * Fix private entry type logic * Test that sort of works * Test passes * Changelog * Removed needless test cases * Fix some warnings * More specific error * Relax test case * Remove dbg!s and backtraces * Apply suggestions from code review Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>

* Fix clone cell idempotency for enable/disable (#2093) * Fix old tokio dep * Failing test and fixes of clone cell enable/disable idempotency * Changelog * Update comment * Differentate between CellMissing and CellDisabled, and other fixes (#2092) * Fix old tokio dep * Possible fix to intermittent disabled apps * Clear all ephemeral functions at once * Make cell_by_id differentiate between reasons for cell missing * Revert app status check and add detailed comment * Fix test * Clippy * Update comment for ListCellIds * Changelog * Take `name` out of DnaDefHash (#2099) * Take `name` out of DnaDefHash Fixes #2038 * Changelog * Update tests * Update crates/holochain_zome_types/CHANGELOG.md Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> * refactor(zome-call): optimize cap grant verification (#2097) (#2116) * refactor(zome-call): cap grant verification * refactor: simplify cap grant validation flow * update changelog * simplify assigned cap access arm * add PR reference to changelog * add comment to test * Apply suggestions from code review * refactor: use high level db query fn * replace hardcoded cap access variant by method --------- Co-authored-by: Michael Dougherty <maackle.d@gmail.com> * fix(conductor-api): reject duplicate clone cells (#1997) * add failing test for duplicate clone cell * reject duplicate dna hash * add duplicate dna hash error * specify error and test again after disabling clone * update changelog * add test for two agents cloning cells in same conductor * check for duplicate cell id in conductor state * OpType -> FlatOp + some reorg (#1909) * OpType -> FlatOp * Reorg * Reorg * Move flat_op to hdi * Tightening up OpType/FlatOp logic (#1910) * Failing test * Fix test * WIP big refactor * Fix all compiler errors * Add FlatOps for storing cap entries * Fix private entry type logic * Test that sort of works * Test passes * Changelog * Removed needless test cases * Fix some warnings * More specific error * Relax test case * Remove dbg!s and backtraces * Apply suggestions from code review Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> * fix typo * fix conductor tests * AppManifest `version` becomes `installed_hash` (#2157) * Make `version` take only a single optional hash * Rename AppManifest's `version` -> `installed_hash` * Changelog * Look up local DNAs before resolving via Location * Include AppManifest in AppInfo (#2156) * Return manifest with AppInfo and rewrite legacy app install * Clippy * update source 'nixpkgs' on branch 'develop' (#2158) [create-pull-request] automated change Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> * update source 'rust-overlay' on branch 'develop' (#2159) [create-pull-request] automated change Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> * fix(release-automation): consider dependency state of all matched crates (#2160) fix(release-automation): consider dependency state of all matched crates (#2149) * fix(release-automation): consider dependency state of all matched crates the change filter could lead to dependency changes not including the changed dependency in the release, if the selected crate itself didn't have any changed. the blocked filter was removed because it adds unnecessary obfuscation. * chore(release-automation): remove duplicate test * test(relase-automation): cover the fixed case * chore: add FIXME comment for a bug in the dependencies_in_workspace_filter function * test(release-automation): extend dependency bump test to cover the transitive case * refactor(release-automation/crate_selection): clarify transitive change detection * update source 'nixpkgs' on branch 'develop' (#2161) [create-pull-request] automated change Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> * Test that manifest includes most recent modifiers * Investigating BadChecksum * Update from_raw_32 to only allow 32 * Use 32 byte hash for a test where it matters * Fix fixturator * Fix one more test --------- Co-authored-by: Holochain Release Automation <100725712+holochain-release-automation2@users.noreply.github.com> Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com> --------- Co-authored-by: Holochain Release Automation <100725712+holochain-release-automation2@users.noreply.github.com> Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com> * Add backward compat test and fix existing manifest serde test * Rename to `_version` * Fix visibility * Fix bad backport * Tests that network seed always affects dna hash (#2102) * Take `name` out of DnaDefHash Fixes #2038 * Changelog * Write passing test for reported bug #2039 * Add tests for paths -- still passes * Extremely comprehensive test of network seed and location: still passes! * Even more comprehensive test: still passes! * Small fries, wip * Added more cases; still passes * Better debug * Add regression test and fix the problem * Changelog * Update crates/holochain/src/conductor/tests/install_app_bundle.rs Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> * Fix test * Improve HoloHash::from_raw_32 and use it (#2162) * Update from_raw_32 to only allow 32 * Use 32 byte hash for a test where it matters * Fix fixturator * Fix one more test * Fix test --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> Co-authored-by: Holochain Release Automation <100725712+holochain-release-automation2@users.noreply.github.com> Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com>

This reverts commit 8dbbded.

maackle added 13 commits February 9, 2023 15:33

Failing test

3662e06

Fix test

c8df80b

WIP big refactor

56525eb

Fix all compiler errors

eb5ecd5

Add FlatOps for storing cap entries

a646874

Fix private entry type logic

9987b48

Test that sort of works

14e1ba0

Test passes

26f8dcd

Changelog

e4bfeca

Removed needless test cases

cc6100f

Fix some warnings

58adc22

More specific error

03ff929

Relax test case

c34bdbf

maackle commented Feb 14, 2023

View reviewed changes

maackle marked this pull request as ready for review February 14, 2023 22:35

maackle requested review from thedavidmeister, jost-s and neonphog February 14, 2023 22:35

maackle commented Feb 14, 2023

View reviewed changes

Remove dbg!s and backtraces

a40b799

jost-s approved these changes Feb 20, 2023

View reviewed changes

crates/holochain_state/src/scratch.rs Outdated Show resolved Hide resolved

crates/holochain_state/src/scratch.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

7a14980

Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>

neonphog approved these changes Feb 20, 2023

View reviewed changes

maackle mentioned this pull request Feb 21, 2023

Fix private entry discrepancy #1866

Closed

2 tasks

maackle merged commit 8dbbded into flat-op Feb 21, 2023

maackle deleted the flat-op-2 branch February 21, 2023 19:01

maackle mentioned this pull request Feb 21, 2023

[BUG] op.to_type() throws error when creating a private entry #1861

Closed

maackle added a commit that referenced this pull request Apr 14, 2023

Revert "Tightening up OpType/FlatOp logic (#1910)"

35bd117

This reverts commit 8dbbded.

This was referenced Apr 19, 2023

Holochain 0.1.4 holochain/scaffolding#89

Merged

Holochain 0.1.4 (plus main branch) holochain/scaffolding#90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tightening up OpType/FlatOp logic #1910

Tightening up OpType/FlatOp logic #1910

maackle commented Feb 10, 2023 •

edited

maackle Feb 14, 2023

maackle Feb 14, 2023

jost-s left a comment

neonphog left a comment

abe-njama commented Mar 23, 2023

Tightening up OpType/FlatOp logic #1910

Tightening up OpType/FlatOp logic #1910

Conversation

maackle commented Feb 10, 2023 • edited

Summary

Detail

TODO:

maackle Feb 14, 2023

Choose a reason for hiding this comment

maackle Feb 14, 2023

Choose a reason for hiding this comment

jost-s left a comment

Choose a reason for hiding this comment

neonphog left a comment

Choose a reason for hiding this comment

abe-njama commented Mar 23, 2023

maackle commented Feb 10, 2023 •

edited