Did an refactoring to enable snapshot for fetch_expanded_live_specs, …

[bbartman]

Description:

This is a continuation of previous work to remove the old style of permissions, and replace it with the snapshot style of permissions.

Remaining uses of internal.user_roles

• fetch_expanded_live_specs crates/control-plane-api/src/live_specs/db.rs:129
• resolve_specs crates/control-plane-api/src/evolutions/db.rs#L81
• user_has_admin_capability crates/control-plane-api/src/directives/storage_mappings.rs#L5
• create_data_plane crates/control-plane-api/src/server/create_data_plane.rs#L57
• update_l2_reporting crates/control-plane-api/src/server/update_l2_reporting.rs#L27

This PR relies on work previously done here.

Documentation links affected:

N/A

Notes for reviewers:

Change approach rationale:

fetch_expanded_live_specs

There was a cascading affect from this up to a trait called Initialize. I did a refactoring on the initialize member of that trait and added the snapshot to the parameter list, I also left the user_id in the parameter list. Here is the rationale for this, 1) The implementer of initialize, needs the user_id to figure out permissions, and 2) not all implementers of the ``initialize` function actually use the snapshot. Leaving both pieces of information there means that those that don't need the permissions check don't use it and can just ignore the parameters.

Here is the rationale for adding this as a function parameter instead of a member. The only caller of the initialize trait had access to the snapshot already, so there was minimal change there, and there was more than one implementer of the initialize trait, and I didn't want to figure out each one was created, add a member for initialization, then thread the snapshot through the rest of the related code. I figured this out after looking into the locations that the implementers of the Initialize trait were being built. The locations where those types were being initialized did not have access to the snapshot, there by spreading the snapshot dependency, and making implementation more difficult.

resolve_specs

This function has no call sites that I can find. Did normal refactoring to remove the user_id, and write the function in terms of a set of permissions. Did an outside of DB query optimization by adding a filter to the construction of the permissions list. In the original DB call we were filtering on admin so I moved the filter to before we sent the arrays to the query.

user_has_admin_capability

This function was changed to use the snapshot. Because this uses a transaction I had claude do some additional verification on if there was any way that permissions could be modified during that time. TL;DR no path that leads to that function changes or modifies permissions so it's safe to use the snapshot.

Claude output.

There is no path that leads to user_can_admin_tenant where permissions are changed in the same transaction — or by any reachable function — before (or after) the check. The requirements for a read-your-writes bug (a caller mutating grants in the same txn and relying on the check seeing them) are not met:

• no grant mutation occurs in the reachable set, and
• each directive gets its own transaction, so beta_onboard's grant writes can't leak into a storage-mappings check's transaction.

create_data_plane & update_l2_reporting

Note both of these functions used identical code so I refactored to use the same funciton.

Here is what was done.

1. Extract the SQL into a single separate function
2. Create a test suite to ensure lack of behavioral changes.
3. Make sure the tests pass as expected.
4. Update the extracted function to use the snapshot
5. Make any implementation changes to function and make sure we preserve behavior.

After a code review, I added support for a deny and retry permission structure.

Added improvements to test harness evaluation speed

This is to fix something I added in a previous MR. Basically, we ran into a problem where we had to wait on the refresh of the snapshot which cost about 20 seconds this eventually made tests runtime cost about 40 - 60 seconds per tests. This change reduces the runtime down to about 67s locally or just over a minute. Here is the claude description of what was fixed:

From claude

The harness no longer uses PgSnapshotSource's timer-gated polling loop. Instead it owns both halves of a tokens::manual() watch:

• New field set_snapshot: Box<dyn Fn(Snapshot) + Send + Sync> — the write handle to the single snapshot_watch (not a second snapshot; just how the harness updates the one snapshot).
• Build site — replaced PgSnapshotSource::new + tokens::watch(...) with tokens::manual(), an initial direct fetch_snapshot, and stored the wrapped set_snapshot closure.
• New helper TestHarness::fetch_snapshot(pool) — runs control_plane_api::snapshot::try_fetch + Snapshot::new (the same query PgSnapshotSource runs, minus the cool-off).
• refresh_snapshot — now just fetch_snapshot + (self.set_snapshot)(snapshot). No revoke.cancel(), no yield_now spin, no background task, no MIN_REFRESH_INTERVAL cool-off, and no reliance on paused-time auto-advance.