First stab at removing Cookie and JWT constraints from HasServer (Auth ...)

[alexjg]

This is a first shot at exposing the stuff needed to build custom authentication schemes without being forced to use ToJWT typeclass or set any cookies. As far as I can tell there's no way to add response headers in the auth check at the moment so it wouldn't be possible to implement the current cookie stuff using the auth check machinery without modification. This is also why the tests around cookie headers fail.

What would be the preferred approach here? Do we need to change the signature of the auth check?

[alpmestan]

Could you please leave this change out? we're now trying to be consistent over all the servant packages about not updating versions or changelogs in PRs, this is all taken care of at release time.

[alpmestan]

As far as I can tell there's no way to add response headers in the auth check at the moment

Could you expand on this? Because indeed, the auth check -- which happens when we get the request -- does not have any say in the response that will be sent further down the road.

[alexjg]

Apologies for taking a while to respond to this. Let me try and explain.

My intention was to allow building auth schemes that don't require custom IsAuth implementations to have user models that implement ToJWT. After investigating this for a while I found that the reason for these restrictions on the HasServer instance for Auth is that the code which sets the session cookie for the cookie authentication scheme is part of the HasServer instance. Specifically this code:

  route _ context subserver =
    route (Proxy :: Proxy (AddSetCookiesApi n api))
          context
          (fmap go subserver `addAuthCheck` authCheck)

    where
      authCheck :: DelayedIO (AuthResult v, SetCookieList ('S ('S 'Z)))
      authCheck = withRequest $ \req -> liftIO $ do
        authResult <- runAuthCheck (runAuths (Proxy :: Proxy auths) context) req
        cookies <- makeCookies authResult
        return (authResult, cookies)
...

So my next thought was that if we want to remove those constraints from HasServer we need to find some other way to implement setting the session cookie. Which is what lead me to the modified auth check signature idea. Does that make sense?

[alexjg]

@alpmestan does that make the problem clearer?

[alpmestan]

I have not forgotten about this, just haven't found the time to come back to this just yet. I'll answer carefully ASAP.

[alpmestan]

I like how simpler (but not simple) the HasServer instance becomes. Feels more natural. This does however mean that we don't have any place to store the cookie that we want to attach to the response, and this is what's bothering you.

I think a proper solution necessarily has to involve changing servant-server itself, to give us a way to stick those cookies (and maybe other things? what else would we want to put there? without inventing crazy scenarios, this is just to guide the thought/design process) somewhere until we actually build the response, at which point we would reach for the cookie and add it to whatever response was about to be sent. (Note: this all sounds a lot like what middleware a does.)

Perhaps Delayed could be augmented with a field for storing additional things to be put in the response? Things that we discover while we are running the other actions we have in a Delayed (like the auth check in our case). When we then run our Delayed, instead of just returning whatever the handler returns (or a routing failure), we could pair it up with those "additional things to be sent" and then construct a response with both.

@alexjg @phadej Thoughts?

(P.S: Sorry for the delay)

[alexjg]

I've been reading through the source code of servant-server to try and better understand what you're suggesting. I see what you mean about possibly adding a field to Delayed for arbitrary middleware style transformations of the response. One thing that occurs to me about that is that it does look like most of the internal implementations of the various combinators in servant-server actually just run the delayed action in their route implementation (e.g the methodRouter or streamRouter used in the implementations of HasServer for Verb and Stream respectively). Would your suggestion basically be to allow implementing those combinators in terms of the new field in Delayed as well?

[alpmestan]

Hmm, so far I was more thinking about a field that would take whatever serverD returns and would be able to "decorate it" with (in your case) a cookie, or other things that transform or augment what we would otherwise be returning, today, with the code as it is now. Do you see what I mean?

And in that case, I don't believe the xxxRouter helpers would be affected, only the functions that run the router and then send the response (we would have to somehow turn the result of our new field into a response). Unless I'm overlooking something?

Please do ask for clarifications or some (simplified) code example if needed, it's a bit late here, but I can take a stab at writing a nicer answer tomorrow, with a bit of code to illustrate my idea, if this comment is not good enough.

[alexjg]

I think that's clear enough for me to make a start on. To be clear though you're suggesting a modification to the servant-server library right? In which case I should open a PR there once I have something to show?

[alpmestan]

@alexjg Yes, my suggestion indeed involves patching servant-server. Please do feel free to open the PR as early as you want and to ask for guidance/help whenever needed, if ever.

It'd also be handy to link to this PR here from the new one, to have the links appear in the github UI and all that.

[chrisdone]

I'm currently teaching servant to a client team and was putting together a demonstrating of a custom auth method and ran across this, in this example: https://gist.github.com/chrisdone/6b893cf02e4655dacd3e1d09143d0bcf The UsernamePassword is a dummy module that just demonstrates putting something together, this isn't something anyone would ever reasonably use. But it's easy to demonstrate to a team.

However, the CookieSettings and JWTSettings are both required, which baffled me for a while before I noticed the constraint in Auth.

[jkarni]

It's been a while since I wrote this, but the cookie and JWT constraints were definitely an ugly hack. My intuition is that we could do something like the following:

class  CookieEncoding enc typ where
  encodeCookie :: Proxy enc -> typ -> Maybe Cookie

data NoCookie
instance CookieEncoding NoCookie typ where
  encodeCookie _ _ = Nothing

instance (ToJWT) CookieEncoding JWT typ where
  encodeCookie _ = ...

data Auth auths cookies usr  -- one more param

instance (AreAuths auths ctx v, CookieEncoding cookie usr, HasServer sub ctx) => HasServer (Auth auths cookie usr :> sub) ctx