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

Priority: Urgent
Current State: Closed
Released In: 2.0.0
Target Release: major
Applies To: Extension
Component: MailerContrib
Reported By: GeorgeClark
Waiting For:
Last Change By: GeorgeClark
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 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
Test topic


Unrestricted web diff
asdf somechange  asdf

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 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, 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


ItemTemplate edit

Summary MailerContrib fails to generate diff correctly on view restricted webs
ReportedBy GeorgeClark
Codebase 1.1.9
SVN Range
AppliesTo Extension
Component MailerContrib
Priority Urgent
CurrentState Closed
TargetRelease major
ReleasedIn 2.0.0
Topic revision: r5 - 05 Jul 2015, GeorgeClark - This page was cached on 27 Oct 2016 - 03:06.

The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License