Item5118: Difference from 4.1.2 - 4.2: Apache loginname no longer works with access control lists

Summary

The access controls are broken in the case where you allow login names, and use these names instead of the wikiname. A feature that worked fine until the usermapping refactoring in 4.2.0.

Failure case 1. login names used in groups. This is actually working as of 06 Jul 2008. Either the reporter meant something else or it has been fixed by other means. KennethLavrsen has tried with a group defined with login names that were registered in TWikiUsers and login names that were only authenticated but not registered. Both works.

Failure case 2. login names used in ALLOW and DENY statements when the user is registered in TWikiUsers. As of 06 Jul 2008 this is confirmed to still be broken.

First the setup.

• We have a TWiki using {LoginManager} as ApacheLogin. Either with LDAP or a valid .htaccess file.
• We have {PasswordManager} set to none.
• {UserMappingManager} is standard TWikiUserMapping
• We have {Register}{AllowLoginName} activated

In this environment we have two types of people.

• Registered users that map to a WikiName
• Un-registered users that do not map to a WikiName but are known by their login name

We use a user who is not an admin for the testing.

Now create a topic with Set ALLOWTOPICCHANGE = something.

In TWiki 4.1.2 I get always assuming that we login with the login ID before we try to edit the topic.

ALLOWTOPICCHANGE set to	Registered user (in TWikiUsers)	recognised as	Able to edit the topic
login	no	login	yes
login	yes	wikiname	yes
WikiName	no	login	no
WikiName	yes	wikiname	yes

In 4.2.0 the access control must match what the user is recognised as.

ALLOWTOPICCHANGE set to	Registered user (in TWikiUsers)	recognised as	Able to edit the topic
login	no	login	yes
login	yes	wikiname	no
WikiName	no	login	no
WikiName	yes	wikiname	yes

So in other words. As long as you are not registered the access rights work also with login ID. The minute you register you no longer have access.

The reason this becomes a problem is that a typical use case is that someone gives a non registered person access to a topic or web using his login ID because the guy has not yet registered. After a short while the new user falls in love with TWiki and decides to register. But suddenly he no longer has access to the resources he had before.

Original report and discussion

When logging in using ApacheLogin, TWiki 4.2.0 RC2 correctly recognises my loginname, provided by our in-house authentication, and happily greets me "Hello, cflewis!"

I used the TWikiAdminUser to change TWikiAdminGroup to:

"Set GROUP = cflewis" while leaving "Set ALLOWTOPICCHANGE = TWikiAdminGroup" unchanged.

However, I am not able to edit the TWikiAdminGroup as cflewis, resulting in the error:

"Access check on TWikiAdminGroup failed. Action "CHANGE": access not allowed on topic."

Other pages locked to the admin group, like WebPreferences, also reject the loginname. It is not affected if the loginname is specified as "cflewis" or "Main.cflewis".

I have done nothing to the UserMapping code, all I have done is select ApacheLogin as the authentication method.

SvenDowideit indicated that this might well be expected behaviour, and that the TWikiAdminGroup is expecting a WikiName, not a loginname, in the GROUP variable. However, my TWiki 4.1.2 installation would use the loginname in the list and authenticate without any trouble.

One thing that seems odd is that 4.2's TWikiGroups page does not show a loginname as being in the list of members. In 4.1.2 it does. Perhaps a change has occurred to the way the GROUPS variable is parsed? I haven't been able to ascertain why this may be, the way the code seems to work between Access.pm and Users.pm indicates that a loginname, as long as it is escaped, would be OK as a valid ID.

I would say 4.1.2s behaviour was a feature rather than a bug. It allows for a lot of flexibility. We were using it to divorce UNIX groups from the system, so TWiki was handling the grouping, and let users build new groups quickly, without having to ask the sysadmin for a new UNIX group. Writing a script to import UNIX groups of relevance to TWiki was a piece of cake, certainly much easier than delving into the UserMappings modules.

In addition, people here identify with their loginname, and having a WikiName would be an extra layer of difficulty that would quickly confuse users.

I am currently writing a UserMapping module to do this "properly", but it really is quite difficult with the minimal amount of documentation available at such a technical level, placing unnecessary pressure on the dev team in the IRC channel.

-- TWiki:Main/ChrisFLewis - 12 Dec 2007

Chris, please don't think the pressure is unneccessary. We are well aware that the doc is poor - I have recently written a user mapper myself. Only through feedback and iterative improvement can we address this problem.

-- TWiki:Main.CrawfordCurrie - 16 Dec 2007

Crawford: Thanks for the reply... the guys in the IRC channel have been very helpful and I managed to get my own UserMapper going by subclassing TWikiUserMapping and reading the comments from the UserMapping interface.

Having played with getWikiName(), I've ascertained that TWiki is quite happy to take a non-CamelCase word as the wikiname. Returning the loginname of the current user (all lowercase) works for me. I am guessing that the 4.1.2 behaviour was not intended, so not coded into 4.2, but using a non-CamelCase wikiname will work if forced.

I guess this bug can be closed when we know what is and isn't expected!

-- TWiki:Main.ChrisFLewis - 16 Dec 2007

If using the login worked in 4.1.2 and previous releases - documented or not documented - then it should also work in the future.

Why should we not allow a TWiki installation to work completely without any registered users, being e.g. LDAP authenticated, and using the login IDs for access control?

I think we have broken an important compatibility here. And we can see that there are actual customers that now are prevented from upgrading.

Requiring that people now write their own user mapper for something that works in 4.1.2 is up at level 8 on the TWiki:Codev.NerdoMeter.

I have raised this to urgent but we may have to accept that it is deferred to 4.2.1.

-- TWiki:Main.KennethLavrsen - 02 Jan 2008

mmm, either I made this work with the commit I did to fix Item5185 , or I need more information of the settings needed to reproduce it.

-- TWiki:Main.SvenDowideit - 07 Jan 2008

I have just upgraded with the 5185 fixes and it still fails. I can understand that you have a problem reproducing this because it is a bit tricky.

First the setup.

• We have a TWiki using {LoginManager} as ApacheLogin. Either with LDAP or a valid .htaccess file.
• We have {PasswordManager} set to none.
• {UserMappingManager} is standard TWikiUserMapping
• We have {Register}{AllowLoginName} activated

In this environment we have two types of people.

• Registered users that map to a WikiName
• Un-registered users that do not map to a WikiName but are known by their login name

We use a user who is not an admin for the testing.

Now create a topic with Set ALLOWTOPICCHANGE = something.

In TWiki 4.1.2 I get always assuming that we login with the login ID before we try to edit the topic.

ALLOWTOPICCHANGE set to	Registered user (in TWikiUsers)	recognised as	Able to edit the topic
login	no	login	yes
login	yes	wikiname	yes
WikiName	no	login	no
WikiName	yes	wikiname	yes

In 4.2.0 the access control must match what the user is recognised as.

ALLOWTOPICCHANGE set to	Registered user (in TWikiUsers)	recognised as	Able to edit the topic
login	no	login	yes
login	yes	wikiname	no
WikiName	no	login	no
WikiName	yes	wikiname	yes

So in other words. As long as you are not registered the access rights work also with login ID. The minute you register you no longer have access.

The reason this becomes a problem is that a typical use case is that someone gives a non registered person access to a topic or web using his login ID because the guy has not yet registered. After a short while the new user falls in love with TWiki and decides to register. But suddenly he no longer has access to the resources he had before.

I assume the same is the case with using login ID in a group definition. I did not test that combination. I assume you have what you need to work with now.

-- TWiki:Main.KennethLavrsen - 07 Jan 2008

I reported Item5381 for LdapContrib, but reading the comments in here it could be duplicate of this item.

-- TWiki:Main.KevinGoeser - 28 Feb 2008

I'm puzzled. Where did the idea that the login name could be used in place of the wikiname come from? As far as I'm aware, the doc has always been clear that group topics and all access controls work off the wikiname.

IMHO the argument If using the login worked in 4.1.2 and previous releases - documented or not documented - then it should also work in the future is fundamentally flawed. It basically says you can never fix bugs, because someone might be assuming a side-effect of the bug.

So, what is being discussed here is in fact an enhancement. OK, it may be the case that an bug previously allowed TWiki to behave this way, but this is still an enhancement to the spec and as such should have a proposal topic in Codev. Shouldn't it?

-- TWiki:Main.CrawfordCurrie - 16 Apr 2008

Where is the spec in e.g. Cairo that access rights only works with WikiNames?

Fact is that several independent users have reported this issue and that this issue was agreed as being urgent at several release meetings since December.

The problem does not go away no matter what you call it.

-- TWiki:Main.KennethLavrsen - 16 Apr 2008

Where is the spec in e.g. Cairo that access rights only works with WikiNames? it's in TWikiAccessControl: Access control is based on the familiar concept of Users and Groups. Users are defined by their WikiNames. My point is that the code change that removed the ability to use login names was in effect a bugfix, because it corrected undocumented (and, I suspect, accidental) behaviour. TWiki:Codev.ProcessToDocUndocumentedStuff covers exactly this case.

Undocumented functionality needs to be documented and accessible for testing somewhere, so that developers do not overlook it or remove it by accident.

Criteria: Every undocumented functionality needs to

1. be documented in the code
2. have unit tests and/or test cases.

Note: Test cases are still incomplete, this is for documented and undocumented functionality. If a feature has no test cases but is documented in the code, it should be discussed in Codev, and either (1) have written test cases within 4 weeks, or (2) removed from the code.

This report actually relates to a third case; functionality that has no test cases and is not documented in code. I have raised this point in that topic, and this discussion should continue there.

-- CrawfordCurrie - 17 Apr 2008

I think this bug item is somewhat unique.

First - the feature that worked in Cairo, TWiki 4.0.X and TWiki 4.1.X is logical. It makes sense that it works. If it had never been there people would live fine without it today.

Problem is that the normal innocent users have used the feature, seen it worked and used it. It is not a hack. The feature makes sense from a logical point of view.

And in 4.2 the feature partly works. It is not removed. People can use login IDs to allow people access to a topic and it works. Until the poor bloke finally registers and is suddenly shut out.

This is what made us say the feature had to be restored to fully working. People get trapped.

It is not like people finding a secret undocument twiki variable and started using it or used a very exotic regex search in the meta.

Normal users get trapped by this ALMOST working feature. As you can see from the table I made it is ONE out of four combination that does not work. The 3 of 4 work. This is what is so unfortunate about this bug.

If the feature had not been there before I would not even have raised it as a change proposal.

-- TWiki:Main.KennethLavrsen - 17 Apr 2008

I think the criteria that undocumented features must have unit tests and/or test cases is only defendable after all existing undocumented features do have unit tests and/or test cases. It is a different case for new undocumented features, where we expect unit tests and/or test cases.

This functionality is used in TWiki's primary target deployment, I think it is a release blocker that needs to be fixed.

-- TWiki:Main.PeterThoeny - 20 Apr 2008

I think if a couple of lines were added to Users.pm in the sub isInList we'd be back on track here are the new lines plus the existing lines above them:

    my $wn = getWikiName( $this, $user );
    my $ln = getLoginName( $this, $user);

and

      return 1 if( $ident eq $wn );
        return 1 if( $ident eq $ln );

someone who knows their perl better could probably do a better job, but near as I can tell the above would work.

-- TWiki:Main.DrewStevenson - 27 May 2008

TWiki:Main.CrawfordCurrie pointed out that the above would be better in the TWikiUserMapping.pm such that a isInList func there could be called if it exists. I'm working with a mentor here at work to try and take Crawford's example and create a patch. We're in the middle of deploying 4.1.2 to new hardware/new Apache so my time will be at a bit of a premium for a while.

-- TWiki:Main.DrewStevenson - 27 May 2008

(Duplicated the example I gave to Drew here because pastebin will time out)

Index: Users.pm
===================================================================
--- Users.pm   (revision 16816)
+++ Users.pm   (working copy)
 -571,12 +571,16 @@
     my $wn = getWikiName( $this, $user );
     my $umm = $this->_getMapping($user);
 
-    foreach my $ident ( split( /[\,\s]+/, $userlist ) ) {
-        $ident =~ s/^.*\.//;         # Dump the web specifier
-        next unless $ident;
-        return 1 if ( $ident eq $wn );
-        if ( $this->isGroup($ident) ) {
-            return 1 if ( $this->isInGroup( $user, $ident ) );
+            if ($umm->can("isInList")) {
+                return $umm->isInList( $wn, $userlist );
+            } else {
+                foreach my $ident ( split( /[\,\s]+/, $userlist ) ) {
+                $ident =~ s/^.*\.//;         # Dump the web specifier
+                next unless $ident;
+                return 1 if ( $ident eq $wn );
+                if ( $this->isGroup($ident) ) {
+                    return 1 if ( $this->isInGroup( $user, $ident ) );
+                }
+            }
         }
     }
     return 0;

Note this has the double advantage that user mappers can redefine isInList for other interpretations of "user" and "group".

-- CrawfordCurrie - 28 May 2008

I was thinking along similar lines, though would move the 'else' portion into TWiki::UserMapping::isInList() - ie the base class. This would remove the need for the if. There may be need to consult both mappers?

Drew, I'd love to see your work as I have rather alot going on at the moment - as Crawford says - we need a bunch of unit tests to proof and document the effects.

-- TWiki:Main.SvenDowideit - 29 May 2008

Here's the bugfix: The bug is in TWiki::Users::isInList. This method only checked for the WikiName being contained in the ACL. The fix below gets the cUIDs instead:

sub isInList {
    my( $this, $user, $userlist ) = @_;
        $this->ASSERT_IS_CANONICAL_USER_ID($user) if DEBUG;

    return 0 unless $userlist;

    # comma delimited list of users or groups
    # i.e.: "%USERSWEB%.UserA, UserB, Main.UserC  # something else"
    $userlist =~ s/(<[^>]*>)//go;     # Remove HTML tags

    my $cUID = $this->getCanonicalUserID($user);

    foreach my $ident ( split( /[\,\s]+/, $userlist )) {
        $ident =~ s/^.*\.//;       # Dump the web specifier
        next unless $ident;
        my $identCUID = $this->getCanonicalUserID($ident);
        return 1 if( $identCUID eq $cUID );
        if( $this->isGroup( $ident )) {
            return 1 if( $this->isInGroup( $user, $ident ));
        }
    }
    return 0;
}

There might be a faster way to get the cUID from the login/wikiname instead of calling getCanonicalUserID(), e.g. by directly using the cache inside this class. Don't know. Please have a look at this bugfix, i.e. at how to speed this up. I've got the impression that the extra function call inside this deep-inside method might incur an extra performance overhead.

I did not check the testcases yet as I am at a client's site, life-hacking this f**in stuff. I will do that when I come home. If testcases pass, this will be checked in asap.

There seem to be more errors in the same vein: a couple of internal functions are comparing the user name against the login or wikiname instead of the cUID. Just grep for user eq in TWikiUserMapping. Some of these are: isAdmin, isGroup and isInGroup. All these compare the user param without granting that these are cannonified, that is both: the user param as well as the value it is checked against (list of group members or SuperAdminGroup value etc).

Next error: TWiki::Users::TWikiUserMapping::getCanonicaIUserID() does not return a cUID if it is called with a wikiname, the same way as the method of the same name - TWiki::Users::getCanonicalUserID() - seems to do. In any case we need a method to convert both a wikiname or a login name to a cUID. Which one is it?

Here's another fix that cures this:

sub isInGroup {
    my( $this, $user, $group, $scanning ) = @_;
    ASSERT($user) if DEBUG;

    my @users;
    my $users = $this->{session}->{users};
    my $cUID = $users->getCanonicalUserID($user);
    my $it = $this->eachGroupMember($group);
    while ($it->hasNext()) {
        my $u = $it->next();
        my $uCUID = $users->getCanonicalUserID($u);
        next if $scanning->{$uCUID};
        $scanning->{$uCUID} = 1;
        return 1 if $uCUID eq $cUID;
        if( $this->isGroup($u) ) {
            return 1 if $this->isInGroup( $user, $u, $scanning);
        }
    }
    return 0;
}

I am still not 100% sure if that is right. Too bad this function is called with varying parameters. But that's maybe exactly what we need to support. Is there a way to make the transiton from loginnames and wikinames to cuids earlier in the flow of the code? This would bring back somewhat the speed that we are now investing here to get the cuid in the middle of the look.

Next thing I observed: the result for the isInGroup is not cached. That is calling it with the wikiname does not profit from the previous call when it was called with the login name. This needs a closer look to optimize it.

-- TWiki:Main.MichaelDaum - 02 Jun 2008

I've still got about 5 major projects ahead of this in line on my plate. Yes it's important to my employer and me, but I don't have the experience to do it quickly or the time to learn. I'm heading out to San Francisco next week for Apple's World Wide Developer's Conference. Maybe I can sink my teeth into this after I get back.

-- TWiki:Main.DrewStevenson - 06 Jun 2008

Sorry, I gave Micha feedback a while back on irc, but then didn't write it here either...

I recon we should just go with Micha's changes, see how it goes and test for a while. I simply don't have the BW to be useful on this right now.

It is obvious that there should be some unit tests written that would fail right now.

-- TWiki:Main.SvenDowideit - 11 Jun 2008

Is this in the latest nightlies? Is there a build we could test?

-- TWiki:Main.DrewStevenson - 19 Jun 2008

No. nothing is checked in on this bug item since it was raised 6 months ago and there are several other bug items related to it including a security issue.

-- TWiki:Main.KennethLavrsen - 20 Jun 2008

We are many now waiting for the guy who broke it to address this serious and now very old bug.

-- TWiki:Main.KennethLavrsen - 25 Jun 2008

scary, I was waiting for Micha or Drew, or hell, someone that needs it fixed to continue with it. It really needs a suite of unit tests.

even scarier, is there's all this finger pointing to the user mapping changes (and tbh, i expect it to be there too) and yet, I just added a few unit tests, and they don't fail. Not that I'm unwilling to believe there is a problem with the unit tests, but without them, we're just asking for more trouble.

-- TWiki:Main.SvenDowideit - 26 Jun 2008

Just open up your editor and look at the code. This helps sometimes too.

-- TWiki:Main.MichaelDaum - 26 Jun 2008

Cannot see the unit tests added address the problem. Please look at step by step description and the tables I made 07 Jan 2008 above. It is very easy to reproduce this error and the tables tells you exactly what unit tests to write which are expected to fail and which are expected to pass.

-- TWiki:Main.KennethLavrsen - 26 Jun 2008

Hi guys! In case it hasn't been noticed (I'm sure Sven remembers) I've now left my job and no longer on the TWiki implementation project, so I won't be available to test this one anymore. Sorry!

Good luck on it... it looks like a real brain-teaser :/

-- TWiki:Main.ChrisFLewis - 26 Jun 2008

I have summarized the error at the top to aid the fixing.

Sven are you working on this?

-- KennethLavrsen - 01 Jul 2008

no, I'm not, I'm flat out working on other things - almost all the fixes I've made recently are derived from client work - pretty much the only way I'm able to give back to TWiki atm.

Considering how simple Michael is suggesting the issue is, I'm expecting that he can tidy it away pretty quickly - especially as he's previously shown himself capable of writing unit tests.

There are some really really massive things being worked on - I'm looking forward to being allowed to make a few public.

-- TWiki:Main.SvenDowideit - 01 Jul 2008

Then I only see two options.

Either Michael steps up and makes a fix.

Or we revert all the user mapping code back to 4.1.2.

Who will help me do the latter?

-- TWiki:Main.KennethLavrsen - 01 Jul 2008

I will be working on 4.2 user code tomorrow as a client of mine has indicated that the above fixes don't work out properly. However, this will be yet another hot-fix - no real fix. So I am afraid I have to second Kenneth's proposal to revert all user code to what it was in 4.1.2. It will be easier to go forward from that point to a 4.2.1+

-- TWiki:Main.MichaelDaum - 02 Jul 2008

Nope. I won't be working on the 4.2. user code today. My client activated the emergency brakes and downgrades to 4.1.2. He was suffering of wysiwyg errors anyway.

-- TWiki:Main.MichaelDaum - 03 Jul 2008

A small update. The original report reported that login names did not work in groups. I have found that in the 4.2 code as of 06 Jul 2008 the login names used in a Set GROUP = works fine both for registered and not yet registered users.

But when used in an ALLOWXXX or DENYXXX statement in a topic login names do not work - if the user is registered and has an entry in TWikiUsers.

Even if I have ALLOWTOPICCHANGE = MyownGroup and I have used the login name of a registered person in the group definition I get access. It is only the case where the login name of a registered person is used directly in an ALLOWTOPICXXXX that the person is denied access.

If the access control works with both login and wikinames in groups then I cannot see why it cannot be made to work also in topics.

I regret I did not try the group case for a long time so we could have focused on the bug that actually remains. But I have personally been focussed on the plain ALLOWTOPICCHANGE and ALLOWTOPICVIEW case because that is the one my users get caught by quite often when someone want to protect a specific topic and use the core ID of an unregistered new user. When the person then registeres a few days later I have a support request because the person no longer has access to his/her own topic. It is only my advanced users that make groups and they all use Wikinames so this is why my focus has been entirely on the ALLOWTOPICCHANGE/ALLOWTOPICVIEW case. And this is the one I am so keen on getting fixed.

-- KennethLavrsen - 06 Jul 2008

I took a stab at the bug also.

And looking at Michael's work made it a bit easier for me.

Look at this very tiny little fix. lib/TWiki/Users.pm

sub isInList {
    my( $this, $user, $userlist ) = @_;
   $this->ASSERT_IS_CANONICAL_USER_ID($user) if DEBUG;

    return 0 unless $userlist;

    # comma delimited list of users or groups
    # i.e.: "%USERSWEB%.UserA, UserB, Main.UserC  # something else"
    $userlist =~ s/(<[^>]*>)//go;     # Remove HTML tags

    my $wn = getWikiName( $this, $user );
    my $umm = $this->_getMapping($user);

    foreach my $ident ( split( /[\,\s]+/, $userlist )) {
        $ident =~ s/^.*\.//;       # Dump the web specifier
        next unless $ident;
        return 1 if( $ident eq $wn );
        return 1 if( $ident eq $user );
        if( $this->isGroup( $ident )) {
            return 1 if( $this->isInGroup( $user, $ident ));
        }
    }
    return 0;
}

I just added return 1 if( $ident eq $user );

So why does this make sense instead of looking at Canonical user IDs?

Who knows their Canonical user ID?? As I understand it is the login name that is visible for a user not yet registered (I will try and confirm this later - or if you know please help). So this it is for sure the login name and not a "behind-the-scenes" canonical ID that people will use for access controls.

So why not simply let the code compare the login name with the access control name?

Any security issue with that?

If not - and if I am right - then adding this one line of code closes Item5118.

-- KennethLavrsen - 06 Jul 2008

It is the canonical ID which is shown in the user interface. And also the canonical ID used as $user in the code.

Without my fix I can register c12345 and give c12345 access to a topic. And then I can register as c12345. (note the trailing dot) and I still have access. Totally unsecure as already raised as a security issue in another bug report.

Without my fix I can not get access as dcy if dcy is registered. But if I then log in as dcy. then I get access. This is totally crazy.

-- TWiki:Main.KennethLavrsen - 06 Jul 2008

Here is the horror. I am talking about the code without mine or Michaels mods now. The code as it is in SVN.

Here is the situation

• I am authenticated as 'dcy.'
• 'dcy.' is not registered
• A person with login name 'dcy' is registered and mapped to wikiname DianeChayer * I am accessing a topic with Set ALLOWTOPICCHANGE = Main.KennethLavrsen, dcy

When I look at the lib/TWiki/Users.pm and sub isInList

• $user is dcy_46
• $userlist=Main.KennethLavrsen, dcy
• $wn gets assigned to 'dcy' !!! The code that converts to wikiname does not return 'dcy.' or 'dcy_46'. It returns 'dcy' which means the person gets access with this username. This is awful. The getWikiName( $this, $user ) must return the $user unmodified if the mapping did not find a corresponding wikiname. This I need to address first.

-- TWiki:Main.KennethLavrsen - 06 Jul 2008

I found the place the code where the reduced wikiname is returned.

If I fix this to return the login ID then the UI gets goofed in many cases

If I return the Canonical ID then I also get some bad UI details. The same code is used for both things that must look good and things that need to be secure. I think this is a basic problem. The authentication should not at all mangle the login name.

This code need to be rewritten completely.

I will now try the method Michael has proposed which is comparing canonical IDs. Michael what was it that did not work in your fix above?

-- TWiki:Main.KennethLavrsen - 06 Jul 2008

It sometimes worked and sometimes not. Authorization was working totally irregular. I had no chance to debug this because the client had enough and downgraded to 4.1.2. I know this is a non-report. Even on the patched system it would have been a pain to even trigger the error, as it was only happening for very specific users and very specific access rights, somewhere along the line of tests you made. Downgrading resolved the issue for this client, so ...

There is probably a related issue that might mix into your test cases: all of them use a DOT somewhere in the name - i.e. at the end. However, everything before the dot is cut away. The code assumes that everything before a DOT is a webname and the rest is the wikiname - even if the argument is a login name. In your test cases, with a dot at the end, this would result in the empty string being used to authorize the current user in various places, as far as I can see.

Theoretically, this cutting off a suspicious webname from a login name could be used to gain unauthorized access: you could register a user with login Main.john and get access to =john='s stuff.

-- TWiki:Main.MichaelDaum - 07 Jul 2008

I am working on some code addressing exactly the dot issue and I have resolved it for the ALLOWTOPIC case but not yet for the group case. But I am sure the fix is the same.

I ran out of time but will continue working on it. The fix I do is that the code now strips away anything before a dot when it picks up the access rights. So a guy with login kenneth.lavrsen will have his login molested to lavrsen. The best fix I can see is to only strip away the prefix when it matches exactly the userweb (ie Main unless you changed it). I think most organisations can live with not being able to have a user that starts with Main. as login.

If I solve this group problem then I think we have a fix (let is call it a hack) that will work for 4.2.1.

I also looked at making a unit test case but I still cannot grasp the framework. To build unit tests you need to know ALL the internals of both the unit test frame work and the TWiki code. You need to be able to create topics, populate them, create users etc etc using function calls. It is a lot more complicated.

If someone can write just ONE unit test that setup

• A group
• A user who is registered
• A user who is authenticated but not registered.
• AllowLoginname = 1
• A topic with ALLOWTOPICREAD

where one can vary who is in the group, who you are, and change the to list of names of the ALLOWTOPICREAD, then I could multiply this case with the different combinations afterwards. I simply do not understand how to do that. It takes too much knowledge about parts of the twiki code I never studied.

-- TWiki:Main.KennethLavrsen - 07 Jul 2008

I now have code running that also handles groups containing members with a dot in their name.

I have one unit test that fails that I need to address before I check-in my code for review.

The error is related to the fact that I also need to handle access rights written as %<nopUSERSWEB%. or %MAINWEB%.

-- TWiki:Main.KennethLavrsen - 08 Jul 2008

Hm, it might be okay to strip off Main. as most users have a different first name. However, let's check that it is impossible that EvilUser will be able to gain a user's access rights by registering a Main.ceo login name by purpose.

-- TWiki:Main.MichaelDaum - 08 Jul 2008

I have the feeling I have run into the same problem that Michael ran into.

If I have a registered user dcy mapped to DianeChayer but I authenticate with a user called dcy. and I have a topic protected with ALLOWTOPICCHANGE including 'dcy' in the list then I can still login. Some poor mans debugging shows that for some reason the $this->{getCanonicalUserID}->{$login} maps dcy. to dcy. This is with the code modified so authentication compares the cUIDs. But where does dcy. get mapped to dcy?

This is very goofy and I need to get to the bottom of this.

-- TWiki:Main.KennethLavrsen - 08 Jul 2008

I am not home tonight and my code still has the problem described above.

I have a fix that I think take care of the group problem using

s/^($TWiki::cfg{UsersWebName}|%USERSWEB%|%MAINWEB%)\.//;

which takes care of stripping away the web part of a list in a group or ALLOW/DENY bullet.

But it is not in the code I just attached.

Instead of checking in unfinished code (I hate when others do that) I have attached my temporary state to the bug item. Any idea/hints/improvements are welcome for a 4.2.1 scope hack.

If you diff the code against current SVN you will see many lines changed that are just syntax. I had to do that because the code in trunk and TWikiRelease04x02 is not in sync and the only way to compare is to have the same syntax so only real differences between the two branches are seen. It is a bad habbit to fix syntax in one branch and not merge it to the other. I am sure some fixes were missed because if this.

-- TWiki:Main.KennethLavrsen - 08 Jul 2008

I have found the reason for the problem with the $this->{getCanonicalUserID}->{$login} maps dcy. to dcy

The dcy. user is not registered and means that getWikiName returns the login name. And the 4.2.0 code is full of "dot removers" everywhere. It is a flaw that the code consequently assume anything before a dot is a web name. 4.1.2 did not do this. Login names must be able to contain dots so we have to go through all the code and remove the s/^.*\.//; and then implement a better way to handle the dots. If we just remove lines the user dcy. is displayed as WebHome. A lot of cleanup is required here.

-- TWiki:Main.KennethLavrsen - 09 Jul 2008

OK. After having worked day and night on this I have convinced myself that I have hacked the code to a level that is OK for 4.2.1.

What I cannot overview is the consequences for other mapping contribs so those will have to be carefully reviewed by those that are interested.

What I have fixed is

• You can now use both wikinames and login names in both groups and ALLOW lists.
• You can now use login names with dots
• You can still use the format Main.WikiName, !%USERSWEB%.WikiName, !%MAINWEB%.Wikiname, WikiName
• And all the above with login ID (even if this makes less sense)
• You can no longer use any web name in front of the login/wikiname. That was needed to allow dots in login names.
• You cannot use a login name that starts with the name of the Main web followed by a dot

What I have not fixed is the security problem with abc. being same as abc_46 as user name. But that problem has its own bug item.

I am closing this knowing that there are still unit tests that should be added. But for 4.2.1 I want the bug item in Waiting For Release. If someone want to work further on unit tests make a new enhancement bug item for the checkins. I would like to work on the unit tests myself but need help getting started. See request above in this topic.

-- KennethLavrsen - 09 Jul 2008

Re-opened because I have added some unit tests (though by no means exhaustive)

-- CrawfordCurrie - 15 Jul 2008

We also need to address the Use of uninitialized value in substitution (s///) at /home/tw...') called at /home/twiki4/twikisvn/core/lib/TWiki/Render.pm line 1591

Bugs here dies when you navigate to some pages.

In TWikiRelease04x02 branch it is line 1590

view: Use of uninitialized value in substitution (s///) at /var/www/twiki42/lib/TWiki/Render.pm line 1590

I get this in the error log also on my test server

-- KennethLavrsen - 15 Jul 2008

This should be fully resolved now. Please report any new problems in a fresh report.

-- TWiki:Main.CrawfordCurrie - 23 Jul 2008

Re-opened for more unit tests and doc

-- CrawfordCurrie - 23 Jul 2008