Mark some Comment permissions as restricted

[BWPanda]

In working on https://github.com/backdrop/backdrop-issues/issues/5536, I noticed that while permissions like Administer content are restricted (they have the security warning), equivalent Comment permissions aren't.

IMO, the following Comment permissions should be restricted:

• Administer comments
• Administer comment settings - allows approving and deleting comments
• Skip comment approval - allows bypassing approval process
• Edit own comments - allows getting approval for a comment, but then changing it to say something else afterwards (also bypassing approval)

Thoughts?

[avpaderno]

I agree: Those permissions should not given to every user (generally speaking). The warning is worth adding it. Clearly, depending from the community the site is built for, it would be acceptable to give the Skip comment approval permission to any user with an account, but that is something the user building the site needs to evaluate. Those warning should not be takes as Never give this permission. but Think before giving this permission.

[argiepiano]

Thank you for the PR @kiamlaluno! I've left a couple of language suggestions in the PR.

[olafgrabienski]

Hey @kiamlaluno - thanks for your PR! In Backdrop we try to avoid the term "user" for people, or: we try to use the term only for user accounts. How to replace "user", depends on context. E.g., a phrase like the following:

Users could post spam or offensive comments that become immediately visible to every user.

... can be changed to something like:

People could post spam or offensive comments that become immediately visible to every visitor.

It would be great if you could look for "user" alternatives in your PR.

[avpaderno]

I didn't forget this issue: I am checking what phrase is used by Backdrop in these cases. immediately visible to every visitor seems correct to me, as the comment would probably be visible to anonymous users too (except in the case they don't have the permission to see comments). People could post spam or offensive comments sounds strange to me, as it seems to generally speak of people, not people who has an account on the site (who would be users).

[avpaderno]

Looking at the permission descriptions, user and users are used words.

Allows a user to configure patterns for automated URL aliases and bulk delete URL aliases.

Determines whether or not users are shown a notice when an automatic URL alias changes.

The first example says allows a user, but it should be probably be allows users, since it's not a permission a single user has, but all the users with a specific role.

[avpaderno]

I updated the warning sentences to make them similar to other warning sentences used by other modules. In this way, I avoided to use users.

[olafgrabienski]

Thanks for the update, @kiamlaluno . I like the phrases you found, e.g. Allows to edit already approved comments instead of Users could edit ....

Update: also tested in the sandbox, works for me

[klonos]

Thank you @kiamlaluno 🙏🏼 ...I've left some suggestions in the PR. Mainly changing "Allows to do..." to "Allows doing...", as it seems to be the correct way to use the verb "allow" in those cases (see https://english.stackexchange.com/questions/60271/grammatical-complements-for-allow/60285#60285). I hope I'm not misinterpreting things.

Can others (preferably those with English as their primary language) have a look at the PR and chime in?

[jenlampton]

The permission restrict access option is intended to be used for only the most dangerous permissions. Here's the description from the documentation:

This should be used for permissions that have inherent security risks across a variety of potential use cases (for example, the "administer filters" and "bypass node access" permissions provided by Backdrop core).

It should be added to Administer comments and Administer comment settings, but Skip comment approval and Edit own comments wouldn't qualify.

I do think we can also improve those permissions by being more specific in their titles and/or descriptions, however, and I like the recommendations provided. I'll review the PR tomorrow and see if I can help with the grammar.

[jenlampton]

I reviewed the PR and all the grammar looks great 👍

The description for Skip comment approval still needs work, and I put another suggestion on the PR:

Allows comments to be posted without approval, becoming immediately visible to everyone. Because spam and offensive comments are common, this permission should only be granted to those who are trusted, or on a site with additional spam protection measures in place.

This is probably way too long... what if we updated both the title and the description?

title: Skip comment approval: comments will be immediately visible to everyone. description: Because spam and offensive comments are common, this permission should only be granted to those who are trusted, or on a site with additional spam protection measures in place.

Hm, that probably still needs work.

[avpaderno]

I changed the code as suggested. I hope I understood correctly.

[klonos]

@kiamlaluno the PR sandbox is gone (they are automatically deleted after sometime - something like 45 or 60 days I think). Can you please close the PR, then wait 10sec or so, then reopen it? That should trigger the creation of a new sandbox.

Thanks.

[avpaderno]

@klonos It worked: The preview has been re-generated.

[avpaderno]

I synchronized the PR fork with the main repository. Now the coding standards tests fails, but it is because phpcs is not found. I will close and re-open the PR once the current tests are done.

[stpaultim]

WAS:

Because spam and offensive comments are common, this permission should only be granted to those who are trusted, or on a site with additional spam protection measures in place.

SUGGESTION:

To control spam, limit this permission to those who are trusted or be sure to enable additional spam protection.

[stpaultim]

Note, while testing this PR I found that none of the MORE links are working on the permissions page. This is not specific to this PR, it's true in Demo Sandboxes and my local dev site. I opened a new issue.

https://github.com/backdrop/backdrop-issues/issues/6053

[avpaderno]

I have updated the PR. The failing tests are the usual ones, not related with the changes introduced here.

[stpaultim]

I've marked this "WFM." `

I see that earlier langage about SPAM has been removed. I think it's good either way. I would mark this RTBC, but I think it's better that someone else look at the code first. Personally, I'm not sure what the new 'restrict access' => TRUE, statement is doing.

[herbdool]

LGTM

[avpaderno]

I have synchronized the PR with the 1.x branch. Tests are running right now. CSpell complains about the new words introduced with CKEditor 5.

[quicksketch]

Thanks @kiamlaluno for your continued effort to make this change happen. I merged https://github.com/backdrop/backdrop/pull/4163 into 1.x and 1.26.x.

https://github.com/backdrop/backdrop/commit/9a463a2c7a5602c8162315452b76246653db6227 by @kiamlaluno, @stpaultim, @jenlampton, @klonos, @herbdool, @argiepiano, @olafgrabienski, and @BWPanda.