Fine-grained access to the admin panels is not possible

[Ryan J Ollos <ryan.j.ollos@…>]

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.

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11682 of the trunk.

[Ryan J Ollos <ryan.j.ollos@…>]

Authz file for manual functional testing.

[Ryan J Ollos <ryan.j.ollos@…>]

Here are some of my findings and a description of the changes in t11069-r11682-1.patch​:

1. I found that users with PERMISSION_GRANT or PERMISSION_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.
2. Users with PERMISSION_REVOKE can revoke any permission, so we don't have symmetry with PERMISSION_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 with PERMISSION_REVOKE could remove TRAC_ADMIN permission from an administrator, even if they themselves are not an administrator.
3. 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.
4. The logging panel was missing a permission check for render_admin_panel when compared to the render_admin_panel method for most of the other panels. However, it appears this permission check is not necessary because render_admin_panel won't be called unless get_admin_panels yields a tuple, and a permissions check is always done before yielding in get_admin_panels (get panels → check perm → render panels). Therefore I removed the permission checks for all render_admin_panels methods rather than modifying the permissions checks for fine-grained checking. The ticket admin panels don't perform permission checks in render_admin_panel or _render_admin_panel either, presumably for the reason I've described here.
5. 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.

[Steffen Hoffmann]

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.

[Jun Omae]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

In the forthcoming changes, I've modified the behavior of the table from my previous patch:

1. 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.
2. 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).

[Ryan J Ollos]

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.

[Ryan J Ollos]

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 readonlys 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

@@ -89 +89 @@
 // Add the toolbar to all <textarea> elements on the page with the class
 // 'wikitext'.
 jQuery(document).ready(function($) {
-  $("textarea.wikitext").each(function() { addWikiFormattingToolbar(this) });
+  if ($("textarea.wikitext").is(":enabled"))
+    $("textarea.wikitext").each(function() { addWikiFormattingToolbar(this) });
 });

[Remy Blank]

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.

[Peter Suter]

Replying to rjollos:

I see readonly = "True" and disabled = "disabled". These have been changed in the template to readonly = 'readonly' and disabled = '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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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. Only readonly (missing in Genshi) and nohref (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.

[Ryan J Ollos]

Replying to rjollos:

I made some changes to disable the datepicker when the readonly attribute is present. However, if we change the readonlys to disabled, 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.

[Ryan J Ollos]

Latest proposed changes can be found in rjollos.git:t11069.2.

[Ryan J Ollos]

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.

[Jun Omae]

Hmm, after the changes of [12029], randomly the failures in functional tests for authz_policy on ext3….

$ PYTHONPATH=. ~/venv/py25/bin/python trac/admin/tests/functional.py
......F.........
======================================================================
FAIL: Check permissions required to access Permissions panel.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/admin/tests/functional.py", line 130, in runTest
    "Manage Permissions and Groups")
  File "trac/admin/tests/functional.py", line 39, in test_authorization
    tc.find("No administration panels available")
  File "/home/jun66j5/src/trac/edgewall/git/trac/tests/functional/better_twill.py", line 216, in better_find
    raise twill.errors.TwillAssertionError(*args)
TwillAssertionError: ("no match to 'No administration panels available'", '/home/jun66j5/src/trac/edgewall/git/testenv/trac/log/TestPermissionsAuthorization.html')

The workaround is [9a7f61af/jomae.git]. The changes are waiting 1 second before env.config.save() if the filesystem may be ext3.

[Ryan J Ollos]

Replying to jomae:

The workaround is [9a7f61af/jomae.git]. The changes are waiting 1 second before env.config.save() if the filesystem may be ext3.

I guess my feeling is that it would be ideal if we could re-order or change the operations to avoid the failure rather than having an explicit pause, like we previously discussed in comment:14:ticket:11176 when a similar issue with EXT3 came up, but I can't say yet that it is possible. I can do some experimenting this weekend though. Good catch!

[Ryan J Ollos]

In [12031], I inadvertently removed the action from the title attribute, so it is no longer shown on hover-over. This had previously been added in #11095, since the action might be truncated if it has too many characters. The following patch should fix the issue:

trac/admin/templates/admin_perms.html

@@ -69 +69 @@
             <td>$subject</td>
             <td>
               <label py:for="subject, action in perm_group"
-                     py:with="is_valid = action in actions; has_perm = action in perm">
+                     py:with="invalid = action not in actions; has_perm = action in perm">
                 <!--! base64 makes it safe to use ':' as separator when passing
                       both subject and action as one query parameter -->
                 <input py:if="can_revoke" type="checkbox" name="sel"
@@ -78 +78 @@
                        value="${'%s:%s' % (unicode_to_base64(subject),
                                            unicode_to_base64(action))}"
                        disabled="${'disabled' if not has_perm else None}" />
-                <span py:strip="is_valid" class="missing"
-                      title="Action is no longer defined">${action}</span>
+                <span class="${classes(missing=invalid)}"
+                      title="${_('%(action)s is no longer defined', action=action) if invalid else action}">${action}</span>
               </label>
             </td>
           </tr>

[Ryan J Ollos]

I'll add a functional test case for the patch in comment:19 before committing.

[Ryan J Ollos]

Functional test should be added for the work in #10752 as well.

[Ryan J Ollos]

I imagine that we may get more test failures on EXT3 filesystem after #8976 since the test cases uses enable_authz_permpolicy / disable_authz_permpolicy from the FunctionalTestEnvironment class. I'll be sure to take a closer look at comment:17 today.

[Ryan J Ollos]

Fix and regression test for regression of #11095 and regression test for #10752 committed to 1.0-stable in [12159:12160]. Merged to trunk in [12161].

[Ryan J Ollos]

Replying to jomae:

Hmm, after the changes of [12029], randomly the failures in functional tests for authz_policy on ext3….

[…]

The workaround is [9a7f61af/jomae.git]. The changes are waiting 1 second before env.config.save() if the filesystem may be ext3.

I tried out your fix, and I think it's a good solution since it only results in a sleep on low resolution timestamp platforms. I found that it was also necessary to call wait_for_low_fs_time_resolution in _tracadmin. This seemed to be due to the grant_perm / revoke_perm​ calls which lead to a touch command.

I also added a minor optimization, so that we only sleep when necessary. This reduced the execution time of trac.admin.tests.functional from 45 seconds to 30 seconds on my system. Changes can be found in log:rjollos.git:t11069.3. Let me know what you think.

[Ryan J Ollos]

I must not understand how Configuration.parse_if_needed or the environment caching works, because I've been unable to make a call to parse_if_needed with the force parameter behave in the way suggested in comment:2:ticket:11320.

However, I did realize one other thing - I think it is probably better to call wait_on_low_res_fs for only the cases that we need to ensure that the file modification time has changed, rather than for every call to _tracadmin: [59a9bf48/rjollos.git].

[Jun Omae]

Replying to rjollos:

I also added a minor optimization, so that we only sleep when necessary. This reduced the execution time of trac.admin.tests.functional from 45 seconds to 30 seconds on my system. Changes can be found in log:rjollos.git:t11069.3. Let me know what you think.

Looks good to me! But we can simplify wait_on_low_res_fs. The proposed changed can be found in [3cc1ca023/jomae.git].

[Ryan J Ollos]

Replying to rjollos:

However, I did realize one other thing - I think it is probably better to call wait_on_low_res_fs for only the cases that we need to ensure that the file modification time has changed, rather than for every call to _tracadmin: [59a9bf48/rjollos.git].

[59a9bf48/rjollos.git] has a few errors in how wait_on_low_res_fs should be called. The function should be called before the save method, so a sleep operation can be invoked in the case that the save operation won't result in a change to the file modification time.

As I looked at this more, I wondered if it would be simpler and more useful to just fix the underlying issue in the save method. I've created #11329 in order to discuss this further.

[Ryan J Ollos]

#9445 closed as a duplicate.

[Ryan J Ollos]

#7015 closed as a duplicate.