Fix group membership by scratch variables for Visitor. Fixes bug #12195. Kudos to trex for the base patch. Also fix the same problem in the IP based group membership.

perlDreamer · perlDreamer · commit e65368c7c17e · 2011-08-22T19:17:58.000-07:00
diff --git a/docs/changelog/7.x.x.txt b/docs/changelog/7.x.x.txt
@@ -1,6 +1,7 @@
 7.10.23
  - fixed #12225: Stock asset, multiple instances on a page
  - fixed #12229: Indexed thingy data has gateway url prepended to it
+ - fixed #12195: Visitor group by scratch membership shared among all Visitors (Dale Trexel)
 
 7.10.22
  - rfe   #12223: Add date type to content profiling (metadata)
diff --git a/lib/WebGUI/Group.pm b/lib/WebGUI/Group.pm
@@ -1091,22 +1091,29 @@ Membership will always be false if no IpFilter has been set
 
 id of the user to check for membership
 
+=head3 sessionId
+
+id of the session to check for user data.  If no sessionId is passed in, then the
+group's session will be used to find one.
+
+
 =cut
 
 sub hasIpUser {
 	my $self    = shift;
-    my $userId  = shift;
     my $session = $self->session;
+    my $userId  = shift;
+    my $userSessionId  = shift || $session->getId;
     
     my $IpFilter = $self->ipFilter();
     return 0 unless ($IpFilter && $userId);
 
 	$IpFilter =~ s/\s//g;
 	my @filters = split /;/, $IpFilter;
 
-	my @ips = $session->db->buildArray(
-        q{ select lastIP from userSession where expires > ? and userId = ? }
-        ,[ time(), $userId ]
+    my @ips = $session->db->buildArray(
+        q{ select lastIP from userSession where expires > ? and userId = ? and sessionId=?}
+        ,[ time(), $userId, $userSessionId, ]
     );
 
 	foreach my $ip (@ips) {
@@ -1207,7 +1214,7 @@ sub hasLDAPUser {
 
 #-------------------------------------------------------------------
 
-=head2 hasScratchUser ( userId )
+=head2 hasScratchUser ( userId, [ $sessionId ] )
 
 Determine if the user passed in is a member of this group via session scratch
 variable settings and this group's scratchFilter.
@@ -1218,12 +1225,18 @@ If no scratchFilter has been set for this group, membership will always be false
 
 id of the user to check for membership
 
+=head3 sessionId
+
+id of the session for the user being checked for membership.  If no sessionId is passed in, then the
+group's session will be used to find one.
+
 =cut
 
 sub hasScratchUser {
 	my $self    = shift;
-    my $userId  = shift;
     my $session = $self->session; 
+    my $userId  = shift;
+    my $userSessionId = shift || $self->session;
 
 	my $scratchFilter = $self->scratchFilter();
 	return 0 unless ($scratchFilter && $userId);
@@ -1232,7 +1245,7 @@ sub hasScratchUser {
 	my @filters = split /;/, $scratchFilter;
 
 	my @scratchClauses      = ();
-	my @scratchPlaceholders = ( $userId, time() );
+	my @scratchPlaceholders = ( $userSessionId, $userId, time() );
 	foreach my $filter (@filters) {
 		my ($name, $value) = split /=/, $filter;
 		push @scratchClauses, "(s.name=? AND s.value=?)";
@@ -1246,6 +1259,7 @@ sub hasScratchUser {
         from
             userSession u, userSessionScratch s
         where
+            u.sessionId = ? AND
             u.sessionId=s.sessionId AND
             u.userId = ? AND
             u.expires > ? AND
@@ -1273,6 +1287,7 @@ sub hasUser {
 	my $self           = shift;
     my $session        = $self->session;
     my $user           = shift || WebGUI::User->new($session,3);      #Check the admin account if no user is passed in
+    my $uSessionId     = $user->session->getId;
 	my $gid            = $self->getId;
 	my $db             = $session->db;
 
@@ -1359,9 +1374,9 @@ sub hasUser {
 		my $groupToCheck = __PACKAGE__->new($session,$groupIdInGroup);
         ### Check the 'has' method for each of the 'other' group methods available for this user
         ### perform checks in a least -> most expensive manner.  If we find the user, stow the cache and return true
-		if( $groupToCheck->hasIpUser($uid)
+		if( $groupToCheck->hasIpUser($uid, $uSessionId)
 			|| $groupToCheck->hasKarmaUser($uid)
-			|| $groupToCheck->hasScratchUser($uid)
+			|| $groupToCheck->hasScratchUser($uid, $uSessionId)
 			|| $groupToCheck->hasDatabaseUser($uid)
 			|| $groupToCheck->hasLDAPUser($uid)
 		) {
diff --git a/t/Group.t b/t/Group.t
@@ -640,15 +640,17 @@ WebGUI::Test->addToCleanup(@sessionBank);
 
 #isInGroup test
 foreach my $scratchTest (@scratchTests) {
-	is($scratchTest->{user}->isInGroup($gS->getId), $scratchTest->{expect}, $scratchTest->{comment});
+    is($scratchTest->{user}->isInGroup($gS->getId), $scratchTest->{expect}, $scratchTest->{comment});
 }
 
 WebGUI::Cache->new($session, $gS->getId)->delete();  ##Delete cached key for testing
 $session->stow->delete("isInGroup");
 
 #hasScratchUser test
-foreach my $scratchTest (@scratchTests) {
-	is($gS->hasScratchUser($scratchTest->{user}->getId), $scratchTest->{expect}, $scratchTest->{comment}." - hasScratchUser");
+foreach my $idx (0..$#scratchTests) {
+    my $scratchTest = $scratchTests[$idx];
+    my $sessionId   = $sessionBank[$idx]->getId;
+	is($gS->hasScratchUser($scratchTest->{user}->getId, $sessionId), $scratchTest->{expect}, $scratchTest->{comment}." - hasScratchUser");
 }
 
 
@@ -666,7 +668,7 @@ cmp_bag(
 
 {  ##Add scope to force cleanup
 
-    note "Checking for user Visitor session leak";
+    note "Checking for user Visitor session leak with scratch";
 
     my $remoteSession = WebGUI::Test->newSession;
     $remoteSession->user({userId => 1});
@@ -681,13 +683,12 @@ cmp_bag(
     my $localSession = WebGUI::Test->newSession;
     WebGUI::Test->addToCleanup($localScratchGroup, $remoteSession, $localSession);
     $localSession->user({userId => 1});
-    $remoteSession->scratch->set('local','ok');
+    $localSession->scratch->set('local','ok');
     $localScratchGroup->clearCaches;
 
     ok $localSession->user->isInGroup($localScratchGroup->getId), 'Local Visitor is in the scratch group';
 
     $remoteSession->stow->delete('isInGroup');
-    $localScratchGroup->clearCaches;
     ok !$remoteSession->user->isInGroup($localScratchGroup->getId), 'Remote Visitor is not in the scratch group, even though a different Visitor passed';
 
 }
@@ -710,8 +711,9 @@ foreach my $idx (0..$#ipTests) {
 	##Name this user for convenience
 	$tcps[$idx]->username("tcp$idx");
 
-	##Assign this user to this test to be fetched later
-	$ipTests[$idx]->{user} = $tcps[$idx];
+    ##Assign this user and session to this test to be fetched later
+    $ipTests[$idx]->{user}    = $tcps[$idx];
+    $ipTests[$idx]->{session} = $sessionBank[$idx];
 }
 WebGUI::Test->addToCleanup(@tcps);
 WebGUI::Test->addToCleanup(@sessionBank);
@@ -734,7 +736,7 @@ cmp_bag(
 );
 
 is_deeply(
-	[ (map { $gI->hasIpUser($_->{user}->getId) }  @ipTests) ],
+	[ (map { $gI->hasIpUser($_->{user}->getId, $_->{session}->getId) }  @ipTests) ],
 	[ (map { $_->{expect} } @ipTests) ],
 	'hasIpUsers for group with IP filter'
 );
@@ -745,7 +747,7 @@ foreach my $ipTest (@ipTests) {
 
 {  ##Add scope to force cleanup
 
-    note "Checking for user Visitor session leak";
+    note "Checking for user Visitor session leak via IP address";
 
     $ENV{REMOTE_ADDR} = '191.168.1.1';
     my $remoteSession = WebGUI::Test->newSession;
diff --git a/t/Group/group_scratch.t b/t/Group/group_scratch.t
@@ -20,27 +20,49 @@ use WebGUI::Group;
 
 #----------------------------------------------------------------------------
 # Init
-my $session         = WebGUI::Test->session;
-
+my $session1         = WebGUI::Test->session;
+my $session2         = WebGUI::Session->open(WebGUI::Test::root, WebGUI::Test::file);
 
 #----------------------------------------------------------------------------
 # Tests
+### Updates by DRT test that group membership is restricted by user session
+### ...specifically for Visitors to have separate scratch group memberships
 
-plan tests => 5;        # Increment this number for each test you create
+plan tests => 14;        # Increment this number for each test you create
 
-my $group = WebGUI::Group->new($session, 'new');
+my $group = WebGUI::Group->new($session1, 'new');
 WebGUI::Test->addToCleanup($group);
 $group->scratchFilter("itchy=test_value");
 is( $group->scratchFilter(), "itchy=test_value",'Group->scratchFilter is properly set and retrieved');
 $group->groupCacheTimeout(0);
 is( $group->groupCacheTimeout(), 0, 'set groupCacheTimeout to 0');
 
-$session->user({userId => 1});
-ok( !$session->user->isInGroup($group->getId), 'Visitor is NOT in the group BEFORE scratch value is set'); 
-$session->scratch->set('itchy', 'test_value');
-is ($group->hasScratchUser($session->user->getId), 1, 'Group->hasScratchUser correctly returns 1 immediately after scratch is set');
+$session1->user({userId => 1});
+$session2->user({userId => 1});
+
+### Test group membership before scratch is set
+### NOTE: test hasScratchUser first, because isInGroup sets stow & cache
+is ($group->hasScratchUser($session1->user->getId,$session1->user->session->getId), 0, 'Group->hasScratchUser correctly returns 0 for Visitor 1 before scratch is set');
+is ($group->hasScratchUser($session2->user->getId,$session2->user->session->getId), 0, 'Group->hasScratchUser correctly returns 0 for Visitor 2 before scratch is set');
+ok( !$session1->user->isInGroup($group->getId), 'user1->isInGroup correctly returns 0 before scratch is set');
+ok( !$session2->user->isInGroup($group->getId), 'user2->isInGroup correctly returns 0 before scratch is set');
+
+### Test group membership after scratch is set
+### Clear stow, which is volatile, to simulate new page view
+$session1->stow->deleteAll;
+$session2->stow->deleteAll;
+$session1->scratch->set('itchy', 'test_value');
+is ($group->hasScratchUser($session1->user->getId,$session1->user->session->getId), 1, 'Group->hasScratchUser correctly returns 1 for Visitor 1 after scratch for Visitor 1 is set');
+is ($group->hasScratchUser($session2->user->getId,$session2->user->session->getId), 0, 'Group->hasScratchUser correctly returns 0 for Visitor 2 after scratch for Visitor 1 is set');
+ok( $session1->user->isInGroup($group->getId), 'user1->isInGroup correctly returns 1 after scratch for Visitor 1 is set');
+ok( !$session2->user->isInGroup($group->getId), 'user2->isInGroup correctly returns 0 after scratch for Visitor 1 is set');
 
-##Simulate another page view by clearing stow, which is volatile
-$session->stow->deleteAll;
-$session->scratch->delete('itchy');
-is ($group->hasScratchUser($session->user->getId), 0, 'after clearing scratch, user is not in the group any longer');
+### Test group membership after scratch is deleted
+### Clear stow, which is volatile, to simulate new page view
+$session1->stow->deleteAll;
+$session2->stow->deleteAll;
+$session1->scratch->delete('itchy');
+is ($group->hasScratchUser($session1->user->getId,$session1->user->session->getId), 0, 'Group->hasScratchUser for Visitor 1 correctly returns 0 after clearing scratch for Visitor 1');
+is ($group->hasScratchUser($session2->user->getId,$session2->user->session->getId), 0, 'Group->hasScratchUser for Visitor 2 correctly returns 0 after clearing scratch for Visitor 1');
+ok( !$session1->user->isInGroup($group->getId), 'user1->isInGroup correctly returns 0 after scratch for Visitor 1 is deleted');
+ok( !$session2->user->isInGroup($group->getId), 'user2->isInGroup correctly returns 0 after scratch for Visitor 1 is deleted');
