Merge branch 'stable-3.4' into stable-3.5

* stable-3.4:
  Fix paginationType NONE to run further queries only if needed
  Fix detection of invalid SSH key algorithm
  Demonstrate SshKeyUtil fails to validate invalid SSH keys
  Move comment of PaginatingSource.read()
  Fix visibility filter for changes when paginationType NONE
  Add tests for pagination to back fill the results

Release-Notes: skip
Change-Id: I9df29c9ae3b37026c9866fd97b6bb7f8dbb0dde9

10 files changed, 204 insertions, 34 deletions

diff --git a/java/com/google/gerrit/index/query/IndexedQuery.java b/java/com/google/gerrit/index/query/IndexedQuery.java
index df14e295cc..4bc8043ff7 100644
--- a/java/com/google/gerrit/index/query/IndexedQuery.java
+++ b/java/com/google/gerrit/index/query/IndexedQuery.java
@@ -87,6 +87,12 @@ public class IndexedQuery<I, T> extends Predicate<T> implements DataSource<T>, P
   }
 
   @Override
+  public ResultSet<T> restart(int start) {
+    opts = opts.withStart(start);
+    return search();
+  }
+
+  @Override
   public ResultSet<T> restart(int start, int pageSize) {
     opts = opts.withStart(start).withPageSize(pageSize);
     return search();
diff --git a/java/com/google/gerrit/index/query/Paginated.java b/java/com/google/gerrit/index/query/Paginated.java
index 552199093b..1065019c12 100644
--- a/java/com/google/gerrit/index/query/Paginated.java
+++ b/java/com/google/gerrit/index/query/Paginated.java
@@ -19,6 +19,8 @@ import com.google.gerrit.index.QueryOptions;
 public interface Paginated<T> {
   QueryOptions getOptions();
 
+  ResultSet<T> restart(int start);
+
   ResultSet<T> restart(int start, int pageSize);
 
   ResultSet<T> restart(Object searchAfter, int pageSize);
diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java
index 03d749ac14..0ce0cc9dbb 100644
--- a/java/com/google/gerrit/index/query/PaginatingSource.java
+++ b/java/com/google/gerrit/index/query/PaginatingSource.java
@@ -63,42 +63,54 @@ public class PaginatingSource<T> implements DataSource<T> {
             pageResultSize++;
           }
 
-          if (last != null
-              && source instanceof Paginated
-              // TODO: this fix is only for the stable branches and the real refactoring would be to
-              // restore the logic
-              // for the filtering in AndSource (L58 - 64) as per
-              // https://gerrit-review.googlesource.com/c/gerrit/+/345634/9
-              && !indexConfig.paginationType().equals(PaginationType.NONE)) {
+          if (last != null && source instanceof Paginated) {
             // Restart source and continue if we have not filled the
             // full limit the caller wants.
-            //
             @SuppressWarnings("unchecked")
             Paginated<T> p = (Paginated<T>) source;
             QueryOptions opts = p.getOptions();
             final int limit = opts.limit();
-            int pageSize = opts.pageSize();
-            int pageSizeMultiplier = opts.pageSizeMultiplier();
-            Object searchAfter = resultSet.searchAfter();
-            int nextStart = pageResultSize;
-            while (pageResultSize == pageSize && r.size() <= limit) { // get 1 more than the limit
-              pageSize = getNextPageSize(pageSize, pageSizeMultiplier);
-              ResultSet<T> next =
-                  indexConfig.paginationType().equals(PaginationType.SEARCH_AFTER)
-                      ? p.restart(searchAfter, pageSize)
-                      : p.restart(nextStart, pageSize);
-              pageResultSize = 0;
-              for (T data : buffer(next)) {
-                if (match(data)) {
-                  r.add(data);
+
+            // TODO: this fix is only for the stable branches and the real refactoring would be to
+            // restore the logic
+            // for the filtering in AndSource (L58 - 64) as per
+            // https://gerrit-review.googlesource.com/c/gerrit/+/345634/9
+            if (!indexConfig.paginationType().equals(PaginationType.NONE)) {
+              int pageSize = opts.pageSize();
+              int pageSizeMultiplier = opts.pageSizeMultiplier();
+              Object searchAfter = resultSet.searchAfter();
+              int nextStart = pageResultSize;
+              while (pageResultSize == pageSize && r.size() <= limit) { // get 1 more than the limit
+                pageSize = getNextPageSize(pageSize, pageSizeMultiplier);
+                ResultSet<T> next =
+                    indexConfig.paginationType().equals(PaginationType.SEARCH_AFTER)
+                        ? p.restart(searchAfter, pageSize)
+                        : p.restart(nextStart, pageSize);
+                pageResultSize = 0;
+                for (T data : buffer(next)) {
+                  if (match(data)) {
+                    r.add(data);
+                  }
+                  pageResultSize++;
+                  if (r.size() > limit) {
+                    break;
+                  }
                 }
-                pageResultSize++;
-                if (r.size() > limit) {
-                  break;
+                nextStart += pageResultSize;
+                searchAfter = next.searchAfter();
+              }
+            } else {
+              int nextStart = pageResultSize;
+              while (pageResultSize == limit && r.size() < limit) {
+                ResultSet<T> next = p.restart(nextStart);
+                for (T data : buffer(next)) {
+                  if (match(data)) {
+                    r.add(data);
+                  }
+                  pageResultSize++;
                 }
+                nextStart += pageResultSize;
               }
-              nextStart += pageResultSize;
-              searchAfter = next.searchAfter();
             }
           }
 
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index 5e3e0e2ff3..07d133405f 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -426,11 +426,11 @@ public class LuceneChangeIndex implements ChangeIndex {
       IndexSearcher[] searchers = new IndexSearcher[indexes.size()];
       Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex = new HashMap<>();
       try {
-        int realPageSize = opts.start() + opts.pageSize();
-        if (Integer.MAX_VALUE - opts.pageSize() < opts.start()) {
-          realPageSize = Integer.MAX_VALUE;
+        int pageLimit = AbstractLuceneIndex.getLimitBasedOnPaginationType(opts, opts.pageSize());
+        int queryLimit = opts.start() + pageLimit;
+        if (Integer.MAX_VALUE - pageLimit < opts.start()) {
+          queryLimit = Integer.MAX_VALUE;
         }
-        int queryLimit = AbstractLuceneIndex.getLimitBasedOnPaginationType(opts, realPageSize);
         List<TopFieldDocs> hits = new ArrayList<>();
         int searchAfterHitsCount = 0;
         for (int i = 0; i < indexes.size(); i++) {
@@ -438,7 +438,7 @@ public class LuceneChangeIndex implements ChangeIndex {
           searchers[i] = subIndex.acquire();
           if (isSearchAfterPagination) {
             ScoreDoc searchAfter = getSearchAfter(subIndex);
-            int maxRemainingHits = realPageSize - searchAfterHitsCount;
+            int maxRemainingHits = queryLimit - searchAfterHitsCount;
             if (maxRemainingHits > 0) {
               TopFieldDocs subIndexHits =
                   searchers[i].searchAfter(
diff --git a/java/com/google/gerrit/sshd/InvalidKeyAlgorithmException.java b/java/com/google/gerrit/sshd/InvalidKeyAlgorithmException.java
new file mode 100644
index 0000000000..5f09658ba2
--- /dev/null
+++ b/java/com/google/gerrit/sshd/InvalidKeyAlgorithmException.java
@@ -0,0 +1,44 @@
+// Copyright (C) 2024 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.sshd;
+
+import java.security.PublicKey;
+import java.security.spec.InvalidKeySpecException;
+
+public class InvalidKeyAlgorithmException extends InvalidKeySpecException {
+  private final String invalidKeyAlgo;
+  private final String expectedKeyAlgo;
+  private final PublicKey publicKey;
+
+  public InvalidKeyAlgorithmException(
+      String invalidKeyAlgo, String expectedKeyAlgo, PublicKey publicKey) {
+    super("Key algorithm mismatch: expected " + expectedKeyAlgo + " but got " + invalidKeyAlgo);
+    this.invalidKeyAlgo = invalidKeyAlgo;
+    this.expectedKeyAlgo = expectedKeyAlgo;
+    this.publicKey = publicKey;
+  }
+
+  public String getInvalidKeyAlgo() {
+    return invalidKeyAlgo;
+  }
+
+  public String getExpectedKeyAlgo() {
+    return expectedKeyAlgo;
+  }
+
+  public PublicKey getPublicKey() {
+    return publicKey;
+  }
+}
diff --git a/java/com/google/gerrit/sshd/SshKeyCacheImpl.java b/java/com/google/gerrit/sshd/SshKeyCacheImpl.java
index 628a0503c0..58e331bb65 100644
--- a/java/com/google/gerrit/sshd/SshKeyCacheImpl.java
+++ b/java/com/google/gerrit/sshd/SshKeyCacheImpl.java
@@ -19,6 +19,7 @@ import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USE
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.exceptions.InvalidSshKeyException;
 import com.google.gerrit.server.account.AccountSshKey;
 import com.google.gerrit.server.account.VersionedAuthorizedKeys;
 import com.google.gerrit.server.account.externalids.ExternalId;
@@ -144,7 +145,16 @@ public class SshKeyCacheImpl implements SshKeyCache {
         // to do with the key object, and instead we must abort this load.
         //
         throw e;
-      } catch (Exception e) {
+      } catch (InvalidKeyAlgorithmException e) {
+        logger.atWarning().withCause(e).log(
+            "SSH key %d of account %s has an invalid algorithm %s: fixing the algorithm to %s",
+            k.seq(), k.accountId(), e.getInvalidKeyAlgo(), e.getExpectedKeyAlgo());
+        if (fixKeyAlgorithm(k, e.getExpectedKeyAlgo())) {
+          kl.add(new SshKeyCacheEntry(k.accountId(), e.getPublicKey()));
+        } else {
+          markInvalid(k);
+        }
+      } catch (Throwable e) {
         markInvalid(k);
       }
     }
@@ -158,5 +168,20 @@ public class SshKeyCacheImpl implements SshKeyCache {
             "Failed to mark SSH key %d of account %s invalid", k.seq(), k.accountId());
       }
     }
+
+    private boolean fixKeyAlgorithm(AccountSshKey k, String keyAlgo) {
+      try {
+        logger.atInfo().log(
+            "Fixing SSH key %d of account %s algorithm to %s", k.seq(), k.accountId(), keyAlgo);
+        authorizedKeys.deleteKey(k.accountId(), k.seq());
+        String sshKey = k.sshPublicKey();
+        authorizedKeys.addKey(k.accountId(), keyAlgo + sshKey.substring(sshKey.indexOf(' ')));
+        return true;
+      } catch (IOException | ConfigInvalidException | InvalidSshKeyException e) {
+        logger.atSevere().withCause(e).log(
+            "Failed to fix SSH key %d of account %s with algo %s", k.seq(), k.accountId(), keyAlgo);
+        return false;
+      }
+    }
   }
 }
diff --git a/java/com/google/gerrit/sshd/SshUtil.java b/java/com/google/gerrit/sshd/SshUtil.java
index abbd81d83a..29d0e90f9f 100644
--- a/java/com/google/gerrit/sshd/SshUtil.java
+++ b/java/com/google/gerrit/sshd/SshUtil.java
@@ -57,7 +57,12 @@ public class SshUtil {
         throw new InvalidKeySpecException("No key string");
       }
       final byte[] bin = BaseEncoding.base64().decode(s);
-      return new ByteArrayBuffer(bin).getRawPublicKey();
+      String publicKeyAlgo = new ByteArrayBuffer(bin).getString();
+      PublicKey publicKey = new ByteArrayBuffer(bin).getRawPublicKey();
+      if (!key.algorithm().equals(publicKeyAlgo)) {
+        throw new InvalidKeyAlgorithmException(key.algorithm(), publicKeyAlgo, publicKey);
+      }
+      return publicKey;
     } catch (RuntimeException | SshException e) {
       throw new InvalidKeySpecException("Cannot parse key", e);
     }
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
index 9717bfb919..54d0c8786b 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
@@ -20,6 +20,7 @@ import static com.google.gerrit.common.data.GlobalCapability.QUERY_LIMIT;
 import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
 import static com.google.gerrit.testing.GerritJUnit.assertThrows;
 
+import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.server.config.AllProjectsName;
@@ -98,4 +99,27 @@ public abstract class LuceneQueryChangesTest extends AbstractQueryChangesTest {
     Change[] expected = new Change[] {change6, change5, change4, change3, change2, change1};
     assertQuery(newQuery("project:repo").withNoLimit(), expected);
   }
+
+  @Test
+  public void skipChangesNotVisible() throws Exception {
+    // create 1 new change on a repo
+    TestRepository<Repo> repo = createProject("repo");
+    Change visibleChange = insert(repo, newChangeWithStatus(repo, Change.Status.NEW));
+    Change[] expected = new Change[] {visibleChange};
+
+    // pagination does not need to restart the datasource, the request is fulfilled
+    assertQuery(newQuery("status:new").withLimit(1), expected);
+
+    // create 2 new private changes
+    Account.Id user2 =
+        accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
+
+    Change invisibleChange1 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW), user2);
+    Change invisibleChange2 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW), user2);
+    gApi.changes().id(invisibleChange1.getChangeId()).setPrivate(true, null);
+    gApi.changes().id(invisibleChange2.getChangeId()).setPrivate(true, null);
+
+    // pagination should back-fill when the results skipped because of the visibility
+    assertQuery(newQuery("status:new").withLimit(1), expected);
+  }
 }
diff --git a/javatests/com/google/gerrit/sshd/BUILD b/javatests/com/google/gerrit/sshd/BUILD
index 3e11ff22e2..44b9c62a79 100644
--- a/javatests/com/google/gerrit/sshd/BUILD
+++ b/javatests/com/google/gerrit/sshd/BUILD
@@ -4,8 +4,11 @@ junit_tests(
     name = "sshd_tests",
     srcs = glob(["**/*.java"]),
     deps = [
+        "//java/com/google/gerrit/entities",
         "//java/com/google/gerrit/extensions:api",
+        "//java/com/google/gerrit/server",
         "//java/com/google/gerrit/sshd",
+        "//java/com/google/gerrit/testing:gerrit-test-util",
         "//lib/mina:sshd",
         "//lib/truth",
     ],
diff --git a/javatests/com/google/gerrit/sshd/SshUtilTest.java b/javatests/com/google/gerrit/sshd/SshUtilTest.java
new file mode 100644
index 0000000000..1585bc3a97
--- /dev/null
+++ b/javatests/com/google/gerrit/sshd/SshUtilTest.java
@@ -0,0 +1,49 @@
+// Copyright (C) 2024 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.sshd;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.server.account.AccountSshKey;
+import java.security.spec.InvalidKeySpecException;
+import org.junit.Test;
+
+public class SshUtilTest {
+  private static final Account.Id TEST_ACCOUNT_ID = Account.id(1);
+  private static final int TEST_SSHKEY_SEQUENCE = 1;
+  private static final String INVALID_ALGO = "invalid-algo";
+  private static final String VALID_OPENSSH_RSA_KEY =
+      "AAAAB3NzaC1yc2EAAAABIwAAAIEA0R66EoZ7hFp81w9sAJqu34UFyE+w36H/mobUqnT5Lns7PcTOJh3sgMJAlswX2lFAWqvF2gd2PRMpMhbfEU4iq2SfY8x+RDCJ4ZQWESln/587T41BlQjOXzu3W1bqgmtHnRCte3DjyWDvM/fucnUMSwOgP+FVEZCLTrk3thLMWsU=";
+  private static final Object VALID_SSH_RSA_ALGO = "ssh-rsa";
+
+  @Test
+  public void shouldFailParsingOpenSshKeyWithInvalidAlgo() {
+    String sshKeyWithInvalidAlgo = String.format("%s %s", INVALID_ALGO, VALID_OPENSSH_RSA_KEY);
+    AccountSshKey sshKey =
+        AccountSshKey.create(TEST_ACCOUNT_ID, TEST_SSHKEY_SEQUENCE, sshKeyWithInvalidAlgo);
+    assertThrows(InvalidKeySpecException.class, () -> SshUtil.parse(sshKey));
+  }
+
+  @Test
+  public void shouldParseSshKeyWithAlgoMatchingKey() {
+    String sshKeyWithValidKeyAlgo =
+        String.format("%s %s", VALID_SSH_RSA_ALGO, VALID_OPENSSH_RSA_KEY);
+    AccountSshKey sshKey =
+        AccountSshKey.create(TEST_ACCOUNT_ID, TEST_SSHKEY_SEQUENCE, sshKeyWithValidKeyAlgo);
+    assertThat(sshKey).isNotNull();
+  }
+}