Skip to content

Commit

Permalink
wsd: reliable locking with async loading
Browse files Browse the repository at this point in the history 
Change-Id: Ida0f9159596fbd83793b646879d0a9c5599cb7f9
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
  • Loading branch information
@Ashod @mmeeks
Ashod authored and mmeeks committed Jul 2, 2024
1 parent 8aee9ca commit 20b6b94
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 38 deletions.
50 changes: 30 additions & 20 deletions test/UnitWOPILock.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@ class UnitWopiLock : public WopiTestServer
/// 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;
Expand All @@ -158,7 +160,7 @@ class UnitWopiLockReadOnly : public WopiTestServer
public:
UnitWopiLockReadOnly()
: WopiTestServer("UnitWopiLockReadOnly")
, _phase(Phase::LoadViewer)
, _phase(Phase::Connect)
, _lockState("UNLOCK")
, _checkFileInfoCount(0)
, _viewCount(0)
Expand All @@ -169,9 +171,10 @@ class UnitWopiLockReadOnly : public WopiTestServer
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");
Expand All @@ -191,6 +194,8 @@ class UnitWopiLockReadOnly : public WopiTestServer
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"),
Expand All @@ -206,7 +211,7 @@ class UnitWopiLockReadOnly : public WopiTestServer
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
Expand All @@ -220,8 +225,8 @@ class UnitWopiLockReadOnly : public WopiTestServer
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"));
Expand Down Expand Up @@ -250,7 +255,7 @@ class UnitWopiLockReadOnly : public WopiTestServer
++_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)");
Expand All @@ -273,16 +278,15 @@ class UnitWopiLockReadOnly : public WopiTestServer
{
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;
}

Expand All @@ -309,24 +313,30 @@ class UnitWopiLockReadOnly : public WopiTestServer
{
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:
Expand Down
114 changes: 98 additions & 16 deletions wsd/DocumentBroker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -932,6 +932,7 @@ bool DocumentBroker::download(

wopiStorage->handleWOPIFileInfo(*wopiFileInfo, *_lockCtx);
_isViewFileExtension = COOLWSD::IsViewFileExtension(wopiStorage->getFileExtension());
userCanWrite = wopiFileInfo->getUserCanWrite();

if (session)
{
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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)
Expand All @@ -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))
{
Expand Down Expand Up @@ -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())
Expand Down
9 changes: 7 additions & 2 deletions wsd/DocumentBroker.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -626,8 +626,7 @@ class DocumentBroker : public std::enable_shared_from_this<DocumentBroker>

/// 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);

Expand All @@ -645,10 +644,16 @@ class DocumentBroker : public std::enable_shared_from_this<DocumentBroker>
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);
Expand Down

0 comments on commit 20b6b94

Please sign in to comment.