Repository: geldata/gel
Pull request: Doc: access policy fix and expansion
Fixes https://github.com/edgedb/edgedb/issues/6389 where a user noticed that the syntax declaration for access policies makes it look like annotations should be outside the braces when they should be inside, and that we don't have any access policy examples that show where annotations belong either so it took some experimentation to figure it out.

In our current example there is a bit of new learning to do so instead of declaring a new annotation at the same time, it uses the built-in `errmessage` to show where to add such notes and then mentions in the breakdown below that other annotations can go in this space as well. Plus adds an example where you might see this error: where an app is configured wrongly such that a user is allowed to *try* to write a post but is prevented by the access policy.

Problem statement:
Document Access Policy Annotations
The docs are misleading right now about where annotations can be declared with access policies in the schema.
It took me awhile to figure out they are supposed to be inside the `{}` & a sibling of the `errmessage` set clause.
There were no examples of this, so I was just using the synopsis block.
```diff
 access policy name
   [ when (condition) ]
   { allow | deny } action [, action ... ]
   [ using (expr) ]
-  [ annotation-declarations ]
   [ "{"
      [ errmessage := value ; ]
+     [ annotation-declarations ]
     "}" ] ;
```
```diff
 [ with with-item [, ...] ]
 { create | alter } type TypeName "{"
   [ ... ]
   create access policy name
     [ when (condition) ; ]
     { allow | deny } action [, action ... ; ]
     [ using (expr) ; ]
-     [ create annotation annotation-name := value ; ]
     [ "{"
        [ set errmessage := value ; ]
+       [ create annotation annotation-name := value ; ]
       "}" ; ]
 "}"
```

Hints from issue discussion:
Good catch! I'll put a PR together fixing this along with adding an annotation to the example on the access policy page.

Commit message: Add line

Patch to review:
diff --git a/docs/datamodel/access_policies.rst b/docs/datamodel/access_policies.rst
index 0ffb8689154..f36681a42f3 100644
--- a/docs/datamodel/access_policies.rst
+++ b/docs/datamodel/access_policies.rst
@@ -188,7 +188,9 @@ Let's add a policy to our sample schema.
 
     +     access policy author_has_full_access
     +       allow all
-    +       using (global current_user ?= .author.id);
+    +       using (global current_user ?= .author.id) {
+    +          errmessage := "User must have same ID as blog author";
+    +       }
         }
 
 .. code-block:: sdl-diff
@@ -205,7 +207,9 @@ Let's add a policy to our sample schema.
 
     +     access policy author_has_full_access
     +       allow all
-    +       using (global current_user ?= .author.id);
+    +       using (global current_user ?= .author.id) {
+    +          errmessage := "User must have same ID as blog author";
+    +       }
         }
 
 
@@ -227,6 +231,10 @@ one else will be able to edit, delete, or view this post.
   ``update read``, ``update write``.
 - ``using (<expr>)``: A boolean expression. Think of this as a ``filter``
   expression that defines the set of objects to which the policy applies.
+- ``errmessage``: Here we have added an error message that will show in case
+  the policy expression returns ``false``. We could have added other
+  annotations of our own inside this code block instead of, or in addition
+  to ``errmessage``.
 
 Let's do some experiments.
 
@@ -253,7 +261,36 @@ we'll see the post we just created.
   db> select count(BlogPost);
   {1}
 
-Now let's unset ``current_user`` and see what happens.
+Next, let's set the ``global current_user`` to some other id. This user has
+yet to write any posts, so the number of ``BlogPost`` objects returned via
+the ``count`` function is now zero:
+
+.. code-block:: edgeql-repl
+
+  db> set global current_user := <uuid>'d1c64b84-8e3c-11ee-86f0-d7ddecf3e9bd';
+  OK: SET GLOBAL
+  db> select count(BlogPost);
+  {0}
+
+Now let's see what happens when we try to insert a ``BlogPost`` under this new
+id. This might represent a situation where our app is misconfigured to allow a
+user to switch accounts while still remaining on the blog editing page used
+for the previous account. Fortunately, the access policy prevents this post
+from being published under the wrong id:
+
+.. code-block:: edgeql-repl
+
+  db> set global current_user := <uuid>'d1c64b84-8e3c-11ee-86f0-d7ddecf3e9bd';
+  OK: SET GLOBAL
+  db> insert BlogPost {
+  ...    title := "My post",
+  ...    author := (select User filter .id = global current_user)
+  ...  };
+  edgedb error: AccessPolicyError: access policy violation on insert of
+  default::BlogPost (User must have same ID as blog author)
+
+Finally, let's unset ``current_user`` and see how many blog posts are returned
+when we count them.
 
 .. code-block:: edgeql-repl
 
@@ -264,9 +301,10 @@ Now let's unset ``current_user`` and see what happens.
   db> select count(BlogPost);
   {0}
 
-Now ``select BlogPost`` returns zero results. We can only ``select`` the
-*posts* written by the *user* specified by ``current_user``. When
-``current_user`` has no value, we can't read any posts.
+``select BlogPost`` returns zero results in this case as well. We can only
+``select`` the *posts* written by the *user* specified by ``current_user``.
+When ``current_user`` has no value, or a different value from existing
+``BlogPost`` objects, we can't read any posts.
 
 The access policies use global variables to define a "subgraph" of data that
 is visible to a particular query.
diff --git a/docs/reference/ddl/access_policies.rst b/docs/reference/ddl/access_policies.rst
index f53bcff8ed7..1f06651847b 100644
--- a/docs/reference/ddl/access_policies.rst
+++ b/docs/reference/ddl/access_policies.rst
@@ -25,7 +25,10 @@ Create access policy
         [ when (<condition>) ; ]
         { allow | deny } <action> [, <action> ... ; ]
         [ using (<expr>) ; ]
-        [ create annotation <annotation-name> := <value> ; ]
+        [ "{"
+           [ set errmessage := value ; ]
+           [ create annotation annotation-name := value ; ]
+          "}" ]
     "}"
 
     # where <action> is one of
@@ -37,17 +40,17 @@ Create access policy
 
 .. eql:synopsis::
 
-    [ with with-item [, ...] ]
-    { create | alter } type TypeName "{"
+    [ with <with-item> [, ...] ]
+    { create | alter } type <TypeName> "{"
       [ ... ]
-      create access policy name
-        [ when (condition) ; ]
+      create access policy <name>
+        [ when (<condition>) ; ]
         { allow | deny } action [, action ... ; ]
-        [ using (expr) ; ]
-        [ create annotation annotation-name := value ; ]
+        [ using (<expr>) ; ]
         [ "{"
            [ set errmessage := value ; ]
-          "}" ; ]
+           [ create annotation annotation-name := value ; ]
+          "}" ]
     "}"
 
     # where <action> is one of
diff --git a/docs/reference/sdl/access_policies.rst b/docs/reference/sdl/access_policies.rst
index 4b9c1675866..09662200561 100644
--- a/docs/reference/sdl/access_policies.rst
+++ b/docs/reference/sdl/access_policies.rst
@@ -77,7 +77,10 @@ commands <ref_eql_ddl_access_policies>`.
       [ when (<condition>) ]
       { allow | deny } <action> [, <action> ... ]
       [ using (<expr>) ]
-      [ <annotation-declarations> ] ;
+      [ "{"
+         [ errmessage := value ; ]
+         [ <annotation-declarations> ]
+        "}" ] ;
 
     # where <action> is one of
     all
@@ -93,9 +96,9 @@ commands <ref_eql_ddl_access_policies>`.
       [ when (<condition>) ]
       { allow | deny } <action> [, <action> ... ]
       [ using (<expr>) ]
-      [ <annotation-declarations> ]
       [ "{"
          [ errmessage := value ; ]
+         [ <annotation-declarations> ]
         "}" ] ;
 
     # where <action> is one of


Code review comments:
diff_hunk: @@ -253,7 +261,36 @@ we'll see the post we just created.
   db> select count(BlogPost);
   {1}
 
-Now let's unset ``current_user`` and see what happens.
+Next, let's set the ``global current_user`` to some other id. This user has
+yet to write any posts, so the number of ``BlogPost`` objects returned via
+the ``count`` function is now zero:
+
+.. code-block:: edgeql-repl
+
+  db> set global current_user := <uuid>'d1c64b84-8e3c-11ee-86f0-d7ddecf3e9bd';
+  OK: SET GLOBAL
+  db> select count(BlogPost);
+  {0}
+
+Now let's see what happens when we try to insert a ``BlogPost`` under this new
+id. This might represent a situation where our app is misconfigured to allow a
+user to switch accounts while still remaining on the blog editing page used
+for the previous account. Fortunately, the access policy prevents this post
+from being published under the wrong id:
+
+.. code-block:: edgeql-repl
+
+  db> set global current_user := <uuid>'d1c64b84-8e3c-11ee-86f0-d7ddecf3e9bd';
+  OK: SET GLOBAL
+  db> insert BlogPost {
+  ...    title := "My post",
+  ...    author := (select User filter .id = global current_user)
+  ...  };
+  edgedb error: AccessPolicyError: access policy violation on insert of
+  default::BlogPost (User must have same ID as blog author)
path: docs/datamodel/access_policies.rst
text: @user1:
I'm confused how this would be prevented by the access policy. This error message would only trigger if `current_user` is different from `.author.id`, but the `User` for `.author` is being selected here based on `current_user`, so they would have to match, right?

@author:
Yeah, not a good example as we discussed last week. A better idea struck me which requires two globals but still simple enough and feels much more like a real world example.

diff_hunk: @@ -253,7 +261,36 @@ we'll see the post we just created.
   db> select count(BlogPost);
   {1}
 
-Now let's unset ``current_user`` and see what happens.
+Next, let's set the ``global current_user`` to some other id. This user has
+yet to write any posts, so the number of ``BlogPost`` objects returned via
+the ``count`` function is now zero:
+
+.. code-block:: edgeql-repl
+
+  db> set global current_user := <uuid>'d1c64b84-8e3c-11ee-86f0-d7ddecf3e9bd';
+  OK: SET GLOBAL
+  db> select count(BlogPost);
+  {0}
+
+Now let's see what happens when we try to insert a ``BlogPost`` under this new
+id. This might represent a situation where our app is misconfigured to allow a
+user to switch accounts while still remaining on the blog editing page used
+for the previous account. Fortunately, the access policy prevents this post
+from being published under the wrong id:
+
+.. code-block:: edgeql-repl
+
+  db> set global current_user := <uuid>'d1c64b84-8e3c-11ee-86f0-d7ddecf3e9bd';
path: docs/datamodel/access_policies.rst
text: @user1:
```suggestion
  db> set global current_user :=
  ...   <uuid>'d1c64b84-8e3c-11ee-86f0-d7ddecf3e9bd';
```