haskell-servant / haskell-servant/servant

`acceptLogin` should not return a `Maybe`

Open
#1,613 0 comments 5 reactions 0 assignees View on GitHub
Dominant language
Haskell
Stars
2k
Forks
427
Avg merge
2d 23h
Merged PRs (30d)
5

Description

This is a fairly nitty ask, so no worries if this is low priority.

[`acceptLogin`](https://hackage.haskell.org/package/servant-auth-server-0.4.7.0/docs/Servant-Auth-Server.html#v:acceptLogin) in `servant-auth-server` currently returns an `IO (Maybe (response -> withTwoCookies))`. For an example usage, see [the snippet from the `servant-auth` README](https://github.com/haskell-servant/servant/tree/master/servant-auth#cookies):

```hs
-- Here is the login handler
checkCreds :: CookieSettings
-> JWTSettings
-> Login
-> Handler (Headers '[ Header "Set-Cookie" SetCookie
, Header "Set-Cookie" SetCookie]
NoContent)
checkCreds cookieSettings jwtSettings (Login "Ali Baba" "Open Sesame") = do
-- Usually you would ask a database for the user info. This is just a
-- regular servant handler, so you can follow your normal database access
-- patterns (including using 'enter').
let usr = User "Ali Baba" "ali@email.com"
mApplyCookies <- liftIO $ acceptLogin cookieSettings jwtSettings usr
case mApplyCookies of
Nothing -> throwError err401
Just applyCookies -> return $ applyCookies NoContent
checkCreds _ _ _ = throwError err401
```

In particular, notice that `mApplyCookies` here is a `Maybe (response -> withTwoCookies)`.

IMO, this is an unnecessarily unergonomic interface. Instead, `acceptLogin` should return a `IO (response -> withTwoCookies)`, and throw an exception in cases where it would otherwise return `Nothing`. Here's why:

1. `acceptLogin` ([source](https://hackage.haskell.org/package/servant-auth-server-0.4.7.0/docs/src/Servant.Auth.Server.Internal.Cookie.html#acceptLogin)) currently returns a `Maybe` because [`makeSessionCookie`](https://hackage.haskell.org/package/servant-auth-server-0.4.7.0/docs/src/Servant.Auth.Server.Internal.Cookie.html#makeSessionCookie) returns a `Maybe` because [`makeJWT`](https://hackage.haskell.org/package/servant-auth-server-0.4.7.0/docs/src/Servant.Auth.Server.Internal.JWT.html#makeJWT) returns an `Either Jose.Error ByteString`.
2. However, this [`JOSE.Error`](https://hackage.haskell.org/package/jose-0.9/docs/Crypto-JOSE-Error.html#t:Error) type encompasses a bunch of exceptional cases that users should not be expecting to occur (e.g. requested algorithm is not implemented, key is not usable, etc.), all of which seem to me can only result from configuration errors that are unhandleable at runtime. Throwing an exception would cause the handler to 500 by default when JWT creation failed, which seems like the correct thing to do (I actually disagree with the snippet here, which suggests that a `Nothing` should result in a 401 - none of the possible errors seem like client errors to me).
3. `acceptLogin` already occurs within `IO`, where users should expect exceptions anyway.

How do people feel about changing this interface? I can take a stab at the change when time permits if folks agree that it's a good idea.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.