#12719 closed enhancement (fixed)
Add default permission policies for ticket and wiki realms — at Version 21
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: |
|
||
API Changes: | |||
Internal Changes: |
Description (last modified by )
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.
Change History (21)
comment:1 by , 7 years ago
comment:2 by , 7 years ago
Description: | modified (diff) |
---|
follow-up: 4 comment:3 by , 7 years ago
Seems quite nice!
users with
TICKET_CHGPROP
This restriction seems to be missing from the proposed code?
comment:4 by , 7 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 , 7 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): 1288 1288 ticket.populate(ticket._old) 1289 1289 1290 1290 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 1296 1294 1297 1295 # Mid air collision? 1298 1296 if ticket.exists and (ticket._old or comment or force_collision_check):
I will add a test before committing.
comment:7 by , 7 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 , 7 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
.
follow-up: 11 comment:9 by , 7 years ago
It seems that req.authname
is determined by RequestDispatcher.authenticate
at 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
.
comment:10 by , 7 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
.
comment:11 by , 7 years ago
Replying to Jun Omae:
It seems that
req.authname
is determined byRequestDispatcher.authenticate
at tags/trac-1.2/trac/web/main.py@:169,196#L169, notLoginModule.authenticate
. I think this cannot happen.
You're right, my bad. I had a quick look… at the wrong place ;-)
Go for #12231 ;-)
follow-up: 16 comment:12 by , 7 years ago
Maybe we should also allow users to replace their own attachments in the default permission policy?
comment:13 by , 7 years ago
Milestone: | next-dev-1.3.x → 1.3.2 |
---|---|
Owner: | set to |
Status: | new → assigned |
comment:14 by , 7 years ago
DONE Update description of TICKET_EDIT_DESCRIPTION
in TracPermissions#TicketSystem when these changes are committed.
comment:15 by , 7 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
comment:16 by , 7 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 , 7 years ago
Allowing users to delete their own attachments sounds good. Do you think that should be allowed for all realms?
comment:19 by , 7 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 , 7 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].
- DONE Replace
ReadonlyWikiPolicy
withDefaultWikiPolicy, DefaultTicketPolicy
throughout documentation. - DONE Document changes on 1.3/TracUpgrade.
- DONE Document
LegacyAttachmentPolicy
on TracPermissions page.
comment:21 by , 7 years ago
Release Notes: | modified (diff) |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
Committed to trunk in r15749.
See also comment:9:ticket:8778. With EvenFinerGrainedPermissions, the implementation would presumably be something like:
[wiki:*::readonly] anonymous = !edit
Ideas are captured in log:rjollos.git:t12719_new_permission_policies. More testing and code review is needed before considering to commit.