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 c5ba387c8e
.
Change-Id: Id18851f503e711f38647843fd454cb25a579c54f
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
@@ -13,7 +13,6 @@
|
||||
// limitations under the License.
|
||||
package com.google.gerrit.extensions.api.access;
|
||||
|
||||
import com.google.common.base.MoreObjects;
|
||||
import java.util.Map;
|
||||
import java.util.Objects;
|
||||
|
||||
@@ -42,13 +41,4 @@ public class PermissionInfo {
|
||||
public int hashCode() {
|
||||
return Objects.hash(label, exclusive, rules);
|
||||
}
|
||||
|
||||
@Override
|
||||
public String toString() {
|
||||
return MoreObjects.toStringHelper(this)
|
||||
.add("label", label)
|
||||
.add("exclusive", exclusive)
|
||||
.add("rules", rules)
|
||||
.toString();
|
||||
}
|
||||
}
|
||||
|
@@ -13,7 +13,6 @@
|
||||
// limitations under the License.
|
||||
package com.google.gerrit.extensions.api.access;
|
||||
|
||||
import com.google.common.base.MoreObjects;
|
||||
import java.util.Objects;
|
||||
|
||||
public class PermissionRuleInfo {
|
||||
@@ -51,14 +50,4 @@ public class PermissionRuleInfo {
|
||||
public int hashCode() {
|
||||
return Objects.hash(action, force, min, max);
|
||||
}
|
||||
|
||||
@Override
|
||||
public String toString() {
|
||||
return MoreObjects.toStringHelper(this)
|
||||
.add("action", action)
|
||||
.add("force", force)
|
||||
.add("min", min)
|
||||
.add("max", max)
|
||||
.toString();
|
||||
}
|
||||
}
|
||||
|
Reference in New Issue
Block a user