Disallow deletion of user branches
When accounts are migrated to NoteDb the existence of a user branch is mandatory for an account. Deleting the user branch would mean deleting the account but we don't support account deletion. Hence disallow deletion of user branches even if the user has the DELETE permission. Only allow the deletion of a user branch if the calling user also has the ACCESS_DATABASE capability. With this capability an account entry can be deleted in ReviewDb, hence it should allow to do the same in NoteDb. Change-Id: I5e67a86b51a038483222f3e57bd1399a9f290db3 Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:

committed by
David Pursehouse

parent
3f221dc596
commit
8b7c852b13
@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.api.accounts;
|
|||||||
import static com.google.common.base.Preconditions.checkNotNull;
|
import static com.google.common.base.Preconditions.checkNotNull;
|
||||||
import static com.google.common.truth.Truth.assertThat;
|
import static com.google.common.truth.Truth.assertThat;
|
||||||
import static com.google.common.truth.Truth.assert_;
|
import static com.google.common.truth.Truth.assert_;
|
||||||
|
import static com.google.gerrit.acceptance.GitUtil.deleteRef;
|
||||||
import static com.google.gerrit.acceptance.GitUtil.fetch;
|
import static com.google.gerrit.acceptance.GitUtil.fetch;
|
||||||
import static com.google.gerrit.gpg.PublicKeyStore.REFS_GPG_KEYS;
|
import static com.google.gerrit.gpg.PublicKeyStore.REFS_GPG_KEYS;
|
||||||
import static com.google.gerrit.gpg.PublicKeyStore.keyToString;
|
import static com.google.gerrit.gpg.PublicKeyStore.keyToString;
|
||||||
@@ -44,6 +45,7 @@ import com.google.gerrit.acceptance.PushOneCommit;
|
|||||||
import com.google.gerrit.acceptance.Sandboxed;
|
import com.google.gerrit.acceptance.Sandboxed;
|
||||||
import com.google.gerrit.acceptance.TestAccount;
|
import com.google.gerrit.acceptance.TestAccount;
|
||||||
import com.google.gerrit.acceptance.UseSsh;
|
import com.google.gerrit.acceptance.UseSsh;
|
||||||
|
import com.google.gerrit.common.data.GlobalCapability;
|
||||||
import com.google.gerrit.common.data.Permission;
|
import com.google.gerrit.common.data.Permission;
|
||||||
import com.google.gerrit.extensions.api.accounts.EmailInput;
|
import com.google.gerrit.extensions.api.accounts.EmailInput;
|
||||||
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
|
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
|
||||||
@@ -99,6 +101,8 @@ import org.eclipse.jgit.lib.Ref;
|
|||||||
import org.eclipse.jgit.lib.RefUpdate;
|
import org.eclipse.jgit.lib.RefUpdate;
|
||||||
import org.eclipse.jgit.lib.Repository;
|
import org.eclipse.jgit.lib.Repository;
|
||||||
import org.eclipse.jgit.transport.PushCertificateIdent;
|
import org.eclipse.jgit.transport.PushCertificateIdent;
|
||||||
|
import org.eclipse.jgit.transport.PushResult;
|
||||||
|
import org.eclipse.jgit.transport.RemoteRefUpdate;
|
||||||
import org.junit.After;
|
import org.junit.After;
|
||||||
import org.junit.Assert;
|
import org.junit.Assert;
|
||||||
import org.junit.Before;
|
import org.junit.Before;
|
||||||
@@ -682,6 +686,50 @@ public class AccountIT extends AbstractDaemonTest {
|
|||||||
WatchConfig.WATCH_CONFIG, admin.getId().get(), project.get(), invalidNotifyValue));
|
WatchConfig.WATCH_CONFIG, admin.getId().get(), project.get(), invalidNotifyValue));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
@Sandboxed
|
||||||
|
public void cannotDeleteUserBranch() throws Exception {
|
||||||
|
grant(
|
||||||
|
Permission.DELETE,
|
||||||
|
allUsers,
|
||||||
|
RefNames.REFS_USERS + "${" + RefPattern.USERID_SHARDED + "}",
|
||||||
|
true,
|
||||||
|
REGISTERED_USERS);
|
||||||
|
|
||||||
|
TestRepository<InMemoryRepository> allUsersRepo = cloneProject(allUsers);
|
||||||
|
String userRef = RefNames.refsUsers(admin.id);
|
||||||
|
PushResult r = deleteRef(allUsersRepo, userRef);
|
||||||
|
RemoteRefUpdate refUpdate = r.getRemoteUpdate(userRef);
|
||||||
|
assertThat(refUpdate.getStatus()).isEqualTo(RemoteRefUpdate.Status.REJECTED_OTHER_REASON);
|
||||||
|
assertThat(refUpdate.getMessage()).contains("Not allowed to delete user branch.");
|
||||||
|
|
||||||
|
try (Repository repo = repoManager.openRepository(allUsers)) {
|
||||||
|
assertThat(repo.exactRef(userRef)).isNotNull();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
@Sandboxed
|
||||||
|
public void deleteUserBranchWithAccessDatabaseCapability() throws Exception {
|
||||||
|
allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
|
||||||
|
grant(
|
||||||
|
Permission.DELETE,
|
||||||
|
allUsers,
|
||||||
|
RefNames.REFS_USERS + "${" + RefPattern.USERID_SHARDED + "}",
|
||||||
|
true,
|
||||||
|
REGISTERED_USERS);
|
||||||
|
|
||||||
|
TestRepository<InMemoryRepository> allUsersRepo = cloneProject(allUsers);
|
||||||
|
String userRef = RefNames.refsUsers(admin.id);
|
||||||
|
PushResult r = deleteRef(allUsersRepo, userRef);
|
||||||
|
RemoteRefUpdate refUpdate = r.getRemoteUpdate(userRef);
|
||||||
|
assertThat(refUpdate.getStatus()).isEqualTo(RemoteRefUpdate.Status.OK);
|
||||||
|
|
||||||
|
try (Repository repo = repoManager.openRepository(allUsers)) {
|
||||||
|
assertThat(repo.exactRef(userRef)).isNull();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void addGpgKey() throws Exception {
|
public void addGpgKey() throws Exception {
|
||||||
TestKey key = validKeyWithoutExpiration();
|
TestKey key = validKeyWithoutExpiration();
|
||||||
|
@@ -14,10 +14,13 @@
|
|||||||
package com.google.gerrit.server.git.validators;
|
package com.google.gerrit.server.git.validators;
|
||||||
|
|
||||||
import com.google.common.base.Predicate;
|
import com.google.common.base.Predicate;
|
||||||
|
import com.google.common.collect.ImmutableList;
|
||||||
import com.google.common.collect.Iterables;
|
import com.google.common.collect.Iterables;
|
||||||
import com.google.gerrit.extensions.registration.DynamicSet;
|
import com.google.gerrit.extensions.registration.DynamicSet;
|
||||||
import com.google.gerrit.reviewdb.client.Project;
|
import com.google.gerrit.reviewdb.client.Project;
|
||||||
|
import com.google.gerrit.reviewdb.client.RefNames;
|
||||||
import com.google.gerrit.server.IdentifiedUser;
|
import com.google.gerrit.server.IdentifiedUser;
|
||||||
|
import com.google.gerrit.server.config.AllUsersName;
|
||||||
import com.google.gerrit.server.events.RefReceivedEvent;
|
import com.google.gerrit.server.events.RefReceivedEvent;
|
||||||
import com.google.gerrit.server.validators.ValidationException;
|
import com.google.gerrit.server.validators.ValidationException;
|
||||||
import com.google.inject.Inject;
|
import com.google.inject.Inject;
|
||||||
@@ -42,15 +45,18 @@ public class RefOperationValidators {
|
|||||||
update.getExpectedOldObjectId(), update.getNewObjectId(), update.getName(), type);
|
update.getExpectedOldObjectId(), update.getNewObjectId(), update.getName(), type);
|
||||||
}
|
}
|
||||||
|
|
||||||
private final RefReceivedEvent event;
|
private final AllUsersName allUsersName;
|
||||||
private final DynamicSet<RefOperationValidationListener> refOperationValidationListeners;
|
private final DynamicSet<RefOperationValidationListener> refOperationValidationListeners;
|
||||||
|
private final RefReceivedEvent event;
|
||||||
|
|
||||||
@Inject
|
@Inject
|
||||||
RefOperationValidators(
|
RefOperationValidators(
|
||||||
|
AllUsersName allUsersName,
|
||||||
DynamicSet<RefOperationValidationListener> refOperationValidationListeners,
|
DynamicSet<RefOperationValidationListener> refOperationValidationListeners,
|
||||||
@Assisted Project project,
|
@Assisted Project project,
|
||||||
@Assisted IdentifiedUser user,
|
@Assisted IdentifiedUser user,
|
||||||
@Assisted ReceiveCommand cmd) {
|
@Assisted ReceiveCommand cmd) {
|
||||||
|
this.allUsersName = allUsersName;
|
||||||
this.refOperationValidationListeners = refOperationValidationListeners;
|
this.refOperationValidationListeners = refOperationValidationListeners;
|
||||||
event = new RefReceivedEvent();
|
event = new RefReceivedEvent();
|
||||||
event.command = cmd;
|
event.command = cmd;
|
||||||
@@ -59,11 +65,13 @@ public class RefOperationValidators {
|
|||||||
}
|
}
|
||||||
|
|
||||||
public List<ValidationMessage> validateForRefOperation() throws RefOperationValidationException {
|
public List<ValidationMessage> validateForRefOperation() throws RefOperationValidationException {
|
||||||
|
|
||||||
List<ValidationMessage> messages = new ArrayList<>();
|
List<ValidationMessage> messages = new ArrayList<>();
|
||||||
boolean withException = false;
|
boolean withException = false;
|
||||||
|
List<RefOperationValidationListener> listeners = new ArrayList<>();
|
||||||
|
listeners.add(new DisallowDeletionOfUserBranches(allUsersName));
|
||||||
|
refOperationValidationListeners.forEach(l -> listeners.add(l));
|
||||||
try {
|
try {
|
||||||
for (RefOperationValidationListener listener : refOperationValidationListeners) {
|
for (RefOperationValidationListener listener : listeners) {
|
||||||
messages.addAll(listener.onRefOperation(event));
|
messages.addAll(listener.onRefOperation(event));
|
||||||
}
|
}
|
||||||
} catch (ValidationException e) {
|
} catch (ValidationException e) {
|
||||||
@@ -95,4 +103,26 @@ public class RefOperationValidators {
|
|||||||
return input.isError();
|
return input.isError();
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private static class DisallowDeletionOfUserBranches implements RefOperationValidationListener {
|
||||||
|
private final AllUsersName allUsersName;
|
||||||
|
|
||||||
|
DisallowDeletionOfUserBranches(AllUsersName allUsersName) {
|
||||||
|
this.allUsersName = allUsersName;
|
||||||
|
}
|
||||||
|
|
||||||
|
@Override
|
||||||
|
public List<ValidationMessage> onRefOperation(RefReceivedEvent refEvent)
|
||||||
|
throws ValidationException {
|
||||||
|
if (refEvent.project.getNameKey().equals(allUsersName)
|
||||||
|
&& (refEvent.command.getRefName().startsWith(RefNames.REFS_USERS)
|
||||||
|
&& !refEvent.command.getRefName().equals(RefNames.REFS_USERS_DEFAULT))
|
||||||
|
&& refEvent.command.getType().equals(ReceiveCommand.Type.DELETE)) {
|
||||||
|
if (!refEvent.user.getCapabilities().canAccessDatabase()) {
|
||||||
|
throw new ValidationException("Not allowed to delete user branch.");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return ImmutableList.of();
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
Reference in New Issue
Block a user