fix(dal,sdf): when finding change sets, ensure it belongs to the relevant workspace by britmyerss · Pull Request #5245 · systeminit/si

britmyerss · 2025-01-13T20:21:11Z

This change introduces ChangeSet::find_across_workspaces to differentiate from ChangeSet::find to ensure we're not acting on ChangeSets that don't belong to the Workspace in question.

via The Tonight Show Starring Jimmy Fallon on GIPHY

github-actions · 2025-01-13T20:21:30Z

Dependency Review

✅ No vulnerabilities or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Files

britmyerss · 2025-01-13T20:25:18Z

lib/dal/src/workspace_snapshot/migrator.rs


        for change_set in open_change_sets {
-            let mut change_set = ChangeSet::find(ctx, change_set.id)
+            let mut change_set = ChangeSet::find_across_workspaces(ctx, change_set.id)


I believe this is correct - @zacharyhamm could use a check

It's hard to imagine this isn't correct, given the fact that these change set IDs came from list_open_for_all_workspaces()!. (Again, it at least doesn't make things worse.)

this is what we want, yes. The migrator will set up the dal context for the right visibility and tenancy but only after it finds the workspace, so this is definitely necessary

britmyerss · 2025-01-13T22:37:26Z

/try

github-actions · 2025-01-13T22:37:39Z

Okay, starting a try! I'll update this comment once it's running...\n
🚀 Try running here! 🚀

britmyerss · 2025-01-14T00:30:28Z

lib/dal/src/context.rs

+    /// for the current [`DalContext`]
    pub async fn update_snapshot_to_visibility(&mut self) -> TransactionsResult<()> {
-        let change_set = ChangeSet::find(self, self.change_set_id())
+        let change_set = ChangeSet::find_across_workspaces(self, self.change_set_id())


Could do a similar move here to have a tenancy-scoped version of this method, but not in this PR. Note that scoping this to the workspace breaks all of our integration tests at the moment.

britmyerss · 2025-01-14T00:31:49Z

lib/dal/tests/integration_test/change_set.rs

+        .expect("could not build dal context");
+
+    //first, let's ensure we can't find it when using the user 1 dal ctx
+    let user_2_change_set_unfound = ChangeSet::find(&user_1_dal_context, user_2_change_set.id)


before this change, we would User-1-Workspace-1 would successfully find (and be able to abandon) the change set in User-2's Workspace.

jkeiser

Super glad to see it narrowed down to just a few callers!

jkeiser · 2025-01-14T01:05:12Z

lib/dal/src/change_set.rs

    pub async fn apply_to_base_change_set(ctx: &mut DalContext) -> ChangeSetApplyResult<ChangeSet> {
        // Apply to the base change with the current change set (non-editing) and commit.
-        let mut change_set_to_be_applied = Self::find(ctx, ctx.change_set_id())
+        let mut change_set_to_be_applied = Self::find_across_workspaces(ctx, ctx.change_set_id())


This one might be able to be ChangeSet::find() (it seems like if we really need find_across_workspaces than we have been given a busted DalContext). But if you're worried about it or noticed it breaks things, I'm 100% on board with "fix most stuff and then look at these in another PR").

yeah I think you're right! Will fix

jkeiser · 2025-01-14T01:06:16Z

lib/dal/src/workspace_snapshot/migrator.rs


        for change_set in open_change_sets {
-            let mut change_set = ChangeSet::find(ctx, change_set.id)
+            let mut change_set = ChangeSet::find_across_workspaces(ctx, change_set.id)


It's hard to imagine this isn't correct, given the fact that these change set IDs came from list_open_for_all_workspaces()!. (Again, it at least doesn't make things worse.)

britmyerss · 2025-01-14T15:44:28Z

/try

github-actions · 2025-01-14T15:44:42Z

Okay, starting a try! I'll update this comment once it's running...\n
🚀 Try running here! 🚀

zacharyhamm · 2025-01-14T15:50:06Z

lib/dal/src/change_set.rs

+                }
+                Ok(Some(change_set))
+            }
+            None => Ok(None),


If we emitted an info or error here, we could track any time this returns None. Which should be almost never, except maybe stale URLs in cache for applied/abandoned change sets

(or set an attribute, whichever makes the most sense for a honeycomb query)

good shout - will add!

…vant workspace

jkeiser

Lock it down!

britmyerss requested a review from fnichol January 13, 2025 20:21

github-actions bot added A-sdf Area: Primary backend API service [Rust] A-dal labels Jan 13, 2025

britmyerss requested review from jkeiser and zacharyhamm January 13, 2025 20:21

britmyerss commented Jan 13, 2025

View reviewed changes

britmyerss force-pushed the brit/ensure-change-sets-belong-to-workspaces branch 2 times, most recently from 425cd9c to a5c49bb Compare January 13, 2025 22:36

britmyerss force-pushed the brit/ensure-change-sets-belong-to-workspaces branch 2 times, most recently from bded452 to f7f8e33 Compare January 14, 2025 00:28

britmyerss commented Jan 14, 2025

View reviewed changes

britmyerss marked this pull request as ready for review January 14, 2025 00:32

jkeiser previously approved these changes Jan 14, 2025

View reviewed changes

britmyerss dismissed jkeiser’s stale review via 2603a0a January 14, 2025 15:24

britmyerss force-pushed the brit/ensure-change-sets-belong-to-workspaces branch from f7f8e33 to 2603a0a Compare January 14, 2025 15:24

zacharyhamm reviewed Jan 14, 2025

View reviewed changes

jkeiser previously approved these changes Jan 14, 2025

View reviewed changes

fix(dal,sdf): when finding change sets, ensure it belongs to the rele…

8e15c47

…vant workspace

britmyerss dismissed jkeiser’s stale review via 8e15c47 January 14, 2025 19:27

britmyerss force-pushed the brit/ensure-change-sets-belong-to-workspaces branch from 2603a0a to 8e15c47 Compare January 14, 2025 19:27

britmyerss requested a review from jkeiser January 15, 2025 02:20

jkeiser approved these changes Jan 15, 2025

View reviewed changes

jkeiser added this pull request to the merge queue Jan 15, 2025

jkeiser removed this pull request from the merge queue due to a manual request Jan 15, 2025

jkeiser added this pull request to the merge queue Jan 15, 2025

Merged via the queue into main with commit 8ada592 Jan 15, 2025

jkeiser deleted the brit/ensure-change-sets-belong-to-workspaces branch January 15, 2025 21:26

Conversation

britmyerss commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

OpenSSF Scorecard

Scanned Files

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

britmyerss commented Jan 13, 2025

Uh oh!

github-actions bot commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkeiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

britmyerss commented Jan 14, 2025

Uh oh!

github-actions bot commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkeiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

britmyerss commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading