Problem/Motivation
Views uses EntityViewsData and sub-classes to generate views data for entity base fields.
The bulk of these use the 'standard' plugin which extends from HandlerBase.
HandlerBase::access does not respect field level access.
As a result no field level-access is applied for entity base when used in views.
Patch demonstrates that comment's hostname field level access isn't respected.
Proposed resolution
Make the 'standard' plugin defer to the relevant entity access controller.
Remaining tasks
Patch
Review
User interface changes
none
API changes
none
Beta phase evaluation
Issue category | Bug because if you implement an access plugin to define field level access, you'd expect views to respect it |
---|---|
Issue priority | Critical because security |
Postponed until
Comment | File | Size | Author |
---|---|---|---|
#45 | views-base-field-access-2385443.45.patch | 8.52 KB | larowlan |
#36 | views-base-field-access-2385443.36-fail.patch | 8.52 KB | larowlan |
#36 | interdiff.txt | 1.92 KB | larowlan |
#35 | views-base-field-access-2385443.34.patch | 8.33 KB | larowlan |
#33 | views-base-field-access-2385443.33.patch | 22.53 KB | larowlan |
Comments
Comment #1
larowlanComment #2
jibranTagging
Comment #4
larowlanSo here's a pass at fixing it, burns me up to add use a function, but I think we need to tackle HandlerBase using function_exists before we have a choice? and that seems like a big bite to chew.
Comment #6
dawehnerWhat a wonder, views did not got an update while the entity world changed.
Let's expand the unit test coverage of EntityViewsData all the time
Does this need to be a function or can it be any kind of callable?
Note: This all is really related to #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency which basically would tackle the problem from the other side
Comment #7
larowlanNote that #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency would solve this
Comment #8
larowlanRelated #2344151: Comment field access doesn't work if $items isn't present
Comment #9
larowlanSomething less icky
Comment #10
larowlanmissed a bit
Comment #11
jhodgdonWhy don't we postpone this issue, or mark it as a duplicate? It seems like we need to have the base fields using Field UI anyway on #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency, which is already critical and should fix this problem, so this is just a stop-gap fix. I would be in favor of making the title of that issue "... and entity access is not respected" and combining this into it, since the real fix would be the same and we already need to fix that other issue. Whatever is being done here will just be discarded.
Comment #12
larowlan@jhodgdon - I asked the same question on irc and was told to leave this one open, in case the other was demoted to major.
Comment #13
larowlanComment #14
chx CreditAttribution: chx commentedComment #15
Gábor HojtsyI sure hope there is no chance #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency is demoted to major, it is impossible to make a multilingual view where both base fields and configurable fields exist now, so...
Comment #16
dawehnerJust to be clear, its entirely wrong that this behaviour is attached to the Standard class for multiple reasons:
a) numeric fields are using a different class already
b) why do we not use the existing 'access callback' mechanism ... as you probably know, we could also expand that one to support what you want. its a callback, you can't get more flexibility.
Comment #17
catchIf this is postponed on [#9416717] then that issue can't/shouldn't be demoted to major.
Comment #18
chx CreditAttribution: chx commentedWhat is this postponed on...? catch's link doesn't seem to be valid or even resembling any of the issue mentioned here (I even tried https://www.drupal.org/comment/9416717/view)
Comment #19
jibranI think it is postponed on #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency
Comment #20
dawehnerYeah its exactly postpoed on that.
Comment #21
jhodgdonShouldn't this actually just be marked as a duplicate of that other issue, rather than postponed on the other issue? Will there be anything left to do here when the other (which is already also critical) is fixed?
Comment #22
dawehner@jhodgdon
I'm a huge fan of using issues as bugs, not only for solutions.
... I think we could for example leverage this issue to have dedicated tests for access checking.
Comment #23
jhodgdonOK. There is no reason to postpone writing tests, which would also allow us to see if the other issue fixes the problem.
Comment #24
dawehnerAgreed
Comment #25
alexpottComment #26
alexpottDIscussed with @dawehner on IRC - it would be great if #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency contained the test coverage since its implementation will solve this - but we should check once that issue is in.
Comment #27
xjmComment #28
xjmComment #29
plachComment #30
webchickSince this is security-related, tagging as D8 upgrade path.
Comment #31
dawehnerNote: I think we should have a test which uses the entity_test entity instead of just the comment entity type as @larowlan did earlier.
Comment #32
larowlangiddyup
Comment #33
larowlanSo here's a test with entity_test - but it passes :( - so maybe this is a non-issue?
But comment test above shows it is?
No idea.
Battery flat, that's me for today.
Comment #34
larowlanOh, it passes because field API field != base field, will fix when battery is back
Comment #35
larowlanthis moving closer, but getting errors with 'missing base fields'
something not right in my field definition in the views config.
Comment #36
larowlanOk, so this demonstrates the fail/access bypass.
Not starting on fix - do you prefer the fix at #9 or #4 ?
Comment #38
dawehnerWell #7 honestly, but we need a proper test coverage.
Comment #39
larowlanEven better, so should we move this test over there and close this?
Comment #40
dawehnerWell, not sure, we could do that for sure, or just leave the test for this issue, as they are logically two separated issues, its just an implementation detail that the
other issue fixes this already.
Do what you think is the best thing to do, but yeah adding the comment test coverage should be also included. More test coverage is
not a bad idea.
Comment #41
Gábor HojtsySo #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency solves some of the base fields. IMHO we should figure out what other base fields are critical to resolve and either elevate those issues as well and postpone this one or choose another course of action.
Comment #42
jhodgdonWe should discuss that on #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, which lists all of the fields, probably?
Comment #43
dawehner.
Comment #44
dawehner$issue_title--
Comment #45
larowlanBlocker is in
Comment #46
larowlanComment #47
larowlanWith the blocker in, this now passes - as expected - so this test-only patch is ready in my books.
Comment #48
Gábor HojtsyLet's get this in then :)
Comment #49
dawehner+1
Nice work!
Comment #52
webchickYay, moar test coverage! :)
Committed and pushed to 8.0.x. Thanks!
Comment #54
Gábor HojtsySuperb thanks all!
Comment #55
YesCT CreditAttribution: YesCT commentedlooks like this evolved into a task along the way.