#11069 closed defect (fixed)
Fine-grained access to the admin panels is not possible — at Version 16
Reported by: | Owned by: | Ryan J Ollos | |
---|---|---|---|
Priority: | normal | Milestone: | 1.0.2 |
Component: | admin/web | Version: | 1.0-stable |
Severity: | normal | Keywords: | permissions authzpolicy |
Cc: | Branch: | ||
Release Notes: |
Implemented fine-grained permission checks for the |
||
API Changes: | |||
Internal Changes: |
Description (last modified by )
I found during the investigation of #11067 that fine-grained permission checks are not being performed for the admin panels, so it is not possible to grant access to these panels using TracFineGrainedPermissions.
The changes start to get complex pretty quickly, so I'll provide a patch, and if there is interest in pulling it in I'll add some functional tests. Similar issues with fine-grained permissions checks exist in #8976 and #11028.
This feature was requested on the mailing list.
Change History (21)
by , 11 years ago
Attachment: | t11069-r11682-1.patch added |
---|
follow-up: 3 comment:1 by , 11 years ago
Here are some of my findings and a description of the changes in t11069-r11682-1.patch:
- I found that users with
PERMISSION_GRANT
orPERMISSION_ADMIN
can only grant permissions that they possess (I'll add documentation for this on the TracPermissions page). The patch includes a change to filter the select list to only those permissions that the user can grant. Prior to the patch the user would see an error when trying to grant those permissions that they don't themselves have, so it seems better to filter the list and just not show those permissions to them. - Users with
PERMISSION_REVOKE
can revoke any permission, so we don't have symmetry withPERMISSION_GRANT
. The patch includes a change to disable the checkboxes and prevent users from revoking permissions that they don't themselves have. I could imagine throwing this part away if it is deemed too complex. However, consider that a user withPERMISSION_REVOKE
could removeTRAC_ADMIN
permission from an administrator, even if they themselves are not an administrator. - To add a subject to a group, the user must have all of the permissions that belong to that group. If not, on the first permission check that fails, an error such as TICKET_CREATE privileges are required to perform this operation. You don't have the required permissions. will be raised. This seems a bit ambiguous and we might want to improve on the message that is displayed. I think this would typically be confusing to a user as to why they can't add a subject to a group.
- The logging panel was missing a permission check for
render_admin_panel
when compared to therender_admin_panel
method for most of the other panels. However, it appears this permission check is not necessary becauserender_admin_panel
won't be called unlessget_admin_panels
yields a tuple, and a permissions check is always done before yielding inget_admin_panels
(get panels → check perm → render panels). Therefore I removed the permission checks for allrender_admin_panels
methods rather than modifying the permissions checks for fine-grained checking. The ticket admin panels don't perform permission checks inrender_admin_panel
or_render_admin_panel
either, presumably for the reason I've described here. - The milestones on the Admin Milestone panel are not filtered by fine-grained permissions checks, and the panel is only shown if the user has the coarse-grained
MILESTONE_VIEW
permission. I spent some time looking at this, and while I feel that we might want to do fine-grained permission checks on the milestones, it seems like this patch is getting overly complex and I need some other opinions on the matter. I didn't dive into doing fine-grained permission checks on the repositories for the same reason (and because it was making my head spin).
This patch definitely needs a functional test, and may be better broken up into multiple patches. If there is interest in integrating this into the core and I get feedback on the questions I've raised here, I'll continue by generating a new patch or patches.
comment:2 by , 11 years ago
Milestone: | → next-stable-1.0.x |
---|---|
Owner: | set to |
Status: | new → assigned |
comment:3 by , 11 years ago
Replying to Ryan J Ollos <ryan.j.ollos@…>:
Here are some of my findings and a description of the changes in t11069-r11682-1.patch
I feel that especially not being able to remove what one couldn't re-add later on is a sensible approach indeed, very nice.
comment:4 by , 11 years ago
Milestone: | next-stable-1.0.x → 1.0.2 |
---|---|
Owner: | changed from | to
Version: | → 1.0-stable |
follow-up: 6 comment:5 by , 11 years ago
Only one thing. In your patch, we should translate 'You don\'t have permission to revoke this action'
and 'Action is no longer defined'
in the title attribute.
comment:6 by , 11 years ago
Replying to jomae:
Only one thing. In your patch, we should translate
'You don\'t have permission to revoke this action'
and'Action is no longer defined'
in the title attribute.
Thanks, I'll make those changes and break up the patch into a sequence of changes that include functional tests.
by , 11 years ago
Attachment: | PermissionsTable.png added |
---|
comment:7 by , 11 years ago
In the forthcoming changes, I've modified the behavior of the table from my previous patch:
- If a user has
PERMISSION_REVOKE
but doesn't have permission to revoke a particular permission (because they don't possess that permission), the checkbox for that permission is disabled (however the label is left unchanged). The title attribute of the checkbox is You don't have permission to revoke this action. - The behavior for missing permissions is preserved. The
missing
class is added and the title attribute for the label is Action is no longer defined.
In the initial patch, I was adding the missing
class for case (1), and the title attribute only existed for the label, with the title text for (2) taking precedence over that for (1).
comment:8 by , 11 years ago
I'm deferring issues (3) and (5) from comment:1 for another ticket in order to get the primary goals of this ticket completed for 1.0.2.
The latest proposed changes are prepared in rjollos.git:t11069.
I changed the signature of FunctionTestEnvironment.enable_authz_permpolicy
, which was added in [11830]. I'm not too worried about changing the signature since this is the first time the method is being used, however I came across a problem with the method while working another ticket (#8976, IIRC). Since I've passed authz_content
to ConfigObj
in a dictionary, we can't be sure the order that the entries will be written, but the order is important for the authz file. For cases that order matters, I guess I'll need to pass the configuration as a string, as shown in the example. I think it will work fine to support both dictionary and string as input with no modifications, but the documentation should be extended.
by , 11 years ago
Attachment: | ManageMilestoneCurrent.png added |
---|
by , 11 years ago
Attachment: | ManageMilestoneDisabled.png added |
---|
follow-ups: 10 11 14 comment:9 by , 11 years ago
admin_milestone.html specifies readonly = True
and disabled = True
in the template when the user doesn't have MILESTONE_MODIFY
. When rendered in the browser (Trac 1.0-stable and Genshi 0.6.1), I see readonly = "True"
and disabled = "disabled"
. I've modified the template so that readonly = 'readonly'
and disabled = 'disabled'
for explicit XHTML compliance (readonly and disabled). Maybe this is a Genshi defect? Genshi should be XHTML compliant?
I made some changes to disable the datepicker when the readonly
attribute is present. However, if we change the readonly
s to disabled
, then the datepicker seems to be implicitly disabled. The first screen capture shows the current rendered template when the user doesn't have MILESTONE_MODIFY
, the second shows the rendered template after changing readonly
to disabled
for four input
elements.
I've also been considering that the wiki toolbar should only be present when the textarea
is enabled:
-
trac/htdocs/js/wikitoolbar.js
diff --git a/trac/htdocs/js/wikitoolbar.js b/trac/htdocs/js/wikitoolbar.js index b5def19..8488898 100644
a b 89 89 // Add the toolbar to all <textarea> elements on the page with the class 90 90 // 'wikitext'. 91 91 jQuery(document).ready(function($) { 92 $("textarea.wikitext").each(function() { addWikiFormattingToolbar(this) }); 92 if ($("textarea.wikitext").is(":enabled")) 93 $("textarea.wikitext").each(function() { addWikiFormattingToolbar(this) }); 93 94 });
follow-up: 12 comment:10 by , 11 years ago
Replying to rjollos:
I've also been considering that the wiki toolbar should only be present when the
textarea
is enabled:
I wouldn't do that. In the case where a disabled textarea.wikitext
is in the generated XHTML, and it is enabled dynamically through JavaScript (e.g. after setting a checkbox), the wiki toolbar would be missing (or would have to be added once explicitly in the checkbox handler). Always having the toolbar is also visually more consistent.
follow-up: 13 comment:11 by , 11 years ago
Replying to rjollos:
I see
readonly = "True"
anddisabled = "disabled"
. These have been changed in the template toreadonly = 'readonly'
anddisabled = 'disabled'
for explicit XHTML compliance. Maybe this is a Genshi defect?
Do you think 'readonly'
should be added to Genshi's list in XHTMLSerializer._BOOLEAN_ATTRS? The XHTML spec has a similar list in an informative appendix. Only readonly
(missing in Genshi) and nohref
(missing in the appendix) are different.
comment:12 by , 11 years ago
Replying to rblank:
I wouldn't do that. In the case where a disabled
textarea.wikitext
is in the generated XHTML, and it is enabled dynamically through JavaScript (e.g. after setting a checkbox), the wiki toolbar would be missing (or would have to be added once explicitly in the checkbox handler). Always having the toolbar is also visually more consistent.
That makes sense, thanks for the feedback. I'll post my latest proposed changes tomorrow after giving them one more review.
(5) from comment:1 has been partially implemented now. Access to the Milestone administration page can be restricted using a fine-grained permissions policy. To view the milestone administration page, the following two permissions are required:
[admin:ticket/milestones] anonymous = MILESTONE_VIEW, TICKET_ADMIN
Additionally, MILESTONE_MODIFY
, MILESTONE_CREATE
and MILESTONE_DELETE
enable addition functionality (basically [8165] was extended to support Authz permissions policy).
However, fine-grained permissions checks are not yet done for each milestone that is displayed in the milestone listing. For example. if a permissions policy is in effect that allows a user to access milestone1
but not milestone2
,
[milestone:milestone1] anonymous = MILESTONE_VIEW [milestone:milestone2] anonymous =
users with MILESTONE_VIEW
and TICKET_ADMIN
for admin:ticket/milestones
will currently still see all milestones. I don't really want to try to fix this issue in this ticket, but thinking about it and experimenting has me questioning now whether my other changes are implemented correctly.
You can see from my previous changes, that for controlling access to /admin/ticket/milestone
I've treated admin
as the "realm" and ticket/milestone
as the "resource id" when doing permission checks with a PermissionCache
object: 'MILESTONE_VIEW' in perm('admin', 'ticket/milestones')
. Suppose though that we want to control the milestones listed on the /admin/ticket/milestones
page. Should the permission check be 'MILESTONE_VIEW' in perm(milestone.resource)
so that a policy like the one shown above would restrict access? Or should the permission check be 'MILESTONE_VIEW' in perm('admin', 'ticket/admin/' + milestone.name)
, so that a permission policy like the following would restrict access?
[admin:ticket/milestones/milestone1] anonymous = MILESTONE_VIEW
comment:13 by , 11 years ago
Replying to psuter:
Do you think
'readonly'
should be added to Genshi's list in XHTMLSerializer._BOOLEAN_ATTRS? The XHTML spec has a similar list in an informative appendix. Onlyreadonly
(missing in Genshi) andnohref
(missing in the appendix) are different.
It looks to me like it should be added, and there is a ticket in which it has been requested, genshi:#570. readonly
appears to be supported in HTML4 though, so I wonder if it was just an oversight.
comment:14 by , 11 years ago
Replying to rjollos:
I made some changes to disable the datepicker when the
readonly
attribute is present. However, if we change thereadonly
s todisabled
, then the datepicker seems to be implicitly disabled.
I guess though that it makes more sense to display the fields as read-only, so maybe I'll just conditionally add the JavaScript for the datepicker only when the user has MILESTONE_MODIFY
.
comment:15 by , 11 years ago
Description: | modified (diff) |
---|
Latest proposed changes can be found in rjollos.git:t11069.2.
comment:16 by , 11 years ago
Release Notes: | modified (diff) |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
Changes committed to 1.0-stable in [12026:12034], and merged to trunk in [12036].
I still have concerns about the questions I raised in comment:12, which are the complications that result from using a relative path for the "resource id" when setting up fine-grained access to the admin realm. I'll probably revisit this issue in the future if it becomes more clear.
Patch against r11682 of the trunk.