From 09cdcd503744c5ce234268fa75fcfb2e3d8c5259 Mon Sep 17 00:00:00 2001 From: Kaushik Lingarkar Date: Thu, 7 Mar 2024 09:26:59 -0800 Subject: Allow plugins to use PredicateArgs Change visibility of the PredicateArgs constructor to public. This will allow plugins implementing operators with args to use PredicateArgs instead of implementing their own arg parser. Release-Notes: skip Change-Id: Ief39f4606a96c24ae3ad58bd0031a825ce4cdcab --- java/com/google/gerrit/server/query/change/PredicateArgs.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/query/change/PredicateArgs.java b/java/com/google/gerrit/server/query/change/PredicateArgs.java index d82b9bc828..aa6bfbcec1 100644 --- a/java/com/google/gerrit/server/query/change/PredicateArgs.java +++ b/java/com/google/gerrit/server/query/change/PredicateArgs.java @@ -41,7 +41,7 @@ public class PredicateArgs { * * @param args arguments to be parsed */ - PredicateArgs(String args) throws QueryParseException { + public PredicateArgs(String args) throws QueryParseException { positional = new ArrayList<>(); keyValue = new HashMap<>(); -- cgit v1.2.3 From 242d514e6143b2d9c38b4bc27f4617289f27a853 Mon Sep 17 00:00:00 2001 From: Diego Zambelli Sessona Date: Thu, 14 Mar 2024 15:12:04 +0100 Subject: Add tests for pagination to back fill the results When querying Lucene, if we have not filled the full limit the caller wants, `PaginatingSource.read()` need to restart the source and continue. This should happen regardless of the paginationType. Bug: Issue 328958027 Release-Notes: skip Change-Id: I02bc97eeecde2149e374dd036b87dd4c06034f27 --- .../query/change/LuceneQueryChangesTest.java | 26 ++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java index 9717bfb919..4996ff95d7 100644 --- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java @@ -20,8 +20,10 @@ 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.account.AuthRequest; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.testing.InMemoryModule; import com.google.gerrit.testing.InMemoryRepositoryManager.Repo; @@ -31,6 +33,7 @@ import com.google.inject.Injector; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.revwalk.RevCommit; +import org.junit.Ignore; import org.junit.Test; public abstract class LuceneQueryChangesTest extends AbstractQueryChangesTest { @@ -98,4 +101,27 @@ public abstract class LuceneQueryChangesTest extends AbstractQueryChangesTest { Change[] expected = new Change[] {change6, change5, change4, change3, change2, change1}; assertQuery(newQuery("project:repo").withNoLimit(), expected); } + + @Ignore + public void skipChangesNotVisible() throws Exception { + // create 1 new change on a repo + TestRepository 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(AuthRequest.forUser("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); + } } -- cgit v1.2.3 From db911c50c6d87d92da8f0c78dcf9063974269a18 Mon Sep 17 00:00:00 2001 From: Diego Zambelli Sessona Date: Thu, 14 Mar 2024 16:26:27 +0100 Subject: Fix visibility filter for changes when paginationType NONE The introduction of paginationType NONE in change 379354 affected the API pagination. In particular, if some results are skipped because of of the visibility constraints, more changes need to be asked from the index, until we have filled the full limit the caller wants or if there are no more changes [1]. [1]: https://gerrit-review.googlesource.com/c/gerrit/+/379354/8/java/com/google/gerrit/index/query/PaginatingSource.java#72 Bug: Issue 328958027 Release-Notes: Fix NONE paginationType filtering of changes Change-Id: I3ccff7f8ba5a6d3903f9acbe3f5c439c55225ce2 --- .../google/gerrit/index/query/IndexedQuery.java | 6 ++ java/com/google/gerrit/index/query/Paginated.java | 2 + .../gerrit/index/query/PaginatingSource.java | 81 +++++++++++++--------- .../google/gerrit/lucene/LuceneChangeIndex.java | 10 +-- .../query/change/LuceneQueryChangesTest.java | 3 +- 5 files changed, 64 insertions(+), 38 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 @@ -86,6 +86,12 @@ public class IndexedQuery extends Predicate implements DataSource, P return source.readRaw(); } + @Override + public ResultSet restart(int start) { + opts = opts.withStart(start); + return search(); + } + @Override public ResultSet restart(int start, int pageSize) { opts = opts.withStart(start).withPageSize(pageSize); 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 { QueryOptions getOptions(); + ResultSet restart(int start); + ResultSet restart(int start, int pageSize); ResultSet 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 4eceb319e7..8847005629 100644 --- a/java/com/google/gerrit/index/query/PaginatingSource.java +++ b/java/com/google/gerrit/index/query/PaginatingSource.java @@ -55,47 +55,66 @@ public class PaginatingSource implements DataSource { List r = new ArrayList<>(); T last = null; int pageResultSize = 0; + boolean skipped = false; for (T data : buffer(resultSet)) { if (!isMatchable() || match(data)) { r.add(data); + } else { + skipped = true; } last = data; 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)) { - // Restart source and continue if we have not filled the - // full limit the caller wants. - // - @SuppressWarnings("unchecked") - Paginated p = (Paginated) 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 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); + 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 + if (!indexConfig.paginationType().equals(PaginationType.NONE)) { + // Restart source and continue if we have not filled the + // full limit the caller wants. + // + @SuppressWarnings("unchecked") + Paginated p = (Paginated) 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 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++; + } + nextStart += pageResultSize; + searchAfter = next.searchAfter(); + } + } else { + @SuppressWarnings("unchecked") + Paginated p = (Paginated) source; + while (skipped && r.size() < p.getOptions().limit() + start) { + skipped = false; + ResultSet next = p.restart(pageResultSize); + + for (T data : buffer(next)) { + if (match(data)) { + r.add(data); + } else { + skipped = true; + } + pageResultSize++; } - 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 ca73fdcff4..3fc4de2038 100644 --- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -464,11 +464,11 @@ public class LuceneChangeIndex implements ChangeIndex { IndexSearcher[] searchers = new IndexSearcher[indexes.size()]; Map 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 hits = new ArrayList<>(); int searchAfterHitsCount = 0; for (int i = 0; i < indexes.size(); i++) { @@ -476,7 +476,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/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java index 4996ff95d7..603564c59e 100644 --- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java @@ -33,7 +33,6 @@ import com.google.inject.Injector; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.revwalk.RevCommit; -import org.junit.Ignore; import org.junit.Test; public abstract class LuceneQueryChangesTest extends AbstractQueryChangesTest { @@ -102,7 +101,7 @@ public abstract class LuceneQueryChangesTest extends AbstractQueryChangesTest { assertQuery(newQuery("project:repo").withNoLimit(), expected); } - @Ignore + @Test public void skipChangesNotVisible() throws Exception { // create 1 new change on a repo TestRepository repo = createProject("repo"); -- cgit v1.2.3 From 371b0687e9ed9cc3ebb489e6f28e1058e064d807 Mon Sep 17 00:00:00 2001 From: Diego Zambelli Sessona Date: Tue, 19 Mar 2024 15:39:43 +0100 Subject: Move comment of PaginatingSource.read() Move the comment about restarting the DataSource for back filling results skipped for visibility. The comment in fact is valid for any PaginationType specified. Adress the comment in change 413358 [1]. [1]: https://gerrit-review.googlesource.com/c/gerrit/+/413358/comment/d7a90bb0_0f8ebc9f/ Release-Notes: skip Change-Id: I9826b8be4005a946c7952ae89eb81f8c191997e3 --- java/com/google/gerrit/index/query/PaginatingSource.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java index 8847005629..9ec64e63e0 100644 --- a/java/com/google/gerrit/index/query/PaginatingSource.java +++ b/java/com/google/gerrit/index/query/PaginatingSource.java @@ -67,14 +67,14 @@ public class PaginatingSource implements DataSource { } if (last != null && source instanceof Paginated) { + // Restart source and continue if we have not filled the + // full limit the caller wants. + // // 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)) { - // Restart source and continue if we have not filled the - // full limit the caller wants. - // @SuppressWarnings("unchecked") Paginated p = (Paginated) source; QueryOptions opts = p.getOptions(); -- cgit v1.2.3 From 78812bd5ed5c1b8f24a29c4a19cb25437aa45384 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 21 Mar 2024 20:49:04 +0000 Subject: Demonstrate SshKeyUtil fails to validate invalid SSH keys The SshKeyUtil has always missed the validation of the SSH key algo specified as a prefix of the Base-64 encoded key. Whilst the behaviour has always been the same since 2008, it is nonetheless buggy and should be validated for preventing the storage of invalid keys. TODO: Mark the SSH key validation test as disabled for allowing the build to succeed. The test can be enabled back again once the validation has been amended to verify the key algorithm. Bug: Issue 330758152 Release-Notes: skip Change-Id: I42b1c6474fa876828e5353e81e97b21b981665f9 (cherry picked from commit 4f3ff5abdb18ad34078d8dd2f0ad1d4e610957d1) --- javatests/com/google/gerrit/sshd/BUILD | 3 ++ javatests/com/google/gerrit/sshd/SshUtilTest.java | 51 +++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 javatests/com/google/gerrit/sshd/SshUtilTest.java 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..3225230bbd --- /dev/null +++ b/javatests/com/google/gerrit/sshd/SshUtilTest.java @@ -0,0 +1,51 @@ +// 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.Ignore; +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"; + + @Ignore("To be enabled once the SSH key parsing is fixed") + @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(); + } +} -- cgit v1.2.3 From 60276878a34403dca79d208881577d81467ce399 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 21 Mar 2024 22:05:36 +0000 Subject: Fix detection of invalid SSH key algorithm Verify that the OpenSSH key algorithm matches the one associated with the public key. Throw a new specific InvalidKeyAlgorithmException if the two algorithms do not match. This is a breaking change because invalid OpenSSH keys have been tolerated since the very beginning, when Shawn introduced the SSH support in Change 6610. Attempting to store new invalid SSH keys would fail and result an HTTP status 400 error as response to the add keys REST-API. Make SshKeyCacheImpl tolerant to existing keys stored in the accounts' profile, otherwise Gerrit may start flagging keys that have been previously stored as invalid, resulting in random authentication failures by existing users. Existing invalid keys are reported in the error_log with the associated exceptions and automatically fixed, removing the invalid key from the accounts profile and adjusting the key algorithm with the one associated with the public key. Bug: Issue 330758152 Release-Notes: Breaking change: validate and reject SSH keys with invalid or mismatched algorithm Change-Id: I83c89a786f70aa3b88744a70f10012415f45f284 (cherry picked from commit 6eac4fe62a6a081c5c9395f8874bdc49615eea0d) --- .../gerrit/sshd/InvalidKeyAlgorithmException.java | 44 ++++++++++++++++++++++ java/com/google/gerrit/sshd/SshKeyCacheImpl.java | 25 ++++++++++++ java/com/google/gerrit/sshd/SshUtil.java | 7 +++- javatests/com/google/gerrit/sshd/SshUtilTest.java | 2 - 4 files changed, 75 insertions(+), 3 deletions(-) create mode 100644 java/com/google/gerrit/sshd/InvalidKeyAlgorithmException.java 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 773c25bce6..713435d302 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; @@ -138,6 +139,15 @@ public class SshKeyCacheImpl implements SshKeyCache { // to do with the key object, and instead we must abort this load. // throw 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); } @@ -152,5 +162,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/sshd/SshUtilTest.java b/javatests/com/google/gerrit/sshd/SshUtilTest.java index 3225230bbd..1585bc3a97 100644 --- a/javatests/com/google/gerrit/sshd/SshUtilTest.java +++ b/javatests/com/google/gerrit/sshd/SshUtilTest.java @@ -20,7 +20,6 @@ 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.Ignore; import org.junit.Test; public class SshUtilTest { @@ -31,7 +30,6 @@ public class SshUtilTest { "AAAAB3NzaC1yc2EAAAABIwAAAIEA0R66EoZ7hFp81w9sAJqu34UFyE+w36H/mobUqnT5Lns7PcTOJh3sgMJAlswX2lFAWqvF2gd2PRMpMhbfEU4iq2SfY8x+RDCJ4ZQWESln/587T41BlQjOXzu3W1bqgmtHnRCte3DjyWDvM/fucnUMSwOgP+FVEZCLTrk3thLMWsU="; private static final Object VALID_SSH_RSA_ALGO = "ssh-rsa"; - @Ignore("To be enabled once the SSH key parsing is fixed") @Test public void shouldFailParsingOpenSshKeyWithInvalidAlgo() { String sshKeyWithInvalidAlgo = String.format("%s %s", INVALID_ALGO, VALID_OPENSSH_RSA_KEY); -- cgit v1.2.3 From 94a7b12235f5add266871e02db3ad2911b930389 Mon Sep 17 00:00:00 2001 From: Diego Zambelli Sessona Date: Thu, 21 Mar 2024 17:29:24 +0100 Subject: Run tests in FakeQueryChangesTest for paginationType NONE The tests in FakeQueryChangesTest were executed only for the default pagination (OFFSET) and SEARCH_AFTER Release-Notes: skip Change-Id: Ic5c84bdc28fccbf0a4c931d7137ce8f232907622 --- java/com/google/gerrit/index/QueryOptions.java | 4 ++++ java/com/google/gerrit/index/testing/AbstractFakeIndex.java | 8 ++++++-- .../query/change/FakeQueryChangesLatestIndexVersionTest.java | 7 +++++++ .../query/change/FakeQueryChangesPreviousIndexVersionTest.java | 7 +++++++ .../google/gerrit/server/query/change/FakeQueryChangesTest.java | 4 ++++ 5 files changed, 28 insertions(+), 2 deletions(-) diff --git a/java/com/google/gerrit/index/QueryOptions.java b/java/com/google/gerrit/index/QueryOptions.java index 29ab6d0119..40677a18da 100644 --- a/java/com/google/gerrit/index/QueryOptions.java +++ b/java/com/google/gerrit/index/QueryOptions.java @@ -128,4 +128,8 @@ public abstract class QueryOptions { limit(), filter.apply(this)); } + + public int getLimitBasedOnPaginationType() { + return PaginationType.NONE == config().paginationType() ? limit() : pageSize(); + } } diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java index 4197c64011..9101ae5ff8 100644 --- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java +++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java @@ -135,10 +135,14 @@ public abstract class AbstractFakeIndex implements Index { .findFirst() .orElse(-1) + 1; - int toIndex = Math.min(fromIndex + opts.pageSize(), valueList.size()); + int toIndex = Math.min(fromIndex + opts.getLimitBasedOnPaginationType(), valueList.size()); results = valueList.subList(fromIndex, toIndex); } else { - results = valueStream.skip(opts.start()).limit(opts.pageSize()).collect(toImmutableList()); + results = + valueStream + .skip(opts.start()) + .limit(opts.getLimitBasedOnPaginationType()) + .collect(toImmutableList()); } queryCount++; } diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java index 4dde452dc9..df6ff5a15e 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java @@ -33,4 +33,11 @@ public class FakeQueryChangesLatestIndexVersionTest extends FakeQueryChangesTest config.setString("index", null, "paginationType", "SEARCH_AFTER"); return config; } + + @ConfigSuite.Config + public static Config nonePaginationType() { + Config config = defaultConfig(); + config.setString("index", null, "paginationType", "NONE"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java index 95896dc991..805c99abdb 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java @@ -43,4 +43,11 @@ public class FakeQueryChangesPreviousIndexVersionTest extends FakeQueryChangesTe config.setString("index", null, "paginationType", "SEARCH_AFTER"); return config; } + + @ConfigSuite.Config + public static Config nonePaginationType() { + Config config = againstPreviousIndexVersion(); + config.setString("index", null, "paginationType", "NONE"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java index 3968a33707..af14683e8b 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java @@ -19,10 +19,12 @@ import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.a import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block; import static com.google.gerrit.common.data.GlobalCapability.QUERY_LIMIT; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; +import static org.junit.Assume.assumeFalse; import com.google.gerrit.acceptance.UseClockStep; import com.google.gerrit.entities.Permission; import com.google.gerrit.entities.Project; +import com.google.gerrit.index.PaginationType; import com.google.gerrit.index.testing.AbstractFakeIndex; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.index.change.ChangeIndexCollection; @@ -83,6 +85,7 @@ public abstract class FakeQueryChangesTest extends AbstractQueryChangesTest { @UseClockStep @SuppressWarnings("unchecked") public void noLimitQueryPaginates() throws Exception { + assumeFalse(PaginationType.NONE == getCurrentPaginationType()); TestRepository testRepo = createProject("repo"); // create 4 changes insert(testRepo, newChange(testRepo)); @@ -110,6 +113,7 @@ public abstract class FakeQueryChangesTest extends AbstractQueryChangesTest { @UseClockStep @SuppressWarnings("unchecked") public void internalQueriesPaginate() throws Exception { + assumeFalse(PaginationType.NONE == getCurrentPaginationType()); // create 4 changes TestRepository testRepo = createProject("repo"); insert(testRepo, newChange(testRepo)); -- cgit v1.2.3 From 1fbd90da749cc80c115f745e458cea31d0bb5dec Mon Sep 17 00:00:00 2001 From: Diego Zambelli Sessona Date: Tue, 19 Mar 2024 15:58:18 +0100 Subject: Fix paginationType NONE to run further queries only if needed If the first query to the index did not fill the full limit the caller wants, we paginate to obtain more results. However, subsequent queries are not needed if the previous query returned less results than the limit. This is a follow up of the change 413358 [1], that has reintroduced the issue fixed in change 344695. [1]: https://gerrit-review.googlesource.com/c/gerrit/+/413358/comment/85bca522_2b3552b2/ Bug: Issue 330195358 Release-Notes: Fix pagination to stop querying when there are no more results Change-Id: Ie326f3628f5312a83bf83177a3fa5134f042b59a --- .../gerrit/index/query/PaginatingSource.java | 25 ++++++++-------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java index 9ec64e63e0..d89fe66c14 100644 --- a/java/com/google/gerrit/index/query/PaginatingSource.java +++ b/java/com/google/gerrit/index/query/PaginatingSource.java @@ -55,12 +55,9 @@ public class PaginatingSource implements DataSource { List r = new ArrayList<>(); T last = null; int pageResultSize = 0; - boolean skipped = false; for (T data : buffer(resultSet)) { if (!isMatchable() || match(data)) { r.add(data); - } else { - skipped = true; } last = data; pageResultSize++; @@ -69,16 +66,16 @@ public class PaginatingSource implements DataSource { if (last != null && source instanceof Paginated) { // Restart source and continue if we have not filled the // full limit the caller wants. - // + @SuppressWarnings("unchecked") + Paginated p = (Paginated) source; + QueryOptions opts = p.getOptions(); + final int limit = opts.limit(); + // 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)) { - @SuppressWarnings("unchecked") - Paginated p = (Paginated) source; - QueryOptions opts = p.getOptions(); - final int limit = opts.limit(); int pageSize = opts.pageSize(); int pageSizeMultiplier = opts.pageSizeMultiplier(); Object searchAfter = resultSet.searchAfter(); @@ -100,20 +97,16 @@ public class PaginatingSource implements DataSource { searchAfter = next.searchAfter(); } } else { - @SuppressWarnings("unchecked") - Paginated p = (Paginated) source; - while (skipped && r.size() < p.getOptions().limit() + start) { - skipped = false; - ResultSet next = p.restart(pageResultSize); - + int nextStart = pageResultSize; + while (pageResultSize == limit && r.size() < limit) { + ResultSet next = p.restart(nextStart); for (T data : buffer(next)) { if (match(data)) { r.add(data); - } else { - skipped = true; } pageResultSize++; } + nextStart += pageResultSize; } } } -- cgit v1.2.3 From 1d7803586a74ab9688b32d64f9df15527fafbfde Mon Sep 17 00:00:00 2001 From: Diego Zambelli Sessona Date: Thu, 21 Mar 2024 19:23:05 +0100 Subject: Add test paginationType NONE to query only if there are more results If the first query to the index did not fill the full limit the caller wants, we paginate to obtain more results. However, subsequent queries are not needed if there are no more results from the previous query. This add a test for the change 413358 [1], that has reintroduced the issue fixed in change 344695. [1]: https://gerrit-review.googlesource.com/c/gerrit/+/413358/comment/85bca522_2b3552b2/ Bug: Issue 330195358 Release-Notes: skip Change-Id: If578ee877587488fdf3715331bd8d23e9d86f750 --- .../server/query/change/FakeQueryChangesTest.java | 39 ++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java index af14683e8b..2f5ae17e31 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java @@ -22,8 +22,11 @@ import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS import static org.junit.Assume.assumeFalse; import com.google.gerrit.acceptance.UseClockStep; +import com.google.gerrit.entities.Account; +import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Permission; import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.index.PaginationType; import com.google.gerrit.index.testing.AbstractFakeIndex; import com.google.gerrit.server.config.AllProjectsName; @@ -34,6 +37,7 @@ import com.google.gerrit.testing.InMemoryRepositoryManager.Repo; import com.google.inject.Guice; import com.google.inject.Inject; import com.google.inject.Injector; +import java.util.List; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.junit.Test; @@ -81,6 +85,41 @@ public abstract class FakeQueryChangesTest extends AbstractQueryChangesTest { assertThat(idx.getQueryCount()).isEqualTo(1); } + @Test + @UseClockStep + @SuppressWarnings("unchecked") + public void queryRightNumberOfTimes() throws Exception { + TestRepository repo = createProject("repo"); + Account.Id user2 = + accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId(); + + // create 1 visible change + Change visibleChange1 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW)); + + // create 4 private changes + Change invisibleChange2 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW), user2); + Change invisibleChange3 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW), user2); + Change invisibleChange4 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW), user2); + Change invisibleChange5 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW), user2); + gApi.changes().id(invisibleChange2.getChangeId()).setPrivate(true, null); + gApi.changes().id(invisibleChange3.getChangeId()).setPrivate(true, null); + gApi.changes().id(invisibleChange4.getChangeId()).setPrivate(true, null); + gApi.changes().id(invisibleChange5.getChangeId()).setPrivate(true, null); + + AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex(); + int queriesBeforeExecution = idx.getQueryCount(); + List queryResult = newQuery("status:new").withLimit(2).get(); + assertThat(queryResult).hasSize(1); + assertThat(queryResult.get(0).changeId).isEqualTo(visibleChange1.getKey().get()); + + // Since the limit of the query (i.e. 2), 2 index searches are expected in fact: + // 1: The first query will return invisibleChange5, invisibleChange4 and invisibleChange3, + // 2: Another query is needed to back-fill the limit requested by the user. + // even if one result in the second query is skipped because it is not visible, + // there are no more results to query. + assertThat(idx.getQueryCount() - queriesBeforeExecution).isEqualTo(2); + } + @Test @UseClockStep @SuppressWarnings("unchecked") -- cgit v1.2.3 From 65da242079ead7ee68bd7ee4241838bd466b93f2 Mon Sep 17 00:00:00 2001 From: Diego Zambelli Sessona Date: Thu, 4 Apr 2024 09:11:31 +0200 Subject: Reset pageResultSize when PaginationType NONE back-fill results This is a follow up of change 413358. In particular, if some results are skipped because of of the visibility constraints, more changes need to be asked from the index, and pageResultSize needs to be reset to know how many results are returned from each query. Without this change querying for more changes could end up in an infinite loop as exposed in a test in 3.6 [1] [1]: https://gerrit.googlesource.com/gerrit/+/refs/heads/stable-3.6/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java#126 Release-Notes: skip Change-Id: I566010c6f5bfcb4fbc003bc6693aa25d4dd44a81 --- java/com/google/gerrit/index/query/PaginatingSource.java | 1 + 1 file changed, 1 insertion(+) diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java index d89fe66c14..8390cbb329 100644 --- a/java/com/google/gerrit/index/query/PaginatingSource.java +++ b/java/com/google/gerrit/index/query/PaginatingSource.java @@ -100,6 +100,7 @@ public class PaginatingSource implements DataSource { int nextStart = pageResultSize; while (pageResultSize == limit && r.size() < limit) { ResultSet next = p.restart(nextStart); + pageResultSize = 0; for (T data : buffer(next)) { if (match(data)) { r.add(data); -- cgit v1.2.3