opendev/gerrit
Code Issues Proposed changes

Make project state check in READ explicit

Browse Source 
The majority of code in {Project,Ref,Change}Control is now about
permissions, but not all. Exceptions include checks for a project's
state. This is confusing, because users are presented with an exception
telling them that they lack some kind of permission while the real
reason for the failed operation is that the project's current state
doesn't permit the operation.

This is part of a series of commits to remove all project state checks
from *Control classes and make explicit checks instead.

Calls from resources in restapi/change/* need no explicit check if the
project state permits reads as this is checked in ChangesCollection.

Change-Id: Ifde8885cf48d7a63af52fe5ce3347f880f131d48
This commit is contained in:
Patrick Hiesel
2018-01-15 17:58:15 +01:00
parent a9a057e1db
commit 0431dc2cae
20 changed files with 171 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
java/com/google/gerrit/httpd/raw/CatServlet.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.httpd.raw;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Patch;
@@ -29,6 +30,7 @@ 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.ProjectCache;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -57,6 +59,7 @@ public class CatServlet extends HttpServlet {
private final PatchSetUtil psUtil;
private final ChangeNotes.Factory changeNotesFactory;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
@Inject
CatServlet(
@@ -65,13 +68,15 @@ public class CatServlet extends HttpServlet {
ChangeEditUtil ceu,
PatchSetUtil psu,
ChangeNotes.Factory cnf,
PermissionBackend pb) {
PermissionBackend pb,
ProjectCache pc) {
requestDb = sf;
userProvider = usrprv;
changeEditUtil = ceu;
psUtil = psu;
changeNotesFactory = cnf;
permissionBackend = pb;
projectCache = pc;
}
@Override
@@ -131,6 +136,7 @@ public class CatServlet extends HttpServlet {
.change(notes)
.database(requestDb)
.check(ChangePermission.READ);
projectCache.checkedGet(notes.getProjectName()).checkStatePermitsRead();
if (patchKey.getParentKey().get() == 0) {
// change edit
Optional<ChangeEdit> edit = changeEditUtil.byChange(notes);
@@ -148,10 +154,10 @@ public class CatServlet extends HttpServlet {
}
revision = patchSet.getRevision().get();
}
} catch (NoSuchChangeException | AuthException e) {
} catch (ResourceConflictException | NoSuchChangeException | AuthException e) {
rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
} catch (OrmException | PermissionBackendException e) {
} catch (OrmException | PermissionBackendException | IOException e) {
getServletContext().log("Cannot query database", e);
rsp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
return;

12
java/com/google/gerrit/server/ApprovalsUtil.java
View File

@@ -49,10 +49,12 @@ import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.LabelVote;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -109,6 +111,7 @@ public class ApprovalsUtil {
private final IdentifiedUser.GenericFactory userFactory;
private final ApprovalCopier copier;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
@VisibleForTesting
@Inject
@@ -116,11 +119,13 @@ public class ApprovalsUtil {
NotesMigration migration,
IdentifiedUser.GenericFactory userFactory,
ApprovalCopier copier,
PermissionBackend permissionBackend) {
PermissionBackend permissionBackend,
ProjectCache projectCache) {
this.migration = migration;
this.userFactory = userFactory;
this.copier = copier;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
}
/**
@@ -263,8 +268,9 @@ public class ApprovalsUtil {
private boolean canSee(ReviewDb db, ChangeNotes notes, Account.Id accountId) {
try {
IdentifiedUser user = userFactory.create(accountId);
return permissionBackend.user(user).change(notes).database(db).test(ChangePermission.READ);
} catch (PermissionBackendException e) {
return projectCache.checkedGet(notes.getProjectName()).statePermitsRead()
&& permissionBackend.user(user).change(notes).database(db).test(ChangePermission.READ);
} catch (IOException | PermissionBackendException e) {
log.warn(
String.format(
"Failed to check if account %d can see change %d",

3
java/com/google/gerrit/server/change/ChangeInserter.java
View File

@@ -470,7 +470,8 @@ public class ChangeInserter implements InsertChangeOp {
.user(user)
.change(notes)
.database(db)
.test(ChangePermission.READ);
.test(ChangePermission.READ)
&& projectState.statePermitsRead();
} catch (PermissionBackendException e) {
log.warn(
String.format(

11
java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -124,6 +124,7 @@ 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.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
@@ -1469,13 +1470,19 @@ public class ChangeJson {
: withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change()));
}
private boolean isWorldReadable(ChangeData cd) throws OrmException, PermissionBackendException {
private boolean isWorldReadable(ChangeData cd)
throws OrmException, PermissionBackendException, IOException {
try {
permissionBackendForChange(anonymous, cd).check(ChangePermission.READ);
return true;
} catch (AuthException ae) {
return false;
}
ProjectState projectState = projectCache.checkedGet(cd.project());
if (projectState == null) {
log.error("project state for project " + cd.project() + " is null");
return false;
}
return projectState.statePermitsRead();
}
@AutoValue

2
java/com/google/gerrit/server/events/EventBroker.java
View File

@@ -163,7 +163,7 @@ public class EventBroker implements EventDispatcher {
return false;
}
ProjectState pe = projectCache.get(change.getProject());
if (pe == null) {
if (pe == null || !pe.statePermitsRead()) {
return false;
}
ReviewDb db = dbProvider.get();

16
java/com/google/gerrit/server/git/LocalMergeSuperSetComputation.java
View File

@@ -33,6 +33,8 @@ 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.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -87,10 +89,14 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
private final Provider<InternalChangeQuery> queryProvider;
private final Map<QueryKey, List<ChangeData>> queryCache;
private final Map<Branch.NameKey, Optional<RevCommit>> heads;
private final ProjectCache projectCache;
@Inject
LocalMergeSuperSetComputation(
PermissionBackend permissionBackend, Provider<InternalChangeQuery> queryProvider) {
PermissionBackend permissionBackend,
Provider<InternalChangeQuery> queryProvider,
ProjectCache projectCache) {
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
this.queryProvider = queryProvider;
this.queryCache = new HashMap<>();
@@ -171,8 +177,12 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
}
private boolean isVisible(ReviewDb db, ChangeSet changeSet, ChangeData cd, CurrentUser user)
throws PermissionBackendException {
boolean visible = changeSet.ids().contains(cd.getId());
throws PermissionBackendException, IOException {
ProjectState projectState = projectCache.checkedGet(cd.project());
boolean visible =
changeSet.ids().contains(cd.getId())
&& (projectState != null)
&& projectState.statePermitsRead();
if (visible
&& !permissionBackend.user(user).change(cd).database(db).test(ChangePermission.READ)) {
// We thought the change was visible, but it isn't.

26
java/com/google/gerrit/server/git/MergeSuperSet.java
View File

@@ -27,6 +27,8 @@ 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.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -75,6 +77,7 @@ public class MergeSuperSet {
private final DynamicItem<MergeSuperSetComputation> mergeSuperSetComputation;
private final PermissionBackend permissionBackend;
private final Config cfg;
private final ProjectCache projectCache;
private MergeOpRepoManager orm;
private boolean closeOrm;
@@ -86,13 +89,15 @@ public class MergeSuperSet {
Provider<InternalChangeQuery> queryProvider,
Provider<MergeOpRepoManager> repoManagerProvider,
DynamicItem<MergeSuperSetComputation> mergeSuperSetComputation,
PermissionBackend permissionBackend) {
PermissionBackend permissionBackend,
ProjectCache projectCache) {
this.cfg = cfg;
this.changeDataFactory = changeDataFactory;
this.queryProvider = queryProvider;
this.repoManagerProvider = repoManagerProvider;
this.mergeSuperSetComputation = mergeSuperSetComputation;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
}
public static boolean wholeTopicEnabled(Config config) {
@@ -115,9 +120,17 @@ public class MergeSuperSet {
}
ChangeData cd = changeDataFactory.create(db, change.getProject(), change.getId());
ProjectState projectState = projectCache.checkedGet(cd.project());
ChangeSet changeSet =
new ChangeSet(
cd, permissionBackend.user(user).change(cd).database(db).test(ChangePermission.READ));
cd,
projectState != null
&& projectState.statePermitsRead()
&& permissionBackend
.user(user)
.change(cd)
.database(db)
.test(ChangePermission.READ));
if (wholeTopicEnabled(cfg)) {
return completeChangeSetIncludingTopics(db, changeSet, user);
}
@@ -149,7 +162,7 @@ public class MergeSuperSet {
CurrentUser user,
Set<String> topicsSeen,
Set<String> visibleTopicsSeen)
throws OrmException, PermissionBackendException {
throws OrmException, PermissionBackendException, IOException {
List<ChangeData> visibleChanges = new ArrayList<>();
List<ChangeData> nonVisibleChanges = new ArrayList<>();
@@ -208,7 +221,10 @@ public class MergeSuperSet {
}
private boolean canRead(ReviewDb db, CurrentUser user, ChangeData cd)
throws PermissionBackendException {
return permissionBackend.user(user).change(cd).database(db).test(ChangePermission.READ);
throws PermissionBackendException, IOException {
ProjectState projectState = projectCache.checkedGet(cd.project());
return projectState != null
&& projectState.statePermitsRead()
&& permissionBackend.user(user).change(cd).database(db).test(ChangePermission.READ);
}
}

5
java/com/google/gerrit/server/git/VisibleRefFilter.java
View File

@@ -316,7 +316,8 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>();
for (ChangeData cd : changeCache.getChangeData(db.get(), project)) {
ChangeNotes notes = changeNotesFactory.createFromIndexedChange(cd.change());
if (perm.indexedChange(cd, notes).test(ChangePermission.READ)) {
if (projectState.statePermitsRead()
&& perm.indexedChange(cd, notes).test(ChangePermission.READ)) {
visibleChanges.put(cd.getId(), cd.change().getDest());
}
}
@@ -349,7 +350,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
return null;
}
try {
if (perm.change(r.notes()).test(ChangePermission.READ)) {
if (projectState.statePermitsRead() && perm.change(r.notes()).test(ChangePermission.READ)) {
return r.notes();
}
} catch (PermissionBackendException e) {

3
java/com/google/gerrit/server/mail/send/ChangeEmail.java
View File

@@ -408,7 +408,8 @@ public abstract class ChangeEmail extends NotificationEmail {
@Override
protected boolean isVisibleTo(Account.Id to) throws OrmException, PermissionBackendException {
return args.permissionBackend
return projectState.statePermitsRead()
&& args.permissionBackend
.user(args.identifiedUserFactory.create(to))
.change(changeData)
.database(args.db.get())

10
java/com/google/gerrit/server/patch/PatchScriptFactory.java
View File

@@ -42,6 +42,7 @@ import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
@@ -93,6 +94,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
private final ChangeEditUtil editReader;
private final Provider<CurrentUser> userProvider;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
private Optional<ChangeEdit> edit;
private final Change.Id changeId;
@@ -116,6 +118,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
ChangeEditUtil editReader,
Provider<CurrentUser> userProvider,
PermissionBackend permissionBackend,
ProjectCache projectCache,
@Assisted ChangeNotes notes,
@Assisted String fileName,
@Assisted("patchSetA") @Nullable PatchSet.Id patchSetA,
@@ -131,6 +134,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
this.editReader = editReader;
this.userProvider = userProvider;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.fileName = fileName;
this.psa = patchSetA;
@@ -152,6 +156,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
ChangeEditUtil editReader,
Provider<CurrentUser> userProvider,
PermissionBackend permissionBackend,
ProjectCache projectCache,
@Assisted ChangeNotes notes,
@Assisted String fileName,
@Assisted int parentNum,
@@ -167,6 +172,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
this.editReader = editReader;
this.userProvider = userProvider;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.fileName = fileName;
this.psa = null;
@@ -209,6 +215,10 @@ public class PatchScriptFactory implements Callable<PatchScript> {
}
}
if (!projectCache.checkedGet(notes.getProjectName()).statePermitsRead()) {
throw new NoSuchChangeException(changeId);
}
try (Repository git = repoManager.openRepository(notes.getProjectName())) {
bId = toObjectId(psEntityB);
if (parentNum < 0) {

2
java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -130,7 +130,7 @@ class ChangeControl {
if (getChange().isPrivate() && !isPrivateVisible(db, cd)) {
return false;
}
return refControl.isVisible() && getProjectControl().getProject().getState().permitsRead();
return refControl.isVisible();
}
/** Can this user abandon this change? */

22
java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java
View File

@@ -24,8 +24,11 @@ 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.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Provider;
import java.io.IOException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -36,17 +39,20 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
protected final ChangeNotes.Factory notesFactory;
protected final CurrentUser user;
protected final PermissionBackend permissionBackend;
protected final ProjectCache projectCache;
public ChangeIsVisibleToPredicate(
Provider<ReviewDb> db,
ChangeNotes.Factory notesFactory,
CurrentUser user,
PermissionBackend permissionBackend) {
PermissionBackend permissionBackend,
ProjectCache projectCache) {
super(ChangeQueryBuilder.FIELD_VISIBLETO, IndexUtils.describe(user));
this.db = db;
this.notesFactory = notesFactory;
this.user = user;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
}
@Override
@@ -60,6 +66,20 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
}
ChangeNotes notes = notesFactory.createFromIndexedChange(change);
try {
ProjectState projectState = projectCache.checkedGet(cd.project());
if (projectState == null) {
logger.info("No such project: {}", cd.project());
return false;
}
if (!projectState.statePermitsRead()) {
return false;
}
} catch (IOException e) {
throw new OrmException("unable to read project state", e);
}
boolean visible;
try {
visible =

3
java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

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

9
java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
View File

@@ -34,6 +34,7 @@ import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
import com.google.gerrit.server.index.change.IndexedChangeQuery;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.ProjectCache;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.ArrayList;
@@ -63,6 +64,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
private final ChangeNotes.Factory notesFactory;
private final DynamicMap<ChangeAttributeFactory> attributeFactories;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
static {
// It is assumed that basic rewrites do not touch visibleto predicates.
@@ -82,7 +84,8 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
Provider<ReviewDb> db,
ChangeNotes.Factory notesFactory,
DynamicMap<ChangeAttributeFactory> attributeFactories,
PermissionBackend permissionBackend) {
PermissionBackend permissionBackend,
ProjectCache projectCache) {
super(
metricMaker,
ChangeSchemaDefinitions.INSTANCE,
@@ -96,6 +99,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
this.notesFactory = notesFactory;
this.attributeFactories = attributeFactories;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
}
@Override
@@ -138,7 +142,8 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
protected Predicate<ChangeData> enforceVisibility(Predicate<ChangeData> pred) {
return new AndChangeSource(
pred,
new ChangeIsVisibleToPredicate(db, notesFactory, userProvider.get(), permissionBackend),
new ChangeIsVisibleToPredicate(
db, notesFactory, userProvider.get(), permissionBackend, projectCache),
start);
}
}

8
java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
View File

@@ -30,6 +30,7 @@ import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Provider;
import java.io.IOException;
public class EqualsLabelPredicate extends ChangeIndexPredicate {
protected final ProjectCache projectCache;
@@ -123,8 +124,11 @@ public class EqualsLabelPredicate extends ChangeIndexPredicate {
try {
PermissionBackend.ForChange perm =
permissionBackend.user(reviewer).database(dbProvider).change(cd);
return perm.test(ChangePermission.READ);
} catch (PermissionBackendException e) {
ProjectState projectState = projectCache.checkedGet(cd.project());
return projectState != null
&& projectState.statePermitsRead()
&& perm.test(ChangePermission.READ);
} catch (PermissionBackendException | IOException e) {
return false;
}
}

8
java/com/google/gerrit/server/restapi/change/ChangesCollection.java
View File

@@ -135,13 +135,17 @@ public class ChangesCollection
return createChange;
}
private boolean canRead(ChangeNotes notes) throws PermissionBackendException {
private boolean canRead(ChangeNotes notes) throws PermissionBackendException, IOException {
try {
permissionBackend.user(user).change(notes).database(db).check(ChangePermission.READ);
return true;
} catch (AuthException e) {
return false;
}
ProjectState projectState = projectCache.checkedGet(notes.getProjectName());
if (projectState == null) {
return false;
}
return projectState.statePermitsRead();
}
private void checkProjectStatePermitsRead(Project.NameKey project)

2
java/com/google/gerrit/server/restapi/change/GetRelated.java
View File

@@ -101,7 +101,7 @@ public class GetRelated implements RestReadView<RevisionResource> {
reloadChangeIfStale(cds, basePs);
for (RelatedChangesSorter.PatchSetData d : sorter.sort(cds, basePs, rsrc.getUser())) {
for (RelatedChangesSorter.PatchSetData d : sorter.sort(cds, basePs)) {
PatchSet ps = d.patchSet();
RevCommit commit;
if (isEdit && ps.getId().equals(basePs.getId())) {

52
java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java
View File

@@ -34,6 +34,8 @@ import com.google.gerrit.server.git.GitRepositoryManager;
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.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -61,25 +63,30 @@ class RelatedChangesSorter {
private final GitRepositoryManager repoManager;
private final PermissionBackend permissionBackend;
private final Provider<ReviewDb> dbProvider;
private final ProjectCache projectCache;
private final Provider<CurrentUser> currentUserProvider;
@Inject
RelatedChangesSorter(
GitRepositoryManager repoManager,
PermissionBackend permissionBackend,
Provider<ReviewDb> dbProvider) {
Provider<ReviewDb> dbProvider,
ProjectCache projectCache,
Provider<CurrentUser> currentUserProvider) {
this.repoManager = repoManager;
this.permissionBackend = permissionBackend;
this.dbProvider = dbProvider;
this.projectCache = projectCache;
this.currentUserProvider = currentUserProvider;
}
public List<PatchSetData> sort(List<ChangeData> in, PatchSet startPs, CurrentUser user)
public List<PatchSetData> sort(List<ChangeData> in, PatchSet startPs)
throws OrmException, IOException, PermissionBackendException {
checkArgument(!in.isEmpty(), "Input may not be empty");
// Map of all patch sets, keyed by commit SHA-1.
Map<String, PatchSetData> byId = collectById(in);
PatchSetData start = byId.get(startPs.getRevision().get());
checkArgument(start != null, "%s not found in %s", startPs, in);
PermissionBackend.WithUser perm = permissionBackend.user(user).database(dbProvider);
// Map of patch set -> immediate parent.
ListMultimap<PatchSetData, PatchSetData> parents =
@@ -106,9 +113,9 @@ class RelatedChangesSorter {
}
}
Collection<PatchSetData> ancestors = walkAncestors(perm, parents, start);
Collection<PatchSetData> ancestors = walkAncestors(parents, start);
List<PatchSetData> descendants =
walkDescendants(perm, children, start, otherPatchSetsOfStart, ancestors);
walkDescendants(children, start, otherPatchSetsOfStart, ancestors);
List<PatchSetData> result = new ArrayList<>(ancestors.size() + descendants.size() - 1);
result.addAll(Lists.reverse(descendants));
result.addAll(ancestors);
@@ -140,17 +147,15 @@ class RelatedChangesSorter {
return result;
}
private static Collection<PatchSetData> walkAncestors(
PermissionBackend.WithUser perm,
ListMultimap<PatchSetData, PatchSetData> parents,
PatchSetData start)
throws PermissionBackendException {
private Collection<PatchSetData> walkAncestors(
ListMultimap<PatchSetData, PatchSetData> parents, PatchSetData start)
throws PermissionBackendException, IOException {
LinkedHashSet<PatchSetData> result = new LinkedHashSet<>();
Deque<PatchSetData> pending = new ArrayDeque<>();
pending.add(start);
while (!pending.isEmpty()) {
PatchSetData psd = pending.remove();
if (result.contains(psd) || !isVisible(psd, perm)) {
if (result.contains(psd) || !isVisible(psd)) {
continue;
}
result.add(psd);
@@ -159,26 +164,24 @@ class RelatedChangesSorter {
return result;
}
private static List<PatchSetData> walkDescendants(
PermissionBackend.WithUser perm,
private List<PatchSetData> walkDescendants(
ListMultimap<PatchSetData, PatchSetData> children,
PatchSetData start,
List<PatchSetData> otherPatchSetsOfStart,
Iterable<PatchSetData> ancestors)
throws PermissionBackendException {
throws PermissionBackendException, IOException {
Set<Change.Id> alreadyEmittedChanges = new HashSet<>();
addAllChangeIds(alreadyEmittedChanges, ancestors);
// Prefer descendants found by following the original patch set passed in.
List<PatchSetData> result =
walkDescendentsImpl(perm, alreadyEmittedChanges, children, ImmutableList.of(start));
walkDescendentsImpl(alreadyEmittedChanges, children, ImmutableList.of(start));
addAllChangeIds(alreadyEmittedChanges, result);
// Then, go back and add new indirect descendants found by following any
// other patch sets of start. These show up after all direct descendants,
// because we wouldn't know where in the walk to insert them.
result.addAll(
walkDescendentsImpl(perm, alreadyEmittedChanges, children, otherPatchSetsOfStart));
result.addAll(walkDescendentsImpl(alreadyEmittedChanges, children, otherPatchSetsOfStart));
return result;
}
@@ -189,12 +192,11 @@ class RelatedChangesSorter {
}
}
private static List<PatchSetData> walkDescendentsImpl(
PermissionBackend.WithUser perm,
private List<PatchSetData> walkDescendentsImpl(
Set<Change.Id> alreadyEmittedChanges,
ListMultimap<PatchSetData, PatchSetData> children,
List<PatchSetData> start)
throws PermissionBackendException {
throws PermissionBackendException, IOException {
if (start.isEmpty()) {
return ImmutableList.of();
}
@@ -205,7 +207,7 @@ class RelatedChangesSorter {
pending.addAll(start);
while (!pending.isEmpty()) {
PatchSetData psd = pending.remove();
if (seen.contains(psd) || !isVisible(psd, perm)) {
if (seen.contains(psd) || !isVisible(psd)) {
continue;
}
seen.add(psd);
@@ -236,14 +238,16 @@ class RelatedChangesSorter {
return result;
}
private static boolean isVisible(PatchSetData psd, PermissionBackend.WithUser perm)
throws PermissionBackendException {
private boolean isVisible(PatchSetData psd) throws PermissionBackendException, IOException {
PermissionBackend.WithUser perm =
permissionBackend.user(currentUserProvider).database(dbProvider);
try {
perm.change(psd.data()).check(ChangePermission.READ);
return true;
} catch (AuthException e) {
return false;
}
ProjectState state = projectCache.checkedGet(psd.data().project());
return state != null && state.statePermitsRead();
}
@AutoValue

10
java/com/google/gerrit/server/restapi/change/Revisions.java
View File

@@ -33,6 +33,7 @@ import com.google.gerrit.server.edit.ChangeEditUtil;
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.ProjectCache;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -51,6 +52,7 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
private final ChangeEditUtil editUtil;
private final PatchSetUtil psUtil;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
@Inject
Revisions(
@@ -58,12 +60,14 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
Provider<ReviewDb> dbProvider,
ChangeEditUtil editUtil,
PatchSetUtil psUtil,
PermissionBackend permissionBackend) {
PermissionBackend permissionBackend,
ProjectCache projectCache) {
this.views = views;
this.dbProvider = dbProvider;
this.editUtil = editUtil;
this.psUtil = psUtil;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
}
@Override
@@ -105,14 +109,14 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
}
}
private boolean visible(ChangeResource change) throws PermissionBackendException {
private boolean visible(ChangeResource change) throws PermissionBackendException, IOException {
try {
permissionBackend
.user(change.getUser())
.change(change.getNotes())
.database(dbProvider)
.check(ChangePermission.READ);
return true;
return projectCache.checkedGet(change.getProject()).statePermitsRead();
} catch (AuthException e) {
return false;
}

5
java/com/google/gerrit/sshd/ChangeArgumentParser.java
View File

@@ -92,11 +92,12 @@ public class ChangeArgumentParser {
if (!changes.containsKey(notes.getChangeId())
&& inProject(projectState, notes.getProjectName())
&& (canMaintainServer
|| permissionBackend
|| (permissionBackend
.user(currentUser)
.change(notes)
.database(db)
.test(ChangePermission.READ))) {
.test(ChangePermission.READ)
&& projectState.statePermitsRead()))) {
toAdd.add(notes);
}
}