| 2 | | * 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. |
| 3 | | * 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. |
| 4 | | * 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. |
| 5 | | * 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` ([/browser/trunk/trac/admin/web_ui.py?rev=11054&marks=169#L163 get panels] -> [/browser/trunk/trac/admin/web_ui.py?rev=11054&marks=209-210#L201 check perm] -> [/browser/trunk/trac/admin/web_ui.py?rev=11054&marks=89,123-124#L88 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. |
| 6 | | * The milestone 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). |
| | 2 | 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. |
| | 3 | 1. 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. |
| | 4 | 1. 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. |
| | 5 | 1. 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` ([/browser/trunk/trac/admin/web_ui.py?rev=11054&marks=169#L163 get panels] -> [/browser/trunk/trac/admin/web_ui.py?rev=11054&marks=209-210#L201 check perm] -> [/browser/trunk/trac/admin/web_ui.py?rev=11054&marks=89,123-124#L88 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. |
| | 6 | 1. The milestone 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). |
