[UX] Allow limiting allowed text formats per field instance (per content type).

[klonos]

This came up in a Gitter discussion between @laryn and @klonos and it is basically getting https://www.drupal.org/project/better_formats in core. There is a respective D8 core issue: https://www.drupal.org/node/784672 (from where https://www.drupal.org/project/allowed_formats was born). From the issue summary in d.org (some emphasis mine):

Problem/Motivation

Currently text fields can either be restricted to plain text, or the user may select between all accessible text formats independently of the context. This means that a privileged user who needs access to a permissive text formats (for example, to put tables or embedded remote content in basic pages) will get access to that format on every formatted text field (for example on a comment field).

There are three problems with this approach, and most experience Drupal developers have faced at least one of those in the past:

• Consistency At the moment you have to count on competence, good will and diligence of privileged users not to put certain markup in certain places. It would be convenient if a text field could be forced to use a specific text format (other than plain text). For example, you may want to make sure that comments only allow a very limited set of HTML tags ("filtertered HTML" for example) independently of the user's role, even if the same user has access to less restrictive formats in other places.
• Usability The ability to select text formats is a common source of confusion. By limiting the available text formats we also remove confusing user interface elements.
• Security If a privileged user account is taken over (for example, through social engineering), the attack surface is large due to the fact that every single text field can be used for XSS injections. By limiting where a dangerous text format can be used, we restrict the possibilities to inject malicious content.

Proposed resolution

Add an optional setting to the text field types that lets the site-builder determine if the text formats should be restricted. This setting is then used in the default textfield and textarea widgets to remove any non-allowed text formats. If nothing is set, the current behavior is unchanged.

Note that as it uses the underlying '#allowed_formats' form API property, the settings can't be used to give access to text formats that the user wouldn't have access to otherwise.

...

User interface changes

Checkboxes on list of available formats on text field configuration. Reduced set of allowed formats on content edit forms, where used. No fields use the new setting by default, so the patch doesn't affect the user interface for those who don't do anything with this functionality.

API changes

None

Data model changes

One setting is added to the field settings. The structure of the field data is unchanged.

---

PR: https://github.com/backdrop/backdrop/pull/2894

[klonos]

This is how better_formats goes about it:

[laryn]

@klonos Thanks for typing this up!

The difference between restricting formats and changing the default order I suppose is that the former enforces availability (you can only use the following) and the latter suggests a format (by changing the order you give a default a format to use, but that can be changed if desired).

There was also a checkbox option that appeared under Configuration > Content authoring > Text editors and formats > Settings which might actually be intended to do what I initially was hoping Backdrop did out of the box:

However, I have to test further as I think this doesn't seem to be working correctly. (The "Number of values" doesn't seem to save correctly with this enabled, and then the field doesn't display on the Node creation/edit form).

[klonos]

...and here's how allowed_formats works (D8 only):

The reason I have the drop-down open in the screenshot is to show that despite restricting the allowed formats, the "default value" field still shows all available in the system. I guess a glitch which is expected as the module is being ironed out.

[klonos]

@laryn ...yeah. that makes sense about setting the default order.

Main point is that I filed this issue as an annoyance/usability issue, but it turns out it also has security implications.

[laryn]

@klonos I'm not sure it has security implications (at least I don't think it should). Is it actually giving access to these formats or just changing order/availability based on existing access level?

[klonos]

We might be able to pull it off with a combination of sortable table rows + checkboxes. Mockup:

Too much?

[klonos]

@laryn from the d.org issue (emphasis mine):

If a privileged user account is taken over (for example, through social engineering), the attack surface is large due to the fact that every single text field can be used for XSS injections. By limiting where a dangerous text format can be used, we restrict the possibilities to inject malicious content.

[klonos]

...in fact, (although I haven't thought this through very carefully) I think that we can get rid of the initial plain text vs filtered text radio set if we had it so that the plain text option in the sortable table is always checked and disabled in the list, acting as a fall-back. If other text formats are selected, the plain text format checkbox could become available again so that it can be deselected (if the site admin/builder) does not want it as an option in the specific field.

[klonos]

Something like this perhaps:

[ghost]

Made a PR that seems to work with limited testing, however it didn't work until I included my PR from https://github.com/backdrop/backdrop-issues/issues/4076 as well (so this PR includes those changes too). Not sure if that's the best way to wite a PR that depends on another, but can re-configure if not.

[klonos]

Good job @BWPanda ...and nice catch on calling filter_fallback_format() instead of pulling from config 👍

I have not tested this extensively either, but it implements the very basics of this request as discussed earlier. For now, I have the following comments:

• I don't like how the setting for the text area rows sits between "Text processing" and "Allowed text formats":

  

  ...especially since the later is set to be shown/hidden via #states, depending on the value of the former. Can we please move that directly after the "Required field" checkbox instead?

• I would like to see some indication as to which user roles have access to each format. This would help site builders decide which formats are (dis)allowed.

• I would like to have either a single link to /admin/config/content/formats in the help text, or individual links to the configuration page for each text format listed (so that site builders can change role permissions).

• With the current implementation, the fallback format is being removed from the checkbox list. What happens though when all checkboxes for all formats are unchecked? I think that the fallback format should be included in the list, but hidden via states. Then if all other checkboxes have been unticked, it would be shown and remain ticked + locked. This would (I assume - have not tested or thought it through thoroughly) remove the need for this bit: If an existing field uses a text format not selected here, authors' access will be denied. ...because the authors would always be able to enter plain text (or whatever the fallback format has been configured to allow).

[ghost]

I've updated the PR but leaving this as 'needs work' because I can't get the 'plain text' option to work properly. It keeps getting unset, despite being disabled. Not sure how to fix it, someone else is welcome to take over and fix if possible.

I tried fixing the weights/ordering and added a link to the text formats page to the description. I think that should also suffice for seeing what roles are assigned to what formats, rather than duplicating that on this screen too.

[klonos]

...I can't get the 'plain text' option to work properly. ... someone else is welcome to take over and fix if possible.

I might give it a crack 🙂

I think that should also suffice for seeing what roles are assigned to what formats, rather than duplicating that on this screen too.

This forces people to open another page to see info that could very easily be available where they currently are. The point is to allow site admins to make informed decisions re the configuration in this form, without having to go pogo-sticking around the admin UI for additional info. Think similarly to how we are embedding permissions in various forms (instead of providing a link to the permissions page).

[klonos]

...I can't get the 'plain text' option to work properly. It keeps getting unset, despite being disabled.

It's because of https://www.drupal.org/project/drupal/issues/2911473 / #4083 @BWPanda 😉 ...working on a crossport of the most recent 8.x patch (got it to work on my local).

[quicksketch]

I'd like to advocate for this issue once I complete https://github.com/backdrop/backdrop-issues/issues/5748.

[quicksketch]

I took a look at this PR and it works great!

I've updated the PR but leaving this as 'needs work' because I can't get the 'plain text' option to work properly. It keeps getting unset, despite being disabled. Not sure how to fix it, someone else is welcome to take over and fix if possible.

I solved this issue. #disabled checkboxes don't submit a value, but you can force them by setting #value => TRUE.

I think the only thing really missing here are tests. I'll work on them.

[quicksketch]

I filed a new PR at https://github.com/backdrop/backdrop/pull/4423 that is ready for testing. Still working on tests.

[klonos]

FTR: This feature also made into D10.1.x last November. Change record: Text fields can enforce a specific text format

In Drupal, there is FAPI support for this. For example:

return [
  ...
  'field_type' => 'text',
  'settings' => [
    'allowed_formats' => [],
  ],
  ...
];

[klonos]

@quicksketch thanks to @argiepiano we've gotten to the bottom of the regression with #indentation in #6090, which means that we should be able to use that here as well 😉

I've also left a comment in the PR about the weight and the order of the new checkboxes relative to existing form elements.

[quicksketch]

Thanks @klonos, I plan to imitate the tests from the Drupal issue. I reworked my PR a bit because I found that previously that a list of allowed formats was required. For backwards-compatibility and to match expected functionality, if no formats are specified (i.e. #allowed_formats = array() or NULL) then all formats should be shown (still limited by permissions of course).

This new approach should also get a lot more of our current tests passing.

[quicksketch]

I ported the tests from the Drupal issue. They're not quite as comprehensive as I would have hoped but it covers the basic concepts of showing/hiding text formats based on the field settings. Our logic for showing/hiding matches Drupal exactly. Other than (substantial) syntax changes, the test coverage is nearly identical.

All tests are passing, including the new ones. An update hook could be added to update existing field definitions, but since an empty value maintains the current behavior, it doesn't have any effect on current sites.

[stpaultim]

Checking to see if this is something I can test.

[quicksketch]

Testing instructions:

• Log in as admin to the sandbox
• Visit admin/structure/types/manage/page/fields/body
• Disable a text format, such as "Raw HTML".
• Visit node/add/page
• Confirm the selected text format has been removed.
• Return to admin/structure/types/manage/page/fields/body, disable all but one text format (other than Plain text)
• Visit node/add/page
• No select list should be shown at all, since there is now only one text format.
• Return to admin/structure/types/manage/page/fields/body, enable all text formats.
• Check the JSON in field.instance.node.page.body.json, confirm that the "allowed_formats" key is an empty array (enabling all text formats saves internally as no text formats).

You can also try creating additional text formats, assigning them to the Editor role, and various combinations of allowed formats on the fields combined with the user-level permissions.

[stpaultim]

I'm a bit surprised that this was not already possible. I've done a round of testing following @quicksketch's suggestions and a few more things, everything seems to work for me.

Also, usability seems fine. It's not too hard to understand the new options.

Marked as "Works for Me". But, someone else will need to review code.

[laryn]

I looked over the code and I’m comfortable giving thumbs up.

[quicksketch]

Thanks @laryn, @stpaultim, and @klonos for reviewing! I have merged https://github.com/backdrop/backdrop/pull/4423 into 1.x for 1.25.0! Big thanks to @BWPanda for making the original PR!