refactor: remove auth and logging middleware#4884
refactor: remove auth and logging middleware#4884steve-chavez wants to merge 1 commit intoPostgREST:mainfrom
Conversation
This commit removes auth middleware for it hides side effects and obscures logic. The auth operations are now done in its own stage in the request-response cycle. It also removes the logging middleware because now we instead use observation module to log the response.
steve-chavez
force-pushed
the
remove-middleware-auth-logger
branch
from
57aef68 to
403b1c6
Compare
|
|
||
| let bearerAuth = ApiRequest.userBearerAuth req | ||
|
|
||
| (jwtTime, authResult@AuthResult{..}) <- withExceptT (Nothing,) $ -- no role at this stage |
There was a problem hiding this comment.
This is more explicit now, explaining better the behaviors on #4882
| when (shouldLogResponse logLevel status) $ do | ||
| zTime <- stateGetZTime loggerState | ||
| let handl = stdout -- doing this indirection since the linter wants to change "hPutStr stdout" to "putStr", but we want "stdout" to appear explicitly | ||
| hPutStr handl $ apacheFormat maybeRole (BS.pack $ formatTime defaultTimeLocale "%d/%b/%Y:%T %z" zTime) req status contentLen |
There was a problem hiding this comment.
Why not logWithZTime $ observationMessages o here?
| WarpServerObs txt -> | ||
| pure $ "Warp server: " <> txt | ||
| ResponseObs {} -> | ||
| mempty |
There was a problem hiding this comment.
Wouldn't it be better to move the formatting code here?
There was a problem hiding this comment.
Yes, it would be better, but it's not very straight forward. I think issue here is that observation message includes time from loggerState which is not in scope here. It would need more refactoring.
Maybe add a comment to clarify?
|
|
||
| let | ||
| initApp sCache st config = do | ||
| initApp sCache _ config = do |
There was a problem hiding this comment.
Maybe remove this unneeded parameter from everywhere in the module or add a TODO to do it later?
| Nothing -> do | ||
| lift $ observer SchemaCacheEmptyObs | ||
| throwError Error.NoSchemaCacheError | ||
| throwError (authRole', Error.NoSchemaCacheError) |
There was a problem hiding this comment.
Hmm, maybe inlining it would be more clear so it matches the function signature (Maybe ByteString, Error)?
| throwError (authRole', Error.NoSchemaCacheError) | |
| throwError (Just authRole, Error.NoSchemaCacheError) |
| -> Wai.Request | ||
| -> Handler IO Wai.Response | ||
| postgrestResponse appState conf@AppConfig{..} maybeSchemaCache authResult@AuthResult{..} req = do | ||
| -> ExceptT (Maybe ByteString, Error) IO (ByteString, Wai.Response) |
There was a problem hiding this comment.
To be honest passing the role like this (ie. both in the result and in the exception type) using tuples is not too good - this looks like a state monad in disguise.
I understand we need to have the role name available in the caller in both cases (because Error might happen after auth).
I would either:
- extract authentication from
postgrestResponseand pass the role name to it. - pass some
IORef (Maybe ByteString)explicitly to it to record the state after auth.
The first alternative looks the cleanest.
WDYT?
3 participants
Continues #4064 and #4062.
I think it's important to do this now before #3140, otherwise we'll accumulate tech debt.
Benefits: