Item13126: MailerContrib fails to generate diff correctly on view restricted webs

When the web is view restricted, the diff is mangled - starts with the web.topic name and the first line or two of topic, no diff actually is created.

This is reported IRC Log and on the discuss mailing list.

-- GeorgeClark - 01 Dec 2014

I can confirm that I've recreated this locally. Task http://foswiki.org/Tasks/Item13126 opened.

1. Add user to WebNotify topic of two webs, one without restrictions, one with ALLOWWEBVIEW = SomeUser
2. Create two identical topics. One in web without view restriction, one in web with ALLOWWEBVIEW = SomeUser.
3. run mailnotify to flush the notifications
4. Edit topics, make a minor change, and force save as a new rev.
5. Run mailnotify.

The Diff of the topic in the web with view restrictions is wrong.

Restricted web diff

Restricted.SomeNewTopic
Restricted.SomeNewTopic
Test topic

asdf

Unrestricted web diff

asdf
asdf
asdf somechange  asdf
asdf
wq34
agd BAR BAR

On 11/30/2014 10:12 PM, Heath Raftery wrote:
> I've completed my investigation and found the heart of the problem. I've 
> only implemented a workaround though because I can't see how it's 
> supposed to work. Perhaps someone can suggest a proper fix:
>
> In Meta.pm line 3384, it calls:
>
> $oldTopicObject->haveAccess('VIEW')
>
> This returns false because at this point $session->{user} is 
> BaseUserMapping_666, and because I have a ALLOWWEBVIEW set for this web, 
> it doesn't find that user listed there.
>
> Thus, the oldTopicObject is blanked and the diff continues by comparing 
> the current rev to a blank version.
>
> My workaround is to remove that blanking code. Alternatively, adding 
> "BaseUserMapping_666" to ALLOWWEBVIEW also worked.
>
> I can't understand why the check is done here in the first place - 
> access to the current rev is also checked a few lines earlier, which 
> also fails, but then it goes and grabs the topic text anyway.
>
> More strangely, much earlier in WebNotify.pm:260, checkAccessPermission 
> is called using the actual subscriber user and the topic, so it seems to 
> me that permissions have already been checked.
>
> My guess is that Meta is designed to be called when a user is logged in 
> through the browser, so $session->{user} would normally be the current 
> user. Even so, it seems strange to retrieve the topic text, and then 
> check whether VIEW access is available.
>
> In summary: in Foswiki 1.1.9 and MailerContrib 2.60, setting a 
> ALLOWWEBVIEW in WebPreferences breaks the diff section of the mailnotify 
> emails because Meta::summariseChanges calls Meta::haveAccess which 
> returns false when BaseUserMapping_666 is not found in the ALLOWWEBVIEW 
> list.
>
> Regards,
> Heath

-- GeorgeClark - 01 Dec 2014

sigh several revs of MailerContrib ago, we used to 'spoof' the current user so that all lower-level access control checks operated as the mailing user. There were other access control issues that made that a really bad idea, so I changed it - obviously without working through all the possible implications.

The solution is for the call to summariseChanges to accept an optional parameter of the CUID of the calling user (defaulting it to the default user). That way we maintain the API, but support checking for a different user.

-- CrawfordCurrie - 01 Dec 2014

OK, changed it so if it can't get at either the start or end revs using the subscriber's ID it doesn't report the change.

-- CrawfordCurrie - 16 Dec 2014

Commenting is disabled while not logged in