diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt index 9c9ea266c3..6ce9688e65 100644 --- a/Documentation/rest-api-config.txt +++ b/Documentation/rest-api-config.txt @@ -156,6 +156,7 @@ link:#consistency-check-input[ConsistencyCheckInput] entity. Content-Type: application/json; charset=UTF-8 { + "check_accounts": {}, "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": { "problems": [ { @@ -1505,6 +1514,9 @@ consistency checks. [options="header",cols="1,^1,5"] |================================================ |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| The result of running the account external ID consistency check as a link:#check-account-external-ids-result-info[ @@ -1519,6 +1531,9 @@ consistency checks should be run. [options="header",cols="1,^1,5"] |========================================= |Field Name ||Description +|`check_accounts` |optional| +Input for the account consistency check as +link:#check-accounts-input[CheckAccountsInput] entity. |`check_account_external_ids`|optional| Input for the account external ID consistency check as link:#check-account-external-ids-input[CheckAccountExternalIdsInput] diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 2c68bc82f6..73a66a4a5f 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -199,7 +199,7 @@ public abstract class AbstractDaemonTest { @Inject @GerritServerConfig protected Config cfg; @Inject protected AcceptanceTestRequestScope atrScope; @Inject protected AccountCache accountCache; - @Inject protected AccountCreator accounts; + @Inject protected AccountCreator accountCreator; @Inject protected AllProjectsName allProjects; @Inject protected BatchUpdate.Factory batchUpdateFactory; @Inject protected ChangeData.Factory changeDataFactory; @@ -327,8 +327,8 @@ public abstract class AbstractDaemonTest { server.getTestInjector().injectMembers(this); Transport.register(inProcessProtocol); toClose = Collections.synchronizedList(new ArrayList()); - admin = accounts.admin(); - user = accounts.user(); + admin = accountCreator.admin(); + user = accountCreator.user(); // Evict cached user state in case tests modify it. accountCache.evict(admin.getId()); @@ -370,7 +370,7 @@ public abstract class AbstractDaemonTest { private TestAccount getCloneAsAccount(Description description) { 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) { diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 89d54e142d..80e187212d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -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.ReviewInput; 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.ChangeInfo; import com.google.gerrit.extensions.common.GpgKeyInfo; @@ -90,6 +94,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.EnumSet; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Locale; @@ -207,7 +212,7 @@ public class AccountIT extends AbstractDaemonTest { @Test public void create() throws Exception { - TestAccount foo = accounts.create("foo"); + TestAccount foo = accountCreator.create("foo"); AccountInfo info = gApi.accounts().id(foo.id.get()).get(); assertThat(info.username).isEqualTo("foo"); if (SshMode.useSsh()) { @@ -386,7 +391,7 @@ public class AccountIT extends AbstractDaemonTest { @Test public void ignoreChange() throws Exception { - TestAccount user2 = accounts.user2(); + TestAccount user2 = accountCreator.user2(); accountIndexedCounter.clear(); PushOneCommit.Result r = createChange(); @@ -495,7 +500,7 @@ public class AccountIT extends AbstractDaemonTest { @Test public void cannotAddNonConfirmedEmailWithoutModifyAccountPermission() throws Exception { - TestAccount account = accounts.create(name("user")); + TestAccount account = accountCreator.create(name("user")); EmailInput input = new EmailInput(); input.email = "test@test.com"; input.noConfirmation = true; @@ -1047,6 +1052,41 @@ public class AccountIT extends AbstractDaemonTest { 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 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 sshKeys) { int seq = 1; for (SshKeyInfo key : sshKeys) { diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java index fbeeafd34c..307e76a958 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java @@ -52,7 +52,7 @@ public class GeneralPreferencesIT extends AbstractDaemonTest { @Before public void setUp() throws Exception { String name = name("user42"); - user42 = accounts.create(name, name + "@example.com", "User 42"); + user42 = accountCreator.create(name, name + "@example.com", "User 42"); } @After diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 5a2cb2ad59..bf506abfd8 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -453,16 +453,16 @@ public class ChangeIT extends AbstractDaemonTest { ReviewInput in = ReviewInput.approve(); 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 - 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()).submit(); // expect both the original reviewers and CCs to be preserved // original owner should be added as reviewer, user requesting the revert (new owner) removed - setApiUser(accounts.admin2()); + setApiUser(accountCreator.admin2()); Map> result = gApi.changes().id(r.getChangeId()).revert().get().reviewers; assertThat(result).containsKey(ReviewerState.REVIEWER); @@ -473,11 +473,11 @@ public class ChangeIT extends AbstractDaemonTest { assertThat(result).containsKey(ReviewerState.CC); List ccs = 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()); } else { 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; // notify unrelated account as TO - TestAccount user2 = accounts.user2(); + TestAccount user2 = accountCreator.user2(); setApiUser(user); recommend(r.getChangeId()); setApiUser(admin); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java index 7752f3ed08..b687e4c1d5 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -92,8 +92,8 @@ public class GroupsIT extends AbstractDaemonTest { @Test public void addMultipleMembers() throws Exception { String g = createGroup("users"); - TestAccount u1 = accounts.create("u1", "u1@example.com", "Full Name 1"); - TestAccount u2 = accounts.create("u2", "u2@example.com", "Full Name 2"); + TestAccount u1 = accountCreator.create("u1", "u1@example.com", "Full Name 1"); + TestAccount u2 = accountCreator.create("u2", "u2@example.com", "Full Name 2"); gApi.groups().id(g).addMembers(u1.username, u2.username); assertMembers(g, u1, u2); } @@ -101,10 +101,10 @@ public class GroupsIT extends AbstractDaemonTest { @Test public void addMembersWithAtSign() throws Exception { 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 = - accounts.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@something", "u11@example.com", "Full Name 11 With At"); + accountCreator.create("u11", "u11.another@example.com", "Full Name 11 Without At"); gApi.groups().id(g).addMembers(u10.username, u11_at.username); assertMembers(g, u10, u11_at); } @@ -529,7 +529,7 @@ public class GroupsIT extends AbstractDaemonTest { // reindex is tested by {@link AbstractQueryGroupsTest#reindex} @Test public void reindexPermissions() throws Exception { - TestAccount groupOwner = accounts.user2(); + TestAccount groupOwner = accountCreator.user2(); GroupInput in = new GroupInput(); in.name = name("group"); in.members = @@ -611,7 +611,7 @@ public class GroupsIT extends AbstractDaemonTest { private String createAccount(String name, String group) throws Exception { name = name(name); - accounts.create(name, group); + accountCreator.create(name, group); return name; } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java index c96b33932b..255c0cbfdf 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java @@ -666,7 +666,7 @@ public class RevisionIT extends AbstractDaemonTest { assertThat(sender.getMessages()).hasSize(0); // 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.notify = NotifyHandling.NONE; input.notifyDetails = diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 877a42a7ce..773ad44a2e 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -247,7 +247,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { @Test public void pushForMasterWithNotify() throws Exception { // 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 projectsToWatch = new ArrayList<>(); ProjectWatchInfo pwi = new ProjectWatchInfo(); pwi.project = project.get(); @@ -257,7 +257,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { setApiUser(user3); gApi.accounts().self().setWatchedProjects(projectsToWatch); - TestAccount user2 = accounts.user2(); + TestAccount user2 = accountCreator.user2(); String pushSpec = "refs/for/master%reviewer=" + user.email + ",cc=" + user2.email; sender.clear(); @@ -335,11 +335,14 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { + ",cc=" + user.email + ",cc=" - + accounts.user2().email); + + accountCreator.user2().email); r.assertOkStatus(); // Check that admin isn't CC'd as they own the change 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 String nonExistingEmail = "non.existing@example.com"; @@ -365,7 +368,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { r.assertChange(Change.Status.NEW, topic, user); // 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 = pushTo( "refs/for/master/" diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/CheckAccessIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/CheckAccessIT.java index c16f93260d..53c89c5c95 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/CheckAccessIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/CheckAccessIT.java @@ -48,7 +48,7 @@ public class CheckAccessIT extends AbstractDaemonTest { secretRefProject = createProject("secretRef"); 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); assertThat(gApi.groups().id(privilegedGroup.getGroupUUID().get()).members().get(0).email) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java index b221ec510d..05e5f99927 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java @@ -87,7 +87,7 @@ public class ImpersonationIT extends AbstractDaemonTest { @Before public void setUp() throws Exception { anonRestSession = new RestSession(server, null); - admin2 = accounts.admin2(); + admin2 = accountCreator.admin2(); GroupInput gi = new GroupInput(); gi.name = name("New-Group"); gi.members = ImmutableList.of(user.id.toString()); @@ -322,7 +322,7 @@ public class ImpersonationIT extends AbstractDaemonTest { @Test public void voteOnBehalfOfInvisibleUserNotAllowed() throws Exception { allowCodeReviewOnBehalfOf(); - setApiUser(accounts.user2()); + setApiUser(accountCreator.user2()); assertThat(accountControlFactory.get().canSee(user.id)).isFalse(); PushOneCommit.Result r = createChange(); @@ -401,7 +401,7 @@ public class ImpersonationIT extends AbstractDaemonTest { @Test public void submitOnBehalfOfInvisibleUserNotAllowed() throws Exception { allowSubmitOnBehalfOf(); - setApiUser(accounts.user2()); + setApiUser(accountCreator.user2()); assertThat(accountControlFactory.get().canSee(user.id)).isFalse(); PushOneCommit.Result r = createChange(); @@ -502,7 +502,7 @@ public class ImpersonationIT extends AbstractDaemonTest { // X-Gerrit-RunAs user (user2). allowRunAs(); allowCodeReviewOnBehalfOf(); - TestAccount user2 = accounts.user2(); + TestAccount user2 = accountCreator.user2(); PushOneCommit.Result r = createChange(); ReviewInput in = new ReviewInput(); @@ -542,7 +542,7 @@ public class ImpersonationIT extends AbstractDaemonTest { in.message = "Message on behalf of"; in.label("Code-Review", 1); - setApiUser(accounts.user2()); + setApiUser(accountCreator.user2()); gApi.changes().id(r.getChangeId()).revision(r.getPatchSetId().getId()).review(in); ChangeInfo info = @@ -551,7 +551,7 @@ public class ImpersonationIT extends AbstractDaemonTest { ChangeMessageInfo changeMessageInfo = Iterables.getLast(info.messages); 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 { diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/PutUsernameIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/PutUsernameIT.java index 17dabde04c..7de9d70e80 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/PutUsernameIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/PutUsernameIT.java @@ -27,7 +27,7 @@ public class PutUsernameIT extends AbstractDaemonTest { PutUsername.Input in = new PutUsername.Input(); in.username = "myUsername"; RestResponse r = - adminRestSession.put("/accounts/" + accounts.create().id.get() + "/username", in); + adminRestSession.put("/accounts/" + accountCreator.create().id.get() + "/username", in); r.assertOK(); 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(); in.username = admin.username; adminRestSession - .put("/accounts/" + accounts.create().id.get() + "/username", in) + .put("/accounts/" + accountCreator.create().id.get() + "/username", in) .assertConflict(); } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeOwnerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeOwnerIT.java index 49f5c5a278..5ade72a853 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeOwnerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeOwnerIT.java @@ -39,7 +39,7 @@ public class ChangeOwnerIT extends AbstractDaemonTest { @Before public void setUp() throws Exception { setApiUser(user); - user2 = accounts.user2(); + user2 = accountCreator.user2(); } @Test diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java index 0809bf2873..c7c02b20c2 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java @@ -199,7 +199,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest { // CC a group that overlaps with some existing reviewers and CCed accounts. 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); assertThat(result.error).isNull(); sender.clear(); @@ -425,7 +425,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest { @Test public void reviewAndAddReviewers() throws Exception { - TestAccount observer = accounts.user2(); + TestAccount observer = accountCreator.user2(); PushOneCommit.Result r = createChange(); ReviewInput input = ReviewInput.approve().reviewer(user.email).reviewer(observer.email, CC, false); @@ -480,7 +480,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest { .id(mediumGroup) .addMembers(usernames.subList(0, mediumGroupSize).toArray(new String[mediumGroupSize])); - TestAccount observer = accounts.user2(); + TestAccount observer = accountCreator.user2(); PushOneCommit.Result r = createChange(); // Attempt to add overly large group as reviewers. @@ -610,9 +610,12 @@ public class ChangeReviewersIT extends AbstractDaemonTest { @Test public void addOverlappingGroups() throws Exception { String emailPrefix = "addOverlappingGroups-"; - TestAccount user1 = accounts.create(name("user1"), emailPrefix + "user1@example.com", "User1"); - TestAccount user2 = accounts.create(name("user2"), emailPrefix + "user2@example.com", "User2"); - TestAccount user3 = accounts.create(name("user3"), emailPrefix + "user3@example.com", "User3"); + TestAccount user1 = + accountCreator.create(name("user1"), emailPrefix + "user1@example.com", "User1"); + 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 group2 = createGroup("group2"); gApi.groups().id(group1).addMembers(user1.username, user2.username); @@ -796,7 +799,8 @@ public class ChangeReviewersIT extends AbstractDaemonTest { List result = new ArrayList<>(n); for (int i = 0; i < n; i++) { 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; } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java index 03bc71146c..897ac4861b 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java @@ -421,7 +421,8 @@ public class SuggestReviewersIT extends AbstractDaemonTest { private TestAccount user(String name, String fullName, String emailName, AccountGroup... groups) throws Exception { 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 { diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index 8f256be130..c2fcd87881 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -118,7 +118,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { @Test public void missingOwner() throws Exception { - TestAccount owner = accounts.create("missing"); + TestAccount owner = accountCreator.create("missing"); ChangeControl ctl = insertChange(owner); accountsUpdate.create().deleteByKey(db, owner.getId()); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java index 0671840750..140a756884 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java @@ -350,7 +350,7 @@ public class ProjectWatchIT extends AbstractDaemonTest { sender.clear(); // watch project as user2 - TestAccount user2 = accounts.create("user2", "user2@test.com", "User2"); + TestAccount user2 = accountCreator.create("user2", "user2@test.com", "User2"); setApiUser(user2); watch(watchedProject, null); @@ -467,7 +467,7 @@ public class ProjectWatchIT extends AbstractDaemonTest { sender.clear(); // watch project as user2 - TestAccount user2 = accounts.create("user2", "user2@test.com", "User2"); + TestAccount user2 = accountCreator.create("user2", "user2@test.com", "User2"); setApiUser(user2); watch(anyProject, null); @@ -568,7 +568,7 @@ public class ProjectWatchIT extends AbstractDaemonTest { // watch project as user that can view all drafts TestAccount userThatCanViewDrafts = - accounts.create("user2", "user2@test.com", "User2", groupThatCanViewDrafts.name); + accountCreator.create("user2", "user2@test.com", "User2", groupThatCanViewDrafts.name); setApiUser(userThatCanViewDrafts); watch(watchedProject, null); @@ -639,7 +639,7 @@ public class ProjectWatchIT extends AbstractDaemonTest { @Test public void deleteAllProjectWatchesIfWatchConfigIsTheOnlyFileInUserBranch() throws Exception { // 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. Map> watches = new HashMap<>(); @@ -694,7 +694,8 @@ public class ProjectWatchIT extends AbstractDaemonTest { // watch project as user that can view all private change TestAccount userThatCanViewPrivateChanges = - accounts.create("user2", "user2@test.com", "User2", groupThatCanViewPrivateChanges.name); + accountCreator.create( + "user2", "user2@test.com", "User2", groupThatCanViewPrivateChanges.name); setApiUser(userThatCanViewPrivateChanges); watch(watchedProject, null); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInfo.java index e3b7dd359d..e44eb286a9 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInfo.java @@ -18,8 +18,17 @@ import java.util.List; import java.util.Objects; public class ConsistencyCheckInfo { + public CheckAccountsResultInfo checkAccountsResult; public CheckAccountExternalIdsResultInfo checkAccountExternalIdsResult; + public static class CheckAccountsResultInfo { + public List problems; + + public CheckAccountsResultInfo(List problems) { + this.problems = problems; + } + } + public static class CheckAccountExternalIdsResultInfo { public List problems; diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInput.java index 170db0ff71..f3d927e0a5 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInput.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/config/ConsistencyCheckInput.java @@ -15,7 +15,10 @@ package com.google.gerrit.extensions.api.config; public class ConsistencyCheckInput { + public CheckAccountsInput checkAccounts; public CheckAccountExternalIdsInput checkAccountExternalIds; + public static class CheckAccountsInput {} + public static class CheckAccountExternalIdsInput {} } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java new file mode 100644 index 0000000000..10a1411b5b --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java @@ -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 dbProvider; + private final ExternalIds externalIds; + + @Inject + AccountsConsistencyChecker(Provider dbProvider, ExternalIds externalIds) { + this.dbProvider = dbProvider; + this.externalIds = externalIds; + } + + public List check() throws OrmException, IOException { + List 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 problems) { + problems.add(new ConsistencyProblemInfo(ConsistencyProblemInfo.Status.ERROR, error)); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java index 9db00d5d94..2f68e98062 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java @@ -14,8 +14,11 @@ package com.google.gerrit.server.account.externalids; +import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap; 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.CacheLoader; import com.google.common.cache.LoadingCache; @@ -44,8 +47,7 @@ import org.slf4j.LoggerFactory; class ExternalIdCacheImpl implements ExternalIdCache { private static final Logger log = LoggerFactory.getLogger(ExternalIdCacheImpl.class); - private final LoadingCache> - extIdsByAccount; + private final LoadingCache extIdsByAccount; private final ExternalIdReader externalIdReader; private final Lock lock; @@ -217,7 +219,7 @@ class ExternalIdCacheImpl implements ExternalIdCache { @Override public Set byAccount(Account.Id accountId) throws IOException { try { - return extIdsByAccount.get(externalIdReader.readRevision()).get(accountId); + return extIdsByAccount.get(externalIdReader.readRevision()).byAccount().get(accountId); } catch (ExecutionException e) { throw new IOException("Cannot list external ids by account", e); } @@ -226,12 +228,7 @@ class ExternalIdCacheImpl implements ExternalIdCache { @Override public Set byEmail(String email) throws IOException { try { - return extIdsByAccount - .get(externalIdReader.readRevision()) - .values() - .stream() - .filter(e -> email.equals(e.email())) - .collect(toSet()); + return extIdsByAccount.get(externalIdReader.readRevision()).byEmail().get(email); } catch (ExecutionException e) { throw new IOException("Cannot list external ids by email", e); } @@ -245,12 +242,15 @@ class ExternalIdCacheImpl implements ExternalIdCache { try { ListMultimap m; if (!ObjectId.zeroId().equals(oldNotesRev)) { - m = MultimapBuilder.hashKeys().arrayListValues().build(extIdsByAccount.get(oldNotesRev)); + m = + MultimapBuilder.hashKeys() + .arrayListValues() + .build(extIdsByAccount.get(oldNotesRev).byAccount()); } else { m = MultimapBuilder.hashKeys().arrayListValues().build(); } update.accept(m); - extIdsByAccount.put(newNotesRev, ImmutableSetMultimap.copyOf(m)); + extIdsByAccount.put(newNotesRev, AllExternalIds.create(m)); } catch (ExecutionException e) { log.warn("Cannot update external IDs", e); } finally { @@ -262,8 +262,7 @@ class ExternalIdCacheImpl implements ExternalIdCache { Collections2.transform(ids, e -> e.key()).removeAll(toRemove); } - private static class Loader - extends CacheLoader> { + private static class Loader extends CacheLoader { private final ExternalIdReader externalIdReader; Loader(ExternalIdReader externalIdReader) { @@ -271,13 +270,31 @@ class ExternalIdCacheImpl implements ExternalIdCache { } @Override - public ImmutableSetMultimap load(ObjectId notesRev) throws Exception { + public AllExternalIds load(ObjectId notesRev) throws Exception { Multimap extIdsByAccount = MultimapBuilder.hashKeys().arrayListValues().build(); for (ExternalId extId : externalIdReader.all(notesRev)) { extIdsByAccount.put(extId.accountId(), extId); } - return ImmutableSetMultimap.copyOf(extIdsByAccount); + return AllExternalIds.create(extIdsByAccount); } } + + @AutoValue + abstract static class AllExternalIds { + static AllExternalIds create(Multimap byAccount) { + ImmutableSetMultimap 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 byAccount(); + + public abstract ImmutableSetMultimap byEmail(); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/CheckConsistency.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/CheckConsistency.java index f4249952de..416ba37476 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/CheckConsistency.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/CheckConsistency.java @@ -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.CheckAccountExternalIdsResultInfo; +import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.CheckAccountsResultInfo; import com.google.gerrit.extensions.api.config.ConsistencyCheckInput; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.AccountsConsistencyChecker; import com.google.gerrit.server.account.externalids.ExternalIdsConsistencyChecker; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -31,19 +34,22 @@ import java.io.IOException; @Singleton public class CheckConsistency implements RestModifyView { private final Provider userProvider; + private final AccountsConsistencyChecker accountsConsistencyChecker; private final ExternalIdsConsistencyChecker externalIdsConsistencyChecker; @Inject CheckConsistency( Provider currentUser, + AccountsConsistencyChecker accountsConsistencyChecker, ExternalIdsConsistencyChecker externalIdsConsistencyChecker) { this.userProvider = currentUser; + this.accountsConsistencyChecker = accountsConsistencyChecker; this.externalIdsConsistencyChecker = externalIdsConsistencyChecker; } @Override public ConsistencyCheckInfo apply(ConfigResource resource, ConsistencyCheckInput input) - throws RestApiException, IOException { + throws RestApiException, IOException, OrmException { IdentifiedUser user = userProvider.get(); if (!user.isIdentifiedUser()) { throw new AuthException("Authentication required"); @@ -52,11 +58,15 @@ public class CheckConsistency implements RestModifyView