Item13177: when loginName contains specials, $SESSION->{user} double-encoded as a cUID when passed to checkAccessPermission()
Priority: Urgent
Current State: Proposal Required
Released In: n/a
Target Release: major
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