strepsirrhini/libreoffice-online
3
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

wsd: reliable locking with async loading

Browse source 
Change-Id: Ida0f9159596fbd83793b646879d0a9c5599cb7f9
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
This commit is contained in:
Ashod Nakashian 2024-06-26 20:19:42 -04:00 committed by Michael Meeks
parent 8aee9caa6b
commit 20b6b94a32
3 changed files with 135 additions and 38 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

50
test/UnitWOPILock.cpp
View file

@ -148,7 +148,9 @@ public:
/// when the first view is read-only.
class UnitWopiLockReadOnly : public WopiTestServer
{
STATE_ENUM(Phase, LoadViewer, LoadEditor, Lock, WaitModify, Unlock, WaitUnload, Done) _phase;
STATE_ENUM(Phase, Connect, FirstCheckFileInfo, Lock, LoadViewer, WaitModify, Upload, Unlock,
WaitUnload, Done)
_phase;
std::string _lockState;
std::string _lockToken;
@ -158,7 +160,7 @@ class UnitWopiLockReadOnly : public WopiTestServer
public:
UnitWopiLockReadOnly()
: WopiTestServer("UnitWopiLockReadOnly")
, _phase(Phase::LoadViewer)
, _phase(Phase::Connect)
, _lockState("UNLOCK")
, _checkFileInfoCount(0)
, _viewCount(0)
@ -169,9 +171,10 @@ public:
Poco::JSON::Object::Ptr& fileInfo) override
{
// Make the first session the editor, subsequent ones read-only.
const bool firstView = _checkFileInfoCount == 0;
++_checkFileInfoCount;
const bool firstView = _checkFileInfoCount == 1;
LOG_TST("CheckFileInfo: " << (firstView ? "viewer" : "editor"));
fileInfo->set("SupportsLocks", "true");
@ -191,6 +194,8 @@ public:
LOG_TST("In " << toString(_phase) << ", X-WOPI-Lock: " << lockToken << ", X-WOPI-Override: "
<< newLockState << ", for URI: " << request.getURI());
LOG_ASSERT_MSG(_checkFileInfoCount == 2, "Must have had two CheckFileInfo requests");
if (_phase == Phase::Lock)
{
LOK_ASSERT_EQUAL_MESSAGE("Expected X-WOPI-Override:LOCK", std::string("LOCK"),
@ -206,7 +211,7 @@ public:
LOK_ASSERT_EQUAL_MESSAGE("Document is not locked", std::string("LOCK"), _lockState);
LOK_ASSERT_EQUAL_MESSAGE("The lock token has changed", _lockToken, lockToken);
// TRANSITION_STATE(_phase, Phase::Done);
TRANSITION_STATE(_phase, Phase::WaitUnload);
// exitTest(TestResult::Ok);
}
else
@ -220,8 +225,8 @@ public:
std::unique_ptr<http::Response>
assertPutFileRequest(const Poco::Net::HTTPRequest& request) override
{
LOK_ASSERT_STATE(_phase, Phase::Unlock);
TRANSITION_STATE(_phase, Phase::WaitUnload);
LOK_ASSERT_STATE(_phase, Phase::Upload);
TRANSITION_STATE(_phase, Phase::Unlock);
// The document is modified.
LOK_ASSERT_EQUAL(std::string("true"), request.get("X-COOL-WOPI-IsModifiedByUser"));
@ -250,7 +255,7 @@ public:
++_viewCount;
if (_viewCount == 1)
{
LOK_ASSERT_STATE(_phase, Phase::LoadEditor);
LOK_ASSERT_STATE(_phase, Phase::LoadViewer);
TRANSITION_STATE(_phase, Phase::Lock);
LOG_TST("Loading second view (editor)");
@ -273,16 +278,15 @@ public:
{
LOG_TST("onDocumentModified: [" << message << ']');
if (_phase != Phase::Unlock)
// We get this twice, skip the second one.
if (_phase != Phase::Upload)
{
LOK_ASSERT_STATE(_phase, Phase::WaitModify);
TRANSITION_STATE(_phase, Phase::Unlock);
TRANSITION_STATE_MSG(_phase, Phase::Upload, "Disconnecting Editor, expecting PutFile");
// Simulate the editor closing browser.
deleteSocketAt(1);
}
// Simulate the editor closing browser.
LOG_TST("Disconnecting Editor");
deleteSocketAt(1);
return true;
}
@ -309,24 +313,30 @@ public:
{
switch (_phase)
{
case Phase::LoadViewer:
case Phase::Connect:
{
// Always transition before issuing commands.
TRANSITION_STATE(_phase, Phase::LoadEditor);
LOG_TST("Creating first connection");
TRANSITION_STATE_MSG(_phase, Phase::FirstCheckFileInfo,
"Creating first connection, expecting first CheckFileInfo");
initWebsocket("/wopi/files/0?access_token=anything");
LOG_TST("Creating second connection");
// With async loading, we download based the initial connection,
// ahead of the load command. By then, we have done CheckFileInfo
// and found out that this is an editor, and so must take the lock.
TRANSITION_STATE_MSG(
_phase, Phase::Lock,
"Creating second connection, expecting second CheckFileInfo+Lock");
addWebSocket();
LOG_TST("Loading first view (viewer)");
TRANSITION_STATE_MSG(_phase, Phase::LoadViewer, "Loading viewer");
WSD_CMD_BY_CONNECTION_INDEX(0, "load url=" + getWopiSrc());
break;
}
case Phase::LoadEditor:
case Phase::FirstCheckFileInfo:
case Phase::Lock:
case Phase::LoadViewer:
case Phase::WaitModify:
case Phase::Upload:
case Phase::Unlock:
case Phase::WaitUnload:
case Phase::Done:

114
wsd/DocumentBroker.cpp
View file

@ -904,7 +904,7 @@ bool DocumentBroker::download(
// Call the storage specific fileinfo functions
std::string templateSource;
bool userCanWrite = false;
#if !MOBILEAPP
std::chrono::milliseconds checkFileInfoCallDurationMs = std::chrono::milliseconds::zero();
WopiStorage* wopiStorage = dynamic_cast<WopiStorage*>(_storage.get());
@ -932,6 +932,7 @@ bool DocumentBroker::download(
wopiStorage->handleWOPIFileInfo(*wopiFileInfo, *_lockCtx);
_isViewFileExtension = COOLWSD::IsViewFileExtension(wopiStorage->getFileExtension());
userCanWrite = wopiFileInfo->getUserCanWrite();
if (session)
{
@ -1038,13 +1039,16 @@ bool DocumentBroker::download(
if (session)
broadcastLastModificationTime(session);
// Only lock the document on storage for editing sessions.
lockIfEditing(session, uriPublic, userCanWrite);
// Let's download the document now, if not downloaded.
std::chrono::milliseconds getFileCallDurationMs = std::chrono::milliseconds::zero();
if (!_storage->isDownloaded())
{
const Authorization auth =
session ? session->getAuthorization() : Authorization::create(uriPublic);
if (!doDownloadDocument(session, auth, templateSource, fileInfo.getFilename(),
if (!doDownloadDocument(auth, templateSource, fileInfo.getFilename(),
getFileCallDurationMs))
{
LOG_DBG("Failed to download or process downloaded document");
@ -1073,8 +1077,65 @@ bool DocumentBroker::download(
return true;
}
bool DocumentBroker::doDownloadDocument(const std::shared_ptr<ClientSession>& session,
const Authorization& auth,
void DocumentBroker::lockIfEditing(const std::shared_ptr<ClientSession>& session,
const Poco::URI& uriPublic, bool userCanWrite)
{
if (_lockCtx == nullptr || !_lockCtx->_supportsLocks || _lockCtx->_isLocked)
{
return; // Nothing to do.
}
if (session)
{
// If we have a session, isReadOnly() will be correctly set
// based on the URI (which may include a readonly permission),
// as well as the WOPI Info that we got above.
if (!session->isReadOnly())
{
LOG_DBG("Locking docKey [" << _docKey << "], which is editable");
std::string error;
if (!updateStorageLockState(*session, /*lock=*/true, error))
{
LOG_ERR("Failed to lock docKey [" << _docKey << "] with session ["
<< session->getId()
<< "] after downloading: " << error);
}
}
return;
}
// No Session yet, we need to rely on the URI and
// the WOPI Info we got above, explicitly.
bool isReadOnly = _isViewFileExtension || !userCanWrite;
if (!isReadOnly)
{
// See if we have permission override from the UI.
// Primarily used by mobile, which starts in read-only
// mode until the user clicks on the "edit" button.
for (const auto& param : uriPublic.getQueryParameters())
{
LOG_TRC("Query param: " << param.first << ", value: " << param.second);
if (param.first == "permission" && param.second == "readonly")
{
isReadOnly = true;
break;
}
}
if (!isReadOnly)
{
LOG_DBG("Locking docKey [" << _docKey << "], which is editable");
std::string error;
if (!updateStorageLockState(Authorization::create(uriPublic), error))
{
LOG_ERR("Failed to lock docKey [" << _docKey << "] in advance: " << error);
}
}
}
}
bool DocumentBroker::doDownloadDocument(const Authorization& auth,
const std::string& templateSource,
const std::string& filename,
std::chrono::milliseconds& getFileCallDurationMs)
@ -1094,18 +1155,6 @@ bool DocumentBroker::doDownloadDocument(const std::shared_ptr<ClientSession>& se
_docState.setStatus(DocumentState::Status::Loading); // Done downloading.
// Only lock the document on storage for editing sessions
// FIXME: why not lock before downloadStorageFileToLocal? Would also prevent race conditions
if (session && !session->isReadOnly())
{
std::string error;
if (!updateStorageLockState(*session, /*lock=*/true, error))
{
LOG_ERR("Failed to lock docKey [" << _docKey << "] with session [" << session->getId()
<< "] after downloading: " << error);
}
}
#if !MOBILEAPP
if (!processPlugins(localPath))
{
@ -1460,6 +1509,39 @@ void DocumentBroker::endRenameFileCommand()
endActivity();
}
bool DocumentBroker::updateStorageLockState(const Authorization& auth, std::string& error)
{
assert(_lockCtx && "Expected an initialized LockContext");
assert(_lockCtx->_supportsLocks && "Expected to have lock support");
assert(!_lockCtx->_isLocked && "Expected not to have locked already");
const StorageBase::LockUpdateResult result =
_storage->updateLockState(auth, *_lockCtx, /*lock=*/true, _currentStorageAttrs);
error = _lockCtx->_lockFailureReason;
switch (result)
{
case StorageBase::LockUpdateResult::UNSUPPORTED:
LOG_DBG("Locks on docKey [" << _docKey << "] are unsupported");
return true; // Not an error.
break;
case StorageBase::LockUpdateResult::OK:
LOG_DBG("Locked docKey [" << _docKey << "] successfully");
return true;
break;
case StorageBase::LockUpdateResult::UNAUTHORIZED:
LOG_ERR("Failed to " << "Locked docKey [" << _docKey
<< "]. Invalid or expired access token");
break;
case StorageBase::LockUpdateResult::FAILED:
LOG_ERR("Failed to " << "Locked docKey [" << _docKey << "] with reason [" << error
<< ']');
break;
}
return false;
}
bool DocumentBroker::updateStorageLockState(ClientSession& session, bool lock, std::string& error)
{
if (session.getAuthorization().isExpired())

9
wsd/DocumentBroker.hpp
View file

@ -626,8 +626,7 @@ private:
/// Actual document download and post-download processing.
/// Must be called only when creating the storage for the first time.
bool doDownloadDocument(const std::shared_ptr<ClientSession>& session,
const Authorization& auth, const std::string& templateSource,
bool doDownloadDocument(const Authorization& auth, const std::string& templateSource,
const std::string& filename,
std::chrono::milliseconds& getFileCallDurationMs);
@ -645,10 +644,16 @@ private:
bool isLoaded() const { return _docState.hadLoaded(); }
bool isInteractive() const { return _docState.isInteractive(); }
void lockIfEditing(const std::shared_ptr<ClientSession>& session, const Poco::URI& uriPublic,
bool userCanWrite);
/// Updates the document's lock in storage to either locked or unlocked.
/// Returns true iff the operation was successful.
bool updateStorageLockState(ClientSession& session, bool lock, std::string& error);
/// Take the lock before loading the first session, if we know we can edit.
bool updateStorageLockState(const Authorization& auth, std::string& error);
std::size_t getIdleTimeSecs() const
{
const auto duration = (std::chrono::steady_clock::now() - _lastActivityTime);