Allow for more flexible access rules declarations

[da-z]

Makes sure every rule passes (not just some), picks the result of the first failing rule (we are mostly interested in the "Location" header)

[yogthos]

I'm not sure I agree that having every rule pass is better. For example, this way you wouldn't be able to create whitelists for routes.

Say you wanted some routes to be unavailable to regular users, but make them available to the admin user. You could have a general rule that redirects, and have an admin rule that overrides it.

[da-z]

I am not sure I am understanding your use case but can it be solved by creating a :rule that verifies the role of the logged user and returns accordingly?

I think this is a good time to discuss how the vector specified by :rules is evaluated. I was assuming that noir checks that all those rules in the vector return true. It does not seem to be the case though.

To eliminate confusion I'd like to propose specifying a single rule (predicate), as :rule (singular) instead, which gives control back to the user. In my application I find the general case to be a single rule, and two or more rules the exception, in which case we can anyway use the language to our advantage to combine functions (rules)

What do you think ?

[yogthos]

The way the current approach works is by checking all the rules that are applicable to a particular URI and if any of the rules return true then the URI is accessible.

The rules can be specified either as a global or as a group. If you wish to create a global rule, then you simply provide a function that accepts the request and returns a truthy value, eg:

:access-rules [(fn [req] (session/get :user))]

All such rules will be checked for every restricted URI and redirect to "/". So, if your application only has a single rule that you'd like to be checked for all restricted routes, you don't need to specify anything else.

You can also create rule groups by creating a map that contains :redirect, :uri and :rules keys.

If :redirect is specified then all the rules in this group will redirect there. If :uri is specified then the rules in the group will only be checked for that URI pattern. The :rules key points to a vector of rules that apply to this group.

So, let's say we wanted to have rules that only affect the "/admin/*" pattern, then we could write:

{:uri "/admin/*"
 :rules [(fn [req] (session/get :admin))]}

This group will be ignored for any other URI pattern and succeed if there is an :admin key in the session.

[yogthos]

I feel that switching to the approach of every rule having to pass would make it more difficult to reason whether a URI will be available or not.

[da-z]

Thank you for explaining this. I already knew this from the documentation and your blog. My issue is with the choice of "if any of the rules return true then the URI is accessible". Let me show you my access rules

(wrap-access-rules [{:uri "/account/sign-in" :rules [(complement is-logged-in)]} ; no reason to sign in if already logged
                    {:uri "/account/sign-up" :rules [(complement is-logged-in)]} ; no reason to sign up if already logged
                    {:uri "/account/settings*" :rules [is-logged-in] :redirect "/account/sign-in"}
                    {:uri "/admin*" :rules [is-logged-in] :redirect "/account/sign-in"}
                    {:uri "/admin*" :rules [is-merchant]}
                    {:uri "/admin*" :rules [has-stores] :redirect "/welcome/merchant"}
                    {:uri "/welcome/merchant" :rules [is-logged-in] :redirect "/account/sign-in"}                    
                    {:uri "/welcome/merchant" :rules [is-merchant (complement has-stores)]}])

As you see, the last rule is where I wanted to use multiple access restrictions, and I was expecting them to be all evaluated. The "/welcome/merchant" should be accessible to all merchants that don't have a store yet, as this page allows them to create their first one.

I find that it is cumbersome to wrap the rule in a vector when most of the time I have a single rule. Instead, I would have found more useful the ability to pass multiple :uri patterns at once, in a vector, for the is-logged-in rule, as they all need the same :rule and same :redirect.

PS: I know I could have written all the is-logged-in groups a global rule, but I like the readability and consistency of the rule groups API. As I said, allowing vectors for :uri and a single :rule instead of rules would simplify things in my opinion.

[yogthos]

it sounds like it might make sense to have keys for both single and plural definitions for the uri and the rules, so you could do:

{:uris ["/account/sign-in" "/account/sign-up"]
 :rule (complement is-logged-in)}

That certainly seems reasonable to me, I'd be happy to add this change in.

As I mentioned, my main concern with having all the rules match is that it could be difficult to reconcile all the rules that affect a particular route. Right now, as long as you specify a single rule that makes it accessible then you don't need to find other rules that affect it. I guess it's a trade off either way.

[da-z]

I really like your idea of having both single and plural. If you have multiple :rules, then you also need to allow for flexibility in interpretation, like declaring the algorithm too. I see the following possible declarations:

{:rule my-rule} ; for most of the cases
{:rules [rule1 rule2]} ; equivalent of {:rules {:every [rule1 rule2]}}
{:rules {:every [rule1 rule2]}}
{:rules {:any [rule1 rule2]}} ; your case
{:rules {:every [rule1 rule2] :any [rule3 rule4]}} ; rule1 and rule2 and at least one of rule3 or rule4

[yogthos]

That seems quite reasonable to me, this way you can do whatever approach makes sense for a particular group.

[da-z]

As for the :uris, I think the route patterns can be pre-compiled behind the scenes, using compojure's compiled route fn. One more advantage against letting the user invent his own mechanism. Not sure though if this brings any performance improvements.

[yogthos]

Hmm, I'm actually leaning towards simply having a top level keyword :every that will affect the group:

{:rules [rule1 rule2]
 :every true}

If specified then each rule will have to pass as opposed to some. I'll take a look at implementing that today then.

As for :uris, right now it's using clout.core/route-matches to check the route, but if you give an example of what you're thinking I could add that as well.

[yogthos]

I updated the rule handling with this pull:

https://github.com/noir-clojure/lib-noir/pull/67/files

I added the :uris and :rule keys as well as :match-all key that allows specifying that all rules should pass.

[yogthos]

@da-z ok so I updated the rules to work as you suggested using :every and :any keys. If you're happy with the change I'll close the issue and push out a new release.

[da-z]

Excellent. Thank you very much!

I've fixed a couple of things on top of your latest commit, and added some tests. I've also found recently that :uris does not behave as expected. Please check the failing test. We are almost there..

[da-z]

Oops. Looks like I've made one change too many as part of the e01f634 commit. Reverted back that code in f0583eb

Sorry about that. But hey, we have a test for it now :)

[yogthos]

ok so I've got the changes in and made a new release. I personally would still have preferred some as the default, but I don't really care that strongly about it. :)

[da-z]

Thank you very much. I really appreciate it and I'm hoping other people will find this functionality as useful as I do.

[yogthos]

Glad to see it all worked out and hopefully now it's robust enough to fit most needs. :)