You are here: Foswiki>Tasks Web>Item13177 (19 Dec 2015, GeorgeClark)Edit Attach

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

pencil
Priority: Urgent
Current State: Proposal Required
Released In: n/a
Target Release: major
Applies To: Engine
Component: FoswikiFunc, FoswikiUsers
Branches:
Reported By: FredTarbox
Waiting For: FredTarbox
Last Change By: GeorgeClark
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
 

ItemTemplate edit

Summary when loginName contains specials, $SESSION->{user} double-encoded as a cUID when passed to checkAccessPermission()
ReportedBy FredTarbox
Codebase trunk
SVN Range
AppliesTo Engine
Component FoswikiFunc, FoswikiUsers
Priority Urgent
CurrentState Proposal Required
WaitingFor FredTarbox
Checkins
TargetRelease major
ReleasedIn n/a
CheckinsOnBranches
trunkCheckins
masterCheckins
ItemBranchCheckins
Release02x00Checkins
Release01x01Checkins
Topic revision: r13 - 19 Dec 2015, GeorgeClark
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy