Skip to content
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

[UX] Add the 'Administer content' permission to the editor role on new installs #4541

Closed
klonos opened this issue Aug 14, 2020 · 24 comments · Fixed by backdrop/backdrop#3248
Closed

Comments

@klonos
Copy link
Member

klonos commented Aug 14, 2020

On a vanilla installation of Backdrop, if you create a new user and assign them the "Editor" role, this is what they see when trying to create content:

image

This means that the "Editor" role cannot schedule content for publishing, and more importantly, they cannot save drafts. "Locking" people to this state where they can only either make content live immediately, or keep browser tabs open in order to not lose their job is bad UX.

There's the "Administer content" permission (machine name administer nodes), which if you grant it to them allows them access to the "Publishing options" vertical tab, but also the "Authoring information" vtab:

image

I believe that it is expected for content editors to be able to publish, save as draft, and schedule content for publishing. On the other hand, being able to change the authoring date and author information is not something that the average site would allow.

So I would like to propose that we introduce a new permission (something like "Access content publishing and scheduling options"), and that we grant that to the "Editor" role OOTB.

@klonos
Copy link
Member Author

klonos commented Aug 14, 2020

...I have done a search across our codebase for 'administer nodes', to determine how/where this permission is used, because we have a separate "Access the content overview page" permission for allowing people to access the content list/view (and this permission is granted to the "Editor" role OOTB).

TL;DR: it seems that the administer nodes permission is a super-set of other "sub-permissions", so I think that it would be useful to introduce a new sub-permission, to specifically/explicitly allow access to the publish/draft/schedule actions.


Other than .test files, the 'administer nodes' string appears:

  • 2 times in an inline comment in user.module
  • in block.overview_content.inc, to determine whether the "Manage content" link is shown in the "Content overview" block on the Dashboard. I think that we should be using the "Access the content overview page" permission here instead. If people also have the administer nodes permission, they'll be able to edit content; otherwise, they'll just be able to see the content list.
  • in book.admin.inc, to determine access to the "edit" and "delete" links of books.
  • in book.module, in book_node_presave(), to determine if a new revision should be saved.
  • 6 times in node.pages.inc:
    • in node_form():
      • to determine access to the "Revision information" tab, and the "Create new revision" checkbox.
      • to determine access to the "Authoring information" tab
      • to determine access to the "Publishing options" tab
    • 2 times in node_revision_overview(), as an alternative/additional way to determine access to revert and delete revisions (we already have the 'revert revisions' and 'delete revisions' permissions, but this allows people to explicitly manage revisions of their own content)
  • 3 times in node.module:
    • in node_permission(), to declare the permission
    • 2 times in _node_revision_access()
  • in views_handler_field_node_revision_link_revert.inc, as a superset/alternative of the 'revert revisions' permission
  • in views_handler_field_node_revision_link_delete.inc, as a superset/alternative of the 'delete revisions' permission
  • in views_handler_field_node_revision_link.inc, as a superset/alternative of the 'view revisions' permission
  • in views_handler_filter_node_access.inc
  • in node.views.inc, in node_views_query_substitutions()

@klonos
Copy link
Member Author

klonos commented Aug 14, 2020

So we could do this the "proper" way, by introducing a new permission, or we could just go down the easy path, and simply grant the "Administer content" permission to the "Editor" role (knowing that this would allow content editors access to authoring information, and revision settings if enabled for the content type).

Thoughts?

@stpaultim
Copy link
Member

I've added that permission to the editor role (test site) and browsed around to see what I can do. Seems like the effects are pretty much limited to "publishing options," "authoring information," and "revisions."

I think it would be perfectly safe to give all these permissions to the editor role.

My gut reaction is that creating a new permission for the ability to schedule content publishing probably isn't necessary. But, I could be convinced otherwise.

@klonos
Copy link
Member Author

klonos commented Aug 15, 2020

I think it would be perfectly safe to give all these permissions to the editor role. ...creating a new permission for the ability to schedule content publishing probably isn't necessary.

Yup, as I said: "...or we could just go down the easy path" 😄

@klonos
Copy link
Member Author

klonos commented Aug 15, 2020

Seems like the effects are pretty much limited to "publishing options," "authoring information," and "revisions."

Have you tried editing content that another user has created?

@stpaultim
Copy link
Member

Have you tried editing content that another user has created?

Yes, it seems to work fine. I went ahead and created a PR that just adds the "administer nodes" permission to the editor role, so that folks can try it out.

Login to the sandbox for this PR with:

UN = Test Editor
PW = bd8723

This user has the editor role and you can see what they are able to do.

@klonos
Copy link
Member Author

klonos commented Aug 15, 2020

Yes, it seems to work fine.

No it doesn't 😅 ...

  1. create user1 -> assign editor role
  2. create user2 -> also assign editor role
  3. log in as user1 -> create a page -> log out
  4. log in as user2 -> create a page
  5. try to edit user1's page -> you can do that, you shouldn't be able to 👎 -> log out
  6. log in as user1 again -> edit user2's page -> you can do that, you shouldn't be able to 👎

That's why the "Administer content" permission has this description/note:

Warning: Give to trusted roles only; this permission has security implications.

@klonos
Copy link
Member Author

klonos commented Aug 15, 2020

...we need a permission that allows users to access publishing/scheduling options for their own content. "Administer content" allows them to access and edit ALL content.

@stpaultim
Copy link
Member

stpaultim commented Aug 15, 2020

try to edit user1's page -> you can do that, you shouldn't be able to 👎 -> log out

Editors have permission to edit other peoples content by default. We already have a permission for that and we currently give it to editors by default.

In my view its a reasonable assumption that most site editors should be able to edit anyone's content. That is what the role is, editor. I view an editor as the person responsible for maintaining content on the site. I expect that a regular authenticated user can edit their own content, but not edit the content of others. An editor, in my view, has permission to edit anyones content.

Clearly, some sites may choose to assign these permissions differently. We could remove the permission to edit other peoples content, but our current position is that they can by default, which makes sense to me.

If we don't want editors to edit other peoples content, we shouldn't give them that permission.

...we need a permission that allows users to access publishing/scheduling options for their own content.

This might make sense, but that would be a different issue. This issue was about editors, not regular users. The ability to give end users the ability to schedule posts would be reason for creating a new permission. But, I don't think that a new permission is necessary to give the "editor" role the ability to schedule posts.

@klonos
Copy link
Member Author

klonos commented Aug 15, 2020

Editors have permission to edit other peoples content by default. We already have a permission for that and we currently give it to editors by default.

Hmm 🤔 ...you are absolutely right @stpaultim. Sorry about that then. I guess that too much work on GovCMS and how government agencies have established the defaults for their workflows has affected how my brain is wired (they have "Content Editor" and "Content Approver" roles OOTB). Please ignore me.

PS: also, if I had checked that to begin with, I would have saved myself so much time 😅

@ghost
Copy link

ghost commented Aug 15, 2020

I'd be OK with giving the Editor role the Administer Content permission OOTB.

@olafgrabienski
Copy link

I'd be OK with giving the Editor role the Administer Content permission OOTB.

Understandable in the current situation, as long as Backdrop core is missing fine-grained content editing permissions (see discussion in #815, contrib module: Override Node Options). Administer Content is however very permissive. I guess we provide it only on new sites as default, right?

For reference: Revisit permissions for the OOTB "Editor" role

@klonos
Copy link
Member Author

klonos commented Aug 15, 2020

Administer Content is however very permissive.

Yup. That was my main concern; that's why I proposed a new permission, specifically for publishing/unpublishing/drafting/scheduling.

I guess we provide it only on new sites as default, right?

Correct.

@stpaultim
Copy link
Member

stpaultim commented Aug 15, 2020

I guess we provide it only on new sites as default, right?

Yes, all permissions for the editor role are assigned by the standard install profile, so they will only effect new sites. We can tweak these settings over time without effecting existing sites.

Again, I created an editor account in the PR sandbox for anyone that wants to easily test exactly what an editor can do after this PR:

UN = Test Editor
PW = bd8723

...we need a permission that allows users to access publishing/scheduling options for their own content. "Administer content" allows them to access and edit ALL content.

I don't know how I feel about this yet (if we need it in core), but it seems like a legit use case. Opening a separate issue to get feedback.
#4543

@olafgrabienski
Copy link

olafgrabienski commented Aug 17, 2020

For reference: Revisit permissions for the OOTB "Editor" role

Should we use that issue #4339 to collect all the existing and upcoming Editor permission issues?

@ghost
Copy link

ghost commented Aug 21, 2020

I know this issue is RTBC, but I'm hesitant to merge its PR as this issue was only opened less than a week ago and so far has only 4 participants. For such a significant change (Editors getting more permissions OOTB) I wonder if we need more feedback...?

@stpaultim
Copy link
Member

I disagree that it's really that significant. We're giving the site editor the ability to administer nodes. In my view, that's pretty much what the role of editor is. Before we added this role, the same user would probably have been given full admin privileges to administer everything. And, this only effects new sites. ;-)

HOWEVER, I do understand your concern and thinks it's totally reasonable to wait for a few more folks to chime in - in case I'm wrong. I'll work on rounding up a few more reviewers. It would be nice to hear from @quicksketch or @jenlampton if they have any concerns.

@ghost
Copy link

ghost commented Aug 21, 2020

In Zulip, @quicksketch said: "With OOTB issues like this, it can be a big change but easily reversible (though we would probably wait minor releases between changes). So +1 for go ahead with it."

@olafgrabienski
Copy link

Does anyone have an idea for a better issue title? The current one doesn't mention the "Administer content" permission and might make it hard to find this issue after a while.

Current title:

Allow access to content publishing/scheduling options to the "Editor" role

@ghost ghost changed the title [UX] Allow access to content publishing/scheduling options to the "Editor" role OOTB [UX] Give Editor role the 'Administer content' permission OOTB Aug 21, 2020
@ghost
Copy link

ghost commented Aug 21, 2020

@olafgrabienski How's that?

@olafgrabienski
Copy link

Looks good, thank you!

@herbdool
Copy link

I'm ok with this change

@ghost ghost removed the needs - more feedback label Aug 21, 2020
@ghost ghost added this to the 1.17.0 milestone Aug 21, 2020
@ghost
Copy link

ghost commented Aug 21, 2020

Thanks for the feedback all! I feel this is ok to go into the next minor release (it doesn't have an advocate, but its PR is ready to be committed).

@ghost
Copy link

ghost commented Aug 21, 2020

Thank you @klonos for the suggestion, and to @stpaultim for the PR! I've merged backdrop/backdrop#3248 into 1.x.

@klonos klonos removed their assignment Aug 21, 2020
@jenlampton jenlampton changed the title [UX] Give Editor role the 'Administer content' permission OOTB [UX] Add the 'Administer content' permission to the editor role on new installs Sep 15, 2020
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants