Don't use #test(Permission) when #check(permission) is expected

By design, the result of #test(permission) should only considered as a
hint and they might be more permissive than reality. This method should
be used by some cases, e.g. rendering UI, where the unreliable results
are acceptable.

However, there are lots of #test usages which should have been #check.
This is not a problem for DefaultPermissionBackend because both #check
and #test go the same method in the end. But it is a problem for other
permission backend implementations because #test and #check could be
different there.

This CL replaces those #test with #check. It should be a no-op and
should't change existing behaviors.

Change-Id: Ie2cc7adb81ccde4591daff35bacca74f97bc3e25
This commit is contained in:
Changcheng Xiao
2018-07-04 16:44:57 +02:00
parent 5a92e22f4c
commit 2530d60b49
25 changed files with 313 additions and 132 deletions

View File

@@ -127,8 +127,12 @@ public abstract class ProjectAccessHandler<T> extends Handler<T> {
replace(config, toDelete, section); replace(config, toDelete, section);
} else if (AccessSection.isValid(name)) { } else if (AccessSection.isValid(name)) {
if (checkIfOwner && !forProject.ref(name).test(RefPermission.WRITE_CONFIG)) { if (checkIfOwner) {
continue; try {
forProject.ref(name).check(RefPermission.WRITE_CONFIG);
} catch (AuthException e) {
continue;
}
} }
RefPattern.validate(name); RefPattern.validate(name);
@@ -143,8 +147,15 @@ public abstract class ProjectAccessHandler<T> extends Handler<T> {
config.remove(config.getAccessSection(name)); config.remove(config.getAccessSection(name));
} }
} else if (!checkIfOwner || forProject.ref(name).test(RefPermission.WRITE_CONFIG)) { } else if (!checkIfOwner) {
config.remove(config.getAccessSection(name)); config.remove(config.getAccessSection(name));
} else {
try {
forProject.ref(name).check(RefPermission.WRITE_CONFIG);
config.remove(config.getAccessSection(name));
} catch (AuthException e) {
// Do nothing.
}
} }
} }

View File

@@ -263,12 +263,17 @@ 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 {
return projectCache.checkedGet(notes.getProjectName()).statePermitsRead() if (!projectCache.checkedGet(notes.getProjectName()).statePermitsRead()) {
&& permissionBackend return false;
.absentUser(accountId) }
.change(notes) permissionBackend
.database(db) .absentUser(accountId)
.test(ChangePermission.READ); .change(notes)
.database(db)
.check(ChangePermission.READ);
return true;
} catch (AuthException e) {
return false;
} catch (IOException | PermissionBackendException e) { } catch (IOException | PermissionBackendException e) {
logger.atWarning().withCause(e).log( logger.atWarning().withCause(e).log(
"Failed to check if account %d can see change %d", "Failed to check if account %d can see change %d",

View File

@@ -29,6 +29,7 @@ import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
@@ -459,18 +460,24 @@ public class ChangeInserter implements InsertChangeOp {
.stream() .stream()
.filter( .filter(
accountId -> { accountId -> {
if (!projectState.statePermitsRead()) {
return false;
}
try { try {
return permissionBackend permissionBackend
.absentUser(accountId) .absentUser(accountId)
.change(notes) .change(notes)
.database(db) .database(db)
.test(ChangePermission.READ) .check(ChangePermission.READ);
&& projectState.statePermitsRead(); return true;
} catch (PermissionBackendException e) { } catch (PermissionBackendException e) {
logger.atWarning().withCause(e).log( logger.atWarning().withCause(e).log(
"Failed to check if account %d can see change %d", "Failed to check if account %d can see change %d",
accountId.get(), notes.getChangeId().get()); accountId.get(), notes.getChangeId().get());
return false; return false;
} catch (AuthException e) {
return false;
} }
}) })
.collect(toSet()); .collect(toSet());

View File

@@ -171,21 +171,31 @@ public class EventBroker implements EventDispatcher {
return false; return false;
} }
ReviewDb db = dbProvider.get(); ReviewDb db = dbProvider.get();
return permissionBackend try {
.user(user) permissionBackend
.change(notesFactory.createChecked(db, change)) .user(user)
.database(db) .change(notesFactory.createChecked(db, change))
.test(ChangePermission.READ); .database(db)
.check(ChangePermission.READ);
return true;
} catch (AuthException e) {
return false;
}
} }
protected boolean isVisibleTo(Branch.NameKey branchName, CurrentUser user) protected boolean isVisibleTo(Branch.NameKey branchName, CurrentUser user)
throws PermissionBackendException { throws PermissionBackendException {
ProjectState pe = projectCache.get(branchName.getParentKey()); ProjectState pe = projectCache.get(branchName.getParentKey());
if (pe == null) { if (pe == null || !pe.statePermitsRead()) {
return false;
}
try {
permissionBackend.user(user).ref(branchName).check(RefPermission.READ);
return true;
} catch (AuthException e) {
return false; return false;
} }
return pe.statePermitsRead()
&& permissionBackend.user(user).ref(branchName).test(RefPermission.READ);
} }
protected boolean isVisibleTo(Event event, CurrentUser user) protected boolean isVisibleTo(Event event, CurrentUser user)

View File

@@ -2528,11 +2528,23 @@ class ReceiveCommits {
if (magicBranch != null if (magicBranch != null
&& (magicBranch.workInProgress || magicBranch.ready) && (magicBranch.workInProgress || magicBranch.ready)
&& magicBranch.workInProgress != change.isWorkInProgress() && magicBranch.workInProgress != change.isWorkInProgress()
&& (!user.getAccountId().equals(change.getOwner()) && !user.getAccountId().equals(change.getOwner())) {
&& !permissions.test(ProjectPermission.WRITE_CONFIG) boolean hasWriteConfigPermission = false;
&& !permissionBackend.user(user).test(GlobalPermission.ADMINISTRATE_SERVER))) { try {
reject(inputCommand, ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP); permissions.check(ProjectPermission.WRITE_CONFIG);
return false; hasWriteConfigPermission = true;
} catch (AuthException e) {
// Do nothing.
}
if (!hasWriteConfigPermission) {
try {
permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
} catch (AuthException e1) {
reject(inputCommand, ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP);
return false;
}
}
} }
if (magicBranch != null && (magicBranch.edit || magicBranch.draft)) { if (magicBranch != null && (magicBranch.edit || magicBranch.draft)) {

View File

@@ -403,12 +403,19 @@ public abstract class ChangeEmail extends NotificationEmail {
@Override @Override
protected boolean isVisibleTo(Account.Id to) throws PermissionBackendException { protected boolean isVisibleTo(Account.Id to) throws PermissionBackendException {
return projectState.statePermitsRead() if (!projectState.statePermitsRead()) {
&& args.permissionBackend return false;
.absentUser(to) }
.change(changeData) try {
.database(args.db) args.permissionBackend
.test(ChangePermission.READ); .absentUser(to)
.change(changeData)
.database(args.db)
.check(ChangePermission.READ);
return true;
} catch (AuthException e) {
return false;
}
} }
/** 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

@@ -299,9 +299,14 @@ class DefaultRefFilter {
Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>(); Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>();
for (ChangeData cd : changeCache.getChangeData(db.get(), project)) { for (ChangeData cd : changeCache.getChangeData(db.get(), project)) {
ChangeNotes notes = changeNotesFactory.createFromIndexedChange(cd.change()); ChangeNotes notes = changeNotesFactory.createFromIndexedChange(cd.change());
if (projectState.statePermitsRead() if (!projectState.statePermitsRead()) {
&& permissionBackendForProject.indexedChange(cd, notes).test(ChangePermission.READ)) { continue;
}
try {
permissionBackendForProject.indexedChange(cd, notes).check(ChangePermission.READ);
visibleChanges.put(cd.getId(), cd.change().getDest()); visibleChanges.put(cd.getId(), cd.change().getDest());
} catch (AuthException e) {
// Do nothing.
} }
} }
return visibleChanges; return visibleChanges;
@@ -334,11 +339,16 @@ class DefaultRefFilter {
"Failed to load change %s in %s", r.id(), projectState.getName()); "Failed to load change %s in %s", r.id(), projectState.getName());
return null; return null;
} }
if (!projectState.statePermitsRead()) {
return null;
}
try { try {
if (projectState.statePermitsRead() permissionBackendForProject.change(r.notes()).check(ChangePermission.READ);
&& permissionBackendForProject.change(r.notes()).test(ChangePermission.READ)) { return r.notes();
return r.notes(); } catch (AuthException e) {
} // Skip.
} catch (PermissionBackendException e) { } catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log( logger.atSevere().withCause(e).log(
"Failed to check permission for %s in %s", r.id(), projectState.getName()); "Failed to check permission for %s in %s", r.id(), projectState.getName());

View File

@@ -19,6 +19,7 @@ import com.google.common.collect.Lists;
import com.google.common.flogger.FluentLogger; import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints; import com.google.common.primitives.Ints;
import com.google.gerrit.common.errors.NotSignedInException; import com.google.gerrit.common.errors.NotSignedInException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.index.Index; import com.google.gerrit.index.Index;
import com.google.gerrit.index.Schema; import com.google.gerrit.index.Schema;
import com.google.gerrit.index.query.LimitPredicate; import com.google.gerrit.index.query.LimitPredicate;
@@ -120,12 +121,17 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
public Predicate<AccountState> cansee(String change) public Predicate<AccountState> cansee(String change)
throws QueryParseException, OrmException, PermissionBackendException { throws QueryParseException, OrmException, PermissionBackendException {
ChangeNotes changeNotes = args.changeFinder.findOne(change); ChangeNotes changeNotes = args.changeFinder.findOne(change);
if (changeNotes == null if (changeNotes == null) {
|| !args.permissionBackend throw error(String.format("change %s not found", change));
.user(args.getUser()) }
.database(args.db)
.change(changeNotes) try {
.test(ChangePermission.READ)) { args.permissionBackend
.user(args.getUser())
.database(args.db)
.change(changeNotes)
.check(ChangePermission.READ);
} catch (AuthException e) {
throw error(String.format("change %s not found", change)); throw error(String.format("change %s not found", change));
} }
@@ -218,7 +224,12 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
} }
private boolean canSeeSecondaryEmails() throws PermissionBackendException, QueryParseException { private boolean canSeeSecondaryEmails() throws PermissionBackendException, QueryParseException {
return args.permissionBackend.user(args.getUser()).test(GlobalPermission.MODIFY_ACCOUNT); try {
args.permissionBackend.user(args.getUser()).check(GlobalPermission.MODIFY_ACCOUNT);
return true;
} catch (AuthException e) {
return false;
}
} }
private boolean checkedCanSeeSecondaryEmails() { private boolean checkedCanSeeSecondaryEmails() {

View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.query.account; package com.google.gerrit.server.query.account;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.index.query.PostFilterPredicate; import com.google.gerrit.index.query.PostFilterPredicate;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.AccountState;
@@ -40,13 +41,16 @@ public class CanSeeChangePredicate extends PostFilterPredicate<AccountState> {
@Override @Override
public boolean match(AccountState accountState) throws OrmException { public boolean match(AccountState accountState) throws OrmException {
try { try {
return permissionBackend permissionBackend
.absentUser(accountState.getAccount().getId()) .absentUser(accountState.getAccount().getId())
.database(db) .database(db)
.change(changeNotes) .change(changeNotes)
.test(ChangePermission.READ); .check(ChangePermission.READ);
return true;
} catch (PermissionBackendException e) { } catch (PermissionBackendException e) {
throw new OrmException("Failed to check if account can see change", e); throw new OrmException("Failed to check if account can see change", e);
} catch (AuthException e) {
return false;
} }
} }

View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.query.change; package com.google.gerrit.server.query.change;
import com.google.common.flogger.FluentLogger; import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.index.query.IsVisibleToPredicate; import com.google.gerrit.index.query.IsVisibleToPredicate;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -83,13 +84,12 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
throw new OrmException("unable to read project state", e); throw new OrmException("unable to read project state", e);
} }
boolean visible;
PermissionBackend.WithUser withUser = PermissionBackend.WithUser withUser =
user.isIdentifiedUser() user.isIdentifiedUser()
? permissionBackend.absentUser(user.getAccountId()) ? permissionBackend.absentUser(user.getAccountId())
: permissionBackend.user(anonymousUserProvider.get()); : permissionBackend.user(anonymousUserProvider.get());
try { try {
visible = withUser.indexedChange(cd, notes).database(db).test(ChangePermission.READ); withUser.indexedChange(cd, notes).database(db).check(ChangePermission.READ);
} catch (PermissionBackendException e) { } catch (PermissionBackendException e) {
Throwable cause = e.getCause(); Throwable cause = e.getCause();
if (cause instanceof RepositoryNotFoundException) { if (cause instanceof RepositoryNotFoundException) {
@@ -98,12 +98,12 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
return false; return false;
} }
throw new OrmException("unable to check permissions on change " + cd.getId(), e); throw new OrmException("unable to check permissions on change " + cd.getId(), e);
} catch (AuthException e) {
return false;
} }
if (visible) {
cd.cacheVisibleTo(user); cd.cacheVisibleTo(user);
return true; return true;
}
return false;
} }
@Override @Override

View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.query.change;
import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
@@ -125,10 +126,13 @@ public class EqualsLabelPredicate extends ChangeIndexPredicate {
PermissionBackend.ForChange perm = PermissionBackend.ForChange perm =
permissionBackend.absentUser(approver).database(dbProvider).change(cd); permissionBackend.absentUser(approver).database(dbProvider).change(cd);
ProjectState projectState = projectCache.checkedGet(cd.project()); ProjectState projectState = projectCache.checkedGet(cd.project());
return projectState != null if (projectState == null || !projectState.statePermitsRead()) {
&& projectState.statePermitsRead() return false;
&& perm.test(ChangePermission.READ); }
} catch (PermissionBackendException | IOException e) {
perm.check(ChangePermission.READ);
return true;
} catch (PermissionBackendException | IOException | AuthException e) {
return false; return false;
} }
} }

View File

@@ -71,10 +71,12 @@ public class Capabilities implements ChildCollection<AccountResource, AccountRes
} }
GlobalOrPluginPermission perm = parse(id); GlobalOrPluginPermission perm = parse(id);
if (permissionBackend.user(target).test(perm)) { try {
permissionBackend.user(target).check(perm);
return new AccountResource.Capability(target, globalOrPluginPermissionName(perm)); return new AccountResource.Capability(target, globalOrPluginPermissionName(perm));
} catch (AuthException e) {
throw new ResourceNotFoundException(id);
} }
throw new ResourceNotFoundException(id);
} }
private GlobalOrPluginPermission parse(IdString id) throws ResourceNotFoundException { private GlobalOrPluginPermission parse(IdString id) throws ResourceNotFoundException {

View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.account;
import com.google.common.collect.Sets; import com.google.common.collect.Sets;
import com.google.gerrit.extensions.common.AccountDetailInfo; import com.google.gerrit.extensions.common.AccountDetailInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.CurrentUser;
@@ -56,12 +57,19 @@ public class GetDetail implements RestReadView<AccountResource> {
AccountDetailInfo info = new AccountDetailInfo(a.getId().get()); AccountDetailInfo info = new AccountDetailInfo(a.getId().get());
info.registeredOn = a.getRegisteredOn(); info.registeredOn = a.getRegisteredOn();
info.inactive = !a.isActive() ? true : null; info.inactive = !a.isActive() ? true : null;
Set<FillOptions> fillOptions = Set<FillOptions> fillOptions;
self.get().hasSameAccountId(rsrc.getUser()) if (self.get().hasSameAccountId(rsrc.getUser())) {
|| permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT) fillOptions = EnumSet.allOf(FillOptions.class);
? EnumSet.allOf(FillOptions.class) } else {
: Sets.difference( try {
permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
fillOptions = EnumSet.allOf(FillOptions.class);
} catch (AuthException e) {
fillOptions =
Sets.difference(
EnumSet.allOf(FillOptions.class), EnumSet.of(FillOptions.SECONDARY_EMAILS)); EnumSet.allOf(FillOptions.class), EnumSet.of(FillOptions.SECONDARY_EMAILS));
}
}
directory.fillAccountInfo(Collections.singleton(info), fillOptions); directory.fillAccountInfo(Collections.singleton(info), fillOptions);
return info; return info;
} }

View File

@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.client.ListAccountsOption; import com.google.gerrit.extensions.client.ListAccountsOption;
import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.AccountVisibility; import com.google.gerrit.extensions.common.AccountVisibility;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
@@ -171,9 +172,15 @@ public class QueryAccounts implements RestReadView<TopLevelResource> {
fillOptions.addAll(AccountLoader.DETAILED_OPTIONS); fillOptions.addAll(AccountLoader.DETAILED_OPTIONS);
fillOptions.add(FillOptions.EMAIL); fillOptions.add(FillOptions.EMAIL);
if (modifyAccountCapabilityChecked if (modifyAccountCapabilityChecked) {
|| permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
fillOptions.add(FillOptions.SECONDARY_EMAILS); fillOptions.add(FillOptions.SECONDARY_EMAILS);
} else {
try {
permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
fillOptions.add(FillOptions.SECONDARY_EMAILS);
} catch (AuthException e) {
// Do nothing.
}
} }
} }
accountLoader = accountLoaderFactory.create(fillOptions); accountLoader = accountLoaderFactory.create(fillOptions);

View File

@@ -352,14 +352,17 @@ public class PostReviewers
NotifyHandling notify, NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify) ListMultimap<RecipientType, Account.Id> accountsToNotify)
throws PermissionBackendException { throws PermissionBackendException {
if (!permissionBackend try {
.user(anonymousProvider.get()) permissionBackend
.change(rsrc.getNotes()) .user(anonymousProvider.get())
.database(dbProvider) .change(rsrc.getNotes())
.test(ChangePermission.READ)) { .database(dbProvider)
.check(ChangePermission.READ);
} catch (AuthException e) {
return fail( return fail(
reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer)); reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
} }
if (!migration.readChanges()) { if (!migration.readChanges()) {
// addByEmail depends on NoteDb. // addByEmail depends on NoteDb.
return fail( return fail(

View File

@@ -22,6 +22,7 @@ import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.api.changes.ReviewerInfo; import com.google.gerrit.extensions.api.changes.ReviewerInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.PatchSetApproval;
@@ -135,10 +136,15 @@ public class ReviewerJson {
for (SubmitRecord.Label label : rec.labels) { for (SubmitRecord.Label label : rec.labels) {
String name = label.label; String name = label.label;
LabelType type = labelTypes.byLabel(name); LabelType type = labelTypes.byLabel(name);
if (!out.approvals.containsKey(name) if (out.approvals.containsKey(name) || type == null) {
&& type != null continue;
&& perm.test(new LabelPermission(type))) { }
try {
perm.check(new LabelPermission(type));
out.approvals.put(name, formatValue((short) 0)); out.approvals.put(name, formatValue((short) 0));
} catch (AuthException e) {
// Do nothing.
} }
} }
} }

View File

@@ -27,6 +27,7 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.extensions.common.GroupBaseInfo; import com.google.gerrit.extensions.common.GroupBaseInfo;
import com.google.gerrit.extensions.common.SuggestedReviewerInfo; import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.index.IndexConfig; import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.QueryOptions; import com.google.gerrit.index.QueryOptions;
@@ -300,10 +301,14 @@ public class ReviewersUtil {
private List<SuggestedReviewerInfo> loadAccounts(List<Account.Id> accountIds) private List<SuggestedReviewerInfo> loadAccounts(List<Account.Id> accountIds)
throws PermissionBackendException { throws PermissionBackendException {
Set<FillOptions> fillOptions = Set<FillOptions> fillOptions;
permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT) try {
? EnumSet.of(FillOptions.SECONDARY_EMAILS) permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
: EnumSet.noneOf(FillOptions.class); fillOptions = EnumSet.of(FillOptions.SECONDARY_EMAILS);
} catch (AuthException e) {
fillOptions = EnumSet.noneOf(FillOptions.class);
}
fillOptions.addAll(AccountLoader.DETAILED_OPTIONS); fillOptions.addAll(AccountLoader.DETAILED_OPTIONS);
AccountLoader accountLoader = accountLoaderFactory.create(fillOptions); AccountLoader accountLoader = accountLoaderFactory.create(fillOptions);

View File

@@ -65,16 +65,28 @@ public class SetReadyForReview extends RetryingRestModifyView<ChangeResource, In
protected Response<?> applyImpl( protected Response<?> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input) BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input)
throws RestApiException, UpdateException, PermissionBackendException { throws RestApiException, UpdateException, PermissionBackendException {
Change change = rsrc.getChange(); if (!rsrc.isUserOwner()) {
if (!rsrc.isUserOwner() boolean hasAdministrateServerPermission = false;
&& !permissionBackend.currentUser().test(GlobalPermission.ADMINISTRATE_SERVER) try {
&& !permissionBackend permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
.currentUser() hasAdministrateServerPermission = true;
.project(rsrc.getProject()) } catch (AuthException e) {
.test(ProjectPermission.WRITE_CONFIG)) { // Skip.
throw new AuthException("not allowed to set ready for review"); }
if (!hasAdministrateServerPermission) {
try {
permissionBackend
.currentUser()
.project(rsrc.getProject())
.check(ProjectPermission.WRITE_CONFIG);
} catch (AuthException exp) {
throw new AuthException("not allowed to set ready for review");
}
}
} }
Change change = rsrc.getChange();
if (change.getStatus() != Status.NEW) { if (change.getStatus() != Status.NEW) {
throw new ResourceConflictException("change is " + ChangeUtil.status(change)); throw new ResourceConflictException("change is " + ChangeUtil.status(change));
} }

View File

@@ -65,17 +65,28 @@ public class SetWorkInProgress extends RetryingRestModifyView<ChangeResource, In
protected Response<?> applyImpl( protected Response<?> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input) BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input)
throws RestApiException, UpdateException, PermissionBackendException { throws RestApiException, UpdateException, PermissionBackendException {
Change change = rsrc.getChange(); if (!rsrc.isUserOwner()) {
boolean hasAdministrateServerPermission = false;
try {
permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
hasAdministrateServerPermission = true;
} catch (AuthException e) {
// Skip.
}
if (!rsrc.isUserOwner() if (!hasAdministrateServerPermission) {
&& !permissionBackend.currentUser().test(GlobalPermission.ADMINISTRATE_SERVER) try {
&& !permissionBackend permissionBackend
.currentUser() .currentUser()
.project(rsrc.getProject()) .project(rsrc.getProject())
.test(ProjectPermission.WRITE_CONFIG)) { .check(ProjectPermission.WRITE_CONFIG);
throw new AuthException("not allowed to set work in progress"); } catch (AuthException exp) {
throw new AuthException("not allowed to set work in progress");
}
}
} }
Change change = rsrc.getChange();
if (change.getStatus() != Status.NEW) { if (change.getStatus() != Status.NEW) {
throw new ResourceConflictException("change is " + ChangeUtil.status(change)); throw new ResourceConflictException("change is " + ChangeUtil.status(change));
} }

View File

@@ -237,11 +237,15 @@ public class GetAccess implements RestReadView<ProjectResource> {
} }
} }
if (info.ownerOf.isEmpty() if (info.ownerOf.isEmpty()) {
&& permissionBackend.currentUser().test(GlobalPermission.ADMINISTRATE_SERVER)) { try {
// Special case: If the section list is empty, this project has no current permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
// access control information. Fall back to site administrators. // Special case: If the section list is empty, this project has no current
info.ownerOf.add(AccessSection.ALL); // access control information. Fall back to site administrators.
info.ownerOf.add(AccessSection.ALL);
} catch (AuthException e) {
// Do nothing.
}
} }
if (config.getRevision() != null) { if (config.getRevision() != null) {

View File

@@ -24,6 +24,7 @@ import com.google.gerrit.extensions.api.projects.ProjectApi.ListRefsRequest;
import com.google.gerrit.extensions.common.ActionInfo; import com.google.gerrit.extensions.common.ActionInfo;
import com.google.gerrit.extensions.common.WebLinkInfo; import com.google.gerrit.extensions.common.WebLinkInfo;
import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.extensions.restapi.RestReadView;
@@ -185,9 +186,13 @@ public class ListBranches implements RestReadView<ProjectResource> {
// showing the resolved value, show the name it references. // showing the resolved value, show the name it references.
// //
String target = ref.getTarget().getName(); String target = ref.getTarget().getName();
if (!perm.ref(target).test(RefPermission.READ)) {
try {
perm.ref(target).check(RefPermission.READ);
} catch (AuthException e) {
continue; continue;
} }
if (target.startsWith(Constants.R_HEADS)) { if (target.startsWith(Constants.R_HEADS)) {
target = target.substring(Constants.R_HEADS.length()); target = target.substring(Constants.R_HEADS.length());
} }
@@ -212,10 +217,13 @@ public class ListBranches implements RestReadView<ProjectResource> {
continue; continue;
} }
if (perm.ref(ref.getName()).test(RefPermission.READ)) { try {
perm.ref(ref.getName()).check(RefPermission.READ);
branches.add( branches.add(
createBranchInfo( createBranchInfo(
perm.ref(ref.getName()), ref, rsrc.getProjectState(), rsrc.getUser(), targets)); perm.ref(ref.getName()), ref, rsrc.getProjectState(), rsrc.getUser(), targets));
} catch (AuthException e) {
// Do nothing.
} }
} }
Collections.sort(branches, new BranchComparator()); Collections.sort(branches, new BranchComparator());

View File

@@ -18,6 +18,7 @@ import static com.google.gerrit.reviewdb.client.RefNames.REFS_DASHBOARDS;
import com.google.common.flogger.FluentLogger; import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.api.projects.DashboardInfo; import com.google.gerrit.extensions.api.projects.DashboardInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project;
@@ -104,8 +105,15 @@ public class ListDashboards implements RestReadView<ProjectResource> {
RevWalk rw = new RevWalk(git)) { RevWalk rw = new RevWalk(git)) {
List<DashboardInfo> all = new ArrayList<>(); List<DashboardInfo> all = new ArrayList<>();
for (Ref ref : git.getRefDatabase().getRefsByPrefix(REFS_DASHBOARDS)) { for (Ref ref : git.getRefDatabase().getRefsByPrefix(REFS_DASHBOARDS)) {
if (perm.ref(ref.getName()).test(RefPermission.READ) && state.statePermitsRead()) { if (!state.statePermitsRead()) {
continue;
}
try {
perm.ref(ref.getName()).check(RefPermission.READ);
all.addAll(scanDashboards(state.getProject(), git, rw, ref, project, setDefault)); all.addAll(scanDashboards(state.getProject(), git, rw, ref, project, setDefault));
} catch (AuthException e) {
// Do nothing.
} }
} }
return all; return all;

View File

@@ -25,6 +25,7 @@ import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.data.SubmitTypeRecord; import com.google.gerrit.common.data.SubmitTypeRecord;
import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -182,14 +183,19 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
changeSet.ids().contains(cd.getId()) changeSet.ids().contains(cd.getId())
&& (projectState != null) && (projectState != null)
&& projectState.statePermitsRead(); && projectState.statePermitsRead();
if (visible if (!visible) {
&& !permissionBackend.user(user).change(cd).database(db).test(ChangePermission.READ)) { return false;
}
try {
permissionBackend.user(user).change(cd).database(db).check(ChangePermission.READ);
return true;
} catch (AuthException e) {
// 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.
visible = false; return false;
} }
return visible;
} }
private SubmitType submitType(ChangeData cd) throws OrmException { private SubmitType submitType(ChangeData cd) throws OrmException {

View File

@@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkState;
import com.google.common.base.Strings; import com.google.common.base.Strings;
import com.google.common.collect.Iterables; import com.google.common.collect.Iterables;
import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Change; 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;
@@ -100,17 +101,22 @@ public class MergeSuperSet {
List<ChangeData> cds = queryProvider.get().byLegacyChangeId(change.getId()); List<ChangeData> cds = queryProvider.get().byLegacyChangeId(change.getId());
checkState(cds.size() == 1, "Expected exactly one ChangeData, got " + cds.size()); checkState(cds.size() == 1, "Expected exactly one ChangeData, got " + cds.size());
ChangeData cd = Iterables.getFirst(cds, null); ChangeData cd = Iterables.getFirst(cds, null);
ProjectState projectState = projectCache.checkedGet(cd.project());
ChangeSet changeSet = boolean visible = false;
new ChangeSet( if (cd != null) {
cd, ProjectState projectState = projectCache.checkedGet(cd.project());
projectState != null
&& projectState.statePermitsRead() if (projectState.statePermitsRead()) {
&& permissionBackend try {
.user(user) permissionBackend.user(user).change(cd).database(db).check(ChangePermission.READ);
.change(cd) visible = true;
.database(db) } catch (AuthException e) {
.test(ChangePermission.READ)); // Do nothing.
}
}
}
ChangeSet changeSet = new ChangeSet(cd, visible);
if (wholeTopicEnabled(cfg)) { if (wholeTopicEnabled(cfg)) {
return completeChangeSetIncludingTopics(db, changeSet, user); return completeChangeSetIncludingTopics(db, changeSet, user);
} }
@@ -203,8 +209,15 @@ public class MergeSuperSet {
private boolean canRead(ReviewDb db, CurrentUser user, ChangeData cd) private boolean canRead(ReviewDb db, CurrentUser user, ChangeData cd)
throws PermissionBackendException, IOException { throws PermissionBackendException, IOException {
ProjectState projectState = projectCache.checkedGet(cd.project()); ProjectState projectState = projectCache.checkedGet(cd.project());
return projectState != null if (projectState == null || !projectState.statePermitsRead()) {
&& projectState.statePermitsRead() return false;
&& permissionBackend.user(user).change(cd).database(db).test(ChangePermission.READ); }
try {
permissionBackend.user(user).change(cd).database(db).check(ChangePermission.READ);
return true;
} catch (AuthException e) {
return false;
}
} }
} }

View File

@@ -86,15 +86,22 @@ public class ChangeArgumentParser {
} }
for (ChangeNotes notes : matched) { for (ChangeNotes notes : matched) {
if (!changes.containsKey(notes.getChangeId()) if (!changes.containsKey(notes.getChangeId())
&& inProject(projectState, notes.getProjectName()) && inProject(projectState, notes.getProjectName())) {
&& (canMaintainServer if (canMaintainServer) {
|| (permissionBackend toAdd.add(notes);
.currentUser() continue;
.change(notes) }
.database(db)
.test(ChangePermission.READ) if (!projectState.statePermitsRead()) {
&& projectState.statePermitsRead()))) { continue;
toAdd.add(notes); }
try {
permissionBackend.currentUser().change(notes).database(db).check(ChangePermission.READ);
toAdd.add(notes);
} catch (AuthException e) {
// Do nothing.
}
} }
} }