diff --git a/CHANGELOG.md b/CHANGELOG.md index e547a515a..729577d2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/src/PostgREST/ApiRequest.hs b/src/PostgREST/ApiRequest.hs index 38f607f5a..ce45e3df3 100644 --- a/src/PostgREST/ApiRequest.hs +++ b/src/PostgREST/ApiRequest.hs @@ -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 diff --git a/src/PostgREST/App.hs b/src/PostgREST/App.hs index c853b91ab..baaabe51f 100644 --- a/src/PostgREST/App.hs +++ b/src/PostgREST/App.hs @@ -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 (..)) diff --git a/src/PostgREST/DbRequestBuilder.hs b/src/PostgREST/DbRequestBuilder.hs index 1aeeb8dc5..3527cc3c3 100644 --- a/src/PostgREST/DbRequestBuilder.hs +++ b/src/PostgREST/DbRequestBuilder.hs @@ -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) diff --git a/src/PostgREST/Private/QueryFragment.hs b/src/PostgREST/Private/QueryFragment.hs index 14cede69a..9f047d892 100644 --- a/src/PostgREST/Private/QueryFragment.hs +++ b/src/PostgREST/Private/QueryFragment.hs @@ -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) diff --git a/src/PostgREST/QueryBuilder.hs b/src/PostgREST/QueryBuilder.hs index 9a6c78255..0919c4d95 100644 --- a/src/PostgREST/QueryBuilder.hs +++ b/src/PostgREST/QueryBuilder.hs @@ -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 diff --git a/src/PostgREST/Statements.hs b/src/PostgREST/Statements.hs index 45097029f..d4ee4f595 100644 --- a/src/PostgREST/Statements.hs +++ b/src/PostgREST/Statements.hs @@ -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 diff --git a/src/PostgREST/Types.hs b/src/PostgREST/Types.hs index 02e165f85..88047d6d6 100644 --- a/src/PostgREST/Types.hs +++ b/src/PostgREST/Types.hs @@ -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 diff --git a/test/Feature/DeleteSpec.hs b/test/Feature/DeleteSpec.hs index eb799db0a..43436c412 100644 --- a/test/Feature/DeleteSpec.hs +++ b/test/Feature/DeleteSpec.hs @@ -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 diff --git a/test/Feature/InsertSpec.hs b/test/Feature/InsertSpec.hs index 73c443d67..822628e88 100644 --- a/test/Feature/InsertSpec.hs +++ b/test/Feature/InsertSpec.hs @@ -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 diff --git a/test/fixtures/data.sql b/test/fixtures/data.sql index 4027f0406..ccca05a95 100644 --- a/test/fixtures/data.sql +++ b/test/fixtures/data.sql @@ -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'); diff --git a/test/fixtures/privileges.sql b/test/fixtures/privileges.sql index cfd0050bc..3aeb05f72 100644 --- a/test/fixtures/privileges.sql +++ b/test/fixtures/privileges.sql @@ -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; diff --git a/test/fixtures/schema.sql b/test/fixtures/schema.sql index d7ca13ffa..5ec6d565a 100755 --- a/test/fixtures/schema.sql +++ b/test/fixtures/schema.sql @@ -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 +);