tweag / servant-oauth2

A modern servant wrapper around the wai-middleware-auth OAuth2 provider implementations.
MIT License
9 stars 4 forks source link

Refactor wai-middleware-auth to be more convenient #10

Open silky opened 2 years ago

silky commented 2 years ago

wai-middleware-auth is specialised to Wai.Response in how it throws errors and attempts redirects.

Moreover, it's quite hard to get out the loginUrl (unless you go via the handleLogin path, which is a bit overkill, as it needs an entire route dedicated to it, kind of.)

The task here is to do a significant refactor of the wai library so that it, perhaps, supports both Servant (Handler's) and raw Wai Responses. This would at least let it continued to be used by raw wai applications (i.e. yesod or otherwise) and servant ones.

Alternatively, we could just completely fork it and just change it to be suitable for servant. While this is less work, I'm more reluctant to do it, because it needlessly partitions the codebase; so that if there are bugfixes in one they won't get adopted in the other.

The main goal would be to eliminate the kind of hacky transformations you see here: https://github.com/tweag/servant-oauth2/blob/main/src/Servant/OAuth2.hs#L122 and here https://github.com/tweag/servant-oauth2/blob/main/src/Servant/OAuth2/Hacks.hs#L49

sigrdrifa commented 3 weeks ago

I've been trying to use this package but it seems that wai-middleware-auth has gone pretty stale and as a result I'm having issues even building my project when I pull in servant-oauth2 (dependency issues with hoauth2 version constraints inherited from an outdated wai-middleware-auth package).

I saw that you have a PR into wai-middleware-auth (https://github.com/fpco/wai-middleware-auth/pull/32) but it also seems pretty dead

I guess my question is, considering how stale wai-middleware-auth is now, would it be a good idea to refactor this library to focus on Servant instead and drop the support for raw Wai responses?

silky commented 3 weeks ago

Thanks for giving it a try!

I guess my question is, considering how stale wai-middleware-auth is now, would it be a good idea to refactor this library to focus on Servant instead and drop the support for raw Wai responses?

Yes, I'm very open-minded to whatever might work best for you.

Another idea could be to issue a take-over request for the wai-* packages that are blocking us; they are still so central in the community but somewhat unmaintained, so it would be really great if someone would take on that work!

sigrdrifa commented 3 weeks ago

Ah yeah a take-over would be really good, I just don't know if I'd be able to maintain it properly (and if they'd let us take it over in the first place..). I'm not entirely new to haskell but I'm no expert either and I rarely ever touch lower level WAI or warp things.

That being said, I'm open to giving it a shot!

Alternatively, I was thinking of integrating OAUTH2 "directly" into Servant by making use of their Generalized Authentication workflow? https://docs.servant.dev/en/stable/tutorial/Authentication.html#generalized-authentication

My understanding is that it's a middleware that can do things like check a cookie value when pre-processing requests which seemingly would be able to check an OAUTH / JWT cookie? We'd just have to do the code to do the redirect to get the cookie set in the first place

silky commented 3 weeks ago

Alternatively, I was thinking of integrating OAUTH2 "directly" into Servant by making use of their Generalized Authentication workflow? https://docs.servant.dev/en/stable/tutorial/Authentication.html#generalized-authentication

FWIW, I believe (and forgive me for not quite remembering!) that this is what we do here anyway.

Probably the easiest way to proceed is just as I indicate above; just hack wai-authentication-middleware to give up on Wai and use it only for servant; then just depend on that fork.

That said, feel free to take a careful glance around here - https://github.com/tweag/servant-oauth2/blob/main/src/Servant/OAuth2.hs#L103

I honestly don't remember, but I think it had to be that way (details of wai) because of the "complete" step of OAuth2. Unfortunately it seems I didn't document it well enough to quite remember why it was needed 😅

sigrdrifa commented 3 weeks ago

No problem at all and thanks for your help!

silky commented 3 weeks ago

Yeah I think the basic problem is that handleLogin deals in Wai.Response; so we need to wrap the ident in that - https://github.com/tweag/servant-oauth2/blob/main/src/Servant/OAuth2.hs#L115 - in order to later read it.

That's a bit of a shame.

That type, instead, could be generic, and just specialise where needed, and I think everything would be fine (i.e. the AuthProvider tells you what type it wants the responses to be).

I think the generalised authentication documentation cheats a bit, in that it just assumes a cookie is present; but for our purposes we can't assume that, we need to actually do the entire OAuth2 flow.

sigrdrifa commented 3 weeks ago

Ah yes good catch! I'm away this week but I might see if I can take a stab at making it more generic when I'm back.

For now I've solved the issue by forking your updated version of the middleware package and pulling that one into my project.

Thanks again!