edgedb / rfcs

RFCs for major changes to EdgeDB
Apache License 2.0
35 stars 5 forks source link

RFC 1011: Policy based query rewrite #50

Closed elprans closed 2 years ago

elprans commented 2 years ago

A couple of open questions:

  1. Do we want the ability to specify multiple actions per policy. If we do, then the syntax can be adjusted to
create policy on select {
  permit (.foo = 10);
  do after (log access);
}

The downside is that with this formulation we will likely need to specify or expose policy names to address them in alter or drop, which brings us to:

  1. Do we need the ability to give policies names? Currently, the design follows that of constraints and indexes where the actual schema object name is derived from the definition and works well for SDL, as you don't need to be bothered with giving policies bogus names. DDL users will have a harder time, but the answer, like with complex constraints or indexes, is "run DESCRIBE" and copy-paste.
elprans commented 2 years ago

Honestly, I find it hard to comprehend interactions between WHEN, PERMIT and RESTRICT TO. I'm not sure if this is better than just writing a single policy with manually writing a filter with all needed AND and OR.

Not sure I understand. How are you going to specify the way inherited policies should combine?

tailhook commented 2 years ago

Honestly, I find it hard to comprehend interactions between WHEN, PERMIT and RESTRICT TO. I'm not sure if this is better than just writing a single policy with manually writing a filter with all needed AND and OR.

Not sure I understand. How are you going to specify the way inherited policies should combine?

Yes I did not realize that there will be inherited policies. This can easily become security nightmare, though.

Not for my "single policy" proposal. But one option to resolve inheritance nightmare, would be naming policies and specifying inherited names:

abstract type Shareable {
  # permit read access to friends
  policy "permit_friends" on select permit (global user in .owner.friends);
}

# Post inherits policies from Owned and Shareable
# which allow access to either owner or friends initially...
type Post extending Owned, Shareable {
  inherits policy "permit_friends";
  # ... other policies
}

This will make it easier to read policies for specific objects. And will make less cases for restrict to policy.

This could also create a new pattern where base type is kinda a "library" for the policies, that different inherited types can pick from (not sure if it's a good or bad, though)

elprans commented 2 years ago

But one option to resolve inheritance nightmare, would be naming policies and specifying inherited names:

I don't think it's a good idea at all. First of all, everything else in the schema is inherited, so there will be reasonable expectation that policies do as well. Second, even if you are aware of the need to manually propagate policies, it's still very error prone, as you will have to trace the inheritance hierarchy manually, and it's easy to overlook some types and fail to inherit a "restrict".

tailhook commented 2 years ago

But one option to resolve inheritance nightmare, would be naming policies and specifying inherited names:

I don't think it's a good idea at all. First of all, everything else in the schema is inherited, so there will be reasonable expectation that policies do as well. Second, even if you are aware of the need to manually propagate policies, it's still very error prone, as you will have to trace the inheritance hierarchy manually, and it's easy to overlook some types and fail to inherit a "restrict".

RESTRICT in inherited types is a huge footgun, I think. Here is the scenario:

  1. You see that something is invisible for your current task, say you're doing moderator interface and Articles are only visible to the user
  2. You add PERMIT rule, it doesn't work
  3. You traverse hierarchy and see where the offending RESTRICT rule is
  4. You find a the RESTRICT TO rule, and add an exception to it.
  5. Now you need to do one of the two things: a. Evaluate whether the visibility of every other object is fine with the new rule b. Add original RESTRICT TO rule to every other object inherited from that base class

I can predict that in 90% of the cases nobody will do the step (5), or just pretend that they did (5a). But we can't even write a good guide on how to do (5a).

elprans commented 2 years ago

This RFC has been superseded by #54 (object-level security), and a separate future triggers RFC.