Edgewall Software
Modify

Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#11474 closed enhancement (fixed)

Fine grained permission checks for EMAIL_VIEW are skipped when formatting author

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.6
Component: web frontend Version:
Severity: normal Keywords: email obfuscation
Cc:
Release Notes:

Fine-grained permission checks are implemented when rendering an author in the ticket workflow dialog.

API Changes:
  • The EMAIL_VIEW permission is defined in the Chrome class rather than the PermissionSystem class.
  • Chrome.format_author and Chrome.authorinfo perform fine-grained permission checks when passed an optional Resource object.
  • show_email_addresses is deprecated in the Chrome data dictionary that is used to render templates.

Description (last modified by 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

    diff --git a/trac/timeline/templates/timeline.html b/trac/timeline/templates/timeline.html
    index 957222f..571d6bc 100644
    a b  
    4747              <a href="${event.render('url', context)}" py:choose="">
    4848                <py:when test="event.author"><i18n:msg params="time, title, author">
    4949                  <span class="time">${format_time(event.date, 'short')}</span> ${event.render('title', context)}
    50                   by <span class="author">${format_author(event.author)}</span>
     50                  by <span class="author">${format_author(event.author, Resource('timeline'))}</span>
    5151                </i18n:msg></py:when>
    5252                <py:otherwise>
    5353                  <span class="time">${format_time(event.date, 'short')}</span> ${event.render('title', context)}
  • trac/web/chrome.py

    diff --git a/trac/web/chrome.py b/trac/web/chrome.py
    index 1ac7970..d4441fa 100644
    a b class Chrome(Component):  
    11031103            return match.group(1) or match.group(2)
    11041104        return author
    11051105
    1106     def format_author(self, req, author):
     1106    def format_author(self, req, author, resource=None):
    11071107        if not author or author == 'anonymous':
    11081108            return _("anonymous")
    1109         if self.show_email_addresses or not req or 'EMAIL_VIEW' in req.perm:
     1109        if self.show_email_addresses or not req or \
     1110                'EMAIL_VIEW' in req.perm(resource):
    11101111            return author
    11111112        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.

Attachments (6)

workflow1.png (42.3 KB ) - added by Ryan J Ollos 4 years ago.
workflow2.png (43.8 KB ) - added by Ryan J Ollos 4 years ago.
ticketproperties.png (20.5 KB ) - added by Ryan J Ollos 4 years ago.
workflow3.png (41.9 KB ) - added by Ryan J Ollos 4 years ago.
workflow4.png (43.7 KB ) - added by Ryan J Ollos 4 years ago.
PropertyChange.png (15.3 KB ) - added by Ryan J Ollos 4 years ago.

Download all attachments as: .zip

Change History (29)

Changed 4 years ago by Ryan J Ollos

Attachment: workflow1.png added

Changed 4 years ago by Ryan J Ollos

Attachment: workflow2.png added

Changed 4 years ago by Ryan J Ollos

Attachment: ticketproperties.png added

comment:1 Changed 4 years ago by 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)

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:2 Changed 4 years ago by 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]).

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

Changed 4 years ago by Ryan J Ollos

Attachment: workflow3.png added

Changed 4 years ago by Ryan J Ollos

Attachment: workflow4.png added

Changed 4 years ago by Ryan J Ollos

Attachment: PropertyChange.png added

comment:3 Changed 4 years ago by 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)

comment:4 in reply to:  description Changed 3 years ago by 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.

comment:5 Changed 3 years ago by Peter Suter

Looks good to me.

comment:6 Changed 3 years ago by Ryan J Ollos

API Changes: modified (diff)
Status: newassigned

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

comment:7 Changed 3 years ago by Ryan J Ollos

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

comment:8 Changed 3 years ago by Ryan J Ollos

Milestone: 1.0.31.0.2

comment:9 Changed 3 years ago by Ryan J Ollos

Milestone: 1.0.21.0.3

comment:10 Changed 3 years ago by 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.

comment:11 Changed 3 years ago by 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]..

Last edited 2 years ago by Ryan J Ollos (previous) (diff)

comment:14 Changed 3 years ago by Ryan J Ollos

Milestone: 1.0.31.0.4

comment:15 Changed 3 years ago by Ryan J Ollos

Milestone: 1.0.41.0.5

comment:16 Changed 3 years ago by Ryan J Ollos

Milestone: 1.0.51.0.6

Narrowing focus for 1.0.5.

comment:17 Changed 3 years ago by Ryan J Ollos

Milestone: 1.0.61.1.5

comment:18 Changed 3 years ago by Ryan J Ollos

Milestone: 1.1.51.2

comment:19 Changed 3 years ago by Ryan J Ollos

Milestone: 1.21.1.6

Milestone renamed

comment:20 Changed 2 years ago by 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.

Last edited 2 years ago by Ryan J Ollos (previous) (diff)

comment:21 Changed 2 years ago by Ryan J Ollos

API Changes: modified (diff)
Release Notes: modified (diff)

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

comment:22 Changed 2 years ago by Ryan J Ollos

Description: modified (diff)

comment:23 Changed 2 years ago by Ryan J Ollos

Resolution: fixed
Status: assignedclosed

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].

comment:24 Changed 2 years ago by Ryan J Ollos

Added docstrings to modified Chrome methods in [14168].

comment:25 Changed 2 years ago by 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.

Last edited 2 years ago by Ryan J Ollos (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .

 
Note: See TracTickets for help on using tickets.