Include impersonated user to email, sent on their behalf
Previously, if an email was sent from the user, they were only added to
the email if they had 'CC_ON_OWN_COMMENTS' email strategy ('Every
comment' in Settings on UI). Otherwise they would not be notified.
This is problematic in cases when the calling user is different from the
user that email is sent from. If the user is not included in the email
recipients in any other way (e.g. reviewer, owner), they would not be
notified at all. An example of such case is 'Submit' operation, executed
onBehalfOn[1].
With this change, a copy of an email will be always sent to the sender
in case of impersonation, unless email notifications are explicitly
disabled by the user. In cases, when the user is not being impersonated,
an email is sent according to notification preferences.
It is possible, that we can get rid of fromUserId entirely, and always
send an email from the identified user. I am suspicious that it might
break plugins.
[1]https://gerrit.googlesource.com/gerrit/+/bc69bccb/java/com/google/gerrit/server/restapi/change/Submit.java#177
Change-Id: Ida0c153103b16c4e9902d771c000078b5a752063
diff --git a/java/com/google/gerrit/server/mail/send/EmailArguments.java b/java/com/google/gerrit/server/mail/send/EmailArguments.java
index 7fff232..258c9af 100644
--- a/java/com/google/gerrit/server/mail/send/EmailArguments.java
+++ b/java/com/google/gerrit/server/mail/send/EmailArguments.java
@@ -18,6 +18,7 @@
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdentProvider;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.IdentifiedUser.GenericFactory;
@@ -95,6 +96,7 @@
final OutgoingEmailValidator validator;
final boolean addInstanceNameInSubject;
final Provider<String> instanceNameProvider;
+ final Provider<CurrentUser> currentUserProvider;
@Inject
EmailArguments(
@@ -126,7 +128,8 @@
Provider<InternalAccountQuery> accountQueryProvider,
OutgoingEmailValidator validator,
@GerritInstanceName Provider<String> instanceNameProvider,
- @GerritServerConfig Config cfg) {
+ @GerritServerConfig Config cfg,
+ Provider<CurrentUser> currentUserProvider) {
this.server = server;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
@@ -157,5 +160,6 @@
this.instanceNameProvider = instanceNameProvider;
this.addInstanceNameInSubject = cfg.getBoolean("sendemail", "addInstanceNameInSubject", false);
+ this.currentUserProvider = currentUserProvider;
}
}
diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
index f5fb6b0..ddcc0cf 100644
--- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
+++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -30,6 +30,7 @@
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailFormat;
import com.google.gerrit.mail.MailHeader;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -127,18 +128,40 @@
Optional<AccountState> fromUser = args.accountCache.get(fromId);
if (fromUser.isPresent()) {
GeneralPreferencesInfo senderPrefs = fromUser.get().generalPreferences();
+ CurrentUser user = args.currentUserProvider.get();
+ boolean isImpersonating = user.isIdentifiedUser() && user.isImpersonating();
+ if (isImpersonating && user.getAccountId() != fromId) {
+ // This should not be possible, if this is the case it means the RequestContext is not
+ // set up correctly.
+ throw new EmailException(
+ String.format(
+ "User %s is sending email from %s, while acting on behalf of %s",
+ user.asIdentifiedUser().getRealUser().getAccountId(),
+ fromId,
+ user.getAccountId()));
+ }
if (senderPrefs != null && senderPrefs.getEmailStrategy() == CC_ON_OWN_COMMENTS) {
- // If we are impersonating a user, make sure they receive a CC of
- // this message so they can always review and audit what we sent
- // on their behalf to others.
+ // Include the sender in email if they enabled email notifications on their own
+ // comments.
//
logger.atFine().log(
"CC email sender %s because the email strategy of this user is %s",
fromUser.get().account().id(), CC_ON_OWN_COMMENTS);
add(RecipientType.CC, fromId);
+ } else if (isImpersonating) {
+ // If we are impersonating a user, make sure they receive a CC of
+ // this message regardless of email strategy, unless email notifications are explicitly
+ // disabled for this user. This way they can always review and audit what we sent
+ // on their behalf to others.
+ logger.atFine().log(
+ "CC email sender %s because the email is sent on behalf of and email notifications"
+ + " are enabled for this user.",
+ fromUser.get().account().id());
+ add(RecipientType.CC, fromId);
+
} else if (!notify.accounts().containsValue(fromId) && rcptTo.remove(fromId)) {
// If they don't want a copy, but we queued one up anyway,
- // drop them from the recipient lists.
+ // drop them from the recipient lists, but only if the user is not being impersonated.
//
logger.atFine().log(
"Not CCing email sender %s because the email strategy of this user is not %s but %s",
diff --git a/java/com/google/gerrit/server/submit/EmailMerge.java b/java/com/google/gerrit/server/submit/EmailMerge.java
index 109c9c3..3d38f6c 100644
--- a/java/com/google/gerrit/server/submit/EmailMerge.java
+++ b/java/com/google/gerrit/server/submit/EmailMerge.java
@@ -16,7 +16,6 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.server.CurrentUser;
@@ -42,7 +41,7 @@
EmailMerge create(
Project.NameKey project,
Change change,
- Account.Id submitter,
+ IdentifiedUser submitter,
NotifyResolver.Result notify,
RepoView repoView,
String stickyApprovalDiff);
@@ -51,12 +50,11 @@
private final ExecutorService sendEmailsExecutor;
private final MergedSender.Factory mergedSenderFactory;
private final ThreadLocalRequestContext requestContext;
- private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final MessageIdGenerator messageIdGenerator;
private final Project.NameKey project;
private final Change change;
- private final Account.Id submitter;
+ private final IdentifiedUser submitter;
private final NotifyResolver.Result notify;
private final RepoView repoView;
private final String stickyApprovalDiff;
@@ -66,18 +64,16 @@
@SendEmailExecutor ExecutorService executor,
MergedSender.Factory mergedSenderFactory,
ThreadLocalRequestContext requestContext,
- IdentifiedUser.GenericFactory identifiedUserFactory,
MessageIdGenerator messageIdGenerator,
@Assisted Project.NameKey project,
@Assisted Change change,
- @Assisted @Nullable Account.Id submitter,
+ @Assisted @Nullable IdentifiedUser submitter,
@Assisted NotifyResolver.Result notify,
@Assisted RepoView repoView,
@Assisted String stickyApprovalDiff) {
this.sendEmailsExecutor = executor;
this.mergedSenderFactory = mergedSenderFactory;
this.requestContext = requestContext;
- this.identifiedUserFactory = identifiedUserFactory;
this.messageIdGenerator = messageIdGenerator;
this.project = project;
this.change = change;
@@ -99,7 +95,7 @@
MergedSender emailSender =
mergedSenderFactory.create(project, change.getId(), Optional.of(stickyApprovalDiff));
if (submitter != null) {
- emailSender.setFrom(submitter);
+ emailSender.setFrom(submitter.getAccountId());
}
emailSender.setNotify(notify);
emailSender.setMessageId(
@@ -120,7 +116,7 @@
@Override
public CurrentUser getUser() {
if (submitter != null) {
- return identifiedUserFactory.create(submitter).getRealUser();
+ return submitter;
}
throw new OutOfScopeException("No user on email thread");
}
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index 09470d4..f181c36 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -503,7 +503,7 @@
.create(
ctx.getProject(),
toMerge.change(),
- submitter.accountId(),
+ args.caller,
ctx.getNotify(getId()),
ctx.getRepoView(),
stickyApprovalDiff)
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index 4a8c376..0f50797 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -89,6 +89,7 @@
.forUpdate()
.add(allow(Permission.FORGE_COMMITTER).ref("refs/*").group(REGISTERED_USERS))
.add(allow(Permission.SUBMIT).ref("refs/*").group(REGISTERED_USERS))
+ .add(allow(Permission.SUBMIT_AS).ref("refs/*").group(REGISTERED_USERS))
.add(allow(Permission.ABANDON).ref("refs/*").group(REGISTERED_USERS))
.add(allowLabel(LabelId.CODE_REVIEW).ref("refs/*").group(REGISTERED_USERS).range(-2, +2))
.update();
@@ -1623,6 +1624,75 @@
assertThat(sender).didNotSend();
}
+ @Test
+ public void mergeOnBehalfOfEmailEnabled_impersonatedOwnerNotified() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ // If notification is enabled, onBehalfOfUser is always notified.
+ setEmailStrategy(sc.owner, ENABLED);
+ merge(sc.changeId, other, sc.owner, ALL);
+ assertThat(sender)
+ .sent("merged", sc)
+ .to(sc.owner)
+ .cc(sc.reviewer, sc.ccer)
+ .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL)
+ .bcc(sc.starrer)
+ .bcc(ALL_COMMENTS, SUBMITTED_CHANGES)
+ .noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfEmailEnabled_impersonatedReviewerNotified() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ // If notification is enabled, onBehalfOfUser is always notified.
+ setEmailStrategy(sc.reviewer, ENABLED);
+ merge(sc.changeId, other, sc.reviewer, ALL);
+ assertThat(sender)
+ .sent("merged", sc)
+ .to(sc.owner)
+ .cc(sc.reviewer, sc.ccer)
+ .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL)
+ .bcc(sc.starrer)
+ .bcc(ALL_COMMENTS, SUBMITTED_CHANGES)
+ .noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfReviewerNotifyOwner_impersonatedReviewerInCC() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ setEmailStrategy(sc.reviewer, ENABLED);
+ // Even though Submit strategy is OWNER, impersonated reviewer is added to CC.
+ merge(sc.changeId, other, sc.reviewer, OWNER);
+ assertThat(sender).sent("merged", sc).to(sc.owner).cc(sc.reviewer).noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfOtherNotifyOwner_impersonatedOtherInCC() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ // Unrelated impersonated user is added to CC.
+ merge(sc.changeId, sc.reviewer, other, OWNER);
+ assertThat(sender).sent("merged", sc).to(sc.owner).cc(other).noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfEmailDisabled_doesNotNotify() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ setEmailStrategy(other, EmailStrategy.DISABLED);
+ merge(sc.changeId, sc.reviewer, other, OWNER);
+ assertThat(sender).sent("merged", sc).to(sc.owner).noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfNotifyNone() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ merge(sc.changeId, other, sc.owner, NONE);
+ assertThat(sender).didNotSend();
+ }
+
private void merge(String changeId, TestAccount by) throws Exception {
merge(changeId, by, ENABLED);
}
@@ -1648,6 +1718,20 @@
gApi.changes().id(changeId).current().submit(in);
}
+ private void merge(String changeId, TestAccount by, TestAccount onBehalfOf) throws Exception {
+ merge(changeId, by, onBehalfOf, /*notify=*/ null);
+ }
+
+ private void merge(
+ String changeId, TestAccount by, TestAccount onBehalfOf, @Nullable NotifyHandling notify)
+ throws Exception {
+ requestScopeOperations.setApiUser(by.id());
+ SubmitInput in = new SubmitInput();
+ in.notify = notify;
+ in.onBehalfOf = onBehalfOf.id().toString();
+ gApi.changes().id(changeId).current().submit(in);
+ }
+
private StagedChange stageChangeReadyForMerge() throws Exception {
StagedChange sc = stageReviewableChange();
requestScopeOperations.setApiUser(sc.reviewer.id());
