Merge pull request #590 from begriffs/proper-403

Return proper 401/403 when access denied

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

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
 

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

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 :: Bool -> P.UsageError -> Response
-pgErrResponse e = responseLBS (httpStatus e)
+pgErrResponse authed e =
-  [(hContentType, "application/json")] (JSON.encode 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 :: Bool -> P.UsageError -> HT.Status
-httpStatus (P.ConnectionError _) =
+httpStatus _ (P.ConnectionError _) = HT.status500
-  HT.status500
+httpStatus authed (P.SessionError (H.ResultError (H.ServerError c _ _ _))) =
-httpStatus (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.ResultError _)) = HT.status500
-httpStatus (P.SessionError (H.ClientError _)) = HT.status503
+httpStatus _ (P.SessionError (H.ClientError _)) = HT.status503

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
+runWithClaims conf eClaims app req =
-    let eClaims = jwtClaims jwtSecret (iJWT req) time
+  case eClaims of
-    case eClaims of
+    Left e -> clientErr e
-      Left e -> clientErr e
+    Right claims ->
-      Right claims ->
+      if M.null claims && not (null $ iJWT req)
-        if M.null claims && not (null $ iJWT req)
+        then clientErr "Invalid JWT"
-          then clientErr "Invalid JWT"
+        else do
-          else do
+          -- role claim defaults to anon if not specified in jwt
-            -- role claim defaults to anon if not specified in jwt
+          H.sql . mconcat . claimsToSQL $ M.union claims (M.singleton "role" anon)
-            H.sql . mconcat . claimsToSQL $ M.union claims (M.singleton "role" anon)
+          app req
-            app req
   where
-    jwtSecret = configJwtSecret conf
     anon = String . cs $ configAnonRole conf
     clientErr = return . errResponse status400
 

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" $
+  it "denies access to tables that anonymous does not own" $
-    get "/authors_only" `shouldRespondWith` 404
+    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" } |]

test/Feature/StructureSpec.hs

@@ -25,6 +25,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}

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

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: -
 --