* stable-2.14:
Use Logger's built-in string formatting where possible
Doc: Fix code example in JS API
Change-Id: I1b953b6d9ab5cd066b8b6f43bb1843dcb1736d1b
* stable-2.14:
Update git submodules
Update git submodules
ldap.Helper: Use local logger and make logger in LdapRealm private
Remove ValidationError#createLoggerSink to avoid passing around loggers
LdapLoginServlet: Improve exception handling
OperatingSystemMXBeanProvider: Log exception for ReflectiveOperationException
WorkQueue: Don't fail when queue metric already exists
WorkQueue: Sanitize metric name when queue is created
DropWizardMetricMaker: Introduce method to sanitize metric name
Change-Id: I4729d537aeb5ef934fcae90b610e28966a6ada9a
* Handle AuthenticationFailedException separately:
This exception is a subclass of AccountException that is thrown if the
user provides wrong credentials. For this exception we want to return
"Invalid username or password." as message to the client.
* Return a more general message for other AccountExceptions:
Likely they are not caused by invalid username or password since this
would cause a AuthenticationFailedException which we handle before.
* Increase log level to warning:
This is the log level that we use for these exceptions in other places
(e.g. ProjectBasicAuthFilter). Make it consistent.
* Log the stacktrace for AccountExceptions:
We do this everywhere else (e.g. ProjectBasicAuthFilter,
HttpLoginServlet). Make it consistent.
Change-Id: Ie34687d087b5a6cd102bf8cebd0f9830f54c9c1c
Signed-off-by: Edwin Kempin <ekempin@google.com>
* stable-2.14:
HttpPluginServlet: Don't trim leading whitespace from about.md content
ProjectConfig: Don't use JGit's StringUtils to convert to lower case
Do not abort indexing if < 50% projects failed
Revert "AllChangesIndexer: Don't abort when failing to open repository"
VersionedAccountDestinations: Remove unused createSink(String) method
ProjectBasicAuthFilter: Add comment why cause is not logged
BazelBuild: Fix exception message when command was interrupted
GitwebServlet: Write only one log entry for CGI errors
GitwebServlet: Log unexpected errors on error level
PostGpgKeys: Remove unneeded use of Joiner
Remove some logs for errors that are rethrown
DropWizardMetricMaker: Improve error messages for invalid arguments
DropWizardMetricMaker: Improve error message when metric name is invalid
AllChangesIndexer: Don't abort when failing to open repository
Change-Id: I6febb890b7717731fcb5f0653360982668469069
We used ProcessBuilder#toString() but ProcessBuilder doesn't implement
the toString() method, hence calling toString() would return the
instance identity (e.g. 'java.lang.ProcessBuilder@4488aabb'). Instead
include the command which is more useful.
This issue was detected by ErrorProne.
Change-Id: I780f21bbfeae8e7c1b3f4467d789bcf148293324
Signed-off-by: Edwin Kempin <ekempin@google.com>
If a separate log statement is written for each line then the entries in
the log file can be interleaved with log entries from other threads,
which makes them less readable.
Change-Id: I038a0d8bc906746cc23b8f3bfb31d6c4d98b53c0
Signed-off-by: Edwin Kempin <ekempin@google.com>
Especially it's seems odd that copyStderrToLog logs the input on error
level but failing to do so results only in a debug log.
Change-Id: I52302be34e8f4a62639015acffe26d0a11a5c8da
Signed-off-by: Edwin Kempin <ekempin@google.com>
Add a new setting, http.addUserAsResponseHeader, which when enabled
causes the servlet response to include a 'User' header that contains
the name of the logged in user.
This will enable reverse proxies to log the name of the user that
issued the http request.
The new setting is disabled by default.
Change-Id: I5c3c783813f3aa71209320610bb8168a51305cba
Some endpoints allow both JSON and raw input. parseRequest selects
whether to parse to JSON using a reader or to provide the raw input as
an InputStream based on the request's content-type.
Since v2.15-rc0~1847^2 (Discard request HTTP bodies before writing
response, 2017-03-16), on endpoints that permit raw input, we call
getInputStream to obtain the rest of the response body and discard it
before writing the response. When the request was JSON, this produces
errors from Jetty, since calling getInputStream after getReader violates
the servlet API:
[HTTP-66] ERROR com.google.gerrit.httpd.restapi.RestApiServlet : Error in PUT /a/plugins/reviewers.jar
java.lang.IllegalStateException: READER
at org.eclipse.jetty.server.Request.getInputStream(Request.java:844)
at javax.servlet.ServletRequestWrapper.getInputStream(ServletRequestWrapper.java:138)
at javax.servlet.ServletRequestWrapper.getInputStream(ServletRequestWrapper.java:138)
To fix it, instead of guessing whether this was a raw request based on
whether the endpoint supports raw requests, use the parseRequest result
to decide whether this is a raw request for which we need to discard any
unconsumed content.
Bug: Issue 8677
Change-Id: I1db69104f31e1c04b137d994523422a07ca5cf43
(cherry picked from commit 91136bb28ec45cbbd66e7d8aabe209a6faa7eb2a)
The problem we are facing on the stable-2.14 branch is: we have
intermediate NoteDb migration state for accounts entity due to
merge of: Ic9bd5791e84. That why we are writing to both backends:
ReviewDb and NoteDb. It creates potential risk to be out of sync
between ReviewDb and NoteDb (and secondary index). In addition it
is always bad from performance point of view to unnecessary write
to 2 different backends. The real migration to NoteDb for accounts
entities (phase 2) happens in: Ia1dae9306b7 and Schema_144, that is
migrating the external IDs from ReviewDb to NoteDb, and that change
is not a part of stable-2.14.
In retrospective, we shouldn't include partially migrated code paths
for the production releases. It's error prone and bad for the
performance. Originally, multi-phase upgrade procedure was done on
master only to support multi master and zero downtime upgrades. These
feature is not related to open source gerrit version.
Moreover, now, that we are facing intermittent account corruption
problems: Issue 7652 that is hard to track down, understand and fix,
we are seeing automatic recovery attempt: [1], that is trying to
detect database corruption and synchronize both backends. This change
takes a different approach and avoids two backends where only ReviewDb
is actually used on production release line 2.14.
To avoid fixing too many caller sites the interfaces of ExternalIds,
ExternalIdsOnInit, ExternalIdsBatchUpdate and ExternalIdsUpdate are
mostly preserved, but the code paths for NoteDb mutations is dropped.
This partially reverts commit 744d2b896719e2058539db98443c80eb9368fd77.
[1] https://gerrit-review.googlesource.com/162450
Change-Id: Iec8d0c5639e462d88a7c5d0906febfd6f3337277
When integrators of Gerrit want to style the login forms, having a class or
ID for the <body> tag come in very handy.
Change-Id: I073424241c5bf4430ce3c3476b2836f022dc1140
Double-clickers are logging in twice because the form is submitted twice
and the server will handle both requests concurrently. This is not a big
issue but it becomes one when a user login in for the first time is a
double-clicker. Server will handle both requests concurrently resulting
in creating 2 accounts for the same user.
This change disables the form submission after the first submit.
Change-Id: Ida55e632618c72ab11e536854c654ed423a0f195
Allow to request static or Documentation content with respect to CORS.
This change was inspired by [1] and [2]. It sets
Access-Control-Allow-Origin header to origin of the client if the
client's domain matches a regular expression defined in
'site.allowOriginRegex' or when 'site.allowOriginRegex' is empty
(assumption is that access to documentation is not restricted).
[1] https://gerrit-review.googlesource.com/c/gerrit/+/84191
[2] https://gerrit-review.googlesource.com/c/gitiles/+/84151
Change-Id: I0343ac1cdce9da10fea9bc207a4114e1596fbfab
Signed-off-by: Jacek Centkowski <jcentkowski@collab.net>
When CanonicalWebURL contains a context like 'gerrit'.
https://a-host/gerrit the context was not taken into consideration
when redirecting polygerrit urls to GWT urls.
pg-uri: /gerrit/c/123/
context + '#' + pg-uri = /gerrit/#/gerrit/c/123
Strip context from pg-uri before transforming it to GWT url.
Change-Id: I7aa10c53a40c21c3240f227d015827ca38e19da8
I suggested to Luca at the hackathon that he could use this mechanism to
limit refs advertised to Jenkins, instead of writing a custom
PermissionBackend. Getting this in 2.15 means people using his plugin
won't have to wait for the next release to stop using the
PermissionBackend hack.
Change-Id: I8c38eef94d6e505b926b3da6c470e34f6613ca2c
Since CGI.pm 4.05 (2014-10-08), a warning is shown for every gitweb
request involving the "h" or "hb" parameters. There is no vulnerability
since "add_review_link" only takes one scalar parameter.
Force a scalar to prevent the warning in gerrit error log.
Bug: Issue 5897
Change-Id: I1b7e6b608af7700225da8625cb749fa12e971591
* changes:
Factor out Contributor Agreements from ProjectControl
Add ProjectPermissions for upload and receive pack, migrate callers
Add ProjectPermission.READ_NO_CONFIG
This commit factors out the check for valid CLAs from ProjectControl.
CLAs will continue to exist separate of permission backend as the fact
if a user has signed a CLA has nothing to do with permissions per-se.
This refactoring also removes the callers of
ProjectControl#canPushToAtLeastOneRef() which will be removed in a
follow-up change when the last caller was migrated.
Change-Id: Ib3f0849f9fbb720fee2cbc422127f7769a45a20f
ProjectControl#canRunUploadPack() and #canRunReceivePack() are just
permission checks using group membership. Therefore they can easily be
checked using PermissionBackend.
Installations that do not use these permissions at all (like Google) can
just have their own PermissionBackend implementation always deny that
permission.
Change-Id: I9d12ed4664c94ef77a9a0958bc91595bef6dfd5d
The GWT UI static servlets (e.g. WarGwtUiServlet) were already using
class load time as the "mtime" of a file, to account for the fact that
the build system always sets the mtime of zipfile entries to the Unix
epoch. (This is intentional, to keep bazel builds reproducible.)
PolyGerrit has the same problem served from a WAR file, but was not
applying this hack. Complicating the situation is that the same class
(PolyGerritUiServlet) is used for the dev server, where we can trust
the mtime. Hackily check whether the path in question is on-disk in the
implementation of getLastModifiedTime.
There was also a bug in ResourceServlet#maybeStream that was bypassing
getLastModifiedTime when checking If-Modified-Since; fix that as well.
While we're in here, fix the other broken class in this hierarchy,
WarDocServlet.
In retrospect, it may have been a mistake to make ResourceServlet try to
handle all possible caching behaviors, since it makes bugs like this all
too easy to introduce. Long term, may want to disentangle the on-disk
implementation, which may depend on mtime to determine staleness, from
the zipfile implementation, which shouldn't.
Bug: Issue 6885
Change-Id: Iea041774c1005cb9918c462d01aef05cff61da23
(cherry picked from commit 65c8345a96c4de35ccf4ddc09a6bc74ce6a429f8)
The GWT UI static servlets (e.g. WarGwtUiServlet) were already using
class load time as the "mtime" of a file, to account for the fact that
the build system always sets the mtime of zipfile entries to the Unix
epoch. (This is intentional, to keep bazel builds reproducible.)
PolyGerrit has the same problem served from a WAR file, but was not
applying this hack. Complicating the situation is that the same class
(PolyGerritUiServlet) is used for the dev server, where we can trust
the mtime. Hackily check whether the path in question is on-disk in the
implementation of getLastModifiedTime.
There was also a bug in ResourceServlet#maybeStream that was bypassing
getLastModifiedTime when checking If-Modified-Since; fix that as well.
While we're in here, fix the other broken class in this hierarchy,
WarDocServlet.
In retrospect, it may have been a mistake to make ResourceServlet try to
handle all possible caching behaviors, since it makes bugs like this all
too easy to introduce. Long term, may want to disentangle the on-disk
implementation, which may depend on mtime to determine staleness, from
the zipfile implementation, which shouldn't.
Bug: Issue 6885
Change-Id: Iea041774c1005cb9918c462d01aef05cff61da23
In most cases, this method was used as a fallback in case no ref owner
was defined. Comments in the code suggested that it was always intended
to just fall back to ADMINISTRATE_SERVER.
Looking at all use cases individually, it seems safe to just fall back
to ADMINISTRATE_SERVER directly.
Change-Id: I3c7726c3720492f36f737edf7c0e4e7e64c5e6f1
New design mocks have the site's fonts entirely in Roboto and Roboto
Medium. This change adds the fonts as dependncies.
Bug: Issue 7139
Change-Id: I4e8175e49dda42a68b22d4a5966aba597c84c132
In an effort to make {Ref,Project,Change}Control package-private all
public methods that aren't replaced by PermissionBackend need to move
elsewhere. Both ProjectControl#getLabelTypes() and
ChangeControl#getLabelTypes() rely on the project state, so they
can move there. ChangeControl#getLabelTypes() needs the destination
branch and current user in addition, which are added as arguments.
Change-Id: I080fd93103679552952872dde49cc149a59cd43e
Reduce the usage of ProjectControl by replacing it with ProjectState and
CurrentUser where it was just used as a container for these.
Eventually, {Ref,Change,Project}Control should be package-private. This
commit is an incremental step towards that goal.
Change-Id: I204d7cae3816e81a7859672a6ac3b5d5273997fa
* changes:
config-sso.txt: Update sections that talk about external IDs
Document accounts in NoteDb
Allow to push updates of account.config file
Remove support for writing accounts from ReviewDb
Remove support for reading accounts from ReviewDb
Reduce the usage of ChangeControl by replacing it with ChangeNotes and
CurrentUser where it was just used as a container for these.
Change return type of ChangeResource#getUser from IdentifiedUser to
CurrentUser to be more versatile and allow individual endpoints to check
if the user is an identified user. Since anonymous users can also
access some change resources it seems more consistent to only assume
that we can get a CurrentUser from ChangeResource.
Eventually, {Ref,Change,Project}Control should be package-private. This
commit is an incremental step towards that goal.
Change-Id: I6790f6f9d56240a96aa007bae140422b71c59750
Accounts have been fully migrated to NoteDb. However inside of Google we
still have code that depends on having all accounts in the Accounts
table. This means we must still write account updates to the Accounts
table. On the other hand also for us Gerrit server is always reading the
accounts from NoteDb. This means the code for reading accounts from
ReviewDb can be removed already now. This is why removing the support
for accounts in ReviewDb is done in 2 parts:
1. remove support for reading accounts from ReviewDb (this change)
2. remove support for writing accounts to ReviewDb (follow-up change)
Change-Id: Id51a0504635de30f95205ec76d51e2be8ff6f462
Signed-off-by: Edwin Kempin <ekempin@google.com>
* changes:
AccountIndex: Add field for exact preferred email
Return only exact matches from InternalAccountQuery#byPreferredEmail
AccountIndex: Add fields that allow to detect an account document as stale
Doing an account query on the preferredemail field finds accounts with a
preferred email that matches the given email case insensitive or that
starts with that prefix (also case insensitive). However all callers are
only interested in exact matches. Instead of letting each caller filter
out non-exact matches do this filtering once in
InternalAccountQuery#byPreferredEmail.
Change-Id: If893d1bd8857fd69b3c2222ba104aa9ee212f936
Signed-off-by: Edwin Kempin <ekempin@google.com>
These two methods were a convenience implementation that checked
ChangeControl#isVisible before constructing a ChangeControl. This commit
migrates the callers to check ChangePermission.READ instead.
Change-Id: I6101e5cdd9852d28859e78458dc55fb2584600eb
Instead of using a generic string "Update NoteDb refs", record the
class name of the RestModifyView that caused this operation to
execute. This makes inspection of the history of a change much
easier, as its possible to correlate events to specific sections of
the Gerrit server.
The implementation favors going from the RestApiServlet down the
stack, so that plugin handlers are caught first and logged as the
plugin's RestModifyView implementation, even if the plugin is using
an internal API call to a standard view like PutTopic.
Change-Id: Ifd6a9843b6bd5f3e7f1ff7e145fa433f237b147a
