Merge "Document the permissions that are required for rebasing a change"
diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
index d298f1c..ec81cf6 100644
--- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java
+++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -24,6 +24,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Address;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeSizeBucket;
@@ -465,7 +466,18 @@
}
@Override
- protected boolean isVisibleTo(Account.Id to) throws PermissionBackendException {
+ protected boolean isRecipientAllowed(Address addr) throws PermissionBackendException {
+ if (!projectState.statePermitsRead()) {
+ return false;
+ }
+ return args.permissionBackend
+ .user(args.anonymousUser.get())
+ .change(changeData)
+ .test(ChangePermission.READ);
+ }
+
+ @Override
+ protected boolean isRecipientAllowed(Account.Id to) throws PermissionBackendException {
if (!projectState.statePermitsRead()) {
return false;
}
diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
index fa4f04e..d00c874 100644
--- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
+++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -528,6 +528,45 @@
* Adds a recipient that the email will be sent to.
*
* @param rt category of recipient (TO, CC, BCC)
+ * @param addr Name and email of the recipient.
+ */
+ public final void addByEmail(RecipientType rt, Address addr) {
+ addByEmail(rt, addr, false);
+ }
+
+ /**
+ * Adds a recipient that the email will be sent to.
+ *
+ * @param rt category of recipient (TO, CC, BCC).
+ * @param addr Name and email of the recipient.
+ * @param override if the recipient was added previously and override is false no change is made
+ * regardless of {@code rt}.
+ */
+ public final void addByEmail(RecipientType rt, Address addr, boolean override) {
+ try {
+ if (isRecipientAllowed(addr)) {
+ add(rt, addr, override);
+ }
+ } catch (PermissionBackendException e) {
+ logger.atSevere().withCause(e).log("Error checking permissions for email address: %s", addr);
+ }
+ }
+
+ /**
+ * Returns whether this email is allowed to be sent to the given address
+ *
+ * @param addr email address of recipient.
+ * @throws PermissionBackendException thrown if checking a permission fails due to an error in the
+ * permission backend
+ */
+ protected boolean isRecipientAllowed(Address addr) throws PermissionBackendException {
+ return true;
+ }
+
+ /**
+ * Adds a recipient that the email will be sent to.
+ *
+ * @param rt category of recipient (TO, CC, BCC)
* @param to Gerrit Account of the recipient.
*/
protected void addByAccountId(RecipientType rt, Account.Id to) {
@@ -544,45 +583,27 @@
*/
protected void addByAccountId(RecipientType rt, Account.Id to, boolean override) {
try {
- if (!rcptTo.contains(to) && isVisibleTo(to)) {
+ if (!rcptTo.contains(to) && isRecipientAllowed(to)) {
rcptTo.add(to);
- addByEmail(rt, toAddress(to), override);
+ add(rt, toAddress(to), override);
}
} catch (PermissionBackendException e) {
- logger.atSevere().withCause(e).log("Error reading database for account: %s", to);
+ logger.atSevere().withCause(e).log("Error checking permissions for account: %s", to);
}
}
/**
- * Returns whether this email is visible to the given account
+ * Returns whether this email is allowed to be sent to the given account
*
* @param to account.
* @throws PermissionBackendException thrown if checking a permission fails due to an error in the
* permission backend
*/
- protected boolean isVisibleTo(Account.Id to) throws PermissionBackendException {
+ protected boolean isRecipientAllowed(Account.Id to) throws PermissionBackendException {
return true;
}
- /**
- * Adds a recipient that the email will be sent to.
- *
- * @param rt category of recipient (TO, CC, BCC)
- * @param addr Name and email of the recipient.
- */
- public final void addByEmail(RecipientType rt, Address addr) {
- addByEmail(rt, addr, false);
- }
-
- /**
- * Adds a recipient that the email will be sent to.
- *
- * @param rt category of recipient (TO, CC, BCC).
- * @param addr Name and email of the recipient.
- * @param override if the recipient was added previously and override is false no change is made
- * regardless of {@code rt}.
- */
- public final void addByEmail(RecipientType rt, Address addr, boolean override) {
+ private final void add(RecipientType rt, Address addr, boolean override) {
if (addr != null && addr.email() != null && addr.email().length() > 0) {
if (!args.validator.isValid(addr.email())) {
logger.atWarning().log("Not emailing %s (invalid email address)", addr.email());
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index cced47f..28c795d 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.ABANDONED_CHANGES;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.ALL_COMMENTS;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.NEW_CHANGES;
@@ -28,6 +29,7 @@
import static com.google.gerrit.extensions.api.changes.NotifyHandling.OWNER_REVIEWERS;
import static com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy.CC_ON_OWN_COMMENTS;
import static com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy.ENABLED;
+import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
import static com.google.gerrit.server.project.testing.TestLabels.value;
@@ -306,6 +308,25 @@
addReviewerToReviewableChange(batch());
}
+ @Test
+ public void addReviewerToChangeNoAnonymousUsersNotified() throws Exception {
+ StagedChange sc = stageReviewableChange();
+ // Remove read permission for anonymous users.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/*").group(ANONYMOUS_USERS))
+ .add(allow(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+
+ TestAccount reviewer = accountCreator.create("added", "added@example.com", "added", null);
+ addReviewer(singly(), sc.changeId, sc.owner, reviewer.email());
+
+ // No BY_EMAIL cc's.
+ assertThat(sender).sent("newchange", sc).to(reviewer).cc(sc.reviewer).noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
private void addReviewerToReviewableChangeByOwnerCcingSelf(Adder adder) throws Exception {
StagedChange sc = stageReviewableChange();
TestAccount reviewer = accountCreator.create("added", "added@example.com", "added", null);
@@ -672,6 +693,30 @@
}
@Test
+ public void commentOnChangeNotVisibleToAnonymousByReviewer() throws Exception {
+ StagedChange sc = stageReviewableChange();
+
+ // Remove read permission for anonymous users.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/*").group(ANONYMOUS_USERS))
+ .add(allow(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+
+ review(sc.reviewer, sc.changeId, ENABLED);
+ // Not cc'ed to BY_EMAIL added addresses.
+ assertThat(sender)
+ .sent("comment", sc)
+ .to(sc.owner)
+ .cc(sc.ccer)
+ .bcc(sc.starrer)
+ .bcc(ALL_COMMENTS)
+ .noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
public void commentOnReviewableChangeByOwnerCcingSelf() throws Exception {
StagedChange sc = stageReviewableChange();
review(sc.owner, sc.changeId, CC_ON_OWN_COMMENTS);
diff --git a/plugins/reviewnotes b/plugins/reviewnotes
index 10db2cf..158435d 160000
--- a/plugins/reviewnotes
+++ b/plugins/reviewnotes
@@ -1 +1 @@
-Subproject commit 10db2cf772989d031c6f3558010c51fe07cf9722
+Subproject commit 158435d2aa9f8729e4e78835969e54701af9203b
