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

This commit is contained in:
xchangcheng
2018-07-11 07:54:02 +00:00
committed by Gerrit Code Review
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;
@@ -132,10 +133,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.
}
} }
} }