Ability to lock a task to prevent further edits
Closed, ResolvedPublic

Description

It would be useful for our FDA regulated workflow to be able to lock a task to prevent further changes. The tricky part is controlling who can lock/unlock edits and I'm open to any solution that fits with the rest of the auth model - for example, allowing the author or admin to lock and only the admin to unlock etc. Thanks.

cos created this task.Mar 1 2017, 5:32 AM
chad renamed this task from Ability to lock a task to prevent further edits. to Ability to lock a task to prevent further edits.Mar 1 2017, 5:40 AM
chad added a project: Maniphest.

I have a few questions about this, and some ideas for how we could solve it:


Questions:

Common or rare? Is this workflow common (you want to lock a lot of tasks) or rare (you only want to lock a few tasks)?

If this is rare, we can look at solving this with more complicated workflows (for example, requiring several steps to lock a task) since it won't be as burdensome. If you want to lock a lot of tasks, we should try to provide an easier way to lock tasks.

Lock comments? Do you need to lock comments too, or just other types of edits (like status, priority, title, description, etc)?

Request or requirement? Is it good enough if locking serves as a request to users ("Please don't comment on or edit this task any more."), or does it need to be a requirement (actually prevent them from commenting/editing)?

For example, GitHub has a way to lock tasks, but I think the primary use case there is to stop drama and arguments on open source projects. Hopefully, there isn't too much internal drama at private companies, so maybe an advisory/request lock is sufficient?

In particular, could a solution where anyone could unlock the task be acceptable, as long as they had to explicitly unlock it? Or is it a hard regulatory requirement that unlocking requires some sort of administrator/oversight step?

Lock other objects? Do you need to lock other types of objects, or think you might in the future?


How Things Work Today

The simplest approach today would be to use Edit TaskEditable By and change it to something like "Administrators". However, I assume some of these are problems with this approach:

  • You Can't Lock Yourself Out: A user (say, @alice) can't make an edit which prevents her from making more edits: you can't edit a task and remove your own edit permission. We do this to prevent users from locking themselves out of tasks by accident. But this means that @alice can't set "Editable By" to "Administrators" unless she's an administrator. (However, she could set the policy to "@alice and administrators"; or "only @alice".)
  • Assignee Has Automatic Edit Permission: The task assignee always automatically has permission to edit the task, even if the edit policy would otherwise prevent them from editing it. We do this to make permission management simpler. It normally doesn't make sense to assign a task to someone who can't edit it, so we assume that the intent is to let the assignee edit even if the edit policy technically excludes them.
  • No Way to Prevent Comments: Commenting does not require edit permission, so changing the edit policy doesn't prevent anyone from commenting.
  • Comment Actions Can Perform Some Edits: Like commenting, updates which can be made through the comment action dropdown (including "Change Status", "Change Priority", etc) also do not require edit permission.
  • Not a First-Class Workflow: This isn't as obvious as a "Lock Task" action would be, requires a couple of steps, and doesn't make it clear in the transaction log that the edit is a lock. You could make this a little more clear by adding a tag like #locked at the same time, or posting a copy/pasted comment explaining that the task has been locked.

Other Context and Discussion

Abuse: There is a class of issues in T10215 related to "abuse", which we define as users spamming, making messes, and otherwise causing trouble. This is only really a problem for public/open source installs, because employees on private installs have a lot to lose and nothing to gain by spamming or vandalizing tasks.

So far this is more of an annoyance than a real problem, but it's probably something we'll have to solve in a real way eventually. To solve this problem in a general way, we will probably need to build an "under review" state into every object and comment in every application so that content submitted by untrusted users is initially "under review" and becomes visible to other users only once it is approved (for example, manually by an administrator or automatically by a spam detection system).

This probably does not impact "lock" very much, but some of the logic handling "under review" and "locked" might eventually interact, so I think it's worth mentioning as something to keep in mind in thinking about design here.

Open Source and GitHub: GitHub has a "Lock Issue" feature, but I believe it is used almost entirely to stop drama rather than for workflow purposes. It's common for open source projects to occasionally have tasks that inherently have a lot of drama and which get linked from places like Hacker News and see many users pile on to make jokes or add their opinions or yell at each other.

Hopefully this doesn't happen too much at private companies, since commenters have a lot more to lose.

Phabricator also isn't centralized like GitHub, so it's also less susceptible to drama like this because users who just want to post OMGLUL or a funny image usually have to register rather than just mash Submit.

As far as I can recall, we haven't seen requests for this feature from other open source installs yet, and I haven't seen many issues while browsing downstream installs. We'll probably see some of this eventually, but my sense is that this will probably never be a major issue and that being decentralized makes the barrier to this kind of drive-by drama high enough to mostly protect installs from it.

Reviving Closed Tasks: With some frequency, users who want to report issues on this install (secure.phabricator.com) find a similar-sounding task and leave a comment there instead of filing a new task or request. Often, the task they find isn't really related to their issue, and over time this leads to some old tasks having a series of unrelated comments at the bottom. We'd strongly prefer they file a new request: it's much easier to merge tasks than separate them, and much easier to keep track of things if they're in separate tasks.

We have other, larger problems with this workflow (described most recently in T12134) and hope to solve them by moving the entire workflow to a new application ("Nuance") in the future. But if we had an option to prevent comments on closed tasks, we'd probably enable it for this install until then.

Maybe slightly better would be something like "lock closed tasks after 7 days", to give users a window for followups -- but I think we'd need to do T4434 first.

We haven't seen other requests for this yet, and it also isn't too important on this install. My sense is that this project generally has more structure around task organization than many other projects, so we might feel this issue more acutely (it might be further down the list of frustrations for other installs).

"Lock as Security Issue": We previously ran into a somewhat-similar use case in T8434 and related tasks, where an install wanted a workflow for reporting sensitive security problems. The two use cases are not entirely similar -- the "Security" use case was more about who could see a task, not who could edit it -- but it bears a mention.

Comment / Comment Form Policies: The somewhat-surprising interaction between policies and the comment form is most recently discussed in T11207. Some vaguely related issues, specific to dragging tasks on workboards, are in T10722 and T6502.


Approaches

To recap, here's the closest thing we have to a "lock" workflow today:

  • To lock a task, @alice edits the task to have "Editable By: alice".
  • Optionally, she tags the task with #locked or copy/pastes a form comment onto the task "This task is locked, please don't comment on it. For help understanding locked tasks, see ((locked))."
  • Optionally, you add a "Locked" custom task status to maniphest.statuses, and she changes the task status to "Locked". This wouldn't enforce any behavior, but might be a more clear way to label the task as locked. The disadvantage of this approach is that it would prevent the task from being in other statuses ("Resolved", "Invalid", etc), since the "Locked" status would replace whatever status the task previously had.

These are the limitations:

  • @alice can still edit the task.
  • Anyone who can see the task can still comment on it.
  • Anyone who can see the task can still perform a limited set of edits via the "Actions" dropdown on the comment form.
  • There's no top-level / first-class "locked" state, so it isn't obvious at a glance that the task is locked, unless you add a custom "Locked" status.
  • Cumbersome: @alice might forget to tag the task or copy/paste the comment, and even if she does remember it's labor-intensive and slow.

Maybe all these limitations are acceptable; if so, you should be able to build this workflow by convention today.

Assuming at least some of these limitations are problems, though, here are some specific approaches we could take to tackle them:

Apply Edit Policy to Most Comment Actions: I think being able to take most of the comment actions (like "Change Status" and "Change Priority") without edit permission is probably a bad behavior in the long run, and we should require edit permission for these. There are some historical and product arguments for this behavior, but I think they're pretty weak.

For consistency, it's probably even good to remove "Change Subscribers" and "Move on Workboard", even though users who can not edit a task may be able to take those actions in other ways (you can always remove yourself as a subscriber, add other subscribers by commenting with @mentions, and moving tasks on workboards is more complicated). However, I think it's correct to leave "Comment", and only require View permission to be able to comment.

This would remove the "anyone can still perform a limited set of edits" limitation above with no further changes. I think this change is desirable no matter what, and makes the product behave in a more expected way for all users.

Custom Actions for Self-Lockout: If it's important for users to be able to lock themselves out of tasks, I think this workflow doesn't fit very naturally into the upstream: it's easy for users to make mistakes if they can lock themselves out, and we'd like to avoid this.

Instead, we can write a custom extension which would appear in the task action menu (under "Edit Task", "Edit Related Tasks", etc) and provide a specific "Lock Task" workflow. This workflow could set the edit permission to "No One" and apply any other appropriate edits (adding a comment, adding tags, changing status, etc).

To unlock tasks, we could provide a corresponding "Unlock Task" workflow, or operations staff could use the existing bin/policy unlock Txxx command from the commandline to force the task open again if unlocks are rare and it's acceptable or desirable to require a high level of administrative oversight to perform an unlock (this command just sets the policies of an object to "All Users"; it isn't related to "Locking" in the sense that this task discusses).

This would remove the limitation where the locking user can still edit the task, and could also remove any manual work involved in secondary actions (tagging, status changes). The major downside is that you'd need to run custom code. But this is probably the best route if "self-lockout" is important.

Internal "Interact" Permission: I think it would be reasonable for us to introduce a new "Interact" permission internally, which would sort of be somewhere between "View" and "Edit". It would control commenting, subscribing, and probably a few other interactions. By default, it would work like this:

  • If you can edit an object, you can always interact with it.
  • If an object doesn't have any special interaction rules, everyone who can view it can interact with it.

So, by default, nothing would change. But some objects, like tasks, could then gain special behaviors for this permission.

On its own, this wouldn't provide any UI or workflow options, but it would give us some internal tools to tackle some of the other limitations.

Tasks with this "interact" permission somehow set to "no one" (see below for ways we could do this) would address the "anyone can still comment" and "no first-class state" issues. Users would need edit permission to comment, and the UI could formally react to the lack of interact permission by providing cues to users ("You can not comment because this task is locked.").

Some ways we could expose this interact permission in the UI:

  • Explicitly add a "Can Interact" policy control to "Edit Task", like "Editable By" and "Visible To". This is the simplest option, but I don't think it's a good solution. I think it adds a lot of complexity and isn't the best way to approach this problem. Although it theoretically allows for more sophisticated workflows (like letting members of a particular project continue to comment on a task, while other users may not) we don't actually know of any use cases for this yet. More importantly, it would make every normal edit/create form more complicated, and locking still wouldn't be that easy (you'd have to go fiddle with a complex policy control), just possible. At least for now, I think we're better off finding ways to automatically set this policy to reasonable values without requiring the user to adjust a policy control themselves.
  • Automatically via Task Status: We could add configuration to maniphest.statuses so that tasks in certain statuses (like "Resolved") automatically lock the task (that is, set its "interact" permission to "no one"). Alternately, this would let you create a "Locked" status that really locked the task (by preventing commenting) instead of (or in addition to) having normal statuses like "Resolved" lock the task. After T4434, we could let this operate as "Automatically lock after X days" instead. This might be pretty reasonable if it's very common to lock tasks, or you want the default state of resolved tasks to be locked. We would probably enable this option on this install. I think this is a pretty clean approach and I like that it doesn't add new actions or workflows, but I'm not sure if it's a good approach in your use case.
  • Explicit Lock Workflow: We could also add an explicit upstream "Lock" workflow which sets a flag that causes the "interact" permission to become "No One". I think this is better than having a full-blown "Can Interact" policy control, but I think very few installs would use this workflow. I'd much rather make locking an automatic side-effect of task status.

Note that in all cases, without other changes, users with edit permission could still unlock the task, by using Edit Task and changing the interact policy, or re-opening it, or using "unlock" (which could only reasonably be driven by the edit policy, I think). So, on its own, this wouldn't provide a high oversight barrier to unlocking things.

Herald Actions: Custom Herald actions could address some of these limitations. In general, Herald could trigger based on tasks going into certain statuses (like "Resolved", or a custom "Locked" status) or tags being added (like a #locked tag).

Custom Herald actions require an extension, so if Herald is doing the entire workflow it's better to just have the extension provide a workflow instead, since then you get a formal "Lock Task" link instead of a weird, hard-to-find workflow where you add a #locked tag.

However, Herald might be a better approach if another solution (like the "interact" permission) solves most of the problems, and you only need one or two supplementary things. For example, Herald could do this:

When:
[ Task status ][ is ][ Locked ]
Take actions:
[ Set edit policy to no one ]

The "Set edit policy to no one" action would be a custom extension, but it would be much less custom code than a full "Lock Task" workflow would require.


Tentative Recommendation

I'm not sure about all of the details of your workflow, but here's my tentative recommendation on how to move forward:

  • We change to require "Edit" permission to take edit-like actions via the comment form.
  • We introduce a new internal "Interact" permission.
  • We let you configure task statuses to be "Locked" statuses via the existing maniphest.statuses option.

Then, the workflow would look like this:

  • To lock a task, @alice sets the status to a custom "Locked" status you've configured in maniphest.statuses.
  • Optionally, you can configure other statuses, like "Resolved", to also lock tasks, if you would generally like completed tasks to become immutable.

That produces these desirable behaviors:

  • No one can comment on the task or subscribe to it.
  • No one can take other actions via the comment form.
  • The UI can clearly show users that the task is locked.

The major limitations this doesn't address is:

  • Users who can edit the task can still edit it via Edit Task (but we could put a dialog on this workflow like "This task is locked, edit it anyway?").
  • Users who can edit the task can unlock it by using Edit Task to change the status back to "Open" (or another "unlocked" status).

I imagine these workflows may be reasonable since they'd be very explicit and hard to make a mistake with (and, in all cases, there's always a complete audit log of all actions, which users took them, and when), but I'm not sure what regulatory barriers you may face or how determined your users are to break rules.

If you need stricter policy locks, the Herald or custom workflow approaches can provide them, but might motivate making fewer upstream changes, and would require you to run some custom code.

epriestley closed subtask T12336: 233 as Invalid.Mar 1 2017, 1:43 PM
cos added a comment.Mar 1 2017, 3:54 PM

Q. Common or rare?
A. Common - we'd lock all tasks before a release.
Q. Lock comments? Do you need to lock comments too, or just other types of edits (like status, priority, title, description, etc)?
A. I'm ambivalent about this, if the lock is mandatory then being able to add comments seems reasonable since someone can point out that the task needs to be unlocked for some reason or other.
Q. In particular, could a solution where anyone could unlock the task be acceptable, as long as they had to explicitly unlock it? Or is it a hard regulatory requirement that unlocking requires some sort of administrator/oversight step?
A. Unclear, I'll ask, but it's probably fine since there's also a 'process' that states how people use the system. (And you'd be surprised how much drama there is inside private companies:-) That is, our conventions are formally documented and people expected (and expect) to follow them.
Q. Lock other objects?
A. Possibly, but I've been pushing back on it for things like code reviews by arguing that 'everything is logged and there's an audit trail' so what does a lock by us? Obviously locking git doesn't really make sense and new comments can't hide old ones so I don't see what the problem is. For tasks, I don't feel I can make that argument since people will naturally just look at the current state of the task unlike code reviews where the history is 'in your face' as it were.

I'll run your tentative recommendation by my team and get back to you shortly, thanks!

cos added a comment.Mar 1 2017, 10:38 PM

I can confirm that advisory locking is acceptable. Locking the whole task is preferable, but we can live with comments not being locked. Thanks!

epriestley added a comment.EditedMar 3 2017, 1:23 AM

We've implemented a basic version of this, roughly along the lines of the recommendation above. Here's how it works:

You can configure the new locked option on the existing maniphest.statuses in Config. In this example I've created a new "Locked" status, but you can set this flag on any existing status, like "Resolved":

When a task is locked by changing its status to a locked status, the comment form is removed and replaced with a note that the task is locked:

If you try to edit a locked task, you'll be prompted to confirm that you want to override the lock:

If you do, editing behaves normally. You can remove the lock by editing the task and reverting the status back to an open status.

Future Work

  • Locking a task doesn't change edit permissions, so users who could edit the task before the lock can still edit it and unlock the task, as long as they explicitly confirm that they want to override the lock. I think this will probably be OK based on your feedback that an advisory lock would be acceptable, but if this turns out to be a problem in practice we can explore other solutions.
  • The Conduit API works normally on locked tasks, since the action is essentially an edit and we don't have a way to prompt programs calling the API. I think this is probably the right behavior, but is worth being aware of. But we should possibly prevent comments via the API on locked tasks or make other behavioral adjustments here.
  • I think inbound email interactions currently work normally on locked tasks, but probably should not. Presumably, this won't cause much of an issue in practice. We'll clean this up sooner or later.

One use case for "locking" a task could be:

  1. For a release of the software product all tasks which represent the software changes on the version being released require review
  2. Reviewer looks over task form fields and if everything looks good, "signs" (locks) the task in a "reviewed" state
  3. Tasks can be unsigned prior to release of the version, allowing updates to tasks as necessary but also requiring another review
  4. Upon release of the version, tasks can no longer be unsigned

This process would ensure that all tasks for the version are reviewed and remain unchanged after review. Task forms are treated as documents that represent the design, development activities, and verification & validation of requirements to the product. These forms would need to be unchangeable as they would be part of a larger document that represents the released product.

d.maznekov added a subscriber: d.maznekov.EditedMar 8 2017, 11:51 AM

I have to report a possible bug as asked in Q584.
Message for editing locked task when create new non-existing task:

PS: The revision of local install is:

commit 9124bb416241bb117d50ced260e3b23e830882dd
Merge: 216f6be 8e26916
Author: epriestley <git@epriestley.com>
Date: Sat Mar 4 16:18:38 2017 -0800

(stable) Promote 2017 Week 9

Any resolution/guide how to avoid that obstacle ?

beber added a subscriber: beber.Mar 23 2017, 12:30 AM
This comment was removed by beber.

@beber, please file a new task with reproduction instructions that work on a clean install if you believe you've found a bug.

I suspect you didn't fully restart something after upgrading, since that method exists. See Restarting Phabricator for detailed guidance.

epriestley closed this task as Resolved.Apr 7 2017, 12:45 PM
epriestley claimed this task.

T12335#214054 discusses some future/followup work, but that can be filed separately as it becomes relevant.