commit b3d9736f251485f45669607c8e17c88c2f2b2958
author Edwin Kempin <ekempin@google.com> Mon Nov 13 09:26:03 2017 +0100
committer Edwin Kempin <ekempin@google.com> Mon Nov 13 13:31:28 2017 +0100
tree 054de83b734c1d0b4ec6d8d3e6ebb839a5a2c166
parent d8fbad036bc0c2571caf91340282267ee0e1e651

Revert "Represent group owner in NoteDb as permissions on group ref"

For groups in NoteDb we wanted to store group owners as permissions on
the group refs but we changed our mind and hence the change for storing
group owners as permissions is reverted.

The idea was that group owners are purely defined by permissions on the
group ref (PUSH/READ). Everyone who could modify the group ref was a
group owner. As with all permissions PUSH/READ could have been granted
directly on the group refs, on ref patterns (e.g. 'refs/*',
'refs/groups/*') or on refs with regular expressions, either on
All-Users or via inheritance on All-Projects. With this one could have
had multiple group owners, which was fine, but determining which groups
were group owner would have been difficult as this would have required
to evaluate all relevant permissions. Thus supporting an API to get and
set group owners would have been difficult (e.g. what should have been
done if a group was owner via permissions on 'refs/groups/*' but then
this group was removed as owner from one of the groups?). This is why
the API to get and set group owners was supposed to be deprecated and
removed. But then users would have needed to use the permission API to
configure group owners. This would have worked, but wouldn't have been
nice for the users. In practise this would have meant that:

* the group info screen could no longer show the group owner,

* to see/edit the group owners the user would have needed look at the
  group permissions on All-Users and find the corresponding group branch
  which only contains the sharded UUID of the group, but not the group
  name (and there could be thousands of group branches).

To address this we could have offered an API that only looks at the
permissions that are directly assigned on the group refs. But then this
API could provide wrong results (e.g. a group that was onwer by
permissions on the group ref, but blocked via inheritance, would still
be seen as group owner).

Since having an API to get and set group owners was found to be critical
and the approach of storing group owners as permissions doesn't work
well for it the idea to store group owners as permissions was discarded.
Hence it is reverted in this change. Follow-up changes will take care to
revert the deprecation of the group owner API.

Instead of storing group owners as permissions we now want to keep the
owner property on groups and store it in the 'groups.config' file inside
the group ref. The main advantage is that this requires no API changes.
We can continue to support the owner fields in GroupInfo, the
GetOwner/PutOwner REST endpoints and the ‘owner’ query operator. It also
simplifies the code that is needed for storing the group owner.

There were 2 concerns with this solution which is why storing group
owners as permissions was previously preferred:

* The group refs should be advertised for the group owners. If there are
  no permissions on the group refs VisibleRefFilter needs to check for
  each group if the calling user is a group owner and we thought that
  checking this would be too expensive. However with the caching that we
  already have the performance shouldn't be that bad. IdentifiedUser
  already caches the groups of the calling user and the group cache
  caches group information including the group owners. Assuming that the
  group cache is large enough to cache all groups all checks will be
  done in memory. Having a large enough group cache is relatively cheap
  and is also required to achieve good performance for other use cases.
  Also ls-remote calls to All-Users are relatively rare and hence
  performance of it is not that critical.

* There should be a single way to define group owners. By granting
  PUSH/READ permissions on the group ref additional owners groups can be
  assigned (anyone who can modify the group ref is effectively a group
  owner). Group owners via permissions on the group ref and the group
  owner in 'groups.config' can mismatch, which would be a source of
  confusion. To prevent this we decided to ignore all permissions on
  group refs and only respect the owner from 'groups.config'. This may
  be slightly confusing for experienced Gerrit admins ("I added
  permissions on the group ref but they are ignored?") but we already
  have a similar situation with the user branches. Permissions to user
  branches are ignored for foreign user branches if the calling user
  doesn't have the ACCESS_DATABASE capability. Doing the same for the
  group branches should be fine.

Since both concerns for storing group owners as group properties can be
addressed we think that this approach is better than storing group
owners as permissions which breaks our API (see above). There will be
follow-up changes to implement the new approach.

This reverts commit c5ba387c8ec35907f0e687426aa1d65cdef8cc95.

Change-Id: Id18851f503e711f38647843fd454cb25a579c54f
Signed-off-by: Edwin Kempin <ekempin@google.com>