Item13177: when loginName contains specials, $SESSION->{user} double-encoded as a cUID when passed to checkAccessPermission()

The user is in ldap as fred.tarbox.

HistoryPlugin has the line:

Foswiki::Func::checkAccessPermission("VIEW", $session->{user}, undef, $topic, $web)

$session->{user} resolves to fred_2etarbox because of how Ldap maps dots. This works its way down to LdapUserMapping::login2cUID, which fails to recognize this as a valid user.

Before a certain version of LdapUserMapping (sorry for being vague), eventually UnknownUser gets returned, which in my case creates the impression that HistoryPlugin is working properly.

After a certain version (and on the latest version), fred_5f2etarbox gets returned, which in my case causes HistoryPlugin to return a 403.

Replacing HistoryPlugin's lines with similar lines stolen from TopicInteractionPlugin:

    my $wikiName = Foswiki::Func::getWikiName();
    unless (
        Foswiki::Func::checkAccessPermission(
            "VIEW", $wikiName, undef, $topic, $web
        )
      )
.
.
.

Returns it to the expected behavior.

I'm uncertain whether this should be attributed to HistoryPlugin or LdapContrib.

-- FredTarbox - 23 Dec 2014

Reading the documentation for Func::checkAccessPermission, ID is defined as being a wikiname, but can also be the login name. I think that this should be changed as FredTarbox suggests. $session->{user} is the internal cUID and I'm guessing that results in the double encoding.

-- GeorgeClark - 23 Dec 2014

Nope... strike that last comment. Foswiki::Func specifically documents that usage as the way to call this routine.

-- GeorgeClark - 23 Dec 2014

I believe that the root of the issue is a flaw in the way the design of the user / cUID handling has evolved.

• Foswiki.pm initializes the $session->{user} by calling Foswiki::Users::initialiseUser(remoteUser)
• Users::initialiseUser returns a cUID as documented, so the $SESSION->{user} is a cUID.
• Foswiki::Func::checkAccessPermission() unconditionally calls $cUID = Func::getCanonicalUserID($user).
• Func::getCanonicalUserID() calls $cUID = $Foswiki::Plugins::SESSION->{users}->getCanonicalUserID($user);
• Foswiki::Users::getCanonicalUserID explicitly states that it must not be called with a cUID:

# This function was previously known as forceCUID. It differs from that
# implementation in that it does *not* accept a CUID as parameter, which
# if why it has been renamed.

So either the documented usage instructions are wrong, or there is a deeper bug. Foswiki::Func::checkAccessPermissions() has the following example:

        Foswiki::Func::checkAccessPermission(
            "VIEW", $session->{user}, undef, $topic, $web

I suspect that either Func::getCanonicalUserID, or Users::getCanonicalUserID need to check if the user parameter is already know as a cUID and avoid double encoding the passed user. But this is all pretty complex and needs someone more knowledgable to review.

The bottom line is that calling Users::getCanonicalUserID on a cUID results in double encoding. I think this one needs to be urgent. I've removed LdapContrib and HistoryPlugin from the Components, as this is a core bug.

-- GeorgeClark - 23 Dec 2014

IRC Discussion starts here: http://irclogs.foswiki.org/bin/irclogger_log/foswiki?date=2014-12-23,Tue&sel=163#l159

-- GeorgeClark - 24 Dec 2014

This code needs a redesign for Foswiki-2.0. The current internal api is too fragile and performs far too many calls to getCanonicalUserID as well as getMapper. Key would be to have a Foswiki::User object that holds the properties with no need to identify the cUID or mapper responsible for it again and again all over the code on different levels.

-- MichaelDaum - 24 Dec 2014

Since the comments claim that getCanonicalUserID isn't meant to be called with a cUID, how much trouble would I be in if I just tested input for cUIDness and returned it unchanged if found? Given that I can guarantee that none of my users will have a login name identical to someone else's cUID.

-- FredTarbox - 25 Dec 2014

I'm not sure of the implications, but the unit tests still pass with the following patch:

diff --git a/core/lib/Foswiki/Users.pm b/core/lib/Foswiki/Users.pm
index 0ec47a6..43466b1 100644
--- a/core/lib/Foswiki/Users.pm
+++ b/core/lib/Foswiki/Users.pm
@@ -464,6 +464,16 @@ sub getCanonicalUserID {
 
     ASSERT( defined $identifier ) if DEBUG;
 
+    # Called with a cUID?
+    # SMELL: This routine claims that it should never be called with a cUID,
+    # but core has evolved and calls this with $SESSION->{user} which actually
+    # is the current users cUID.  That causes double encoding in some cases..
+    # See Item13177
+
+    return $identifier
+      if ( defined $this->{cUID2Login}->{$identifier}
+        || defined $this->{cUID2WikiName}->{$identifier} );
+
     # Someone we already know?
 
     if ( defined( $this->{login2cUID}->{$identifier} ) ) {

-- GeorgeClark - 26 Dec 2014

That should be ok so long as the set of login names and the set of cUIDs don't overlap (or are equal). Consider the possibility of:

• Login Name: juan_tree CUID: juantree
• Login Name: juan²tree CUID: juantree

I don't think there are any unit tests for this (and doesn't it depend on the value of {LoginNameFilter} or something like that?)

-- CrawfordCurrie - 27 Dec 2014

In LDAP cuids and login names are identical.

-- MichaelDaum - 27 Dec 2014

I'm deferring this task until 2.0. There is too much risk to making changes in this area just before beta.

-- GeorgeClark - 15 Jan 2015

Deferring to 2.1, and marking as Needs Proposal. There are dragons here I suspect.

-- Main.GeorgeClark - 06 Jul 2015 - 03:27

Commenting is disabled while not logged in