plainblack.com
Username Password
search
Bookmark and Share
View All Tickets
Visitor group by scratch membership shared among all Visitors  (#12195)
Issue
Problem: group-by-scratch-value membership is shared among all Visitors as soon as one person joins the group. All Visitors are members of the group as long as one Visitor has the scratch value set.

Cause: Group->hasScratchUser only checks userId, not session, to determine whether a user is in a group via scratch value. Therefore, hasScratchUser returns true for all Visitors (userId = 1) as long as one Visitor has the scratch value set.

Solution: Update Group->hasScratchUser to check scratch based on userId AND that user’s sessionId

Discussion: hasScratchUser is only called by hasUser, so the fix is simple: (1) update hasUser to pass both userId and user’s sessionId to hasScratchUser, then (2) update hasScratchUser to limit results by user sessionId. This has been done in the attached patch.

The attached patch passes userId and user sessionId separately to hasScratchUser. However, it may be preferable to pass the user session itself to hasScratchUser, then pull out userId and user sessionId within hasScratchUser. This is how it's done when hasUser is called.

Note 1: The meat of hasUser is a chain of if conditions that check group by IP, karma, scratch, database and LDAP. In each case the group checking function is passed only the userId, so it’s likely that the above patch to hasScratchUser should have an analogous patch applied to hasIpUser, hasKarmaUser, hasDatabaseUser, hasLDAPUser, in which the user's session is considered in addition to userId.

Note 2: With the current patch, session is checked for ALL users, not just Visitors. This means that previously, if a user was logged in to multiple sessions (different browsers, different computers), then the scratch variable group membership was shared among all the sessions for this user. The patch will cause the scratch group membership to now only be applied to the session in which the scratch value was set. To limit the new behavior to only Visitors, you’d have to build a condition into the SQL statement that includes the session restriction only if userId == 1.

Note 3: Despite this patch, caching is still an issue for Visitors. If the visitor joins a group via scratch variable, then clicks to a page they had previously viewed (and the link has no query string to force a reload) the user is shown a cached version of the page, not the updated version that might be different because of the scratch variable now being set. Is there any way to update the caching behavior to clear the Visitor’s cache every time they add/remove a group via scratch, IP or other methods available to Visitor? I presume that the cache is still specific per user, not shared among all Visitors, otherwise you’d have to take even more drastic measures.
  1. group_scratch2.t
    This is an expansion of the existing test in WG 7.9.32, to include tests for visitors who have a separate session than the one to which scratch values are applied. Compare results of this test between the current Group.pm and the patched Group.pm for 7.9.32

  2. Group.patch
    The patch that updates Group.pm in 7.9.32 to check for user session in hasScratchUser

  3. Group.pm.new
    A copy of Group.pm with the patch applied

  4. Group.pm.comment
    The same as #3, only with comments specific to the patch
Solution Summary
Comments
Trex
0
8/1/2011 11:15 am
To add support to the idea that this is a bug and not a feature request, please see the final comment on this previous bug report:

http://www.webgui.org/use/bugs/tracker/11552#field_id_comment_lr9aCOZtGhvu6-iziKUYQQ

If IP and scratch groups are being cached by both userId and sessionId, then that would only make sense if the group membership lookup for scratch-based groups is also by both userId and sessionId.
perlDreamer
0
8/22/2011 7:59 pm
Applying this patch causes tests in Group.t to fail.
perlDreamer
0
8/22/2011 9:27 pm
I modified the patch a little, mainly whitespace changes, and fixed the failing tests in Group.t  Then, I made the same kind of changes to hasIpUsers, because it also did not check for a unique sessionId.

Fixed in 7.10.23 (e65368c7c17e4a8fb01bc9477d8832912316c583)
Trex
5
8/22/2011 9:37 pm
Thank you! I had just started digging into the original Group.t to see if the failures you reported were due to the patch or whether the tests needed to be updated due to the changes made by the patch. I will presume you found that it was the latter.

I'll confirm and close this, even though I'm not currently set up with 7.10.
Details
Ticket Status Closed  
Rating5.0 
Submitted ByTrex 
Date Submitted2011-07-13 
Assigned To unassigned  
Date Assigned2019-05-20 
Assigned By 
Severity Critical (mostly not working)  
What's the bug in? WebGUI Stable  
WebGUI / WRE Version 7.9.32  
URLuse/bugs/tracker/12195
Keywords
Related Files
Ticket History
8/23/2011
2:37 AM
Closed Trex
8/23/2011
2:27 AM
Resolved perlDreamer
7/13/2011
4:17 PM
Ticket created Trex
© 2019 Plain Black Corporation | All Rights Reserved