Report failing access policy

[aljazerzen]

Closes #4095

• refactor clauses.py
• report names of failed access policies

I have tests to do, but would ask for a quick peek if the major changes are ok.

[aljazerzen]

For this schema:

module default {
  global cur_user -> str;

  type Movie {
    required property title -> str;
    property release_year -> int64;
    multi link actors -> Person;
    link director -> Person;

    access policy insert_only_by_aljaz
      allow insert using (
          (global cur_user = 'aljaz') ?? false
      );
    access policy prevent_cats
      deny insert using (.title = 'cats');

	access policy cur_watchers
      allow select using (
          (global cur_user = 'admin') ?? false
      );
  }  
}

This is how it displays the errors:

edgedb> insert Movie { title := 'space man' };
edgedb error: AccessPolicyError: access policy violation on insert of default::Movie (none of these allow policies match: __::insert_only_by_aljaz)

edgedb> set global cur_user := 'aljaz';
OK: SET GLOBAL

edgedb> insert Movie { title := 'space man' };
{default::Movie {id: a78638c4-4bd6-11ed-85fe-9374378a0a92}}


edgedb> insert Movie { title := 'cats' };
edgedb error: AccessPolicyError: access policy violation on insert of default::Movie (denied by policy __::prevent_cats)


edgedb> insert Movie { title := '1942' };
{default::Movie {id: ae02dbc6-4bd6-11ed-85fe-b79b097b96ae}}


edgedb> select Movie { title };
{}

edgedb> set global cur_user := 'admin';
OK: SET GLOBAL

edgedb> select Movie { title };
{default::Movie {title: 'space man'}, default::Movie {title: '1942'}}

[aljazerzen]

The codegen is getting complex, I don't know how to simplify it.

This is generated SQL

...
  "policy~1" AS MATERIALIZED (
    (
        SELECT
            edgedb.raise_on_null(
                nullif("expr-29~2"."expr~29_value~1", false),
                'insufficient_privilege',
                msg => 'access policy violation on insert of default::Movie (none of these allow policies match: __::insert_only_by_aljaz)',
                "table" => 'cd33f102-4ee2-11ed-93f1-01441a342449'
            ) AS error_allow,
            edgedb.raise_on_not_null(((
                SELECT string_agg(error_msg, ', ')
                FROM ((
                    SELECT 'denied by policy __::prevent_cats'
                    WHERE "expr-31~2"."expr~31_value~1"
                ) UNION (
                    SELECT 'denied by policy __::prevent_start_on_c'
                    WHERE "expr-35~2"."expr~35_value~1"
                )) AS t(error_msg))),
                'insufficient_privilege',
                msg => (
                    'access policy violation on insert of default::Movie (' || (((
                        SELECT string_agg(error_msg, ', ')
                        FROM ((
                            SELECT 'denied by policy __::prevent_cats'
                            WHERE "expr-31~2"."expr~31_value~1"
                        ) UNION (
                            SELECT 'denied by policy __::prevent_start_on_c'
                            WHERE "expr-35~2"."expr~35_value~1"
                        )) AS t(error_msg))
                    ) || ')')
                ),
                "table" => 'cd33f102-4ee2-11ed-93f1-01441a342449'
            ) AS error_deny
        FROM
...

[msullivan]

This is very solid looking, and it does a great job of handling pretty nasty requirements nicely.

I've got a hopefully final round of formatting nitpicks, though I'm also open to arguments about them :P

[msullivan]

I think I'd prefer to drop most of the extra helpful messages here, and just say access policy violation and then, if applicable, following it, the relevant specified error messages.

My reasoning here is that the particular error might be sensitive information, and we don't want to give out any more than what the user asked to give. I'm willing to be convinced that this is not necessary, though.

I think it would be best to be able to produce error messages from both allow and deny rules at the same time, too?

The good news is I think all of these nitpicks should simplify the code, not make it more complex :P

[aljazerzen]

I agree, given that this is security it should not be verbose when denying access.

[msullivan]

Once the errmessage stuff is there, we'll need to handle the case where it errors but nothing with an error message failed

[msullivan]

Looks great!

[msullivan]

?

[msullivan]

nit: Probably better to keep this expanded

[msullivan]

As reference for later documentation (in #4943), I believe the algorithm is:

• Collect all "relevant" policy errmessage: this means every deny rule with an errmessage that was true, and every allow rule with an errmessage that was false
• (Note that this means that either every allow rule with an errmessage will be included or none of them will)
• If at least one message was collected, stick them all comma separated in the error message
• Otherwise return a fully generic error message that does not indicate which rules failed

The rationale for this system is to avoid leaking any information that the developer did not opt-in to leak, in case the error messages are directly exposed to an end-user. Why a security policy failed is often sensitive information, so it needs to only be opt-in, and so we never do anything like just directly reporting policy names.