From 480800edbd7376b3e688435121953ddb976c98f4 Mon Sep 17 00:00:00 2001 From: Joe Nelson Date: Tue, 25 Aug 2015 00:22:23 -0700 Subject: [PATCH 1/4] Problem after exceptions when authed Reproduces #264 --- test/Feature/AuthSpec.hs | 17 ++++++++++++----- test/fixtures/schema.sql | 13 +++++++++++++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/test/Feature/AuthSpec.hs b/test/Feature/AuthSpec.hs index ae94f4f8f..915526f06 100644 --- a/test/Feature/AuthSpec.hs +++ b/test/Feature/AuthSpec.hs @@ -28,19 +28,26 @@ spec = beforeAll let auth = authHeaderBasic "jdoe" "1234" request methodGet "/authors_only" [auth] "" `shouldRespondWith` 200 - + + it "recovers after 400 error with logged in user" $ do + _ <- post "/postgrest/users" [json| { "id":"jdoe", "pass": "1234", "role": "postgrest_test_author" } |] + let auth = authHeaderBasic "jdoe" "1234" + _ <- request methodPost "/rpc/problem" [auth] "" + request methodGet "/authors_only" [auth] "" + `shouldRespondWith` 200 + it "allows users to login (JWT)" $ do _ <- post "/postgrest/users" [json| { "id":"jdoe", "pass": "1234", "role": "postgrest_test_author" } |] - post "/postgrest/tokens" [json| { "id":"jdoe", "pass": "1234" } |] + post "/postgrest/tokens" [json| { "id":"jdoe", "pass": "1234" } |] `shouldRespondWith` ResponseMatcher { matchBody = Just [json| {"token":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoicG9zdGdyZXN0X3Rlc3RfYXV0aG9yIiwiaWQiOiJqZG9lIn0.y4vZuu1dDdwAl0-S00MCRWRYMlJ5YAMSir6Es6WtWx0"} |] , matchStatus = 201 , matchHeaders = ["Content-Type" <:> "application/json"] } - + it "indicates login failure (JWT)" $ do _ <- post "/postgrest/users" [json| { "id":"jdoe", "pass": "1234", "role": "postgrest_test_author" } |] - post "/postgrest/tokens" [json| { "id":"jdoe", "pass": "NOPE" } |] + post "/postgrest/tokens" [json| { "id":"jdoe", "pass": "NOPE" } |] `shouldRespondWith` ResponseMatcher { matchBody = Just [json| {"message":"Failed authentication."} |] , matchStatus = 401 @@ -51,4 +58,4 @@ spec = beforeAll _ <- post "/postgrest/users" [json| { "id":"jdoe", "pass": "1234", "role": "postgrest_test_author" } |] let auth = authHeaderJWT "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoicG9zdGdyZXN0X3Rlc3RfYXV0aG9yIiwiaWQiOiJqZG9lIn0.y4vZuu1dDdwAl0-S00MCRWRYMlJ5YAMSir6Es6WtWx0" request methodGet "/authors_only" [auth] "" - `shouldRespondWith` 200 \ No newline at end of file + `shouldRespondWith` 200 diff --git a/test/fixtures/schema.sql b/test/fixtures/schema.sql index 3d705f9ef..6796d2167 100755 --- a/test/fixtures/schema.sql +++ b/test/fixtures/schema.sql @@ -228,6 +228,14 @@ CREATE FUNCTION "1".sayhello(name text) RETURNS text AS $$ $$ LANGUAGE SQL; +CREATE FUNCTION "1".problem() RETURNS void LANGUAGE plpgsql AS +$$ +BEGIN + RAISE 'bad thing'; +END; +$$; + + CREATE TABLE menagerie ( "integer" integer NOT NULL, double double precision NOT NULL, @@ -544,6 +552,11 @@ GRANT EXECUTE ON FUNCTION sayhello(text) TO postgrest_test; GRANT EXECUTE ON FUNCTION sayhello(text) TO postgrest_anonymous; +REVOKE ALL ON FUNCTION problem() FROM PUBLIC; +REVOKE ALL ON FUNCTION problem() FROM postgrest_test_author; +GRANT EXECUTE ON FUNCTION problem() TO postgrest_test_author; + + REVOKE ALL ON SEQUENCE items_id_seq FROM PUBLIC; REVOKE ALL ON SEQUENCE items_id_seq FROM postgrest_test; GRANT ALL ON SEQUENCE items_id_seq TO postgrest_test; From 594327924c696dce7306926bbff99e7426b003b9 Mon Sep 17 00:00:00 2001 From: Joe Nelson Date: Tue, 25 Aug 2015 23:33:44 -0700 Subject: [PATCH 2/4] Set role locally in a tx to ensure it is reset after error --- src/PostgREST/Auth.hs | 4 ++-- src/PostgREST/Main.hs | 2 +- test/SpecHelper.hs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/PostgREST/Auth.hs b/src/PostgREST/Auth.hs index 597dae421..6f42eafbb 100644 --- a/src/PostgREST/Auth.hs +++ b/src/PostgREST/Auth.hs @@ -53,14 +53,14 @@ checkPass :: Text -> Text -> Bool checkPass = (. cs) . validatePassword . cs setRole :: Text -> H.Tx P.Postgres s () -setRole role = H.unitEx $ B.Stmt ("set role " <> cs (pgFmtLit role)) V.empty True +setRole role = H.unitEx $ B.Stmt ("set local role " <> cs (pgFmtLit role)) V.empty True resetRole :: H.Tx P.Postgres s () resetRole = H.unitEx [H.stmt|reset role|] setUserId :: Text -> H.Tx P.Postgres s () setUserId uid = if uid /= "" then - H.unitEx $ B.Stmt ("set user_vars.user_id = " <> cs (pgFmtLit uid)) V.empty True + H.unitEx $ B.Stmt ("set local user_vars.user_id = " <> cs (pgFmtLit uid)) V.empty True else resetUserId diff --git a/src/PostgREST/Main.hs b/src/PostgREST/Main.hs index acae88bd6..e7a9d40ef 100644 --- a/src/PostgREST/Main.hs +++ b/src/PostgREST/Main.hs @@ -73,7 +73,7 @@ main = do runSettings appSettings $ middle $ \req respond -> do body <- strictRequestBody req - resOrError <- liftIO $ H.session pool $ H.tx Nothing $ + resOrError <- liftIO $ H.session pool $ H.tx (Just (H.ReadUncommitted, Just True)) $ authenticated conf (app conf body) req either (respond . errResponse) respond resOrError diff --git a/test/SpecHelper.hs b/test/SpecHelper.hs index a9c5df075..e24c75fb1 100644 --- a/test/SpecHelper.hs +++ b/test/SpecHelper.hs @@ -54,7 +54,7 @@ withApp perform = do perform $ middle $ \req resp -> do body <- strictRequestBody req - result <- liftIO $ H.session pool $ H.tx Nothing + result <- liftIO $ H.session pool $ H.tx (Just (H.ReadUncommitted, Just True)) $ authenticated cfg (app cfg body) req either (resp . errResponse) resp result From 1656fb9f5729288fac0040cc2c1fe9675c8ac154 Mon Sep 17 00:00:00 2001 From: Joe Nelson Date: Wed, 26 Aug 2015 19:58:57 -0700 Subject: [PATCH 3/4] Let the transaction reset the role and user id for us --- src/PostgREST/Auth.hs | 9 +++------ src/PostgREST/Main.hs | 2 +- src/PostgREST/Middleware.hs | 9 +++------ test/SpecHelper.hs | 2 +- 4 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/PostgREST/Auth.hs b/src/PostgREST/Auth.hs index 6f42eafbb..9d8f5f99c 100644 --- a/src/PostgREST/Auth.hs +++ b/src/PostgREST/Auth.hs @@ -55,9 +55,6 @@ checkPass = (. cs) . validatePassword . cs setRole :: Text -> H.Tx P.Postgres s () setRole role = H.unitEx $ B.Stmt ("set local role " <> cs (pgFmtLit role)) V.empty True -resetRole :: H.Tx P.Postgres s () -resetRole = H.unitEx [H.stmt|reset role|] - setUserId :: Text -> H.Tx P.Postgres s () setUserId uid = if uid /= "" then H.unitEx $ B.Stmt ("set local user_vars.user_id = " <> cs (pgFmtLit uid)) V.empty True @@ -90,15 +87,15 @@ signInWithJWT secret input = case maybeRole of Just (Just (String uid)) -> LoginSuccess (cs role) (cs uid) _ -> LoginFailed _ -> LoginFailed - where + where maybeRole = (Data.Map.lookup "role" <$> claims) ::Maybe (Maybe Value) maybeUserId = (Data.Map.lookup "id" <$> claims) ::Maybe (Maybe Value) claims = JWT.unregisteredClaims <$> JWT.claims <$> decoded decoded = JWT.decodeAndVerifySignature (JWT.secret secret) input - + tokenJWT :: Text -> Text -> Text -> Text tokenJWT secret uid role = JWT.encodeSigned JWT.HS256 (JWT.secret secret) claimsSet where claimsSet = JWT.def { JWT.unregisteredClaims = Data.Map.fromList [("id", String uid), ("role", String role)] - } \ No newline at end of file + } diff --git a/src/PostgREST/Main.hs b/src/PostgREST/Main.hs index e7a9d40ef..1a14c2f4d 100644 --- a/src/PostgREST/Main.hs +++ b/src/PostgREST/Main.hs @@ -73,7 +73,7 @@ main = do runSettings appSettings $ middle $ \req respond -> do body <- strictRequestBody req - resOrError <- liftIO $ H.session pool $ H.tx (Just (H.ReadUncommitted, Just True)) $ + resOrError <- liftIO $ H.session pool $ H.tx (Just (H.ReadCommitted, Just True)) $ authenticated conf (app conf body) req either (respond . errResponse) respond resOrError diff --git a/src/PostgREST/Middleware.hs b/src/PostgREST/Middleware.hs index b14bf5005..f560e8313 100644 --- a/src/PostgREST/Middleware.hs +++ b/src/PostgREST/Middleware.hs @@ -23,7 +23,7 @@ import Network.Wai.Middleware.Static (staticPolicy, only) import Network.URI (URI(..), parseURI) import PostgREST.Config (AppConfig(..), corsPolicy) -import PostgREST.Auth (LoginAttempt(..), signInRole, signInWithJWT, setRole, resetRole, setUserId, resetUserId) +import PostgREST.Auth (LoginAttempt(..), signInRole, signInWithJWT, setRole, setUserId) import PostgREST.App (contentTypeForAccept) import Codec.Binary.Base64.String (decode) @@ -62,10 +62,7 @@ authenticated conf app req = do runInRole r uid = do setUserId uid setRole r - res <- app req - resetRole - resetUserId - return res + app req redirectInsecure :: Application -> Application @@ -93,7 +90,7 @@ unsupportedAccept :: Application -> Application unsupportedAccept app req respond = do let accept = lookup hAccept $ requestHeaders req - if isNothing $ contentTypeForAccept accept + if isNothing $ contentTypeForAccept accept then respond $ responseLBS status415 [] "Unsupported Accept header, try: application/json" else app req respond diff --git a/test/SpecHelper.hs b/test/SpecHelper.hs index e24c75fb1..e5ae89db8 100644 --- a/test/SpecHelper.hs +++ b/test/SpecHelper.hs @@ -54,7 +54,7 @@ withApp perform = do perform $ middle $ \req resp -> do body <- strictRequestBody req - result <- liftIO $ H.session pool $ H.tx (Just (H.ReadUncommitted, Just True)) + result <- liftIO $ H.session pool $ H.tx (Just (H.ReadCommitted, Just True)) $ authenticated cfg (app cfg body) req either (resp . errResponse) resp result From fcdae73f495009c99d550b883483c72f74165f8b Mon Sep 17 00:00:00 2001 From: Joe Nelson Date: Wed, 26 Aug 2015 20:01:42 -0700 Subject: [PATCH 4/4] Note fix in changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f191a8c38..2c12e9cfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - Allow filters by computed columns - @diogob ### Fixed +- Reset user role on error - Compatible with Stack - Add materialized views to results in GET / - @diogob - Indicate insertable=true for views that are insertable through triggers - @diogob