πŸ‡¬πŸ‡§United Kingdom @andy inman

Gloucestershire, UK
Account created on 14 December 2007, over 17 years ago
#

Merge Requests

Recent comments

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Hi @pankajroyal , thanks for the contribution, I'll review it when I can.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Hi @Vardan Harutyunyan I’ve retested, as before, and confirmed that access to the AJAX callback is now suitably restricted.

About the confirmation message - I agree it makes sense to keep it simple and standard. I did some reading: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Ajax!MessageComma... - the part // A status message added an alternate location. part would be relevant - but would need a suitable location in the page. I notice that, when you click copy-to-clipboard, the Drupal AJAX spinner appears for a moment, inside the paragraph that was copied - that’s where the message could be shown. But I have no idea how you would identify that element. So, I’m sure it would be possible, but too much work for a small benefit.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Hi @Vardan Harutyunyan I’ve tested it with Drupal 10 using the https://dev-lcmd10.pantheonsite.io/ site.

Using copy-to-clipboard from the context menu works great. I tried pasting to into the same paragraph and into a different paragraph on a new node. Both ok. Check Mark

But I think there’s a potential security issue: the new route at /paragraphs-clipboard/paragraphs/{paragraph}/copy is accessible to users who would not have access to view the paragraph being copied. So maybe instead of using permission access content, you need to do an access check on the paragraph. It seems that Paragraphs Edit does something like that - I haven’t looked at the code, but looked at the related URLs from the context menu, and tried accessing them as anonymous user. I’m using this test page: https://lcmd10.lndo.site/en/node/20

Edit: https://dev-lcmd10.pantheonsite.io/en/paragraphs_edit/node/20/paragraphs... β†’ Access Denied. Check Mark

Clone: https://lcmd10.lndo.site/en/paragraphs_edit/node/20/paragraphs/4/clone?d... β†’ Access Denied. Check Mark

Delete: https://lcmd10.lndo.site/en/paragraphs_edit/node/20/paragraphs/4/delete?... β†’ Access Denied. Check Mark

Copy to clipboard: https://lcmd10.lndo.site/en/paragraphs-clipboard/paragraphs/4/copy?desti... β†’ OK, returns JSON. Cross Mark

The JSON returned doesn’t seem to include any sensitive, data, but nonetheless, this behaviour doesn’t seem appropriate.

Finally, just a minor point: The Paragraph has been copied to clipboard message appears right at the top of the page, which is often not visible. It’s definitely useful to show a confirmation that the copy happened - maybe use a JS alert box instead? Or, insert at the top of the paragraph that was copied, but I imagine that would be a lot more work.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Hi @finn-lewis and @pankajroyal

My thoughts on how this could be done - I hope this is some help:

Add a record back into the module's queue - more or less what's done by the existing defer() method in WebformDeferredProcessor.php. The other handler (the one making the API request) could write directly to the webform_handler_defer_submissions queue, but of course the right way would be some callable method inside the Webform Handler Defer module - probably added to WebformDeferredProcessor.

Potentially there could be more than one handler, per submission, that needs to be retried - ideally use a single queue record for all of them, but using multiple queue records would probably be simpler.

Don't retry until some some reasonable time has passed. Probably, store a timestamp in the queue record - earliest time to retry. Add logic to processSubmissions() to make it skip over any records which should not be retried yet. Maybe also some maximum number of retries or maximum time.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

@gdana Thanks for the feedback. If you have a moment, I would really appreciate it if you could PM me with a bit more info on how you're using it and the benefits obtained. Or, post the same here, if you prefer and it's relevant to others following this thread. Thanks!

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Fixed in 1.1.x because we no longer access anything in the private files directory.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

From further testing today, I've found that it's possible to get the same problem without using copy/paste, also without paragraphs clipboard installed.

So, cause is not paragraphs clipboard. It may be due to an invalid configuration, or some issue with Layout Paragraphs module, but I'm closing this ticket - works as designed.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

I've tested support for Layout Paragraphs using the Layout Paragraphs widget together with the Layout Paragraphs Clipboard submodule.

Basic copy/paste works, so I'm marking this issue as RTBC. Separate issue #3488737 still needs to be resolved.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Hi @hhvardan. I've tested 3485091-improve-behaviour-for:
* Confirmed the copy to clipboard link is disabled for a new unsaved paragraph using the default widget.
* Confirmed the copy to clipboard icon/button is not shown new unsaved paragraph using the Layout Paragraphs widget.

So, the issue here is fixed.

Additional: I realised while testing, there's a related issue: if you change an existing paragraph without saving, then of course copy to clipboard will copy the original version, not the edited version. I think we have to accept that as a limitation, at least for the initial release. Later, if we can make it to copy the edited version of a paragraph, and maybe even a new one, that would be great.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Just a note that we have expect to be releasing a module which will do this, or at least meet the same objective, I think. It's already "finished" and working for a specific client. It just needs a bit more work to generalise and polish, before publishing.

It takes a different (and more flexible) approach to what's discussed here, but if I'm understanding correctly what the need is in this ticket, then I think it will meet it.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Retested with the latest (1.0.0-beta5), all ok now.

* It's no longer possible to paste a previously copied paragraph into a translation.
* Plegar means collapase, not paste - my mistake - that button is meant to be there.
* The yellow triangle warning shown for a collapsed paragraph means "You have unsaved changes on this Paragraph item" - seems wrong, as no changes were made, but it's not related to this module - still shown if the module is uninstalled.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Ok, I've now found in the README:

Automated Testing Kit has a configuration page located at /admin/config/development/automated_testing_kit/edit (stub for now).

So that explains the error. Suggestion: remove the Drupal menu item completely - better than having it trigger an error page IMHO.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

@ yorchperaza

> seems like not is compatible with layout_paragraphs

Correct - I spoke/wrote too soon. But they're in the process of adding that as a submodule.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

I'm hitting what seems to be the same issue, but when using the Responsive Grid rather than table format. Tried patch #65, no change. Should I log this is a separate issue?

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

A big negative for Lando: security weakness, because it makes your entire home directory accessible (read/write) to anything running in the Lando container.

https://github.com/lando/lando/issues/2635#issuecomment-860211387

This behaviour seems to have maybe changed in some recent version, for Linux: on my Ubuntu 22.04 machine, I see an empty /user directory, when testing:

<code>
lando ssh

ls -al /user
</code>

But under MacOS, the documented behaviour is present.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Patch #6 worked for me - the warning "Missing proxy class 'Drupal\book_tree_menu\ProxyClass\oscBookManager' for lazy service 'book.manager'" was previously appearing after every drush cr, it's gone after applying the patch.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Reporting another successful result with current dev-4.x and Drupal 10.1.2.

One detail: about configuration of entityID, we previously had:

        // Can be NULL/unset, in which case an entity ID is generated based on the metadata URL.
        'entityID' => NULL,

It seems the NULL/undefined option is no longer supported - to achieve the same result, I needed to change that to:

        // Can be NULL/unset, in which case an entity ID is generated based on the metadata URL.
        'entityID' => "https://{$_SERVER['HTTP_HOST']}:443/simplesaml/module.php/saml/sp/metadata.php/default-sp",
πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Looks like there's a failing test, likely needs updating to match new logic

Confirmed - access tests were checking edit-own and delete-own permission based on revision uid rather than entity uid. I've changed that. I've also added a couple of tests cases to check update and delete when user has the edit/delete-own permission but is not the owner.

Before marking as RTBC, I think it's worth at least considering the possibility that somebody out there may have an access control strategy that would be broken by these changes. That seems very unlikely to me, but I suppose it's not impossible. In the worst case, people could get edit/delete access to micro-content that they're not supposed to be able to change. There's no simple solution - I'm just suggesting a pause for thought - maybe wait for a bit more input from others.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

Looks like there's a failing test, likely needs updating to match new logic

Confirmed - access tests were checking edit-own and delete-own permission based on revision uid rather than entity uid. I've changed that. I've also added a couple of tests cases to check update and delete when user has the edit/delete-own permission but is not the owner.

Before marking as RTBC, I think it's worth at least considering the possibility that somebody out there may have an access control strategy that would be broken by these changes. That seems very unlikely to me, but I suppose it's not impossible. In the worst case, people could get edit/delete access to micro-content that they're not supposed to be able to change. There's no simple solution - I'm just suggesting a pause for thought - maybe wait for a bit more import from others.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

I have learned: don't right-click the Open MR button hoping to check the URL - that will trigger the button action.

πŸ‡¬πŸ‡§United Kingdom andy inman Gloucestershire, UK

We're also seeing this error with Authorization - Group 8.x-1.0 when trying to add a group.

Possibly related modules installed:

  • Drupal Roles Authorization 8.x-1.0-beta6
  • LDAP Authorization Provider 8.x-4.3
  • Group (group) 8.x-1.5

I need to investigate further, and will post more info when I have it.

Production build 0.71.5 2024