tdf#82744: fix WebDAV lock/unlock behaviour - part 1

There are some areas in ucb outside the issue scope that should later
be addressed, among them:

- in ucb/webdav make flag m_bTransient working right, currently lock
option for WebDAV server not supporting it is suboptimal: there are
not needed lock requests;

- change the method the modified file is checked against the old
one, using DAV:etag instead of the DateTime;

- some http status code returned by the server don't seem to be
managed;

- during WebDAV operation some redundant request of properties is
carried out.
Probably some clean up to remove these not needed transactions
is to be done.
Accessing only those really supported by the referenced href would
be better.

Changes done to the code in ucb, in no particular order

- remove current WebDAV lock management

- have the lock/unlock working correctly when the webdav resource
is first created: in the case of creation is the first lock on
the non existent resource that actually creates it

- fix a problem while fetching WebDAV properties.
If a single WebDAV non-cached property was requested, it would
 not be fetched from the server without this fix.

- change the lock owner name.
This should probably be different. Something to be discussed.
This same string can be read by all the applications accessing the
lock.

Spec reference is:
RFC4918 [2007]: '14.17.  owner XML Element'
link (as of 20150713):
http://tools.ietf.org/html/rfc4918#section-14.17

- manage WebDAV locked file exception directly while locking.
The ucb::InteractiveLockingLockedException is thrown directly
when detected by the lock command, to avoid the user interaction
activated by the cancelCommandExecution method.

- terminate gracefully if WebDAV lock/unlock is not supported

Change-Id: I4c5cd652ac7b2c32fb1c3307c34fc31d1a0305c2
Reviewed-on: https://gerrit.libreoffice.org/17080
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>

2 files changed, 91 insertions, 18 deletions

diff --git a/ucb/source/ucp/webdav-neon/webdavcontent.cxx b/ucb/source/ucp/webdav-neon/webdavcontent.cxx
index 8862b42ce30a..3fc1755a95e8 100644
--- a/ucb/source/ucp/webdav-neon/webdavcontent.cxx
+++ b/ucb/source/ucp/webdav-neon/webdavcontent.cxx
@@ -93,8 +93,6 @@ using namespace com::sun::star;
 using namespace webdav_ucp;
 
 
-
-
 // Content Implementation.
 
 
@@ -112,7 +110,6 @@ Content::Content(
   m_eResourceType( UNKNOWN ),
   m_pProvider( pProvider ),
   m_bTransient( false ),
-  m_bLocked( false ),
   m_bCollection( false ),
   m_bDidGetOrHead( false )
 {
@@ -145,7 +142,6 @@ Content::Content(
   m_eResourceType( UNKNOWN ),
   m_pProvider( pProvider ),
   m_bTransient( true ),
-  m_bLocked( false ),
   m_bCollection( isCollection ),
   m_bDidGetOrHead( false )
 {
@@ -166,8 +162,6 @@ Content::Content(
 // virtual
 Content::~Content()
 {
-    if (m_bLocked)
-        unlock(uno::Reference< ucb::XCommandEnvironment >());
 }
 
 
@@ -504,10 +498,6 @@ uno::Any SAL_CALL Content::execute(
 
         aRet = open( aOpenCommand, Environment );
 
-        if ( (aOpenCommand.Mode == ucb::OpenMode::DOCUMENT ||
-              aOpenCommand.Mode == ucb::OpenMode::DOCUMENT_SHARE_DENY_WRITE) &&
-             supportsExclusiveWriteLock( Environment ) )
-            lock( Environment );
     }
     else if ( aCommand.Name == "insert" )
     {
@@ -610,12 +600,18 @@ uno::Any SAL_CALL Content::execute(
 
         post( aArg, Environment );
     }
-    else if ( aCommand.Name == "lock" && supportsExclusiveWriteLock( Environment ) )
+    else if ( aCommand.Name == "lock" )
     {
 
         // lock
 
-
+        // supportsExclusiveWriteLock()  does not work if the file is being created.
+        // The lack of lock functionality is taken care of inside lock(),
+        // a temporary measure.
+        // This current implementation will result in a wasted lock request issued to web site
+        // that don't support it.
+        // TODO: need to rewrite the managing of the m_bTransient flag, when the resource is non yet existent
+        // and the first lock on a non existed resource first creates it then lock it.
         lock( Environment );
     }
     else if ( aCommand.Name == "unlock" && supportsExclusiveWriteLock( Environment ) )
@@ -1318,7 +1314,13 @@ uno::Reference< sdbc::XRow > Content::getPropertyValues(
                         while ( it != end )
                         {
                             if ( *it == rName )
+                            {
+                                // the failed property in cache is the same as the requested one,
+                                // so add it to the requested properties list
+                                aProperties[ nProps ] = rProperties[ n ];
+                                nProps++;
                                 break;
+                            }
 
                             ++it;
                         }
@@ -2773,6 +2775,21 @@ void Content::lock(
         const uno::Reference< ucb::XCommandEnvironment >& Environment )
     throw( uno::Exception, std::exception )
 {
+// prepare aURL to be used in exception, see below
+    OUString aURL;
+    if ( m_bTransient )
+    {
+        aURL = getParentURL();
+        if ( aURL.lastIndexOf('/') != ( aURL.getLength() - 1 ) )
+            aURL += "/";
+
+        aURL += m_aEscapedTitle;
+    }
+    else
+    {
+        aURL = m_xIdentifier->getContentIdentifier();
+    }
+
     try
     {
         std::unique_ptr< DAVResourceAccess > xResAccess;
@@ -2783,7 +2800,7 @@ void Content::lock(
 
         uno::Any aOwnerAny;
         aOwnerAny
-            <<= OUString("http://ucb.openoffice.org");
+            <<= OUString("LibreOffice - http://www.libreoffice.org/");
 
         ucb::Lock aLock(
             ucb::LockScope_EXCLUSIVE,
@@ -2795,7 +2812,6 @@ void Content::lock(
             uno::Sequence< OUString >() );
 
         xResAccess->LOCK( aLock, Environment );
-        m_bLocked = true;
 
         {
             osl::Guard< osl::Mutex > aGuard( m_aMutex );
@@ -2804,6 +2820,43 @@ void Content::lock(
     }
     catch ( DAVException const & e )
     {
+        // check if the exception thrown is 'already locked'
+        // this exception is mapped directly to the ucb correct one, without
+        // going into the cancelCommandExecution() user interaction
+        // this exception should be managed by the issuer of 'lock' command
+        switch(e.getError())
+        {
+        case DAVException::DAV_LOCKED:
+        {
+            throw(ucb::InteractiveLockingLockedException(
+                      OUString( "Locked!" ),
+                      static_cast< cppu::OWeakObject * >( this ),
+                      task::InteractionClassification_ERROR,
+                      aURL,
+                      false ));
+        }
+        break;
+        case DAVException::DAV_HTTP_ERROR:
+            //grab the error code
+            switch( e.getStatus() )
+            {
+                // this returned error is part of base http 1.1 RFCs
+            case SC_NOT_IMPLEMENTED:
+            case SC_METHOD_NOT_ALLOWED:
+                // this is a temporary measure, means the lock method is not supported
+                // TODO: fix the behaviour of m_bTransient when the file is first created
+                return;
+                break;
+            default:
+                //fallthrough
+                ;
+            }
+            break;
+        default:
+            //fallthrough
+            ;
+        }
+
         cancelCommandExecution( e, Environment, false );
         // Unreachable
     }
@@ -2823,7 +2876,6 @@ void Content::unlock(
         }
 
         xResAccess->UNLOCK( Environment );
-        m_bLocked = false;
 
         {
             osl::Guard< osl::Mutex > aGuard( m_aMutex );
@@ -2832,6 +2884,28 @@ void Content::unlock(
     }
     catch ( DAVException const & e )
     {
+        switch(e.getError())
+        {
+        case DAVException::DAV_HTTP_ERROR:
+            //grab the error code
+            switch( e.getStatus() )
+            {
+                // this returned error is part of base http 1.1 RFCs
+            case SC_NOT_IMPLEMENTED:
+            case SC_METHOD_NOT_ALLOWED:
+                // this is a temporary measure, means the lock method is not supported
+                // TODO: fix the behaviour of m_bTransient when the file is first created
+                return;
+                break;
+            default:
+                //fallthrough
+                ;
+            }
+            break;
+        default:
+            //fallthrough
+            ;
+        }
         cancelCommandExecution( e, Environment, false );
         // Unreachable
     }
@@ -3051,7 +3125,7 @@ uno::Any Content::MapDAVException( const DAVException & e, bool bWrite )
                 static_cast< cppu::OWeakObject * >( this ),
                 task::InteractionClassification_ERROR,
                 aURL,
-                sal_False ); // not SelfOwned
+                false ); // not SelfOwned
         break;
 
     case DAVException::DAV_LOCKED_SELF:
@@ -3061,7 +3135,7 @@ uno::Any Content::MapDAVException( const DAVException & e, bool bWrite )
                 static_cast< cppu::OWeakObject * >( this ),
                 task::InteractionClassification_ERROR,
                 aURL,
-                sal_True ); // SelfOwned
+                true ); // SelfOwned
         break;
 
     case DAVException::DAV_NOT_LOCKED:
diff --git a/ucb/source/ucp/webdav-neon/webdavcontent.hxx b/ucb/source/ucp/webdav-neon/webdavcontent.hxx
index 85f2b3022f74..45605899951d 100644
--- a/ucb/source/ucp/webdav-neon/webdavcontent.hxx
+++ b/ucb/source/ucp/webdav-neon/webdavcontent.hxx
@@ -91,7 +91,6 @@ class Content : public ::ucbhelper::ContentImplHelper,
     ResourceType      m_eResourceType;
     ContentProvider*  m_pProvider; // No need for a ref, base class holds object
     bool              m_bTransient;
-    bool              m_bLocked;
     bool              m_bCollection;
     bool              m_bDidGetOrHead;
     std::vector< OUString > m_aFailedPropNames;