Merge changes Icc3077b4,I33b4bc5d,I824caa34

* changes:
  AbstractDaemonTest: Rename AccountCreator variable to 'accountCreator'
  ExternalIdCacheImpl: Cache external IDs by email
  Add basic consistency checker for accounts
This commit is contained in:
David Pursehouse
2017-06-10 01:29:21 +00:00
committed by Gerrit Code Review
21 changed files with 236 additions and 68 deletions

View File

@@ -156,6 +156,7 @@ link:#consistency-check-input[ConsistencyCheckInput] entity.
Content-Type: application/json; charset=UTF-8 Content-Type: application/json; charset=UTF-8
{ {
"check_accounts": {},
"check_account_external_ids": {} "check_account_external_ids": {}
} }
---- ----
@@ -170,6 +171,14 @@ is returned that contains detected consistency problems.
)]}' )]}'
{ {
"check_accounts_result": {
"problems": [
{
"status": "ERROR",
"message": "Account \u00271000024\u0027 has no external ID for its preferred email \u0027foo.bar@example.com\u0027"
}
]
}
"check_account_external_ids_result": { "check_account_external_ids_result": {
"problems": [ "problems": [
{ {
@@ -1505,6 +1514,9 @@ consistency checks.
[options="header",cols="1,^1,5"] [options="header",cols="1,^1,5"]
|================================================ |================================================
|Field Name ||Description |Field Name ||Description
|`check_accounts_result` |optional|
The result of running the account consistency check as a
link:#check-accounts-result-info[CheckAccountsResultInfo] entity.
|`check_account_external_ids_result`|optional| |`check_account_external_ids_result`|optional|
The result of running the account external ID consistency check as a The result of running the account external ID consistency check as a
link:#check-account-external-ids-result-info[ link:#check-account-external-ids-result-info[
@@ -1519,6 +1531,9 @@ consistency checks should be run.
[options="header",cols="1,^1,5"] [options="header",cols="1,^1,5"]
|========================================= |=========================================
|Field Name ||Description |Field Name ||Description
|`check_accounts` |optional|
Input for the account consistency check as
link:#check-accounts-input[CheckAccountsInput] entity.
|`check_account_external_ids`|optional| |`check_account_external_ids`|optional|
Input for the account external ID consistency check as Input for the account external ID consistency check as
link:#check-account-external-ids-input[CheckAccountExternalIdsInput] link:#check-account-external-ids-input[CheckAccountExternalIdsInput]

View File

@@ -199,7 +199,7 @@ public abstract class AbstractDaemonTest {
@Inject @GerritServerConfig protected Config cfg; @Inject @GerritServerConfig protected Config cfg;
@Inject protected AcceptanceTestRequestScope atrScope; @Inject protected AcceptanceTestRequestScope atrScope;
@Inject protected AccountCache accountCache; @Inject protected AccountCache accountCache;
@Inject protected AccountCreator accounts; @Inject protected AccountCreator accountCreator;
@Inject protected AllProjectsName allProjects; @Inject protected AllProjectsName allProjects;
@Inject protected BatchUpdate.Factory batchUpdateFactory; @Inject protected BatchUpdate.Factory batchUpdateFactory;
@Inject protected ChangeData.Factory changeDataFactory; @Inject protected ChangeData.Factory changeDataFactory;
@@ -327,8 +327,8 @@ public abstract class AbstractDaemonTest {
server.getTestInjector().injectMembers(this); server.getTestInjector().injectMembers(this);
Transport.register(inProcessProtocol); Transport.register(inProcessProtocol);
toClose = Collections.synchronizedList(new ArrayList<Repository>()); toClose = Collections.synchronizedList(new ArrayList<Repository>());
admin = accounts.admin(); admin = accountCreator.admin();
user = accounts.user(); user = accountCreator.user();
// Evict cached user state in case tests modify it. // Evict cached user state in case tests modify it.
accountCache.evict(admin.getId()); accountCache.evict(admin.getId());
@@ -370,7 +370,7 @@ public abstract class AbstractDaemonTest {
private TestAccount getCloneAsAccount(Description description) { private TestAccount getCloneAsAccount(Description description) {
TestProjectInput ann = description.getAnnotation(TestProjectInput.class); TestProjectInput ann = description.getAnnotation(TestProjectInput.class);
return accounts.get(ann != null ? ann.cloneAs() : "admin"); return accountCreator.get(ann != null ? ann.cloneAs() : "admin");
} }
private ProjectInput projectInput(Description description) { private ProjectInput projectInput(Description description) {

View File

@@ -52,6 +52,10 @@ import com.google.gerrit.extensions.api.accounts.EmailInput;
import com.google.gerrit.extensions.api.changes.AddReviewerInput; import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.StarsInput; import com.google.gerrit.extensions.api.changes.StarsInput;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInput;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInput.CheckAccountsInput;
import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.GpgKeyInfo; import com.google.gerrit.extensions.common.GpgKeyInfo;
@@ -90,6 +94,7 @@ import java.util.ArrayList;
import java.util.Arrays; import java.util.Arrays;
import java.util.Collection; import java.util.Collection;
import java.util.EnumSet; import java.util.EnumSet;
import java.util.HashSet;
import java.util.Iterator; import java.util.Iterator;
import java.util.List; import java.util.List;
import java.util.Locale; import java.util.Locale;
@@ -207,7 +212,7 @@ public class AccountIT extends AbstractDaemonTest {
@Test @Test
public void create() throws Exception { public void create() throws Exception {
TestAccount foo = accounts.create("foo"); TestAccount foo = accountCreator.create("foo");
AccountInfo info = gApi.accounts().id(foo.id.get()).get(); AccountInfo info = gApi.accounts().id(foo.id.get()).get();
assertThat(info.username).isEqualTo("foo"); assertThat(info.username).isEqualTo("foo");
if (SshMode.useSsh()) { if (SshMode.useSsh()) {
@@ -386,7 +391,7 @@ public class AccountIT extends AbstractDaemonTest {
@Test @Test
public void ignoreChange() throws Exception { public void ignoreChange() throws Exception {
TestAccount user2 = accounts.user2(); TestAccount user2 = accountCreator.user2();
accountIndexedCounter.clear(); accountIndexedCounter.clear();
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
@@ -495,7 +500,7 @@ public class AccountIT extends AbstractDaemonTest {
@Test @Test
public void cannotAddNonConfirmedEmailWithoutModifyAccountPermission() throws Exception { public void cannotAddNonConfirmedEmailWithoutModifyAccountPermission() throws Exception {
TestAccount account = accounts.create(name("user")); TestAccount account = accountCreator.create(name("user"));
EmailInput input = new EmailInput(); EmailInput input = new EmailInput();
input.email = "test@test.com"; input.email = "test@test.com";
input.noConfirmation = true; input.noConfirmation = true;
@@ -1047,6 +1052,41 @@ public class AccountIT extends AbstractDaemonTest {
gApi.accounts().id(admin.username).index(); gApi.accounts().id(admin.username).index();
} }
@Test
@Sandboxed
public void checkConsistency() throws Exception {
allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
resetCurrentApiUser();
// Create an account with a preferred email.
String username = name("foo");
String email = username + "@example.com";
TestAccount account = accountCreator.create(username, email, "Foo Bar");
ConsistencyCheckInput input = new ConsistencyCheckInput();
input.checkAccounts = new CheckAccountsInput();
ConsistencyCheckInfo checkInfo = gApi.config().server().checkConsistency(input);
assertThat(checkInfo.checkAccountsResult.problems).isEmpty();
Set<ConsistencyProblemInfo> expectedProblems = new HashSet<>();
// Delete the external ID for the preferred email. This makes the account inconsistent since it
// now doesn't have an external ID for its preferred email.
externalIdsUpdate.delete(ExternalId.createEmail(account.getId(), email));
expectedProblems.add(
new ConsistencyProblemInfo(
ConsistencyProblemInfo.Status.ERROR,
"Account '"
+ account.getId().get()
+ "' has no external ID for its preferred email '"
+ email
+ "'"));
checkInfo = gApi.config().server().checkConsistency(input);
assertThat(checkInfo.checkAccountsResult.problems).hasSize(expectedProblems.size());
assertThat(checkInfo.checkAccountsResult.problems).containsExactlyElementsIn(expectedProblems);
}
private void assertSequenceNumbers(List<SshKeyInfo> sshKeys) { private void assertSequenceNumbers(List<SshKeyInfo> sshKeys) {
int seq = 1; int seq = 1;
for (SshKeyInfo key : sshKeys) { for (SshKeyInfo key : sshKeys) {

View File

@@ -52,7 +52,7 @@ public class GeneralPreferencesIT extends AbstractDaemonTest {
@Before @Before
public void setUp() throws Exception { public void setUp() throws Exception {
String name = name("user42"); String name = name("user42");
user42 = accounts.create(name, name + "@example.com", "User 42"); user42 = accountCreator.create(name, name + "@example.com", "User 42");
} }
@After @After

View File

@@ -453,16 +453,16 @@ public class ChangeIT extends AbstractDaemonTest {
ReviewInput in = ReviewInput.approve(); ReviewInput in = ReviewInput.approve();
in.reviewer(user.email); in.reviewer(user.email);
in.reviewer(accounts.user2().email, ReviewerState.CC, true); in.reviewer(accountCreator.user2().email, ReviewerState.CC, true);
// Add user as reviewer that will create the revert // Add user as reviewer that will create the revert
in.reviewer(accounts.admin2().email); in.reviewer(accountCreator.admin2().email);
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in); gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in);
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit(); gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
// expect both the original reviewers and CCs to be preserved // expect both the original reviewers and CCs to be preserved
// original owner should be added as reviewer, user requesting the revert (new owner) removed // original owner should be added as reviewer, user requesting the revert (new owner) removed
setApiUser(accounts.admin2()); setApiUser(accountCreator.admin2());
Map<ReviewerState, Collection<AccountInfo>> result = Map<ReviewerState, Collection<AccountInfo>> result =
gApi.changes().id(r.getChangeId()).revert().get().reviewers; gApi.changes().id(r.getChangeId()).revert().get().reviewers;
assertThat(result).containsKey(ReviewerState.REVIEWER); assertThat(result).containsKey(ReviewerState.REVIEWER);
@@ -473,11 +473,11 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(result).containsKey(ReviewerState.CC); assertThat(result).containsKey(ReviewerState.CC);
List<Integer> ccs = List<Integer> ccs =
result.get(ReviewerState.CC).stream().map(a -> a._accountId).collect(toList()); result.get(ReviewerState.CC).stream().map(a -> a._accountId).collect(toList());
assertThat(ccs).containsExactly(accounts.user2().id.get()); assertThat(ccs).containsExactly(accountCreator.user2().id.get());
assertThat(reviewers).containsExactly(user.id.get(), admin.id.get()); assertThat(reviewers).containsExactly(user.id.get(), admin.id.get());
} else { } else {
assertThat(reviewers) assertThat(reviewers)
.containsExactly(user.id.get(), admin.id.get(), accounts.user2().id.get()); .containsExactly(user.id.get(), admin.id.get(), accountCreator.user2().id.get());
} }
} }
@@ -1633,7 +1633,7 @@ public class ChangeIT extends AbstractDaemonTest {
in.notify = NotifyHandling.NONE; in.notify = NotifyHandling.NONE;
// notify unrelated account as TO // notify unrelated account as TO
TestAccount user2 = accounts.user2(); TestAccount user2 = accountCreator.user2();
setApiUser(user); setApiUser(user);
recommend(r.getChangeId()); recommend(r.getChangeId());
setApiUser(admin); setApiUser(admin);

View File

@@ -92,8 +92,8 @@ public class GroupsIT extends AbstractDaemonTest {
@Test @Test
public void addMultipleMembers() throws Exception { public void addMultipleMembers() throws Exception {
String g = createGroup("users"); String g = createGroup("users");
TestAccount u1 = accounts.create("u1", "u1@example.com", "Full Name 1"); TestAccount u1 = accountCreator.create("u1", "u1@example.com", "Full Name 1");
TestAccount u2 = accounts.create("u2", "u2@example.com", "Full Name 2"); TestAccount u2 = accountCreator.create("u2", "u2@example.com", "Full Name 2");
gApi.groups().id(g).addMembers(u1.username, u2.username); gApi.groups().id(g).addMembers(u1.username, u2.username);
assertMembers(g, u1, u2); assertMembers(g, u1, u2);
} }
@@ -101,10 +101,10 @@ public class GroupsIT extends AbstractDaemonTest {
@Test @Test
public void addMembersWithAtSign() throws Exception { public void addMembersWithAtSign() throws Exception {
String g = createGroup("users"); String g = createGroup("users");
TestAccount u10 = accounts.create("u10", "u10@example.com", "Full Name 10"); TestAccount u10 = accountCreator.create("u10", "u10@example.com", "Full Name 10");
TestAccount u11_at = TestAccount u11_at =
accounts.create("u11@something", "u11@example.com", "Full Name 11 With At"); accountCreator.create("u11@something", "u11@example.com", "Full Name 11 With At");
accounts.create("u11", "u11.another@example.com", "Full Name 11 Without At"); accountCreator.create("u11", "u11.another@example.com", "Full Name 11 Without At");
gApi.groups().id(g).addMembers(u10.username, u11_at.username); gApi.groups().id(g).addMembers(u10.username, u11_at.username);
assertMembers(g, u10, u11_at); assertMembers(g, u10, u11_at);
} }
@@ -529,7 +529,7 @@ public class GroupsIT extends AbstractDaemonTest {
// reindex is tested by {@link AbstractQueryGroupsTest#reindex} // reindex is tested by {@link AbstractQueryGroupsTest#reindex}
@Test @Test
public void reindexPermissions() throws Exception { public void reindexPermissions() throws Exception {
TestAccount groupOwner = accounts.user2(); TestAccount groupOwner = accountCreator.user2();
GroupInput in = new GroupInput(); GroupInput in = new GroupInput();
in.name = name("group"); in.name = name("group");
in.members = in.members =
@@ -611,7 +611,7 @@ public class GroupsIT extends AbstractDaemonTest {
private String createAccount(String name, String group) throws Exception { private String createAccount(String name, String group) throws Exception {
name = name(name); name = name(name);
accounts.create(name, group); accountCreator.create(name, group);
return name; return name;
} }
} }

View File

@@ -666,7 +666,7 @@ public class RevisionIT extends AbstractDaemonTest {
assertThat(sender.getMessages()).hasSize(0); assertThat(sender.getMessages()).hasSize(0);
// Disable the notification. The user provided in the 'notifyDetails' should still be notified. // Disable the notification. The user provided in the 'notifyDetails' should still be notified.
TestAccount userToNotify = accounts.user2(); TestAccount userToNotify = accountCreator.user2();
input.destination = "branch-3"; input.destination = "branch-3";
input.notify = NotifyHandling.NONE; input.notify = NotifyHandling.NONE;
input.notifyDetails = input.notifyDetails =

View File

@@ -247,7 +247,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
@Test @Test
public void pushForMasterWithNotify() throws Exception { public void pushForMasterWithNotify() throws Exception {
// create a user that watches the project // create a user that watches the project
TestAccount user3 = accounts.create("user3", "user3@example.com", "User3"); TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3");
List<ProjectWatchInfo> projectsToWatch = new ArrayList<>(); List<ProjectWatchInfo> projectsToWatch = new ArrayList<>();
ProjectWatchInfo pwi = new ProjectWatchInfo(); ProjectWatchInfo pwi = new ProjectWatchInfo();
pwi.project = project.get(); pwi.project = project.get();
@@ -257,7 +257,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
setApiUser(user3); setApiUser(user3);
gApi.accounts().self().setWatchedProjects(projectsToWatch); gApi.accounts().self().setWatchedProjects(projectsToWatch);
TestAccount user2 = accounts.user2(); TestAccount user2 = accountCreator.user2();
String pushSpec = "refs/for/master%reviewer=" + user.email + ",cc=" + user2.email; String pushSpec = "refs/for/master%reviewer=" + user.email + ",cc=" + user2.email;
sender.clear(); sender.clear();
@@ -335,11 +335,14 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
+ ",cc=" + ",cc="
+ user.email + user.email
+ ",cc=" + ",cc="
+ accounts.user2().email); + accountCreator.user2().email);
r.assertOkStatus(); r.assertOkStatus();
// Check that admin isn't CC'd as they own the change // Check that admin isn't CC'd as they own the change
r.assertChange( r.assertChange(
Change.Status.NEW, topic, ImmutableList.of(), ImmutableList.of(user, accounts.user2())); Change.Status.NEW,
topic,
ImmutableList.of(),
ImmutableList.of(user, accountCreator.user2()));
// cc non-existing user // cc non-existing user
String nonExistingEmail = "non.existing@example.com"; String nonExistingEmail = "non.existing@example.com";
@@ -365,7 +368,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
r.assertChange(Change.Status.NEW, topic, user); r.assertChange(Change.Status.NEW, topic, user);
// add several reviewers // add several reviewers
TestAccount user2 = accounts.create("another-user", "another.user@example.com", "Another User"); TestAccount user2 =
accountCreator.create("another-user", "another.user@example.com", "Another User");
r = r =
pushTo( pushTo(
"refs/for/master/" "refs/for/master/"

View File

@@ -48,7 +48,7 @@ public class CheckAccessIT extends AbstractDaemonTest {
secretRefProject = createProject("secretRef"); secretRefProject = createProject("secretRef");
privilegedGroup = groupCache.get(new AccountGroup.NameKey(createGroup("privilegedGroup"))); privilegedGroup = groupCache.get(new AccountGroup.NameKey(createGroup("privilegedGroup")));
privilegedUser = accounts.create("privilegedUser", "snowden@nsa.gov", "Ed Snowden"); privilegedUser = accountCreator.create("privilegedUser", "snowden@nsa.gov", "Ed Snowden");
gApi.groups().id(privilegedGroup.getGroupUUID().get()).addMembers(privilegedUser.username); gApi.groups().id(privilegedGroup.getGroupUUID().get()).addMembers(privilegedUser.username);
assertThat(gApi.groups().id(privilegedGroup.getGroupUUID().get()).members().get(0).email) assertThat(gApi.groups().id(privilegedGroup.getGroupUUID().get()).members().get(0).email)

View File

@@ -87,7 +87,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
@Before @Before
public void setUp() throws Exception { public void setUp() throws Exception {
anonRestSession = new RestSession(server, null); anonRestSession = new RestSession(server, null);
admin2 = accounts.admin2(); admin2 = accountCreator.admin2();
GroupInput gi = new GroupInput(); GroupInput gi = new GroupInput();
gi.name = name("New-Group"); gi.name = name("New-Group");
gi.members = ImmutableList.of(user.id.toString()); gi.members = ImmutableList.of(user.id.toString());
@@ -322,7 +322,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
@Test @Test
public void voteOnBehalfOfInvisibleUserNotAllowed() throws Exception { public void voteOnBehalfOfInvisibleUserNotAllowed() throws Exception {
allowCodeReviewOnBehalfOf(); allowCodeReviewOnBehalfOf();
setApiUser(accounts.user2()); setApiUser(accountCreator.user2());
assertThat(accountControlFactory.get().canSee(user.id)).isFalse(); assertThat(accountControlFactory.get().canSee(user.id)).isFalse();
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
@@ -401,7 +401,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
@Test @Test
public void submitOnBehalfOfInvisibleUserNotAllowed() throws Exception { public void submitOnBehalfOfInvisibleUserNotAllowed() throws Exception {
allowSubmitOnBehalfOf(); allowSubmitOnBehalfOf();
setApiUser(accounts.user2()); setApiUser(accountCreator.user2());
assertThat(accountControlFactory.get().canSee(user.id)).isFalse(); assertThat(accountControlFactory.get().canSee(user.id)).isFalse();
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
@@ -502,7 +502,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
// X-Gerrit-RunAs user (user2). // X-Gerrit-RunAs user (user2).
allowRunAs(); allowRunAs();
allowCodeReviewOnBehalfOf(); allowCodeReviewOnBehalfOf();
TestAccount user2 = accounts.user2(); TestAccount user2 = accountCreator.user2();
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
ReviewInput in = new ReviewInput(); ReviewInput in = new ReviewInput();
@@ -542,7 +542,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
in.message = "Message on behalf of"; in.message = "Message on behalf of";
in.label("Code-Review", 1); in.label("Code-Review", 1);
setApiUser(accounts.user2()); setApiUser(accountCreator.user2());
gApi.changes().id(r.getChangeId()).revision(r.getPatchSetId().getId()).review(in); gApi.changes().id(r.getChangeId()).revision(r.getPatchSetId().getId()).review(in);
ChangeInfo info = ChangeInfo info =
@@ -551,7 +551,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
ChangeMessageInfo changeMessageInfo = Iterables.getLast(info.messages); ChangeMessageInfo changeMessageInfo = Iterables.getLast(info.messages);
assertThat(changeMessageInfo.realAuthor).isNotNull(); assertThat(changeMessageInfo.realAuthor).isNotNull();
assertThat(changeMessageInfo.realAuthor._accountId).isEqualTo(accounts.user2().id.get()); assertThat(changeMessageInfo.realAuthor._accountId).isEqualTo(accountCreator.user2().id.get());
} }
private void allowCodeReviewOnBehalfOf() throws Exception { private void allowCodeReviewOnBehalfOf() throws Exception {

View File

@@ -27,7 +27,7 @@ public class PutUsernameIT extends AbstractDaemonTest {
PutUsername.Input in = new PutUsername.Input(); PutUsername.Input in = new PutUsername.Input();
in.username = "myUsername"; in.username = "myUsername";
RestResponse r = RestResponse r =
adminRestSession.put("/accounts/" + accounts.create().id.get() + "/username", in); adminRestSession.put("/accounts/" + accountCreator.create().id.get() + "/username", in);
r.assertOK(); r.assertOK();
assertThat(newGson().fromJson(r.getReader(), String.class)).isEqualTo(in.username); assertThat(newGson().fromJson(r.getReader(), String.class)).isEqualTo(in.username);
} }
@@ -37,7 +37,7 @@ public class PutUsernameIT extends AbstractDaemonTest {
PutUsername.Input in = new PutUsername.Input(); PutUsername.Input in = new PutUsername.Input();
in.username = admin.username; in.username = admin.username;
adminRestSession adminRestSession
.put("/accounts/" + accounts.create().id.get() + "/username", in) .put("/accounts/" + accountCreator.create().id.get() + "/username", in)
.assertConflict(); .assertConflict();
} }

View File

@@ -39,7 +39,7 @@ public class ChangeOwnerIT extends AbstractDaemonTest {
@Before @Before
public void setUp() throws Exception { public void setUp() throws Exception {
setApiUser(user); setApiUser(user);
user2 = accounts.user2(); user2 = accountCreator.user2();
} }
@Test @Test

View File

@@ -199,7 +199,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
// CC a group that overlaps with some existing reviewers and CCed accounts. // CC a group that overlaps with some existing reviewers and CCed accounts.
TestAccount reviewer = TestAccount reviewer =
accounts.create(name("reviewer"), "addCcGroup-reviewer@example.com", "Reviewer"); accountCreator.create(name("reviewer"), "addCcGroup-reviewer@example.com", "Reviewer");
result = addReviewer(changeId, reviewer.username); result = addReviewer(changeId, reviewer.username);
assertThat(result.error).isNull(); assertThat(result.error).isNull();
sender.clear(); sender.clear();
@@ -425,7 +425,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
@Test @Test
public void reviewAndAddReviewers() throws Exception { public void reviewAndAddReviewers() throws Exception {
TestAccount observer = accounts.user2(); TestAccount observer = accountCreator.user2();
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
ReviewInput input = ReviewInput input =
ReviewInput.approve().reviewer(user.email).reviewer(observer.email, CC, false); ReviewInput.approve().reviewer(user.email).reviewer(observer.email, CC, false);
@@ -480,7 +480,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
.id(mediumGroup) .id(mediumGroup)
.addMembers(usernames.subList(0, mediumGroupSize).toArray(new String[mediumGroupSize])); .addMembers(usernames.subList(0, mediumGroupSize).toArray(new String[mediumGroupSize]));
TestAccount observer = accounts.user2(); TestAccount observer = accountCreator.user2();
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
// Attempt to add overly large group as reviewers. // Attempt to add overly large group as reviewers.
@@ -610,9 +610,12 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
@Test @Test
public void addOverlappingGroups() throws Exception { public void addOverlappingGroups() throws Exception {
String emailPrefix = "addOverlappingGroups-"; String emailPrefix = "addOverlappingGroups-";
TestAccount user1 = accounts.create(name("user1"), emailPrefix + "user1@example.com", "User1"); TestAccount user1 =
TestAccount user2 = accounts.create(name("user2"), emailPrefix + "user2@example.com", "User2"); accountCreator.create(name("user1"), emailPrefix + "user1@example.com", "User1");
TestAccount user3 = accounts.create(name("user3"), emailPrefix + "user3@example.com", "User3"); TestAccount user2 =
accountCreator.create(name("user2"), emailPrefix + "user2@example.com", "User2");
TestAccount user3 =
accountCreator.create(name("user3"), emailPrefix + "user3@example.com", "User3");
String group1 = createGroup("group1"); String group1 = createGroup("group1");
String group2 = createGroup("group2"); String group2 = createGroup("group2");
gApi.groups().id(group1).addMembers(user1.username, user2.username); gApi.groups().id(group1).addMembers(user1.username, user2.username);
@@ -796,7 +799,8 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
List<TestAccount> result = new ArrayList<>(n); List<TestAccount> result = new ArrayList<>(n);
for (int i = 0; i < n; i++) { for (int i = 0; i < n; i++) {
result.add( result.add(
accounts.create(name("u" + i), emailPrefix + "-" + i + "@example.com", "Full Name " + i)); accountCreator.create(
name("u" + i), emailPrefix + "-" + i + "@example.com", "Full Name " + i));
} }
return result; return result;
} }

View File

@@ -421,7 +421,8 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
private TestAccount user(String name, String fullName, String emailName, AccountGroup... groups) private TestAccount user(String name, String fullName, String emailName, AccountGroup... groups)
throws Exception { throws Exception {
String[] groupNames = Arrays.stream(groups).map(AccountGroup::getName).toArray(String[]::new); String[] groupNames = Arrays.stream(groups).map(AccountGroup::getName).toArray(String[]::new);
return accounts.create(name(name), name(emailName) + "@example.com", fullName, groupNames); return accountCreator.create(
name(name), name(emailName) + "@example.com", fullName, groupNames);
} }
private TestAccount user(String name, String fullName, AccountGroup... groups) throws Exception { private TestAccount user(String name, String fullName, AccountGroup... groups) throws Exception {

View File

@@ -118,7 +118,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
@Test @Test
public void missingOwner() throws Exception { public void missingOwner() throws Exception {
TestAccount owner = accounts.create("missing"); TestAccount owner = accountCreator.create("missing");
ChangeControl ctl = insertChange(owner); ChangeControl ctl = insertChange(owner);
accountsUpdate.create().deleteByKey(db, owner.getId()); accountsUpdate.create().deleteByKey(db, owner.getId());

View File

@@ -350,7 +350,7 @@ public class ProjectWatchIT extends AbstractDaemonTest {
sender.clear(); sender.clear();
// watch project as user2 // watch project as user2
TestAccount user2 = accounts.create("user2", "user2@test.com", "User2"); TestAccount user2 = accountCreator.create("user2", "user2@test.com", "User2");
setApiUser(user2); setApiUser(user2);
watch(watchedProject, null); watch(watchedProject, null);
@@ -467,7 +467,7 @@ public class ProjectWatchIT extends AbstractDaemonTest {
sender.clear(); sender.clear();
// watch project as user2 // watch project as user2
TestAccount user2 = accounts.create("user2", "user2@test.com", "User2"); TestAccount user2 = accountCreator.create("user2", "user2@test.com", "User2");
setApiUser(user2); setApiUser(user2);
watch(anyProject, null); watch(anyProject, null);
@@ -568,7 +568,7 @@ public class ProjectWatchIT extends AbstractDaemonTest {
// watch project as user that can view all drafts // watch project as user that can view all drafts
TestAccount userThatCanViewDrafts = TestAccount userThatCanViewDrafts =
accounts.create("user2", "user2@test.com", "User2", groupThatCanViewDrafts.name); accountCreator.create("user2", "user2@test.com", "User2", groupThatCanViewDrafts.name);
setApiUser(userThatCanViewDrafts); setApiUser(userThatCanViewDrafts);
watch(watchedProject, null); watch(watchedProject, null);
@@ -639,7 +639,7 @@ public class ProjectWatchIT extends AbstractDaemonTest {
@Test @Test
public void deleteAllProjectWatchesIfWatchConfigIsTheOnlyFileInUserBranch() throws Exception { public void deleteAllProjectWatchesIfWatchConfigIsTheOnlyFileInUserBranch() throws Exception {
// Create account that has no files in its refs/users/ branch. // Create account that has no files in its refs/users/ branch.
Account.Id id = accounts.create().id; Account.Id id = accountCreator.create().id;
// Add a project watch so that a watch.config file in the refs/users/ branch is created. // Add a project watch so that a watch.config file in the refs/users/ branch is created.
Map<ProjectWatchKey, Set<NotifyType>> watches = new HashMap<>(); Map<ProjectWatchKey, Set<NotifyType>> watches = new HashMap<>();
@@ -694,7 +694,8 @@ public class ProjectWatchIT extends AbstractDaemonTest {
// watch project as user that can view all private change // watch project as user that can view all private change
TestAccount userThatCanViewPrivateChanges = TestAccount userThatCanViewPrivateChanges =
accounts.create("user2", "user2@test.com", "User2", groupThatCanViewPrivateChanges.name); accountCreator.create(
"user2", "user2@test.com", "User2", groupThatCanViewPrivateChanges.name);
setApiUser(userThatCanViewPrivateChanges); setApiUser(userThatCanViewPrivateChanges);
watch(watchedProject, null); watch(watchedProject, null);

View File

@@ -18,8 +18,17 @@ import java.util.List;
import java.util.Objects; import java.util.Objects;
public class ConsistencyCheckInfo { public class ConsistencyCheckInfo {
public CheckAccountsResultInfo checkAccountsResult;
public CheckAccountExternalIdsResultInfo checkAccountExternalIdsResult; public CheckAccountExternalIdsResultInfo checkAccountExternalIdsResult;
public static class CheckAccountsResultInfo {
public List<ConsistencyProblemInfo> problems;
public CheckAccountsResultInfo(List<ConsistencyProblemInfo> problems) {
this.problems = problems;
}
}
public static class CheckAccountExternalIdsResultInfo { public static class CheckAccountExternalIdsResultInfo {
public List<ConsistencyProblemInfo> problems; public List<ConsistencyProblemInfo> problems;

View File

@@ -15,7 +15,10 @@
package com.google.gerrit.extensions.api.config; package com.google.gerrit.extensions.api.config;
public class ConsistencyCheckInput { public class ConsistencyCheckInput {
public CheckAccountsInput checkAccounts;
public CheckAccountExternalIdsInput checkAccountExternalIds; public CheckAccountExternalIdsInput checkAccountExternalIds;
public static class CheckAccountsInput {}
public static class CheckAccountExternalIdsInput {} public static class CheckAccountExternalIdsInput {}
} }

View File

@@ -0,0 +1,64 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.account;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
@Singleton
public class AccountsConsistencyChecker {
private final Provider<ReviewDb> dbProvider;
private final ExternalIds externalIds;
@Inject
AccountsConsistencyChecker(Provider<ReviewDb> dbProvider, ExternalIds externalIds) {
this.dbProvider = dbProvider;
this.externalIds = externalIds;
}
public List<ConsistencyProblemInfo> check() throws OrmException, IOException {
List<ConsistencyProblemInfo> problems = new ArrayList<>();
for (Account account : dbProvider.get().accounts().all()) {
if (account.getPreferredEmail() != null) {
if (!externalIds
.byAccount(account.getId())
.stream()
.anyMatch(e -> account.getPreferredEmail().equals(e.email()))) {
addError(
String.format(
"Account '%s' has no external ID for its preferred email '%s'",
account.getId().get(), account.getPreferredEmail()),
problems);
}
}
}
return problems;
}
private static void addError(String error, List<ConsistencyProblemInfo> problems) {
problems.add(new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.ERROR, error));
}
}

View File

@@ -14,8 +14,11 @@
package com.google.gerrit.server.account.externalids; package com.google.gerrit.server.account.externalids;
import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
import static java.util.stream.Collectors.toSet; import static java.util.stream.Collectors.toSet;
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader; import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache; import com.google.common.cache.LoadingCache;
@@ -44,8 +47,7 @@ import org.slf4j.LoggerFactory;
class ExternalIdCacheImpl implements ExternalIdCache { class ExternalIdCacheImpl implements ExternalIdCache {
private static final Logger log = LoggerFactory.getLogger(ExternalIdCacheImpl.class); private static final Logger log = LoggerFactory.getLogger(ExternalIdCacheImpl.class);
private final LoadingCache<ObjectId, ImmutableSetMultimap<Account.Id, ExternalId>> private final LoadingCache<ObjectId, AllExternalIds> extIdsByAccount;
extIdsByAccount;
private final ExternalIdReader externalIdReader; private final ExternalIdReader externalIdReader;
private final Lock lock; private final Lock lock;
@@ -217,7 +219,7 @@ class ExternalIdCacheImpl implements ExternalIdCache {
@Override @Override
public Set<ExternalId> byAccount(Account.Id accountId) throws IOException { public Set<ExternalId> byAccount(Account.Id accountId) throws IOException {
try { try {
return extIdsByAccount.get(externalIdReader.readRevision()).get(accountId); return extIdsByAccount.get(externalIdReader.readRevision()).byAccount().get(accountId);
} catch (ExecutionException e) { } catch (ExecutionException e) {
throw new IOException("Cannot list external ids by account", e); throw new IOException("Cannot list external ids by account", e);
} }
@@ -226,12 +228,7 @@ class ExternalIdCacheImpl implements ExternalIdCache {
@Override @Override
public Set<ExternalId> byEmail(String email) throws IOException { public Set<ExternalId> byEmail(String email) throws IOException {
try { try {
return extIdsByAccount return extIdsByAccount.get(externalIdReader.readRevision()).byEmail().get(email);
.get(externalIdReader.readRevision())
.values()
.stream()
.filter(e -> email.equals(e.email()))
.collect(toSet());
} catch (ExecutionException e) { } catch (ExecutionException e) {
throw new IOException("Cannot list external ids by email", e); throw new IOException("Cannot list external ids by email", e);
} }
@@ -245,12 +242,15 @@ class ExternalIdCacheImpl implements ExternalIdCache {
try { try {
ListMultimap<Account.Id, ExternalId> m; ListMultimap<Account.Id, ExternalId> m;
if (!ObjectId.zeroId().equals(oldNotesRev)) { if (!ObjectId.zeroId().equals(oldNotesRev)) {
m = MultimapBuilder.hashKeys().arrayListValues().build(extIdsByAccount.get(oldNotesRev)); m =
MultimapBuilder.hashKeys()
.arrayListValues()
.build(extIdsByAccount.get(oldNotesRev).byAccount());
} else { } else {
m = MultimapBuilder.hashKeys().arrayListValues().build(); m = MultimapBuilder.hashKeys().arrayListValues().build();
} }
update.accept(m); update.accept(m);
extIdsByAccount.put(newNotesRev, ImmutableSetMultimap.copyOf(m)); extIdsByAccount.put(newNotesRev, AllExternalIds.create(m));
} catch (ExecutionException e) { } catch (ExecutionException e) {
log.warn("Cannot update external IDs", e); log.warn("Cannot update external IDs", e);
} finally { } finally {
@@ -262,8 +262,7 @@ class ExternalIdCacheImpl implements ExternalIdCache {
Collections2.transform(ids, e -> e.key()).removeAll(toRemove); Collections2.transform(ids, e -> e.key()).removeAll(toRemove);
} }
private static class Loader private static class Loader extends CacheLoader<ObjectId, AllExternalIds> {
extends CacheLoader<ObjectId, ImmutableSetMultimap<Account.Id, ExternalId>> {
private final ExternalIdReader externalIdReader; private final ExternalIdReader externalIdReader;
Loader(ExternalIdReader externalIdReader) { Loader(ExternalIdReader externalIdReader) {
@@ -271,13 +270,31 @@ class ExternalIdCacheImpl implements ExternalIdCache {
} }
@Override @Override
public ImmutableSetMultimap<Account.Id, ExternalId> load(ObjectId notesRev) throws Exception { public AllExternalIds load(ObjectId notesRev) throws Exception {
Multimap<Account.Id, ExternalId> extIdsByAccount = Multimap<Account.Id, ExternalId> extIdsByAccount =
MultimapBuilder.hashKeys().arrayListValues().build(); MultimapBuilder.hashKeys().arrayListValues().build();
for (ExternalId extId : externalIdReader.all(notesRev)) { for (ExternalId extId : externalIdReader.all(notesRev)) {
extIdsByAccount.put(extId.accountId(), extId); extIdsByAccount.put(extId.accountId(), extId);
} }
return ImmutableSetMultimap.copyOf(extIdsByAccount); return AllExternalIds.create(extIdsByAccount);
} }
} }
@AutoValue
abstract static class AllExternalIds {
static AllExternalIds create(Multimap<Account.Id, ExternalId> byAccount) {
ImmutableSetMultimap<String, ExternalId> byEmail =
byAccount
.values()
.stream()
.filter(e -> !Strings.isNullOrEmpty(e.email()))
.collect(toImmutableSetMultimap(ExternalId::email, e -> e));
return new AutoValue_ExternalIdCacheImpl_AllExternalIds(
ImmutableSetMultimap.copyOf(byAccount), byEmail);
}
public abstract ImmutableSetMultimap<Account.Id, ExternalId> byAccount();
public abstract ImmutableSetMultimap<String, ExternalId> byEmail();
}
} }

View File

@@ -16,13 +16,16 @@ package com.google.gerrit.server.config;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo; import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.CheckAccountExternalIdsResultInfo; import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.CheckAccountExternalIdsResultInfo;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.CheckAccountsResultInfo;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInput; import com.google.gerrit.extensions.api.config.ConsistencyCheckInput;
import com.google.gerrit.extensions.restapi.AuthException; 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.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountsConsistencyChecker;
import com.google.gerrit.server.account.externalids.ExternalIdsConsistencyChecker; import com.google.gerrit.server.account.externalids.ExternalIdsConsistencyChecker;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.Provider; import com.google.inject.Provider;
import com.google.inject.Singleton; import com.google.inject.Singleton;
@@ -31,19 +34,22 @@ import java.io.IOException;
@Singleton @Singleton
public class CheckConsistency implements RestModifyView<ConfigResource, ConsistencyCheckInput> { public class CheckConsistency implements RestModifyView<ConfigResource, ConsistencyCheckInput> {
private final Provider<IdentifiedUser> userProvider; private final Provider<IdentifiedUser> userProvider;
private final AccountsConsistencyChecker accountsConsistencyChecker;
private final ExternalIdsConsistencyChecker externalIdsConsistencyChecker; private final ExternalIdsConsistencyChecker externalIdsConsistencyChecker;
@Inject @Inject
CheckConsistency( CheckConsistency(
Provider<IdentifiedUser> currentUser, Provider<IdentifiedUser> currentUser,
AccountsConsistencyChecker accountsConsistencyChecker,
ExternalIdsConsistencyChecker externalIdsConsistencyChecker) { ExternalIdsConsistencyChecker externalIdsConsistencyChecker) {
this.userProvider = currentUser; this.userProvider = currentUser;
this.accountsConsistencyChecker = accountsConsistencyChecker;
this.externalIdsConsistencyChecker = externalIdsConsistencyChecker; this.externalIdsConsistencyChecker = externalIdsConsistencyChecker;
} }
@Override @Override
public ConsistencyCheckInfo apply(ConfigResource resource, ConsistencyCheckInput input) public ConsistencyCheckInfo apply(ConfigResource resource, ConsistencyCheckInput input)
throws RestApiException, IOException { throws RestApiException, IOException, OrmException {
IdentifiedUser user = userProvider.get(); IdentifiedUser user = userProvider.get();
if (!user.isIdentifiedUser()) { if (!user.isIdentifiedUser()) {
throw new AuthException("Authentication required"); throw new AuthException("Authentication required");
@@ -52,11 +58,15 @@ public class CheckConsistency implements RestModifyView<ConfigResource, Consiste
throw new AuthException("not allowed to run consistency checks"); throw new AuthException("not allowed to run consistency checks");
} }
if (input == null || input.checkAccountExternalIds == null) { if (input == null || (input.checkAccounts == null && input.checkAccountExternalIds == null)) {
throw new BadRequestException("input required"); throw new BadRequestException("input required");
} }
ConsistencyCheckInfo consistencyCheckInfo = new ConsistencyCheckInfo(); ConsistencyCheckInfo consistencyCheckInfo = new ConsistencyCheckInfo();
if (input.checkAccounts != null) {
consistencyCheckInfo.checkAccountsResult =
new CheckAccountsResultInfo(accountsConsistencyChecker.check());
}
if (input.checkAccountExternalIds != null) { if (input.checkAccountExternalIds != null) {
consistencyCheckInfo.checkAccountExternalIdsResult = consistencyCheckInfo.checkAccountExternalIdsResult =
new CheckAccountExternalIdsResultInfo(externalIdsConsistencyChecker.check()); new CheckAccountExternalIdsResultInfo(externalIdsConsistencyChecker.check());