Add default permission policies for ticket and wiki realms

[Ryan J Ollos]

These ideas are generated from ​gmessage:trac-users:T-bb2GAvqxI/tFs0_7yQDAAJ:

• By default users are allowed to edit their own comments, and I think in most cases it makes sense to allow users with TICKET_CHGPROP to edit their own ticket description. We could allow users to edit their own ticket descriptions through a permission policy, thus making it easy for Trac sites that don't want the behavior to change it by replacing the permission policy or adding a different policy earlier in the list of permission policies.
• #10909 requests a permission for allowing a user to edit their own comment. Alternatively, we could just move the edit own comment behavior to a permission policy, which can then be replaced by sites that want different behavior.
• ReadonlyWikiPolicy is very specific. Also, if a site wants to replace the policy, for example adding a custom permission for editing readonly pages, a new policy with the same name needs to be implemented (due to tags/trac-1.2/trac/wiki/web_ui.py@:567#L558​). It would be better to allow an arbitrary policy name, and by default to have a general wiki policy that implements the readonly behavior. The wiki policy can be extended in the future with additional rules for the wiki realm.

The proposed changes implement the described rules in two policies, DefaultTicketPolicy and DefaultWikiPolicy, which can then be extended in the future with additional rules for the ticket and wiki realms. I like the idea of having specific policies associated with realms, and moving the aforementioned behaviors out of the Module classes and into permission policies.

[Ryan J Ollos]

Ideas are captured in log:rjollos.git:t12719_new_permission_policies. More testing and code review is needed before considering to commit.

[Peter Suter]

Seems quite nice!

users with TICKET_CHGPROP

This restriction seems to be missing from the proposed code?

[Ryan J Ollos]

Replying to Peter Suter:

This restriction seems to be missing from the proposed code?

Yes, thanks for spotting. Added in [373bbdad/rjollos.git].

[Ryan J Ollos]

I noticed that TICKET_CHGPROP is checked in _validate_ticket, but only TICKET_APPEND should allow adding a comment. Proposed change for 1.2-stable:

trac/ticket/web_ui.py

@@ -1288 +1288 @@
                 ticket.populate(ticket._old)
 
         comment = req.args.get('comment')
-        if comment:
-            if not ('TICKET_CHGPROP' in req.perm(resource) or
-                    'TICKET_APPEND' in req.perm(resource)):
-                add_warning(req, _("No permissions to add a comment."))
-                valid = False
+        if comment and 'TICKET_APPEND' not in req.perm(resource):
+            add_warning(req, _("No permissions to add a comment."))
+            valid = False
 
         # Mid air collision?
         if ticket.exists and (ticket._old or comment or force_collision_check):

I will add a test before committing.

[Ryan J Ollos]

comment:5 change committed in [15665:15666].

[Ryan J Ollos]

I noticed again in tags/trac-1.2/trac/ticket/web_ui.py@:615-616#L608​, the following pattern that is seen throughout Trac:

req.authname and req.authname != 'anonymous'

I can't think of any scenario in which this isn't equivalent to:

req.authname != 'anonymous'

[Christian Boos]

When req.authname is None? If you look at LoginModule.authenticate​, this can happen.

But I agree, we could use something more explicit, e.g. req.is_authenticated or req.is_logged_in.

[Jun Omae]

It seems that req.authname is determined by RequestDispatcher.authenticateat tags/trac-1.2/trac/web/main.py@:169,196#L169​, not LoginModule.authenticate. I think this cannot happen.

RequestDispatcher.authenticate returns anonymous when all authenticators return false value. Then, req.authname would be anonymous even if LoginModule.authenticate returns None.

[Ryan J Ollos]

I considered adding req.is_authenticated in #12231. It might be worth looking at again, but I'm glad to know that req.authname shouldn't be None.

[Christian Boos]

Replying to Jun Omae:

It seems that req.authname is determined by RequestDispatcher.authenticateat tags/trac-1.2/trac/web/main.py@:169,196#L169​, not LoginModule.authenticate. I think this cannot happen.

You're right, my bad. I had a quick look… at the wrong place ;-)

Go for #12231 ;-)

[Ryan J Ollos]

Maybe we should also allow users to replace their own attachments in the default permission policy?

[Ryan J Ollos]

DONE Update description of TICKET_EDIT_DESCRIPTION in TracPermissions#TicketSystem when these changes are committed.

[Jun Omae]

I think we should check the user is not anonymous. Otherwise, any changes of anonymous could be modified by any anonymous users.

             ticket = Ticket(self.env, resource.id)
-            if username != 'anonymous' and username == ticket['reporter']:
+            if username == ticket['reporter']:
                 return True
 ...
             change = ticket.get_change(resource.id)
-            if change and username == change['author']:
+            if change and username != 'anonymous' username == change['author']:
                 return True

[Jun Omae]

Replying to Ryan J Ollos:

Maybe we should also allow users to replace their own attachments in the default permission policy?

Sounds good to me. The check is implemented at tags/trac-1.3.1/trac/attachment.py@:451-454#L447​. However, there is no action for replacing attachment.

BTW, I've recently implemented policy which allows users to delete their own attachments for my production. TICKET_APPEND is required to attach new files but TICKET_ADMIN is required to delete attachments from ticket. The attachments cannot be deleted for almost users. That behavior is inconvenient….

[Ryan J Ollos]

Allowing users to delete their own attachments sounds good. Do you think that should be allowed for all realms?

[Jun Omae]

Yes. I think we should be allowed for all realms.

[Ryan J Ollos]

I think it makes sense to require TICKET_APPEND or TICKET_CHGPROP to edit own ticket description. Originally I had thought to only require TICKET_CHGPROP, but even if user's aren't allowed to edit other ticket properties I think it would make sense to allow one to edit their own ticket description as long as they have permission to comment.

I added suggested changes to log:rjollos.git:t12719_new_permission_policies.1, along with additional changes.

[Ryan J Ollos]

The readonly_wiki attribute seems to be compensating for the lack of EvenFinerGrainedPermissions, and doesn't even function as intended since WIKI_ADMIN is required for the checkbox to be displayed. As intended it would allow, for instance, a special permission be defined and used to control whether the checkbox is rendered. What we really want is a check like 'modify_readonly' in perm(resource).

As a workaround I used a permission, WIKI_CHANGE_READONLY, that isn't defined through an IPermissionRequestor and therefore can't be directly granted to a user (in that way, it's like the permissions defined in LegacyAttachmentPolicy). A user can now implement a replacement for DefaultWikiPolicy and control the rendering of the readonly checkbox. I consider this to be non-ideal, but it's just an implementation detail that can go away when we implement the finer grained permission scheme.

Proposed change: [453181d8d/rjollos.git].

• DONE Replace ReadonlyWikiPolicy with DefaultWikiPolicy, DefaultTicketPolicy throughout documentation.
• DONE Document changes on 1.3/TracUpgrade.
• DONE Document LegacyAttachmentPolicy on TracPermissions page.

[Ryan J Ollos]

Committed to trunk in r15749.

See also comment:9:ticket:8778. With EvenFinerGrainedPermissions, the implementation would presumably be something like:

[wiki:*::readonly]
anonymous = !edit

[anonymous]

r15749 bumps DB version to 43 but there is no upgrade script.

[Ryan J Ollos]

Missing upgrade script added in r15765.

[Ryan J Ollos]

Replying to Ryan J Ollos:

As a workaround I used a permission, WIKI_CHANGE_READONLY, that isn't defined through an IPermissionRequestor and therefore can't be directly granted to a user (in that way, it's like the permissions defined in LegacyAttachmentPolicy). A user can now implement a replacement for DefaultWikiPolicy and control the rendering of the readonly checkbox.

Similarly, comment:14:ticket:8778 proposes adding TICKET_EDIT_MILESTONE without defining the action in an IPermissionRequestor implementation.

[Ryan J Ollos]

Edited:

• 1.3/TracFineGrainedPermissions@2
• ReadonlyWikiPolicy@6
• 1.3/TracUpgrade@9

[anonymous]

I upgraded an old Trac environment. The setting permission_policies = DefaultPermissionPolicy,LegacyAttachmentPolicy was set, so the new policies were not added automatically. I missed this completely due to my own fault: I hadn't bothered to look at TracUpgrade and also didn't pay attention to the INFO output of trac-admin.

Only later it was noticed that the milestone field has disappeared. I then found in the log that "No policy allowed anonymous performing TICKET_CHG_MILESTONE on …" and from there figured out my mistake.

I only report this back for your amusement and in case it helps anyone else googling for "Trac milestone field missing".

[Ryan J Ollos]

DefaultTicketPolicy constructs a Ticket object when checking certain actions, which results in 2 queries:

SELECT summary,reporter,owner,description,type,status,priority,milestone,component,version,severity,resolution,keywords,cc,time,changetime FROM ticket WHERE id=%s
SELECT name, value FROM ticket_custom WHERE ticket=%s

For an authenticated user with TICKET_APPEND, viewing a ticket results in the constructions of (2 + number of comments) Ticket objects. We probably want to make that more efficient. I'm exploring how we might do that, such as caching the objects in an attribute of PermissionCache.

[Jun Omae]

I've considered that I'd like to modify PermissionCache and check_permission to accept a model instance rather than a resource instance to prevent fetching records each call.

[Ryan J Ollos]

Added documentation in TracPermissions@107.

[Ryan J Ollos]

See #13282 for related changes.

[Ryan J Ollos]

Replying to Jun Omae:

I've considered that I'd like to modify PermissionCache and check_permission to accept a model instance rather than a resource instance to prevent fetching records each call.

That does sound interesting and nice because resource can be easily fetched through ticket.resource. Also, it would likely be backward compatible to most permission policies since both Ticket and Resource have the same fields: realm, id, versions (but not parent).