Merge branch 'stable-2.9' into stable-2.10

* stable-2.9:
  Fix SubmoduleOp tests
  Fix quoted-printable encoding of e-mail addresses
  Fix incorrect submodule subscriptions
  Remove 'send email' checkbox from reply box on change screen
  Update system group documentation
  Consider rule action while constructing local owners list
  Increase the size of HTTP passwords
  Do not throw away random bytes from the CSPRNG

Conflicts:
	gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
	gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java
	gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java

Change-Id: If086a9021aabb512023753d8dea59f50799cd91a

Documentation/access-control.txt

@@ -11,35 +11,13 @@ users.
 
 Gerrit comes with the following system groups:
 
-* Administrators
 * Anonymous Users
 * Change Owner
-* Non-Interactive Users
 * Project Owners
 * Registered Users
 
 The system groups are assigned special access and membership management
-privileges.  The identity of these groups is set in the `system_config`
-table within the database, so the groups can be renamed after installation
-if desired.
-
-
-[[administrators]]
-=== Administrators
-
-This is the Gerrit "root" identity.
-
-Users in the 'Administrators' group can perform any action under
-the Admin menu, to any group or project, without further validation
-or any other access controls.  In most installations only those
-users who have direct filesystem and database access would be
-placed into this group.
-
-Membership in the 'Administrators' group does not imply any other
-access rights.  Administrators do not automatically get code review
-approval or submit rights in projects.  This is a feature designed
-to permit administrative users to otherwise access Gerrit as any
-other normal user would, without needing two different accounts.
+privileges.
 
 
 [[anonymous_users]]
@@ -57,21 +35,6 @@ to grant `Read` access to this group as Gerrit requires an account
 identity for all other operations.
 
 
-[[non-interactive_users]]
-=== Non-Interactive Users
-
-This is an internal user group, members of this group are not expected
-to perform interactive operations on the Gerrit web front-end.
-
-However, sometimes such a user may need a separate thread pool in
-order to prevent it from grabbing threads from the interactive users.
-
-These users live in a second thread pool, which separates operations
-made by the non-interactive users from the ones made by the interactive
-users. This ensures that the interactive users can keep working when
-resources are tight.
-
-
 [[project_owners]]
 === Project Owners
 
@@ -120,6 +83,59 @@ Registered users are always permitted to make and publish comments
 on any change in any project they have `Read` access to.
 
 
+== Predefined Groups
+
+Predefined groups differs from system groups by the fact that they
+exist in the ACCOUNT_GROUPS table (like normal groups) but predefined groups
+are created on Gerrit site initialization and unique UUIDs are assigned
+to those groups. These UUIDs are different on different Gerrit sites.
+
+Gerrit comes with two predefined groups:
+
+* Administrators
+* Non-Interactive Users
+
+
+[[administrators]]
+=== Administrators
+
+This is the Gerrit "root" identity. The capability
+link:access-control.html#capability_administrateServer['Administrate Server']
+is assigned to this predefined group on Gerrit site creation.
+
+Users in the 'Administrators' group can perform any action under
+the Admin menu, to any group or project, without further validation
+or any other access controls.  In most installations only those
+users who have direct filesystem and database access would be
+placed into this group.
+
+Membership in the 'Administrators' group does not imply any other
+access rights.  Administrators do not automatically get code review
+approval or submit rights in projects.  This is a feature designed
+to permit administrative users to otherwise access Gerrit as any
+other normal user would, without needing two different accounts.
+
+
+[[non-interactive_users]]
+=== Non-Interactive Users
+
+This is the Gerrit "batch" identity. The capabilities
+link:access-control.html#capability_priority['Priority BATCH'] and
+link:access-control.html#capability_streamEvents['Stream Events']
+are assigned to this predefined group on Gerrit site creation.
+
+The members of this group are not expected to perform interactive
+operations on the Gerrit web front-end.
+
+However, sometimes such a user may need a separate thread pool in
+order to prevent it from grabbing threads from the interactive users.
+
+These users live in a second thread pool, which separates operations
+made by the non-interactive users from the ones made by the interactive
+users. This ensures that the interactive users can keep working when
+resources are tight.
+
+
 == Account Groups
 
 Account groups contain a list of zero or more user account members,

Documentation/rest-api-accounts.txt

@@ -269,7 +269,7 @@ Retrieves the HTTP password of an account.
   Content-Type: application/json;charset=UTF-8
 
   )]}'
-  "ETxgpih8xrNs"
+  "Qmxlc21ydCB1YmVyIGFsbGVzIGluIGRlciBXZWx0IQ"
 ----
 
 If the account does not have an HTTP password the response is `404 Not Found`.

gerrit-server/src/main/java/com/google/gerrit/server/account/PutHttpPassword.java

@@ -45,7 +45,7 @@ public class PutHttpPassword implements RestModifyView<AccountResource, Input> {
     public boolean generate;
   }
 
-  private static final int LEN = 12;
+  private static final int LEN = 31;
   private static final SecureRandom rng;
 
   static {
@@ -126,8 +126,8 @@ public class PutHttpPassword implements RestModifyView<AccountResource, Input> {
     rng.nextBytes(rand);
 
     byte[] enc = Base64.encodeBase64(rand, false);
-    StringBuilder r = new StringBuilder(LEN);
-    for (int i = 0; i < LEN; i++) {
+    StringBuilder r = new StringBuilder(enc.length);
+    for (int i = 0; i < enc.length; i++) {
       if (enc[i] == '=') {
         break;
       }

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -66,6 +66,7 @@ import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.reviewdb.client.SubmoduleSubscription;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalCopier;
 import com.google.gerrit.server.ApprovalsUtil;
@@ -612,6 +613,17 @@ public class ReceiveCommits {
                 break;
 
               case DELETE:
+                ResultSet<SubmoduleSubscription> submoduleSubscriptions = null;
+                Branch.NameKey projRef = new Branch.NameKey(project.getNameKey(),
+                    c.getRefName());
+                try {
+                  submoduleSubscriptions =
+                      db.submoduleSubscriptions().bySuperProject(projRef);
+                  db.submoduleSubscriptions().delete(submoduleSubscriptions);
+                } catch (OrmException e) {
+                  log.error("Cannot delete submodule subscription(s) of branch "
+                      + projRef + ": " + submoduleSubscriptions, e);
+                }
                 break;
             }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.git;
 
 import com.google.common.base.Strings;
+import com.google.common.collect.Lists;
 import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Account;
@@ -233,23 +234,36 @@ public class SubmoduleOp {
         }
 
         // update subscribers of this module
+        List<SubmoduleSubscription> incorrectSubscriptions = Lists.newLinkedList();
         for (final SubmoduleSubscription s : subscribers) {
-          if (!updatedSubscribers.add(s.getSuperProject())) {
-            log.error("Possible circular subscription involving "
-                + s.toString());
-          } else {
+          try {
+            if (!updatedSubscribers.add(s.getSuperProject())) {
+              log.error("Possible circular subscription involving " + s);
+            } else {
 
             Map<Branch.NameKey, ObjectId> modules = new HashMap<>(1);
-            modules.put(updatedBranch, mergedCommit);
+              modules.put(updatedBranch, mergedCommit);
 
             Map<Branch.NameKey, String> paths = new HashMap<>(1);
-            paths.put(updatedBranch, s.getPath());
-
-            try {
+              paths.put(updatedBranch, s.getPath());
               updateGitlinks(s.getSuperProject(), myRw, modules, paths, msgbuf);
-            } catch (SubmoduleException e) {
-              throw e;
             }
+          } catch (SubmoduleException e) {
+              log.warn("Cannot update gitlinks for " + s + " due to " + e.getMessage());
+              incorrectSubscriptions.add(s);
+          } catch (Exception e) {
+              log.error("Cannot update gitlinks for " + s, e);
+          }
+        }
+
+        if (!incorrectSubscriptions.isEmpty()) {
+          try {
+            schema.submoduleSubscriptions().delete(incorrectSubscriptions);
+            log.info("Deleted incorrect submodule subscription(s) "
+                + incorrectSubscriptions);
+          } catch (OrmException e) {
+            log.error("Cannot delete submodule subscription(s) "
+                + incorrectSubscriptions, e);
           }
         }
       }
@@ -365,8 +379,8 @@ public class SubmoduleOp {
       // Recursive call: update subscribers of the subscriber
       updateSuperProjects(subscriber, recRw, commitId, msgbuf.toString());
     } catch (IOException e) {
-      logAndThrowSubmoduleException("Cannot update gitlinks for "
-          + subscriber.get(), e);
+        throw new SubmoduleException("Cannot update gitlinks for "
+            + subscriber.get(), e);
     } finally {
       if (recRw != null) {
         recRw.release();

gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailHeader.java

@@ -64,23 +64,47 @@ public abstract class EmailHeader {
     return false;
   }
 
+  static boolean needsQuotedPrintableWithinPhrase(final int cp) {
+    switch (cp) {
+      case '!':
+      case '*':
+      case '+':
+      case '-':
+      case '/':
+      case '=':
+      case '_':
+        return false;
+      default:
+        if (('a' <= cp && cp <= 'z')
+            || ('A' <= cp && cp <= 'Z')
+            || ('0' <= cp && cp <= '9')) {
+          return false;
+        } else {
+          return true;
+        }
+    }
+  }
+
   static java.lang.String quotedPrintable(java.lang.String value)
       throws UnsupportedEncodingException {
     final StringBuilder r = new StringBuilder();
-    final byte[] encoded = value.getBytes("UTF-8");
 
     r.append("=?UTF-8?Q?");
-    for (byte b : encoded) {
-      if (b == ' ') {
+    for (int i = 0; i < value.length(); i++) {
+      final int cp = value.codePointAt(i);
+      if (cp == ' ') {
         r.append('_');
 
-      } else if (b == ',' || b == '=' || b == '"' || b == '_' || b < ' ' || '~' <= b) {
-        r.append('=');
-        r.append(Integer.toHexString((b >>> 4) & 0x0f).toUpperCase());
-        r.append(Integer.toHexString(b & 0x0f).toUpperCase());
+      } else if (needsQuotedPrintableWithinPhrase(cp)) {
+        byte[] buf = new java.lang.String(Character.toChars(cp)).getBytes("UTF-8");
+        for (byte b: buf) {
+          r.append('=');
+          r.append(Integer.toHexString((b >>> 4) & 0x0f).toUpperCase());
+          r.append(Integer.toHexString(b & 0x0f).toUpperCase());
+        }
 
       } else {
-        r.append((char) b);
+        r.append(Character.toChars(cp));
       }
     }
     r.append("?=");

gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java

@@ -19,12 +19,14 @@ import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.reviewdb.client.SubmoduleSubscription;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.project.DeleteBranch.Input;
 import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.ResultSet;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
@@ -92,6 +94,9 @@ public class DeleteBranch implements RestModifyView<BranchResource, Input>{
         case FORCED:
           referenceUpdated.fire(rsrc.getNameKey(), u);
           hooks.doRefUpdatedHook(rsrc.getBranchKey(), u, identifiedUser.get().getAccount());
+          ResultSet<SubmoduleSubscription> submoduleSubscriptions =
+            dbProvider.get().submoduleSubscriptions().bySuperProject(rsrc.getBranchKey());
+          dbProvider.get().submoduleSubscriptions().delete(submoduleSubscriptions);
           break;
 
         case REJECTED_CURRENT_BRANCH:

gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.project;
 
+import static com.google.gerrit.common.data.PermissionRule.Action.ALLOW;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.base.Function;
@@ -142,7 +143,7 @@ public class ProjectState {
         if (owner != null) {
           for (PermissionRule rule : owner.getRules()) {
             GroupReference ref = rule.getGroup();
-            if (ref.getUUID() != null) {
+            if (rule.getAction() == ALLOW && ref.getUUID() != null) {
               groups.add(ref.getUUID());
             }
           }

gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java

@@ -14,13 +14,10 @@
 
 package com.google.gerrit.server.git;
 
+import static org.easymock.EasyMock.anyObject;
 import static org.easymock.EasyMock.capture;
-import static org.easymock.EasyMock.createNiceMock;
-import static org.easymock.EasyMock.createStrictMock;
 import static org.easymock.EasyMock.eq;
 import static org.easymock.EasyMock.expect;
-import static org.easymock.EasyMock.replay;
-import static org.easymock.EasyMock.verify;
 import static org.junit.Assert.assertEquals;
 
 import com.google.gerrit.common.ChangeHooks;
@@ -41,6 +38,8 @@ import com.google.gwtorm.server.StandardKeyEncoder;
 import com.google.inject.Provider;
 
 import org.easymock.Capture;
+import org.easymock.EasyMock;
+import org.easymock.IMocksControl;
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.dircache.DirCacheBuilder;
 import org.eclipse.jgit.dircache.DirCacheEntry;
@@ -75,6 +74,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
 
   private static final String newLine = System.getProperty("line.separator");
 
+  private IMocksControl mockMaker;
   private SchemaFactory<ReviewDb> schemaFactory;
   private SubmoduleSubscriptionAccess subscriptions;
   private ReviewDb schema;
@@ -89,23 +89,22 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
   public void setUp() throws Exception {
     super.setUp();
 
-    schemaFactory = createStrictMock(SchemaFactory.class);
-    schema = createStrictMock(ReviewDb.class);
-    subscriptions = createStrictMock(SubmoduleSubscriptionAccess.class);
-    urlProvider = createStrictMock(Provider.class);
-    repoManager = createStrictMock(GitRepositoryManager.class);
-    gitRefUpdated = createStrictMock(GitReferenceUpdated.class);
-    changeHooks = createNiceMock(ChangeHooks.class);
+    mockMaker = EasyMock.createStrictControl();
+    schemaFactory = mockMaker.createMock(SchemaFactory.class);
+    schema = mockMaker.createMock(ReviewDb.class);
+    subscriptions = mockMaker.createMock(SubmoduleSubscriptionAccess.class);
+    urlProvider = mockMaker.createMock(Provider.class);
+    repoManager = mockMaker.createMock(GitRepositoryManager.class);
+    gitRefUpdated = mockMaker.createMock(GitReferenceUpdated.class);
+    changeHooks = mockMaker.createMock(ChangeHooks.class);
   }
 
   private void doReplay() {
-    replay(schemaFactory, schema, subscriptions, urlProvider, repoManager,
-        gitRefUpdated);
+    mockMaker.replay();
   }
 
   private void doVerify() {
-    verify(schemaFactory, schema, subscriptions, urlProvider, repoManager,
-        gitRefUpdated);
+    mockMaker.verify();
   }
 
   /**
@@ -640,6 +639,8 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
     Capture<RefUpdate> ruCapture = new Capture<>();
     gitRefUpdated.fire(eq(targetBranchNameKey.getParentKey()),
         capture(ruCapture));
+    changeHooks.doRefUpdatedHook(eq(targetBranchNameKey),
+        anyObject(RefUpdate.class), EasyMock.<Account>isNull());
 
     expect(schema.submoduleSubscriptions()).andReturn(subscriptions);
     final ResultSet<SubmoduleSubscription> emptySubscriptions =
@@ -743,6 +744,8 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
     Capture<RefUpdate> ruCapture = new Capture<>();
     gitRefUpdated.fire(eq(targetBranchNameKey.getParentKey()),
         capture(ruCapture));
+    changeHooks.doRefUpdatedHook(eq(targetBranchNameKey),
+          anyObject(RefUpdate.class), EasyMock.<Account>isNull());
 
     expect(schema.submoduleSubscriptions()).andReturn(subscriptions);
     final ResultSet<SubmoduleSubscription> incorrectSubscriptions =

gerrit-server/src/test/java/com/google/gerrit/server/mail/AddressTest.java

@@ -130,6 +130,12 @@ public class AddressTest {
     assertEquals("=?UTF-8?Q?A_=E2=82=AC_B?= <a@a>", format("A \u20ac B", "a@a"));
   }
 
+  @Test
+  public void testToHeaderString_NameEmail7() {
+    assertEquals("=?UTF-8?Q?A_=E2=82=AC_B_=28Code_Review=29?= <a@a>",
+        format("A \u20ac B (Code Review)", "a@a"));
+  }
+
   @Test
   public void testToHeaderString_Email1() {
     assertEquals("a@a", format(null, "a@a"));

gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java

@@ -72,11 +72,23 @@ public class RefControlTest {
   public void testOwnerProject() {
     allow(local, OWNER, ADMIN, "refs/*");
 
-    ProjectControl uBlah = util.user(local, DEVS);
-    ProjectControl uAdmin = util.user(local, DEVS, ADMIN);
+    assertAdminsAreOwnersAndDevsAreNot();
+  }
 
-    assertFalse("not owner", uBlah.isOwner());
-    assertTrue("is owner", uAdmin.isOwner());
+  @Test
+  public void testDenyOwnerProject() {
+    allow(local, OWNER, ADMIN, "refs/*");
+    deny(local, OWNER, DEVS, "refs/*");
+
+    assertAdminsAreOwnersAndDevsAreNot();
+  }
+
+  @Test
+  public void testBlockOwnerProject() {
+    allow(local, OWNER, ADMIN, "refs/*");
+    block(local, OWNER, DEVS, "refs/*");
+
+    assertAdminsAreOwnersAndDevsAreNot();
   }
 
   @Test
@@ -520,4 +532,12 @@ public class RefControlTest {
     assertFalse("u can vote -2", range.contains(-2));
     assertFalse("u can vote +2", range.contains(2));
   }
+
+  private void assertAdminsAreOwnersAndDevsAreNot() {
+    ProjectControl uBlah = util.user(local, DEVS);
+    ProjectControl uAdmin = util.user(local, DEVS, ADMIN);
+
+    assertFalse("not owner", uBlah.isOwner());
+    assertTrue("is owner", uAdmin.isOwner());
+  }
 }