-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integrate permissions with shapes #1165
Integrate permissions with shapes #1165
Conversation
1686523
to
e54a287
Compare
40308b1
to
c8d6cc8
Compare
8251548
to
555bf56
Compare
83b079b
to
a892073
Compare
64bd8d7
to
7e794d3
Compare
580bb6b
to
ea37fe1
Compare
ea37fe1
to
8467ab8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bravo! 🥂
components/electric/lib/electric/satellite/permissions/structure.ex
Outdated
Show resolved
Hide resolved
components/electric/lib/electric/satellite/permissions/structure.ex
Outdated
Show resolved
Hide resolved
components/electric/lib/electric/satellite/permissions/structure.ex
Outdated
Show resolved
Hide resolved
components/electric/lib/electric/satellite/permissions/graph.ex
Outdated
Show resolved
Hide resolved
3f6681e
to
919a4d6
Compare
3f6075d
to
8eceac0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
components/electric/lib/electric/replication/eval/env/known_functions.ex
Outdated
Show resolved
Hide resolved
case filtering_context do | ||
%{perms: perms, xid: xid} -> | ||
{accepted_changes, rejected_changes} = | ||
Permissions.Read.filter_shape_data(perms, graph, changes, xid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code is tested in test/electric/satellite/subscriptions/permissions_read_filter_test.exs
along with the rest of the integration.
919a4d6
to
1adff8d
Compare
3bd12a7
to
e54ca27
Compare
1adff8d
to
bc81b17
Compare
e54ca27
to
028449f
Compare
bc81b17
to
e18e5e9
Compare
e1fa5d4
to
e593a50
Compare
e18e5e9
to
d658701
Compare
7cb879e
to
d930797
Compare
cefbf6e
to
8395e84
Compare
b4012b1
to
8dc6b0e
Compare
93d48b0
to
e4b22fc
Compare
8dc6b0e
to
31a887c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
|> Enum.reduce_while({:ok, %{}}, fn {path, type}, {:ok, acc} -> | ||
key = | ||
case path do | ||
[key] -> key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the Elixir.Graph.t()
type?...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Can we maybe alias as a PermGraph
to avoid shadowing the Graph
library leading to confusion while reading code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# @spec filter_changes_with_context( | ||
# perms(), | ||
# graph() | nil, | ||
# graph(), | ||
# change_list(), | ||
# xid(), | ||
# mapper_fun() | ||
# ) :: | ||
# {[term()], [term()], moves()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# @spec filter_changes_with_context( | |
# perms(), | |
# graph() | nil, | |
# graph(), | |
# change_list(), | |
# xid(), | |
# mapper_fun() | |
# ) :: | |
# {[term()], [term()], moves()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops.
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
e4b22fc
to
9799eda
Compare
bc29918
to
0cdee40
Compare
a1f498e
into
garry/vax-1425-create-permissions-protobuf-definitions-and-write-validation
Also includes fixes for: - Support for booleans encoded as `"true"` and `"false"` coming from the shapes associated data triggers
Also includes fixes for:
"true"
and"false"
coming from the shapes associated data triggers