Re: Add support for restrictive RLS policies

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen, the typo "awseome" in the tests is a bit distracting.  Can you
> please fix it?

Done.

> I think you should use braces here, not parens:

Fixed.

> I don't think this paragraph is right -- you should call out each of the
> values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
> typos "Alternativly" and "visibillity".

Done.

> I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> only appear in comments within rowsecurity.c (of your authorship too, I
> imagine).  I think it'd be better to find actual words for those
> actions.

Reworded to not attempt to use AND and OR as verbs.  Additionally, a
patch is also included to remove those from the comments in
rowsecurity.c.  There are a few other places where we have "OR'd" in the
code base, but I didn't think it made sense to change those as part of
this effort.

* Jeevan Chalke (jeevan.chalke@enterprisedb.com) wrote:
> With this patch, pg_policy catalog now has seven columns, however
> Natts_pg_policy is still set to 6. It should be updated to 7 now.
> Doing this regression seems OK.

Ah, certainly interesting that it only caused incorrect behavior and not
a crash (and no incorrect behavior even on my system, at least with the
regression tests and other testing I've done).

Fixed.

> 1. In documentation, we should put both permissive as well as restrictive in
> the header like permissive|restrictive.

I'm not sure which place in the documentation you are referring to
here..?  [ AS { PERMISSIVE | RESTRICTIVE } ] was added to the CREATE
POLICY synopsis documentation.

> 2. "If the policy is a "permissive" or "restrictive" policy." seems broken
> as
> sentence starts with "If" and there is no other part to it. Will it be
> better
> to say "Specifies whether the policy is a "permissive" or "restrictive"
> policy."?

Rewrote this to be clearer, I hope.

> 3. " .. a policy can instead by "restrictive""
> Do you mean "instead be" here?

This was also rewritten.

> 4. It will be good if we have an example for this in section
> "5.7. Row Security Policies"

I haven't added one yet, but will plan to do so.

> 5. AS ( PERMISSIVE | RESTRICTIVE )
> should be '{' and '}' instead of parenthesis.

Fixed.

> 6. I think following changes are irrelevant for this patch.
> Should be submitted as separate patch if required.

As mentioned, this is tab-completion for the new options which this
patch introduces.

> 7. Natts_pg_policy should be updated to 7 now.

Fixed.

> 8. In following error, $2 and @2 should be used to correctly display the
> option and location.

Fixed.

> I think adding negative test to test this error should be added in
> regression.

Done.

> 9. Need to update following comments in gram.y to reflect new changes.

Done.

> 10. ALTER POLICY has no changes for this. Any reason why that is not
> considered here.

As mentioned, I don't see a use-case for it currently.

> 11. Will it be better to use boolean for polpermissive in _policyInfo?
> And then set that appropriately while getting the policies. So that later we
> only need to test the boolean avoiding string comparison.

Done.

> 12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
> appropriate, like other default cases.

Done, for this and the other defaults.

> 13. Since PERMISSIVE is default, do we need changes like below?
> -            \QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
> +            \QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
> PUBLIC \E

Updated to reflect what pg_dump now produces.

> 14. While displaying policy details in permissionsList, per syntax, we
> should
> display (RESTRICT) before the command option. Also will it be better to use
> (RESTRICTIVE) instead of (RESTRICT)?

Fixed.

> 15. Similarly in describeOneTableDetails() too, can we have RESTRICTIVE
> after
> policy name and before command option ?
> If we do that then changes related to adding "POLICY" followed by
> "RESTRICTIVE"
> will be straight forward.

Fixed.

> 16. It be good to have test-coverage for permissionsList,
> describeOneTableDetails and dump-restore changes. Please add those.

Done.

> 17. In pg_policies view, we need to add details related to PERMISSIVE and
> RESTRICTIVE. Please do so. Also add test for it.

Done.

> 18. Fix typos pointed earlier by Alvera.

Done.

Updated patch attached.

Thanks!

Stephen