locking problems, again

[Michael Smith <ms...@xn.com.au>]

Hi all,

As of tuesday (with a bunch of commits concerning locking), locking
seems to not work properly (or, more accurately - UNLOCKING doesn't
work, at least from the webdav servlet).

What appears to be happening is that the unlock() method in the LockImpl
class does a series of 3 checks, which is sensible enough. The final of
thes, checkLockToken(), calls slideToken.checkLockToken(). This then
fails, because there's no reason that that particular slideToken would
have been used for the lock. 

Surely we should be either not doing this at all (because when we do the
actual lock removal, if the lock isn't present, it'll fail), OR we
should be consulting the stores - the store knows whether there's a
lock, nothing else is guaranteed to (unless I'm missing something).

For the moment, I've just got rid of condition3 (the checkLockToken()
one) in LockImpl.unlock(), which seems to work - I don't want to commit
this without an explanation of why it was like this to begin with,
though.

Michael

p.s. I'm going into hiding to study for exams after today, so if I don't
get a response soon (i.e. within a few hours), I probably won't read it
for a couple of weeks - Remy, please a) explain why I'm crazy, or b) fix
it, since I won't be around to commit stuff.

Re: locking problems, again

[Michael Smith <ms...@xn.com.au>]

 
> > What appears to be happening is that the unlock() method in the LockImpl
> > class does a series of 3 checks, which is sensible enough.
> 
> Before, it wasn't checking anything, which was bad because DELETE started by
> unlocking all the locks which were on a resource before deleting the
> resource itself (so you could delete locked resources).

Ahh... I didn't realise that. ok, seems reasonable now.

> 
> > The final of
> > thes, checkLockToken(), calls slideToken.checkLockToken(). This then
> > fails, because there's no reason that that particular slideToken would
> > have been used for the lock.
> 
> That one should be ok.
> With the WebDAV servlet, the client has to submit the lock's lock token in
> the unlock command, and the lock token ends up in the Slide token.
> If you don't use WebDAV, SlideToken.isEnforceLockTokens() should return
> false, and checkLockToken() will always return true.

That was the key I was missing - I didn't realise that
isEnforceLockTokens() should return false (and indeed, my code which
uses slide directly does not - I'll fix that), so none of this ended up
making sense to me :-)
The fact that the lock token wasn't ending up the slide token also
confused me.


> 
> > Surely we should be either not doing this at all (because when we do the
> > actual lock removal, if the lock isn't present, it'll fail), OR we
> > should be consulting the stores - the store knows whether there's a
> > lock, nothing else is guaranteed to (unless I'm missing something).
> 
> You have to check the lock token to prevent :
> - being able to delete or overwrite (using copy or move) a locked resource
> - lock stealing
> 
> Also, it's required by WebDAV, and I thought it was a good concept, so I
> moved the token check to the core (instead of doing it in the WebDAV
> servlet).

Yes, this makes sense now that I understand why it's neccesary to check
at this point.

Just noticed a commit - thanks for fixing this!

> 
> Good luck for your exams :)

Thanks a lot.

Michael

Re: locking problems, again

[Remy Maucherat <re...@apache.org>]

> Hi all,
>
> As of tuesday (with a bunch of commits concerning locking), locking
> seems to not work properly (or, more accurately - UNLOCKING doesn't
> work, at least from the webdav servlet).

Well, with DAV E, UNLOCK is working (at least for me).

I just installed one of the components of Office2k, and tried with that, and
indeed UNLOCK is broken :(

I didn't try the Slide client. (it could be a bug with the client)

> What appears to be happening is that the unlock() method in the LockImpl
> class does a series of 3 checks, which is sensible enough.

Before, it wasn't checking anything, which was bad because DELETE started by
unlocking all the locks which were on a resource before deleting the
resource itself (so you could delete locked resources).

> The final of
> thes, checkLockToken(), calls slideToken.checkLockToken(). This then
> fails, because there's no reason that that particular slideToken would
> have been used for the lock.

That one should be ok.
With the WebDAV servlet, the client has to submit the lock's lock token in
the unlock command, and the lock token ends up in the Slide token.
If you don't use WebDAV, SlideToken.isEnforceLockTokens() should return
false, and checkLockToken() will always return true.

> Surely we should be either not doing this at all (because when we do the
> actual lock removal, if the lock isn't present, it'll fail), OR we
> should be consulting the stores - the store knows whether there's a
> lock, nothing else is guaranteed to (unless I'm missing something).

You have to check the lock token to prevent :
- being able to delete or overwrite (using copy or move) a locked resource
- lock stealing

Also, it's required by WebDAV, and I thought it was a good concept, so I
moved the token check to the core (instead of doing it in the WebDAV
servlet).

> For the moment, I've just got rid of condition3 (the checkLockToken()
> one) in LockImpl.unlock(), which seems to work - I don't want to commit
> this without an explanation of why it was like this to begin with,
> though.

I think it's another bug which is causing that problem.

> Michael
>
> p.s. I'm going into hiding to study for exams after today,

Good luck for your exams :)

> so if I don't
> get a response soon (i.e. within a few hours),

Deal. I'll try to fix the bug with Office very soon, at least.

> I probably won't read it
> for a couple of weeks - Remy, please a) explain why I'm crazy, or b) fix
> it, since I won't be around to commit stuff.

Ok.

Remy