Problem/Motivation

Translating an entity using IEF allow users to bypass field access set by other modules (like field_permissions).

Access override is in Drupal\inline_entity_form\Form\EntityInlineForm.php:

    if (!empty($entity_form['#translating'])) {
      // Hide the non-translatable fields.
      foreach ($entity->getFieldDefinitions() as $field_name => $definition) {
        if (isset($entity_form[$field_name]) && $field_name != $langcode_key) {
          $entity_form[$field_name]['#access'] = $definition->isTranslatable();
        }
      }
    }

Steps to reproduce:

  1. Install drupal
  2. Enable all core multilingual modules (config_translation/config_translation/locale/language)
  3. Add a new language
  4. Create a new role (let's say redactor), and a new user (let's say John) having only the redactor role
  5. Install and enable inline_entity_form (1.0.0-rc6), field_permissions (1.0.0-rc2) and paragraphs (1.12.0)
  6. Create a new paragraph type
  7. Add two textfields to this paragraph type:
    • Field 1: set 'Field visibility and permissions' to custom permissions, and keep the default values
    • Field 2: keep all the default values
  8. Add a field to article node type referencing multiple paragraphs of the above paragraph type
  9. Under the form display tab, use the IEF - Complex for this paragraph field
  10. Make article & paragraph type translatable (and make sure all of the fields are translatable)
  11. Give the redactor role the following permissions:
    • create translations
    • create/update/translate article node type
    • see the administration theme
    • Use the toolbar
    • Access the Content overview page
    • View published content
  12. Log in as John, create and publish an article having a paragraph. You should only see field 2
  13. Translate it into another language, and edit the paragraph. You can now see both fields, even if you don't have permissions to edit field 1

Proposed resolution

The attached patch fixes the bypass.

CommentFileSizeAuthor
#8 3144002-8.patch5.51 KBgeek-merlin
fix_access_bypass.patch729 byteskbriand

Issue fork inline_entity_form-3144002

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

kbriand created an issue. See original summary.

kbriand’s picture

kbriand commented
Status: Active » Needs review
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Issue tags: +Security improvements, +Good issue summary

Thanks for bringing this up and the good debugging, code review and issue summary.

This goes into the right direction, but messing with #access has its traps (#access instanceof AccessResult).
I'm working on an abstraction currently.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Assigned: Unassigned » geek-merlin
Status: Needs review » Needs work
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Needs work » Active
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Version: 8.x-1.0-rc6 » 8.x-1.x-dev
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Active » Needs review
StatusFileSize
new 3144002-8.patch5.51 KB

Hmm, issue forks are broken currently, so add a patch for now.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

So let's summarize.
- We have this specific issue, where IEF overwrites FieldPermissions's #access=AccessResultForbidden.
- Adding to #access securely turns out to have a lot of gotchas and should live in a library (for a first writeup see https://git.drupalcode.org/project/rat/-/blob/1.0.x/README.md).
- I created RenderArrayTool (RAT) for this and added it to fix this issue and all other access assignments.
- The problem of adding to #access applies in principle to each and every #access assignment, and we should fix all of them to better be safe than sorry.

The patch does exactly that, NR.

Although RAT accessAlter is test covered, a test for the case of this issue would be fine. (It need not be via FieldPermissions, a simple hardcoded TestAccessAlter module would be OK.)

chris matthews’s picture

chris matthews commented
Issue tags: -

If anyone is interested our nonprofit would be happy to sponsor the time to help get this issue across the finish line so that Inline Entity Form can move to a stable 8.x-1.0 (or 2.0.0) release.

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Version: 8.x-1.x-dev » 2.0.x-dev
Status: Needs review » Needs work 

git apply 3144002-8.patch 
error: patch failed: composer.json:6
error: composer.json: patch does not apply
error: patch failed: src/Form/EntityInlineForm.php:15
error: src/Form/EntityInlineForm.php: patch does not apply
error: patch failed: src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php:16
error: src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php: patch does not apply

needs reroll

shivam_tiwari made their first commit to this issue’s fork.

shivam_tiwari’s picture

shivam_tiwari commented
Assigned: geek-merlin » shivam_tiwari

shivam_tiwari opened merge request !74

shivam_tiwari’s picture

shivam_tiwari commented
Assigned: shivam_tiwari » Unassigned
Status: Needs work » Needs review
podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Status: Needs review » Fixed

MR!74 is in
tnx

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Version: 2.0.x-dev » 2.0.0-beta8

Status: Fixed » Closed (fixed)

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

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Version: 2.0.0-beta8 » 8.x-1.x-dev
Status: Closed (fixed) » Needs review
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

TODO: Add issue to bump php to 7.4 as the dep needs this.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Version: 8.x-1.x-dev » 3.x-dev
Priority: Normal » Major
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Needs review » Reviewed & tested by the community

Thanks to all the unconsented test rabbits...

  • geek-merlin committed c43d6001 on 3.x 
    Issue #3144002 by shivam_tiwari, geek-merlin, kbriand, Chris Matthews:...
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Reviewed & tested by the community » Fixed

  • geek-merlin committed 510553df on 3.x 
    Fixup Issue #3144002 by shivam_tiwari, geek-merlin, kbriand, Chris...
agoradesign’s picture

agoradesign commented

You should add this dependency to the info file as well, otherwise it won't get automatically installed, when you enable IEF. imho we'd also need an update hook, checking if rat is already installed, otherwise enable it

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Fixed » Needs work

@agoradesign You are so right. And i was so wrong when i thought that having had that some months in that 2 branch would have been test enough. Reverting and rolling a release.

  • geek-merlin committed 3e8c0bdf on 3.x 
    Revert "Issue #3144002 by shivam_tiwari, geek-merlin, kbriand, Chris...

  • geek-merlin committed 2b22b345 on 3.x 
    Revert "Fixup Issue #3144002 by shivam_tiwari, geek-merlin, kbriand,...
agoradesign’s picture

agoradesign commented

PS: aaaaaaaaaaaaaaa I'm sorry, I've just read trough the related issue #3362832: Dependency on RenderArrayTool, and now know that the rat module is more a library than a real Drupal module, so it would technically work, without enabling the module, I guess. However, this would be a very unconventional approach for a Drupal module dependency.

I have done manual code review before updating to rc18, instead of just tryin' - and that new dependency, that wasn't even declared in info file, nor installed in update hook, simply refrained me from even trying to update.

So my conclusion: rc18 wasn't that "broken" - or better BC-breaking - as I thought, but I don't really like the approach of adding a Drupal module dependency without needing to install it - so I highly appreciate the revert in rc19 :-)

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

@agoradesign Oh! Then this was a big misunderstanding, i assumed you checked.
Nevermind. Better safe than sorry. Yes RAT is a library, and libraries CAN be hosted on d.o.
And i can imagine you may even like the library.

agoradesign’s picture

agoradesign commented

yes, it actually was so :( but you're right - better safe than sorry :)

I'll have a look for sure - if it will be re-added to IEF again, it'll be present in any of my projects anyway :D

  • geek-merlin committed f758cb02 on 3.x 
    Issue #3144002 by shivam_tiwari, geek-merlin, kbriand, Chris Matthews:...
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Needs work » Fixed

Back to fixed.

Status: Fixed » Closed (fixed)

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