The migration to permission backend in I9458bd55fa wasn't equivalent:
Old code:
CapabilityUtils.checkRequiresCapability(globals.currentUser,
null, rc.getClass());
New code:
globals
.permissionBackend
.user(globals.currentUser)
.checkAny(GlobalPermission.fromAnnotation(d.pluginName,
d.view.getClass()));
The skipping of capability check in the base version was erroneously
omitted:
if (ctl.canAdministrateServer()) {
return;
}
This broke some plugins, most notably importer plugin. Plugin name is
resolved to null (this is probably wrong too and indicates, that the
capability check for plugin own capability is broken but it is a
different bug). That why the check doesn't work, because import is a
plugin capability and not gerrit core capability. The same wrong
resolution of the plugin name to null is happening on stable-2.14
branch, but the only reason it works there, is because the capability
check is omitted for administrators on stable-2.14 branch.
Bug: Issue 8859
Change-Id: I61534cd9c5cd0da34782e671ae53c0b7fc2e4b65
* stable-2.14:
GitOverHttpModule: Bind REST auth filter in its own module
Ensure user authentication in AllRequestFilter filters
Change-Id: I873737ead014c20f0590ab0f246e9ded0601e4ef
GitOverHttpModule is binding both filters for git over HTTP and for REST
requests. Extract filter definition for REST requests in its own module.
Change-Id: If03e76c906bc3e0cac827b49f5f087cc859be4cd
* stable-2.14:
Minor improvements in receive.maxObjectSizeLimit documentation
Bazel: Consume rules_closure from HEAD
Bump auto-value to 1.6.2
Change-Id: I401942a40c5001300f77f9437d342001cd42e619
So far, the logs were being flooded with empty error messages every time
a gitweb operation was done:
[Gitweb-ErrorLogger] ERROR com.google.gerrit.httpd.gitweb.GitwebServlet :
Check error message is not empty before log in it.
Change-Id: Ida6a19092d69e65782041d4327ed97ec5f4dc70e
* 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
