diff options
14 files changed, 255 insertions, 67 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/query/IndexedQuery.java b/java/com/google/gerrit/index/query/IndexedQuery.java index ee25ef96eb..cb98c06a67 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..84572d712a 100644 --- a/java/com/google/gerrit/index/query/PaginatingSource.java +++ b/java/com/google/gerrit/index/query/PaginatingSource.java @@ -63,42 +63,55 @@ 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); + pageResultSize = 0; + 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/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java index 27c7fe8002..692d5248a2 100644 --- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java +++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java @@ -149,10 +149,14 @@ public abstract class AbstractFakeIndex<K, V, D> implements Index<K, V> { .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++; resultsSizes.add(results.size()); diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java index f6f1eda59a..59456f6fc4 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/server/query/change/PredicateArgs.java b/java/com/google/gerrit/server/query/change/PredicateArgs.java index ebe4390f08..02d2ca6951 100644 --- a/java/com/google/gerrit/server/query/change/PredicateArgs.java +++ b/java/com/google/gerrit/server/query/change/PredicateArgs.java @@ -71,7 +71,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<>(); 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/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java index d20a00465c..8b7edacc2f 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java @@ -25,6 +25,8 @@ import static org.junit.Assume.assumeFalse; import static org.junit.Assume.assumeTrue; 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; @@ -83,12 +85,49 @@ public abstract class FakeQueryChangesTest extends AbstractQueryChangesTest { AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex(); newQuery("status:new").withLimit(5).get(); + // Since the limit of the query (i.e. 5) is more than the total number of changes (i.e. 4), + // only 1 index search is expected. assertThatSearchQueryWasNotPaginated(idx.getQueryCount()); } @Test @UseClockStep @SuppressWarnings("unchecked") + public void queryRightNumberOfTimes() throws Exception { + TestRepository<Repo> 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(); + idx.resetQueryCount(); + List<ChangeInfo> 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. + assertThatSearchQueryWasPaginated(idx.getQueryCount(), 2); + } + + @Test + @UseClockStep + @SuppressWarnings("unchecked") public void noLimitQueryPaginates() throws Exception { assumeFalse(PaginationType.NONE == getCurrentPaginationType()); @@ -123,37 +162,7 @@ public abstract class FakeQueryChangesTest extends AbstractQueryChangesTest { @Test @UseClockStep - public void invisibleChangesNotPaginatedWithNonePaginationType() throws Exception { - assumeTrue(PaginationType.NONE == getCurrentPaginationType()); - AbstractFakeIndex idx = setupRepoWithFourChanges(); - final int LIMIT = 3; - - projectOperations - .project(allProjectsName) - .forUpdate() - .removeAllAccessSections() - .add(allow(Permission.READ).ref("refs/*").group(SystemGroupBackend.REGISTERED_USERS)) - .update(); - - // Set queryLimit to 3 - projectOperations - .project(allProjects) - .forUpdate() - .add(allowCapability(QUERY_LIMIT).group(ANONYMOUS_USERS).range(0, LIMIT)) - .update(); - - requestContext.setContext(anonymousUserProvider::get); - List<ChangeInfo> result = newQuery("status:new").withLimit(LIMIT).get(); - assertThat(result.size()).isEqualTo(0); - assertThatSearchQueryWasNotPaginated(idx.getQueryCount()); - assertThat(idx.getResultsSizes().get(0)).isEqualTo(LIMIT + 1); - } - - @Test - @UseClockStep public void invisibleChangesPaginatedWithPagination() throws Exception { - assumeFalse(PaginationType.NONE == getCurrentPaginationType()); - AbstractFakeIndex idx = setupRepoWithFourChanges(); final int LIMIT = 3; 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(); + } +} |