Feature Proposal: Finish off the deprecation of the empty DENY* Rules started back on TWiki

Motivation

This is long overdue and has been implemented by TWiki 6, so we need to be somewhat compatible. It also eliminates an extremely confusing and error prone part of AccessControl, which has the following to say on this subject:

Setting to an empty value has caused confusion and great debate and it has been decided that the empty setting syntax will be replaced by something which is easier to understand in a later version of Foswiki. A method to upgrade will be provided. Please read the release notes carefully when you upgrade.

...

1. If DENYTOPIC is set to empty( i.e. Set DENYTOPIC =)
   • access is PERMITTED i.e no-one is denied access to this topic.
     

     Attention: Use this with caution. This is deprecated and will likely change in the next release.

After the long discussion and confusion on the SmartGroups proposal, this is a narrow focused proposal specifically intended to mitigate the need for the use of DENY* settings within the ACLs. It is not intended to represent a "group" of users. Any use of the word group or implication of the existence of a group is a typo. This is an ACL change. The TWiki implementation made the unfortunate choice of representing these conditions by a "pseudo-group" however as seen in the SmartGroups discussion, it is extremely confusing to overlap GROUP concepts onto an ACL requirement.

Description and Documentation

Eliminate the empty DENY syntax

Add an ACL that expresses ALLOW All access to this topic. While it's possible to think of this in terms of a group, this proposal discourages that thinking. This is a "wildcard" match representing ANY endpoint access.

(withdrawn) Reduce the need for the DENY syntax.

Foswiki will continue to recommend the DENY=WikiGuest instead of adding the (confusing) AllAuthUsersGroup

It is likely that the 2nd most common use of the DENY rule is for expressing ALLOW Any identified in user access to this topic. The choice of the term identified is important because not all identified users have gone through a traditional login or authentication process. They might use client certificates, or any other mechanism implemented through custom login managers to identify a user. Possibly even their IP address. This is a "wildcard" match representing ANY endpoint that is identified, regardless of how they were identified. It is equivalent to denying the WikiGuest.

Other requirements

• The actual ACL setting should be configurable: They represent an important concept to end users so it's probably important that they be expressible in the prevalent language in use by the installation. Withdrawn
• It should not usable within groups: It should not be possible to create a group that contains the wildcard. This is only an ACL setting. (This is not to say that the core must prevent a Group from being created with one of these wildcards, only that they are not operational there. It is desirable however that the Group API block this string.
• It is not a fully functional wildcard, you can't specify Joe* to include any user starting with the name Joe.

implementation

Accept the wildcard character "*" in any of the DENY or ALLOW settings. It is equivalent of listing every possible user.

• In the ALLOW setting, this is equivalent of listing every possible user including the WikiGuest. It is also the equivalent to setting DENY = (the empty string
• In the DENY setting, it denies everybody. The only exception being the AdminGroup which always has complete access.

The current hierarchy of DENY/ALLOW remains. It is not possible to override a DENY with an ALLOW. DENY rules are checked first, If DENY matches, the user is rejected and no consideration of ALLOW is given. That will not change with this proposal, so a "DENY=*" in the topic will lock down a topic without exception. A DENY=* at the Web level however will be subject to override in individual topics, and inherited / overridden in subwebs. There should be no functional change in this area.

The removal of the empty DENY processing is rather trivial to make optional. This will make migration for existing sites a bit less painful.

# **BOOLEAN**
# Optionally restore the deprecated empty DENY ACL behavior.   If this setting is enabled, 
# the "Empty" DENY ACL is interpreted as  "Deny nobody", which is equivalent to "Allow all".   
# It is recommended that this setting remain disabled,  and that
# these rules be replaced with the  * wildcard on the ALLOW setting:
# <pre>
#    * Set DENYTOPICVIEW =        Should be replaced with:
#    * Set ALLOWTOPICVIEW = *
# </pre>
$Foswiki::cfg{AccessControlACL}{enableDeprecatedEmptyDeny} = $FALSE;

TWiki Compatibility

If the TWikiCompatibilityPlugin is enabled, core should also match the AnyUserGroup and AnyAuthUserGroup strings used as wildcards in TWiki. These strings will not be checked by default and the corresponding group topics will not be created.

Examples

   Set ALLOWTOPICVIEW =  * This is the equivalent of  Set DENYTOPICVIEW =   or the TWiki implemenation Set ALLOWTOPICVIEW = AnyUserGroup
   Set DENYTOPICVIEW = * This is the equivalent of  Set ALLOWTOPICVIEW = Main.AdminGroup

Impact

Elimination of the empty DENY syntax has the most significant impact. There are a couple of possibilities to mitigate this:

• Leave the empty DENY syntax alone. This leaves the system vulnerable to typos if the setting is accidentally made in a topic.
• Allow the empty DENY syntax to be enabled by configuration, allowing a site to migrate. Disabled by default. This is added complexity.
• Ship a utility to convert existing empty DENY rules into the equivalent ALLOW. This has the side effect of checking in a new revision to any topic and updating the date last modified to the current date. This potentially misleads users by showing a recent change date that was made for administrative purposes. This was the TWiki solution. We might consider an option to record the change as occurring in the past, for example the last topic change date plus some seconds.

It might also be useful to include a beforeSaveHandler to warn if a user has used the invalid DENY= syntax.

íITTABLE{format="|label,1|text,70|" changerows="off"}%

WhatDoesItAffect:

%WHATDOESITAFFECT%

Implementation

-- Contributors: GeorgeClark - 08 Apr 2014

Discussion

Let's better not use preference settings as they might be overridden on so much levels that it is bound to create a security problem.

This is going to extend the BaseUserMapping which alread is customizable with regards to the IDs in use. See the definitions in Foswiki::Users::BaseUserMapping for ºSE_USERS% and ºSE_GROUPS%. These define the IDs and their mappings currently in use.

BASE_USERS:

CUID	Login	WikiName
BaseUserMapping_111	ProjectContributor	ProjectContributor
BaseUserMapping_222	$Foswiki::cfg{Register}{RegistrationAgentWikiName}	$Foswiki::cfg{Register}{RegistrationAgentWikiName}
BaseUserMapping_333	$Foswiki::cfg{AdminUserLogin}	$Foswiki::cfg{AdminUserLogin}
BaseUserMapping_666	$Foswiki::cfg{DefaultUserLogin}	$Foswiki::cfg{DefaultUserLogin}
BaseUserMapping_999	unknown	UnknownUser

BASE_GROUPS:

Name	Members
$Foswiki::cfg{SuperAdminGroup}	BaseUserMapping_333
BaseGroup	BaseUserMapping_111, BaseUserMapping_222, BaseUserMapping_333, BaseUserMapping_666, BaseUserMapping_999
NobodyGroup

To implement any user and any known user, we should continue in this design by defining the relevant IDs.

-- MichaelDaum - 08 Apr 2014

I strongly object to the use of Group. If it's a group, then users are going to want to know the membership of the group. It's not a group. it's AnyUser. period. no group.

My only purpose of the setting was to get translation. From an ACL perspective %ANYUSER% is a fixed string, not a macro. It displays as a macro when the group is viewed.

-- GeorgeClark - 08 Apr 2014

It is a user id or a group id. I don't mind.

-- MichaelDaum - 08 Apr 2014

Restarting the discussion on the revised proposal.

I don't mind group or user either, or even *.

If we do go for a name then AnyUser is pretty good. I have thought about the some time ago and wondered about EveryUser instead. I think I convinced myself that was better than AnyUser. I tried to imagine writing docs with each term and found that 'Every' seemed to fit better. However that was some time ago, but it might be worth a thought.

I also just thought about EveryUser or AnyUser. That is include the two bracketing '*' to embolden the fact you are allowing/denying Every User and at the same time this is unlikely to already exist as a user/group name.

-- JulianLevens - 09 Apr 2014

I don't want a reserved name, because code then has to deal with reservation of that name - everywhere. For example, what if someone creates a topic AnyUser? Then the autolink code links to it .... better to avoid the issue by nor using a name (and certainly not a wikiword). For the same reason I don't want a macro. * works best for me.

-- CrawfordCurrie - 09 Apr 2014

NatEditPlugin makes use of the empty DENY rule in its permissions interface. Replacing it with an ALLOW = * rule is easy enough. However I'd like to keep the plugin compatible with older Foswikies. How would I do so?

This here won't work:

<input type="radio" class="foswikiRadioButton" data-perm-type="view" data-perms='%IF{"Foswiki < 1.2.0" then="{\"deny\":\" \"}" else="{\"allow\": \"*\"}%' ... />

-- MichaelDaum - 09 Apr 2014

Here is what I've got implemented, locally, along with successful unit tests.

• Set ALLOW....... = *

And a Foswiki.spec configuration param, $Foswiki::cfg{AccessControlACL}{enableDeprecatedEmptyDeny} = $FALSE

If that is enabled, the system will honor the old deprecated syntax while still supporting the new wildcard.

I don't have the TWikiCompatibility code working yet. It's more complex on Foswiki, as TWiki doesn't have the BASE_GROUP configuration structure, the names "just work". We have to add them to the group definitions, or just hardcode them in the isGroup() code,

   if ( TWikiCompatibiltliyPlugin enabled )
       if (group =~ m/$Any(Authorized)?UserGroup/ )
           return true

I don't like leaving plugin dependent code in core even for a default plugin. Another alternative, the plugin should update the BASE_GROUP definition in Foswiki::Users::BaseUserMapping to at least minimize the contamination. The isInGroup code would still have to have the matches for the rules.

-- GeorgeClark - 09 Apr 2014

So, just for clarification, under this proposal, empty DENY and ALLOW are both NOPs. Thus:

   * Set ALLOWTOPICVIEW = *
   * Set ALLOWTOPICVIEW = 

does nothing.

-- CrawfordCurrie - 10 Apr 2014

Correct.

Unless LocalSite.cfg explicitly enables the old behavior.

-- GeorgeClark - 10 Apr 2014

If I have

   * Set DENYWEBVIEW = JoeBloggs

... and in SpecificTopic

   * Set DENYTOPICVIEW =

Can JoeBloggs view SpecificTopic?

I don't know what the answer should be.

• Julian answered below

-- MichaelTempest - 10 Apr 2014

Currently he would be allowed access as

   * Set DENYTOPICVIEW =

would allow access to everyone.

OTOH

   * Set DENYTOPICVIEW =

will become a nop (after the change), then the

   * Set DENYWEBVIEW = JoeBloggs

would apply denying JoeBloggs

Therefore, to keep the same functionality you need to convert

   * Set DENYTOPICVIEW = 

to

   * Set ALLOWTOPICVIEW = *

BTW: I'm very happy with this syntax, despite my earlier EveryUser idea

• Correct, an ALLOWTOPICVIEW will trump a DENYWEBVIEW. - C.

-- JulianLevens - 10 Apr 2014

Question: if you were converting from the old to new syntax via a utility would you convert:

   * Set ALLOWTOPICVIEW = *
   * Set ALLOWTOPICVIEW = John, Bill

to

   * Set ALLOWTOPICVIEW = *, John, Bill

Or to

   * Set ALLOWTOPICVIEW = *

I'd be inclined to use the 1st option as although "John, Bill" is redundant, I'd want to keep as much of the original as possible.

Anybody disagree?

-- JulianLevens - 10 Apr 2014

Don't care, really. I don't really like mixing * with "real" user and group names, but it's not a big deal.

-- CrawfordCurrie - 10 Apr 2014

Regarding the utility, I'll pull over the TWiki utility, but now that I've reviewed the code a bit, it needs some work.

• It blindly converts any empty Set DENYxxxx to the corresponding Set ALLOWxxxx = AllUsersGroup.
• It makes no attempt to merge the Set ALLOW with existing statements. If an allow already was in the file, a duplicate gets created
• It makes no attempt to detect this error or warn that an improper ACL might have been created.
• It doesn't consider permissions in the Topic settings / Metadata. Only inline Set statements are processed.

So if an ALLOW is already in the topic, it either overrides, or becomes ignored. (The last Set for any variable wins.) And if the user hid the settings in the meta, well, they lose.

I've been trying to figure out a better way of describing the ACL evaluation sequence. Here's an attempt, though the more I look at it, the less sure I am about including it in the AccessControl topic. Anyway, this shows the changes to foswiki 1.2, including how the * fits in.

I'll also propose that we state that the setting: enableDeprecatedEmptyDeny be eliminated in Foswiki 2.0.

• Foswiki-auth-flow.jpg:
  

-- GeorgeClark - 11 Apr 2014

I think this diagram is very valuable and worth including because it is precise. I have always struggled to understand the access control rules. The hard part is being sure that I understand the system correctly. A diagram like this will help with that.

-- MichaelTempest - 11 Apr 2014

> And if the user hid the settings in the meta, well, they lose

... which is always the case when editing user permissions via the NatEditPlugin UI.

-- MichaelDaum - 11 Apr 2014

George

I have some ideas about how to improve the diagram, do you have the original in another format?

-- JulianLevens - 11 Apr 2014

Julian

I've attached an OpenOffice drawing file. That's what I drew it in as a quick and dirty. If you don't have openoffice I can save it as another format.

-- GeorgeClark - 11 Apr 2014

Michael

If NatEditPlugin ignores the TopicSettings, that's a serious issue to me. I'm aware of places where most of all security is in the settings rather than using the html comments to keep the documents clean. IMHO that should probably block 1.2.

-- GeorgeClark - 11 Apr 2014

What exactly do you mean by "ignores the TopicSettings" and in how far is it a "serious issue"? Can you elaborate and craft a scenario that you consider an issue, please?

For now the only issue I have with this proposal is that I still don't see a way for NatEditPlugin to run on foswiki < 1.20 and >= 1.20. See the example I provided above.

George and I had a chat on IRC and found a good solution to finally act upon the deprecation process.

-- MichaelDaum - 11 Apr 2014

My comment about NatEditPlugin was because I misunderstood your comment. In the IRC discussion you clarified. NatEditPlugin uses the the META for ACL's exclusively. There is still a concern in that we need to somehow convert existing inline settings to META settings. Given that this deprecation needs a utility to convert empty DENY to ALLOW * wildcard , maybe we could also include an option to convert all ACLs into META settings. Anyone want to work on that?

The Concern raised by Michael that he and I discussed is that we don't have a good deprecation process for this particular change, that is backwards compatible with Foswiki 1.1.x. It's common practice to bundle & ship WikiApps that use blank DENYTOPIC ACLs for access to important topics. Any App that ships using the wildcard ACL will not be backwards compatible, but any that use the blank DENYTOPIC won't be compatible with 1.2+ with the blank rule disabled.

We agreed that we should build an PatchFoswikiContrib which patches the wildcard feature into Foswiki 1.1. Any extensions that want to use the new wildcard should call out PatchItem12849Contrib as a dependency. This will add the wildcard, but will not remove the empty DENY support. It appears that the patch will fit with 5 patches: 1.1.0, 1.1.1-1.1.3, 1.1.4, 1.1.5, and 1.1.6-1.1.9. We will not provide a patch for 1.0.x.

-- GeorgeClark - 11 Apr 2014

I'm struggling a bit to figure out how to implement the TWiki compatibility for AllUsersGroup and AllAuthUsersGroup. I believe the objectives should be:

• The TWiki All(Auth)?UsersGroup should only be active if the TWikiCompatibiltiyPlugin is enabled.
• If a user has manually created an All* group, then it must not get any special treatment

Implementation however is not straightforward

• The behaviour of the group is relatively simple, though it's TWiki specific code embedded in Foswiki::Users.
• Conditionally adding the groups to the list of base groups has been more challenging. I tried to insert them into the Foswiki::Users::BaseUserMapping module using the init handler of the compat plugin, but was unsuccessful. I was able to add them by creating a Foswiki::Users::TWikiBaseUserMapping that subclasses the BaseUserMapping. It adds the two new groups, and returns 0 from the allowsChange. (allowsChange hardcoded the condition rather than checking the keys of BASE_GROUPS.)
  • The configuration for the Base mapping module is not documented in Foswiki.spec, but it appears to be active. I can't see anywhere that it was ever used?
  • If someone can figure out how to insert these two group names into the BaseUserMapping GROUPS from the plugin, that would be great.
  • If we subclass the Base mapper, then a checker could ensure that $Foswiki::cfg{BaseUserMappingManager} is set correctly when TWikiCompatibility is enabled.
• Making WikiGroups behave correctly has been more difficult. The descriptions are taken from literals in the templates, using %IF tests. I'm not sure how to conditionally add these groups only when TWikiCompatibilityPlugin is enabled.
• Regarding the topics for these two groups, I'm not sure if we should ship them. Maybe the TWikiCompatibilityPlugin could map the topic names when enabled.

It appears that on TWiki, these special groups don't show up in the TWikiGroups page. Actually none of the "special groups" appear to show up there.

-- GeorgeClark - 13 Apr 2014

The original rationale of the TWikiCompatibilityPlugin was to support users transitioning from TWiki for Foswiki by minimising the work they had to do to port content across. Since we wrote that plugin, the TWiki team has taken it off in directions that run counter to the Foswiki principles of clean, modular and reusable code, and simple use models combined with full specification of features. So if TWiki introduces a feature that requires changes to Foswiki core, my reaction is "don't sweat it, we don't want to go there anyway".

With regard to the BaseUserMapping - I do not believe you should be trying to modify this module. The BaseUsermapping is used as an "oh my god" minimum mapping module that is used when all else fails. AFAIK TWiki only supports topic user mapping, so it is the TopicUserMapping module that should be subclassed. My reaction would be to subclass TopicUserMapping and override eachGroup, isGroup-, =eachGroupMember, eachMemberhip, groupAllowsView, groupAllowsChange, addUserToGroup and removeUserFromGroup. KISS.

-- CrawfordCurrie - 14 Apr 2014

BaseUserMapping does some very importand things, for one provide an admin user. The core makes sure it is always in the loop by explicitly calling it as a last resort. Not so TopicUserMapping, which is completely replaceable with no trail left behind.

-- MichaelDaum - 14 Apr 2014

Checking the implementation, TWiki does use the BaseUserMapping, TWikiUserMapping, and also implements LdapUserMapping, etc. in extensions. We haven't drifted that far away.

However regarding compatibility, their implementation doesn't make any attempt to list the membership of the All*UsersGroup. So there is no need to override any of the iterators. Actually, in looking at the their implementation, it's nothing more than a hack to add the special group names as a hard coded override in twiki.pm, after the groups->hasNext iterator is exhausted. I don't think they know how the mappers work. I'm not going to use their implementation.

I was trying to add it to Base mapping because as Michael says, it's always called. It's the place where the last resort groups are added. It creates BaseGroup and AdminGroup. I'm surprised that it doesn't create NobodyGroup - that one lives in TopicUserMapping.

-- GeorgeClark - 14 Apr 2014

Commenting is disabled while not logged in