Problem/Motivation

That's a challenging one...

We have implemented our custom summary method and said inaccessible Paragraphs should not have the Edit button, but the summary.

Now turns out that for instance unpublished Paragraphs loose access and thus the summary doesn't render anything at all.
This even effects all nested children if you unpublish a parent Container Paragraph.

Proposed resolution

We need a summary for non-editable things.

The problem here is that

  • outputting the inaccessible elements could be a security breach.
  • the user might not even have view permission to the elements and still should see a summary.

Remaining tasks

Define precisely when we output elements that are inaccessible for the current user.

User interface changes

CommentFileSizeAuthor
#20 interdiff-2937660-18-20.txt649 bytesjohnchque
#20 empty_summary-2937660-20.patch5.51 KBjohnchque
#18 interdiff-2937660-17-18.txt1.17 KBjohnchque
#18 empty_summary-2937660-18.patch5.51 KBjohnchque
#17 interdiff-2937660-14-17.txt655 bytesjohnchque
#17 empty_summary-2937660-17.patch5.45 KBjohnchque
#14 interdiff-2937660-9-14.txt1.33 KBjohnchque
#14 empty_summary-2937660-14.patch5.41 KBjohnchque
#9 empty_summary-2937660-9-interdiff.txt2.22 KBBerdir
#9 empty_summary-2937660-9.patch5.14 KBBerdir
#6 interdiff-2937660-4-6.txt1.06 KBjohnchque
#6 empty_summary-2937660-6.patch5 KBjohnchque
#4 empty_summary-2937660-4.patch4.78 KBjohnchque
#4 empty_summary-2937660-4-test-only.patch3 KBjohnchque
paragraph-summary-link.png4.63 KBmiro_dietiker
parargaph-summary-container.png35.27 KBmiro_dietiker
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miro_dietiker created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Parent issue: » #2862083: [META] Improve paragraphs summary
miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Priority: Normal » Major

Discussed and decided to improve the summary:

  • Check field view access in the loop
  • Check entity target view access
  • Skip view access check of the Paragraph itself

Since this raises the complexity, chances are that we need summary caching to keep the UI fast in this issue:
#2935134: [meta] Improve performance of paragraphs widget

At least before commit, we should do profiling.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque commented
Status: Active » Needs review
FileSize
empty_summary-2937660-4-test-only.patch3 KB
empty_summary-2937660-4.patch4.78 KB

This should work, tests added. :)

Status: Needs review » Needs work

The last submitted patch, 4: empty_summary-2937660-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque commented
Status: Needs work » Needs review
FileSize
empty_summary-2937660-6.patch5 KB
interdiff-2937660-4-6.txt1.06 KB

Not really sure why this is failing, this works fine locally.

Status: Needs review » Needs work

The last submitted patch, 6: empty_summary-2937660-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Assigned: Unassigned » Berdir
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Assigned: Berdir » Unassigned
Status: Needs work » Needs review
FileSize
empty_summary-2937660-9.patch5.14 KB
empty_summary-2937660-9-interdiff.txt2.22 KB

Not sure what the problem is. But the default widget is radios which looks weird and is not how you'd actually configure a published status. So lets try with a checkbox instead. Works fine locally.

There are still more things about the status that we should consider, one is that this still displays an info in the backend summary as an icon that the paragraph is not accessible by you, should probably instead check for status specifically and provide a better message. But that's like a different issue.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque commented

Yeah, the icon can have a better message by checking the status. +1 for that. Anything else to do in this issue?

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque commented
Related issues: +#2940709: Change icon description to reflect paragraph status

Adding related issue for icon description.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Issue tags: +needs profiling

I'd like to do some profiling on this, the access checks on each field do add a bit of overhead. Wondering if a possibly simple improvement would be to special case the base fields and only loop over configurable fields. Will have a look.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work
Issue tags: -needs profiling

Profiled.

As I feared, I'm seeing a 60% increase on a page with a lot of paragraphs being displayed.

What I've noticed before is that we loop over base fields but we never actually add anything from them anyway. If we add code to skip them, then that about levels out the difference and in my testing, it was just 2% slower. Like this:

> if ($field_definition instanceof BaseFieldDefinition || !$this->get($field_name)->access('view')) {

also include a comment as to why.

This also means we can skip a check a bit later that excludes type/uid/revision_uid (the uid fields don't exist anyway)

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque commented
Status: Needs work » Needs review
FileSize
empty_summary-2937660-14.patch5.41 KB
interdiff-2937660-9-14.txt1.33 KB

Indeed, added and commented. :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Comment could maybe be a bit better, lets see what Miro thinks.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Reviewed & tested by the community » Needs work

So we had lots of discussions about edge cases, known limitations and fun problems such as multilingual fallbacks...

But that can be all covered in one of the many follow-ups (tm) we have to create based from our learnings... ;-)

...except this:

+++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
@@ -729,7 +729,6 @@ class ParagraphsWidget extends WidgetBase {
-                '#access' => $paragraphs_entity->access('view'),

Let's check here for edit OR view permissions to avoid potential security issues.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque commented
Status: Needs work » Needs review
FileSize
empty_summary-2937660-17.patch5.45 KB
interdiff-2937660-14-17.txt655 bytes

Adding edit access check. :)

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque commented
FileSize
empty_summary-2937660-18.patch5.51 KB
interdiff-2937660-17-18.txt1.17 KB

Oops sorry, it should be better now.

Status: Needs review » Needs work

The last submitted patch, 18: empty_summary-2937660-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque commented
Status: Needs work » Needs review
FileSize
empty_summary-2937660-20.patch5.51 KB
interdiff-2937660-18-20.txt649 bytes

My bad, it wasn't edit, it was update.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Was about to write that in a comment but crossposted with you. This should be fine now. Writing test coverage for this would probably be quite tricky, seems enough to have the permission and making sure we can see summary of unpublished paragraphs.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Very nice, committing. :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.