Re: Add support for restrictive RLS policies

 

Here are the review comments:

1. In documentation, we should put both permissive as well as restrictive in
the header like permissive|restrictive. Or may be a generic term, say, policy
type (like we have command and then options mentioned like all, select etc.),
followed by options in the description. Or like we have CASCADE and RESTRICT
in drop case.

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."?

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

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

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

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

@@ -2133,6 +2139,25 @@ psql_completion(const char *text, int start, int end)
     /* Complete "CREATE POLICY <name> ON <table> USING (" */
     else if (Matches6("CREATE", "POLICY", MatchAny, "ON", MatchAny, "USING"))
         COMPLETE_WITH_CONST("(");
+    /* CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE FOR ALL|SELECT|INSERT|UPDATE|DELETE */
+    else if (Matches8("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS", MatchAny, "FOR"))
+        COMPLETE_WITH_LIST5("ALL", "SELECT", "INSERT", "UPDATE", "DELETE");
+    /* Complete "CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE FOR INSERT TO|WITH CHECK" */
+    else if (Matches9("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS", MatchAny, "FOR", "INSERT"))
+        COMPLETE_WITH_LIST2("TO", "WITH CHECK (");
+    /* Complete "CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE FOR SELECT|DELETE TO|USING" */
+    else if (Matches9("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS", MatchAny, "FOR", "SELECT|DELETE"))
+        COMPLETE_WITH_LIST2("TO", "USING (");
+    /* CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE FOR ALL|UPDATE TO|USING|WITH CHECK */
+    else if (Matches9("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS", MatchAny, "FOR", "ALL|UPDATE"))
+        COMPLETE_WITH_LIST3("TO", "USING (", "WITH CHECK (");
+    /* Complete "CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE TO <role>" */
+    else if (Matches8("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS", MatchAny, "TO"))
+        COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles);
+    /* Complete "CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE USING (" */
+    else if (Matches8("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS", MatchAny, "USING"))
+        COMPLETE_WITH_CONST("(");

7. Natts_pg_policy should be updated to 7 now.

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

                        ereport(ERROR,
                                (errcode(ERRCODE_SYNTAX_ERROR),
                             errmsg("unrecognized row security option \"%s\"", $1),
                                 errhint("Only PERMISSIVE or RESTRICTIVE policies are supported currently."),
                                     parser_errposition(@1)));

You will see following result otherwise:

postgres=# CREATE POLICY my_policy ON foo AS a123;
ERROR:  unrecognized row security option "as"
LINE 1: CREATE POLICY my_policy ON foo AS a123;
                                       ^
HINT:  Only PERMISSIVE or RESTRICTIVE policies are supported currently.

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

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

 *        QUERIES:
 *                CREATE POLICY name ON table [FOR cmd] [TO role, ...]
 *                    [USING (qual)] [WITH CHECK (with_check)]

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

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.

12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
appropriate, like other default cases.
strcmp(polinfo->polpermissive,"t") == 0 ? "PERMISSIVE" : "RESTRICTIVE"

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

I am not familiar or used TAP framework. So no idea about these changes.

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)?

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.

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

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

18. Fix typos pointed earlier by Alvera.

Thanks