Introducing a cache for external ids is a preparation step for moving
the external ids from ReviewDb into NoteDb. For NoteDb it is planned
to store the external ids in a git note branch, where the note keys
are the sha1's of the external ids and the note values contain the
external id, the account id and optionally email and password. With
this format we can easily lookup external ids by the external id, but
listing all external ids of an account requires parsing all external
ids. Looking up the external ids of an account is possible from the
account index, however for reindexing an account we would still need
to lookup all external ids of the account from git. Having a cache for
the external ids ensures that the external ids are only loaded once
from git. If there is any update to external ids, we take care to
update the cache as well. For this change it means that all code that
modifies external ids must do an extra call to update the external id
cache. This is not optimal since updating the cache can be easily
forgotten. This is why the follow-up change cleans this up by
introducing a dedicated class that handles all external id updates and
then this is the only class that must take care to update the external
id cache.
Change-Id: I9ea979c646cddb9b39e723de5c061a70a2ce6fd6
Signed-off-by: Edwin Kempin <ekempin@google.com>
The account data is moved from ReviewDb into git.
Change-Id: I643827179b24601b138f394cfff5890f919b9da9
Signed-off-by: Edwin Kempin <ekempin@google.com>
* submodules:
* Update plugins/replication from branch 'master'
- Remove test prefix from test methods in replication plugin
We previously used 'test' to prefix tests but have decided to stop this.
This change removes the prefix from all test code.
Change-Id: I42e6191ece7872f4647e425e3ca0acf8c6452412
Reformat the Bazel build files with the buildifier tool [1].
The style is different for Bazel files. Most notably, indentation level
is 4 spaces instead of 2, and " is used instead of '.
[1] https://github.com/bazelbuild/buildifier
Change-Id: I95c0c6f11b6d76572797853b4ebb5cee5ebd3c98
We previously used 'test' to prefix tests but have decided to stop this.
This change removes the prefix from all test code.
Change-Id: I229a36751adc6a87fbae8d6f373671e141529496
We will implement code to receive emails soon and having that code
side-by-side with the code to send email makes the whole package seem
overloaded.
Change-Id: I88dd9d5f537c4ed8f6608ee62d08f7dbd75d21a4
These suggestions were produced by a refactoring tool that we have;
the particular decisions it made were based on heuristics over which I
have no control. Some suggestions were edited for brevity, and to omit
references to unreleased Guava features.
Change-Id: I9deac0afd6eda8fdc5a369816a4ee2bbe16924ba
This version fixed a major issue: [1] that was a reason of frustration
of many plugin developers: Not cache sources files under symbolic link.
Now for all such source files, the warning is issued:
"
Disabling caching for target //plugins/wip:wip__plugin, because one or
more input files are under a symbolic link
({plugins/wip=/home/davido/projects/wip}). This will severely impact
performance! To resolve this, use separate rules and declare
dependencies instead of using symbolic links.
"
To suppress this warning we add project.allow_symlink option. This
doesn't have any impact for gerrit core but silences the warning above
when plugins are built in gerrit tree mode.
As pointed out in this issue: [2], we are using some artifacts as source
to the java_library() rule as well as binary_jar for prebuilt_ja rule.
To avoid the warning, we rename sources to have "-sources.jar" suffix
and we rename *.zip to end with .jar in other places.
"
Assuming edit.src.zip is a JAR and renaming to edit.src.jar in
//gerrit-patch-jgit:edit_src. Change the extension of the binary_jar to
'.jar' to remove this warning.
"
source_under_test attribute was removed from java_test() rule.
Replication and cookbook-plugin are updated as well.
local.properties support was removed, but we use it only for download
process customization in our own python script, so that we can keep it
usage and not need to move it to .buckconfig.local.
[1] https://github.com/facebook/buck/issues/341
[2] https://github.com/facebook/buck/issues/855
Change-Id: Idf76cc71c21df43e808179b645f9175767b322a8
GpgApiAdapter throws an exception if the GPG API is disabled. The
client respects whether the server advertises signed push is enabled
when deciding whether to send the PUSH_CERTIFICATES option, but we
can't assume all clients will be so well behaved.
Recently in I04ddc340 we started passing this option when generating
events, causing breakage when processing a patch set with a push
certificate but the GPG API is disabled.
Change-Id: I53981d521f67906def7e8d3018f87816d5e43ab9
If available, use the account index to find accounts by username. This
is a preparation for moving the external ids from ReviewDb into git.
Change-Id: I8fe3adc0a65e90e3e7a21641cfca7352c46a458f
Signed-off-by: Edwin Kempin <ekempin@google.com>
When posting a new GPG key use the account index to check if the GPG
key is already associated with another account.
This is a preparation for moving the external IDs (GPG keys) from
ReviewDb into git. If an account index is not available we still fall
back to check the existence of the key in the database.
Change-Id: I00d0930ce9c92c7caac19311b1092f882c4b20e0
Signed-off-by: Edwin Kempin <ekempin@google.com>
GerritPublicKeyChecker needs to find the account for a given public
PGP key. Lookup the coresponding external ID via the account index
instead of loading it from the database.
This is a preparation for moving the external IDs into git.
Change-Id: Ia456c5bdb89da294b51a86087b92ad14165eae8a
Signed-off-by: Edwin Kempin <ekempin@google.com>
For each GPG key there is an external ID. This external ID is
created/deleted/updated when a GPG key is posted/deleted. Since
external IDs are cached with AccountState in the account cache, the
account must be evicted from the account cache whenever an external ID
is modified.
This change also adapts AccountIT to read external IDs from the cache
rather than looking them up directly from the database. This revealed
the problem with not evicting the account from the account cache when
a GPG key is posted/deleted.
Change-Id: Ie65ef07bdd3511ded03edb2b2e01bef550ca09a2
Signed-off-by: Edwin Kempin <ekempin@google.com>
To run the tests:
bazel test //...
To build the Gerrit plugin API, run:
bazel build gerrit-plugin-api:plugin-api_deploy.jar
To build the Gerrit extension API, run:
bazel build gerrit-extension-api:extension-api_deploy.jar
TODOs:
Licenses
Reduce visibility (all public for now)
Generate HTML Documentation
Core plugins
gerrit_plugin() rule to build plugins in tree and standalone modes
GWT UI (only gwt_module() skylark rule is provided, no gwt_binary())
PolyGerrit UI
WAR
Publish artifacts to Maven Central
Ask Bazel team to add Gerrit to their CI on ci.bazel.io
Contributed-By: Han-Wen Nienhuys <hanwen@google.com>
Change-Id: I9a86e670882a44a5c966579cdeb8ed79b1590de3
This field is also no longer used in some factory methods, which means
many callers need to get updated as well. One side effect is to
consolidate many of the GenericFactory#create methods. This in turn
means that the remote peer provider is never null; previously the code
was treating a null Provider and a Provider that returns null
identically, so this should have no behavior change.
Change-Id: I107fcfbd1a1d8daa15ee3afc02c8e7e709ffb3ea
Guava is discouraging these in favor of the static methods to allow
optimizations like lazy-loading of implementations.
Change-Id: I2e6ab4859ccfe5cc934f507dcbc46d2892e2dabd
* changes:
Use different NotesMigration implementation for testing
Ensure NotesMigration method calls aren't cached
Extract an abstract base class for NotesMigration
I39f2d5d7 isolated jgit in its own cell, that is based on this JGit
Buck build implementation: [1]. Migration was done seamlessly, meant
that single BUCK file in lib/jgit represents JGit cell root location.
However, the real structure of JGit project is divided to number of
different sub-projects. To map between simplified JGit cell in gerrit
and real JGit project structure in JGit project, java_library() rules
were added to root BUCK file in JGit project that work like proxy to
real rules located in JGit sub-projects. For example //:jgit in JGit
tree was implemented as:
java_library(
name = 'jgit',
exported_deps = ['//org.eclipse.jgit:jgit'],
visibility = ['PUBLIC'],
)
Such proxies are needed for every artifact that is referenced from
gerrit build and make Buck build implementation unnecessary verbose.
Moreover this introduced some subtle issues, like using JGit
dependencies in context of java_doc rules, where $(location :foo) macro
is unable to resolve the underlying files because java_library with
exported dependencies only do not have association with output file.
An attempt to replace java_library with only exported dependenies with
prebuilt_jar: [2] that depends on the real artifact introduced another
problem with assembly of gerrit.war, because now jgit.jar is twice in
the classpath (because prebuilt_jar has output file association). To
fix this we would need to filter potential duplicates in the assembly
process of gerrit.war.
Instead of using proxy approach and to try to provide yet another
workaround to subtle problems, emulate the JGit project structure and
reference directly the same artifacts paths within gerrit JGit cell
in gerrit build:
deps = [
'@jgit//org.eclipse.jgit:jgit',
],
This simplifies JGit Buck build implementation, as we wouldn't need to
proxy all artifacts referenced from gerrit build from the root build
file. And this would fix all remaining issues.
This approach make gerrit build slightly more verbose. JGit upgrade
process would need to touch 4 files instead of only one. But given that
the Gerrit/JGit development integration is important feature, we would
like to support (as this integration attempt shows: [3]) in our build
toolchain, this overhead is justified.
With this change, the root build file in JGit project can be stripped:
[4].
[1] https://git.eclipse.org/r/61938
[2] https://git.eclipse.org/r/66547
[3] https://gerrit-review.googlesource.com/61892
[4] https://git.eclipse.org/r/66562
Change-Id: I2d278f80d0fedc4c5e9943804873f57145877dfe
This uses volatile booleans for each bit that can be changed on the
fly. This is not especially important for most AbstractDaemonTests,
since we don't use this functionality and they previously used
ConfigSuite to set the fields. But it will be necessary for testing
notedb rebuilding.
Change-Id: I55f01c873d6d1f4fb3d459799bb3933afbd0c59f
Consume JGit as first third party library from its own cell. Normally
the cell is defined as lib/jgit directory. It can be easily replaced
with CLI:
buck build --config repositories.jgit=path/to/dev/jgit gerrit
or tweaking the .buckconfig:
[repositories]
jgit = path/to/dev/jgit
The former approach is sufficient to build and run the test from the
CLI, the latter is needed to generate eclipse project.
To isolate the JGit rules in its own cell some refactoring was needed.
JGit patch for GWT module was moved to gerrit-patch-jgit project, and
some symlinks were needed for maven machinery to work. include_defs()
doesn't work for now across cell boundaries, and native `buck fetch`
feature still has some limitations: [1]. Moreover, excluding paths,
unsigning JARs and license linking should be re-implemented on top
of it.
[1] https://github.com/facebook/buck/issues/602
Test Plan:
Normal gerrit build and the build with hijacked JGit cell should work
in both standalone (gerrit.war) and Eclipse environment. Note, that
to test --config repositories.jgit=path/to/dev/jgit use case, the most
recent JGit tree must be used, that contains Buck driven build
implementation.
Change-Id: I39f2d5d75bbac88804406d6242b5e714f4916926
We already have the warning "Incomplete 'switch' cases on enum" enabled,
but it does not warn when a case is missing if the switch has a default
block.
Enable the "Signal even if 'default' case exists" option. This would
have enabled us to catch the problem that was fixed in Ied8ff0f8f.
In most instances of the warning we can simply add the missing case(s)
above the 'default', meaning there is no change in behaviour.
However there are some instances where the missing case is a bug and
should be handled correctly. These are fixed separately in follow-up
commits.
Change-Id: I3675d29981423043266a26b1a78932c5708a6272
- FS is no longer required for LockFile constructor.
- TestRepository#getClock was renamed getDate.
- Replace usages of RefDatabase#getRef with either exactRef, when we
know we can bypass the full lookup path, or findRef, where the
input is user-provided and we might need to add an implicit prefix.
Change-Id: I6309b91ccf99ffaa724be273ddae0d9857b825f9
assertProblems(foo) with an implicit empty list can be confusing to
read. You can tell I figured this out, because
GerritPublicKeyCheckerTest, which postdates the other tests, already
used this pattern. Use it consistently everywhere.
Use (String, String...) in method signatures to make invalid use of
assertProblems with no expected problems a compile-time error.
Change-Id: I5621c586ac16dff2a34d30efd1cca8949907316d
The JDK Javadocs use "null" when used as a normal English adjective,
and "{@code null}" when referring to a variable or return value
having this value. Start being a little more consistent about this.
Change-Id: I44ebde98f43696b4085c797620aec95ef9d9f35a
When checking push certificates, we only care about whether the key
used for signing was valid at the time the signature was created; it
may have since expired or been superseded, and that's ok. Add an
optional setter to PublicKeyChecker to tell it to use a different
effective date, only considering revocations and expirations prior to
that date.
There are some cases where we explicitly do not want to respect this
effective date:
- If a key is compromised, all signatures made with that key are
invalid.
- Allow after-the-fact web-of-trust assertions, so for example the
push certificate stored in a PatchSet can go from OK to TRUSTED
simply by adding the proper certification today.
Change-Id: I078fd0f4b431af8279948961a99e340f932229b7
Bouncy Castle's isRevoked() method is so dumb as to be harmful: it
only checks whether there is a revocation packet in the key, without
doing any sort of validity checks on the packet.
Improve the check by verifying the signature against the key that
provided it, and for non-self-signatures, verifying that the signer is
an authorized revocation key.
This requires always passing in a store to any PublicKeyChecker.
However, we still have the same initialization order problem as
before, where a store is not always available at PublicKeyChecker
creation, in particular in the PushCertificateChecker codepath.
Argument precondition checks are the best we can do. Also add a
convenience method to GerritPublicKeyChecker.Factory for the very
common case (particularly in tests) where we do have both an expected
user and a store.
Change-Id: Id15714f0395a200fcb33fb199c57355b860187a3
Instead of overloading the constructor and check methods with optional
arguments, expose optional arguments via setters. We are about to add
another one, and this was getting unwieldy.
We still use a Factory to create GerritPublicKeyChecker, which parses
the config once in a @Singleton and subsequently configures all
checkers with that config. Since the checker is initially created when
a store might not be available (because the repo hasn't been opened),
we need to separate the trusted key config setter from the store
setter, and check state at use time.
Change-Id: Ide6bc76a8180f91caa6f03f1a0eb25ec6d20ba45
Make tests slightly more readable by describing the necessary keys.
(Note that this does not apply to TestTrustKeys, which have more
opaque names that refer to diagrams in the class Javadoc; readers are
expected to have the diagrams in front of them.)
Change-Id: I30642d37d2563084c0a372549bf41668f280c433
This is controlled by receive.requireSignedPush in the project.config,
which is a separate bit from receive.enableSignedPush. (Adding an
inheritable tri-state enum would have been complex to implement and
have hard-to-define semantics.)
requireSignedPush is only inspected if enableSignedPush is true; this
allows project owners to temporarily disable signed push entirely e.g.
due to a bug, without having to flip both bits.
Change-Id: I07999b6fa185d470b30509941473e3158f9dfa2c
Administrators may choose to populate GPG keys by hand, or to
temporarily restrict adding new keys, without disabling signed push
verification or verification of push certs stored in PatchSets.
Separate the UI and REST API for editing GPG keys from the protocol
machinery for verifying signed push.
Change-Id: I2c921cf92a3452d44ee8cb02efd77ab7f2bd02dd
This separates the logic for extracting the enableSignedPush bit from
the host config from the actual API bindings, allowing alternative
implementations of that bit to come from elsewhere.
Change-Id: I878842eabaa6e03d8f73bef993905495e7c2f0b3
To do this, we need to teach PushCertificateChecker to include in its
result type the key that it looked up from the store, in addition to
the results of the certificate check. This in turn needs to be
converted to the REST API return type in the GpgApiAdapter.
Change-Id: Ic83e3d4b66126629de6e32470089fa96173af5a9
This needs to be checked by the server during push to prevent replay
attacks, but when checking certificates after the fact we can blindly
trust the nonce.
Change-Id: I6256cd2409f9e90dfd2f2dd8d4c1c3c55070a9e9
Injecting PublicKeyChecker is possible, since it has a public no-args
constructor, but is inadvisable, since it can't check against specific
Gerrit users. Use GerritPublicKeyChecker instead.
Change-Id: I6fb3ccef2908526d40d40861f9ea97c506ba1f74
We want to use this same checker, which uses GerritPublicKeyChecker,
to check certificates after the fact, not just during push.
Change-Id: I065c303213520f644c6ae7717c99f8adc38d62c2
