Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Issues like #2456691: User email field need to use Field-Entity-aware formatters in Views fixes potential security problems.
Let's ensure that we have access test coverage for all of them.
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff.txt | 943 bytes | dawehner |
#34 | 2462589-34.patch | 22.47 KB | dawehner |
#29 | interdiff.txt | 5.5 KB | dawehner |
#29 | 2462589-29.patch | 22.42 KB | dawehner |
#27 | interdiff.txt | 6.04 KB | dawehner |
Comments
Comment #1
dawehnerAlright, let's start with testing the node title. It turns out, ... everyone can access node titles all the time ... Well, its about people with access
to nodes, which is controlled by the listing already.
Comment #3
dawehnerSome work on it.
Tested the base implementation on node, user and entity_test. The helper class will allow it much easier to expand the test coverage on the other issues,
and ensure we don't mix up things again in the future.
Comment #4
jhodgdonLet's make this a child issue of #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality
Comment #5
jhodgdonCan we expand this test to one of the entities from #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality that still has a critical follow-up, to verify that the test would catch the problem?
Comment #6
dawehnerStarted adding some tests for file, aggregator_item, aggregator_feed and comment entity types.
This time, they will fail, as I have no motivated to run them, that late during the night.
Comment #8
dawehnerAlright, it has been late yesterday night.
Comment #10
alexpottSince we're not going to add test coverage on issue like #2456691: User email field need to use Field-Entity-aware formatters in Views this needs to be critical.
Comment #11
dawehnerSo we miss quite some of the access checking for file fields.
Comment #13
dawehnerReroll.
Comment #14
jhodgdon@dawehner -- I got dreditor, let's see how it went... Anyway this patch looks great! I just had a couple of ideas about API docs. Surprise, surprise. ;)
Also we need to:
a) Make sure all the base fields are in there for all the entities.
b) Make sure that on the "convert this to use a proper handler" issues, we uncomment the test lines, or else have a follow-up @todo with an issue to uncomment them all and make sure the tests still pass.
Should this be part of this patch? Seems like it belongs somewhere else?
Maybe these first-line class descriptions should be something more like:
Tests base field access in Views for the foo entity. ??
Can we change this to something like:
To use this method, set up an entity of type $entity_type_id, with field $field_name. Create an entity instance that contains content $field_content in that field. This method will check that a user with permission can see the content in a view, and a user without access permission on that field cannot.
Comment #15
jhodgdonOh, and didn't we have test coverage for access on a lot of these fields added when you did that "Convert the standard plugins to use 'field'" issue? Maybe we can remove that in favor of this test, which is clearer and comprehensive?
Comment #16
dawehnerYeah that belongs more into #2450153: Add a default_formatter to UUIDs fields honestly.
Not sure whether we really want to remove some other test coverage, but yeah these tests are really helpful to cover a wider range.
Comment #17
jhodgdonLooking good!
So I started looking through all the entities, from here:
https://api.drupal.org/api/drupal/8/search/basefielddefinitions
It doesn't look like this patch includes all of the base fields on all of the entities. Should it? For instance, I started at the top. The aggregator Feed entity has:
fid
uuid
title
langcode
url
refresh
checked
...
The patch has only title (commented out, as that field hasn't been converted yet), url, langcode.
Would it make sense in this test to use some sort of discovery method on the entity type to find out what the base fields are, and test all of them systematically? It seems like we want to aim for 100% test coverage...
Comment #18
dawehnerWell, I think a generic approach would be hard, given that they first need to setup proper data in the entity, which is hard,
and then you have to know how those values are gonna be rendered.
Just added a few more fields for user and node.
Comment #22
dawehnerJust a little bit more test coverage.
Comment #23
dawehnerA couple of more.
Comment #24
jhodgdonOK, this is looking good, aside from not covering all the fields, which we cannot do now since not all of them are fixed yet.
So, should we:
a) Wait until all the other critical children on the parent are done, and make this patch cover everything then?
b) Commit it now with a follow-up to have this cover everything when the other issues are done? [add lots of @todo statements to this patch, one per test class, and a new issue they can all point to]
c) Something else?
Comment #25
dawehnerI'd prefer to get this issue in first and then maybe, if needed expand the test coverage, because for the conversions it would be great to know whether things are actually fixed, so option b).
Comment #26
jhodgdonI agree. So I filed a followup; patch just needs @todo added to refer to it. [so, needs work]
#2464635: Follow-up: Enable additional tests for base entity field access checking in Views
Comment #27
dawehnerThanks for filing the follow up.
Comment #28
jhodgdonOops.
Is this from a different issue? I guess it's OK to have here if it's needed.
Nitpick: checks not check
Same here. Etc.
I think these are the wrong link? should be https://www.drupal.org/node/2464635 ?
Comment #29
dawehner#2429191: Deprecate entity_reference.module and move its functionality to core does the same, but yeah its required to show the node type label.
Comment #30
YesCT CreditAttribution: YesCT commentedis this still intended to be done in this issue, or do we need a follow-up to reference?
Comment #31
larowlanThis method is slick, love it
So given the goal is to get this in and then uncomment lines in the follow up after the conversions are done, I think this is ready.
Boldly setting to RTBC.
Comment #32
dawehnerWell, this was just a line of turning on my brain, it doesn't belong into this issue, its just a general valid comment 100% unreated with the issue.
Comment #33
jhodgdon+1 for RTBC. And +1 for a follow-up (not at all related to this patch) that sets the default for Status field to Published/Unpublished. Actually though we should probably file that issue and either put the issue link in the @todo or take it out since that's not where the code will go. And...
Presumably Status should say "Published"/"Unpublished", promote should say "Promoted"/"Not promoted", and sticky should say "Sticky"/Not sticky. We should be able to do all of them in one issue.
Needs work to fix/remove that @todo and file the new issue.
Comment #34
dawehnerThere we go: https://www.drupal.org/node/2465623
Comment #35
webchickExcellent. This looks like a really great start.
Committed and pushed to 8.0.x. Thanks!