diff options
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(); + } +} |