Workflow: Disable modifying ticket description only

[Florian Schricker]

Hi,

I am uncertain whether this is possible at all so I am asking here: I would like to disable modifying the ticket description when the ticket is in a given state while still allowing commenting. Can anything like this be configured in a workflow?

Any help or answers are greatly appreciated! A simple "function not installed" (if correct) would also prevent me from digging any deeper... :-)

regards,

Florian

[RjOllos]

1. BlackMagicTicketsPlugin (1) allows a permission to be specified to edit a ticket field.

2. ExtraPermissionsProvider (2) allow you to define new permissions.

3. A permissions policy could be implemented, similar to ReadonlySignedTickets (3), to deny a permission when the ticket is in a particular state.

Putting those three things together, I think it might work to define a new permission for editing the ticket description, and deny the permission when the ticket is in a particular state.

(1) https://trac-hacks.org/wiki/BlackMagicTicketTweaksPlugin 

(2) https://trac.edgewall.org/wiki/TracPermissions#CreatingNewPrivileges

(3) https://trac.edgewall.org/wiki/CookBook/Configuration/SignedTickets#Readonlylogic

- Ryan

[Florian Schricker]

Hi Ryan,

thanks for picking up the question!

Am Dienstag, 19. Juli 2016 16:53:43 UTC+2 schrieb RjOllos:

On Tuesday, July 19, 2016 at 6:43:54 AM UTC-7, Florian Schricker wrote:

I am uncertain whether this is possible at all so I am asking here: I would like to disable modifying the ticket description when the ticket is in a given state while still allowing commenting. Can anything like this be configured in a workflow?

1. BlackMagicTicketsPlugin (1) allows a permission to be specified to edit a ticket field. 

2. ExtraPermissionsProvider (2) allow you to define new permissions.

3. A permissions policy could be implemented, similar to ReadonlySignedTickets (3), to deny a permission when the ticket is in a particular state.

Putting those three things together, I think it might work to define a new permission for editing the ticket description, and deny the permission when the ticket is in a particular state.

(1) https://trac-hacks.org/wiki/BlackMagicTicketTweaksPlugin 

(2) https://trac.edgewall.org/wiki/TracPermissions#CreatingNewPrivileges

(3) https://trac.edgewall.org/wiki/CookBook/Configuration/SignedTickets#Readonlylogic

 

From looking at ReadonlySignedTickets I am wondering if this plugin would not suffice already:

• Using the "Allow ticket comments" variation (basically enabling permission TICKET_APPEND again commenting would be enabled, but not modifying the description (TICKET_MODIFY required, no?).
• Enhancing the lines "if t['status'] == 'closed' and t['resolution'] == 'signed': return False" with the actual ticket states I am having here in my setup would do the rest. There would be no configuration via trac.ini but I can do without.

I think I'll give it a go, thanks again for these good pointers to actual solutions!!

Florian

[RjOllos]

It sounds like that would work. You could even grant TICKET_CHGPROP, allowing ticket properties to be edited but not the ticket description.

- Ryan

[Florian Schricker]

Hi,

I setup a variant of ReadonlySignedTickets on our test system and worked.

On our production system as soon as I add the plugin as permission policy I receive a runtime error when using the timeline function: RuntimeError: maximum recursion depth exceeded while calling a Python object

Single-file plugin code:

author = "xxxx"

from trac.core import *

from trac.perm import IPermissionPolicy

from trac.ticket.model import Ticket

class XxxxxxReadonlyTicketsPlugin(Component):

        implements(IPermissionPolicy)

        def check_permission(self, action, username, resource, perm):

                if resource is None or resource.realm != 'ticket' or \

                   resource.id is None or \

                   action in ['TICKET_VIEW', 'TICKET_APPEND'] or \

                   action == 'TICKET_ADMIN' or 'TICKET_ADMIN' in perm:

                        return None

                t = Ticket(self.env, resource.id)

                if t['status'] == 'closed' and t['type'] == 'review':

                        return False

Current permession policy setup:

permission_policies = PrivateWikiSystem, PrivateTicketsPolicy, AttachmentDeletePolicy, DefaultPermissionPolicy, LegacyAttachmentPolicy

I tested adding the plugin as first policy entry and right before "DefaultPermissionPolicy" with the same runtime error.

Does anybody have an idea?

[Florian Schricker]

Update: I check the log and found the following "checking permission to death" most probably causing the issue.

2016-08-29 09:40:26,888 Trac[web_ui] ERROR: Timeline event provider failed:

Traceback (most recent call last):

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/timeline/web_ui.py", line 190, in process_request

    filters) or []:

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/ticket/web_ui.py", line 335, in get_timeline_events

    for (ev, t) in produce_ticket_change_events(db):

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/ticket/web_ui.py", line 308, in produce_ticket_change_events

    cid)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/ticket/web_ui.py", line 262, in produce_event

    if 'TICKET_VIEW' not in req.perm(ticket):

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 556, in has_permission

    return self._has_permission(action, resource)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 570, in _has_permission

    check_permission(action, perm.username, resource, perm)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 462, in check_permission

    perm)

  File "/usr/lib/python2.6/site-packages/TracPrivateTickets-2.0.2dev_r11777-py2.6.egg/privatetickets/policy.py", line 34, in check_permission

    'TRAC_ADMIN' in perm:

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 556, in has_permission

    return self._has_permission(action, resource)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 570, in _has_permission

    check_permission(action, perm.username, resource, perm)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 462, in check_permission

    perm)

  File "/var/trac/imstec/plugins/XxxxxReadonlyTicketsPlugin.py", line 14, in check_permission

    action == 'TICKET_ADMIN' or 'TICKET_ADMIN' in perm:

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 556, in has_permission

    return self._has_permission(action, resource)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 570, in _has_permission

    check_permission(action, perm.username, resource, perm)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 462, in check_permission

    perm)

  File "/usr/lib/python2.6/site-packages/TracPrivateTickets-2.0.2dev_r11777-py2.6.egg/privatetickets/policy.py", line 34, in check_permission

    'TRAC_ADMIN' in perm:

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 556, in has_permission

    return self._has_permission(action, resource)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 570, in _has_permission

    check_permission(action, perm.username, resource, perm)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 462, in check_permission

    perm)

  File "/var/trac/imstec/plugins/XxxxxReadonlyTicketsPlugin.py", line 14, in check_permission

    action == 'TICKET_ADMIN' or 'TICKET_ADMIN' in perm:

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 556, in has_permission

    return self._has_permission(action, resource)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 570, in _has_permission

    check_permission(action, perm.username, resource, perm)

  File "/usr/lib/python2.6/site-packages/Trac-1.0-py2.6.egg/trac/perm.py", line 462, in check_permission

    perm)

[... and so on ...]

As can be seen "TracPrivateTickets" might be the culprit?

Any idea? Or a workaround in my XxxxxReadonlyTicketsPlugin?

regards

[Peter Suter]

On 29.08.2016 11:26, Florian Schricker wrote:

  File "/usr/lib/python2.6/site-packages/TracPrivateTickets-2.0.2dev_r11777-py2.6.egg/privatetickets/policy.py", line 34, in check_permission

    'TRAC_ADMIN' in perm:

...

  File "/var/trac/imstec/plugins/XxxxxReadonlyTicketsPlugin.py", line 14, in check_permission

    action == 'TICKET_ADMIN' or 'TICKET_ADMIN' in perm:

These two policies go back and forth, one asking "is TICKET_ADMIN allowed?" and the other replying "is TRAC_ADMIN" allowed?" and so on.

As can be seen "TracPrivateTickets" might be the culprit?

Any idea? Or a workaround in my XxxxxReadonlyTicketsPlugin?

I think you need to break the cycle and when asked "is TRAC_ADMIN allowed?" drop out before asking back about TICKET_ADMIN. Try adding `action == 'TRAC_ADMIN'` to the check as follows:

        def check_permission(self, action, username, resource, perm):

                if resource is None or resource.realm != 'ticket' or \

                   resource.id is None or \

                   action in ['TICKET_VIEW', 'TICKET_APPEND'] or \

                   action == 'TRAC_ADMIN' or \

                   action == 'TICKET_ADMIN' or 'TICKET_ADMIN' in perm:

                        return None

Hope this helps.

Peter

[Florian Schricker]

While this now fixes the Timeline function, modifying tickets now fails with the same recursion error. I have not yet checked the logs.

For a better understanding let me ask the following question: 

From my research I was not expecting the policies to "interact" with each other recursively at all. I was thinking that all configured policies are asked in sequence until some policy returns True or False, with returning None just meaning "ask the next policy". Where is the recursion? Is it a bug in TracPrivateTickets? 

(I checked the TracPrivateTickets defects, newer revisions do not fix anything wrt to permissions.)

[RjOllos]

On Tuesday, August 30, 2016 at 12:25:26 AM UTC-7, Florian Schricker wrote:

While this now fixes the Timeline function, modifying tickets now fails with the same recursion error. I have not yet checked the logs.

For a better understanding let me ask the following question: 

From my research I was not expecting the policies to "interact" with each other recursively at all. I was thinking that all configured policies are asked in sequence until some policy returns True or False, with returning None just meaning "ask the next policy". Where is the recursion? Is it a bug in TracPrivateTickets?

There shouldn't be recursion in ReadonlySignedTickets because of the check "action == 'TICKET_ADMIN' or 'TICKET_ADMIN' in perm". "'TICKET_ADMIN' in perm" will cause a recursive permission check for the action TICKET_ADMIN, but when ReadonlySignedTickets is called again it will return None due to the "action == 'TICKET_ADMIN'" conditional.

There were some indentation problems in the example on the Cookbook page, as well as some omissions, which I hope are fixed now:
https://trac.edgewall.org/wiki/CookBook/Configuration/SignedTickets?action=diff&version=6&old_version=4

Could you please try again with the example on that page? The recipe describes how to include 'TICKET_APPEND' as an allowed action.
 
If it still does not work for you, please try with just ReadonlySignedTickets and the default permission policies, and work backward by adding your additional policies until you reproduce the issue. Please include the logs in your follow-up.

Finally, an idea on how to fix the recursion with PrivateTicketsPlugin, based on your traceback is:

# -*- coding: utf-8 -*-

from trac.core import *
from trac.perm import IPermissionPolicy
from trac.ticket.model import Ticket

class ReadonlySignedTickets(Component):

    implements(IPermissionPolicy)

    allowed_actions = ('TICKET_VIEW', 'TICKET_APPEND')
    admin_actions = ('TICKET_ADMIN', 'TRAC_ADMIN')

    def check_permission(self, action, username, resource, perm):
        if resource is None or resource.realm != 'ticket' or \
                resource.id is None or \

                action in self.allowed_actions or \
                action in self.admin_actions or \
                any(a in perm for a in self.admin_actions):

            return None

        t = Ticket(self.env, resource.id)

        if t['status'] == 'closed' and t['resolution'] == 'signed':
            return False

If that change works for you, I'll update the cookbook page.

(I checked the TracPrivateTickets defects, newer revisions do not fix anything wrt to permissions.)

- Ryan
 

[Florian Schricker]

Unfortunately it still fails.

This time I am attaching a more or less complete log from tracd-start until "recursion error" and the source file of the plugin I am using.

I hope that helps - I am more than willing in testing anything while providing any information I can collect. Any config files or other files of interest? Just let me know.

regards,

Florian

[Peter Suter]

On 21.09.2016 09:06, Florian Schricker wrote:

Unfortunately it still fails.

This time I am attaching a more or less complete log from tracd-start until "recursion error" and the source file of the plugin I am using.

I hope that helps - I am more than willing in testing anything while providing any information I can collect. Any config files or other files of interest? Just let me know.

regards,

Florian

Am Freitag, 16. September 2016 03:16:17 UTC+2 schrieb RjOllos:

On Tuesday, August 30, 2016 at 12:25:26 AM UTC-7, Florian Schricker wrote:

While this now fixes the Timeline function, modifying tickets now fails with the same recursion error. I have not yet checked the logs.

For a better understanding let me ask the following question: 

From my research I was not expecting the policies to "interact" with each other recursively at all. I was thinking that all configured policies are asked in sequence until some policy returns True or False, with returning None just meaning "ask the next policy". Where is the recursion? Is it a bug in TracPrivateTickets?

It's "mutual recursion". Unfortunately that means it's not a simple bug you can necessarily fix in one plugin alone. It's an unfortunate interaction between the two plugins.
The idea was, exactly as you say, to ask each policy in turn. But each plugin basically says: "Hang on, before I answer, let me ask you this first: Is this other thing allowed?"
And again each policy is asked in turn about this new question, and so on forever...

Each policy is protected against its own such recursive questions:

Am Freitag, 16. September 2016 03:16:17 UTC+2 schrieb RjOllos:

There shouldn't be recursion in ReadonlySignedTickets because of the check "action == 'TICKET_ADMIN' or 'TICKET_ADMIN' in perm". "'TICKET_ADMIN' in perm" will cause a recursive permission check for the action TICKET_ADMIN, but when ReadonlySignedTickets is called again it will return None due to the "action == 'TICKET_ADMIN'" conditional.

But neither policy can detect re-entrancy in general. It only shortcuts its own kind of followup question.
Since the other policy asks about a different kind of permission, this is not detected.

It's basically again the same problem as before.

Detailed step-by-step description of events:
1. The initial question is from web_ui.py about TICKET_VIEW.
2. The Permission System asks each policy about this in turn.
3. First it asks the privatetickets policy, which interjects (recursively calls):
   "To answer that, I first need to know if TICKET_VIEW_REPORTER is available."
4. The Permission System again poses this new question to each policy in turn.
5. First it asks the privatetickets policy, which expected this and replies "None".
    (This is not visible in the stack trace.)
6. Then it asks ReadonlyTicketsPlugin, which interjects:
   "To answer that, I first need to know if TICKET_ADMIN is available."
7. The Permission System again poses this new question to each policy in turn.
8. First it asks the privatetickets policy, which interjects (recursively calls):
   "To answer that I first need to know if TICKET_VIEW_REPORTER is available".

And since 8. is exactly the same situation as 3. it continues in the same way forever.



The dangerous thing is calling perm.has_permission(action2) inside check_permission(...action1...).
But it's only a problem if a second policy calls perm.has_permission(action1) inside check_permission(...action2...).
(Or some similar more complex variations.)

How can this be avoided?
One way is that by convention all policies must make sure not to "go back to an earlier question".
For example if all permission actions are ordered somehow (for example TRAC_ADMIN>TICKET_ADMIN>TICKET_VIEW>...) ,
then inside check_permission(...action1...) only use perm.has_permission(action2) if action2>action1.
But the PrivateTickets policy does not follow such a convention.

I think it's an inherent problem with the design that was not foreseen.
You can sometimes add workarounds for specific situations, as in the first case.
In this case it might be easier to add the workaround to the PrivateTickets policy.


Peter

[RjOllos]

On Wednesday, September 21, 2016 at 11:12:43 AM UTC-7, Peter Suter wrote:

On 21.09.2016 09:06, Florian Schricker wrote:

Unfortunately it still fails.

This time I am attaching a more or less complete log from tracd-start until "recursion error" and the source file of the plugin I am using.

I hope that helps - I am more than willing in testing anything while providing any information I can collect. Any config files or other files of interest? Just let me know.

regards,

Florian

Am Freitag, 16. September 2016 03:16:17 UTC+2 schrieb RjOllos:

On Tuesday, August 30, 2016 at 12:25:26 AM UTC-7, Florian Schricker wrote:

While this now fixes the Timeline function, modifying tickets now fails with the same recursion error. I have not yet checked the logs.

For a better understanding let me ask the following question: 

From my research I was not expecting the policies to "interact" with each other recursively at all. I was thinking that all configured policies are asked in sequence until some policy returns True or False, with returning None just meaning "ask the next policy". Where is the recursion? Is it a bug in TracPrivateTickets?

It's "mutual recursion". Unfortunately that means it's not a simple bug you can necessarily fix in one plugin alone. It's an unfortunate interaction between the two plugins.
The idea was, exactly as you say, to ask each policy in turn. But each plugin basically says: "Hang on, before I answer, let me ask you this first: Is this other thing allowed?"
And again each policy is asked in turn about this new question, and so on forever...

Each policy is protected against its own such recursive questions:

Am Freitag, 16. September 2016 03:16:17 UTC+2 schrieb RjOllos:

There shouldn't be recursion in ReadonlySignedTickets because of the check "action == 'TICKET_ADMIN' or 'TICKET_ADMIN' in perm". "'TICKET_ADMIN' in perm" will cause a recursive permission check for the action TICKET_ADMIN, but when ReadonlySignedTickets is called again it will return None due to the "action == 'TICKET_ADMIN'" conditional.

But neither policy can detect re-entrancy in general. It only shortcuts its own kind of followup question.
Since the other policy asks about a different kind of permission, this is not detected.

Based on what you said, I considered if we could detect re-entrancy by passing the policy to the permission cache when doing a PermissionCache.has_permission check inside of PermissionCache.check_permission:
https://trac.edgewall.org/ticket/12597

The changes might need some additional work, but it seems to work with:

permission_policies = ReadonlySignedTickets, PrivateTicketsPolicy, DefaultPermissionPolicy, LegacyAttachmentPolicy

I expect other permission policies such as PrivateTicketsPlugin would need to be modified in the way proposed for SignedTicketsPolicy.

I also posted modifications to SignedTickets, but I don't expect they will fix this "interaction" issue:
https://trac.edgewall.org/wiki/CookBook/Configuration/SignedTickets?version=9

- Ryan

[Peter Suter]

On 22.09.2016 07:17, RjOllos wrote:
> Based on what you said, I considered if we could detect re-entrancy by
> passing the policy to the permission cache when doing a
> PermissionCache.has_permission check inside of
> PermissionCache.check_permission:
> https://trac.edgewall.org/ticket/12597
>

Replied in ticket.

> I also posted modifications to SignedTickets, but I don't expect they
> will fix this "interaction" issue:
> https://trac.edgewall.org/wiki/CookBook/Configuration/SignedTickets?version=9

>>> - 'TICKET_ADMIN' in perm:
>>> + any(a in perm for a in self.admin_actions):

This change seems unnecessary and maybe even more problematic than before.
Unnecessary because TRAC_ADMIN implies TICKET_ADMIN anyway, so there's
no need to check for TRAC_ADMIN explicitly. (But there may be more
subtle details I'm missing.)
Problematic because checking more permissions leads to more potentially
problematic interactions (and possible mutual recursion) with other
policies.



Peter

[Florian Schricker]

Am Mittwoch, 21. September 2016 20:12:43 UTC+2 schrieb Peter Suter:

Detailed step-by-step description of events:

1. The initial question is from web_ui.py about TICKET_VIEW.
2. The Permission System asks each policy about this in turn.
3. First it asks the privatetickets policy, which interjects (recursively calls):
   "To answer that, I first need to know if TICKET_VIEW_REPORTER is available."
4. The Permission System again poses this new question to each policy in turn.
5. First it asks the privatetickets policy, which expected this and replies "None".
    (This is not visible in the stack trace.)
6. Then it asks ReadonlyTicketsPlugin, which interjects:
   "To answer that, I first need to know if TICKET_ADMIN is available."
7. The Permission System again poses this new question to each policy in turn.
8. First it asks the privatetickets policy, which interjects (recursively calls):
   "To answer that I first need to know if TICKET_VIEW_REPORTER is available".

And since 8. is exactly the same situation as 3. it continues in the same way forever.

So I am wondering if "patching" PrivateTicketsPolicy/check_permission would do the trick: By adding "TICKET_ADMIN" to the list of "ignore permissions / actions" the "mutual recursion" would stop, wouldn't it?

For me such a "patch fix" is absolutely ok; dropping the PrivateTicketsPlugin is not. And I fear I am not of any help in tackling "inherent problem with the design that was not foreseen."

(But I have the test system around for any tests you might want me to try out, though. We are using Trac extensively here, so there is a strong will to give back!)

regards,

Florian

[Peter Suter]

On 22.09.2016 09:27, Florian Schricker wrote:
>
> So I am wondering if "patching" PrivateTicketsPolicy/check_permission
> would do the trick: By adding "TICKET_ADMIN" to the list of "ignore
> permissions / actions" the "mutual recursion" would stop, wouldn't it?
>

Yes, or at least it sounds plausible to me that this might work.
Is it acceptable to you that anyone with TICKET_ADMIN permission will
not be restricted by the private tickets policy?
Normally the PrivateTicketsPlugin requires TRAC_ADMIN permission to see
all private tickets.

Please let us know if you test this.

Peter

[RjOllos]

On Wednesday, September 21, 2016 at 11:27:34 PM UTC-7, Peter Suter wrote:

> I also posted modifications to SignedTickets, but I don't expect they
> will fix this "interaction" issue:
> https://trac.edgewall.org/wiki/CookBook/Configuration/SignedTickets?version=9

 >>> -         'TICKET_ADMIN' in perm:
 >>> +         any(a in perm for a in self.admin_actions):

This change seems unnecessary and maybe even more problematic than before.
Unnecessary because TRAC_ADMIN implies TICKET_ADMIN anyway, so there's
no need to check for TRAC_ADMIN explicitly. (But there may be more
subtle details I'm missing.)
Problematic because checking more permissions leads to more potentially
problematic interactions (and possible mutual recursion) with other
policies.

That sounds right, I reverted the change.

https://trac.edgewall.org/wiki/CookBook/Configuration/SignedTickets?version=10

- Ryan 

[Florian Schricker]

Sorry for the delay, here are the results: it's working very well now after adding TICKET_ADMIN to the private tickets permission ignore list.

I am still puzzled by the whole "mutual recursion thing" but that just shows my complete lack of knowledge of the inner workings of Trac.

So: Issue closed! If anybody is interested I can post the sources of both my plugin and my change to the private tickets plugin.

Best regards and a big thank you to all involved for sorting this out!!!

Florian

[Florian Schricker]

Hi,

I have a follow-up question on the "readonly tickets" plugin:

Trac now only allows commenting a ticket when the ticket type matches the list of tickets in the plugin. But the ticket workflow has an action "change" defined which would move the ticket to state "new". Is it possible to detect this with the plugin and allow such an action?

kind regards,

Florian

[Peter Suter]

Hi

Maybe you could define an extra permission, configure the workflow
action "change" to require (only) that permission, and add that
permission to the "allowed_actions" of the readonly plugin?
It seems this is similar to what's described here:
https://trac.edgewall.org/wiki/CookBook/Configuration/SignedTickets#TICKET_SIGNpermission
https://trac.edgewall.org/wiki/CookBook/Configuration/SignedTickets#Signworkflowstep

Best regards,
Peter

(By the way, I think the "mutual recursion" flaw has now been improved
in Trac: https://trac.edgewall.org/ticket/12597)