Content lock create access does not account for content translations

The merge request does not lock the entity when adding a content translation

Does indeed resolve the error.

This fix is not quite right. I'm work on comprehensive test coverage atm.

Firstly the module only supports locking on the translation level if the conflict module is enabled otherwise it's on the entity level and you should not be allowed to add or edit translations of a locked entity. So the logic needs to account for that.

Secondly we'd stopped testing with the conflict module because it was not D11 compatible - it is now - so I've fixed that in 📌 Re-enable testing with the conflict module Active

I've added test coverage of both a user without editing permission and the user with editing permission creating a translation and I'm unable to reproduce the error. There's no message about something going wrong.

What am I missing?

I should have clarified the issue occurs when using the 'Lock form using JS' option.

I moved the test coverage to \Drupal\Tests\content_lock\FunctionalJavascript\ContentLockNodeTest::testContentLockWithTranslations with this option enabled so the issue is triggered correctly.

@idebr awesome thanks! Now we can fix it correctly!

Turns there are two problems here:
1. The route access check for the content lock JS lock route is not correct for translations
2. The libraries for the content_lock.js are not correct.

I've fixed 1 by adding a hash check... so if you have access to trigger the form alter that adds the js then you'll have access to check/lock the entity if possible, I think this is reasonable but more heads thinking about this would be great.

Fixing 2 is easy,.

@idebr I think this fix is ready. It would be great to get your feedback on this before I merge.

I expected a CSRF token instead of a custom hash. Can you elaborate why the custom hash is preferred in this case?

@idebr because the access is specific for the content being locked. So a user can't just futz with the params in the URL to lock other nodes... they can only lock the node for the form they're visiting. That said it would be great if the hash was specific to the user's session too (like CSRF). Or maybe the fact it is unique for the logged in user is enough. It's not like users can share hashes. I will add docs so the reason for the hash is more apparent by reading the code.

@idebr actually you are correct. There is no reason to not use CSRF protection. Everything we need to validate is part of the path. One tricky thing is placeholdering a drupalSettings but I've manage to work a way around this.

Come up with an even better way of dealing with placeholdering and drupalSettings... just not using drupalSettings. Also it means we no longer have to deal with matching form IDs because we get the form ID from where the hidden element is. Ends up making things simpler.

Nice to see the CSRF token in place

What is the use for the destination query parameter in content_lock_js_url? This URL is only called in the background and not something a user actually visits?

The destination is used on links in the messages that are display via ajax to the user - see \Drupal\content_lock\Controller\ContentLockController::createLockCall()