Weird problem...what don't I understand about routes?

[Sean Bowman]

I'm trying to wrap some custom authentication middleware around a
selection of my routes. Here's a sample of what I've done:

(defroutes my-routes
(resources "/")

(ANY "/login" request (authentication/form request))
(GET "/test-before" [] "Before")

(wrap-authentication
(routes
(with-name :login (ANY "/login" request (authentication/form
request)))))

(GET "/test-after" [] "After")

(not-found "Page not found"))

What's strange is that any routes defined before the "wrap-
authentication" routes work correctly, but anything after that line
gets run through the wrap-authentication handler, even though it's not
part of the wrap-authentication routes. For example, /test-before
returns a simple page with "Before" on it, but /test-after redirects
to my login form (my wrap-authentication redirects to /login). Or if
I just make up a route that doesn't exist, I never get to the not-
found handler. Instead I'm redirected to /login.

I'm obviously missing something here. Why is it doing this? Is this
a bug, or am I going to have to run all my routes through my
authentication handler and filter with regular expressions (which
seems hugely inefficient)?

[Яков Макаров]

I think that happens, because handlers are applied  in series. So you first check authentication in wrap-authentication and then match path.

2012/5/25 Sean Bowman <pic...@gmail.com>

--
You received this message because you are subscribed to the Google Groups "Compojure" group.
To post to this group, send email to comp...@googlegroups.com.
To unsubscribe from this group, send email to compojure+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/compojure?hl=en.

[Sean Bowman]

It works if I wrap the (wrap-authentication) call in a method or context.  For example (and my original example was a little messed up; don't want to require authentication to /login):

(ANY "/login" request (authentication/form request))

(context "/admin"

  (wrap-authentication

   (routes

     (GET "/" [] (admin/index)))))

I'm obviously not doing something quite right in my wrap-authentication function to have it be at the top level, but this is ok for now.  Maybe I should turn it into a macro instead that wraps each handler inside of it in an authentication request.

[gaz jones]

When I have done this in the past, I have added the middleware the app var:

(def app
(-> #'routes
wrap-authentication))

In the wrap-authentication I would only apply authentication to routes
that matched a list of regexs. For example, passing them to the
wrap-authentication call:

(-> #'routes
(wrap-authentication [#{"something"}]))

Or defining them in a var somewhere.

Cheers,
Gaz

[Sean Bowman]

Yeah, but what I don't want to do is have my app running through a lot of regular expressions for every single request.  That seems really inefficient.  Maybe I could follow your suggestion, but use .startsWith instead of a regex.  

I would suggest if anyone does go the regular expression route, I don't think you should embed the regex in your call.  Instead it's probably better to do something like:

(def auth-routes [#"something"])  

(-> #'routes

      (wrap-authenticate auth-routes))

That way the compiler can compile the expressions ahead of time.  I think your way means that each request has to recompile the expressions--unless the Clojure compiler is doing something very tricky and hugely helpful.  Is Clojure that smart, anyone?

[James Reeves]

On 26 May 2012 17:13, Sean Bowman <pic...@gmail.com> wrote:
> I'm obviously not doing something quite right in my wrap-authentication
> function to have it be at the top level, but this is ok for now.  Maybe I
> should turn it into a macro instead that wraps each handler inside of it in
> an authentication request.

What does the wrap-authentication function look like?

- James

[Nick Bauman]

I can't see any difference in speed FWIW:

=> (time
(loop [x 10000000
found 0]
(if (> x 0)
(if (re-find #"[0..9]" (str x))
(recur (dec x)
(inc found))
(recur (dec x)
found))
found)))
"Elapsed time: 3279.919 msecs"

7603256

(def rex #"[0..9]")

=> (time
(loop [x 10000000
found 0]
(if (> x 0)
(if (re-find rex (str x))
(recur (dec x)
(inc found))
(recur (dec x)
found))
found)))
"Elapsed time: 3296.086 msecs"

7603256

[Bryan Deter]

The reason why your initial attempts don't work is because your authentication middleware returns something other than nil when none of the underlying routes match, which causes ring to assume that that route matches.  This means that it won't try any other routes.

When you wrapped in the context, it first attempts to match to the context and then, only if the context matches, tries applying the middleware.  If there were other routes under admin that didn't require authentication, they wouldn't appear to work.

Using the context is a good choice, and can offer some clarity.  Another path is, in the wrap-authentication middleware, you append to the request the authentication status, even if not authenticated.  Then, in the routes that need authentication, you can check for authentication and redirect as necessary (perhaps passing in the path which the user is trying to reach as well).

2012/5/25 Sean Bowman <pic...@gmail.com>

To unsubscribe from this group, send email to compojure+unsubscribe@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/compojure?hl=en.

--
You received this message because you are subscribed to the Google Groups "Compojure" group.
To post to this group, send email to comp...@googlegroups.com.

To unsubscribe from this group, send email to compojure+unsubscribe@googlegroups.com.

2012/5/25 Sean Bowman <pic...@gmail.com>

To unsubscribe from this group, send email to compojure+unsubscribe@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/compojure?hl=en.

--
You received this message because you are subscribed to the Google Groups "Compojure" group.
To post to this group, send email to comp...@googlegroups.com.

To unsubscribe from this group, send email to compojure+unsubscribe@googlegroups.com.

[Sean Bowman]

Here's my wrap-authentication handler:

(defn wrap-authentication)

  [handler]

  (fn [{session :session :as request}]

    (if-let [user (users/find-dn (:user session))]

      (handler (assoc request :user user))

      (redirect (path/to "/login" {:uri (:uri request)})))))

The :user value in the session will be the LDAP distinguishing name of the user, and users/find-dn looks up that user's account by his or her distinguishing name.  

I think the issue you're describing Bryan focuses on my confusion.  I need to go back and really think about the -> macro, and what it's doing.  Let's see if I understand now...

Using my original attempt:

(->

  (GET "/test-before" [] "Before")

  (wrap-authentication

    (routes

      (ANY "/protected" request (authentication/form request))))

  (GET "/test-after" [] "After"))

What's really happening here is the Compojure routing is first calling that GET "/test-before", which calls the wrap-authentication, which calls the GET "/test-after".  But because my wrap-authentication function redirects if the user isn't found, it never gets to the GET "/test-after" method.  Instead, what I should do is make sure that some route inside my (wrap-authentication (routes ...)) matches (not sure how I do this, but I suppose I can look at context for an example).  If so, then I should authenticate.  If authentication succeeds, I handle the route; if it fails, I redirect.  But if the path doesn't match one of my routes, I should just return nil, so then the GET "/test-after" will be called.

Is this correct?

To view this discussion on the web visit https://groups.google.com/d/msg/compojure/-/eLMJkLl5HAAJ.

To post to this group, send email to comp...@googlegroups.com.

To unsubscribe from this group, send email to compojure+...@googlegroups.com.

[Bryan Deter]

You have the right general idea.  Note that Compojure isn't using the -> macro, it uses something like some?, which will test handlers until one matches, and the way it tells if one matches is by checking that the return value isn't nil.

While I'm not seeing much, most of what I am seeing recommends putting all the authenticated routes under particular context paths and forcing authentication for everything under that path (even a 404).  The only real alternative would be to check for authentication on a per-route basis.  If you do the latter on more than a few requests, I'd write a small macro to help (perhaps by automatically doing so on every request in a set of routes).

The way the context macro works is by doing a specialized match on the beginning of the url and "shifting it" for any requests under it as a handler.

Bryan Deter