Fine grained permission checks for EMAIL_VIEW are skipped when formatting author

[Ryan J Ollos]

Permission checks for EMAIL_VIEW are done in the following places:

1. branches/1.0-stable/trac/ticket/default_workflow.py@12083:232#L222​
   • Fine-grained permission checks are implemented.
2. branches/1.0-stable/trac/ticket/web_ui.py@12322:1749,1764#L1715​
   • Fine-grained permission checks are implemented.
3. branches/1.0-stable/trac/web/chrome.py@12468:864#L827​
   • Fine-grained permission checks can't be implemented in this location, however the boolean could be changed to a function that takes a Resource argument. This needs to be investigated further.
4. branches/1.0-stable/trac/web/chrome.py@12468:1078,1109#L1069​
   • Fine-grained permission checks should be done in format_emails and format_author.

For the case,

[timeline:*]
* = EMAIL_VIEW

the following changes fix the issue:

trac/timeline/templates/timeline.html

@@ -47 +47 @@
               <a href="${event.render('url', context)}" py:choose="">
                 <py:when test="event.author"><i18n:msg params="time, title, author">
                   <span class="time">${format_time(event.date, 'short')}</span> ${event.render('title', context)}
-                  by <span class="author">${format_author(event.author)}</span>
+                  by <span class="author">${format_author(event.author, Resource('timeline'))}</span>
                 </i18n:msg></py:when>
                 <py:otherwise>
                   <span class="time">${format_time(event.date, 'short')}</span> ${event.render('title', context)}

trac/web/chrome.py

@@ -1103 +1103 @@
             return match.group(1) or match.group(2)
         return author
 
-    def format_author(self, req, author):
+    def format_author(self, req, author, resource=None):
         if not author or author == 'anonymous':
             return _("anonymous")
-        if self.show_email_addresses or not req or 'EMAIL_VIEW' in req.perm:
+        if self.show_email_addresses or not req or \
+                'EMAIL_VIEW' in req.perm(resource):
             return author
         return obfuscate_email_address(author) 

However, that's just a quick implementation, and more care is needed to fully implement the changes. It may be possible to simplify the behavior by isolating the EMAIL_VIEW checks to the Chrome class.

This ties in closely with some other changes that were discovered while working on #10018. Those issues will be reported in other tickets.

On a related note, I wonder if it would make sense to have Chrome implement IPermissionRequestor and return EMAIL_VIEW, rather than defining the permission in perm.py​, which is a bit out of place.

[Ryan J Ollos]

Here is another issue we can fix in this ticket. In the ticket properties box, anonymous is translated. However, anonymous is not translated in the workflow hints.

(See #11689)

[Ryan J Ollos]

Part of the changes are a follow-on to #9322 / [9667]. In render_ticket_action_control​, the method Chrome.format_author​ is nearly duplicated. After passing resource to Chrome.format_author, Chrome.format_author can be used to replace the format_user in render_ticket_action_control. Additionally, this will resolve the issue in comment:1, since anonymous is translated by Chrome.format_author. On the trunk we'll want to replace format_author with Chrome.authorinfo (#11145 / [12340]).

[Ryan J Ollos]

Another issue is that anonymous is not translated when rendering a property diff. This only applies to the CC, Owner and Reporter fields (I can't see how having anonymous in the CC field is even a valid use-case). Initially, I was going out of my way to avoid translating this string since we typically don't translate property values. Even in the case of translating property values though, the aim is to not translate values stored in the database. Therefore, I'll probably just drop obfuscate_author from the proposed changes and instead use format_author, unless anyone feels differently.

    def format_author(self, req, author, resource=None):
        """Obfuscates the email address by calling `obfuscate_author` and
        also translates //anonymous//.
        """
        if not author or author == 'anonymous':
            return _("anonymous")
        return obfuscate_author(req, author, resource)

    def obfuscate_author(self, req, author, resource=None):
        """Obfuscates the email addresses according to the
        `[trac] show_email_addresses` setting and the `EMAIL_VIEW`
        permission. Returns `author` if `author` is not an email address."""
        if self.show_email_addresses or not req or \
                'EMAIL_VIEW' in req.perm(resource):
            return author
        return obfuscate_email_address(author)

[Ryan J Ollos]

Replying to rjollos:

On a related note, I wonder if it would make sense to have Chrome implement IPermissionRequestor and return EMAIL_VIEW, rather than defining the permission in perm.py​, which is a bit out of place.

Proposed changes to address this issue in log:rjollos.git:t11474.1, for commit to the trunk.

[Peter Suter]

Looks good to me.

[Ryan J Ollos]

Thanks for the review. Committed to trunk in [12930].

[Ryan J Ollos]

Latest changes can be found in log:rjollos.git:t11474.2, however more changes will be proposed.

[Ryan J Ollos]

The translation of the workflow action hints (comment:1) was fixed in #11689. Changes for this branch have been rebased in log:rjollos.git:t11474.3.

[Ryan J Ollos]

comment:16:ticket:11145 suggests deprecating the show_email_addresses boolean that is passed in the chrome data dictionary after the changes in this ticket are committed, since it will no longer be used outside of the Chrome class in the code on the trunk. We'll need to consider whether it has any utility, and if not it would be useful to simplify the API by deprecating it for removal. The functions author_email, author_info format_author and format_emails will hopefully provide all of the needed email obfuscation capabilities in a robust form, and the absence of show_email_addresses would push plugin authors away from writing code like that which was refactored in [12992#file0]..

[Ryan J Ollos]

Narrowing focus for 1.0.5.

[Ryan J Ollos]

Milestone renamed

[Ryan J Ollos]

Latest changes against 1.0-stable can be found in log:rjollos.git:t11474.4. I'm rebasing the changes for commit on the trunk though.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11474.5.

[Ryan J Ollos]

I didn't address the issue mentioned in comment:description with rendering events in the timeline. I no longer think the proper fix is passing Resource('timeline'). Shouldn't the permissions checks instead be done for the resource that is being rendered in the timeline? For that we would need to attach a Resource object to event and call ${format_author(event.author, Resource(event.resource))}. I'm unsure if that's even correct, and the use-case for the changes is rather obscure, so I'm going to skip making any changes for now.

Changes committed to trunk in [14151].

[Ryan J Ollos]

Added docstrings to modified Chrome methods in [14168].

[Ryan J Ollos]

[14196] fixes a regression in [14151] in which rendered property changes were incorrect. When changing the owner or reporter, from <prop> to (none) and from (none) to <prop> would be displayed in ticket property changes rather than set to <prop> and <prop> deleted.