unexpected/undocumented behavior from `PATCH /annotations/{id}` for group and permissions

[tgbugs]

trafficstars

Steps to reproduce

1. Issue a PATCH request changing the group and read permissions for an annotation following the api docs. For example if I want to move an annotation from a private group to __world__:

{'group': '__world__',
 'permissions': {'admin': ['acct:[email protected]'],
  'delete': ['acct:[email protected]'],
  'read': ['group:__world__'],
  'update': ['acct:[email protected]']}}

Expected behaviour

The annotation group and permissions should appear as above and be visible in the client to those with access to the specified group and read permissions.
Alternately, if the fields cannot be change the API should return a 400 error and no changes should be made to the annotation.

Actual behaviour

A 200 is returned. The annotation is not moved and is in fact made private.

{'group': '{original_private_group}',
 'permissions': {'admin': ['acct:[email protected]'],
  'delete': ['acct:[email protected]'],
  'read': ['acct:[email protected]'],
  'update': ['acct:[email protected]']}}

Browser/system information

N/A. The code in question.

Additional details

@robertknight pointed me to this line as the cause https://github.com/hypothesis/h/blob/34438d3d48ddb26c8bae370ba1d7c5b9c69ffea0/h/schemas/annotation.py#L198-L200

The very best solution to this would be to be able to include __world__ in the read permissions without having to also change the group as well. That way 'releasing to public' (or any other group) is merely a matter of adding that group to the read permissions. Not entirely sure how the current implementation would handle having replies to a single annotation in multiple groups.

Our use case is to be able to POST a large number of annotations into a private staging group where we (@bandrow @memartone) can check them for errors and then simply 'flip a switch' to make all reviewed annotations public by moving them to __world__ without having to worry about things like https://github.com/hypothesis/h/issues/4704 or some other issue with a change to the POSTing code.

I think one original concern related to implementing this functionality was that moving annotations from one group to another could orphan replies, but this routinely happens in our use due to deletes and the current behavior of making the annotation private also orphans replies (maybe a bug in itself?). I don't know the internals of the system so I don't know how moving an annotation to a group that the reply doesn't have access to interacts with permissions and security as compared to a delete where there is no possibility of leaking access. (There seems to be a longer conversation here about cross-group replies, but it is probably out of scope for this issue.)

[bugzy]

Just ran into this same bug. I spent the whole day trying to get this to work. Then came here to report the issue as a bug only to find this one from two years ago. Whereas it seems odd that the group of an annotation cannot be changed even via the api, the api docs should at least reflect this behavior if it is intended. Hope someone takes and a look at this and updates the docs regarding the current behavior so that no one else spends so much time on this.