From 31cd36803df01694f506a7979cc4bafdb425f923 Mon Sep 17 00:00:00 2001 From: Artyom Kazak Date: Fri, 18 Jan 2019 23:17:20 +1100 Subject: [PATCH 1/4] In SCIM, formar Spar errors using SCIM error schema --- services/spar/src/Spar/Error.hs | 6 +++ services/spar/src/Spar/Scim.hs | 58 +++++++++++++++++++++++++++-- services/spar/src/Spar/Scim/User.hs | 5 ++- 3 files changed, 63 insertions(+), 6 deletions(-) diff --git a/services/spar/src/Spar/Error.hs b/services/spar/src/Spar/Error.hs index e91e3620308..16fb1b6cd5c 100644 --- a/services/spar/src/Spar/Error.hs +++ b/services/spar/src/Spar/Error.hs @@ -28,6 +28,7 @@ import qualified Network.Wai.Utilities.Error as Wai import qualified Network.Wai.Utilities.Server as Wai import qualified SAML2.WebSSO as SAML import qualified System.Logger as Log +import qualified Web.Scim.Schema.Error as Scim type SparError = SAML.Error SparCustomError @@ -77,6 +78,9 @@ data SparCustomError | SparProvisioningNoSingleIdP LT | SparProvisioningTokenLimitReached + + -- | All errors returned from SCIM handlers are wrapped into 'SparScimError' + | SparScimError Scim.ScimError deriving (Eq, Show) sparToServantErrWithLogging :: MonadIO m => Log.Logger -> SparError -> m ServantErr @@ -159,5 +163,7 @@ renderSparError (SAML.CustomError (SparNewIdPWantHttps msg)) = Rig -- Errors related to provisioning renderSparError (SAML.CustomError (SparProvisioningNoSingleIdP msg)) = Right $ Wai.Error status400 "no-single-idp" ("Team should have exactly one IdP configured: " <> msg) renderSparError (SAML.CustomError SparProvisioningTokenLimitReached) = Right $ Wai.Error status403 "token-limit-reached" "The limit of provisioning tokens per team has been reached" +-- SCIM errors +renderSparError (SAML.CustomError (SparScimError err)) = Left $ Scim.scimToServantErr err -- Other renderSparError (SAML.CustomServant err) = Left err diff --git a/services/spar/src/Spar/Scim.hs b/services/spar/src/Spar/Scim.hs index 923285b9242..c1a237d4d8f 100644 --- a/services/spar/src/Spar/Scim.hs +++ b/services/spar/src/Spar/Scim.hs @@ -51,10 +51,16 @@ module Spar.Scim ) where import Imports + +import Control.Lens +import Control.Monad.Catch (try) import Control.Monad.Except +import Data.String.Conversions (cs) import Servant import Servant.API.Generic -import Spar.App (Spar) +import Spar.App (Spar(..), Env(..)) +import Spar.Error (SparError, SparCustomError(SparScimError), + throwSpar, sparToServantErrWithLogging) import Spar.Scim.Types import Spar.Scim.Auth import Spar.Scim.User @@ -79,9 +85,53 @@ apiScim :: ServerT APIScim Spar apiScim = hoistScim (toServant (Scim.siteServer configuration)) :<|> apiScimToken where - hoistScim = hoistServer (Proxy @(Scim.SiteAPI SparTag)) - (Scim.fromScimHandler fromError) - fromError = throwError . SAML.CustomServant . Scim.scimToServantErr + hoistScim = hoistServer + (Proxy @(Scim.SiteAPI SparTag)) + (wrapScimErrors . Scim.fromScimHandler (throwSpar . SparScimError)) + + -- Wrap /all/ errors into the format required by SCIM, even server exceptions that have + -- nothing to do with SCIM. + -- + -- FIXME: this doesn't catch impure exceptions (e.g. thrown with 'error'). + -- Let's hope that SCIM clients can handle non-SCIM-formatted errors + -- properly. See + -- for why it's hard to catch impure exceptions. + wrapScimErrors :: Spar a -> Spar a + wrapScimErrors = over _Spar $ \act -> \env -> do + result :: Either SomeException (Either SparError a) <- try (act env) + + case result of + -- We caught an exception that's not a Spar exception at all. It is wrapped into + -- Scim.serverError. + Left someException -> + pure $ Left . SAML.CustomError . SparScimError $ + Scim.serverError (cs (displayException someException)) + + -- We caught a 'SparScimError' exception. It is left as-is. + Right (Left (SAML.CustomError (SparScimError x))) -> + pure $ Left (SAML.CustomError (SparScimError x)) + + -- We caught some other Spar exception. It is wrapped into Scim.serverError. + -- + -- TODO: does it have to be logged? + Right (Left sparError) -> do + err <- sparToServantErrWithLogging (sparCtxLogger env) sparError + pure $ Left . SAML.CustomError . SparScimError $ + Scim.serverError (cs (errBody err)) + + -- No exceptions! Good. + Right (Right x) -> pure $ Right x + +---------------------------------------------------------------------------- +-- Orphan instances instance Scim.Group.GroupDB SparTag Spar where -- TODO + +---------------------------------------------------------------------------- +-- Utilities + +-- | An isomorphism that unwraps the Spar stack (@Spar . ReaderT . ExceptT@) into a +-- newtype-less form that's easier to work with. +_Spar :: Iso' (Spar a) (Env -> IO (Either SparError a)) +_Spar = coerced diff --git a/services/spar/src/Spar/Scim/User.hs b/services/spar/src/Spar/Scim/User.hs index c77468fafaf..2e57fde05d8 100644 --- a/services/spar/src/Spar/Scim/User.hs +++ b/services/spar/src/Spar/Scim/User.hs @@ -525,7 +525,7 @@ toScimEmail (Email eLocal eDomain) = -- Note [error handling] -- ~~~~~~~~~~~~~~~~~ -- --- There are two problems with error handling here: +-- FUTUREWORK: There are two problems with error handling here: -- -- 1. We want all errors originating from SCIM handlers to be thrown as SCIM -- errors, not as Spar errors. Currently errors thrown from things like @@ -534,4 +534,5 @@ toScimEmail (Email eLocal eDomain) = -- on what is expected by apps that use the SCIM interface. -- -- 2. We want generic error descriptions in response bodies, while still --- logging nice error messages internally. +-- logging nice error messages internally. The current messages might +-- be giving too many internal details away. From 0ed4b47adfbe15065afb2997c8459d26f7598529 Mon Sep 17 00:00:00 2001 From: fisx Date: Tue, 9 Apr 2019 13:47:44 +0200 Subject: [PATCH 2/4] Update services/spar/src/Spar/Scim.hs Co-Authored-By: neongreen --- services/spar/src/Spar/Scim.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/spar/src/Spar/Scim.hs b/services/spar/src/Spar/Scim.hs index c1a237d4d8f..0c370cdfa57 100644 --- a/services/spar/src/Spar/Scim.hs +++ b/services/spar/src/Spar/Scim.hs @@ -108,7 +108,7 @@ apiScim = hoistScim (toServant (Scim.siteServer configuration)) Scim.serverError (cs (displayException someException)) -- We caught a 'SparScimError' exception. It is left as-is. - Right (Left (SAML.CustomError (SparScimError x))) -> + Right err@(Left (SAML.CustomError (SparScimError x))) -> pure $ Left (SAML.CustomError (SparScimError x)) -- We caught some other Spar exception. It is wrapped into Scim.serverError. From 4531d7df1fde9e0528a0a58cd7df2fd2b7f4e1fe Mon Sep 17 00:00:00 2001 From: fisx Date: Tue, 9 Apr 2019 13:47:53 +0200 Subject: [PATCH 3/4] Update services/spar/src/Spar/Scim.hs Co-Authored-By: neongreen --- services/spar/src/Spar/Scim.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/spar/src/Spar/Scim.hs b/services/spar/src/Spar/Scim.hs index 0c370cdfa57..b59b61ef3ea 100644 --- a/services/spar/src/Spar/Scim.hs +++ b/services/spar/src/Spar/Scim.hs @@ -109,7 +109,7 @@ apiScim = hoistScim (toServant (Scim.siteServer configuration)) -- We caught a 'SparScimError' exception. It is left as-is. Right err@(Left (SAML.CustomError (SparScimError x))) -> - pure $ Left (SAML.CustomError (SparScimError x)) + pure err -- We caught some other Spar exception. It is wrapped into Scim.serverError. -- From 59e5593e3a697e8ec7f729d8a513ba196fa723a8 Mon Sep 17 00:00:00 2001 From: Artyom Kazak Date: Tue, 9 Apr 2019 14:25:36 +0200 Subject: [PATCH 4/4] Fix compilation --- services/spar/src/Spar/Scim.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/spar/src/Spar/Scim.hs b/services/spar/src/Spar/Scim.hs index b59b61ef3ea..bad4523f1fd 100644 --- a/services/spar/src/Spar/Scim.hs +++ b/services/spar/src/Spar/Scim.hs @@ -108,7 +108,7 @@ apiScim = hoistScim (toServant (Scim.siteServer configuration)) Scim.serverError (cs (displayException someException)) -- We caught a 'SparScimError' exception. It is left as-is. - Right err@(Left (SAML.CustomError (SparScimError x))) -> + Right err@(Left (SAML.CustomError (SparScimError _))) -> pure err -- We caught some other Spar exception. It is wrapped into Scim.serverError.