-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark some Comment permissions as restricted #5554
Comments
I agree: Those permissions should not given to every user (generally speaking). The warning is worth adding it. |
Thank you for the PR @kiamlaluno! I've left a couple of language suggestions in the PR. |
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:
... can be changed to something like:
It would be great if you could look for "user" alternatives in your PR. |
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). |
Looking at the permission descriptions, user and users are used words.
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. |
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. |
Thanks for the update, @kiamlaluno . I like the phrases you found, e.g. Update: also tested in the sandbox, works for me |
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? |
The permission
It should be added to 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. |
I reviewed the PR and all the grammar looks great 👍 The description for
This is probably way too long... what if we updated both the title and the description? title: Hm, that probably still needs work. |
I changed the code as suggested. I hope I understood correctly. |
I have updated the PR. The failing tests are the usual ones, not related with the changes introduced here. |
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 |
LGTM |
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. |
Thanks @kiamlaluno for your continued effort to make this change happen. I merged backdrop/backdrop#4163 into 1.x and 1.26.x. backdrop/backdrop@9a463a2 by @kiamlaluno, @stpaultim, @jenlampton, @klonos, @herbdool, @argiepiano, @olafgrabienski, and @BWPanda. |
In working on #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 commentsSkip comment approval
- allows bypassing approval processEdit own comments
- allows getting approval for a comment, but then changing it to say something else afterwards (also bypassing approval)Thoughts?
The text was updated successfully, but these errors were encountered: