Integrate permissions with shapes #1165

magnetised · 2024-04-17T16:39:26Z

Also includes fixes for:

Support for booleans encoded as "true" and "false" coming from the shapes associated data triggers

linear · 2024-04-17T16:39:29Z

VAX-1712 Integrate shapes and permissions

components/electric/lib/electric/postgres/schema/fk_graph.ex

alco

Bravo! 🥂

components/electric/lib/electric/satellite/permissions/structure.ex

components/electric/lib/electric/satellite/permissions/graph.ex

components/electric/lib/electric/satellite/permissions/read.ex

components/electric/lib/electric/satellite/protocol.ex

icehaunter

Great work. I've a couple of questions to resolve before merging, but feels good overall

components/electric/lib/electric/plug/satellite_websocket_plug.ex

components/electric/lib/electric/postgres/schema/fk_graph.ex

components/electric/lib/electric/replication/eval/env/known_functions.ex

components/electric/lib/electric/satellite/protocol/state.ex

components/electric/lib/electric/satellite/protocol.ex

icehaunter · 2024-05-03T12:36:34Z

components/electric/lib/electric/replication/shapes/querying.ex

+    case filtering_context do
+      %{perms: perms, xid: xid} ->
+        {accepted_changes, rejected_changes} =
+          Permissions.Read.filter_shape_data(perms, graph, changes, xid)


question: I'm not fluent in permission application code paths yet. This won't ever have a full view of the client's graph, because querying functions are building up a partial graph for only the queried rows (because if we passed a full graph here and updated THAT, then we can't plainly merge the results back because they would have diverged - the graph in the main process updated with ongoing txns and some rows may have been removed) (also because copying a full graph across to a Task process like that feels bad man). Will this partial graph be enough to correctly validate the permissions in more complicated cases?

I don't see a test that covers this code path, and I'd really want to see one. Feels like a very important place that's very edge-case-y

since at the moment the permissions scopes rely on foreign key relations, and the shapes queries follow and fill those fk relations the graph here is complete for permissions resolution (atm at least).

at the point where permissions scopes rely on non-fk info, e.g. in a USING clause on the grants, then we'll either need that information to be passed to the shape query or will need to make cross-graph lookups.

the code is tested in test/electric/satellite/subscriptions/permissions_read_filter_test.exs along with the rest of the integration.

components/electric/lib/electric/satellite/permissions.ex

icehaunter

Looks good with some nitpicks and a question I think warrants addressing, but that's still pretty small. I'm a bit worried about red elixir tests tho...

icehaunter · 2024-05-20T13:17:24Z

components/electric/lib/electric/replication/eval/runner.ex

+    |> Enum.reduce_while({:ok, %{}}, fn {path, type}, {:ok, acc} ->
+      key =
+        case path do
+          [key] -> key


Are you sure we want to auto-map prefix-less columns? I see you're calling this mapping and THEN calling a defp prefix_value in Electric.Satellite.Permissions.Eval, and I'm wondering if it wouldn't be safer to prefix first if you want to prefix at all?

I think it's useful to support prefix-less columns out of the scope of permissions, to avoid requiring every column reference to be prefixed with e.g. this in where clauses etc. the prefix-less version feels very natural, it's exactly how you'd do it in e.g. a select where you only need to prefix to avoid ambiguity.

so there may be some redundancy here, but I think it's worth it

icehaunter · 2024-05-20T13:34:01Z

components/electric/lib/electric/replication/shapes/querying.ex

+    fill_graph_layer(graph, records, key)
+  end
+
+  defp maybe_fill_first_graph_layer(%Graph{} = graph, %Layer{}, _), do: graph
+
+  defp fill_graph_layer(graph, records, key) do
    Enum.reduce(
      records,
      graph,
-      fn {id, _}, graph -> Graph.add_edge(graph, :root, id, label: key) end
+      fn {{_relation, _pk} = id, _change}, graph ->
+        Graph.add_edge(graph, :root, id, label: key)
+      end
    )
  end


nitpick: can we revert this? I really don't like a generic name fill_graph_layer here because someone will reach for it in 6 months, and this is meant explicitly only for first layer, hence :root as one of the nodes on the edge. I don't feel like this change adds readability because of this.

icehaunter · 2024-05-20T14:00:18Z

components/electric/lib/electric/satellite/permissions/eval.ex

+  defp values(expr, record, prefix) do
+    {:ok, record} = Runner.record_to_ref_values(expr.used_refs, record)
+    Map.new(record, &prefix_value(&1, prefix))
+  end


Runner.execute/2 fetches values from the provided values map just based on the full key, no "additional" matching. So if the expression contains a reference to a column, it has to be structured as new.column instead of just column. You're validating it exactly like that by building only prefixed table_refs in this module -- which is great, as it avoids ambiguity, but if that's the case, the single-column clause just can't happen - neither here in prefix_value, nor in Runner.record_to_ref_values/2.

You can get non-prefixed columns if the where/if clause is written without them. The tests in permissions/where_clause_test.exs fail if I remove handling of the prefix-less case

icehaunter · 2024-05-20T14:04:46Z

components/electric/lib/electric/satellite/permissions/read.ex

+  @spec shape_data_mapper(shape_data_row()) :: Changes.change()
+  defp shape_data_mapper({_key, {change, _}}), do: change
+
+  @spec ensure_graph_impl(Elixir.Graph.t() | Graph.impl()) :: Graph.impl()


That's the Elixir.Graph.t() type?...

Not sure what you're asking here. I'm planning a bit of a rename before merging, partly because this Permissions.Graph choice was a bad one in hindsight, partly to just simplify things.

icehaunter · 2024-05-20T14:05:41Z

components/electric/lib/electric/satellite/permissions/read.ex

  alias Electric.Replication.Changes
  alias Electric.Satellite.Permissions
+  alias Electric.Satellite.Permissions.Eval
  alias Electric.Satellite.Permissions.MoveOut
  alias Electric.Satellite.Permissions.Role
  alias Electric.Satellite.Permissions.Graph


nitpick: Can we maybe alias as a PermGraph to avoid shadowing the Graph library leading to confusion while reading code?

You have a point. I've been managing this name conflict but seeing that it'll be much more confusing in 6 months time. See above re renaming.

icehaunter · 2024-05-20T14:06:33Z

components/electric/lib/electric/satellite/permissions/read.ex

+  # @spec filter_changes_with_context(
+  #         perms(),
+  #         graph() | nil,
+  #         graph(),
+  #         change_list(),
+  #         xid(),
+  #         mapper_fun()
+  #       ) ::
+  #         {[term()], [term()], moves()}


Suggested change

# @spec filter_changes_with_context(

# perms(),

# graph() | nil,

# graph(),

# change_list(),

# xid(),

# mapper_fun()

# ) ::

# {[term()], [term()], moves()}

magnetised · 2024-05-29T09:45:01Z

I'm a bit worried about red elixir tests tho...

The next pr in the stack fixes the test failures. Haven't bothered to port those fixes down the stack...

uuid<->text column testing now just works

…connect (#1206)

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch 2 times, most recently from 1686523 to e54a287 Compare April 18, 2024 09:16

Base automatically changed from garry/vax-1439-where-clauses to garry/vax-1645-merge-grant-statements-into-a-global-state April 23, 2024 09:03

Base automatically changed from garry/vax-1645-merge-grant-statements-into-a-global-state to garry/vax-1644-serialise-grant-statements-to-protobuf April 23, 2024 09:04

Base automatically changed from garry/vax-1644-serialise-grant-statements-to-protobuf to garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation April 23, 2024 09:05

magnetised force-pushed the garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation branch from 40308b1 to c8d6cc8 Compare April 23, 2024 09:31

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from 8251548 to 555bf56 Compare April 23, 2024 14:41

magnetised changed the base branch from garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation to garry/migration-message April 23, 2024 14:42

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from 83b079b to a892073 Compare April 24, 2024 08:28

magnetised force-pushed the garry/migration-message branch from 64bd8d7 to 7e794d3 Compare April 24, 2024 12:39

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from 580bb6b to ea37fe1 Compare April 24, 2024 13:29

Base automatically changed from garry/migration-message to garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation April 25, 2024 09:42

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from ea37fe1 to 8467ab8 Compare April 25, 2024 13:57

magnetised marked this pull request as ready for review April 26, 2024 06:02

magnetised requested review from alco and icehaunter April 26, 2024 06:03

alco reviewed Apr 30, 2024

View reviewed changes

components/electric/lib/electric/postgres/schema/fk_graph.ex Outdated Show resolved Hide resolved

components/electric/lib/electric/postgres/schema/fk_graph.ex Outdated Show resolved Hide resolved

components/electric/lib/electric/postgres/schema/fk_graph.ex Outdated Show resolved Hide resolved

alco approved these changes May 1, 2024

View reviewed changes

magnetised force-pushed the garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation branch from 3f6681e to 919a4d6 Compare May 2, 2024 15:11

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from 3f6075d to 8eceac0 Compare May 2, 2024 15:12

icehaunter requested changes May 3, 2024

View reviewed changes

magnetised force-pushed the garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation branch from 919a4d6 to 1adff8d Compare May 9, 2024 16:18

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from 3bd12a7 to e54ca27 Compare May 9, 2024 16:18

magnetised force-pushed the garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation branch from 1adff8d to bc81b17 Compare May 13, 2024 09:29

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from e54ca27 to 028449f Compare May 13, 2024 09:30

magnetised force-pushed the garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation branch from bc81b17 to e18e5e9 Compare May 13, 2024 12:44

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from e1fa5d4 to e593a50 Compare May 13, 2024 12:44

magnetised requested a review from icehaunter May 14, 2024 08:48

magnetised force-pushed the garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation branch from e18e5e9 to d658701 Compare May 14, 2024 12:56

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch 2 times, most recently from 7cb879e to d930797 Compare May 14, 2024 13:32

magnetised force-pushed the garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation branch from cefbf6e to 8395e84 Compare May 15, 2024 09:50

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch 3 times, most recently from b4012b1 to 8dc6b0e Compare May 15, 2024 10:40

magnetised force-pushed the garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation branch from 93d48b0 to e4b22fc Compare May 20, 2024 10:55

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from 8dc6b0e to 31a887c Compare May 20, 2024 10:55

icehaunter approved these changes May 20, 2024

View reviewed changes

magnetised added 15 commits May 29, 2024 11:08

integrate permissions with shapes

0791a12

address pr feedback

1dfee4c

correctly cast record values before eval

58482e3

handle other subscription paths

8754b03

move permissions application to protocol

a92eff3

fix bool stuff

2bd6407

typo

ede5b4a

fix dialyzer and update read perms api

6193e5b

run elixir tests on bigger worker

03500e8

use tmpfs for pg data

cec1da9

remove comment

399ca87

revert 'run elixir tests on bigger worker'

ec1178c

fixes after rebase

56c9ae8

pr feedback

f2d0997

remove explicit casting of user_id column

0cdee40

uuid<->text column testing now just works

magnetised force-pushed the garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation branch from e4b22fc to 9799eda Compare May 29, 2024 10:13

magnetised force-pushed the garry/vax-1712-integrate-shapes-and-permissions branch from bc29918 to 0cdee40 Compare May 29, 2024 10:13

Feature flag and permissions change handling via client reset and dis…

b6f4e77

…connect (#1206)

magnetised merged commit a1f498e into garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation May 29, 2024
13 checks passed

magnetised deleted the garry/vax-1712-integrate-shapes-and-permissions branch May 29, 2024 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate permissions with shapes #1165

Integrate permissions with shapes #1165

magnetised commented Apr 17, 2024 •

edited

linear bot commented Apr 17, 2024

alco left a comment

icehaunter left a comment

icehaunter May 3, 2024

icehaunter May 3, 2024

magnetised May 7, 2024

magnetised May 7, 2024

icehaunter left a comment

icehaunter May 20, 2024

magnetised May 29, 2024

icehaunter May 20, 2024

icehaunter May 20, 2024

magnetised May 29, 2024 •

edited

icehaunter May 20, 2024

magnetised May 29, 2024

icehaunter May 20, 2024

magnetised May 29, 2024

icehaunter May 20, 2024

magnetised May 29, 2024

magnetised commented May 29, 2024

Integrate permissions with shapes #1165

Integrate permissions with shapes #1165

Conversation

magnetised commented Apr 17, 2024 • edited

linear bot commented Apr 17, 2024

alco left a comment

Choose a reason for hiding this comment

icehaunter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icehaunter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magnetised May 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magnetised commented May 29, 2024

magnetised commented Apr 17, 2024 •

edited

magnetised May 29, 2024 •

edited