Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auth middleware #41

Open
swlkr opened this issue Mar 23, 2020 · 6 comments
Open

Auth middleware #41

swlkr opened this issue Mar 23, 2020 · 6 comments

Comments

@swlkr
Copy link
Collaborator

swlkr commented Mar 23, 2020

No description provided.

@swlkr swlkr changed the title Magic link middleware Auth middleware Jun 2, 2020
@jcreager
Copy link

jcreager commented Aug 3, 2020

Any thoughts on what shape this might take? The main reason for the issue I reported earlier (#69) is because I was attempting to roll my own auth middleware for the time being.

I think the before/after middlewares can be used to accomplish what I am after. I want to be able to have a middleware that can be placed in front of specific routes that require auth. If a user is not logged in the after middleware will redirect the user to a login page. Ideally, after the user is done logging in they will be redirected back to their original destination route.

Apart from the issue I reported about the before/after middlewares, I'm struggling to come up with a middleware solution. I tried setting up a a regular middleware that looks something like this:

(defn require-login [handler]
  (fn [req]
    (def dest (req :uri))
    (def logged-in (get-in req [:session :account]))
    (if (and (not logged-in) (= (req :uri) "/reports"))
      (-> (redirect (string "/login?dest=" dest)))
      (handler req))))

And is consumed like this:

(def app (-> (app {:layout layout})
             (require-auth)))

The trouble I am running into is that although the require-login middleware above is capable of redirecting a user, it does not appear to be called at the right point in the middleware stack. As a result, it cannot see :session because :session has not been decoded and added to the request yet.

I'm not sure how to go about creating a middleware that is called after the session middleware is called. Any thoughts?

@swlkr
Copy link
Collaborator Author

swlkr commented Aug 7, 2020

As far as the middleware stack goes, that was why I made before/after (of course they aren't working currently, but that's another thing).

The only option right now if you want to insert something into a certain point in the middleware stack is to copy it and put it in your app:

(-> (handler routes)
    (layout)
    (with-before-middleware)
    (with-after-middleware)
    (csrf-token)
    (session)
    (extra-methods)
    (query-string)
    (body-parser)
    (json-body-parser)
    (server-error)
    (x-headers)
    (static-files)
    (not-found)
    (logger))

I feel like it's pretty common though with more advanced apps, so I should do something like joy eject which will output that middleware stack or attempt to update main.janet maybe.

In the mean time yeah it's just adding the stack to your app

@swlkr
Copy link
Collaborator Author

swlkr commented Aug 8, 2020

I’ve been thinking more about the auth middleware and the before/after middleware(s)? as well.

The wildcard stuff is good, but I think it’s better to be more explicit, something like whole controller and individual routes too, maybe:

(before {:include [:things/thing1 :things/thing2 :todos] :exclude [:accounts]} :before-fn)

and then similarly for auth.

As for auth, I think it might be better to have an auth middleware but only for restricting which routes get accessed and setting the currently logged in account or team or whatever table you decide represents that.

The other part of the auth stuff will be generated, similar to route generation.

The ultimate goal is to have something like devise/omniauth where you can just set your providers, api keys and have at it.

@jcreager
Copy link

jcreager commented Aug 8, 2020

Now that you mention just setting up your own middleware stack if you need something more than the defaults I'm not sure why I didn't consider doing that myself. That seems reasonable to me.

Regarding include/exclude for the before/after middlewares, the only downside I see to that is cases where you really do want to cover a route with a wildcard. If I had a route that was /projects and it had sub routes like /projects/:id, etc, and I wanted to require login to view any of the sub routes, /projects/* is convenient in that case. On the other hand I like having an explicit list for applying the same function and I think this would work well in cases where you have different controllers for different routes that need the same middleware function. It gives you a lot of granularity in one place. I would advocate for being able to supply one or the other. Before/after middlewares can accept a string for route matching, or a map with the keys :include and exclude.

Another thought regarding middlewares is to allow middlewares to be injected into route declarations. In this case the last argument will always be the controller. Some examples below.

Instead of only accepting a controller.

(route :get "/" :foo)

Also allow an optional middlewares like.

(route :get "/" :middleware :controller)

Or maybe an optional list of middlewares.

(route :get "/" [:middlewares] :controller)

These types of middlewares would need to be set up like a regular middleware that acts on the request and response like the default middleware stack rather than ones that act on the request or the response like the before/after middlewares.

@swlkr
Copy link
Collaborator Author

swlkr commented Aug 8, 2020

The middleware in the route thing is interesting, I started down that path in coast and it’s coming up again here.

I like it and it could be even more useful if you had a macro that represented crud routes like this:

(resource :accounts [:mw1 :mw2])

which would do what you’re saying there:

(route :get "/accounts" [:mw1 :mw2] :accounts/index)
(route :get "/accounts/:id" [:mw1 :mw2] :accounts/show)
; rest of the 7 crud routes go here

It’s probably better to put the middleware in the routes than the routes in different middleware stacks which is what happens now.

@swlkr
Copy link
Collaborator Author

swlkr commented Aug 8, 2020

Also if the function signature were [] vs var args you could def different middleware stacks and put that in the route definitions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants