From 18e3c30ad897244782622990983be21271b7e4a1 Mon Sep 17 00:00:00 2001 From: Joe Nelson Date: Sun, 15 May 2016 00:37:36 -0700 Subject: [PATCH] Return proper 401/403 when access denied Fixes #584 --- CHANGELOG.md | 1 + src/PostgREST/App.hs | 8 +++++--- src/PostgREST/Auth.hs | 8 ++++++++ src/PostgREST/Error.hs | 25 +++++++++++++++---------- src/PostgREST/Middleware.hs | 27 ++++++++++++--------------- test/Feature/AuthSpec.hs | 25 +++++++++++++++++++++++-- test/Feature/StructureSpec.hs | 1 + test/fixtures/privileges.sql | 1 + test/fixtures/schema.sql | 7 +++++++ 9 files changed, 73 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e2a0e972..bc57825d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - Support column/node renaming `alias:column` - @ruslantalpa ### Fixed +- Return 401 or 403 for access denied rather than 404 - @begriffs - Omit Content-Type header for empty body - @begriffs - Prevent role from being changed twice - @begriffs - Use read-only transaction for read requests - @ruslantalpa diff --git a/src/PostgREST/App.hs b/src/PostgREST/App.hs index b3355e715..9cd1138ed 100644 --- a/src/PostgREST/App.hs +++ b/src/PostgREST/App.hs @@ -38,7 +38,7 @@ import PostgREST.ApiRequest (ApiRequest(..), ContentType(..) , Action(..), Target(..) , PreferRepresentation (..) , userApiRequest) -import PostgREST.Auth (tokenJWT) +import PostgREST.Auth (tokenJWT, jwtClaims, containsRole) import PostgREST.Config (AppConfig (..)) import PostgREST.DbStructure import PostgREST.Error (errResponse, pgErrResponse) @@ -71,10 +71,12 @@ postgrest conf refDbStructure pool = let schema = cs $ configSchema conf apiRequest = userApiRequest schema req body - handleReq = runWithClaims conf time (app dbStructure conf) apiRequest + eClaims = jwtClaims (configJwtSecret conf) (iJWT apiRequest) time + authed = containsRole eClaims + handleReq = runWithClaims conf eClaims (app dbStructure conf) apiRequest txMode = transactionMode $ iAction apiRequest - resp <- either pgErrResponse id <$> P.use pool + resp <- either (pgErrResponse authed) id <$> P.use pool (HT.run handleReq HT.ReadCommitted txMode) respond resp diff --git a/src/PostgREST/Auth.hs b/src/PostgREST/Auth.hs index 3d5918350..d422b4a7d 100644 --- a/src/PostgREST/Auth.hs +++ b/src/PostgREST/Auth.hs @@ -13,6 +13,7 @@ very simple authentication system inside the PostgreSQL database. -} module PostgREST.Auth ( claimsToSQL + , containsRole , jwtClaims , tokenJWT ) where @@ -80,3 +81,10 @@ tokenJWT secret (Array arr) = jcs = parseMaybe parseJSON obj :: Maybe JWT.JWTClaimsSet in JWT.encodeSigned JWT.HS256 secret $ fromMaybe JWT.def jcs tokenJWT secret _ = tokenJWT secret emptyArray + +{-| + Whether a response from jwtClaims contains a role claim +-} +containsRole :: Either Text (M.HashMap Text Value) -> Bool +containsRole (Left _) = False +containsRole (Right claims) = M.member "role" claims diff --git a/src/PostgREST/Error.hs b/src/PostgREST/Error.hs index c29e9b068..d4bbecc85 100644 --- a/src/PostgREST/Error.hs +++ b/src/PostgREST/Error.hs @@ -21,9 +21,15 @@ import Network.Wai (Response, responseLBS) errResponse :: HT.Status -> Text -> Response errResponse status message = responseLBS status [(hContentType, "application/json")] (cs $ T.concat ["{\"message\":\"",message,"\"}"]) -pgErrResponse :: P.UsageError -> Response -pgErrResponse e = responseLBS (httpStatus e) - [(hContentType, "application/json")] (JSON.encode e) +pgErrResponse :: Bool -> P.UsageError -> Response +pgErrResponse authed e = + let status = httpStatus authed e + jsonType = (hContentType, "application/json") + wwwAuth = ("WWW-Authenticate", "Bearer") + hdrs = if status == HT.status401 + then [jsonType, wwwAuth] + else [jsonType] in + responseLBS status hdrs (JSON.encode e) instance JSON.ToJSON P.UsageError where toJSON (P.ConnectionError e) = JSON.object [ @@ -60,10 +66,9 @@ instance JSON.ToJSON H.Error where "message" .= ("Database client error"::String), "details" .= (fmap cs d::Maybe T.Text)] -httpStatus :: P.UsageError -> HT.Status -httpStatus (P.ConnectionError _) = - HT.status500 -httpStatus (P.SessionError (H.ResultError (H.ServerError c _ _ _))) = +httpStatus :: Bool -> P.UsageError -> HT.Status +httpStatus _ (P.ConnectionError _) = HT.status500 +httpStatus authed (P.SessionError (H.ResultError (H.ServerError c _ _ _))) = case cs c of '0':'8':_ -> HT.status503 -- pg connection err '0':'9':_ -> HT.status500 -- triggered action exception @@ -88,7 +93,7 @@ httpStatus (P.SessionError (H.ResultError (H.ServerError c _ _ _))) = 'P':'0':_ -> HT.status500 -- PL/pgSQL Error 'X':'X':_ -> HT.status500 -- internal Error "42P01" -> HT.status404 -- undefined table - "42501" -> HT.status404 -- insufficient privilege + "42501" -> if authed then HT.status403 else HT.status401 -- insufficient privilege _ -> HT.status400 -httpStatus (P.SessionError (H.ResultError _)) = HT.status500 -httpStatus (P.SessionError (H.ClientError _)) = HT.status503 +httpStatus _ (P.SessionError (H.ResultError _)) = HT.status500 +httpStatus _ (P.SessionError (H.ClientError _)) = HT.status503 diff --git a/src/PostgREST/Middleware.hs b/src/PostgREST/Middleware.hs index 1281e48b5..b1be406c4 100644 --- a/src/PostgREST/Middleware.hs +++ b/src/PostgREST/Middleware.hs @@ -7,7 +7,6 @@ import Data.Aeson (Value (..)) import qualified Data.HashMap.Strict as M import Data.String.Conversions (cs) import Data.Text -import Data.Time.Clock (NominalDiffTime) import qualified Hasql.Transaction as H import Network.HTTP.Types.Header (hAccept) @@ -19,28 +18,26 @@ import Network.Wai.Middleware.Gzip (def, gzip) import Network.Wai.Middleware.Static (only, staticPolicy) import PostgREST.ApiRequest (ApiRequest(..), pickContentType) -import PostgREST.Auth (jwtClaims, claimsToSQL) +import PostgREST.Auth (claimsToSQL) import PostgREST.Config (AppConfig (..), corsPolicy) import PostgREST.Error (errResponse) import Prelude hiding (concat, null) -runWithClaims :: AppConfig -> NominalDiffTime -> +runWithClaims :: AppConfig -> Either Text (M.HashMap Text Value) -> (ApiRequest -> H.Transaction Response) -> ApiRequest -> H.Transaction Response -runWithClaims conf time app req = do - let eClaims = jwtClaims jwtSecret (iJWT req) time - case eClaims of - Left e -> clientErr e - Right claims -> - if M.null claims && not (null $ iJWT req) - then clientErr "Invalid JWT" - else do - -- role claim defaults to anon if not specified in jwt - H.sql . mconcat . claimsToSQL $ M.union claims (M.singleton "role" anon) - app req +runWithClaims conf eClaims app req = + case eClaims of + Left e -> clientErr e + Right claims -> + if M.null claims && not (null $ iJWT req) + then clientErr "Invalid JWT" + else do + -- role claim defaults to anon if not specified in jwt + H.sql . mconcat . claimsToSQL $ M.union claims (M.singleton "role" anon) + app req where - jwtSecret = configJwtSecret conf anon = String . cs $ configAnonRole conf clientErr = return . errResponse status400 diff --git a/test/Feature/AuthSpec.hs b/test/Feature/AuthSpec.hs index 5181f8ec9..3a1f5650e 100644 --- a/test/Feature/AuthSpec.hs +++ b/test/Feature/AuthSpec.hs @@ -13,8 +13,29 @@ import Network.Wai (Application) spec :: SpecWith Application spec = describe "authorization" $ do - it "hides tables that anonymous does not own" $ - get "/authors_only" `shouldRespondWith` 404 + it "denies access to tables that anonymous does not own" $ + get "/authors_only" `shouldRespondWith` ResponseMatcher { + matchBody = Just [json| { + "hint":null, + "details":null, + "code":"42501", + "message":"permission denied for relation authors_only"} |] + , matchStatus = 401 + , matchHeaders = ["WWW-Authenticate" <:> "Bearer"] + } + + it "denies access to tables that postgrest_test_author does not own" $ + let auth = authHeaderJWT "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoicG9zdGdyZXN0X3Rlc3RfYXV0aG9yIiwiaWQiOiJqZG9lIn0.y4vZuu1dDdwAl0-S00MCRWRYMlJ5YAMSir6Es6WtWx0" in + request methodGet "/private_table" [auth] "" + `shouldRespondWith` ResponseMatcher { + matchBody = Just [json| { + "hint":null, + "details":null, + "code":"42501", + "message":"permission denied for relation private_table"} |] + , matchStatus = 403 + , matchHeaders = [] + } it "returns jwt functions as jwt tokens" $ post "/rpc/login" [json| { "id": "jdoe", "pass": "1234" } |] diff --git a/test/Feature/StructureSpec.hs b/test/Feature/StructureSpec.hs index 8162385b1..95aa9a90d 100644 --- a/test/Feature/StructureSpec.hs +++ b/test/Feature/StructureSpec.hs @@ -24,6 +24,7 @@ spec = do , {"schema":"test","name":"comments","insertable":true} , {"schema":"test","name":"complex_items","insertable":true} , {"schema":"test","name":"compound_pk","insertable":true} + , {"schema":"test","name":"empty_table","insertable":true} , {"schema":"test","name":"filtered_tasks","insertable":true} , {"schema":"test","name":"ghostBusters","insertable":true} , {"schema":"test","name":"has_count_column","insertable":false} diff --git a/test/fixtures/privileges.sql b/test/fixtures/privileges.sql index e759c711e..d0e658169 100644 --- a/test/fixtures/privileges.sql +++ b/test/fixtures/privileges.sql @@ -17,6 +17,7 @@ GRANT ALL ON TABLE , comments , complex_items , compound_pk + , empty_table , has_count_column , has_fk , insertable_view_with_join diff --git a/test/fixtures/schema.sql b/test/fixtures/schema.sql index 3214f4fde..9925580eb 100755 --- a/test/fixtures/schema.sql +++ b/test/fixtures/schema.sql @@ -458,6 +458,13 @@ CREATE TABLE empty_table ( ); +-- +-- Name: private_table; Type: TABLE; Schema: test; Owner: - +-- + +CREATE TABLE private_table (); + + -- -- Name: has_count_column; Type: VIEW; Schema: test; Owner: - --