diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java index d86be16d6c..79c57484ea 100644 --- a/java/com/google/gerrit/server/account/AccountManager.java +++ b/java/com/google/gerrit/server/account/AccountManager.java @@ -143,7 +143,21 @@ public class AccountManager { try (ReviewDb db = schema.open()) { ExternalId id = externalIds.get(who.getExternalIdKey()); if (id == null) { + if (who.getUserName().isPresent()) { + ExternalId.Key key = ExternalId.Key.create(SCHEME_USERNAME, who.getUserName().get()); + ExternalId existingId = externalIds.get(key); + if (existingId != null) { + // An inconsistency is detected in the database, having a record for scheme "username:" + // but no record for scheme "gerrit:". Try to recover by linking + // "gerrit:" identity to the existing account. + log.warn( + "User {} already has an account; link new identity to the existing account.", + who.getUserName()); + return link(existingId.accountId(), who); + } + } // New account, automatically create and return. + log.info("External ID not found. Attempting to create new account."); return create(db, who); } @@ -383,6 +397,7 @@ public class AccountManager { public AuthResult link(Account.Id to, AuthRequest who) throws AccountException, OrmException, IOException, ConfigInvalidException { ExternalId extId = externalIds.get(who.getExternalIdKey()); + log.info("Link another authentication identity to an existing account"); if (extId != null) { if (!extId.accountId().equals(to)) { throw new AccountException( @@ -390,6 +405,7 @@ public class AccountManager { } update(who, extId); } else { + log.info("Linking new external ID to the existing account"); accountsUpdateProvider .get() .update( diff --git a/java/com/google/gerrit/server/config/PluginConfigFactory.java b/java/com/google/gerrit/server/config/PluginConfigFactory.java index 6310c0881a..09f2837c8b 100644 --- a/java/com/google/gerrit/server/config/PluginConfigFactory.java +++ b/java/com/google/gerrit/server/config/PluginConfigFactory.java @@ -288,7 +288,32 @@ public class PluginConfigFactory implements ReloadPluginListener { */ public Config getProjectPluginConfigWithInheritance( Project.NameKey projectName, String pluginName) throws NoSuchProjectException { - return getPluginConfig(projectName, pluginName).getWithInheritance(); + return getPluginConfig(projectName, pluginName).getWithInheritance(false); + } + + /** + * Returns the configuration for the specified plugin that is stored in the '{@code + * .config}' file in the 'refs/meta/config' branch of the specified project. + * Parameters from the '{@code .config}' of the parent project are appended to this + * project's '{@code .config}' files. + * + *

E.g.: child project: [mySection "mySubsection"] myKey = childValue + * + *

parent project: [mySection "mySubsection"] myKey = parentValue anotherKey = someValue + * + *

return: [mySection "mySubsection"] myKey = childValue myKey = parentValue anotherKey = + * someValue + * + * @param projectName the name of the project for which the plugin configuration should be + * returned + * @param pluginName the name of the plugin for which the configuration should be returned + * @return the plugin configuration from the '{@code .config}' file of the specified + * project with parameters from the parent projects appended to the project values + * @throws NoSuchProjectException thrown if the specified project does not exist + */ + public Config getProjectPluginConfigWithMergedInheritance( + Project.NameKey projectName, String pluginName) throws NoSuchProjectException { + return getPluginConfig(projectName, pluginName).getWithInheritance(true); } /** @@ -310,7 +335,30 @@ public class PluginConfigFactory implements ReloadPluginListener { */ public Config getProjectPluginConfigWithInheritance( ProjectState projectState, String pluginName) { - return projectState.getConfig(pluginName + EXTENSION).getWithInheritance(); + return projectState.getConfig(pluginName + EXTENSION).getWithInheritance(false); + } + + /** + * Returns the configuration for the specified plugin that is stored in the '{@code + * .config}' file in the 'refs/meta/config' branch of the specified project. + * Parameters from the '{@code .config}' of the parent project are appended to this + * project's '{@code .config}' files. + * + *

E.g.: child project: [mySection "mySubsection"] myKey = childValue + * + *

parent project: [mySection "mySubsection"] myKey = parentValue anotherKey = someValue + * + *

return: [mySection "mySubsection"] myKey = childValue myKey = parentValue anotherKey = + * someValue + * + * @param projectState the project for which the plugin configuration should be returned + * @param pluginName the name of the plugin for which the configuration should be returned + * @return the plugin configuration from the '{@code .config}' file of the specified + * project with inheriting non-set parameters from the parent projects + */ + public Config getProjectPluginConfigWithMergedInheritance( + ProjectState projectState, String pluginName) { + return projectState.getConfig(pluginName + EXTENSION).getWithInheritance(true); } private ProjectLevelConfig getPluginConfig(Project.NameKey projectName, String pluginName) diff --git a/java/com/google/gerrit/server/git/ProjectLevelConfig.java b/java/com/google/gerrit/server/git/ProjectLevelConfig.java index 18dcef88c5..2044db09bf 100644 --- a/java/com/google/gerrit/server/git/ProjectLevelConfig.java +++ b/java/com/google/gerrit/server/git/ProjectLevelConfig.java @@ -14,12 +14,16 @@ package com.google.gerrit.server.git; +import static java.util.stream.Collectors.toList; + import com.google.common.collect.Iterables; +import com.google.common.collect.Streams; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.project.ProjectState; import java.io.IOException; import java.util.Arrays; import java.util.Set; +import java.util.stream.Stream; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; @@ -53,6 +57,22 @@ public class ProjectLevelConfig extends VersionedMetaData { } public Config getWithInheritance() { + return getWithInheritance(false); + } + + /** + * Get a Config that includes the values from all parent projects. + * + *

Merging means that matching sections/subsection will be merged to include the values from + * both parent and child config. + * + *

No merging means that matching sections/subsections in the child project will replace the + * corresponding value from the parent. + * + * @param merge whether to merge parent values with child values or not. + * @return a combined config. + */ + public Config getWithInheritance(boolean merge) { Config cfgWithInheritance = new Config(); try { cfgWithInheritance.fromText(get().toText()); @@ -65,21 +85,41 @@ public class ProjectLevelConfig extends VersionedMetaData { for (String section : parentCfg.getSections()) { Set allNames = get().getNames(section); for (String name : parentCfg.getNames(section)) { + String[] parentValues = parentCfg.getStringList(section, null, name); if (!allNames.contains(name)) { + cfgWithInheritance.setStringList(section, null, name, Arrays.asList(parentValues)); + } else if (merge) { cfgWithInheritance.setStringList( - section, null, name, Arrays.asList(parentCfg.getStringList(section, null, name))); + section, + null, + name, + Stream.concat( + Arrays.stream(cfg.getStringList(section, null, name)), + Arrays.stream(parentValues)) + .sorted() + .distinct() + .collect(toList())); } } for (String subsection : parentCfg.getSubsections(section)) { allNames = get().getNames(section, subsection); for (String name : parentCfg.getNames(section, subsection)) { + String[] parentValues = parentCfg.getStringList(section, subsection, name); if (!allNames.contains(name)) { + cfgWithInheritance.setStringList( + section, subsection, name, Arrays.asList(parentValues)); + } else if (merge) { cfgWithInheritance.setStringList( section, subsection, name, - Arrays.asList(parentCfg.getStringList(section, subsection, name))); + Streams.concat( + Arrays.stream(cfg.getStringList(section, subsection, name)), + Arrays.stream(parentValues)) + .sorted() + .distinct() + .collect(toList())); } } } diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java index 4ff386204a..f9d7083ff1 100644 --- a/java/com/google/gerrit/server/restapi/change/PostReviewers.java +++ b/java/com/google/gerrit/server/restapi/change/PostReviewers.java @@ -194,13 +194,20 @@ public class PostReviewers Addition byAccountId = addByAccountId(reviewer, rsrc, state, notify, accountsToNotify, allowGroup, allowByEmail); + + Addition wholeGroup = null; + if (byAccountId == null || !byAccountId.exactMatchFound) { + wholeGroup = + addWholeGroup( + reviewer, rsrc, state, notify, accountsToNotify, confirmed, allowGroup, allowByEmail); + if (wholeGroup != null && wholeGroup.exactMatchFound) { + return wholeGroup; + } + } + if (byAccountId != null) { return byAccountId; } - - Addition wholeGroup = - addWholeGroup( - reviewer, rsrc, state, notify, accountsToNotify, confirmed, allowGroup, allowByEmail); if (wholeGroup != null) { return wholeGroup; } @@ -216,7 +223,8 @@ public class PostReviewers null, CC, NotifyHandling.NONE, - ImmutableListMultimap.of()); + ImmutableListMultimap.of(), + true); } @Nullable @@ -229,9 +237,14 @@ public class PostReviewers boolean allowGroup, boolean allowByEmail) throws OrmException, PermissionBackendException, IOException, ConfigInvalidException { - Account.Id accountId = null; + IdentifiedUser user = null; + boolean exactMatchFound = false; try { - accountId = accounts.parse(reviewer).getAccountId(); + user = accounts.parse(reviewer); + if (reviewer.equalsIgnoreCase(user.getName()) + || reviewer.equals(String.valueOf(user.getAccountId()))) { + exactMatchFound = true; + } } catch (UnprocessableEntityException | AuthException e) { // AuthException won't occur since the user is authenticated at this point. if (!allowGroup && !allowByEmail) { @@ -242,13 +255,20 @@ public class PostReviewers return null; } - ReviewerResource rrsrc = reviewerFactory.create(rsrc, accountId); + ReviewerResource rrsrc = reviewerFactory.create(rsrc, user.getAccountId()); Account member = rrsrc.getReviewerUser().getAccount(); PermissionBackend.ForRef perm = permissionBackend.user(rrsrc.getReviewerUser()).ref(rrsrc.getChange().getDest()); if (isValidReviewer(member, perm)) { return new Addition( - reviewer, rsrc, ImmutableSet.of(member.getId()), null, state, notify, accountsToNotify); + reviewer, + rsrc, + ImmutableSet.of(member.getId()), + null, + state, + notify, + accountsToNotify, + exactMatchFound); } if (!member.isActive()) { if (allowByEmail && state == CC) { @@ -328,7 +348,7 @@ public class PostReviewers } } - return new Addition(reviewer, rsrc, reviewers, null, state, notify, accountsToNotify); + return new Addition(reviewer, rsrc, reviewers, null, state, notify, accountsToNotify, true); } @Nullable @@ -357,7 +377,7 @@ public class PostReviewers return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInvalid, reviewer)); } return new Addition( - reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify); + reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify, true); } private boolean isValidReviewer(Account member, PermissionBackend.ForRef perm) @@ -395,6 +415,7 @@ public class PostReviewers final ReviewerState state; final ChangeNotes notes; final IdentifiedUser caller; + final boolean exactMatchFound; Addition(String reviewer) { result = new AddReviewerResult(reviewer); @@ -404,6 +425,7 @@ public class PostReviewers state = REVIEWER; notes = null; caller = null; + exactMatchFound = false; } protected Addition( @@ -413,7 +435,8 @@ public class PostReviewers @Nullable Collection

reviewersByEmail, ReviewerState state, @Nullable NotifyHandling notify, - ListMultimap accountsToNotify) { + ListMultimap accountsToNotify, + boolean exactMatchFound) { checkArgument( reviewers != null || reviewersByEmail != null, "must have either reviewers or reviewersByEmail"); @@ -427,6 +450,7 @@ public class PostReviewers op = postReviewersOpFactory.create( rsrc, this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify); + this.exactMatchFound = exactMatchFound; } void gatherResults() throws OrmException, PermissionBackendException { diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 8f27de3805..1a36ca9e85 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -168,7 +168,6 @@ import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.PushResult; import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; @NoHttpd @@ -1506,7 +1505,6 @@ public class ChangeIT extends AbstractDaemonTest { assertThat(sender.getMessages()).hasSize(1); } - @Ignore @Test public void addReviewerThatIsNotPerfectMatch() throws Exception { TestTimeUtil.resetWithClockStep(1, SECONDS); @@ -1551,7 +1549,6 @@ public class ChangeIT extends AbstractDaemonTest { assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs); } - @Ignore @Test public void addGroupAsReviewersWhenANotPerfectMatchedUserExists() throws Exception { TestTimeUtil.resetWithClockStep(1, SECONDS); @@ -1560,16 +1557,17 @@ public class ChangeIT extends AbstractDaemonTest { String oldETag = rsrc.getETag(); Timestamp oldTs = rsrc.getChange().getLastUpdatedOn(); - //create a group named "us" with one user: testUser - TestAccount testUser = accountCreator.create("testUser", "testUser@test.com", "testUser"); - String testGroup = - createGroupWithRealName(user.fullName.substring(0, user.fullName.length() / 2)); + //create a group named "kobe" with one user: lee + TestAccount testUser = accountCreator.create("kobebryant", "kobebryant@test.com", "kobebryant"); + TestAccount myGroupUser = accountCreator.create("lee", "lee@test.com", "lee"); + + String testGroup = createGroupWithRealName("kobe"); GroupApi groupApi = gApi.groups().id(testGroup); groupApi.description("test group"); - groupApi.addMembers(testUser.fullName); + groupApi.addMembers(myGroupUser.fullName); //ensure that user "user" is not in the group - groupApi.removeMembers(user.fullName); + groupApi.removeMembers(testUser.fullName); AddReviewerInput in = new AddReviewerInput(); in.reviewer = testGroup; @@ -1578,11 +1576,11 @@ public class ChangeIT extends AbstractDaemonTest { List messages = sender.getMessages(); assertThat(messages).hasSize(1); Message m = messages.get(0); - assertThat(m.rcpt()).containsExactly(testUser.emailAddress); - assertThat(m.body()).contains("Hello " + testUser.fullName + ",\n"); + assertThat(m.rcpt()).containsExactly(myGroupUser.emailAddress); + assertThat(m.body()).contains("Hello " + myGroupUser.fullName + ",\n"); assertThat(m.body()).contains("I'd like you to do a code review."); assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n"); - assertMailReplyTo(m, testUser.email); + assertMailReplyTo(m, myGroupUser.email); ChangeInfo c = gApi.changes().id(r.getChangeId()).get(); // When NoteDb is enabled adding a reviewer records that user as reviewer @@ -1592,7 +1590,7 @@ public class ChangeIT extends AbstractDaemonTest { Collection reviewers = c.reviewers.get(REVIEWER); assertThat(reviewers).isNotNull(); assertThat(reviewers).hasSize(1); - assertThat(reviewers.iterator().next()._accountId).isEqualTo(testUser.getId().get()); + assertThat(reviewers.iterator().next()._accountId).isEqualTo(myGroupUser.getId().get()); // Ensure ETag and lastUpdatedOn are updated. rsrc = parseResource(r); diff --git a/javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java b/javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java index c1bbf2e553..c78b47bbfe 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java @@ -22,6 +22,7 @@ import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.project.ProjectState; +import java.util.Arrays; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.junit.Before; @@ -114,4 +115,67 @@ public class ProjectLevelConfigIT extends AbstractDaemonTest { assertThat(state.getConfig(configName).get().toText()).isEqualTo(cfg.toText()); } + + @Test + public void withMergedInheritance() throws Exception { + String configName = "test.config"; + + Config parentCfg = new Config(); + parentCfg.setString("s1", null, "k1", "parentValue1"); + parentCfg.setString("s1", null, "k2", "parentValue2"); + parentCfg.setString("s2", "ss", "k3", "parentValue3"); + parentCfg.setString("s2", "ss", "k4", "parentValue4"); + + pushFactory + .create( + db, + admin.getIdent(), + testRepo, + "Create Project Level Config", + configName, + parentCfg.toText()) + .to(RefNames.REFS_CONFIG) + .assertOkStatus(); + + Project.NameKey childProject = createProject("child", project); + TestRepository childTestRepo = cloneProject(childProject); + fetch(childTestRepo, RefNames.REFS_CONFIG + ":refs/heads/config"); + childTestRepo.reset("refs/heads/config"); + + Config cfg = new Config(); + cfg.setString("s1", null, "k1", "parentValue1"); + cfg.setString("s1", null, "k2", "parentValue2"); + cfg.setString("s2", "ss", "k3", "parentValue3"); + cfg.setString("s2", "ss", "k4", "parentValue4"); + cfg.setString("s1", null, "k1", "childValue1"); + cfg.setString("s2", "ss", "k3", "childValue2"); + cfg.setString("s3", null, "k5", "childValue3"); + cfg.setString("s3", "ss", "k6", "childValue4"); + + pushFactory + .create( + db, + admin.getIdent(), + childTestRepo, + "Create Project Level Config", + configName, + cfg.toText()) + .to(RefNames.REFS_CONFIG) + .assertOkStatus(); + + ProjectState state = projectCache.get(childProject); + + Config expectedCfg = new Config(); + expectedCfg.setStringList("s1", null, "k1", Arrays.asList("childValue1", "parentValue1")); + expectedCfg.setString("s1", null, "k2", "parentValue2"); + expectedCfg.setStringList("s2", "ss", "k3", Arrays.asList("childValue2", "parentValue3")); + expectedCfg.setString("s2", "ss", "k4", "parentValue4"); + expectedCfg.setString("s3", null, "k5", "childValue3"); + expectedCfg.setString("s3", "ss", "k6", "childValue4"); + + assertThat(state.getConfig(configName).getWithInheritance(true).toText()) + .isEqualTo(expectedCfg.toText()); + + assertThat(state.getConfig(configName).get().toText()).isEqualTo(cfg.toText()); + } }