Feature Proposal: Plugin hooks to permission checking

(Note: I'm reviving this discussion with a new proposal that has the same objective but with a different approach. See the previous proposal and discussion here.

Motivation

This feature was motivated by the need to be able to say 'only allow the author of this topic to edit it', ie. role-based permissions. Other options i want to implement using this feature in the near-future is to have 'draft' - 'published' - 'revoked' kind of status on topics.

This kind of extensibility is only allowed if Extensions are able to "have a word" in the Authorization of actions.

Description and Documentation

I would like to implement an "Authorization chain". The TWiki::Access module will be responsible to start this chain, which will be composed of several Authorization handlers/modules. Each handler/module will check if the use is allowed to perform the required action. If it cannot determine it, it will call the next handler/module in the chain. If there are no more handler/modules in the chain, then the "Unauthorized" exception will be thrown.

This feature, alongside PluggableAccessControlImplementation, will allow nearly total flexibillity. The admin will select the "root" handler, and Extension would register additional handlers.

What I'm not sure (yet) is what the interface should look like. I'll use the example below, and the needs from the core, WorkflowPlugin and XpTrackerPlugin as the base to extract this inferface, and then we can discuss.

Examples

Eg, to be able to say

* Set ALLOWTOPICCHANGE = Role:Author

Impact

Implementation

Attached experimental patch.

-- Contributors: KoenMartens - 14 Feb 2008

Discussion

Show old discussion... Hide Some clarifying questions and remarks.

The proposal is not for or against role-based permission checking. The proposal is to add a handler that enables writing plugins that can implement this kind of features.

My question is - does this proposal match the plans for Twiki 5.0 to have things like access rights in a database based storage?

Ie does the new handler give the plugin an abstract representation of the assigned access rights so that plugins written for this handler does not regex its way though set variables and meta data.

-- KennethLavrsen - 21 Feb 2008

There are 2 alternatives I can think of (that may not just lead to a better separation of concerns, but may be faster)

1. Implement the Roles as pseudo-groups that can be evaluated, and thus implemented in a UserMapping
2. implement it as a TWikiVariable

There might be other options too

There are other consideration -

• from a language design standpoint, adding a new context sensitive grammar, that uses a totally different operator style (essentially pre2000 Name:Value pair) as a substitution macro, might not be a desirable consequense.
• From a security standpoint, do we really want to make it trivial for any random plugin to inject things into the permission stream? Again, it may be safer to relegate this functionality to the UserMapping system which a TWikiAdmin will hopfully consider more carefully before installing.

-- SvenDowideit - 22 Feb 2008

The proposal is to have plugins be able to extend the range of permission-related constructs. When i sat down to implement this (because it was a requirement for a system i was building), i first thought about having a method that asks the plugin just 'is this user allowed to FOO this topic?'. That plugin would then however have to parse the topic and look for the ALLOWTOPICCHANGE itself, while the core code already did this. Actually, this is maybe an argument for having the 'parse tree' in whatever form being available in a DOM like manner to plugins.

I'm quite open to suggestions on how to improve this proposal. Is there some documentation on UserMapping? I've done a quick search on Codev, only to get lost as usual. For me, UserMapping is so far something quite mysterious, it was discussed and developed right before i became active so it hasn't been on my WebChanges radar yet.

The 'Role:Author' syntax is not relevant to this proposal. It was used in an example, and is just a string. The ':' has no semantics. It could have been 'AuthorRole', 'TopicAuthor', whatever..

To Kenneth's question: i don't think we have a picture of how things would be stored in the database yet. Perhaps now is a good time to start thinking about it, should we declare a feature freeze and resolve these issues first. If proposals are going to be judged against this, we need to define what it is they are judged against. Else it is a moving target, that does not work well.

Lastly, roles are just one thing i could think of implementing when permissions are pluggable. If we reject pluggable permissions, decoupled from my current proposal, outright for security reasons then we'll have to build such things into core. Roles are something i have seen being asked for on irc a couple of times, more CMS like status of topics (draft, published, retracted, ...) another. Both can be implemented by having pluggable permissions. If we don't want that, we need to implement them in core. Fine by me too, and i'll happily do so. I just thought it'd be nice to be able to implement your own permission scheme without having to hack core. Maybe plugins that use this could come with some mechanism warning at installation time that the plugin might change how permissions are handled?

-- KoenMartens - 22 Feb 2008

Koen, we are for sure not on a feature freeze. And I personally have no concerns with having a good API that enables plugins to enhance or limit the access rights features. I'd rather see access rights enhancements in plugins than in the core. So I support the spirit of this feature proposal 100%. My goal is to get the definition of the API to the new handler defined so it will not have to be changed again or will block a good implementation of the storage model. We know 3 things.

• New plugin hooks belong to the official API. Once in, we cannot remove it. So we'd better get it defined right.
• Next release is 5.0 and the release focus is "performance, performance, performance" to use your own words. One of the known performance issues is the way permissions are handled and one of the first things we will implement in a new storage model. We do not need to know the storage model to design this new handler.
• It is obvious that in the future access rights will not be handled by parsing .txt files.

So the implementation of the information sent through this new handler needs to be made so the plugin and core exchange access rights data in a way that is totally transparent to the way the actual access rights are stored. Ie. the plugin should not know if the setting comes from a ALLOWTOPICCHANGE or from a meta setting or from WebPreferences etc.

The decision who to give access needs to happen in the plugin somehow. That is the purpose anyway. So somehow the exchange of information between core and plugin should be that the core says what it sees as the current chain of allow and deny and the plugin responds with its interpretation. Either by a go/nogo or by modifying some exchanged hash of access rights. Do you see where I want to go?

If we look genericly at access rights and performance then one of the "nice AND horrible" features is that access rights since early TWiki releases can be embedded inside the topic text. Later this was enhanced by enabling defining access rights in the meta data. It is nice because it is flexible and nice for humans to deal with in some cases. And horrible because it requires parseing a lot of text files each time you view a topic and because it is also difficult to grog for users. But for compatibility reasons this cannot be changed now.

In a new storage model it is obvious that when you save a topic, the actual access rights will be stored in a database in tables designed for access rights. These access rights tables will contain both topic level access rights as well as web and site level rights. And naturally groups will be defined in tables as well. This will enable the core code that handles access rights to decide if you have access or not very quickly by simply database lookups.

So the design task on this feature proposal is to define this handler so it will not assume that the .txt files are parsed and does not work by replacing ALLOWTOPIC or DENYTOPIC text in the topic because in future we will most likely never parse these but interpret them when the topic is saved and store the information in a database.

-- KennethLavrsen - 22 Feb 2008

I have raised concern. I would like to see the API spec defined to match the needs for 5.0 per our roadmap before this gets accepted by consensus. Once this is complete I expect this to be accepted by consensus.

-- KennethLavrsen - 28 Feb 2008

So, how about this more generic handler:

permissionCheckPostHandler($session, $mode, $user, $text, $meta, $topic, $web);

There's actually two: Post and Pre, the former being called AFTER normal twiki permission checks are done, the latter before.

The plugin gets all the information available to the core access checks, and it is up to the plugin how to validate access. This could be by getting stuff out of $meta (which i imagine could be an API into the database store at some later point, it is already sufficiently abstracted that the plugin doesn't need to worry about whether it is parsed from topic text or retreived from db. Of course, the plugin could (if it wanted to be slow) parse the topic text yet again..

I'll present a patch later on, probably by monday, let me know if this direction of thinking would work for you.

One sidenote on having the access rights in meta: in the past i have discovered bugs in this, where those stored in meta simply got ignored in some cases, revealing data to users that should have no access to it. At that time, those bugs got fixed afaik, but there might be more. I'm not really trusting those, and the fact that you have acces rights in both topic text and meta is a serious complication making it very unclear to most people who is allowed to do what when..

-- KoenMartens - 01 Mar 2008

Perhaps a note on what the plugin handler returns:

<>0 - allow $mode access 0 - deny $mode acces undef - neither allow nor deny $mode access (mostly useful for Pre handler, ie let normal access control apply)

-- KoenMartens - 01 Mar 2008

Hmm, actually, the more i look into this, the more i am convinced i should just drop it. TWiki is not suited for extensions like this without sacrificing performance, because the backward compatibility mantra has introduced so many complications. Ghaa, what a mess

-- KoenMartens - 01 Mar 2008

Ok, gonna abuse this topic for some note taking, will probably refactor at some later point.

Note, why is permission checking done over and over, i've started logging permission checks, and i see the same permission check (VIEW) carried out at least three times for a single page view.

Idea: why not cache the results of access evaluation for the given session?

-- KoenMartens - 01 Mar 2008

I've added myself as concerned - but I htink my concern about security would be mitigated, by not making it a PluginHandler, and instead using or creating a Plugable security evaluation module. Something that the admin has to intentionally select in the Security section of configure, not just download and enable like 'kinda more harmless' plugins.

-- SvenDowideit - 18 Mar 2008

Ok, that is not something i plan on doing, so therefore this feature request is now officialy dead.

-- KoenMartens - 26 Mar 2008

Changed to rejected (by proposer himself).

-- KennethLavrsen - 30 Mar 2008

I collapsed the old discussion into a Twisty until I got around refactoring it.

-- RafaelAlvarez - 19 Aug 2008

I am setting this proposal to parked until owner found and spec is defined.

-- PeterThoeny - 2009-04-20