Skip to content

Commit

Permalink
Fix group membership by scratch variables for Visitor. Fixes bug #121…
Browse files Browse the repository at this point in the history 
…95. Kudos to trex for the base patch. Also fix the same problem in the IP based group membership.
  • Loading branch information
@perlDreamer
perlDreamer committed Aug 23, 2011
1 parent 8ab2dcc commit e65368c
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 31 deletions.
1 change: 1 addition & 0 deletions docs/changelog/7.x.x.txt
View file
@@ -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)
Expand Down
33 changes: 24 additions & 9 deletions lib/WebGUI/Group.pm
View file
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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);
Expand All @@ -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=?)";
Expand All @@ -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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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)
) {
Expand Down
22 changes: 12 additions & 10 deletions t/Group.t
View file
Expand Up @@ -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");
}


Expand All @@ -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});
Expand All @@ -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';

}
Expand All @@ -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);
Expand All @@ -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'
);
Expand All @@ -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;
Expand Down
46 changes: 34 additions & 12 deletions t/Group/group_scratch.t
View file
Expand Up @@ -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');

0 comments on commit e65368c

Please sign in to comment.