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
Throw if document load fails. Ignoring such a scenario would lead
to crash later in the document load process.
Change-Id: Ie80fc4f26e08e4920d4c04726f947edd2837a0cf
Produces a human-readable logging string describing a LOOL protocol
frame. To be used for re-factoring snippets of similar code that
sometimes print also the flags, sometimes pointlessly print the
(binary) contents of a CLOSE frame (as if it was text), etc.
It has always been the intent that getAbbreviatedMessage() is for
producing human-readbale nice output for logging purposes only. But
this has not been mentioned, so (naturally) the function has been
misused. Sigh.
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.
The waiting for the expected number of loolkit processes after that
test never succeeded. That apparently was caused by the fact that the
loolwsd process is waiting for up to COMMAND_TIMEOUT_MS (five seconds
currently) for an auto-save of the document to succeed, and that never
happens in this case.
countLoolKitProcesses() on the other hand was waiting only a bit over
three seconds (calculated in a complicated way from POLL_TIMEOUT_MS
and a few magic numbers) for the number of loolkit processes to reach
the expected number. Or something like that.
COMMAND_TIMEOUT_MS=5000 and POLL_TIMEOUT_MS=COMMAND_TIMEOUT_MS/10 =>
500. We used to have in countLoolKitProcesses():
sleepMs=POLL_TIMEOUT_MS/3 => 166 and repeat=(3000/sleepMs)+1 =>
19. 19*166 => 3154.
Fix by calculating the max time to wait for the expected number of
loolkit processes in countLoolKitProcesses() in a different way, so
that it is always longer than COMMAND_TIMEOUT_MS.
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.)
Doing a plain CPPUNIT_TEST_NAME='somettest' make check will
result in executing all the tests in external test suite. This is
a problem when one wants to execute only internal tests (unit-*
ones) as test harness first executes those followed by all of the
tests in external test-suites.
Lets execute all the tests only when no CPPUNIT_TEST_NAME is
provided, and ignore when it is provided but no match is found.
Change-Id: I7e40b6f3124e6965a86cfb6395d246df3b5c17ba
There doesn't seem to be any failure on this test anymore.
Consecutive 20 runs of this test gives no failure, so lets enable
it again.
Change-Id: I77ddd1c36d18162bdc75fd24d51c1a2df22f749d
addSession() now also loads the document, so querying docbroker
for load duration before it doesn't make any sense.
Change-Id: I3c60bef5e2054878ba695b8f76b6800cdedffe8d
Pass std::string instead of char* and length where
a string is always constructed anyway. Also cleaner
and safer code.
Change-Id: I1c9341e2c81bbdb7adeb29d3fba59849b2617e95
Reviewed-on: https://gerrit.libreoffice.org/29954
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
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>
When upon cleanup we segfault we want to avoid
forcible exit using _Exit(). This was done by
an unconditional wait (sleep).
This patch changes that mechanism into a latch
(mutex) that traps the exit when the sig handler
is invoked, therby preventing exit as long as
necessary for the sighandler to finish.
Change-Id: Ifc8e972be12645a1b310d4cb4e3a4172afc31327
Reviewed-on: https://gerrit.libreoffice.org/29945
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
No need to do a partial cleanup in the kit
when forkit recycles the disk. This avoid
both programatic errors (as the comments in
the removed code explains) as well as hammering
the disk from multiple processes.
By leaving all disk cleanup to forkit we
guarantee safety and that only one process
does disk cleanup, and sequentially at that.
N.B. Kit processes are still responsible for
setting up the jail and the LO binaries etc.
So disk IO is not strictly sequential from a
single process.
Change-Id: Ia6768ab87df71a83a6676c3d52da3d6797f717fc
Reviewed-on: https://gerrit.libreoffice.org/29944
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
