opendev/gerrit
Code Issues Proposed changes

Reserve and protect refs/deleted-groups/ namespace in All-Users

Browse Source 
We may want to use the refs/deleted-groups/ namespace as an attic for
deleted groups. Reserve this namespace now and prevent anyone from
creating branches under this namespace.

Change-Id: I2f7590c03e3d68f324d96e63f5bacd08c7619815
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-11-20 11:29:42 +01:00
parent 2369ea7b6e
commit af9d190d30
3 changed files with 88 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
java/com/google/gerrit/reviewdb/client/RefNames.java
View File

@@ -88,6 +88,12 @@ public class RefNames {
/** NoteDb ref for the NoteMap of all group names */
public static final String REFS_GROUPNAMES = "refs/meta/group-names";
/**
* NoteDb ref for deleted groups {@code refs/deleted-groups}. This ref namespace is foreseen as an
* attic for deleted groups (it's reserved but not used yet)
*/
public static final String REFS_DELETED_GROUPS = "refs/deleted-groups/";
/** Draft inline comments of a user on a change */
public static final String REFS_DRAFT_COMMENTS = "refs/draft-comments/";
@@ -132,6 +138,10 @@ public class RefNames {
return REFS_GROUPS + shardUuid(groupUuid.get());
}
public static String refsDeletedGroups(AccountGroup.UUID groupUuid) {
return REFS_DELETED_GROUPS + shardUuid(groupUuid.get());
}
public static String refsUsers(Account.Id accountId) {
StringBuilder r = newStringBuilder().append(REFS_USERS);
return shard(accountId.get(), r).toString();
@@ -232,12 +242,20 @@ public class RefNames {
return ref.startsWith(REFS_GROUPS);
}
/**
* Whether the ref is a group branch that stores NoteDb data of a deleted group. Returns {@code
* true} for all refs that start with {@code refs/deleted-groups/}.
*/
public static boolean isRefsDeletedGroups(String ref) {
return ref.startsWith(REFS_DELETED_GROUPS);
}
/**
* Whether the ref is used for storing group data in NoteDb. Returns {@code true} for all group
* branches and refs/meta/group-names.
* branches, refs/meta/group-names and deleted group branches.
*/
public static boolean isGroupRef(String ref) {
return isRefsGroups(ref) || REFS_GROUPNAMES.equals(ref);
return isRefsGroups(ref) || isRefsDeletedGroups(ref) || REFS_GROUPNAMES.equals(ref);
}
static Integer parseShardedRefPart(String name) {

42
javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
View File

@@ -871,6 +871,14 @@ public class GroupsIT extends AbstractDaemonTest {
assertPushToGroupBranch(allUsers, groupRef, !groupsInNoteDb(), "group update not allowed");
}
@Test
public void pushToDeletedGroupBranchIsRejectedForAllUsersRepo() throws Exception {
String groupRef =
RefNames.refsDeletedGroups(
new AccountGroup.UUID(gApi.groups().create(name("foo")).get().id));
assertPushToGroupBranch(allUsers, groupRef, true, "group update not allowed");
}
@Test
public void pushToGroupNamesBranchIsRejectedForAllUsersRepo() throws Exception {
assume().that(groupsInNoteDb()).isTrue(); // branch only exists when groups are in NoteDb
@@ -887,6 +895,15 @@ public class GroupsIT extends AbstractDaemonTest {
assertPushToGroupBranch(project, groupRef, true, null);
}
@Test
public void pushToDeletedGroupsBranchForNonAllUsersRepo() throws Exception {
assertCreateGroupBranch(project, null);
String groupRef =
RefNames.refsDeletedGroups(
new AccountGroup.UUID(gApi.groups().create(name("foo")).get().id));
assertPushToGroupBranch(project, groupRef, true, null);
}
@Test
public void pushToGroupNamesBranchForNonAllUsersRepo() throws Exception {
assertPushToGroupBranch(project, RefNames.REFS_GROUPNAMES, true, null);
@@ -897,6 +914,8 @@ public class GroupsIT extends AbstractDaemonTest {
throws Exception {
grant(project, RefNames.REFS_GROUPS + "*", Permission.CREATE, false, REGISTERED_USERS);
grant(project, RefNames.REFS_GROUPS + "*", Permission.PUSH, false, REGISTERED_USERS);
grant(project, RefNames.REFS_DELETED_GROUPS + "*", Permission.CREATE, false, REGISTERED_USERS);
grant(project, RefNames.REFS_DELETED_GROUPS + "*", Permission.PUSH, false, REGISTERED_USERS);
grant(project, RefNames.REFS_GROUPNAMES, Permission.PUSH, false, REGISTERED_USERS);
TestRepository<InMemoryRepository> repo = cloneProject(project);
@@ -989,6 +1008,14 @@ public class GroupsIT extends AbstractDaemonTest {
RefNames.REFS_GROUPS + "*", RefNames.refsGroups(new AccountGroup.UUID(name("foo"))));
}
@Test
@Sandboxed
public void cannotCreateDeletedGroupBranch() throws Exception {
testCannotCreateGroupBranch(
RefNames.REFS_DELETED_GROUPS + "*",
RefNames.refsDeletedGroups(new AccountGroup.UUID(name("foo"))));
}
@Test
@Sandboxed
public void cannotCreateGroupNamesBranch() throws Exception {
@@ -1032,6 +1059,14 @@ public class GroupsIT extends AbstractDaemonTest {
testCannotDeleteGroupBranch(RefNames.REFS_GROUPS + "*", RefNames.refsGroups(adminGroupUuid()));
}
@Test
@Sandboxed
public void cannotDeleteDeletedGroupBranch() throws Exception {
String groupRef = RefNames.refsDeletedGroups(new AccountGroup.UUID(name("foo")));
createBranch(allUsers, groupRef, "Foo");
testCannotDeleteGroupBranch(RefNames.REFS_DELETED_GROUPS + "*", groupRef);
}
@Test
@Sandboxed
public void cannotDeleteGroupNamesBranch() throws Exception {
@@ -1183,6 +1218,11 @@ public class GroupsIT extends AbstractDaemonTest {
}
private void createGroupBranch(Project.NameKey project, String ref) throws IOException {
createBranch(project, ref, "Create Group");
}
private void createBranch(Project.NameKey project, String ref, String commitMessage)
throws IOException {
try (Repository r = repoManager.openRepository(project);
ObjectInserter oi = r.newObjectInserter();
RevWalk rw = new RevWalk(r)) {
@@ -1193,7 +1233,7 @@ public class GroupsIT extends AbstractDaemonTest {
cb.setTreeId(emptyTree);
cb.setCommitter(ident);
cb.setAuthor(ident);
cb.setMessage("Create group");
cb.setMessage(commitMessage);
ObjectId emptyCommit = oi.insert(cb);
oi.flush();

27
javatests/com/google/gerrit/reviewdb/client/RefNamesTest.java
View File

@@ -70,6 +70,20 @@ public class RefNamesTest {
RefNames.refsGroups(groupUuid);
}
@Test
public void refForDeletedGroupIsSharded() throws Exception {
AccountGroup.UUID groupUuid = new AccountGroup.UUID("ABCDEFG");
String groupRef = RefNames.refsDeletedGroups(groupUuid);
assertThat(groupRef).isEqualTo("refs/deleted-groups/AB/ABCDEFG");
}
@Test
public void refForDeletedGroupWithUuidLessThanTwoCharsIsRejected() throws Exception {
AccountGroup.UUID groupUuid = new AccountGroup.UUID("A");
expectedException.expect(IllegalArgumentException.class);
RefNames.refsDeletedGroups(groupUuid);
}
@Test
public void refsUsers() throws Exception {
assertThat(RefNames.refsUsers(accountId)).isEqualTo("refs/users/23/1011123");
@@ -133,11 +147,24 @@ public class RefNamesTest {
assertThat(RefNames.isRefsGroups("refs/heads/master")).isFalse();
assertThat(RefNames.isRefsGroups("refs/users/23/1011123")).isFalse();
assertThat(RefNames.isRefsGroups(RefNames.REFS_GROUPNAMES)).isFalse();
assertThat(RefNames.isRefsGroups("refs/deleted-groups/" + TEST_SHARDED_GROUP_UUID)).isFalse();
}
@Test
public void isRefsDeletedGroups() throws Exception {
assertThat(RefNames.isRefsDeletedGroups("refs/deleted-groups/" + TEST_SHARDED_GROUP_UUID))
.isTrue();
assertThat(RefNames.isRefsDeletedGroups("refs/heads/master")).isFalse();
assertThat(RefNames.isRefsDeletedGroups("refs/users/23/1011123")).isFalse();
assertThat(RefNames.isRefsDeletedGroups(RefNames.REFS_GROUPNAMES)).isFalse();
assertThat(RefNames.isRefsDeletedGroups("refs/groups/" + TEST_SHARDED_GROUP_UUID)).isFalse();
}
@Test
public void isGroupRef() throws Exception {
assertThat(RefNames.isGroupRef("refs/groups/" + TEST_SHARDED_GROUP_UUID)).isTrue();
assertThat(RefNames.isGroupRef("refs/deleted-groups/" + TEST_SHARDED_GROUP_UUID)).isTrue();
assertThat(RefNames.isGroupRef(RefNames.REFS_GROUPNAMES)).isTrue();
assertThat(RefNames.isGroupRef("refs/heads/master")).isFalse();