Refactor and unit test authentication code paths (closes hasura#4736)

The bulk of changes here is some shifting of code around and a little
parameterizing of functions for easier testing.

Also: comments, some renaming for clarity/less-chance-for-misue.

CHANGELOG.md

@@ -72,6 +72,7 @@ Read more about the session argument for computed fields in the [docs](https://h
 - server: fix importing of allow list query from metadata (fix #4687)
 - server: flush log buffer during shutdown (#4800)
 - server: fix edge case with printing logs on startup failure (fix #4772)
+- server: raise error on startup when `--unauthorized-role` is ignored (#4736)
 - console: avoid count queries for large tables (#4692)
 - console: add read replica support section to pro popup (#4118)
 - console: allow modifying default value for PK (fix #4075) (#4679)

docs/graphql/manual/auth/authentication/jwt.rst

@@ -60,7 +60,7 @@ the following:
 1. A ``x-hasura-default-role`` field : indicating the default role of that user i.e. the role that will be
    used in case ``x-hasura-role`` header is not passed.
 2. A ``x-hasura-allowed-roles`` field : a list of allowed roles for the user i.e. acceptable values of the
-   ``x-hasura-role`` header.
+   ``x-hasura-role`` header. The ``x-hasura-default-role`` specified should be a member of this list.
 
 The claims in the JWT can have other ``x-hasura-*`` fields where their values
 can only be strings. You can use these ``x-hasura-*`` fields in your

server/graphql-engine.cabal

@@ -242,6 +242,7 @@ library
                      -- Exposed for testing:
                      , Hasura.Server.Telemetry.Counters
                      , Data.Parser.JSONPath
+                     , Hasura.Server.Auth.JWT
 
                      , Hasura.RQL.Types
                      , Hasura.RQL.Types.Run
@@ -259,7 +260,6 @@ library
                      , Hasura.Incremental.Internal.Dependency
                      , Hasura.Incremental.Internal.Rule
 
-                     , Hasura.Server.Auth.JWT
                      , Hasura.Server.Auth.WebHook
                      , Hasura.Server.Middleware
                      , Hasura.Server.Cors
@@ -430,7 +430,9 @@ test-suite graphql-engine-tests
     , hspec-core >=2.6.1 && <3
     , hspec-expectations-lifted
     , http-client
+    , http-types
     , http-client-tls
+    , jose
     , lifted-base
     , monad-control
     , mtl
@@ -456,6 +458,7 @@ test-suite graphql-engine-tests
     Hasura.RQL.MetadataSpec
     Hasura.Server.MigrateSpec
     Hasura.Server.TelemetrySpec
+    Hasura.Server.AuthSpec
 
 -- Benchmarks related to caching (e.g. the plan cache).
 --

server/src-lib/Hasura/GraphQL/Explain.hs

@@ -83,6 +83,7 @@ getSessVarVal userInfo sessVar =
     rn = _uiRole userInfo
     sessionVariables = _uiSession userInfo
 
+-- CAREFUL! This is potentially executing as admin.
 explainField
   :: (MonadError QErr m, MonadTx m, HasVersion, MonadIO m)
   => UserInfo
@@ -103,6 +104,8 @@ explainField userInfo gCtx sqlGenCtx actionExecuter fld =
       resolvedAST <- RS.traverseQueryRootFldAST (resolveVal userInfo) unresolvedAST
       let (query, remoteJoins) = RS.toPGQuery resolvedAST
           txtSQL = Q.getQueryText query
+          -- CAREFUL!: an `EXPLAIN ANALYZE` here would actually *execute* this query. If `txtSQL` 
+          -- somehow contained a mutation this would result in privilege escalation:
           withExplain = "EXPLAIN (FORMAT TEXT) " <> txtSQL
       -- Reject if query contains any remote joins
       when (remoteJoins /= mempty) $ throw400 NotSupported "Remote relationships are not allowed in explain query"
@@ -126,6 +129,8 @@ explainGQLQuery
   -> GQLExplain
   -> m EncJSON
 explainGQLQuery pgExecCtx sc sqlGenCtx enableAL actionExecuter (GQLExplain query userVarsRaw) = do
+  -- CAREFUL!: we will be executing what follows as admin role, so we better
+  -- not screw this up:
   userInfo <- mkUserInfo (URBFromSessionVariablesFallback adminRoleName) UAdminSecretSent sessionVariables
   (execPlan, queryReusability) <- runReusabilityT $
     E.getExecPlanPartial userInfo sc enableAL query

server/src-lib/Hasura/Server/Auth.hs

@@ -19,6 +19,9 @@ module Hasura.Server.Auth
   , processJwt
   , updateJwkRef
   , UserAuthentication (..)
+
+  -- * Exposed for testing
+  , getUserInfoWithExpTime_
   ) where
 
 import           Control.Concurrent.Extended (forkImmortal)
@@ -35,7 +38,7 @@ import qualified Network.HTTP.Types          as N
 import           Hasura.Logging
 import           Hasura.Prelude
 import           Hasura.RQL.Types
-import           Hasura.Server.Auth.JWT
+import           Hasura.Server.Auth.JWT      hiding (processJwt_)
 import           Hasura.Server.Auth.WebHook
 import           Hasura.Server.Utils
 import           Hasura.Session
@@ -69,6 +72,10 @@ class (Monad m) => UserAuthentication m where
 newtype AdminSecretHash = AdminSecretHash (Crypto.Digest Crypto.SHA512)
   deriving (Ord, Eq)
 
+-- we just care about Show AuthMode, for testing purposes:
+instance Show AdminSecretHash where
+  show _ = "_"
+
 hashAdminSecret :: T.Text -> AdminSecretHash
 hashAdminSecret = AdminSecretHash . Crypto.hash . T.encodeUtf8
 
@@ -83,6 +90,7 @@ data AuthMode
   | AMAdminSecret !AdminSecretHash !(Maybe RoleName)
   | AMAdminSecretAndHook !AdminSecretHash !AuthHook
   | AMAdminSecretAndJWT !AdminSecretHash !JWTCtx !(Maybe RoleName)
+  deriving (Show, Eq)
 
 -- | Validate the user's requested authentication configuration, launching any
 -- required maintenance threads for JWT etc.
@@ -102,13 +110,19 @@ setupAuthMode
   -> m AuthMode
 setupAuthMode mAdminSecretHash mWebHook mJwtSecret mUnAuthRole httpManager logger =
   case (mAdminSecretHash, mWebHook, mJwtSecret) of
-    (Nothing,  Nothing,   Nothing)      -> return AMNoAuth
     (Just hash, Nothing,   Nothing)      -> return $ AMAdminSecret hash mUnAuthRole
-    (Just hash, Just hook, Nothing)      -> unAuthRoleNotReqForWebHook >>
-                                           return (AMAdminSecretAndHook hash hook)
     (Just hash, Nothing,   Just jwtConf) -> do
       jwtCtx <- mkJwtCtx jwtConf
       return $ AMAdminSecretAndJWT hash jwtCtx mUnAuthRole
+    -- Nothing below this case uses unauth role. Throw a fatal error if we would otherwise ignore
+    -- that parameter, lest users misunderstand their auth configuration:
+    _ | isJust mUnAuthRole -> throwError $ 
+        "Fatal Error: --unauthorized-role (HASURA_GRAPHQL_UNAUTHORIZED_ROLE)" 
+          <> requiresAdminScrtMsg
+          <> " and is not allowed when --auth-hook (HASURA_GRAPHQL_AUTH_HOOK) is set"
+
+    (Nothing,  Nothing,   Nothing)  -> return AMNoAuth
+    (Just hash, Just hook, Nothing) -> return $ AMAdminSecretAndHook hash hook
 
     (Nothing, Just _,  Nothing) -> throwError $
       "Fatal Error : --auth-hook (HASURA_GRAPHQL_AUTH_HOOK)" <> requiresAdminScrtMsg
@@ -122,10 +136,6 @@ setupAuthMode mAdminSecretHash mWebHook mJwtSecret mUnAuthRole httpManager logge
     requiresAdminScrtMsg =
       " requires --admin-secret (HASURA_GRAPHQL_ADMIN_SECRET) or "
       <> " --access-key (HASURA_GRAPHQL_ACCESS_KEY) to be set"
-    unAuthRoleNotReqForWebHook =
-      when (isJust mUnAuthRole) $ throwError $
-        "Fatal Error: --unauthorized-role (HASURA_GRAPHQL_UNAUTHORIZED_ROLE) is not allowed"
-        <> " when --auth-hook (HASURA_GRAPHQL_AUTH_HOOK) is set"
 
     -- | Given the 'JWTConfig' (the user input of JWT configuration), create
     -- the 'JWTCtx' (the runtime JWT config used)
@@ -177,12 +187,30 @@ getUserInfoWithExpTime
   -> [N.Header]
   -> AuthMode
   -> m (UserInfo, Maybe UTCTime)
-getUserInfoWithExpTime logger manager rawHeaders = \case
+getUserInfoWithExpTime = getUserInfoWithExpTime_ userInfoFromAuthHook processJwt
+
+-- Broken out for testing with mocks:
+getUserInfoWithExpTime_
+  :: forall m _Manager _Logger_Hasura. (MonadIO m, MonadError QErr m)
+  => (_Logger_Hasura -> _Manager -> AuthHook -> [N.Header] -> m (UserInfo, Maybe UTCTime))
+  -- ^ mock 'userInfoFromAuthHook'
+  -> (JWTCtx -> [N.Header] -> Maybe RoleName -> m (UserInfo, Maybe UTCTime))
+  -- ^ mock 'processJwt'
+  -> _Logger_Hasura
+  -> _Manager
+  -> [N.Header]
+  -> AuthMode
+  -> m (UserInfo, Maybe UTCTime)
+getUserInfoWithExpTime_ userInfoFromAuthHook_ processJwt_ logger manager rawHeaders = \case
 
   AMNoAuth -> withNoExpTime $ mkUserInfoFallbackAdminRole UAuthNotSet
 
+  -- If hasura was started with an admin secret we:
+  --   - check if a secret was sent in the request
+  --     - if so, check it and authorize as admin else fail
+  --   - if not proceed with either webhook or JWT auth if configured
   AMAdminSecret realAdminSecretHash maybeUnauthRole ->
-    withAuthorization realAdminSecretHash $ withNoExpTime $
+    checkingSecretIfSent realAdminSecretHash $ withNoExpTime $
       -- Consider unauthorized role, if not found raise admin secret header required exception
       case maybeUnauthRole of
         Nothing -> throw401 $ adminSecretHeader <> "/"
@@ -191,21 +219,22 @@ getUserInfoWithExpTime logger manager rawHeaders = \case
           mkUserInfo (URBPreDetermined unAuthRole) UAdminSecretNotSent sessionVariables
 
   AMAdminSecretAndHook realAdminSecretHash hook ->
-    withAuthorization realAdminSecretHash $ userInfoFromAuthHook logger manager hook rawHeaders
+    checkingSecretIfSent realAdminSecretHash $ userInfoFromAuthHook_ logger manager hook rawHeaders
 
   AMAdminSecretAndJWT realAdminSecretHash jwtSecret unAuthRole ->
-    withAuthorization realAdminSecretHash $ processJwt jwtSecret rawHeaders unAuthRole
+    checkingSecretIfSent realAdminSecretHash $ processJwt_ jwtSecret rawHeaders unAuthRole
 
   where
+    -- CAREFUL!:
     mkUserInfoFallbackAdminRole adminSecretState =
       mkUserInfo (URBFromSessionVariablesFallback adminRoleName)
       adminSecretState sessionVariables
 
     sessionVariables = mkSessionVariables rawHeaders
 
-    withAuthorization
+    checkingSecretIfSent
       :: AdminSecretHash -> m (UserInfo, Maybe UTCTime) -> m (UserInfo, Maybe UTCTime)
-    withAuthorization realAdminSecretHash actionIfNoAdminSecret = do
+    checkingSecretIfSent realAdminSecretHash actionIfNoAdminSecret = do
       let maybeRequestAdminSecret =
             foldl1 (<|>) $ map (`getSessionVariableValue` sessionVariables)
             [adminSecretHeader, deprecatedAccessKeyHeader]