Allow PATCH/DELETE w/o Prefer when no SELECT privs

PATCH/DELETE can now be done without adding Prefer return=minimal when the user doesn't have SELECT privileges. * Also fix PATCH wrong HTTP status code
2019-10-08 12:41:39 -05:00
parent 337f821e00
commit ed2bfc09a6
13 changed files with 156 additions and 126 deletions
@@ -9,15 +9,17 @@ This project adheres to [Semantic Versioning](http://semver.org/).

 - #1383, Add support for HEAD request - @steve-chavez
 - #1378, Add support for `Prefer: count=planned` and `Prefer: count=estimated` on GET /table - @steve-chavez
+
+### Fixed
+
 - #1301, Fix self join resource embedding on PATCH - @herulume, @steve-chavez
 - #1389, Fix many to many resource embedding on RPC/PATCH - @steve-chavez
+- #1355, Allow PATCH/DELETE without `return=minimal` on tables with no select privileges - @steve-chavez

 ### Changed

 - #1385, bulk RPC call now should be done by specifying a `Prefer: params=multiple-objects` header - @steve-chavez

-### Fixed
-
 ## [6.0.2] - 2019-08-22

 ### Fixed
@@ -11,7 +11,6 @@ module PostgREST.ApiRequest (
 , ContentType(..)
 , Action(..)
 , Target(..)
-, PreferRepresentation (..)
 , mutuallyAgreeable
 , userApiRequest
 ) where
@@ -67,10 +66,6 @@ data Target = TargetIdent QualifiedIdentifier
            | TargetUnknown [Text]
            deriving Eq

-- | How to return the inserted data
-data PreferRepresentation = Full | HeadersOnly | None deriving Eq
-
-
 {-|
  Describes what the user wants to do. This data type is a
  translation of the raw elements of an HTTP request into domain
@@ -79,44 +74,25 @@ data PreferRepresentation = Full | HeadersOnly | None deriving Eq
  if it is an action we are able to perform.
 -}
 data ApiRequest = ApiRequest {
-  -- | Similar but not identical to HTTP verb, e.g. Create/Invoke both POST
-    iAction               :: Action
-  -- | Requested range of rows within response
-  , iRange                :: M.HashMap ByteString NonnegRange
-  -- | Requested range of rows from the top level
-  , iTopLevelRange        :: NonnegRange
-  -- | The target, be it calling a proc or accessing a table
-  , iTarget               :: Target
-  -- | Content types the client will accept, [CTAny] if no Accept header
-  , iAccepts              :: [ContentType]
-  -- | Data sent by client and used for mutation actions
-  , iPayload              :: Maybe PayloadJSON
-  -- | If client wants created items echoed back
-  , iPreferRepresentation :: PreferRepresentation
-  -- | How to pass parameters to a stored procedure
-  , iPreferParameters     :: Maybe PreferParameters
-  -- | Whether the client wants a result count
-  , iPreferCount          :: Maybe PreferCount
-  -- | Whether the client wants to UPSERT or ignore records on PK conflict
-  , iPreferResolution     :: Maybe PreferResolution
-  -- | Filters on the result ("id", "eq.10")
-  , iFilters              :: [(Text, Text)]
-  -- | &and and &or parameters used for complex boolean logic
-  , iLogic                :: [(Text, Text)]
-  -- | &select parameter used to shape the response
-  , iSelect               :: Maybe Text
-  -- | &columns parameter used to shape the payload
-  , iColumns              :: Maybe Text
-  -- | &order parameters for each level
-  , iOrder                :: [(Text, Text)]
-  -- | Alphabetized (canonical) request query string for response URLs
-  , iCanonicalQS          :: ByteString
-  -- | JSON Web Token
-  , iJWT                  :: Text
-  -- | HTTP request headers
-  , iHeaders              :: [(Text, Text)]
-  -- | Request Cookies
-  , iCookies              :: [(Text, Text)]
+    iAction               :: Action                           -- ^ Similar but not identical to HTTP verb, e.g. Create/Invoke both POST
+  , iRange                :: M.HashMap ByteString NonnegRange -- ^ Requested range of rows within response
+  , iTopLevelRange        :: NonnegRange                      -- ^ Requested range of rows from the top level
+  , iTarget               :: Target                           -- ^ The target, be it calling a proc or accessing a table
+  , iAccepts              :: [ContentType]                    -- ^ Content types the client will accept, [CTAny] if no Accept header
+  , iPayload              :: Maybe PayloadJSON                -- ^ Data sent by client and used for mutation actions
+  , iPreferRepresentation :: PreferRepresentation             -- ^ If client wants created items echoed back
+  , iPreferParameters     :: Maybe PreferParameters           -- ^ How to pass parameters to a stored procedure
+  , iPreferCount          :: Maybe PreferCount                -- ^ Whether the client wants a result count
+  , iPreferResolution     :: Maybe PreferResolution           -- ^ Whether the client wants to UPSERT or ignore records on PK conflict
+  , iFilters              :: [(Text, Text)]                   -- ^ Filters on the result ("id", "eq.10")
+  , iLogic                :: [(Text, Text)]                   -- ^ &and and &or parameters used for complex boolean logic
+  , iSelect               :: Maybe Text                       -- ^ &select parameter used to shape the response
+  , iColumns              :: Maybe Text                       -- ^ &columns parameter used to shape the payload
+  , iOrder                :: [(Text, Text)]                   -- ^ &order parameters for each level
+  , iCanonicalQS          :: ByteString                       -- ^ Alphabetized (canonical) request query string for response URLs
+  , iJWT                  :: Text                             -- ^ JSON Web Token
+  , iHeaders              :: [(Text, Text)]                   -- ^ HTTP request headers
+  , iCookies              :: [(Text, Text)]                   -- ^ Request Cookies
  }

 -- | Examines HTTP request and translates it into user intent.
@@ -255,9 +231,11 @@ userApiRequest schema rootSpec req reqBody
        split :: BS.ByteString -> [Text]
        split = map T.strip . T.split (==',') . toS
  representation
-    | hasPrefer "return=representation" = Full
-    | hasPrefer "return=minimal" = None
-    | otherwise = HeadersOnly
+    | hasPrefer (show Full) = Full
+    | hasPrefer (show None) = None
+    | otherwise             = if action == ActionCreate
+                                then HeadersOnly -- Assume the user wants the Location header(for POST) by default
+                                else None
  auth = fromMaybe "" $ lookupHeader hAuthorization
  tokenStr = case T.split (== ' ') (toS auth) of
    ("Bearer" : t : _) -> t
@@ -41,10 +41,8 @@ import Network.Wai

 import PostgREST.ApiRequest       (Action (..), ApiRequest (..),
                                   ContentType (..),
-                                   InvokeMethod (..),
-                                   PreferRepresentation (..),
-                                   Target (..), mutuallyAgreeable,
-                                   userApiRequest)
+                                   InvokeMethod (..), Target (..),
+                                   mutuallyAgreeable, userApiRequest)
 import PostgREST.Auth             (containsRole, jwtClaims,
                                   parseSecret)
 import PostgREST.Config           (AppConfig (..))
@@ -33,8 +33,7 @@ import Control.Applicative
 import Data.Tree
 import Network.Wai

-import PostgREST.ApiRequest (Action (..), ApiRequest (..),
-                             PreferRepresentation (..))
+import PostgREST.ApiRequest (Action (..), ApiRequest (..))
 import PostgREST.Error      (ApiRequestError (..), errorResponseFor)
 import PostgREST.Parsers
 import PostgREST.RangeQuery (NonnegRange, allRange, restrictRange)
@@ -191,3 +191,9 @@ countF countQuery shouldCount =
    else (
        mempty
      , "null::bigint")
+
+returningF :: QualifiedIdentifier -> [FieldName] -> SqlFragment
+returningF qi returnings =
+  if null returnings
+    then "RETURNING 1" -- For mutation cases where there's no ?select, we return 1 to know how many rows were modified
+    else "RETURNING " <> intercalate ", " (pgFmtColumn qi <$> returnings)
@@ -96,7 +96,8 @@ mutateRequestToQuery (Insert mainQi iCols onConflct putConditions returnings) =
           then "DO NOTHING"
           else "DO UPDATE SET " <> intercalate ", " (pgFmtIdent <> const " = EXCLUDED." <> pgFmtIdent <$> S.toList iCols)
                                   ) `emptyOnFalse` null oncCols) onConflct,
-    ("RETURNING " <> intercalate ", " (map (pgFmtColumn mainQi) returnings)) `emptyOnFalse` null returnings]
+    returningF mainQi returnings
+    ]
  where
    cols = intercalate ", " $ pgFmtIdent <$> S.toList iCols
 mutateRequestToQuery (Update mainQi uCols logicForest returnings) =
@@ -108,7 +109,7 @@ mutateRequestToQuery (Update mainQi uCols logicForest returnings) =
        "UPDATE " <> fromQi mainQi <> " SET " <> cols,
        "FROM (SELECT * FROM json_populate_recordset", "(null::", fromQi mainQi, ", " <> selectBody <> ")) _ ",
        ("WHERE " <> intercalate " AND " (pgFmtLogicTree mainQi <$> logicForest)) `emptyOnFalse` null logicForest,
-        ("RETURNING " <> intercalate ", " (pgFmtColumn mainQi <$> returnings)) `emptyOnFalse` null returnings
+        returningF mainQi returnings
        ]
  where
    cols = intercalate ", " (pgFmtIdent <> const " = _." <> pgFmtIdent <$> S.toList uCols)
@@ -117,7 +118,7 @@ mutateRequestToQuery (Delete mainQi logicForest returnings) =
    "WITH " <> ignoredBody,
    "DELETE FROM ", fromQi mainQi,
    ("WHERE " <> intercalate " AND " (map (pgFmtLogicTree mainQi) logicForest)) `emptyOnFalse` null logicForest,
-    ("RETURNING " <> intercalate ", " (map (pgFmtColumn mainQi) returnings)) `emptyOnFalse` null returnings
+    returningF mainQi returnings
    ]

 requestToCallProcQuery :: QualifiedIdentifier -> [PgArg] -> Bool -> Maybe PreferParameters -> SqlQuery
@@ -22,18 +22,15 @@ import           Data.Aeson                      as JSON
 import qualified Data.Aeson.Lens                 as L
 import qualified Data.ByteString.Char8           as BS
 import           Data.Maybe
-import           Data.Text                       (intercalate,
-                                                  unwords)
+import           Data.Text                       (unwords)
 import           Data.Text.Encoding              (encodeUtf8)
 import qualified Hasql.Decoders                  as HD
 import qualified Hasql.Encoders                  as HE
 import qualified Hasql.Statement                 as H
-import           PostgREST.ApiRequest            (PreferRepresentation (..))
 import           PostgREST.Private.Common
 import           PostgREST.Private.QueryFragment
 import           PostgREST.Types
 import           Protolude                       hiding (cast,
-                                                  intercalate,
                                                  replace)
 import           Text.InterpolatedString.Perl6   (qc)

@@ -49,36 +46,27 @@ createWriteStatement :: SqlQuery -> SqlQuery -> Bool -> Bool -> Bool ->
 createWriteStatement selectQuery mutateQuery wantSingle isInsert asCsv rep pKeys =
  unicodeStatement sql (param HE.unknown) decodeStandard True
 where
-  sql = case rep of
-    None -> [qc|
-      WITH {sourceCTEName} AS ({mutateQuery})
-      SELECT '', 0, {noLocationF}, '' |]
-    HeadersOnly -> [qc|
-      WITH {sourceCTEName} AS ({mutateQuery})
-      SELECT {cols}
-      FROM (SELECT 1 FROM {sourceCTEName}) _postgrest_t |]
-    Full -> [qc|
-      WITH {sourceCTEName} AS ({mutateQuery})
-      SELECT {cols}
+  sql = [qc|
+      WITH
+      {sourceCTEName} AS ({mutateQuery})
+      SELECT
+        '' AS total_result_set,
+        pg_catalog.count(_postgrest_t) AS page_total,
+        {locF} AS header,
+        {bodyF} AS body
      FROM ({selectQuery}) _postgrest_t |]

-  cols = intercalate ", " [
-      "'' AS total_result_set", -- when updateing it does not make sense
-      "pg_catalog.count(_postgrest_t) AS page_total",
-      if isInsert
-        then unwords [
-          "CASE",
-            "WHEN pg_catalog.count(_postgrest_t) = 1 THEN",
-              "coalesce(" <> locationF pKeys <> ", " <> noLocationF <> ")",
-            "ELSE " <> noLocationF,
-          "END AS header"]
-        else noLocationF <> "AS header",
-      if rep == Full
-         then bodyF <> " AS body"
-         else "''"
-    ]
+  locF =
+    if isInsert && rep `elem` [Full, HeadersOnly]
+      then unwords [
+        "CASE WHEN pg_catalog.count(_postgrest_t) = 1",
+          "THEN coalesce(" <> locationF pKeys <> ", " <> noLocationF <> ")",
+          "ELSE " <> noLocationF,
+        "END"]
+      else noLocationF

  bodyF
+    | rep `elem` [None, HeadersOnly] = "''"
    | asCsv = asCsvF
    | wantSingle = asJsonSingleF
    | otherwise = asJsonF
@@ -70,6 +70,16 @@ instance Show PreferResolution where
  show MergeDuplicates  = "resolution=merge-duplicates"
  show IgnoreDuplicates = "resolution=ignore-duplicates"

+-- | How to return the mutated data. From https://tools.ietf.org/html/rfc7240#section-4.2
+data PreferRepresentation = Full        -- ^ Return the body plus the Location header(in case of POST).
+                          | HeadersOnly -- ^ Return the Location header(in case of POST). This needs a SELECT privilege on the pk.
+                          | None        -- ^ Return nothing from the mutated data.
+                          deriving Eq
+instance Show PreferRepresentation where
+  show Full        = "return=representation"
+  show None        = "return=minimal"
+  show HeadersOnly = mempty
+
 data PreferParameters
  = SingleObject    -- ^ Pass all parameters as a single json object to a stored procedure
  | MultipleObjects -- ^ Pass an array of json objects as params to a stored procedure
@@ -5,6 +5,7 @@ import Network.Wai (Application)
 import Network.HTTP.Types
 import Test.Hspec
 import Test.Hspec.Wai
+import Test.Hspec.Wai.JSON
 import Text.Heredoc

 import Protolude hiding (get)
@@ -62,3 +63,24 @@ spec =
    context "totally unknown route" $
      it "fails with 404" $
        request methodDelete "/foozle?id=eq.101" [] "" `shouldRespondWith` 404
+
+    context "table with limited privileges" $ do
+      it "fails deleting the row when return=representation and selecting all the columns" $
+        request methodDelete "/app_users?id=eq.1" [("Prefer", "return=representation")] mempty
+            `shouldRespondWith` 401
+
+      it "succeeds deleting the row when return=representation and selecting only the privileged columns" $
+        request methodDelete "/app_users?id=eq.1&select=id,email" [("Prefer", "return=representation")]
+          [json| { "password": "passxyz" } |]
+            `shouldRespondWith` [json|[ { "id": 1, "email": "test@123.com" } ]|]
+            { matchStatus  = 200
+            , matchHeaders = ["Content-Range" <:> "*/*"]
+            }
+
+      it "suceeds deleting the row with no explicit select when using return=minimal" $
+        request methodDelete "/app_users?id=eq.2" [("Prefer", "return=minimal")] mempty
+            `shouldRespondWith` 204
+
+      it "suceeds deleting the row with no explicit select by default" $
+        request methodDelete "/app_users?id=eq.3" [] mempty
+            `shouldRespondWith` 204
@@ -148,15 +148,6 @@ spec actualPgVersion = do
            simpleBody p `shouldBe` [json| [] |]
            simpleStatus p `shouldBe` created201

-        it "can insert in tables with no select privileges" $ do
-          p <- request methodPost "/insertonly"
-                       [("Prefer", "return=minimal")]
-                       [json| { "v":"some value" } |]
-          liftIO $ do
-            simpleBody p `shouldBe` ""
-            simpleStatus p `shouldBe` created201
-
-
        it "can post nulls" $ do
          p <- request methodPost "/no_pk"
                       [("Prefer", "return=representation")]
@@ -260,36 +251,6 @@ spec actualPgVersion = do
          , matchHeaders = []
          }

-    context "table with limited privileges" $ do
-      it "succeeds if correct select is applied" $
-        request methodPost "/limited_article_stars?select=article_id,user_id" [("Prefer", "return=representation")]
-          [json| {"article_id": 2, "user_id": 1} |] `shouldRespondWith` [str|[{"article_id":2,"user_id":1}]|]
-          { matchStatus  = 201
-          , matchHeaders = []
-          }
-      it "fails if more columns are selected" $
-        request methodPost "/limited_article_stars?select=article_id,user_id,created_at" [("Prefer", "return=representation")]
-          [json| {"article_id": 2, "user_id": 2} |] `shouldRespondWith` (
-        if actualPgVersion >= pgVersion112 then
-        [str|{"hint":null,"details":null,"code":"42501","message":"permission denied for view limited_article_stars"}|]
-           else
-        [str|{"hint":null,"details":null,"code":"42501","message":"permission denied for relation limited_article_stars"}|]
-                                                                        )
-          { matchStatus  = 401
-          , matchHeaders = []
-          }
-      it "fails if select is not specified" $
-        request methodPost "/limited_article_stars" [("Prefer", "return=representation")]
-          [json| {"article_id": 3, "user_id": 1} |] `shouldRespondWith` (
-        if actualPgVersion >= pgVersion112 then
-        [str|{"hint":null,"details":null,"code":"42501","message":"permission denied for view limited_article_stars"}|]
-           else
-        [str|{"hint":null,"details":null,"code":"42501","message":"permission denied for relation limited_article_stars"}|]
-                                                                        )
-          { matchStatus  = 401
-          , matchHeaders = []
-          }
-
    context "POST with ?columns parameter" $ do
      it "ignores json keys not included in ?columns" $ do
        request methodPost "/articles?columns=id,body" [("Prefer", "return=representation")]
@@ -664,3 +625,53 @@ spec actualPgVersion = do
        { matchStatus  = 200,
          matchHeaders = [matchContentTypeJson]
        }
+
+  context "table with limited privileges" $ do
+    it "succeeds inserting if correct select is applied" $
+      request methodPost "/limited_article_stars?select=article_id,user_id" [("Prefer", "return=representation")]
+        [json| {"article_id": 2, "user_id": 1} |] `shouldRespondWith` [str|[{"article_id":2,"user_id":1}]|]
+        { matchStatus  = 201
+        , matchHeaders = []
+        }
+
+    it "fails inserting if more columns are selected" $
+      request methodPost "/limited_article_stars?select=article_id,user_id,created_at" [("Prefer", "return=representation")]
+        [json| {"article_id": 2, "user_id": 2} |] `shouldRespondWith` (
+      if actualPgVersion >= pgVersion112 then
+      [str|{"hint":null,"details":null,"code":"42501","message":"permission denied for view limited_article_stars"}|]
+         else
+      [str|{"hint":null,"details":null,"code":"42501","message":"permission denied for relation limited_article_stars"}|]
+                                                                      )
+        { matchStatus  = 401
+        , matchHeaders = []
+        }
+
+    it "fails inserting if select is not specified" $
+      request methodPost "/limited_article_stars" [("Prefer", "return=representation")]
+        [json| {"article_id": 3, "user_id": 1} |] `shouldRespondWith` (
+      if actualPgVersion >= pgVersion112 then
+      [str|{"hint":null,"details":null,"code":"42501","message":"permission denied for view limited_article_stars"}|]
+         else
+      [str|{"hint":null,"details":null,"code":"42501","message":"permission denied for relation limited_article_stars"}|]
+                                                                      )
+        { matchStatus  = 401
+        , matchHeaders = []
+        }
+
+    it "can insert in a table with no select and return=minimal" $ do
+      p <- request methodPost "/insertonly"
+                   [("Prefer", "return=minimal")]
+                   [json| { "v":"some value" } |]
+      liftIO $ do
+        simpleBody p `shouldBe` ""
+        simpleStatus p `shouldBe` created201
+
+    it "succeeds updating row and gives a 204 when using return=minimal" $
+      request methodPatch "/app_users?id=eq.1" [("Prefer", "return=minimal")]
+        [json| { "password": "passxyz" } |]
+          `shouldRespondWith` 204
+
+    it "can update without return=minimal and no explicit select" $
+      request methodPatch "/app_users?id=eq.1" []
+        [json| { "password": "passabc" } |]
+          `shouldRespondWith` 204
@@ -507,3 +507,8 @@ INSERT INTO web_content VALUES (1, 'fezz', 0);
 INSERT INTO web_content VALUES (2, 'foo', 0);
 INSERT INTO web_content VALUES (3, 'bar', 0);
 INSERT INTO web_content VALUES (4, 'wut', 1);
+
+TRUNCATE TABLE app_users CASCADE;
+INSERT INTO app_users (id, email, "password") VALUES (1, 'test@123.com','pass');
+INSERT INTO app_users (id, email, "password") VALUES (2, 'abc@123.com','pass');
+INSERT INTO app_users (id, email, "password") VALUES (3, 'def@123.com','pass');
@@ -123,6 +123,10 @@ GRANT SELECT (article_id, user_id) ON TABLE limited_article_stars TO postgrest_t
 GRANT INSERT (article_id, user_id) ON TABLE limited_article_stars TO postgrest_test_anonymous;
 GRANT UPDATE (article_id, user_id) ON TABLE limited_article_stars TO postgrest_test_anonymous;

+GRANT SELECT(id, email) ON TABLE app_users TO postgrest_test_anonymous;
+GRANT INSERT, UPDATE    ON TABLE app_users TO postgrest_test_anonymous;
+GRANT DELETE            ON TABLE app_users TO postgrest_test_anonymous;
+
 REVOKE EXECUTE ON FUNCTION privileged_hello(text) FROM PUBLIC; -- All functions are available to every role(PUBLIC) by default
 GRANT EXECUTE ON FUNCTION privileged_hello(text) TO postgrest_test_author;

@@ -1753,3 +1753,9 @@ CREATE TABLE web_content (
 CREATE FUNCTION getallusers() RETURNS SETOF users AS $$
  SELECT * FROM test.users;
 $$ LANGUAGE sql;
+
+create table app_users (
+  id       integer    primary key,
+  email    text       unique not null,
+  password text       not null
+);