enforce column presets of update permission with upserts (fix #1647) (#1653)

server/src-lib/Hasura/GraphQL/Resolve/Context.hs

@@ -7,7 +7,7 @@ module Hasura.GraphQL.Resolve.Context
   , OrdByItem(..)
   , FuncArgSeq
   , PGColArgMap
-  , UpdPermForIns
+  , UpdPermForIns(..)
   , InsCtx(..)
   , InsCtxMap
   , RespTx

server/src-lib/Hasura/GraphQL/Resolve/ContextTypes.hs

@@ -35,7 +35,12 @@ type FuncArgSeq = Seq.Seq FuncArgItem
 -- insert context
 type RelationInfoMap = Map.HashMap RelName RelInfo
 
-type UpdPermForIns = ([PGCol], AnnBoolExpSQL)
+data UpdPermForIns
+  = UpdPermForIns
+  { upfiCols   :: ![PGCol]
+  , upfiFilter :: !AnnBoolExpSQL
+  , upfiSet    :: !PreSetCols
+  } deriving (Show, Eq)
 
 data InsCtx
   = InsCtx

server/src-lib/Hasura/GraphQL/Resolve/Insert.hs

@@ -143,16 +143,17 @@ parseOnConflict
   :: (MonadError QErr m)
   => QualifiedTable -> Maybe UpdPermForIns
   -> AnnGValue -> m RI.ConflictClauseP1
-parseOnConflict tn updFiltrM val = withPathK "on_conflict" $
+parseOnConflict tn updPermM val = withPathK "on_conflict" $
   flip withObject val $ \_ obj -> do
     constraint <- RI.Constraint <$> parseConstraint obj
     updCols <- getUpdCols obj
     case updCols of
       [] -> return $ RI.CP1DoNothing $ Just constraint
       _  -> do
-          (_, updFiltr) <- onNothing updFiltrM $ throw500
+          UpdPermForIns _ updFiltr preSet <- onNothing updPermM $ throw500
             "cannot update columns since update permission is not defined"
-          return $ RI.CP1Update constraint updCols $ toSQLBoolExp (S.mkQual tn) updFiltr
+          return $ RI.CP1Update constraint updCols preSet $
+            toSQLBoolExp (S.mkQual tn) updFiltr
 
   where
     getUpdCols o = do
@@ -167,7 +168,7 @@ parseOnConflict tn updFiltrM val = withPathK "on_conflict" $
       return $ ConstraintName $ G.unName $ G.unEnumValue enumVal
 
 toSQLExps :: (MonadError QErr m, MonadState PrepArgs m)
-     => [(PGCol, AnnGValue)] -> m [(PGCol, S.SQLExp)]
+          => [(PGCol, AnnGValue)] -> m [(PGCol, S.SQLExp)]
 toSQLExps cols =
   forM cols $ \(c, v) -> do
     prepExpM <- asPGColValM v >>= mapM prepare

server/src-lib/Hasura/GraphQL/Schema.hs

@@ -1522,9 +1522,9 @@ mkInsCtx role tableCache fields insPermInfo updPermM = do
     rels = getValidRels fields
     iView = ipiView insPermInfo
     setCols = ipiSet insPermInfo
-    mkUpdPermForIns upi =
-      (Set.toList $ upiCols upi, upiFilter upi)
     updPermForIns = mkUpdPermForIns <$> updPermM
+    mkUpdPermForIns upi = UpdPermForIns (toList $ upiCols upi)
+                          (upiFilter upi) (upiSet upi)
 
     isInsertable Nothing _          = False
     isInsertable (Just _) viewInfoM = isMutable viIsInsertable viewInfoM
@@ -1542,7 +1542,7 @@ mkAdminInsCtx tn tc fields = do
       isMutable viIsInsertable viewInfoM
 
   let relInfoMap = Map.fromList $ catMaybes relTupsM
-      updPerm = (map pgiName cols, noFilter)
+      updPerm = UpdPermForIns (map pgiName cols) noFilter Map.empty
 
   return $ InsCtx tn cols Map.empty relInfoMap $ Just updPerm
   where

server/src-lib/Hasura/RQL/DML/Insert.hs

@@ -27,7 +27,7 @@ data ConflictTarget
 
 data ConflictClauseP1
   = CP1DoNothing !(Maybe ConflictTarget)
-  | CP1Update !ConflictTarget ![PGCol] !S.BoolExp
+  | CP1Update !ConflictTarget ![PGCol] !PreSetCols !S.BoolExp
   deriving (Show, Eq)
 
 data InsertQueryP1
@@ -51,8 +51,8 @@ toSQLConflict :: ConflictClauseP1 -> S.SQLConflict
 toSQLConflict conflict = case conflict of
   CP1DoNothing Nothing          -> S.DoNothing Nothing
   CP1DoNothing (Just ct)        -> S.DoNothing $ Just $ toSQLCT ct
-  CP1Update ct inpCols filtr    -> S.Update (toSQLCT ct)
-    (S.buildSEWithExcluded inpCols) $ Just $ S.WhereFrag filtr
+  CP1Update ct inpCols preSet filtr    -> S.Update (toSQLCT ct)
+    (S.buildUpsertSetExp inpCols preSet) $ Just $ S.WhereFrag filtr
 
   where
     toSQLCT ct = case ct of
@@ -125,13 +125,13 @@ buildConflictClause tableInfo inpCols (OnConflict mTCol mTCons act) =
       "Expecting 'constraint' or 'constraint_on' when the 'action' is 'update'"
     (Just col, Nothing, CAUpdate)   -> do
       validateCols col
-      updFiltr <- getUpdFilter
-      return $ CP1Update (Column $ getPGCols col) inpCols $
+      (updFiltr, preSet) <- getUpdPerm
+      return $ CP1Update (Column $ getPGCols col) inpCols preSet $
         toSQLBool updFiltr
     (Nothing, Just cons, CAUpdate)  -> do
       validateConstraint cons
-      updFiltr <- getUpdFilter
-      return $ CP1Update (Constraint cons) inpCols $
+      (updFiltr, preSet) <- getUpdPerm
+      return $ CP1Update (Constraint cons) inpCols preSet $
         toSQLBool updFiltr
     (Just _, Just _, _)             -> throw400 UnexpectedPayload
       "'constraint' and 'constraint_on' cannot be set at a time"
@@ -152,12 +152,13 @@ buildConflictClause tableInfo inpCols (OnConflict mTCol mTCons act) =
                    <<> " for table " <> tiName tableInfo
                    <<> " does not exist"
 
-    getUpdFilter = do
+    getUpdPerm = do
       upi <- askUpdPermInfo tableInfo
       let updFiltr = upiFilter upi
+          preSet = upiSet upi
           updCols = HS.toList $ upiCols upi
       validateInpCols inpCols updCols
-      return updFiltr
+      return (updFiltr, preSet)
 
 
 convInsertQuery
@@ -242,7 +243,7 @@ insertP2 (u, p) =
     insertSQL = toSQL $ mkSQLInsert u
 
 data ConflictCtx
-  = CCUpdate !ConstraintName ![PGCol] !S.BoolExp
+  = CCUpdate !ConstraintName ![PGCol] !PreSetCols !S.BoolExp
   | CCDoNothing !(Maybe ConstraintName)
   deriving (Show, Eq)
 
@@ -261,9 +262,9 @@ extractConflictCtx cp =
     (CP1DoNothing mConflictTar) -> do
       mConstraintName <- mapM extractConstraintName mConflictTar
       return $ CCDoNothing mConstraintName
-    (CP1Update conflictTar inpCols filtr) -> do
+    (CP1Update conflictTar inpCols preSet filtr) -> do
       constraintName <- extractConstraintName conflictTar
-      return $ CCUpdate constraintName inpCols filtr
+      return $ CCUpdate constraintName inpCols preSet filtr
   where
     extractConstraintName (Constraint cn) = return cn
     extractConstraintName _ = throw400 NotSupported
@@ -281,9 +282,9 @@ setConflictCtx conflictCtxM = do
 
     conflictCtxToJSON (CCDoNothing constrM) =
         encToText $ InsertTxConflictCtx CAIgnore constrM Nothing
-    conflictCtxToJSON (CCUpdate constr updCols filtr) =
+    conflictCtxToJSON (CCUpdate constr updCols preSet filtr) =
         encToText $ InsertTxConflictCtx CAUpdate (Just constr) $
-        Just $ toSQLTxt (S.buildSEWithExcluded updCols)
+        Just $ toSQLTxt (S.buildUpsertSetExp updCols preSet)
                <> " " <> toSQLTxt (S.WhereFrag filtr)
 
 runInsert

server/src-lib/Hasura/SQL/DML.hs

@@ -604,9 +604,17 @@ buildSEI :: PGCol -> Int -> SetExpItem
 buildSEI colName argNumber =
   SetExpItem (colName, SEPrep argNumber)
 
-buildSEWithExcluded :: [PGCol] -> SetExp
-buildSEWithExcluded cols = SetExp $ flip map cols $
-  \col -> SetExpItem (col, SEExcluded $ getPGColTxt col)
+buildUpsertSetExp
+  :: [PGCol]
+  -> HM.HashMap PGCol SQLExp
+  -> SetExp
+buildUpsertSetExp cols preSet =
+  SetExp $ map SetExpItem $ HM.toList setExps
+  where
+    setExps = HM.union preSet $ HM.fromList $
+      flip map cols $ \col ->
+        (col, SEExcluded $ getPGColTxt col)
+
 
 newtype UsingExp = UsingExp [TableName]
                   deriving (Show, Eq)

server/tests-py/queries/graphql_mutation/insert/permissions/blog_on_conflict_update_preset.yaml

@@ -0,0 +1,40 @@
+description: Update blog written by Author 1
+url: /v1alpha1/graphql
+status: 200
+headers:
+  X-Hasura-Role: user
+  X-Hasura-User-Id: '2'
+response:
+  data:
+    insert_blog:
+      affected_rows: 1
+      returning:
+      - id: 1
+        title: first blog
+        content: This content modified by Author 2
+        author_id: 1
+        updated_by: 2
+query:
+  query: |
+    mutation UpsertSet {
+      insert_blog(
+        objects: [{
+          id: 1
+          title: "first blog"
+          content: "This content modified by Author 2"
+        }]
+        on_conflict: {
+          constraint: blog_pkey
+          update_columns: [content]
+        }
+      ){
+        affected_rows
+        returning{
+          id
+          title
+          content
+          author_id
+          updated_by
+        }
+      }
+    }

server/tests-py/queries/graphql_mutation/insert/permissions/setup.yaml

@@ -318,3 +318,55 @@ args:
     - id: 6
       name: Resident 6
       age: 22
+
+#Create blog table
+- type: run_sql
+  args:
+    sql: |
+      CREATE TABLE blog (
+        id serial primary key,
+        title text not null,
+        content text,
+        author_id INTEGER REFERENCES author(id),
+        last_updated timestamptz,
+        updated_by INTEGER REFERENCES author(id)
+      );
+      INSERT INTO blog (id, title, author_id) VALUES
+        (1, 'first blog', 1), (2, 'second blog', 2);
+
+- type: track_table
+  args:
+    name: blog
+    schema: public
+    
+- type: create_select_permission
+  args:
+    table: blog
+    role: user
+    permission:
+      columns: '*'
+      filter:
+        author_id: X-Hasura-User-Id
+
+- type: create_insert_permission
+  args:
+    table: blog
+    role: user
+    permission:
+      check: {}
+
+- type: create_update_permission
+  args:
+    table: blog
+    role: user
+    permission:
+      columns:
+      - title
+      - content
+      filter: {}
+      set:
+        last_updated: 'NOW()'
+        updated_by: X-Hasura-User-Id
+
+
+

server/tests-py/queries/graphql_mutation/insert/permissions/teardown.yaml

@@ -11,24 +11,10 @@ args:
 - type: run_sql
   args:
     sql: |
-      drop table address
-
-- type: run_sql
-  args:
-    sql: |
-      drop table resident
-
-- type: run_sql
-  args:
-    sql: |
-      drop table article
-
-- type: run_sql
-  args:
-    sql: |
-      drop table author
-
-- type: run_sql
-  args:
-    sql: |
-      drop table "Company"
+      drop table address;
+      drop table resident;
+      drop table article;
+      drop table blog;
+      drop table author;
+      drop table "Company";
+    cascade: true

server/tests-py/test_graphql_mutations.py

@@ -128,6 +128,9 @@ class TestGraphqlInsertPermission(DefaultTestQueries):
     def test_resident_5_modifies_resident_6_upsert(self, hge_ctx):
         check_query_f(hge_ctx, self.dir() + "/resident_5_modifies_resident_6_upsert.yaml")
 
+    def test_blog_on_conflict_update_preset(self, hge_ctx):
+        check_query_f(hge_ctx, self.dir() + "/blog_on_conflict_update_preset.yaml")
+
     @classmethod
     def dir(cls):
         return "queries/graphql_mutation/insert/permissions"