Edgewall Software
Modify

Opened 2 years ago

Closed 2 years ago

Last modified 12 months ago

#12719 closed enhancement (fixed)

Add default permission policies for ticket and wiki realms

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.2
Component: general Version:
Severity: normal Keywords: permissions
Cc: Branch:
Release Notes:
  • Added a new permission policy for the ticket system (DefaultTicketPolicy):
    • Authenticated user with TICKET_APPEND or TICKET_CHGPROP can modify description of ticket they reported.
  • Renamed ReadonlyWikiPolicy to DefaultWikiPolicy.
  • Modified LegacyAttachmentPolicy to allow authenticated users to delete their own attachments.
API Changes:

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

Attachments (0)

Change History (28)

comment:1 by Ryan J Ollos, 2 years ago

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

comment:2 by Ryan J Ollos, 2 years ago

Description: modified (diff)

comment:3 by Peter Suter, 2 years ago

Seems quite nice!

users with TICKET_CHGPROP

This restriction seems to be missing from the proposed code?

in reply to:  3 comment:4 by Ryan J Ollos, 2 years ago

Replying to Peter Suter:

This restriction seems to be missing from the proposed code?

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

comment:5 by Ryan J Ollos, 2 years ago

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

    diff --git a/trac/ticket/web_ui.py b/trac/ticket/web_ui.py
    index f22a707ae..a9e08b879 100644
    a b class TicketModule(Component):  
    12881288                ticket.populate(ticket._old)
    12891289
    12901290        comment = req.args.get('comment')
    1291         if comment:
    1292             if not ('TICKET_CHGPROP' in req.perm(resource) or
    1293                     'TICKET_APPEND' in req.perm(resource)):
    1294                 add_warning(req, _("No permissions to add a comment."))
    1295                 valid = False
     1291        if comment and 'TICKET_APPEND' not in req.perm(resource):
     1292            add_warning(req, _("No permissions to add a comment."))
     1293            valid = False
    12961294
    12971295        # Mid air collision?
    12981296        if ticket.exists and (ticket._old or comment or force_collision_check):

I will add a test before committing.

comment:6 by Ryan J Ollos, 2 years ago

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

comment:7 by Ryan J Ollos, 2 years ago

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'

comment:8 by Christian Boos, 2 years ago

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.

comment:9 by Jun Omae, 2 years ago

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 reutrns None.

comment:10 by Ryan J Ollos, 2 years ago

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.

in reply to:  9 comment:11 by Christian Boos, 2 years ago

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 ;-)

comment:12 by Ryan J Ollos, 2 years ago

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

comment:13 by Ryan J Ollos, 2 years ago

Milestone: next-dev-1.3.x1.3.2
Owner: set to Ryan J Ollos
Status: newassigned

comment:14 by Ryan J Ollos, 2 years ago

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

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

comment:15 by Jun Omae, 2 years ago

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

in reply to:  12 comment:16 by Jun Omae, 2 years ago

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

comment:17 by Ryan J Ollos, 2 years ago

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

comment:18 by Jun Omae, 2 years ago

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

comment:19 by Ryan J Ollos, 2 years ago

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.

comment:20 by Ryan J Ollos, 2 years ago

Release Notes: modified (diff)

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

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

comment:21 by Ryan J Ollos, 2 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in r15749.

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

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

comment:22 by anonymous, 2 years ago

Resolution: fixed
Status: closedreopened

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

comment:23 by Ryan J Ollos, 2 years ago

Resolution: fixed
Status: reopenedclosed

Missing upgrade script added in r15765.

in reply to:  20 comment:24 by Ryan J Ollos, 2 years ago

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.

comment:26 by anonymous, 12 months ago

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

comment:27 by Ryan J Ollos, 12 months ago

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.

comment:28 by Jun Omae, 12 months ago

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.

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.