From 16fd3a57ff2f8f2db1cc9b31b41a50f63fe373ae Mon Sep 17 00:00:00 2001 From: Ruslan Talpa Date: Mon, 19 Dec 2016 01:46:53 +0200 Subject: [PATCH] Fix #746 (#769) also fix broken text env After the last couple of commits the tests ran correctly only on a fresh db, in addition, the roles within the db were not created/dropped on each request and we need that since their privileges differ and we need to to have an absolute clean db on each execution --- CHANGELOG.md | 1 + src/PostgREST/App.hs | 7 ++-- src/PostgREST/QueryBuilder.hs | 68 ++++++++++++++++++++--------------- test/Feature/InsertSpec.hs | 22 ++++++++++++ test/Main.hs | 1 + test/SpecHelper.hs | 9 +++++ test/create_test_db | 57 ++++++----------------------- test/fixtures/database.sql | 6 ++-- test/fixtures/jwt.sql | 1 + test/fixtures/privileges.sql | 7 ++-- test/fixtures/roles.sql | 7 ++++ test/fixtures/schema.sql | 4 +++ 12 files changed, 106 insertions(+), 84 deletions(-) create mode 100644 test/fixtures/roles.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 583f6c8c4..71dc8ca9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - Allow using nulls order without explicit order direction - @steve-chavez - Fatal error on postgres unsupported version, format supported version in error message - @steve-chavez - Prevent database memory cosumption by prepared statements caches - @ruslantalpa +- Use specific columns in the RETURNING section - @ruslantalpa ### Changed - Use HTTP 400 for raise\_exception - @begriffs diff --git a/src/PostgREST/App.hs b/src/PostgREST/App.hs index 4cb58885d..f0ceee70c 100644 --- a/src/PostgREST/App.hs +++ b/src/PostgREST/App.hs @@ -62,6 +62,7 @@ import PostgREST.QueryBuilder ( callProc , createReadStatement , createWriteStatement , ResultsWithCount + , returningF ) import PostgREST.Types import PostgREST.OpenAPI @@ -262,8 +263,9 @@ app dbStructure conf apiRequest = mapSnd f (a, b) = (a, f b) readDbRequest = DbRead <$> readRequest (configMaxRows conf) (dbRelations dbStructure) (map (mapSnd pdReturnType) $ dbProcs dbStructure) apiRequest mutateDbRequest = DbMutate <$> mutateRequest apiRequest - selectQuery = requestToQuery schema False <$> readDbRequest - mutateQuery = requestToQuery schema False <$> mutateDbRequest + returningSql = returningF (iTarget apiRequest) (iPreferRepresentation apiRequest) <$> readDbRequest + selectQuery = requestToQuery schema False "" <$> readDbRequest + mutateQuery = requestToQuery schema False <$> returningSql <*> mutateDbRequest countQuery = requestToCountQuery schema <$> readDbRequest readSqlParts = (,) <$> selectQuery <*> countQuery mutateSqlParts = (,) <$> selectQuery <*> mutateQuery @@ -288,6 +290,7 @@ responseContentTypeOrError accepts action = serves contentTypesForRequest accept "None of these Content-Types are available: " <> failed Just ct -> Right ct + splitKeyValue :: BS.ByteString -> (BS.ByteString, BS.ByteString) splitKeyValue kv = (k, BS.tail v) where (k, v) = BS.break (== '=') kv diff --git a/src/PostgREST/QueryBuilder.hs b/src/PostgREST/QueryBuilder.hs index 2c9d41e94..8ce0eafa9 100644 --- a/src/PostgREST/QueryBuilder.hs +++ b/src/PostgREST/QueryBuilder.hs @@ -23,6 +23,7 @@ module PostgREST.QueryBuilder ( , pgFmtLit , requestToQuery , requestToCountQuery + , returningF , sourceCTEName , unquoted , ResultsWithCount @@ -52,8 +53,8 @@ import Data.Scientific ( FPFormat (..) , isInteger ) import Protolude hiding (from, intercalate, ord, cast) +import PostgREST.ApiRequest (PreferRepresentation (..), Target (..)) import Unsafe (unsafeHead) -import PostgREST.ApiRequest (PreferRepresentation (..)) {-| The generic query result format used by API responses. The location header is represented as a list of strings containing variable bindings like @@ -121,12 +122,12 @@ createWriteStatement _ _ mutateQuery _ None WITH {sourceCTEName} AS ({mutateQuery}) SELECT '', 0, {noLocationF}, '' |] -createWriteStatement qi _ mutateQuery isSingle HeadersOnly +createWriteStatement _ _ mutateQuery isSingle HeadersOnly pKeys _ (PayloadJSON _) = unicodeStatement sql encodeUniformObjs decodeStandardMay True where sql = [qc| - WITH {sourceCTEName} AS ({mutateQuery} RETURNING {fromQi qi}.*) + WITH {sourceCTEName} AS ({mutateQuery}) SELECT {cols} FROM (SELECT 1 FROM {sourceCTEName}) _postgrest_t |] cols = intercalate ", " [ @@ -136,12 +137,12 @@ createWriteStatement qi _ mutateQuery isSingle HeadersOnly "''" ] -createWriteStatement qi selectQuery mutateQuery isSingle Full +createWriteStatement _ selectQuery mutateQuery isSingle Full pKeys asCsv (PayloadJSON _) = unicodeStatement sql encodeUniformObjs decodeStandardMay True where sql = [qc| - WITH {sourceCTEName} AS ({mutateQuery} RETURNING {fromQi qi}.*) + WITH {sourceCTEName} AS ({mutateQuery}) SELECT {cols} FROM ({selectQuery}) _postgrest_t |] cols = intercalate ", " [ @@ -320,8 +321,8 @@ requestToCountQuery schema (DbRead (Node (Select _ _ conditions _ _, (mainTbl, _ fn Filter{value=VForeignKey _ _} = False localConditions = filter fn conditions -requestToQuery :: Schema -> Bool -> DbRequest -> SqlQuery -requestToQuery schema isParent (DbRead (Node (Select colSelects tbls conditions ord range, (nodeName, maybeRelation, _)) forest)) = +requestToQuery :: Schema -> Bool -> SqlFragment -> DbRequest -> SqlQuery +requestToQuery schema isParent _ (DbRead (Node (Select colSelects tbls conditions ord range, (nodeName, maybeRelation, _)) forest)) = query where -- TODO! the following helper functions are just to remove the "schema" part when the table is "source" which is the name @@ -358,8 +359,7 @@ requestToQuery schema isParent (DbRead (Node (Select colSelects tbls conditions <> "SELECT array_to_json(array_agg(row_to_json("<>pgFmtIdent table<>"))) " <> "FROM (" <> subquery <> ") " <> pgFmtIdent table <> "), '[]') AS " <> pgFmtIdent (fromMaybe name alias) - where subquery = requestToQuery schema False (DbRead (Node n forst)) - + where subquery = requestToQuery schema False "" (DbRead (Node n forst)) getQueryParts (Node n@(_, (name, Just r@Relation{relType=Parent,relTable=Table{tableName=table}}, alias)) forst) (j,s) = (joi:j,sel:s) where node_name = fromMaybe name alias @@ -369,31 +369,30 @@ requestToQuery schema isParent (DbRead (Node (Select colSelects tbls conditions sel = "row_to_json(" <> pgFmtIdent local_table_name <> ".*) AS " <> pgFmtIdent node_name joi = " LEFT OUTER JOIN ( " <> subquery <> " ) AS " <> pgFmtIdent local_table_name <> " ON " <> intercalate " AND " ( map (pgFmtCondition qi . replaceTableName local_table_name) (getJoinConditions r) ) - where subquery = requestToQuery schema True (DbRead (Node n forst)) + where subquery = requestToQuery schema True "" (DbRead (Node n forst)) getQueryParts (Node n@(_, (name, Just Relation{relType=Many,relTable=Table{tableName=table}}, alias)) forst) (j,s) = (j,sel:s) where sel = "COALESCE ((" <> "SELECT array_to_json(array_agg(row_to_json("<>pgFmtIdent table<>"))) " <> "FROM (" <> subquery <> ") " <> pgFmtIdent table <> "), '[]') AS " <> pgFmtIdent (fromMaybe name alias) - where subquery = requestToQuery schema False (DbRead (Node n forst)) + where subquery = requestToQuery schema False "" (DbRead (Node n forst)) --the following is just to remove the warning --getQueryParts is not total but requestToQuery is called only after addJoinConditions which ensures the only --posible relations are Child Parent Many - getQueryParts _ _ = undefined --error "undefined getQueryParts" -requestToQuery schema _ (DbMutate (Insert mainTbl (PayloadJSON rows))) = - let qi = QualifiedIdentifier schema mainTbl - cols = map pgFmtIdent $ fromMaybe [] (HM.keys <$> (rows V.!? 0)) - colsString = intercalate ", " cols - insInto = unwords [ "INSERT INTO" , fromQi qi, - if T.null colsString then "" else "(" <> colsString <> ")" - ] - vals = unwords $ if T.null colsString - then ["DEFAULT VALUES"] - else ["SELECT", colsString, "FROM json_populate_recordset(null::" , fromQi qi, ", $1)"] in - insInto <> vals - -requestToQuery schema _ (DbMutate (Update mainTbl (PayloadJSON rows) conditions)) = + getQueryParts _ _ = undefined +requestToQuery schema _ returningSql (DbMutate (Insert mainTbl (PayloadJSON rows))) = + insInto <> vals <> returningSql + where qi = QualifiedIdentifier schema mainTbl + cols = map pgFmtIdent $ fromMaybe [] (HM.keys <$> (rows V.!? 0)) + colsString = intercalate ", " cols + insInto = unwords [ "INSERT INTO" , fromQi qi, + if T.null colsString then "" else "(" <> colsString <> ")" + ] + vals = unwords $ if T.null colsString + then ["DEFAULT VALUES"] + else ["SELECT", colsString, "FROM json_populate_recordset(null::" , fromQi qi, ", $1)"] +requestToQuery schema _ returningSql (DbMutate (Update mainTbl (PayloadJSON rows) conditions)) = case rows V.!? 0 of Just obj -> let assignments = map @@ -401,20 +400,33 @@ requestToQuery schema _ (DbMutate (Update mainTbl (PayloadJSON rows) conditions) unwords [ "UPDATE ", fromQi qi, " SET " <> intercalate "," assignments <> " ", - ("WHERE " <> intercalate " AND " ( map (pgFmtCondition qi ) conditions )) `emptyOnNull` conditions + ("WHERE " <> intercalate " AND " ( map (pgFmtCondition qi ) conditions )) `emptyOnNull` conditions, + returningSql ] Nothing -> undefined where qi = QualifiedIdentifier schema mainTbl -requestToQuery schema _ (DbMutate (Delete mainTbl conditions)) = +requestToQuery schema _ returningSql (DbMutate (Delete mainTbl conditions)) = query where qi = QualifiedIdentifier schema mainTbl query = unwords [ "DELETE FROM ", fromQi qi, - ("WHERE " <> intercalate " AND " ( map (pgFmtCondition qi ) conditions )) `emptyOnNull` conditions + ("WHERE " <> intercalate " AND " ( map (pgFmtCondition qi ) conditions )) `emptyOnNull` conditions, + returningSql ] +returningF :: Target -> PreferRepresentation -> DbRequest -> SqlFragment +returningF _ None _ = "" +returningF (TargetIdent qi) _ (DbRead (Node (Select colSelects _ _ _ _, (_, _, _)) forest)) = + " RETURNING " <> + intercalate ", " ( map (pgFmtSelectItem qi) colSelects ++ map (pgFmtColumn qi . colName) fks) + where + fks = concatMap (fromMaybe [] . f) forest + f (Node (_, (_, Just Relation{relFColumns=cols, relType=Parent}, _)) _) = Just cols + f _ = Nothing +returningF _ _ _ = "" + sourceCTEName :: SqlFragment sourceCTEName = "pg_source" diff --git a/test/Feature/InsertSpec.hs b/test/Feature/InsertSpec.hs index 03522ac86..0ffa787ba 100644 --- a/test/Feature/InsertSpec.hs +++ b/test/Feature/InsertSpec.hs @@ -201,6 +201,28 @@ spec = do , matchStatus = 201 , 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` ResponseMatcher { + matchBody = Just [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` ResponseMatcher { + matchBody = Just [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` ResponseMatcher { + matchBody = Just [str|{"hint":null,"details":null,"code":"42501","message":"permission denied for relation limited_article_stars"}|] + , matchStatus = 401 + , matchHeaders = [] + } describe "CSV insert" $ do diff --git a/test/Main.hs b/test/Main.hs index ccb31fcf6..ce7f3494c 100644 --- a/test/Main.hs +++ b/test/Main.hs @@ -31,6 +31,7 @@ import Protolude main :: IO () main = do testDbConn <- getEnvVarWithDefault "POSTGREST_TEST_CONNECTION" "postgres://postgrest_test@localhost/postgrest_test" + setupDb testDbConn pool <- P.acquire (3, 10, toS testDbConn) -- ask for the OS time at most once per second diff --git a/test/SpecHelper.hs b/test/SpecHelper.hs index 2b93e7b8a..d676163bf 100644 --- a/test/SpecHelper.hs +++ b/test/SpecHelper.hs @@ -84,6 +84,15 @@ testCfgBinaryJWT testDbConn = (testCfg testDbConn) { configJwtSecret = Just secr where secretBs = B64.decodeLenient "h2CGB1FoBd51aQooCS2g+UmRgYQfTPQ6v3+9ALbaqM4=" +setupDb :: Text -> IO () +setupDb dbConn = do + loadFixture dbConn "database" + loadFixture dbConn "roles" + loadFixture dbConn "schema" + loadFixture dbConn "jwt" + loadFixture dbConn "privileges" + resetDb dbConn + resetDb :: Text -> IO () resetDb dbConn = loadFixture dbConn "data" diff --git a/test/create_test_db b/test/create_test_db index c69c702ef..7a382b74a 100755 --- a/test/create_test_db +++ b/test/create_test_db @@ -23,10 +23,10 @@ URI=$(echo $1 | cut -d'/' -f1-3) HOST_PORT=$(echo $URI | cut -d'/' -f3 | cut -d'@' -f2 ) DB=$2 # Specify the username of choice, or let the script create a random unique user by appending the database name -TEST_USER_NAME=${3:-postgrest_test_$DB} +TEST_USER_NAME=postgrest_test_authenticator # New password will get assigned only if the user does not already exist # Otherwise make sure to provide the correct password for the existing user -TEST_USER_PASS=${4:-$(cat /dev/urandom | env LC_CTYPE=C tr -dc 'a-zA-Z0-9' | fold -w 16 | head -n 1)a} +TEST_USER_PASS=$(cat /dev/urandom | env LC_CTYPE=C tr -dc 'a-zA-Z0-9' | fold -w 16 | head -n 1) PGOPTIONS='-c client_min_messages=WARNING' psql "$URI" -Xq >/dev/null -c 'select rolcreatedb from pg_authid where rolname = current_user;' 2>/dev/null if [ $? -ne 0 ]; then @@ -41,53 +41,16 @@ FROM pg_stat_activity WHERE pg_stat_activity.datname = '$DB' AND pid <> pg_backend_pid(); -drop database if exists "$DB"; - -create database "$DB" encoding = 'UTF8'; - -DO \$\$ - BEGIN - IF NOT EXISTS (SELECT * FROM pg_catalog.pg_roles WHERE rolname = '$TEST_USER_NAME') - THEN CREATE ROLE $TEST_USER_NAME WITH LOGIN NOINHERIT PASSWORD '$TEST_USER_PASS'; - END IF; - END \$\$; -DO \$\$ - BEGIN - IF NOT EXISTS (SELECT * FROM pg_catalog.pg_roles WHERE rolname = 'postgrest_test_anonymous') - THEN CREATE ROLE postgrest_test_anonymous; - END IF; - END \$\$; -DO \$\$ - BEGIN - IF NOT EXISTS (SELECT * FROM pg_catalog.pg_roles WHERE rolname = 'postgrest_test_default_role') - THEN CREATE ROLE postgrest_test_default_role; - END IF; - END \$\$; -DO \$\$ - BEGIN - IF NOT EXISTS (SELECT * FROM pg_catalog.pg_roles WHERE rolname = 'postgrest_test_author') - THEN CREATE ROLE postgrest_test_author; - END IF; - END \$\$; - -DO \$\$ - BEGIN - IF NOT EXISTS (select * from pg_roles where rolname = 'postgrest_test_anonymous' AND pg_has_role('$TEST_USER_NAME', oid, 'member')) - THEN GRANT postgrest_test_anonymous TO $TEST_USER_NAME; - END IF; - IF NOT EXISTS (select * from pg_roles where rolname = 'postgrest_test_author' AND pg_has_role('$TEST_USER_NAME', oid, 'member')) - THEN GRANT postgrest_test_author TO $TEST_USER_NAME; - END IF; - IF NOT EXISTS (select * from pg_roles where rolname = 'postgrest_test_default_role' AND pg_has_role('$TEST_USER_NAME', oid, 'member')) - THEN GRANT postgrest_test_default_role TO $TEST_USER_NAME; - END IF; -END \$\$; +DROP DATABASE IF EXISTS $DB; +DROP ROLE IF EXISTS $TEST_USER_NAME; +CREATE USER $TEST_USER_NAME WITH LOGIN NOINHERIT PASSWORD '$TEST_USER_PASS' CREATEROLE; +CREATE DATABASE $DB OWNER $TEST_USER_NAME; EOF -PGDATABASE=$DB PGOPTIONS='-c client_min_messages=WARNING' psql "$URI" --set=db=$DB -Xqf $BASEPATH/fixtures/database.sql -PGDATABASE=$DB PGOPTIONS='-c client_min_messages=WARNING' psql "$URI" -Xqf $BASEPATH/fixtures/schema.sql -PGDATABASE=$DB PGOPTIONS='-c client_min_messages=WARNING' psql "$URI" -Xqf $BASEPATH/fixtures/jwt.sql -PGDATABASE=$DB PGOPTIONS='-c client_min_messages=WARNING' psql "$URI" --set=test_user_name="$TEST_USER_NAME" -Xqf $BASEPATH/fixtures/privileges.sql +PGDATABASE=$DB PGOPTIONS='-c client_min_messages=WARNING' psql "$URI" --set=db=$DB -Xq <