Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #2205 (closed enhancement: fixed)

Opened 9 years ago

Last modified 9 years ago

Provide a simple way to evaluate repoze.what predicates

Reported by: chrisz Owned by: Gustavo
Priority: highest Milestone: 2.0b6
Component: TurboGears Version: trunk
Severity: major Keywords: repoze.what predicates permissions
Cc:

Description (last modified by jorge.vargas) (diff)

There is no simple way to evaluate repoze.what predicates such as has_permission() inside the controller or inside a template. Such a feature would be useful for changing the behavior or displaying additional information in the template depending on the permissions, a very common requirement.

We already have related tickets #2172 and #2173, but #2172 does only cover one special case (and is not yet implemented) and #2173 is not simple enough.

There is a  thread on turbogears-trunk summing up the discussion and proposing different solutions for this problem. Unfortunately, there was not enough feedback yet to make a good decision which of the ideas should get implemented. Maybe there is also a better solution or a modifications of these ideas will do.

I think this is an important feature that should get into TG 2.0, so I've given this ticket a high priority. I's actually easy to implement, but we need to make an API decision here. Any comments on this ticket or on the thread mentioned above are highly appreciated.

The two ideas from that thread that need further discussion were numbers 3) and 4) which I will repeat here:

3) Using the standard boolean evaluation mechanism

Michael came up with the idea to overwrite the __nonzero__ method of predicates (side note, __nonzero__ is for boolean evaluation and thus was renamed to __bool__ in Python 3.0).

This method could for example raise an error when used in the form py:if="has_permission('manage')" without the evaluate call (as proposed in #2173). But we could also go a step further and let __nonzero__ automatically evaluate the predicate. The predicates would have a dual use then, both for require decorators (not immediately evaluated) and for py:if statements (immediately evaluated).

The following simple monkey-patch would allow this double usage of all repoze.what predicates in TG2:

from tg import request
from repoze.what.predicates import Predicate
Predicate.__nonzero__ = lambda self: self.is_met(request.environ)

Instead, we could also create a TG specific subclass of repoze.what predicates that allows this double usage, and also create copies of the existing predicates on the fly.

Both the monkey-patching (or subclassing and copying) and the double usage may appear a bit hackish and magical though. But I think it's pretty elegant and straightforward and the following idea works a bit magical, too.

4) Providing a special "access" object

A different solution is to add an access object to TG2 and make it a standard template variable that auto-evaluates predicates passed as attributes. Here is a possible implementation:

from tg import request
from repoze.what import predicates

class Access(object):
     """Environ-aware predicates evaluating immediately."""

     def __getattr__(self, name):
         predicate = getattr(predicates, name)
         if callable(predicate):
             def predicate_is_met(*args, **kwargs):
                 return predicate(*args, *kwargs).is_met(
                     request.environ)
             return predicate_is_met
         else:
             return predicate

access = Access() # make this a standard tg template variable

This would allow easy evaluation of all existing predicates in templates in the form tg.acess.has_permission('edit'). We could also provide a mechanism for including additional custom predicates in the access object.

Change History

comment:1 in reply to: ↑ description Changed 9 years ago by Gustavo

  • Cc me@… added
  • Owner changed from faide to Gustavo
  • Status changed from new to assigned

Hopefully with this time we'll get enough feedback before the b6 release. If not, it won't get included in TG 2.0 (maybe in TG 2.1).

Well, I'm afraid that I have to re-repeat my position on this here, so here I go...

First of all, I don't like either of the proposals because they are too hackish and too magical. We'll end up with "The TurboGears way of evaluating predicates", in addition to the standard way. It's like having the "The TurboGears way to do a SELECT with SQLAlchemy", "The TurboGears way to do a redirect", and so on.

However, I agree that such a functionality may come in handy for some people. I just don't like the two proposals above.

Either way, Option 4 would be the only acceptable because it's forward compatible with repoze.what v2.

comment:2 follow-up: ↓ 3 Changed 9 years ago by chrisz

Yes, unfortunately, if we cannot reach a broad consensus quickly, we'll have to set the milestone to 2.1.

I also don't like 4) too much, but 3) would be ok for me. It just needs proper documentation. I don't see a big problem in "the TG way vs. the standard way". It's just a facilitation of predicate usage and such simplifications are also implemented in other areas, e.g. object dispatch instead of routes, using decorators instead of rendering or validating manually, etc. I think we are only following the TG paradigm of making simple things simple and complex things possible. If everything would be standard, then we could just as well use Pylons or use now framework at all. I also don't see why 3) cannot not be made compatible with repoze.what v2, can you elaborate a bit on this?

comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 9 years ago by Gustavo

Replying to chrisz:

Yes, unfortunately, if we cannot reach a broad consensus quickly, we'll have to set the milestone to 2.1.

Well, strictly speaking, now I realize that actually we may use it in 2.0 although possibly not by default: Regardless of the decision we make on this, the solution is not going to be implemented in TurboGears itself. Instead I will implemented it in the  repoze.what-pylons package, an independent project which now TG uses for its repoze.what integration.

If we don't reach a consensus on time, we can still have the functionality in v2.0 from repoze.what-pylons.

I also don't like 4) too much, but 3) would be ok for me. It just needs proper documentation. I don't see a big problem in "the TG way vs. the standard way". It's just a facilitation of predicate usage and such simplifications are also implemented in other areas, e.g. object dispatch instead of routes, using decorators instead of rendering or validating manually, etc. I think we are only following the TG paradigm of making simple things simple and complex things possible. If everything would be standard, then we could just as well use Pylons or use now framework at all. I also don't see why 3) cannot not be made compatible with repoze.what v2, can you elaborate a bit on this?

Oh, I'm sorry, what I said about Option 4 was actually about Option 3, and vice versa. I'm getting crazy with so much proposals ;-)

So, by "Option 4 is not forward compatible" I mean that in repoze.what v2 predicates have been reorganized in different modules because that "groups/permission-based authorization pattern" won't be the supported pattern anymore, but yet another supported pattern, so predicates specific to that pattern are now defined in a different module.

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 9 years ago by chrisz

Replying to Gustavo:

Regardless of the decision we make on this, the solution is not going to be implemented in TG itself. Instead I will implemented it in the repoze.what-pylons package

Yes, Pylons could also benefit from such a mechanism, so that's a very good idea.

Oh, I'm sorry, what I said about Option 4 was actually about Option 3, and vice versa. I'm getting crazy with so much proposals ;-)

So, by "Option 4 is not forward compatible" I mean that in repoze.what v2 predicates have been reorganized in different modules because that "groups/permission-based authorization pattern" won't be the supported pattern anymore, but yet another supported pattern, so predicates specific to that pattern are now defined in a different module.

Ok, clear now. Though this could be solved by modifying the __getattr__() method so that it checks all available modules for the passed predicate name (merging all the names together), or alternatively by passing the module name and then the predicate name as subattribute, like access.permissions.has_permission('edit').

Btw, we have a similar problem with validators since some of them are now in the formencode.national submodule instead of the main module.

comment:5 in reply to: ↑ 4 Changed 9 years ago by Gustavo

Replying to chrisz:

Replying to Gustavo:

Regardless of the decision we make on this, the solution is not going to be implemented in TG itself. Instead I will implemented it in the repoze.what-pylons package

Yes, Pylons could also benefit from such a mechanism, so that's a very good idea.

Exactly :)

Oh, I'm sorry, what I said about Option 4 was actually about Option 3, and vice versa. I'm getting crazy with so much proposals ;-)

So, by "Option 4 is not forward compatible" I mean that in repoze.what v2 predicates have been reorganized in different modules because that "groups/permission-based authorization pattern" won't be the supported pattern anymore, but yet another supported pattern, so predicates specific to that pattern are now defined in a different module.

Ok, clear now. Though this could be solved by modifying the __getattr__() method so that it checks all available modules for the passed predicate name (merging all the names together), or alternatively by passing the module name and then the predicate name as subattribute, like access.permissions.has_permission('edit').

Yes, but still 3) will be easier to implement and use.

Do you agree that we go for 3)? It's so easy to implement that I'd do it tomorrow at the latest.

comment:6 Changed 9 years ago by jorge.vargas

+1 on the monkey patch (option3), it's what people will expect to work. Everything else is academic discussion.

comment:7 Changed 9 years ago by jorge.vargas

  • Description modified (diff)

comment:8 follow-up: ↓ 9 Changed 9 years ago by chrisz

I agree with you both. In case of doubt, we should choose the simpler solution, so +1 for 3) from me too.

Btw, this solution will have an additional benefit, namely if you make the mistake of writing

@require(in_group('editors') and has_permission('delete'))

instead of

@require(All(in_group('editors'), has_permission('delete')))

then you will get an error because the predicates will be evaluated, but there will be no request object at that time, while without the boolean evaluation, there would be no error, but it would behave like

@require(has_permission('delete'))

which is probably not what was intended.

I also suggest making repoze.what.predicates available as a standard template variable so that standard predicates can be evaluated more easily in templates.

comment:9 in reply to: ↑ 8 Changed 9 years ago by Gustavo

  • Priority changed from high to highest
  • Cc me@… removed
  • Version changed from 2.0b1 to trunk

Replying to chrisz:

I also suggest making repoze.what.predicates available as a standard template variable so that standard predicates can be evaluated more easily in templates.

OK.

comment:10 Changed 9 years ago by chrisz

Btw, just noticed solution 3) is not really new. It has been used in TG 1.x all the time (via the IdentityPredicateHelper mix-in class in turbogears.identity.conditions). Yet another reason for preferring this solution.

comment:11 Changed 9 years ago by mramm

I am tempted to consider this a blocker for b6, since it's an API change. Either we need to agree now, or we need to move this to 2.1. And I'm ok with the proposed solution #3 if that's the current consensus.

I do think it's what users expect, and I want to support that. I'm a little bit concerned about the magicality of it, but I don't see a super clear solution that's less magical.

comment:12 Changed 9 years ago by chrisz

Yes, this should be decided, but actually, I don't even think it's an API change since it was nowhere documented how __nonzero__ should behave for a predicate, and as I noticed above, predicates actually always behaved that way in TG (and it seems nobody ever complained or got confused).

comment:13 Changed 9 years ago by jorge.vargas

+1 on nonzero

comment:14 follow-up: ↓ 15 Changed 9 years ago by Gustavo

OK, I got the function implemented in repoze.what-pylons (it's called booleanize_predicates()).

I suggest that such a function is called in the user's code, instead of having TG calling it automatically, to give make clear what is going on. What do you think about it?

comment:15 in reply to: ↑ 14 Changed 9 years ago by Gustavo

Replying to Gustavo:

OK, I got the function implemented in repoze.what-pylons (it's called booleanize_predicates()).

I suggest that such a function is called in the user's code, instead of having TG calling it automatically, to give make clear what is going on. What do you think about it?

This is, in a place like {app}.config.app_cfg, we could have:

# (...)
from repoze.what.plugins.pylonshq.utils import booleanize_predicates
# (...)
booleanize_predicates()
# (...)

I think it's important to make it clear that there's no magic behind.

comment:16 follow-up: ↓ 18 Changed 9 years ago by chrisz

I don't see repoze.what.plugins.pylonshq.util in SVN yet, have you checked it in?

comment:17 follow-up: ↓ 19 Changed 9 years ago by mramm

I'm not super excited about adding more code to quickstart.

And I think this is the kind of thing that will confuse rather than iluminate our users ;)

Perhaps we could put it somewhere else?

comment:18 in reply to: ↑ 16 Changed 9 years ago by Gustavo

Replying to chrisz:

I don't see repoze.what.plugins.pylonshq.util in SVN yet, have you checked it in?

I hadn't checked it in because I had not updated the docs. But now it's in.

comment:19 in reply to: ↑ 17 Changed 9 years ago by Gustavo

Replying to mramm:

I'm not super excited about adding more code to quickstart.

And I think this is the kind of thing that will confuse rather than iluminate our users ;)

Perhaps we could put it somewhere else?

I can call it from tg.configuration:AppConfig.add_auth_middleware(). Then people who configure repoze.who/what by themselves would have to run it themselves.

comment:20 follow-up: ↓ 21 Changed 9 years ago by mramm

Sounds sensible

comment:21 in reply to: ↑ 20 Changed 9 years ago by Gustavo

  • Status changed from assigned to closed
  • Resolution set to fixed

Replying to mramm:

Sounds sensible

OK.

Finally fixed in [6347]! ;-)

comment:22 follow-up: ↓ 23 Changed 9 years ago by chrisz

Looks good. (I'd prefer enable/disable_bool_for_predicates or simply enable/disable_bool over booleanize/debooleanize_predicates. We are only activating the bool() function for predicates, not making booleans from the predicates as such a name suggests. But I can also live with the current names ;-)

comment:23 in reply to: ↑ 22 ; follow-up: ↓ 24 Changed 9 years ago by jorge.vargas

Replying to chrisz:

Looks good. (I'd prefer enable/disable_bool_for_predicates or simply enable/disable_bool over booleanize/debooleanize_predicates. We are only activating the bool() function for predicates, not making booleans from the predicates as such a name suggests. But I can also live with the current names ;-)

agreed, booleanize sounds like it's complicated. +1 for enable/disable, it will also be a good idea to get a better comment :)

Totally unrelated, but since you are paying attention here. Why is the pylons stuff named pylonshq? pylonshq is the pylons site.

comment:24 in reply to: ↑ 23 ; follow-up: ↓ 25 Changed 9 years ago by Gustavo

Replying to jorge.vargas:

Replying to chrisz:

Looks good. (I'd prefer enable/disable_bool_for_predicates or simply enable/disable_bool over booleanize/debooleanize_predicates. We are only activating the bool() function for predicates, not making booleans from the predicates as such a name suggests. But I can also live with the current names ;-)

agreed, booleanize sounds like it's complicated. +1 for enable/disable, it will also be a good idea to get a better comment :)

I think that from a I-dont-care-how-its-implemented point of view, it's just making predicates booleans. ;-)

Totally unrelated, but since you are paying attention here. Why is the pylons stuff named pylonshq? pylonshq is the pylons site.

Because I was getting import errors, even if the dist name was "repoze.what-pylons":

from repoze.what.plugins.pylons import something
ImportError: pylons.something doesn't exist (or something similar).

I'd have used another name, but I didn't know of another way to refer to "Pylons" as.

comment:25 in reply to: ↑ 24 ; follow-up: ↓ 26 Changed 9 years ago by chrisz

Replying to Gustavo:

I think that from a I-dont-care-how-its-implemented point of view, it's just making predicates booleans. ;-)

Not really. They can be evaluated as booleans, but they are no booleans, similar to lists and strings which are false if they are empty.

Because I was getting import errors, even if the dist name was "repoze.what-pylons":

Maybe a general problem with namespace packages? I think we should look into that.

comment:26 in reply to: ↑ 25 ; follow-up: ↓ 27 Changed 9 years ago by Gustavo

Replying to chrisz:

Replying to Gustavo:

I think that from a I-dont-care-how-its-implemented point of view, it's just making predicates booleans. ;-)

Not really. They can be evaluated as booleans, but they are no booleans, similar to lists and strings which are false if they are empty.

What I meant is that they can act as booleans.

Anyway, I see no big deal with those tiny functions which most people won't even use directly.

Because I was getting import errors, even if the dist name was "repoze.what-pylons":

Maybe a general problem with namespace packages? I think we should look into that.

I've heard so. I hit that problem while developing the repoze.who SQLAlchemy plugin: hence it's called repoze.who.plugins.sa instead of repoze.who.plugins.sqlalchemy.

comment:27 in reply to: ↑ 26 ; follow-up: ↓ 28 Changed 9 years ago by chrisz

Replying to Gustavo:

I've heard so. I hit that problem while developing the repoze.who SQLAlchemy plugin: hence it's called repoze.who.plugins.sa instead of repoze.who.plugins.sqlalchemy.

Yes, I already noticed that and found it confusing.

comment:28 in reply to: ↑ 27 Changed 9 years ago by jorge.vargas

Replying to chrisz:

Replying to Gustavo:

I've heard so. I hit that problem while developing the repoze.who SQLAlchemy plugin: hence it's called repoze.who.plugins.sa instead of repoze.who.plugins.sqlalchemy.

Yes, I already noticed that and found it confusing.

This is the exact same reason why tg.ext was drop. The thing is that 1 namespace package is uncommon and potentially buggy, two namespace packages will cause havoc. Remember repoze is a namespaced package. and what.plugins will cause havoc :)

Note: See TracTickets for help on using tickets.