search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 823 forks source link

GridField pagination doesn't interact correctly with canView() permissions #5995

Open talbet opened 7 years ago

talbet commented 7 years ago

In most cases implementing a canView() function on a DataObject, a given user will be able to:

However, if a more complicated canView() function is implemented, where only a small set of DataObject's should be viewable by a user, a situation can arise where the the ORM selects a range of DataObjects for the paginated GridField, and then the canView() removes all of those DataObjects from the visible list.

The user expects to see a list of DataObjects that they have access to, but instead they get see a gridfield that reports the total number of DataObjects available, and hides everything they don't have access to across multiple pages. Most of the pages will report 'No items found' since none in that paginated set are viewable.

This is a difficult problem to solve, as GridField pagination relies on the ORM to request records quickly from the DB, and canView() filters the requested records in PHP. To solve this problem, the permissions have to be checked before the pagination. I've experimented with running filterByCallback() on the datalist, but then you end up filtering every record in PHP and making multiple request to the DB. This is not scalable and was a detrimental performance hit on a DataObject with 500 records (10sec execution time).

Implementing an Access Control List pattern in the database layer, may be the only way to solve this and maintain performance. Since that would be huge undertaking, I'm raising this as an issue in case anyone has any insights or ideas before going down that path.

See also: #3831

PRs

dhensby commented 7 years ago

I think the filtering in PHP needs to be a bit more clever than using filterByCallback because that will check every record when you only need to find the first [num per page] records.

Scalability is less (but still a bit) important in the CMS because only a very select number of users will use the CMS and it's not an operation that needs to scale under load. However, of course if you have thousands of records and no permission to view any of them, you're going to have a problem.

Auto-complete relation joining should also be taken into account.

talbet commented 7 years ago

This project will likely see most users only ever have access to around 10 records out of a thousand plus DataObjects, so filterByCallback is definitely not an option. I have gone down the path of adding extra filters to the ORM queries that fetch the DataLists for the GridField, but this is only possible because I have fields on my models that are used for this purpose (a many_many join to members that should be able to access this)

On the Setting up Permissions Docs there is mention of a way the abstract this out (possibly already setup in the framework), instead of manually implementing it with extensions and query alters, but unfortunately there isn't much more information or an example of how it works.

chillu commented 5 years ago

This has also come up in the context of assetadmin graphql: https://github.com/silverstripe/silverstripe-asset-admin/issues/828

mlewis-everley commented 5 years ago

If anyone finds this issue and is maybe not so worried about performance, I got around this issue by creating a custom GridField and performing the canView check first (before handing the list over to components for manipulation), then use Injector to swap out the new GridField:

https://gist.github.com/mlewis-everley/e3608e2e814ca55f50da4454a62d9bbf

As mentioned above, this method does have a performance implication but I needed it for managing < 10,000 records and so far I haven't noticed a major hit on performance (I will need to do more testing to be sure).