Checking for document limit must be done before allocating
a child process, otherwise the new child process will not
be cleaned up or released, thereby failing the test.
Change-Id: I99b1155bdacf2f0b7a24c7b7330d207e4c7beee8
Reviewed-on: https://gerrit.libreoffice.org/30208
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
permission= parameter in URL is still supported, but overridden
by UserCanWrite parameter.
Also, introduce a new protocol message, perm: which dictates
loleaflet about the permission rather than the other way around
(only in case of WOPI)
It is to be noted that by default loolwsd assumes very
restrictive permissions, so not providing UserCanWrite in WOPI
implementation by a WOPI host would lead to opening of only
readonly session.
Change-Id: I2013c1661fd491c79bb367a41e1a7036fa03f984
These Log::foo() calls are in ClientRequestHandler::handleClientRequest(),
not in ClientRequestHandler::handleRequest().
Actually I wonder why we show the name of the function in this handful of
places. We don't do it in general. Consistency, bah.
I fear they get can out of sync when HTTPCrashTest::killLoKitProcesses()
kills a loolkit process, and this then causes HTTPWSError::testMaxDocuments()
to fail. This is to help debugging that.
Haven't fully understood what is going on yet. But one thing is sure:
It is a bad idea to duplicate the same state information in two
places, we shouldn't really use that separate NumDocBrokers
variable.
Probably also NumConnections tracks state that can easily be
calculated from the data structures, but maybe NumConnections does not
that easily get out of sync.
Use the previously unused UnauthorizedRequestException for this, and
throw a such in StorageBase::create() when the WOPI host doesn't match
any of those configured.
In a developer debug build, without access to any real WOPI
functionality, you can test by setting the FAKE_UNAUTHORIZED
environment variable and attempting to edit a plain local file:
URI. That will cause such an exception to be thrown in that function.
Catch that UnauthorizedRequestException in
ClientRequestHandler::handleGetRequest(), and send an 'error:
cmd=internal kind=unauthorized' message to the client. Handle that in
loleaflet in the same place where the 'error: cmd=internal
kild=diskfull' message is handled, and in the same fashion, giving up
on the document.
Actually, using exceptions for relatively non-exceptional situations
like this is lame and makes understanding the code harder, but that is
just my personal preference...
FIXME: By the time StorageBase::create() gets called we have already
sent three 'statusindicator:' messages ('find', 'connect', and
'ready') to the client. We should ideally do the checks we do in
StorageBase::create() much earlier.
Also consider that ClientRequestHandler::handleClientRequest() has
code that catches UnauthorizedRequestException and
BadRequestException, and tries to set the HTTP response in those
cases. I am not sure if that functionality has ever been exercised,
though. Currently, we upgrade the HTTP connection to WebSocket early,
and only after that we check whether the WOPI host is authorized
etc. By that time it is too late to return an HTTP response to the
user. If that even is what we ideally should do? If not, then we
probably should drop the code that constructs HTTP responses and
attempts to send them.
Also, if I, as a test, force an HTTPResponse::HTTP_BAD_REQUEST to be
sent before the HTTP connection is upgraded to WebSocket, loleaflet
throws up the generic "Well, this is embarrassing" dialog anyway. At
least in Firefox on Linux. (Instead of the browser showing some own
dialog, which I was half-expecting to happen.)
addSession() now also loads the document, so querying docbroker
for load duration before it doesn't make any sense.
Change-Id: I3c60bef5e2054878ba695b8f76b6800cdedffe8d
This avoids early connection failures causing the
session to linger in the Broker indefinetely.
Change-Id: Ibe2b5b386ed1cd6e12e68735bff60b15957188cf
Reviewed-on: https://gerrit.libreoffice.org/29952
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Reduces the number of WOPI calls made during a document load. Earlier
effort was made in edfd3266f8
This commit cleans up and uses better approach for the same.
Other than that, access token of each session is now correctly
used when interacting with the storage. Earlier, we used to
use the same access token for each upload to storage which means
that irrespective of who clicked the save button, changes to the
document were only made on behalf of one person (of whom the
access token is used). This is fixed now.
Also includes minor cleanup left and right.
Change-Id: Id32702ff02aea4f63b7cc6afa9f62664807bb57d
Reviewed-on: https://gerrit.libreoffice.org/29931
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Use a plain pipe, as created by Poco::Process::launch() when you pass
a non-nullptr inPipe pointer to it. Leads to simpler code. No need to
explicitly create a FIFO and open it. In loolforkit, the read fd of
the pipe is now always 0 (stdin).
Creating the pipe ourselves in loolwsd using pipe() and passing the fd
of the read end of the pipe on the loolforkit command-line would not
be much more complex, and was what I first tried. But it did not work,
as Poco::ProcessImpl::launchByForkExecImpl() closes all file
descriptors >= 3. Which is a bit rude, and not documented, but there
you go.
For the 'message' parameter of LOOLSession::shutdown(),
ClientSession::shutdownPeer(), and PrisonerSession::shutdownPeer() we
always passed "". It was passed on as the 'statusMessage' parameter to
WebSocet::shutdown() which has "" as default anyway.
Finally seen linking on a 'perf top' output, and restored this.
UnitPrefork shows that it saves memory, and it should save CPU time
and more memory as the suite is actually used.
Poco's ServerSocket by default uses SO_REUSEPORT when one creates it
using the constructor that takes a port number. If one uses the
default constructor and calls bind() and listen() separately, one can
tell it not to reuse the port.
