Opened 10 years ago
Last modified 9 years ago
#11474 closed enhancement
Fine grained permission checks for EMAIL_VIEW are skipped when formatting author — at Version 6
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: | Branch: | ||
Release Notes: | |||
API Changes: |
The |
||
Internal Changes: |
Description
Permission checks for EMAIL_VIEW
are done in the following places:
- branches/1.0-stable/trac/ticket/default_workflow.py@12083:232#L222
- Fine-grained permission checks are implemented.
- branches/1.0-stable/trac/ticket/web_ui.py@12322:1749,1764#L1715
- Fine-grained permission checks are implemented.
- 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.
- Fine-grained permission checks can't be implemented in this location, however the boolean could be changed to a function that takes a
- branches/1.0-stable/trac/web/chrome.py@12468:1078,1109#L1069
- Fine-grained permission checks should be done in
format_emails
andformat_author
.
- Fine-grained permission checks should be done in
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.
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 47 47 <a href="${event.render('url', context)}" py:choose=""> 48 48 <py:when test="event.author"><i18n:msg params="time, title, author"> 49 49 <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> 51 51 </i18n:msg></py:when> 52 52 <py:otherwise> 53 53 <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): 1103 1103 return match.group(1) or match.group(2) 1104 1104 return author 1105 1105 1106 def format_author(self, req, author ):1106 def format_author(self, req, author, resource=None): 1107 1107 if not author or author == 'anonymous': 1108 1108 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): 1110 1111 return author 1111 1112 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.
Change History (12)
by , 10 years ago
Attachment: | workflow1.png added |
---|
by , 10 years ago
Attachment: | workflow2.png added |
---|
by , 10 years ago
Attachment: | ticketproperties.png added |
---|
comment:2 by , 10 years ago
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]).
by , 10 years ago
Attachment: | workflow3.png added |
---|
by , 10 years ago
Attachment: | workflow4.png added |
---|
by , 10 years ago
Attachment: | PropertyChange.png added |
---|
comment:3 by , 10 years ago
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 by , 10 years ago
Replying to rjollos:
On a related note, I wonder if it would make sense to have
Chrome
implementIPermissionRequestor
and returnEMAIL_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:6 by , 10 years ago
API Changes: | modified (diff) |
---|---|
Status: | new → assigned |
Thanks for the review. Committed to trunk in [12930].
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)