Edgewall Software
Modify

Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#10833 closed defect (fixed)

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

Reported by: Ethan Jucovy <ethan.jucovy@…> Owned by: Ethan Jucovy <ethan.jucovy@…>
Priority: normal Milestone: 1.1.3
Component: ticket system Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Fine-grained permission check is used to populate the list of owners when [ticket] restrict_owner = True.

API Changes:

EnvironmentStub has an insert_known_users method for populating the session table with known users.

Description

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.

Attachments (1)

restrict_owner_finegrained.diff (1.9 KB ) - added by Ethan Jucovy <ethan.jucovy@…> 7 years ago.
introduce a new config option to respect fine-grained permissions in configurable-ticket-workflow's restrict_owner behavior

Download all attachments as: .zip

Change History (10)

by Ethan Jucovy <ethan.jucovy@…>, 7 years ago

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

comment:1 by Ethan Jucovy <ethan.jucovy@…>, 7 years ago

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.

comment:2 by Ethan Jucovy <ethan.jucovy@…>, 7 years ago

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)

comment:3 by Christian Boos, 7 years ago

Milestone: undecided

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

comment:4 by Ryan J Ollos, 6 years ago

Milestone: undecided1.1.3
Owner: set to Ryan J Ollos
Status: newassigned

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

in reply to:  description comment:5 by Ryan J Ollos, 5 years ago

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

comment:6 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [13515].

comment:7 by Ryan J Ollos, 5 years ago

Owner: changed from Ryan J Ollos to Ethan Jucovy <ethan.jucovy@…>

comment:8 by Jun Omae, 5 years ago

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

comment:9 by Ryan J Ollos, 5 years ago

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ethan Jucovy <ethan.jucovy@…>.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Ethan Jucovy <ethan.jucovy@…> to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.