SaturnFramework / Saturn

Opinionated, web development framework for F# which implements the server-side, functional MVC pattern
https://saturnframework.org
MIT License
714 stars 108 forks source link

[Discussion] plug ordering, especially for auth plugs #169

Closed baronfel closed 4 years ago

baronfel commented 5 years ago

I ran into an interesting situation today while migrating some Nancy code to Giraffe (and prospectively Saturn as well) that I don't have a nice pattern and that Saturn might have a way to solve, so I wanted to describe it here and see if anyone had thoughts on it.

So I've got an MVC app with two types of routes:

I've encapsulated these behaviors into a few giraffe handlers:

// redirect to our login url with a query string parameter for the original destination to return to post-login
let redirectToLogin: HttpHandler = ...

let show401: HttpHandler = RequestErrors.UNAUTHORIZED "bleh" "bleh" "get gud, yo"

let mvcAuth: HttpHandler = requiresAuthentication redirectToLogin

let apiAuth: HttpHandler = requiresAuthentication show401

and my initial usage in the migrated route code looked something like:

mvcAuth 
>=> choose [
  route "/" >=> GET >=> htmlView index
  route "/docs" >=> GET >=> htmlView userDocs
]

The problem with this is that the auth is checked and a redirect happens before the routing portion happens, and that's not great because now this pipeline matches everything.

So I solved this by inlining the auth check after the route handling:

choose [
  route "/" >=> GET >=> mvcAuth >=> htmlView index
  route "/docs" >=> GET >=> mvcAuth >=> htmlView userDocs
]

which works fine, but requires a large amount of duplication.

So at this point I'm looking at Saturn routers and the plug mechanism, which would work fine:

let siteRoot = router {
  get "/" (htmlView index)
  get "/docs" (htmlView userDocs)
  plug [mvcAuth]
}

except based on my reading, this too would result in every request triggering the mvcAuth handler, and as such nothing useful ever happening. This is because the plug pipeline is composed and chained before the router functions in the run mechanism of the router CE.

It's almost like what I'm looking for is a mechanism for plugs to run pre-route-binding, post-route-binding, or post-route-handler, if that makes sense. Has anyone else had this use case before?

TheAngryByrd commented 5 years ago

Does aspnet core/giraffe/saturn have a similar concept to PerformPassthrough? This essentially lets the pipeline continue if it matches this delegate.

Krzysztof-Cieslak commented 5 years ago

I'm not sure if any other concept is necessary here. Remember that everything in Saturn is HttpHandler and hence it's easy to compose all the things in any way you want. Built-in features (like plug operation in router and controller) assume certain order of operation and I'd thing it's by-design. However it's really easy to compose it differently.

For example what you want to achieve here would be something like that:

let authedPipeline endpoint = pipeline {
    plug mvcAuth
    plug endpoint
}

let siteRoot = router {
  get "/" (authedPipeline (htmlView index))
  get "/docs" (authedPipeline (htmlView userDocs))
} 

Or even just

let siteRoot = router {
  get "/" (mvcAuth >=> htmlView index)
  get "/docs" (mvcAuth >=> htmlView userDocs)
}
baronfel commented 5 years ago

I agree, I was mainly just hoping to avoid having to decorate each route in a router individually because it clutters up the code to some degree. What you show in your examples is basically what I ended up with, it just introduced so much noise that I wanted to see if there was a better way, especially since in many app frameworks (in my experience) the url routing is used as a 'filter' to determine which auth strategy should be applied.

baronfel commented 5 years ago

Thinking about this a little bit more, would it make sense to have auth as a first class concept in a router? What I mean by this is exposing a new computation expression member called 'auth' that would be given a particular authentication HTTP handler (and/or overloaded with a string option that would call the giraffe requiresAuth handler that checks a particular scheme) that would be run post route matching but pre-route-handler-execution?

isaacabraham commented 5 years ago

@baronfel I'd be in favour of having some auth keyword there - it's such a common thing to need to do and goes (IMHO) with the saturn approach of making it as easy as possible to do these sorts of things.

baronfel commented 5 years ago

I spent a bit of time adding it this weekend, and it certainly feels nicer to me than the current solution, which as shown in one of the samples is a two step of forwarding to a subrouter from a known-protected root, and applying the auth as a pipeline step to the whole router.

My contention is that pattern is just doing the same mechanism in a clunkier way: applying some auth based on (part of) the route

NinoFloris commented 5 years ago

@baronfel this issue is also still open and pretty relevant https://github.com/SaturnFramework/Saturn/issues/147

What I usually do as a decent compromise is to define controllers like this:

let projects = requireAuth >=> controller {
    // ...
}

This will also protect any subControllers, what I like about it is that it's very easy to change from requireAuth on the whole controller to more fine grained auth on some actions and not others.

Krzysztof-Cieslak commented 4 years ago

I'm closing this discussion - in the (near) future we will move to endpoint routing which by design separates path routing from behaviour, and in such situation concept of plugging handlers before routing won't really exist.