Hide ChangeControl#isVisible and migrate callers to PermissionBackend

Change-Id: I8cec3c3c182e0ca2b684370e5beeae11a95b69a1
This commit is contained in:
Patrick Hiesel
2017-08-03 08:54:36 +02:00
parent d501e25340
commit 5116a9984a
33 changed files with 242 additions and 115 deletions

View File

@@ -26,6 +26,7 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.change.Submit; import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.git.ChangeSet; import com.google.gerrit.server.git.ChangeSet;
import com.google.gerrit.server.git.MergeSuperSet; import com.google.gerrit.server.git.MergeSuperSet;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.testutil.ConfigSuite; import com.google.gerrit.testutil.ConfigSuite;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
@@ -304,7 +305,8 @@ public class SubmitResolvingMergeCommitIT extends AbstractDaemonTest {
} }
private void assertChangeSetMergeable(ChangeData change, boolean expected) private void assertChangeSetMergeable(ChangeData change, boolean expected)
throws MissingObjectException, IncorrectObjectTypeException, IOException, OrmException { throws MissingObjectException, IncorrectObjectTypeException, IOException, OrmException,
PermissionBackendException {
ChangeSet cs = mergeSuperSet.get().completeChangeSet(db, change.change(), user(admin)); ChangeSet cs = mergeSuperSet.get().completeChangeSet(db, change.change(), user(admin));
assertThat(submit.unmergeableChanges(cs).isEmpty()).isEqualTo(expected); assertThat(submit.unmergeableChanges(cs).isEmpty()).isEqualTo(expected);
} }

View File

@@ -29,6 +29,7 @@ import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.events.ProjectEvent; import com.google.gerrit.server.events.ProjectEvent;
import com.google.gerrit.server.events.RefEvent; import com.google.gerrit.server.events.RefEvent;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.permissions.ProjectPermission;
@@ -86,7 +87,8 @@ public class EventBroker implements EventDispatcher {
} }
@Override @Override
public void postEvent(Change change, ChangeEvent event) throws OrmException { public void postEvent(Change change, ChangeEvent event)
throws OrmException, PermissionBackendException {
fireEvent(change, event); fireEvent(change, event);
} }
@@ -101,7 +103,7 @@ public class EventBroker implements EventDispatcher {
} }
@Override @Override
public void postEvent(Event event) throws OrmException { public void postEvent(Event event) throws OrmException, PermissionBackendException {
fireEvent(event); fireEvent(event);
} }
@@ -111,7 +113,8 @@ public class EventBroker implements EventDispatcher {
} }
} }
protected void fireEvent(Change change, ChangeEvent event) throws OrmException { protected void fireEvent(Change change, ChangeEvent event)
throws OrmException, PermissionBackendException {
for (UserScopedEventListener listener : listeners) { for (UserScopedEventListener listener : listeners) {
if (isVisibleTo(change, listener.getUser())) { if (isVisibleTo(change, listener.getUser())) {
listener.onEvent(event); listener.onEvent(event);
@@ -138,7 +141,7 @@ public class EventBroker implements EventDispatcher {
fireEventForUnrestrictedListeners(event); fireEventForUnrestrictedListeners(event);
} }
protected void fireEvent(Event event) throws OrmException { protected void fireEvent(Event event) throws OrmException, PermissionBackendException {
for (UserScopedEventListener listener : listeners) { for (UserScopedEventListener listener : listeners) {
if (isVisibleTo(event, listener.getUser())) { if (isVisibleTo(event, listener.getUser())) {
listener.onEvent(event); listener.onEvent(event);
@@ -156,7 +159,8 @@ public class EventBroker implements EventDispatcher {
} }
} }
protected boolean isVisibleTo(Change change, CurrentUser user) throws OrmException { protected boolean isVisibleTo(Change change, CurrentUser user)
throws OrmException, PermissionBackendException {
if (change == null) { if (change == null) {
return false; return false;
} }
@@ -164,9 +168,12 @@ public class EventBroker implements EventDispatcher {
if (pe == null) { if (pe == null) {
return false; return false;
} }
ProjectControl pc = pe.controlFor(user);
ReviewDb db = dbProvider.get(); ReviewDb db = dbProvider.get();
return pc.controlFor(db, change).isVisible(db); return permissionBackend
.user(user)
.change(notesFactory.createChecked(db, change))
.database(db)
.test(ChangePermission.READ);
} }
protected boolean isVisibleTo(Branch.NameKey branchName, CurrentUser user) { protected boolean isVisibleTo(Branch.NameKey branchName, CurrentUser user) {
@@ -178,7 +185,8 @@ public class EventBroker implements EventDispatcher {
return pc.controlForRef(branchName).isVisible(); return pc.controlForRef(branchName).isVisible();
} }
protected boolean isVisibleTo(Event event, CurrentUser user) throws OrmException { protected boolean isVisibleTo(Event event, CurrentUser user)
throws OrmException, PermissionBackendException {
if (event instanceof RefEvent) { if (event instanceof RefEvent) {
RefEvent refEvent = (RefEvent) event; RefEvent refEvent = (RefEvent) event;
String ref = refEvent.getRefName(); String ref = refEvent.getRefName();

View File

@@ -21,6 +21,7 @@ import com.google.gerrit.server.events.ChangeEvent;
import com.google.gerrit.server.events.Event; import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.events.ProjectEvent; import com.google.gerrit.server.events.ProjectEvent;
import com.google.gerrit.server.events.RefEvent; import com.google.gerrit.server.events.RefEvent;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
/** Interface for posting (dispatching) Events */ /** Interface for posting (dispatching) Events */
@@ -31,8 +32,9 @@ public interface EventDispatcher {
* @param change The change that the event is related to * @param change The change that the event is related to
* @param event The event to post * @param event The event to post
* @throws OrmException on failure to post the event due to DB error * @throws OrmException on failure to post the event due to DB error
* @throws PermissionBackendException on failure of permission checks
*/ */
void postEvent(Change change, ChangeEvent event) throws OrmException; void postEvent(Change change, ChangeEvent event) throws OrmException, PermissionBackendException;
/** /**
* Post a stream event that is related to a branch * Post a stream event that is related to a branch
@@ -58,6 +60,7 @@ public interface EventDispatcher {
* *
* @param event The event to post. * @param event The event to post.
* @throws OrmException on failure to post the event due to DB error * @throws OrmException on failure to post the event due to DB error
* @throws PermissionBackendException on failure of permission checks
*/ */
void postEvent(Event event) throws OrmException; void postEvent(Event event) throws OrmException, PermissionBackendException;
} }

View File

@@ -47,6 +47,9 @@ import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.util.LabelVote; import com.google.gerrit.server.util.LabelVote;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
@@ -108,6 +111,7 @@ public class ApprovalsUtil {
private final IdentifiedUser.GenericFactory userFactory; private final IdentifiedUser.GenericFactory userFactory;
private final ChangeControl.GenericFactory changeControlFactory; private final ChangeControl.GenericFactory changeControlFactory;
private final ApprovalCopier copier; private final ApprovalCopier copier;
private final PermissionBackend permissionBackend;
@VisibleForTesting @VisibleForTesting
@Inject @Inject
@@ -115,11 +119,13 @@ public class ApprovalsUtil {
NotesMigration migration, NotesMigration migration,
IdentifiedUser.GenericFactory userFactory, IdentifiedUser.GenericFactory userFactory,
ChangeControl.GenericFactory changeControlFactory, ChangeControl.GenericFactory changeControlFactory,
ApprovalCopier copier) { ApprovalCopier copier,
PermissionBackend permissionBackend) {
this.migration = migration; this.migration = migration;
this.userFactory = userFactory; this.userFactory = userFactory;
this.changeControlFactory = changeControlFactory; this.changeControlFactory = changeControlFactory;
this.copier = copier; this.copier = copier;
this.permissionBackend = permissionBackend;
} }
/** /**
@@ -262,8 +268,8 @@ public class ApprovalsUtil {
private boolean canSee(ReviewDb db, ChangeNotes notes, Account.Id accountId) { private boolean canSee(ReviewDb db, ChangeNotes notes, Account.Id accountId) {
try { try {
IdentifiedUser user = userFactory.create(accountId); IdentifiedUser user = userFactory.create(accountId);
return changeControlFactory.controlFor(notes, user).isVisible(db); return permissionBackend.user(user).change(notes).database(db).test(ChangePermission.READ);
} catch (OrmException e) { } catch (PermissionBackendException e) {
log.warn( log.warn(
String.format( String.format(
"Failed to check if account %d can see change %d", "Failed to check if account %d can see change %d",

View File

@@ -32,6 +32,7 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection; import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.QueryChanges; import com.google.gerrit.server.query.change.QueryChanges;
import com.google.gwtorm.server.OrmDuplicateKeyException; import com.google.gwtorm.server.OrmDuplicateKeyException;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
@@ -67,7 +68,7 @@ public class StarredChanges
@Override @Override
public AccountResource.StarredChange parse(AccountResource parent, IdString id) public AccountResource.StarredChange parse(AccountResource parent, IdString id)
throws ResourceNotFoundException, OrmException { throws ResourceNotFoundException, OrmException, PermissionBackendException {
IdentifiedUser user = parent.getUser(); IdentifiedUser user = parent.getUser();
ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id); ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id);
if (starredChangesUtil if (starredChangesUtil
@@ -104,7 +105,7 @@ public class StarredChanges
return createProvider.get().setChange(changes.parse(TopLevelResource.INSTANCE, id)); return createProvider.get().setChange(changes.parse(TopLevelResource.INSTANCE, id));
} catch (ResourceNotFoundException e) { } catch (ResourceNotFoundException e) {
throw new UnprocessableEntityException(String.format("change %s not found", id.get())); throw new UnprocessableEntityException(String.format("change %s not found", id.get()));
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("cannot resolve change", e); log.error("cannot resolve change", e);
throw new UnprocessableEntityException("internal server error"); throw new UnprocessableEntityException("internal server error");
} }

View File

@@ -33,6 +33,7 @@ import com.google.gerrit.server.StarredChangesUtil.IllegalLabelException;
import com.google.gerrit.server.account.AccountResource.Star; import com.google.gerrit.server.account.AccountResource.Star;
import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection; import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.QueryChanges; import com.google.gerrit.server.query.change.QueryChanges;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
@@ -65,7 +66,7 @@ public class Stars implements ChildCollection<AccountResource, AccountResource.S
@Override @Override
public Star parse(AccountResource parent, IdString id) public Star parse(AccountResource parent, IdString id)
throws ResourceNotFoundException, OrmException { throws ResourceNotFoundException, OrmException, PermissionBackendException {
IdentifiedUser user = parent.getUser(); IdentifiedUser user = parent.getUser();
ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id); ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id);
Set<String> labels = starredChangesUtil.getLabels(user.getAccountId(), change.getId()); Set<String> labels = starredChangesUtil.getLabels(user.getAccountId(), change.getId());

View File

@@ -53,7 +53,9 @@ import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectControl;
@@ -475,8 +477,12 @@ public class ChangeInserter implements InsertChangeOp {
accountId -> { accountId -> {
try { try {
IdentifiedUser user = userFactory.create(accountId); IdentifiedUser user = userFactory.create(accountId);
return changeControlFactory.controlFor(notes, user).isVisible(db); return permissionBackend
} catch (OrmException e) { .user(user)
.change(notes)
.database(db)
.test(ChangePermission.READ);
} catch (PermissionBackendException e) {
log.warn( log.warn(
String.format( String.format(
"Failed to check if account %d can see change %d", "Failed to check if account %d can see change %d",

View File

@@ -26,6 +26,9 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeFinder; import com.google.gerrit.server.ChangeFinder;
import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.QueryChanges; import com.google.gerrit.server.query.change.QueryChanges;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
@@ -44,6 +47,7 @@ public class ChangesCollection
private final ChangeFinder changeFinder; private final ChangeFinder changeFinder;
private final CreateChange createChange; private final CreateChange createChange;
private final ChangeResource.Factory changeResourceFactory; private final ChangeResource.Factory changeResourceFactory;
private final PermissionBackend permissionBackend;
@Inject @Inject
ChangesCollection( ChangesCollection(
@@ -53,7 +57,8 @@ public class ChangesCollection
DynamicMap<RestView<ChangeResource>> views, DynamicMap<RestView<ChangeResource>> views,
ChangeFinder changeFinder, ChangeFinder changeFinder,
CreateChange createChange, CreateChange createChange,
ChangeResource.Factory changeResourceFactory) { ChangeResource.Factory changeResourceFactory,
PermissionBackend permissionBackend) {
this.db = db; this.db = db;
this.user = user; this.user = user;
this.queryFactory = queryFactory; this.queryFactory = queryFactory;
@@ -61,6 +66,7 @@ public class ChangesCollection
this.changeFinder = changeFinder; this.changeFinder = changeFinder;
this.createChange = createChange; this.createChange = createChange;
this.changeResourceFactory = changeResourceFactory; this.changeResourceFactory = changeResourceFactory;
this.permissionBackend = permissionBackend;
} }
@Override @Override
@@ -75,7 +81,7 @@ public class ChangesCollection
@Override @Override
public ChangeResource parse(TopLevelResource root, IdString id) public ChangeResource parse(TopLevelResource root, IdString id)
throws ResourceNotFoundException, OrmException { throws ResourceNotFoundException, OrmException, PermissionBackendException {
List<ChangeControl> ctls = changeFinder.find(id.encoded(), user.get()); List<ChangeControl> ctls = changeFinder.find(id.encoded(), user.get());
if (ctls.isEmpty()) { if (ctls.isEmpty()) {
throw new ResourceNotFoundException(id); throw new ResourceNotFoundException(id);
@@ -84,13 +90,14 @@ public class ChangesCollection
} }
ChangeControl ctl = ctls.get(0); ChangeControl ctl = ctls.get(0);
if (!ctl.isVisible(db.get())) { if (!canRead(ctl)) {
throw new ResourceNotFoundException(id); throw new ResourceNotFoundException(id);
} }
return changeResourceFactory.create(ctl); return changeResourceFactory.create(ctl);
} }
public ChangeResource parse(Change.Id id) throws ResourceNotFoundException, OrmException { public ChangeResource parse(Change.Id id)
throws ResourceNotFoundException, OrmException, PermissionBackendException {
List<ChangeControl> ctls = changeFinder.find(id, user.get()); List<ChangeControl> ctls = changeFinder.find(id, user.get());
if (ctls.isEmpty()) { if (ctls.isEmpty()) {
throw new ResourceNotFoundException(toIdString(id)); throw new ResourceNotFoundException(toIdString(id));
@@ -99,7 +106,7 @@ public class ChangesCollection
} }
ChangeControl ctl = ctls.get(0); ChangeControl ctl = ctls.get(0);
if (!ctl.isVisible(db.get())) { if (!canRead(ctl)) {
throw new ResourceNotFoundException(toIdString(id)); throw new ResourceNotFoundException(toIdString(id));
} }
return changeResourceFactory.create(ctl); return changeResourceFactory.create(ctl);
@@ -118,4 +125,12 @@ public class ChangesCollection
public CreateChange post(TopLevelResource parent) throws RestApiException { public CreateChange post(TopLevelResource parent) throws RestApiException {
return createChange; return createChange;
} }
private boolean canRead(ChangeControl ctl) throws PermissionBackendException {
return permissionBackend
.user(user)
.change(ctl.getNotes())
.database(db)
.test(ChangePermission.READ);
}
} }

View File

@@ -52,6 +52,7 @@ import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.permissions.RefPermission;
@@ -195,7 +196,11 @@ public class CreateChange
throw new UnprocessableEntityException("Base change not found: " + input.baseChange); throw new UnprocessableEntityException("Base change not found: " + input.baseChange);
} }
ChangeControl ctl = Iterables.getOnlyElement(ctls); ChangeControl ctl = Iterables.getOnlyElement(ctls);
if (!ctl.isVisible(db.get())) { if (!permissionBackend
.user(user)
.change(ctl.getNotes())
.database(db)
.test(ChangePermission.READ)) {
throw new UnprocessableEntityException("Base change not found: " + input.baseChange); throw new UnprocessableEntityException("Base change not found: " + input.baseChange);
} }
PatchSet ps = psUtil.current(db.get(), ctl.getNotes()); PatchSet ps = psUtil.current(db.get(), ctl.getNotes());

View File

@@ -24,6 +24,7 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.ChangeSet; import com.google.gerrit.server.git.ChangeSet;
import com.google.gerrit.server.git.MergeSuperSet; import com.google.gerrit.server.git.MergeSuperSet;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.OrmRuntimeException; import com.google.gwtorm.server.OrmRuntimeException;
@@ -74,7 +75,7 @@ public class GetRevisionActions implements ETagView<RevisionResource> {
changeResourceFactory.create(cd.changeControl()).prepareETag(h, user); changeResourceFactory.create(cd.changeControl()).prepareETag(h, user);
} }
h.putBoolean(cs.furtherHiddenChanges()); h.putBoolean(cs.furtherHiddenChanges());
} catch (IOException | OrmException e) { } catch (IOException | OrmException | PermissionBackendException e) {
throw new OrmRuntimeException(e); throw new OrmRuntimeException(e);
} }
return h.hash().toString(); return h.hash().toString();

View File

@@ -56,6 +56,7 @@ import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.mail.send.OutgoingEmailValidator; import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.permissions.RefPermission;
@@ -338,8 +339,12 @@ public class PostReviewers
ReviewerState state, ReviewerState state,
NotifyHandling notify, NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify) ListMultimap<RecipientType, Account.Id> accountsToNotify)
throws OrmException { throws OrmException, PermissionBackendException {
if (!rsrc.getControl().forUser(anonymousProvider.get()).isVisible(dbProvider.get())) { if (!permissionBackend
.user(anonymousProvider)
.change(rsrc.getNotes())
.database(dbProvider)
.test(ChangePermission.READ)) {
return fail( return fail(
reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer)); reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
} }

View File

@@ -34,6 +34,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.MergeOp; import com.google.gerrit.server.git.MergeOp;
import com.google.gerrit.server.git.MergeOpRepoManager; import com.google.gerrit.server.git.MergeOpRepoManager;
import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo; import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.update.UpdateException; import com.google.gerrit.server.update.UpdateException;
@@ -83,7 +84,8 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
@Override @Override
public BinaryResult apply(RevisionResource rsrc) public BinaryResult apply(RevisionResource rsrc)
throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException { throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException,
PermissionBackendException {
if (Strings.isNullOrEmpty(format)) { if (Strings.isNullOrEmpty(format)) {
throw new BadRequestException("format is not specified"); throw new BadRequestException("format is not specified");
} }
@@ -111,7 +113,8 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
} }
private BinaryResult getBundles(RevisionResource rsrc, ArchiveFormat f) private BinaryResult getBundles(RevisionResource rsrc, ArchiveFormat f)
throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException { throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException,
PermissionBackendException {
ReviewDb db = dbProvider.get(); ReviewDb db = dbProvider.get();
ChangeControl control = rsrc.getControl(); ChangeControl control = rsrc.getControl();
IdentifiedUser caller = control.getUser().asIdentifiedUser(); IdentifiedUser caller = control.getUser().asIdentifiedUser();
@@ -131,7 +134,8 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
| UpdateException | UpdateException
| IOException | IOException
| ConfigInvalidException | ConfigInvalidException
| RuntimeException e) { | RuntimeException
| PermissionBackendException e) {
op.close(); op.close();
throw e; throw e;
} }

View File

@@ -217,7 +217,8 @@ public class Submit
} }
public Change mergeChange(RevisionResource rsrc, IdentifiedUser submitter, SubmitInput input) public Change mergeChange(RevisionResource rsrc, IdentifiedUser submitter, SubmitInput input)
throws OrmException, RestApiException, IOException, UpdateException, ConfigInvalidException { throws OrmException, RestApiException, IOException, UpdateException, ConfigInvalidException,
PermissionBackendException {
Change change = rsrc.getChange(); Change change = rsrc.getChange();
if (!change.getStatus().isOpen()) { if (!change.getStatus().isOpen()) {
throw new ResourceConflictException("change is " + ChangeUtil.status(change)); throw new ResourceConflictException("change is " + ChangeUtil.status(change));
@@ -340,7 +341,7 @@ public class Submit
ChangeSet cs; ChangeSet cs;
try { try {
cs = mergeSuperSet.get().completeChangeSet(db, cd.change(), resource.getControl().getUser()); cs = mergeSuperSet.get().completeChangeSet(db, cd.change(), resource.getControl().getUser());
} catch (OrmException | IOException e) { } catch (OrmException | IOException | PermissionBackendException e) {
throw new OrmRuntimeException( throw new OrmRuntimeException(
"Could not determine complete set of changes to be submitted", e); "Could not determine complete set of changes to be submitted", e);
} }

View File

@@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.change.WalkSorter.PatchSetData; import com.google.gerrit.server.change.WalkSorter.PatchSetData;
import com.google.gerrit.server.git.ChangeSet; import com.google.gerrit.server.git.ChangeSet;
import com.google.gerrit.server.git.MergeSuperSet; import com.google.gerrit.server.git.MergeSuperSet;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
@@ -107,7 +108,7 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
@Override @Override
public Object apply(ChangeResource resource) public Object apply(ChangeResource resource)
throws AuthException, BadRequestException, ResourceConflictException, IOException, throws AuthException, BadRequestException, ResourceConflictException, IOException,
OrmException { OrmException, PermissionBackendException {
SubmittedTogetherInfo info = applyInfo(resource); SubmittedTogetherInfo info = applyInfo(resource);
if (options.isEmpty()) { if (options.isEmpty()) {
return info.changes; return info.changes;
@@ -116,7 +117,7 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
} }
public SubmittedTogetherInfo applyInfo(ChangeResource resource) public SubmittedTogetherInfo applyInfo(ChangeResource resource)
throws AuthException, IOException, OrmException { throws AuthException, IOException, OrmException, PermissionBackendException {
Change c = resource.getChange(); Change c = resource.getChange();
try { try {
List<ChangeData> cds; List<ChangeData> cds;

View File

@@ -53,6 +53,7 @@ import com.google.gerrit.server.data.PatchSetAttribute;
import com.google.gerrit.server.data.RefUpdateAttribute; import com.google.gerrit.server.data.RefUpdateAttribute;
import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectCache;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
@@ -261,7 +262,7 @@ public class StreamEventsApiListener
event.oldAssignee = accountAttributeSupplier(ev.getOldAssignee()); event.oldAssignee = accountAttributeSupplier(ev.getOldAssignee());
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -277,7 +278,7 @@ public class StreamEventsApiListener
event.oldTopic = ev.getOldTopic(); event.oldTopic = ev.getOldTopic();
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -295,7 +296,7 @@ public class StreamEventsApiListener
event.uploader = accountAttributeSupplier(ev.getWho()); event.uploader = accountAttributeSupplier(ev.getWho());
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -315,7 +316,7 @@ public class StreamEventsApiListener
approvalsAttributeSupplier(change, ev.getNewApprovals(), ev.getOldApprovals()); approvalsAttributeSupplier(change, ev.getNewApprovals(), ev.getOldApprovals());
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -333,7 +334,7 @@ public class StreamEventsApiListener
event.reviewer = accountAttributeSupplier(reviewer); event.reviewer = accountAttributeSupplier(reviewer);
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} }
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -360,7 +361,7 @@ public class StreamEventsApiListener
event.removed = hashtagArray(ev.getRemovedHashtags()); event.removed = hashtagArray(ev.getRemovedHashtags());
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -399,7 +400,7 @@ public class StreamEventsApiListener
event.uploader = accountAttributeSupplier(ev.getWho()); event.uploader = accountAttributeSupplier(ev.getWho());
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -419,7 +420,7 @@ public class StreamEventsApiListener
event.approvals = approvalsAttributeSupplier(change, ev.getApprovals(), ev.getOldApprovals()); event.approvals = approvalsAttributeSupplier(change, ev.getApprovals(), ev.getOldApprovals());
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -437,7 +438,7 @@ public class StreamEventsApiListener
event.reason = ev.getReason(); event.reason = ev.getReason();
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -455,7 +456,7 @@ public class StreamEventsApiListener
event.newRev = ev.getNewRevisionId(); event.newRev = ev.getNewRevisionId();
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -473,7 +474,7 @@ public class StreamEventsApiListener
event.reason = ev.getReason(); event.reason = ev.getReason();
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }
@@ -493,7 +494,7 @@ public class StreamEventsApiListener
event.approvals = approvalsAttributeSupplier(change, ev.getApprovals(), ev.getOldApprovals()); event.approvals = approvalsAttributeSupplier(change, ev.getApprovals(), ev.getOldApprovals());
dispatcher.get().postEvent(change, event); dispatcher.get().postEvent(change, event);
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Failed to dispatch event", e); log.error("Failed to dispatch event", e);
} }
} }

View File

@@ -63,6 +63,7 @@ import com.google.gerrit.server.git.strategy.SubmitStrategyFactory;
import com.google.gerrit.server.git.strategy.SubmitStrategyListener; import com.google.gerrit.server.git.strategy.SubmitStrategyListener;
import com.google.gerrit.server.git.validators.MergeValidationException; import com.google.gerrit.server.git.validators.MergeValidationException;
import com.google.gerrit.server.git.validators.MergeValidators; import com.google.gerrit.server.git.validators.MergeValidators;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.SubmitRuleOptions; import com.google.gerrit.server.project.SubmitRuleOptions;
@@ -423,6 +424,7 @@ public class MergeOp implements AutoCloseable {
* @param submitInput parameters regarding the merge * @param submitInput parameters regarding the merge
* @throws OrmException an error occurred reading or writing the database. * @throws OrmException an error occurred reading or writing the database.
* @throws RestApiException if an error occurred. * @throws RestApiException if an error occurred.
* @throws PermissionBackendException if permissions can't be checked
*/ */
public void merge( public void merge(
ReviewDb db, ReviewDb db,
@@ -431,7 +433,8 @@ public class MergeOp implements AutoCloseable {
boolean checkSubmitRules, boolean checkSubmitRules,
SubmitInput submitInput, SubmitInput submitInput,
boolean dryrun) boolean dryrun)
throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException { throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException,
PermissionBackendException {
this.submitInput = submitInput; this.submitInput = submitInput;
this.accountsToNotify = notifyUtil.resolveAccounts(submitInput.notifyDetails); this.accountsToNotify = notifyUtil.resolveAccounts(submitInput.notifyDetails);
this.dryrun = dryrun; this.dryrun = dryrun;

View File

@@ -37,6 +37,9 @@ import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo; import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo;
import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gerrit.server.project.SubmitRuleEvaluator;
@@ -101,6 +104,7 @@ public class MergeSuperSet {
private final ChangeData.Factory changeDataFactory; private final ChangeData.Factory changeDataFactory;
private final Provider<InternalChangeQuery> queryProvider; private final Provider<InternalChangeQuery> queryProvider;
private final Provider<MergeOpRepoManager> repoManagerProvider; private final Provider<MergeOpRepoManager> repoManagerProvider;
private final PermissionBackend permissionBackend;
private final Config cfg; private final Config cfg;
private final Map<QueryKey, List<ChangeData>> queryCache; private final Map<QueryKey, List<ChangeData>> queryCache;
private final Map<Branch.NameKey, Optional<RevCommit>> heads; private final Map<Branch.NameKey, Optional<RevCommit>> heads;
@@ -115,13 +119,15 @@ public class MergeSuperSet {
Accounts accounts, Accounts accounts,
ChangeData.Factory changeDataFactory, ChangeData.Factory changeDataFactory,
Provider<InternalChangeQuery> queryProvider, Provider<InternalChangeQuery> queryProvider,
Provider<MergeOpRepoManager> repoManagerProvider) { Provider<MergeOpRepoManager> repoManagerProvider,
PermissionBackend permissionBackend) {
this.cfg = cfg; this.cfg = cfg;
this.accountCache = accountCache; this.accountCache = accountCache;
this.accounts = accounts; this.accounts = accounts;
this.changeDataFactory = changeDataFactory; this.changeDataFactory = changeDataFactory;
this.queryProvider = queryProvider; this.queryProvider = queryProvider;
this.repoManagerProvider = repoManagerProvider; this.repoManagerProvider = repoManagerProvider;
this.permissionBackend = permissionBackend;
queryCache = new HashMap<>(); queryCache = new HashMap<>();
heads = new HashMap<>(); heads = new HashMap<>();
} }
@@ -134,11 +140,13 @@ public class MergeSuperSet {
} }
public ChangeSet completeChangeSet(ReviewDb db, Change change, CurrentUser user) public ChangeSet completeChangeSet(ReviewDb db, Change change, CurrentUser user)
throws IOException, OrmException { throws IOException, OrmException, PermissionBackendException {
try { try {
ChangeData cd = changeDataFactory.create(db, change.getProject(), change.getId()); ChangeData cd = changeDataFactory.create(db, change.getProject(), change.getId());
cd.changeControl(user); cd.changeControl(user);
ChangeSet cs = new ChangeSet(cd, cd.changeControl().isVisible(db, cd)); ChangeSet cs =
new ChangeSet(
cd, permissionBackend.user(user).change(cd).database(db).test(ChangePermission.READ));
if (Submit.wholeTopicEnabled(cfg)) { if (Submit.wholeTopicEnabled(cfg)) {
return completeChangeSetIncludingTopics(db, cs, user); return completeChangeSetIncludingTopics(db, cs, user);
} }
@@ -212,7 +220,7 @@ public class MergeSuperSet {
} }
private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes, CurrentUser user) private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes, CurrentUser user)
throws IOException, OrmException { throws IOException, OrmException, PermissionBackendException {
Collection<ChangeData> visibleChanges = new ArrayList<>(); Collection<ChangeData> visibleChanges = new ArrayList<>();
Collection<ChangeData> nonVisibleChanges = new ArrayList<>(); Collection<ChangeData> nonVisibleChanges = new ArrayList<>();
@@ -231,7 +239,7 @@ public class MergeSuperSet {
+ " at ChangeData creation time"); + " at ChangeData creation time");
boolean visible = changes.ids().contains(cd.getId()); boolean visible = changes.ids().contains(cd.getId());
if (visible && !cd.changeControl().isVisible(db, cd)) { if (visible && !canRead(db, user, cd)) {
// We thought the change was visible, but it isn't. // We thought the change was visible, but it isn't.
// This can happen if the ACL changes during the // This can happen if the ACL changes during the
// completeChangeSet computation, for example. // completeChangeSet computation, for example.
@@ -357,7 +365,7 @@ public class MergeSuperSet {
CurrentUser user, CurrentUser user,
Set<String> topicsSeen, Set<String> topicsSeen,
Set<String> visibleTopicsSeen) Set<String> visibleTopicsSeen)
throws OrmException { throws OrmException, PermissionBackendException {
List<ChangeData> visibleChanges = new ArrayList<>(); List<ChangeData> visibleChanges = new ArrayList<>();
List<ChangeData> nonVisibleChanges = new ArrayList<>(); List<ChangeData> nonVisibleChanges = new ArrayList<>();
@@ -370,7 +378,7 @@ public class MergeSuperSet {
for (ChangeData topicCd : query().byTopicOpen(topic)) { for (ChangeData topicCd : query().byTopicOpen(topic)) {
try { try {
topicCd.changeControl(user); topicCd.changeControl(user);
if (topicCd.changeControl().isVisible(db, topicCd)) { if (canRead(db, user, topicCd)) {
visibleChanges.add(topicCd); visibleChanges.add(topicCd);
} else { } else {
nonVisibleChanges.add(topicCd); nonVisibleChanges.add(topicCd);
@@ -402,7 +410,8 @@ public class MergeSuperSet {
} }
private ChangeSet completeChangeSetIncludingTopics( private ChangeSet completeChangeSetIncludingTopics(
ReviewDb db, ChangeSet changes, CurrentUser user) throws IOException, OrmException { ReviewDb db, ChangeSet changes, CurrentUser user)
throws IOException, OrmException, PermissionBackendException {
Set<String> topicsSeen = new HashSet<>(); Set<String> topicsSeen = new HashSet<>();
Set<String> visibleTopicsSeen = new HashSet<>(); Set<String> visibleTopicsSeen = new HashSet<>();
int oldSeen; int oldSeen;
@@ -443,4 +452,9 @@ public class MergeSuperSet {
logError(msg); logError(msg);
throw new OrmException(msg); throw new OrmException(msg);
} }
private boolean canRead(ReviewDb db, CurrentUser user, ChangeData cd)
throws PermissionBackendException {
return permissionBackend.user(user).change(cd).database(db).test(ChangePermission.READ);
}
} }

View File

@@ -819,7 +819,8 @@ public class ReceiveCommits {
| OrmException | OrmException
| UpdateException | UpdateException
| IOException | IOException
| ConfigInvalidException e) { | ConfigInvalidException
| PermissionBackendException e) {
logError("Error submitting changes to " + project.getName(), e); logError("Error submitting changes to " + project.getName(), e);
reject(magicBranchCmd, "error during submit"); reject(magicBranchCmd, "error during submit");
} }
@@ -2264,7 +2265,8 @@ public class ReceiveCommits {
} }
private void submit(Collection<CreateRequest> create, Collection<ReplaceRequest> replace) private void submit(Collection<CreateRequest> create, Collection<ReplaceRequest> replace)
throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException { throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException,
PermissionBackendException {
Map<ObjectId, Change> bySha = Maps.newHashMapWithExpectedSize(create.size() + replace.size()); Map<ObjectId, Change> bySha = Maps.newHashMapWithExpectedSize(create.size() + replace.size());
for (CreateRequest r : create) { for (CreateRequest r : create) {
checkNotNull(r.change, "cannot submit new change %s; op may not have run", r.changeId); checkNotNull(r.change, "cannot submit new change %s; op may not have run", r.changeId);

View File

@@ -30,8 +30,10 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData;
@@ -262,12 +264,16 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
try { try {
Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>(); Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>();
for (ChangeData cd : changeCache.getChangeData(db.get(), project.getNameKey())) { for (ChangeData cd : changeCache.getChangeData(db.get(), project.getNameKey())) {
if (projectCtl.controlForIndexedChange(cd.change()).isVisible(db.get(), cd)) { if (permissionBackend
.user(user)
.indexedChange(cd, changeNotesFactory.createFromIndexedChange(cd.change()))
.database(db)
.test(ChangePermission.READ)) {
visibleChanges.put(cd.getId(), cd.change().getDest()); visibleChanges.put(cd.getId(), cd.change().getDest());
} }
} }
return visibleChanges; return visibleChanges;
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error( log.error(
"Cannot load changes for project " "Cannot load changes for project "
+ project.getName() + project.getName()
@@ -282,12 +288,12 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
try { try {
Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>(); Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>();
for (ChangeNotes cn : changeNotesFactory.scan(git, db.get(), project)) { for (ChangeNotes cn : changeNotesFactory.scan(git, db.get(), project)) {
if (projectCtl.controlFor(cn).isVisible(db.get())) { if (permissionBackend.user(user).change(cn).database(db).test(ChangePermission.READ)) {
visibleChanges.put(cn.getChangeId(), cn.getChange().getDest()); visibleChanges.put(cn.getChangeId(), cn.getChange().getDest());
} }
} }
return visibleChanges; return visibleChanges;
} catch (IOException | OrmException e) { } catch (IOException | OrmException | PermissionBackendException e) {
log.error( log.error(
"Cannot load changes for project " + project + ", assuming no changes are visible", e); "Cannot load changes for project " + project + ", assuming no changes are visible", e);
return Collections.emptyMap(); return Collections.emptyMap();

View File

@@ -38,6 +38,7 @@ import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListEntry; import com.google.gerrit.server.patch.PatchListEntry;
import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.project.ProjectState;
@@ -405,12 +406,12 @@ public abstract class ChangeEmail extends NotificationEmail {
} }
@Override @Override
protected boolean isVisibleTo(Account.Id to) throws OrmException { protected boolean isVisibleTo(Account.Id to) throws OrmException, PermissionBackendException {
return projectState == null return args.permissionBackend
|| projectState .user(args.identifiedUserFactory.create(to))
.controlFor(args.identifiedUserFactory.create(to)) .change(changeData)
.controlFor(args.db.get(), change) .database(args.db.get())
.isVisible(args.db.get()); .test(ChangePermission.READ);
} }
/** Find all users who are authors of any part of this change. */ /** Find all users who are authors of any part of this change. */

View File

@@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.client.UserIdentity;
import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.mail.send.EmailHeader.AddressList; import com.google.gerrit.server.mail.send.EmailHeader.AddressList;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.validators.OutgoingEmailValidationListener; import com.google.gerrit.server.validators.OutgoingEmailValidationListener;
import com.google.gerrit.server.validators.ValidationException; import com.google.gerrit.server.validators.ValidationException;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
@@ -479,7 +480,7 @@ public abstract class OutgoingEmail {
rcptTo.add(to); rcptTo.add(to);
add(rt, toAddress(to), override); add(rt, toAddress(to), override);
} }
} catch (OrmException e) { } catch (OrmException | PermissionBackendException e) {
log.error("Error reading database for account: " + to, e); log.error("Error reading database for account: " + to, e);
} }
} }
@@ -487,9 +488,10 @@ public abstract class OutgoingEmail {
/** /**
* @param to account. * @param to account.
* @throws OrmException * @throws OrmException
* @throws PermissionBackendException
* @return whether this email is visible to the given account. * @return whether this email is visible to the given account.
*/ */
protected boolean isVisibleTo(Account.Id to) throws OrmException { protected boolean isVisibleTo(Account.Id to) throws OrmException, PermissionBackendException {
return true; return true;
} }

View File

@@ -119,7 +119,12 @@ public class FailedPermissionBackend {
} }
@Override @Override
public ForChange change(ChangeNotes cd) { public ForChange change(ChangeNotes notes) {
return new FailedChange(message, cause);
}
@Override
public ForChange indexedChange(ChangeData cd, ChangeNotes notes) {
return new FailedChange(message, cause); return new FailedChange(message, cause);
} }

View File

@@ -139,6 +139,15 @@ public abstract class PermissionBackend {
return ref(notes.getChange().getDest()).change(notes); return ref(notes.getChange().getDest()).change(notes);
} }
/**
* @return instance scoped for the change loaded from index, and its destination ref and
* project. This method should only be used when database access is harmful and potentially
* stale data from the index is acceptable.
*/
public ForChange indexedChange(ChangeData cd, ChangeNotes notes) {
return ref(notes.getChange().getDest()).indexedChange(cd, notes);
}
/** Verify scoped user can {@code perm}, throwing if denied. */ /** Verify scoped user can {@code perm}, throwing if denied. */
public abstract void check(GlobalOrPluginPermission perm) public abstract void check(GlobalOrPluginPermission perm)
throws AuthException, PermissionBackendException; throws AuthException, PermissionBackendException;
@@ -269,6 +278,12 @@ public abstract class PermissionBackend {
/** @return instance scoped to change. */ /** @return instance scoped to change. */
public abstract ForChange change(ChangeNotes notes); public abstract ForChange change(ChangeNotes notes);
/**
* @return instance scoped to change loaded from index. This method should only be used when
* database access is harmful and potentially stale data from the index is acceptable.
*/
public abstract ForChange indexedChange(ChangeData cd, ChangeNotes notes);
/** Verify scoped user can {@code perm}, throwing if denied. */ /** Verify scoped user can {@code perm}, throwing if denied. */
public abstract void check(RefPermission perm) throws AuthException, PermissionBackendException; public abstract void check(RefPermission perm) throws AuthException, PermissionBackendException;

View File

@@ -136,18 +136,6 @@ public class ChangeControl {
return create(refControl, notesFactory.create(db, project, changeId)); return create(refControl, notesFactory.create(db, project, changeId));
} }
/**
* Create a change control for a change that was loaded from index. This method should only be
* used when database access is harmful and potentially stale data from the index is acceptable.
*
* @param refControl ref control
* @param change change loaded from secondary index
* @return change control
*/
ChangeControl createForIndexedChange(RefControl refControl, Change change) {
return create(refControl, notesFactory.createFromIndexedChange(change));
}
ChangeControl create(RefControl refControl, ChangeNotes notes) { ChangeControl create(RefControl refControl, ChangeNotes notes) {
return new ChangeControl(changeDataFactory, approvalsUtil, refControl, notes, patchSetUtil); return new ChangeControl(changeDataFactory, approvalsUtil, refControl, notes, patchSetUtil);
} }
@@ -209,12 +197,12 @@ public class ChangeControl {
} }
/** Can this user see this change? */ /** Can this user see this change? */
public boolean isVisible(ReviewDb db) throws OrmException { boolean isVisible(ReviewDb db) throws OrmException {
return isVisible(db, null); return isVisible(db, null);
} }
/** Can this user see this change? */ /** Can this user see this change? */
public boolean isVisible(ReviewDb db, @Nullable ChangeData cd) throws OrmException { private boolean isVisible(ReviewDb db, @Nullable ChangeData cd) throws OrmException {
if (getChange().isPrivate() && !isPrivateVisible(db, cd)) { if (getChange().isPrivate() && !isPrivateVisible(db, cd)) {
return false; return false;
} }
@@ -226,6 +214,7 @@ public class ChangeControl {
/** Can this user see the given patchset? */ /** Can this user see the given patchset? */
public boolean isPatchVisible(PatchSet ps, ReviewDb db) throws OrmException { public boolean isPatchVisible(PatchSet ps, ReviewDb db) throws OrmException {
// TODO(hiesel) These don't need to be migrated, just remove after support for drafts is removed
if (ps != null && ps.isDraft() && !isDraftVisible(db, null)) { if (ps != null && ps.isDraft() && !isDraftVisible(db, null)) {
return false; return false;
} }
@@ -234,6 +223,7 @@ public class ChangeControl {
/** Can this user see the given patchset? */ /** Can this user see the given patchset? */
public boolean isPatchVisible(PatchSet ps, ChangeData cd) throws OrmException { public boolean isPatchVisible(PatchSet ps, ChangeData cd) throws OrmException {
// TODO(hiesel) These don't need to be migrated, just remove after support for drafts is removed
checkArgument( checkArgument(
cd.getId().equals(ps.getId().getParentKey()), "%s not for change %s", ps, cd.getId()); cd.getId().equals(ps.getId().getParentKey()), "%s not for change %s", ps, cd.getId());
if (ps.isDraft() && !isDraftVisible(cd.db(), cd)) { if (ps.isDraft() && !isDraftVisible(cd.db(), cd)) {

View File

@@ -190,17 +190,6 @@ public class ProjectControl {
controlForRef(change.getDest()), db, change.getProject(), change.getId()); controlForRef(change.getDest()), db, change.getProject(), change.getId());
} }
/**
* Create a change control for a change that was loaded from index. This method should only be
* used when database access is harmful and potentially stale data from the index is acceptable.
*
* @param change change loaded from secondary index
* @return change control
*/
public ChangeControl controlForIndexedChange(Change change) {
return changeControlFactory.createForIndexedChange(controlForRef(change.getDest()), change);
}
public ChangeControl controlFor(ChangeNotes notes) { public ChangeControl controlFor(ChangeNotes notes) {
return changeControlFactory.create(controlForRef(notes.getChange().getDest()), notes); return changeControlFactory.create(controlForRef(notes.getChange().getDest()), notes);
} }

View File

@@ -712,6 +712,11 @@ public class RefControl {
return getProjectControl().controlFor(notes).asForChange(null, db); return getProjectControl().controlFor(notes).asForChange(null, db);
} }
@Override
public ForChange indexedChange(ChangeData cd, ChangeNotes notes) {
return getProjectControl().controlFor(notes).asForChange(cd, db);
}
@Override @Override
public void check(RefPermission perm) throws AuthException, PermissionBackendException { public void check(RefPermission perm) throws AuthException, PermissionBackendException {
if (!can(perm)) { if (!can(perm)) {

View File

@@ -18,6 +18,9 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.IsVisibleToPredicate; import com.google.gerrit.server.query.IsVisibleToPredicate;
@@ -29,17 +32,20 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
protected final ChangeNotes.Factory notesFactory; protected final ChangeNotes.Factory notesFactory;
protected final ChangeControl.GenericFactory changeControl; protected final ChangeControl.GenericFactory changeControl;
protected final CurrentUser user; protected final CurrentUser user;
protected final PermissionBackend permissionBackend;
public ChangeIsVisibleToPredicate( public ChangeIsVisibleToPredicate(
Provider<ReviewDb> db, Provider<ReviewDb> db,
ChangeNotes.Factory notesFactory, ChangeNotes.Factory notesFactory,
ChangeControl.GenericFactory changeControlFactory, ChangeControl.GenericFactory changeControlFactory,
CurrentUser user) { CurrentUser user,
PermissionBackend permissionBackend) {
super(ChangeQueryBuilder.FIELD_VISIBLETO, describe(user)); super(ChangeQueryBuilder.FIELD_VISIBLETO, describe(user));
this.db = db; this.db = db;
this.notesFactory = notesFactory; this.notesFactory = notesFactory;
this.changeControl = changeControlFactory; this.changeControl = changeControlFactory;
this.user = user; this.user = user;
this.permissionBackend = permissionBackend;
} }
@Override @Override
@@ -47,21 +53,35 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
if (cd.fastIsVisibleTo(user)) { if (cd.fastIsVisibleTo(user)) {
return true; return true;
} }
Change change;
try { try {
Change c = cd.change(); change = cd.change();
if (c == null) { if (change == null) {
return false; return false;
} }
ChangeNotes notes = notesFactory.createFromIndexedChange(c);
ChangeControl cc = changeControl.controlFor(notes, user);
if (cc.isVisible(db.get(), cd)) {
cd.cacheVisibleTo(cc);
return true;
}
} catch (NoSuchChangeException e) { } catch (NoSuchChangeException e) {
// Ignored // Ignored
return false;
} }
ChangeNotes notes = notesFactory.createFromIndexedChange(change);
ChangeControl cc = changeControl.controlFor(notes, user);
boolean visible;
try {
visible =
permissionBackend
.user(user)
.indexedChange(cd, notes)
.database(db)
.test(ChangePermission.READ);
} catch (PermissionBackendException e) {
throw new OrmException("unable to check permissions", e);
}
if (visible) {
cd.cacheVisibleTo(cc);
return true;
}
return false; return false;
} }

View File

@@ -940,7 +940,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
public Predicate<ChangeData> visibleto(CurrentUser user) { public Predicate<ChangeData> visibleto(CurrentUser user) {
return new ChangeIsVisibleToPredicate( return new ChangeIsVisibleToPredicate(
args.db, args.notesFactory, args.changeControlGenericFactory, user); args.db, args.notesFactory, args.changeControlGenericFactory, user, args.permissionBackend);
} }
public Predicate<ChangeData> is_visible() throws QueryParseException { public Predicate<ChangeData> is_visible() throws QueryParseException {

View File

@@ -30,6 +30,7 @@ import com.google.gerrit.server.index.change.ChangeIndexRewriter;
import com.google.gerrit.server.index.change.ChangeSchemaDefinitions; import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
import com.google.gerrit.server.index.change.IndexedChangeQuery; import com.google.gerrit.server.index.change.IndexedChangeQuery;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.Predicate; import com.google.gerrit.server.query.Predicate;
import com.google.gerrit.server.query.QueryProcessor; import com.google.gerrit.server.query.QueryProcessor;
@@ -55,6 +56,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
private final ChangeControl.GenericFactory changeControlFactory; private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeNotes.Factory notesFactory; private final ChangeNotes.Factory notesFactory;
private final DynamicMap<ChangeAttributeFactory> attributeFactories; private final DynamicMap<ChangeAttributeFactory> attributeFactories;
private final PermissionBackend permissionBackend;
static { static {
// It is assumed that basic rewrites do not touch visibleto predicates. // It is assumed that basic rewrites do not touch visibleto predicates.
@@ -74,7 +76,8 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
Provider<ReviewDb> db, Provider<ReviewDb> db,
ChangeControl.GenericFactory changeControlFactory, ChangeControl.GenericFactory changeControlFactory,
ChangeNotes.Factory notesFactory, ChangeNotes.Factory notesFactory,
DynamicMap<ChangeAttributeFactory> attributeFactories) { DynamicMap<ChangeAttributeFactory> attributeFactories,
PermissionBackend permissionBackend) {
super( super(
userProvider, userProvider,
limitsFactory, limitsFactory,
@@ -88,6 +91,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
this.changeControlFactory = changeControlFactory; this.changeControlFactory = changeControlFactory;
this.notesFactory = notesFactory; this.notesFactory = notesFactory;
this.attributeFactories = attributeFactories; this.attributeFactories = attributeFactories;
this.permissionBackend = permissionBackend;
} }
@Override @Override
@@ -130,7 +134,8 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
protected Predicate<ChangeData> enforceVisibility(Predicate<ChangeData> pred) { protected Predicate<ChangeData> enforceVisibility(Predicate<ChangeData> pred) {
return new AndChangeSource( return new AndChangeSource(
pred, pred,
new ChangeIsVisibleToPredicate(db, notesFactory, changeControlFactory, userProvider.get()), new ChangeIsVisibleToPredicate(
db, notesFactory, changeControlFactory, userProvider.get(), permissionBackend),
start); start);
} }
} }

View File

@@ -25,6 +25,7 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection; import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -68,13 +69,13 @@ public class ChangeArgumentParser {
} }
public void addChange(String id, Map<Change.Id, ChangeResource> changes) public void addChange(String id, Map<Change.Id, ChangeResource> changes)
throws UnloggedFailure, OrmException { throws UnloggedFailure, OrmException, PermissionBackendException {
addChange(id, changes, null); addChange(id, changes, null);
} }
public void addChange( public void addChange(
String id, Map<Change.Id, ChangeResource> changes, ProjectControl projectControl) String id, Map<Change.Id, ChangeResource> changes, ProjectControl projectControl)
throws UnloggedFailure, OrmException { throws UnloggedFailure, OrmException, PermissionBackendException {
addChange(id, changes, projectControl, true); addChange(id, changes, projectControl, true);
} }
@@ -83,7 +84,7 @@ public class ChangeArgumentParser {
Map<Change.Id, ChangeResource> changes, Map<Change.Id, ChangeResource> changes,
ProjectControl projectControl, ProjectControl projectControl,
boolean useIndex) boolean useIndex)
throws UnloggedFailure, OrmException { throws UnloggedFailure, OrmException, PermissionBackendException {
List<ChangeControl> matched = List<ChangeControl> matched =
useIndex ? changeFinder.find(id, currentUser) : changeFromNotesFactory(id, currentUser); useIndex ? changeFinder.find(id, currentUser) : changeFromNotesFactory(id, currentUser);
List<ChangeControl> toAdd = new ArrayList<>(changes.size()); List<ChangeControl> toAdd = new ArrayList<>(changes.size());
@@ -97,7 +98,12 @@ public class ChangeArgumentParser {
for (ChangeControl ctl : matched) { for (ChangeControl ctl : matched) {
if (!changes.containsKey(ctl.getId()) if (!changes.containsKey(ctl.getId())
&& inProject(projectControl, ctl.getProject()) && inProject(projectControl, ctl.getProject())
&& (canMaintainServer || ctl.isVisible(db))) { && (canMaintainServer
|| permissionBackend
.user(currentUser)
.change(ctl.getNotes())
.database(db)
.test(ChangePermission.READ))) {
toAdd.add(ctl); toAdd.add(ctl);
} }
} }

View File

@@ -17,6 +17,7 @@ package com.google.gerrit.sshd.commands;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.Index; import com.google.gerrit.server.change.Index;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.sshd.ChangeArgumentParser; import com.google.gerrit.sshd.ChangeArgumentParser;
import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand; import com.google.gerrit.sshd.SshCommand;
@@ -42,7 +43,7 @@ final class IndexChangesCommand extends SshCommand {
void addChange(String token) { void addChange(String token) {
try { try {
changeArgumentParser.addChange(token, changes, null, false); changeArgumentParser.addChange(token, changes, null, false);
} catch (UnloggedFailure | OrmException e) { } catch (UnloggedFailure | OrmException | PermissionBackendException e) {
writeError("warning", e.getMessage()); writeError("warning", e.getMessage());
} }
} }

View File

@@ -23,6 +23,7 @@ import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.DeleteReviewer; import com.google.gerrit.server.change.DeleteReviewer;
import com.google.gerrit.server.change.PostReviewers; import com.google.gerrit.server.change.PostReviewers;
import com.google.gerrit.server.change.ReviewerResource; import com.google.gerrit.server.change.ReviewerResource;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.sshd.ChangeArgumentParser; import com.google.gerrit.sshd.ChangeArgumentParser;
import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.CommandMetaData;
@@ -79,6 +80,8 @@ public class SetReviewersCommand extends SshCommand {
throw new IllegalArgumentException(e.getMessage(), e); throw new IllegalArgumentException(e.getMessage(), e);
} catch (OrmException e) { } catch (OrmException e) {
throw new IllegalArgumentException("database is down", e); throw new IllegalArgumentException("database is down", e);
} catch (PermissionBackendException e) {
throw new IllegalArgumentException("can't check permissions", e);
} }
} }