Problem/Motivation
An issue was filed against rest.module
that GET /entity/vocabulary/tags?_format=…
did not work: #2772413: REST GET fails on entity/taxonomy_vocabulary/{id} 403 Forbidden with error.
Turns out this is because the user did not have the administer taxonomy
permission. But of course… granting that permission has severe security implications too, because that'd mean that user can also create/update/delete vocabularies!
Proposed resolution
Also grant view
access to Vocabulary entities if the user has the access taxonomy overview
permission.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#64 | 2808217-64.patch | 1.92 KB | Wim Leers |
Comments
Comment #2
Wim LeersFailing test coverage.
Comment #3
Wim LeersAnd solution.
Comment #4
Wim LeersComment #6
tedbowSince \Drupal\taxonomy\VocabularyAccessControlHandler::checkAccess defaults to allowed for 'view' then route
entity.taxonomy_vocabulary.overview_form at /admin/structure/taxonomy/manage/{taxonomy_vocabulary}/overview
is now allowed for anonymous users because it uses _entity_access: 'taxonomy_vocabulary.view'
Worse yet an anonymous user can actually change the order of the terms, submit the form and the change sticks.
To try to:
Also on that page you are given edit and delete links but if you click them you get "access denied
I am not sure what permission should apply to that page. "administer taxonomy"? You can't use _entity_access: 'taxonomy_term.update' because a specific term is not sent in the route.
Comment #7
Wim LeersWhat a great review! Thanks so much :)
I think that makes sense.
Comment #8
tedbowChanging route entity.taxonomy_vocabulary.overview_form to require permission 'administer taxonomy' because this form lets you edit multiple terms at once.
Effectively you had to have this permission before this patch with core because there was no separate "view" permission for vocabularies.
Comment #9
sluceroI ran into a similar issue affected by the same permission checking, except that I was just trying to render the vocabulary label. This patch fixed the issue for me and after testing the various administrative pages for vocabularies everything was still properly restricted.
Comment #10
Wim LeersThis looks like it's close!
Why not:
_entity_access: 'taxonomy_vocabulary.update'
?Why not write
VocabularyAccessControlHandlerTest
, which can then just extendUnitTestCase
? No modules to install then. Much faster.Comment #11
tedbowBecause although what's in the route is a taxonomy_vocabulary what the form actually lets you update is multiple taxonomy_term entities. So from my understanding you would write and access checker to check access on every term in the vocabulary which presumably be very expensive. Because they could have could have 'edit terms in BUNDLE" permission but couldn't hook_entity_access override for any particular term. So how do you handle re-order when the user has access to some terms but not others?
Comment #12
Wim LeersRight, reordering terms != "update vocab", it's "update lots of terms".
Thanks for the explanation!
#10.2 is a nit, but it'll make our life easier in the long run. It should actually be pretty simple. So once that's done, this is RTBC.
Comment #13
YesCT CreditAttribution: YesCT commentedworking on #10 2.
(here is just a tiny bit of a start, moving the test and having it be a unit test, not working yet)
Comment #14
YesCT CreditAttribution: YesCT commentedWell, this runs, but doesnt actually make a vocabulary, (or test anything useful).
Note to self, run test with: (after cd core;)
../vendor/bin/phpunit modules/taxonomy/tests/src/Unit/Views/VocabularyAccessControlHandlerTest.php
Not sure what I should be mocking.
Looking at examples in
core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
and
core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php
That's it for now. I'll post back when I start on this again. (So, feel free to work on this, just post a comment.)
Comment #15
tedbowWorking on this
Comment #16
tedbowOk I completed the test @YesCT started.
I mocked the Vocabulary itself. I won't sure if I should have created an actual vocabulary.
Comment #18
tedbowTests passed
Comment #21
Wim LeersHm, I found #16 needlessly complex (and #14 was already heading in that direction too).
So rather than pointing out all the details that I think can be simplified, it was easier to just do it myself in this instance. Hopefully YesCT & tedbow can review this. And hopefully it helps them writing simpler unit tests in the future :)
Key points:
assertVocabularyAccess()
method that YesCT added made things much more complex than necessary. Adding suchassertSomeThing
methods makes sense when there's a lot of reuse. But that's not the case here.Prophecy
for stubbing helps to make things a lot simpler. Added bonus: this doesn't use strings, but actual class references, which improves discoverability/refactorability.@covers
,@coversDefaultClass
etc. This means less useful feedback from PHPUnit, and also needlessly complex/arbitrary documentation.hasPermission('administer taxonomy')
, which is the only thing this is testing.Previous patch vs this one:
43 insertions(+), 96 deletions(-)
Comment #22
Wim LeersOh, and this definitely does not belong in the "Views" subnamespace.
Patches prior to #21 were not yet testing this, and this now is. So it's actually a more complete test too.
Comment #23
Wim LeersChange record created: https://www.drupal.org/node/2830467.Comment #24
Wim LeersEh, that's the CR for #2758897: Move rest module's "link manager" services to serialization module. I posted #23 to the wrong issue. D'oh!
But this probably also needs a CR?
Comment #26
Wim Leers#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method added comprehensive test coverage for the
Vocabulary
entity. We now need to update those tests too.Comment #27
Wim LeersI wonder if we don't want to have some permission to obey? The Block, Comment, File and System modules all use the
access content
permission to do this, despite not depending on thenode
module.So I propose we do the same here. If we fix that at some point, we'll need to move the
access content
permission to the System module and provide an upgrade path anyway.Comment #28
tedbow@Wim Leers thanks I have little experience with this type of unit test and \PHPUnit_Framework_TestCase::prophesize specifically. One can say that in this case you are the prophet of prophesize ;)
I am not sure what the standard for docs on data provider functions. But looking at some of the recent ones added to core it seems to be a comment about what it is returning or "Data provider for ::xx()"
Otherwise looks good.
Comment #29
tedbowComment #30
Wim LeersSome have that, some don't. It's entirely superfluous, so I prefer not adding that.
Comment #31
tedbowOk thanks for clarification. back to RTBC
Comment #32
dawehnerI'm wondering whether this is a regression somehow. I could totally imagine to write a module which allows different roles to manage different taxonomy vocularies. This modul might have hooked into the access checking, not now that will no longer work. Is there no way we could deal with that in a more BC compatible way?
NoteL you don't need
Argument::is()
, simply passing along the value itself does the same.Comment #33
Wim Leers#32
Comment #34
xjmI probably should review this.
Comment #35
xjmComment #36
xjmComment #38
Wim LeersDrupalCI crashed, retesting.
Comment #39
MixologicTesting some other envs to sort out an inconsistent testbot permission issue.
Comment #40
dawehnerCan't we introduce a new operation for that to at least not bypass the entity access system completely?
Comment #41
catchThat looks like CNR.
Comment #43
Wim LeersWe need feedback from the subsystem maintainer on @dawehner's suggestion in #40.
Comment #44
dawehner@Wim Leers
I pinged berdir, as I guess this is sort of an entity API question.
Comment #45
dawehnerBerdir pointed me to #2658956: Taxonomy vocabulary data not available as views fields ... which got reverted due to
. This certainly a problem here as well.
Comment #46
BerdirThat is actually fixed here, by overriding that access check. But still, I think we can merge those two issues.
And there's #2599128: Allow user with edit terms permission to access vocabulary overview as well, which is already referenced. That allows view access to the vocabulary if a user has edit or delete permissions for the contained terms. But that actually conflicts with this as we can't do both.
Comment #47
Wim Leers#45:
#46:
So back to NR.
Comment #48
Wim LeersThis has been blocked on subsystem maintainer review (@xjm, see #34) since December 6.
Comment #49
Wim LeersThis is also blocking #2824408: To be able to create/update/delete Term entities via REST, one should not have to grant the 'administer taxonomy' permission.
Comment #51
xjmOkay, so it's granting "view" permission based on access content, and then just "administer taxonomy" for the rest. That would make sense. I went to look for where the "administer taxonomy" was being granted for the rest:
This specific fix makes sense, since this route is kind of editing the vocabulary in addition to viewing it. It's more than just a page to view the vocabulary; it's an administrative form.
So fall back to the parent if it's anything but the view op. Which comes from the basic entity access control handler, which is now:
So there it defines it as falling back to the admin permission for the entity type. Which is set on the entity annotation:
Looks good to me!
It looks like a lot of the other issue is a duplicate, so how about postponing it on this one?
Comment #52
dawehnerI am wondering whether we could maybe tackle this problem by having a custom operation. Bypassing the entity access system just makes me really nervous.
Comment #53
xjm@dawehner, this is the form exactly where you "administer taxonomy" so I don't think using that permission is bypassing the entity system.
Isn't this the fallback that was agreed on in e.g. the issue for the View entity access control handler, to fall back to the admin permission? All that line does is change a different way of referring to "administer taxonomy" in HEAD to just referring to it directly because using administer taxonomy as a view op for a vocabulary outside the context of that edit and administration form did not make sense.
There are other issues to discuss adding more granular permissions for taxonomy (which I have strong feelings about) but I think this patch actually makes no change to core taxonomy through the UI and just makes non-UI permissions more like what we'd expect.
Did I miss anything here?
Comment #54
dawehnerI would really just ask one of the entity subsystem maintainers whether its okay to not use the entity access control handler. To be honest its probably not worth a discussion/fight :)
Comment #55
xjmWha? How is this patch making taxonomy not use the access control handler?
There's a form for administering taxonomy, on which you view the vocabulary and also administer taxonomy. What's not okay about specifying that that form should require the administer taxonomy permission? I'm clearly missing something.
Comment #56
xjm@dawehner, it is not worth a fight, but it is worth a discussion since (a) I'm the component maintainer for taxonomy and (b) I am also a core committer who might commit this patch, so if I have a misunderstanding about it, it's worth correcting my misunderstanding.
The form that the route controls is an administration form for both the vocabulary and terms in it.
Comment #57
Wim LeersI think dawehner is nervous about changing a route definition to use a permission instead of entity access. That's something we've not done anywhere else for REST-related issues.
xjm says that they're equivalent. Which is true, by default. But it's possible that somebody is altering entity access for vocabulary config entities, which would then result in route access for the
entity.taxonomy_vocabulary.overview_form
route no longer respecting those alterations. I'm pretty sure that that is what dawehner is getting at.That'd also explain why he says he doesn't want to get in a fight over this: because it's a theoretical thing.
Now, why do we do it this way? tedbow proposed this in #6 and implemented it in #8. I asked the same question as dawehner in #10. tedbow explained it in #11.
Comment #58
Wim Leersentity.taxonomy_vocabulary.overview_form
route with less powerful permissions thanadminister taxonomy
So I don't think we can actually close any of the other issues; they're about different problems.
Comment #59
dawehnerWell yeah, as wim described it in
it feels almost like an excuse to shortcut the entity access system here. Its certainly not something I would expect to happen. I'm a bit strict here because I fear that more people in contrib/custom land might follow the example.
Oh wow, we talk about tough stuff here. Equality is non a trivial subject ;)
Comment #60
BerdirI posted a detailed overview of all related issues that we have around this topic in #1848686-179: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission) (#179 if the link does not work).
My recommendation would be to postpone this on that issue and figure out the term overview access there, then this will get fairly trivial.
I also do have two separate suggestions for this issue.
A) We could make REST support the 'view label' permission somehow, and allow to access an entity then but only return the ID and the label (maybe UUID as it is referenced through that?). I think this would solve the 90% use case for having to access a vocabulary in REST (although I think a lot of cases would just hardcode those labels client-side as they don't really change).
B) Related to that and briefly mentioned elsewhere already, we could also automatically expose referenced entity labels in the entity reference normalizers based on the same operation. This would give you immediately give you access to node type label, term labels, author labels and also vocabulary label when fetching a term and solving the 80% use case (rough estimate ;)) for those additional referenced entities *without additional requests* which I think would be a huge win for anyone trying to do decoupled with REST.
Comment #62
Wim Leers#1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission) landed now — hurray! A huge step forward :) It allowed us to close #2824408: To be able to create/update/delete Term entities via REST, one should not have to grant the 'administer taxonomy' permission, because it was solved completely!
Now let's look at this issue. #1848686 did add a
VocabularyAccessControlHandler
, which already helps this issue. But it did not add per-vocabulary permissions. It did add aaccess taxonomy overview
operation forVocabulary
entities.#1848686 also addressed many of the tricky bits that were being discussed above (before #60) — relating to entity access vs permissions etc.
So let's consider this issue having been reset as of #60, because #1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission) has been committed, which completely changes the landscape/this patch.
Berdir made two suggestions in #60:
How would we distinguish between a
GET
that wants toview
vsview label
? I like your thinking, I agree it'd be an elegant solution for a common need for many config entities, but for e.g. Vocabularies it still wouldn't be enough: we need to be able to read thehierarchy
key too. (And yes, for clients that hard code the label, they could also hard code thehierarchy
value. But this issue is about solving the general problem, of providing access to all necessary Vocabulary metadata from a client without granting admin permissions.)Similar answer as the above. On top of that, this only really works for the
hal
normalization. Because only there we have the concept of a?included_fields
query string to customize the response. So it can work for thehal_json
format, but not for thejson
orxml
formats.The mental model for
json
andxml
formats has been "there's nothing to customize".Of course, this is totally up for debate. But again, this doesn't solve the full problem that this issue aims to solve. Created a new feature request issue for this though, with a patch: #2913518: Expose labels of referenced entities.
Comment #63
Wim LeersSo here's the alternative/new proposal I'm making: allowing
Vocabulary
entities to be viewed if the user has theaccess taxonomy overview
permission (or theadminister taxonomy
permission, which is what's required today). Because theaccess taxonomy overview
permission says this:If you can get an overview of all taxonomy vocabularies, you should also be view each individual taxonomy vocabulary, right?
If people prefer creating a new permission, that's fine by me too. But this would be the least disruptive, simplest solution for sure.
Comment #64
Wim LeersForgot the patch, hah!
Comment #65
tedbow@Wim Leers I like this solution. I think it does not mean we can't do @Berdir suggestions in #60 A or B later but think in either case the 2 permissions 'access taxonomy overview' and 'administer taxonomy' should allow a user to view the vocabularies. So this means we don't have to worry about BC breaks later because removing this already existing permission would be a BC break in itself.
Re
The current state of the patch on #2913518: Expose labels of referenced entities that you created does expose the label with no configuration so I think that issue nicely complements this issue.
So with the current the response to GET a vocabulary is
Interestingly on the Terms overview page you can't see the 'description' though it would make sense to show on that page. So that would be something you could see via REST you can't now see through the UI.
But would any of the other properties that come back be an issue. I don't think so but I am not sure what you can do with
default_config_hash
Other than that I think this patch is a good idea.
One last thing, the new permission is defined as
Should we update title or description somehow otherwise it would be nearly impossible to figure this permission would allow you to GET vocabularies via REST.
But maybe this is a more general problem since in
\Drupal\Core\Access\AccessResult::allowedIfHasPermissions
we send back the key for the permission but the user sees the title and description via the UI.Comment #66
dawehnerI think its up to the UI to decide what to show, not the REST resource. Given that I think we should continue exposing the description.
I think we should open up an issue and hide this from the outside world? This is a property which is just set on module install time.
Comment #67
tedbow@dawehner my point in #65 regarding properties returned via REST and not in the UI is that this patch is explicitly using a permission that is described to as giving access to a "page" not the vocabulary itself.
So we are using a permission that gives the user to view a subset of properties of an entity to then give them access to the whole entity.
I think this is probably the best solution but we should be careful because as someone configuring the REST endpoints and permissions it is very unclear what you are actually giving access to.
Comment #68
Wim LeersAgreed. But that's out of scope here — it's something that applies to all config entities exposed via REST (and JSON API)!
I agree with @tedbow's call for caution. That's exactly what I was getting at in #63. But it sounds like both @tedbow and @dawehner think this is acceptable?
Comment #69
dawehnerIts funny that we have descriptions in vocabularies in the first place if we nowhere ever expose it :)
Comment #70
Wim LeersWell… we do show it here:
It's definitely strange that we don't expose it in the actual widget that we expose to content authors though!
Comment #71
Wim Leers@tedbow raised a point about
"_core": {"default_config_hash": … }}
, for which I opened a new issue: #2915414: Omit "_core" key from normalized config entities, which includes the default config hash. Patch is already included!Other than that, it sounds like everyone (well, @tedbow, @dawehner and I) like this new approach? If so, what remains to be done here? Only subsystem maintainer review? Already updated the IS to reflect this new approach.
Comment #72
Wim LeersComment #73
xjmSaved credit for this issue.
Comment #74
Wim Leers#72 has been more than a month, which is more than a week, so removing
in favor of .Comment #75
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think #64 is an improvement to what's in HEAD and has no downside, so removing the "Needs framework manager review" tag.
I agree with #65 that the title and description of the permission don't currently provide enough discoverability to people to know that this permission is needed for access to vocabulary GET requests. I don't think we need to hold up this fix on getting that wording right though, so let's open a follow-up for that.
Do we have a master issue for the general pattern of what permissions to use for things that exist as REST requests with no corollary in HTML (such as "viewing" a vocabulary)? If not, let's open one.
Comment #76
Wim LeersThank you!
You're right that the UI at
/admin/people/permissions
doesn't make that very clear, but I assure you that the REST response does make it crystal clear:(Since #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out.)
Created a follow-up to update those messages to not only provide the permission machine names, but also the human-readable names: #2925894: Consider updating AccessResult::allowedIfHasPermission(s)() to list human-readable permission names when they don't match their machine names.
#2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity" has lots and lots of prior discussion on that subject, so that is the one to look at, and perhaps generalize later. We shouldn't start a new issue from scratch I think.
Comment #77
Wim LeersI wanted to reroll this patch, but there's literally nothing to do. Moving to RTBC per
That's exactly what I did!
(He also raised concerns about vocabulary descriptions being exposed via REST, but not via the taxonomy overview pages, but @dawehner addressed those concerns in #66, as did I in #70.)
Comment #78
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI also think there's nothing left to do on this patch, but I'm not comfortable with committing it if I'm being included in #77 as a partial reason to RTBC it. So, setting back to NR to see if @tedbow, @Berdir, or another reviewer agrees with it being RTBC.
Comment #79
damiankloip CreditAttribution: damiankloip at Acquia commentedDefinite +1 on this change, and for RTBC. It's not ideal for any user to require the 'administer taxonomy' permission just to consume a vocabulary!
Comment #80
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks. Ticking credit boxes for reviewers.
Comment #82
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.5.x.
Comment #83
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't know yet if we want to include this in the "8.5.0 highlights", but tagging for now just in case.
Comment #84
Wim Leers#83: I don't think this is a highlight TBH.
Comment #85
dawehnerI agree with @Wim Leers the usecases of fetching a vocabulary and displaying it is, from my perspective, really limited, still it is an important detail of being "API first".
Comment #86
Wim LeersCompletely agreed, @dawehner!
Comment #87
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOK :)