Create View All Content of this type permission

stpaultim commented 2 years ago

Description of the need

Currently #24 on the new feature priority poll - https://forum.backdropcms.org/feature-requests (July 3, 2024)

I have never understood why I can set permissions per content type for everything, but access.

I frequently want to set up an internal content type that is only for site administrators or editors, but never available to unauthenticated users. It always surprises me that I can't do this in core and need an additional module. I believe that there are multiple modules for Drupal 7 to do this. In Backdrop land we have several of them available. I think that the two most basic use cases would be:

Members only content. In this case, special groups are not necessary. Members login to the site, non-members don't. I'd like to make some content available to members that is not available to the public.
Admin User Guide - I like to enable the book module and create a site user guide that is only accessible to site admins (on sites where admins are the only ones that login). But, in order to make the content only accessible to admins, I have to add another module.

I'm not sure we need all of the functionality of content access in core. But, I'd like the ability to create content types that are accessible by role (using permissions).

Is there a reason why this is difficult in core?

Here is something @quicksketch said in the past when this came up:

You've probably noticed that there are per-type controls on creating/editing/deleting content, but no "view" permissions at all other than "access content". This is because there are so many ways of controlling access to content: by type, by taxonomy terms, by "groups" (as provided by OG) and many other ways. Core does provide a central mechanism for controlling access, through a combination of hook_node_access() and hook_node_grants(). The first is for viewing an individual node, the second when doing queries of multiple nodes that need to be shown in a list.

https://github.com/backdrop/backdrop-issues/issues/1348#issuecomment-156044298

Proposed solution

Alternatives that have been considered

https://backdropcms.org/project/simple_access (48 installs) https://backdropcms.org/project/content_access (95 installs) https://backdropcms.org/project/content_view_access (40 installs) https://backdropcms.org/project/simple_access_grants (28 installs)

Installs as of May 24, 2022

Additional information

As of May 2024 the "Content Acess" module is number 69 (with 185 installations) on the usage list. "Content View Access" is number 219 (with 61 installations). The Simple Access module is number 244 (with 54 installations).

That's a total of 300 installations for basic content access modules. If all of this modules were combined and used on 300 sites, it would be about the 35th most popular module.

I plan to vote for this in the new feature survey, once it shows up on that site. https://forum.backdropcms.org/feature-requests

You can vote for this feature here: https://forum.backdropcms.org/features/create-view-all-content-type-permission

Draft of feature description for Press Release (1 paragraph at most)

Backdrop CMS now includes the ability to restrict access to content of any given type by role. This makes it easy to create an members only or admin only content type that used to be only possible with contrib modules.

argiepiano commented 2 years ago

@stpaultim, thanks for bringing up this issue. I would add Simple Access Grants to the list of contrib modules that do something like this already.

For what it's worth, I have worked mostly with Content Access and have always found it more than adequate to do the things you mention, and it's simple to use.

stpaultim commented 2 years ago

Someone should write a blog post comparing the available access modules for Backdrop CMS.

ghost commented 2 years ago

I think 'Content View Access' is the simplest way to do what this issue is requesting: add a new 'View' permission to content types. I'd suggest that one go into core to add this missing feature.

The other ones all seem to do extra things, which IMO are better suited to contrib.

yorkshire-pudding commented 2 years ago

I agree that Content View Access would be the best fit with the existing content type permissions. Each pair ('View own' and 'View any') should be within the Node group and either be above Create or between Create and Edit, if the aim is to fit into the CRUD pattern.

quicksketch commented 2 years ago

Two possibly good reasons for not including this in core:

Content access checks add measurable and sometimes substantial performance impact because every node query on the site now requires additional JOINs to check user access.
Combining access control modules is usually not a good idea, since they can conflict and grant (or restrict) access in unexpected ways. If included in core but not sufficient for a site's needs, site owners may install the core module first and then follow by installing a contrib solution too. But this might be able to be mitigated with a status report message. That might be a good idea anyway for any site that has multiple access control modules.

stpaultim commented 2 years ago

@quicksketch - Just repeating your first concern back to make sure I understand it. This would be a concern that running an access check everytime we display a node could cause a significant perforance hit. I assume that this check is different from the existing checks, in that most of them only happen when editing a node, not when simply viewing a node.

So it theory, this may have not been included in core, because it's performance implications are much greater than the other checks. This had not occurred to me, but seems like a good thing to be concerned about. I assume it would be possible to test this (or is that not really necessary)?

One of my primary arguments in favor of this feature is that I think most users already assume that this is possible in core, until they try to do it. Because, it's easy to believe that with 5 existing content type permissions - this would be one of them. I know it's something I've looked for and used on quite a few occasions.

Addressing this most obvious access need, might significantly reduce the need to enable more complicated content access modules. Could this permission be disabled by default to avoid the performance hit if it's not needed?

yorkshire-pudding commented 2 years ago

Thanks @quicksketch - that is a really helpful insight and will make me think twice about using a content access module for side use cases.

olafgrabienski commented 2 years ago

Content access checks add measurable and sometimes substantial performance impact because every node query on the site now requires additional JOINs to check user access.

Is this not generally the case in Backdrop? I'm thinking e.g. at the 'Hidden path' option which also restricts access to content. Is the related check treated differently, performance-wise?

quicksketch commented 2 years ago

Before I explain too much further, I want to say that we should base decisions on measuring the performance, not just on the warning that extra joins will likely cause performance loss. And even so, we may be able to find a way to mitigate the performance loss unless the feature is used. But right now, the node access system is toggled based on whether any module implements hook_node_grants(). If one or more modules implements that hook, then extra joins are added to every node query.

I'm thinking e.g. at the 'Hidden path' option which also restricts access to content.

The hidden path option does not actually use the node access system, because nodes created with hidden paths are actually still fully accessible by all users; they just don't have pages. It's expected that hidden path nodes will be placed in blocks or listings of some kind. The full node access system makes it so if you have a listing of nodes (say latest 20 nodes), different users will see a different 20 nodes based on their access, which means Views joins over to the node_access table and whatever module access table you have installed, so usually 2 extra joins for every listing of nodes on the site. I believe node access also disables any query or render caching set for that view.

herbdool commented 2 years ago

In theory we could replicate the approach of 'Hidden path'. It would not impact performance but it would mean ensuring that it also applied to search results, Views, Layout, much like we've done for 'Hidden path'. Any other contrib module wouldn't know about it though, unless they explicitly added a check for the user permission. For example, Search API would need to add it. It may also confuse someone if they've got this feature enabled, plus some contrib node access modules. But as @quicksketch noted, that's already the case and there should be a status message to warn admins.

avpaderno commented 2 years ago

Since this feature request is just asking to add a new permission per content type, it should not impact much the performance. Node::access() already checks which permissions the user has. The new permission would just require to change the following code.

// Check if user can view any unpublished nodes.
if ($op == 'view' && !$this->status && user_access('view any unpublished content', $account) && $account->uid != 0) {
  $rights[$account->uid][$cid][$op] = TRUE;
  return $rights[$account->uid][$cid][$op];
}

// Check if authors can view their own unpublished nodes.
if ($op == 'view' && !$this->status && user_access('view own unpublished content', $account) && $account->uid == $this->uid && $account->uid != 0) {
  $rights[$account->uid][$cid][$op] = TRUE;
  return $rights[$account->uid][$cid][$op];
}

With three lines more, Node::access() would be able to handle the permission to view nodes of a specific content type.

// Check if user can view this content type.
if ($op == 'view' && $this->status  && user_access('view any ' . $this->bundle() . ' content', $account) && $account->uid != 0) {
  $rights[$account->uid][$cid][$op] = TRUE;
  return $rights[$account->uid][$cid][$op];
}

// Check if user can view any unpublished nodes.
if ($op == 'view' && !$this->status && user_access('view any unpublished content', $account) && $account->uid != 0) {
  $rights[$account->uid][$cid][$op] = TRUE;
  return $rights[$account->uid][$cid][$op];
}

// Check if authors can view their own unpublished nodes.
if ($op == 'view' && !$this->status && user_access('view own unpublished content', $account) && $account->uid == $this->uid && $account->uid != 0) {
  $rights[$account->uid][$cid][$op] = TRUE;
  return $rights[$account->uid][$cid][$op];
}

avpaderno commented 2 years ago

I think that adding the permission to view nodes of specific content types would simplify existing core code. Eventually, the performance should be increased.

stpaultim commented 7 months ago

Looking back at this issue, because once again I am adding the Content View Access module because I want a content type that is only accessible to authenticated users.

Combining access control modules is usually not a good idea, since they can conflict and grant (or restrict) access in unexpected ways.

I think an argument could be made that including this pretty basic feature in core, would reduce the need for adding contact access modules that might conflict. Assuming, that we could add this to core in a safe and performant manner. I can't speak to the technical problems of adding this feature, but I know I would use it.

My use case is using the book module in core to provide a custom "User Guide" for the site. I'm trying to do this for all my clients now. This means adding the Content View Acess module to almost every site, so that I can limit access to Book nodes to only editors and admins.

In fact, I've made the User Guide recipe dependent upon the Content View Access module. https://backdropcms.org/project/user_guide_recipe

stpaultim commented 7 months ago

I added this to the original post:

As of May 2024 the "Content Acess" module is number 69 (with 185 installations) on the usage list. "Content View Access" is number 219 (with 61 installations). The Simple Access module is number 244 (with 54 installations).

That's a total of 300 installations for basic content access modules. If all of this modules were combined and used on 300 sites, it would be about the 35th most popular module.

yorkshire-pudding commented 7 months ago

I believe, that there is minimal impact on performance if you stay away from 'View own' permissions. Doing it at the content type level is quite easy and I have a custom module, but I can't remember which site I put it on now (on phone), but it works. Very similar to the code shared earlier in this issue.

jenlampton commented 6 months ago

I think you're misunderstanding the complexity of access control. If this problem were easy to solve securely it would have been in Drupal core decades ago.

Yes, it's possible to hack your own site in any number of ways (custom modules, contrib modules, etc) to get your problem solved - and that's by design - we want people to be able to do whatever they need using the APIs. But when we put something in core it needs to be secure. We can't just add a permission. We need to make sure that when people use that permission to hide thing X, that every single way we display thing X in core, that permission does what people expect, not leaking potentially sensitive information to the public. Using Node::access() alone is not sufficient. There are views by fields, node-reference fields, etc, etc.

If your own personal site isn't using any of those things you might be able to keep it secure, but that doesn't mean it's safe to include in core. The current access control system in core is safe in this way, and that's why it has significant performance implications.

yorkshire-pudding commented 6 months ago

@jenlampton good points. Where I use that custom module doesn't have those other cases (I control it so it doesn't), but you're right that it wouldn't protect if the information was exposed in one of the many other ways possible.

herbdool commented 6 months ago

I'm a bit agnostic on whether to put one in core, but if so, then I also think https://backdropcms.org/project/content_view_access is the simplest. It uses node grants so it works with entity reference and Views, etc. But because of the performance impact it should stay its own module and not enabled by default. And there should be a warning on the status page if other access control modules are also enabled.

stpaultim commented 5 months ago

In reviewing the discussion here: https://github.com/backdrop/backdrop-issues/issues/1929, something just occurred to me.

If my goal is make access to a specific content type only available to a specific role, I already have the tools in core to do this (I think).

If I make a layout for a specific content type, I can give that layout a visible condition that would have the same effect and changing the permissions. At least I think so. Am I missing anything.

One of my use cases was to enable the book module to create a user guide for site editors. I only want these pages to be accessible to editors and admins. It now occurs to me, that while I cannot set an access permission on the content type, I can create a special layout for book nodes and then create a visibility rule for that layout that restricts access to admins and editors.

I guess the downside for this method is would be that these nodes might still accidentally be made visible in a view or some other display. But, I don't think that is a serious problem in my specific use case.

Am I wrong, in that one could use visibility rules as a make-shift permission for access to a specific content type (if one is not worried about exposure through views or such)?

Atten: @argiepiano or @yorkshire-pudding?

yorkshire-pudding commented 5 months ago

In reviewing the discussion here: #1929, something just occurred to me.

If my goal is make access to a specific content type only available to a specific role, I already have the tools in core to do this (I think).

If I make a layout for a specific content type, I can give that layout a visible condition that would have the same effect and changing the permissions. At least I think so. Am I missing anything.

One of my use cases was to enable the book module to create a user guide for site editors. I only want these pages to be accessible to editors and admins. It now occurs to me, that while I cannot set an access permission on the content type, I can create a special layout for book nodes and then create a visibility rule for that layout that restricts access to admins and editors.

I guess the downside for this method is would be that these nodes might still accidentally be made visible in a view or some other display. But, I don't think that is a serious problem in my specific use case.

Am I wrong, in that one could use visibility rules as a make-shift permission for access to a specific content type (if one is not worried about exposure through views or such)?

Atten: @argiepiano or @yorkshire-pudding?

@stpaultim - wouldn't they just use the Default Layout if they didn't have permission for the special "User Guide" layout?

stpaultim commented 5 months ago

@yorkshire-pudding - Darn, you might be right. I suppose I need to test this. But, not today.

That's why I asked.

argiepiano commented 5 months ago

@yorkshire-pudding is right (and I may have misled someone in the forum some time ago! ha!). The Default Layout would be used if someone doesn't have the role to use another node overridden layout. The Default Layout cannot take conditions.

However, a little hack: In the Default Layout, you could put a condition in the Main Page Content block, for both node type and role, to hide that block, and add a Custom block that's shown in those cases that says "you are trying to access restricted content" or something like that.

klonos commented 5 months ago

I re-read this entire thread, and this is my take:

This seems like it would be a really nice feature to have, and we know that it would simplify things that people are trying to achieve, without leaving them resorting to any "hacks" or workarounds that might turn to bite them in the long run.
I see that there are concerns about performance. We should have a PR, so that there is a specific implementation, and then do some profiling to see what sorts of performance loss there actually is (if any, and if measurable/considerable). That would take one factor off the list and allow us to make a better-informed decision. Without it, it all sounds like assumptions to me.
I see that there is also a concern about security. That falls into the implementation I believe; we need to ensure that the core solution works with direct node access, with views, with blocks etc., and then make sure that it is done in a way that allows contrib to hook onto it (which does not necessarily guarantee that there will not be any changes required on contrib).

my 2c

klonos commented 5 months ago

Incidentally, DDEV threw this tip today:

Want to profile your code in an improved more beautiful manner with Xhprof? Try out @tyler36's https://github.com/ddev/ddev-xhgui

backdrop / backdrop-issues