ChangeUtil: Fix findChanges for anonymous users

In I7710befa I didn't realize the user provider was a
Provider<IdentifiedUser>, which caused a ProvisionException when
anonymous users tried to use the REST API to look up changes by
something other than a number. This wasn't noticed sooner because it
didn't affect the web UI, which exclusively uses change numbers.

Change to Provider<CurrentUser> and use asIdentifiedUser only at the
sites where an IdentifiedUser is needed. This will throw a different
unchecked exception type on failure (UnsupportedOperationException
instead of ProvisionException), but callers aren't depending on that.

Add a test, during which I also discovered a bug in the implementation
of AbstractDaemonTest#setApiUserAnonymous. That wasn't already causing
failures because the only user was using it to avoid some side effects
that "may" have happened.

Change-Id: Idbad114d6f5a61167d6fda32ee0093558491cf8e
This commit is contained in:
Dave Borowitz 2015-12-08 09:14:24 -05:00
parent 2e66f80a4f
commit 2afe3e308d
3 changed files with 26 additions and 4 deletions

View File

@ -468,7 +468,8 @@ public abstract class AbstractDaemonTest {
} }
protected Context setApiUserAnonymous() { protected Context setApiUserAnonymous() {
return atrScope.newContext(reviewDbProvider, null, anonymousUser.get()); return atrScope.set(
atrScope.newContext(reviewDbProvider, null, anonymousUser.get()));
} }
protected static Gson newGson() { protected static Gson newGson() {

View File

@ -792,6 +792,27 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(rev2.pushCertificate.key).isNull(); assertThat(rev2.pushCertificate.key).isNull();
} }
@Test
public void anonymousRestApi() throws Exception {
setApiUserAnonymous();
PushOneCommit.Result r = createChange();
ChangeInfo info = gApi.changes().id(r.getChangeId()).get();
assertThat(info.changeId).isEqualTo(r.getChangeId());
String triplet = project.get() + "~master~" + r.getChangeId();
info = gApi.changes().id(triplet).get();
assertThat(info.changeId).isEqualTo(r.getChangeId());
info = gApi.changes().id(info._number).get();
assertThat(info.changeId).isEqualTo(r.getChangeId());
exception.expect(AuthException.class);
gApi.changes()
.id(triplet)
.current()
.review(ReviewInput.approve());
}
private static Iterable<Account.Id> getReviewers( private static Iterable<Account.Id> getReviewers(
Collection<AccountInfo> r) { Collection<AccountInfo> r) {

View File

@ -177,7 +177,7 @@ public class ChangeUtil {
return subject; return subject;
} }
private final Provider<IdentifiedUser> user; private final Provider<CurrentUser> user;
private final Provider<ReviewDb> db; private final Provider<ReviewDb> db;
private final Provider<InternalChangeQuery> queryProvider; private final Provider<InternalChangeQuery> queryProvider;
private final ChangeControl.GenericFactory changeControlFactory; private final ChangeControl.GenericFactory changeControlFactory;
@ -192,7 +192,7 @@ public class ChangeUtil {
private final StarredChangesUtil starredChangesUtil; private final StarredChangesUtil starredChangesUtil;
@Inject @Inject
ChangeUtil(Provider<IdentifiedUser> user, ChangeUtil(Provider<CurrentUser> user,
Provider<ReviewDb> db, Provider<ReviewDb> db,
Provider<InternalChangeQuery> queryProvider, Provider<InternalChangeQuery> queryProvider,
ChangeControl.GenericFactory changeControlFactory, ChangeControl.GenericFactory changeControlFactory,
@ -238,7 +238,7 @@ public class ChangeUtil {
RevCommit commitToRevert = RevCommit commitToRevert =
revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get())); revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get()));
PersonIdent authorIdent = user.get() PersonIdent authorIdent = user.get().asIdentifiedUser()
.newCommitterIdent(myIdent.getWhen(), myIdent.getTimeZone()); .newCommitterIdent(myIdent.getWhen(), myIdent.getTimeZone());
if (commitToRevert.getParentCount() == 0) { if (commitToRevert.getParentCount() == 0) {