ConfigurableTicketWorkflow's "Reassign To" ignores fine-grained permissions with restrict_owner=True

[Ethan Jucovy <ethan.jucovy@…>]

When [ticket] restrict_owner = True, the default ticket workflow implementation renders a dropdown box for actions that perform a set_owner operation.

If the set of possible owners is not specified in the workflow configuration, it is populated with a call to PermissionSystem(env).get_users_with_permission('TICKET_MODIFY'), just like the similar dropdowns that are rendered by TicketSystem.eventually_restrict_owner on the new ticket form and query builder.

However, after fetching the list of all known users with TICKET_MODIFY, the workflow does not check whether those users have the TICKET_MODIFY permission for the current ticket. If the system is configured to use a fine-grained permission policy like browser:trunk/sample-plugins/permissions/vulnerability_tickets.py​, where the global TICKET_MODIFY permission does not guarantee TICKET_MODIFY for any given ticket, this could result in strange states where the ticket's owner does not have permission to close, modify or reassign the ticket.

I'm not sure whether this actually is a bug that should be fixed. For one thing, as long as the users who have permission to reassign a ticket are trusted to know what they're doing, it's not really a problem. It's also not necessarily Trac's job to ensure that the owner field is restricted to "sensible" choices, and anyway there are plenty of other ways that the system could end up in a similar state with certain combinations of configuration.

[Ethan Jucovy <ethan.jucovy@…>]

introduce a new config option to respect fine-grained permissions in configurable-ticket-workflow's restrict_owner behavior

[Ethan Jucovy <ethan.jucovy@…>]

For the sake of argument I've attached a patch which "fixes" this; it introduces a new boolean option restrict_owner_respecting_fine_grained_permissions to maintain the current behavior by default, though that's probably excessive.

[Ethan Jucovy <ethan.jucovy@…>]

At the risk of meandering wildly off-topic, there are also some related issues:

1. The idea of using a permission other than TICKET_MODIFY for "who can be an owner" has been discussed a few times (#3580, #4266). If the behavior is changed here, the patch at attachment:ticket:3580:ticket-3580-v1.patch​ should also be updated, so that fine-grained permissions are respected no matter what permission is involved.
2. However, if Trac starts to make promises that the "set_owner" options are all users who have TICKET_MODIFY on the current ticket, then this creates an inverse issue: the system cannot guarantee that the list is complete. That's because the initial set of users retrieved from PermissionSystem.get_users_with_permission('TICKET_MODIFY') is a coarse-grained permission check, and depending on the active permission components, users may have TICKET_MODIFY for a certain ticket without having it globally. This is related to #5648 (particularly comment:8:ticket:5648) and (inevitably…) #2456 — there is no API to fetch all users with a certain permission with respect to a fine-grained resource.
3. There also might be use cases for using a new API/store to determine who can be an owner (both in default_workflow.py and in TicketSystem.eventually_restrict_owner) instead of always relying on a permission check at all — either because some store other than the permission system is more appropriate (#5858, ​th:browser:flexibleassigntoplugin/0.13/trunk/SampleValidOwnerProvider.py) or because the default IPermissionStore implements the permission check using env.get_known_users() which is inefficient (#4245) and requires that users have logged in at least once which is not always desired (#4289, #7290). A new API might be a way to give users more flexibility to specify this behavior without needing to get all of #4245 and #2456 right first, and might also be reusable by plugins to implement "user-ish" custom field types (#8069, #2662, ​th:ticket:8477)

[Christian Boos]

All the tickets for {20} from last year have probably been seen multiple times by now, yet are still to be triaged…

[Ryan J Ollos]

This seems like a good follow-on to #10018, so lets attempt to work through some of the questions you've raised.

[Ryan J Ollos]

Replying to Ethan Jucovy <ethan.jucovy@…>:

I'm not sure whether this actually is a bug that should be fixed. For one thing, as long as the users who have permission to reassign a ticket are trusted to know what they're doing, it's not really a problem. It's also not necessarily Trac's job to ensure that the owner field is restricted to "sensible" choices, and anyway there are plenty of other ways that the system could end up in a similar state with certain combinations of configuration.

It would be more consistent to check for TICKET_MODIFY permissions on the resource in the same way as TicketSystem.eventually_restrict_owner (tags/trac-1.0.2/trac/ticket/api.py@:419-420#L409​), even though TicketSystem.eventually_restrict_owner isn't used in the ticket interface after #2045 (see [13452#file10]).

I'll wait for any feedback and then add tests to the patch before committing: log:rjollos.git:t10833

[Ryan J Ollos]

Committed to trunk in [13515].

[Jun Omae]

After [13515], unit tests fail with no configobj.

...

======================================================================
ERROR: Fine-grained permission checks when populating the restricted
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/6261677daeaab253028fdd133516690ec1052268/py26-sqlite/trac/ticket/tests/default_workflow.py", line 140, in test_set_owner_fine_grained_permissions
    'reassign')
  File "/run/shm/6261677daeaab253028fdd133516690ec1052268/py26-sqlite/trac/ticket/default_workflow.py", line 255, in render_ticket_action_control
    ticket.resource)]
  File "/run/shm/6261677daeaab253028fdd133516690ec1052268/py26-sqlite/trac/perm.py", line 544, in has_permission
    return self._has_permission(action, resource)
  File "/run/shm/6261677daeaab253028fdd133516690ec1052268/py26-sqlite/trac/perm.py", line 558, in _has_permission
    check_permission(action, perm.username, resource, perm)
  File "/run/shm/6261677daeaab253028fdd133516690ec1052268/py26-sqlite/trac/perm.py", line 455, in check_permission
    perm)
  File "/run/shm/6261677daeaab253028fdd133516690ec1052268/py26-sqlite/tracopt/perm/authz_policy.py", line 145, in check_permission
    self.parse_authz()
  File "/run/shm/6261677daeaab253028fdd133516690ec1052268/py26-sqlite/tracopt/perm/authz_policy.py", line 188, in parse_authz
    raise ConfigurationError()
ConfigurationError: Look in the Trac log for more information.

----------------------------------------------------------------------
Ran 1641 tests in 76.567s

FAILED (errors=2)
SKIP: fine-grained permission tests (ConfigObj not installed)
SKIP: utils/tests/datefmt.py (no pytz installed)
SKIP: utils/tests/datefmt.py (no babel installed)
SKIP: trac.versioncontrol.web_ui.tests.browser.BrowserModulePermissionsTestCase (no configobj installed)
SKIP: tracopt/perm/tests/authz_policy.py (no configobj installed)
make: *** [unit-test] Error 1

[Ryan J Ollos]

Thanks, test will be skipped when ConfigObj is not installed after [13525].