Problem/Motivation

Now that there is a UI, some users should not be able to edit a YAML form's YAML source.

Proposed resolution

Provide a 'edit yamlform source' permission.

Remaining tasks

  • Block users from editing YAML form source
  • Block users from editing YAML form element custom settings.
  • Must provide an update hook

Development Notes

  • Branch Name: 2759531-edit-yamlform-source-permission
  • Commit message: Issue #2759531: Provide 'edit yamlform source' permission
CommentFileSizeAuthor
#17 provide_edit_yamlform-2759531-16.patch11.84 KBjrockowitz
#17 interdiff-2759531-9-16.txt10.17 KBjrockowitz
#14 interdiff-2759531-9-14.txt5.47 KBjrockowitz
#14 provide_edit_yamlform-2759531-14.patch7.07 KBjrockowitz
#11 interdiff-2759531-9-11.txt4.49 KBjrockowitz
#11 provide_edit_yamlform-2759531-11.patch5.85 KBjrockowitz
#9 provide_edit_yamlform_source_permission-2759531.patch4.33 KBscott_euser
#7 provide_edit_yamlform_source_permission-2759531.patch4.13 KBscott_euser
#4 provide_edit_yamlform_source_permission-2759531.patch1.62 KBscott_euser
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jrockowitz created an issue. See original summary.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer commented
Issue summary: View changes
JockeL’s picture

JockeL CreditAttribution: JockeL commented

Maybe this should also include: Block users from entering Default YAML form submission data? (In the form that is attached to an entity)

scott_euser’s picture

scott_euser CreditAttribution: scott_euser commented
Status: Active » Needs review
FileSize
provide_edit_yamlform_source_permission-2759531.patch1.62 KB

I've created a patch to add this functionality: "Block users from editing YAML form source" and "Must provide an update hook".

Without the ui module enabled, I think it doesn't need this permission but when you enable the UI, you have the new source route. I think it makes sense to then add permission to roles which currently have permission to edit the source so I've added an update to the install file for yamlform_ui. That way if someone updates the ui module, their existing permissions are effectively maintained.

Haven't patched before so please let me know if this isn't the best method or I've done something wrong :)

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer commented

@JockeL I think it is really important that all users can set default values for a YAML form. I think the bigger issue is that users need to be educated more about what is YAML and why Drupal 8 and this module is embracing it.

#2759591: What is YAML and why we are using it?

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer commented

@scott_euser Yout patch looks really good. I think you can kill two birds with one stone and also hide 'custom properties'.

In \Drupal\yamlform\YamlFormElementBase::buildConfigurationForm (LINE 654) you just need to add something like

$form['custom'] = [
  '#type' => 'details',
  '#title' => $this->t('Custom settings'),
  '#open' => $element ? TRUE : FALSE,
  '#access' => \Drupal::currentUser()->hasPermission('edit yamlform source'),
];

Can you also please add a test called something like \Drupal\yamlform_ui\Tests\YamlFormUiElementTest::testPermissions.

scott_euser’s picture

scott_euser CreditAttribution: scott_euser commented
FileSize
provide_edit_yamlform_source_permission-2759531.patch4.13 KB

Sounds good, I might not get to work on the test today as I need to take off shortly, but will try to get to it as soon as possible.

Would it be wise to make two separate permissions like this patch?

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz commented

For now I think it should be one permission. The goal is just to block certain users from editing YAML source.

Also the entity.yamform.source_form should check the permission and yamform.update access. We don't want the permission to automatically grant access to update a yamform. I am not sure if you can have both permissions in the route yaml or you will have to create a custom access callback.

scott_euser’s picture

scott_euser CreditAttribution: scott_euser commented
FileSize
provide_edit_yamlform_source_permission-2759531.patch4.33 KB

Sounds good. I've updated the patch.

I'm not 100% sure what you mean by your second paragraph. Access is only granted in the update if they already have the 'administer yamlform' access (since before the patch, they'd have access to the source, this then maintains that access).

scott_euser’s picture

scott_euser CreditAttribution: scott_euser commented

Ah and I also added a testPermissions as you suggested. Still learning D8 so I'm not sure if I need to add it somewhere else so the test gets automatically run or if adding the method there is enough.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz commented
FileSize
provide_edit_yamlform-2759531-11.patch5.85 KB
interdiff-2759531-9-11.txt4.49 KB

No problem, I am new at reviewing code. Your tests are running fine.

  • Move 'edit yamlform source' permission and update hooks back to YAML Form UI module.
  • Created \Drupal\yamlform_ui\Access\YamlFormUiAccess::checkYamlFormSourceAccess. See: https://www.drupal.org/node/2122195
  • Updated YamlFormUiElementTest
  • Used PHP array short syntax

I have also included an interdiff so that you can see my changes.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz commented

BTW Dreditor makes reviewing the interdiff much easier

Status: Needs review » Needs work

The last submitted patch, 11: provide_edit_yamlform-2759531-11.patch, failed testing.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz commented
FileSize
provide_edit_yamlform-2759531-14.patch7.07 KB
interdiff-2759531-9-14.txt5.47 KB

..and I wrote a broken patch.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: provide_edit_yamlform-2759531-14.patch, failed testing.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz commented
Status: Needs work » Needs review
FileSize
interdiff-2759531-9-16.txt10.17 KB
provide_edit_yamlform-2759531-16.patch11.84 KB

I weep sadly as my patch fails again...

scott_euser’s picture

scott_euser CreditAttribution: scott_euser commented
Status: Needs review » Reviewed & tested by the community

Ah now I see what you meant by the custom access. Yeah makes sense to check against whether they also have permission to update.

Also makes sense to have the hook install in both yamlform and yamlform_ui.

Thanks for the Dreditor tip, very useful!

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz commented
Status: Reviewed & tested by the community » Fixed

@scott_euser Done! Thanks for your help with this issue.

scott_euser’s picture

scott_euser CreditAttribution: scott_euser commented

Excellent, thank you! Happy to have this in place!

In a few days I should be able to try to take a stab at the UI for managing options

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz commented

@scott_euser. Let's talk before you start working on the options UI. There is a very specific approach I want to try doing so that the code is reusable for the states ui

scott_euser’s picture

scott_euser CreditAttribution: scott_euser commented

Sounds good, let's move any notes on it to here: https://www.drupal.org/node/2759527

Status: Fixed » Closed (fixed)

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