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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.07 KB

Failing test coverage.

Wim Leers’s picture

FileSize
1.83 KB
2.86 KB

And solution.

Wim Leers’s picture

The last submitted patch, 2: 2808217-2.patch, failed testing.

tedbow’s picture

Status: Needs review » Needs work

Since \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:

  1. Generate some test tags. With devel_generate: drush gent tags
  2. As anonymous goto admin/structure/taxonomy/manage/tags/overview
  3. Move the top term to the bottom
  4. Save the form.
  5. The order stays.

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.

Wim Leers’s picture

What a great review! Thanks so much :)

I am not sure what permission should apply to that page. “administer taxonomy”?

I think that makes sense.

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
607 bytes

Changing 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.

slucero’s picture

I 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.

Wim Leers’s picture

Status: Needs review » Needs work

This looks like it's close!

  1. +++ b/core/modules/taxonomy/taxonomy.routing.yml
    @@ -74,7 +74,7 @@ entity.taxonomy_vocabulary.overview_form:
    -    _entity_access: 'taxonomy_vocabulary.view'
    +    _permission: 'administer taxonomy'
    

    Why not:
    _entity_access: 'taxonomy_vocabulary.update'?

  2. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyTest.php
    @@ -0,0 +1,37 @@
    +class VocabularyTest extends EntityKernelTestBase {
    

    Why not write VocabularyAccessControlHandlerTest, which can then just extend UnitTestCase? No modules to install then. Much faster.

tedbow’s picture

Why not:
_entity_access: 'taxonomy_vocabulary.update'?

Because 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?

Wim Leers’s picture

Right, 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.

YesCT’s picture

working 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)

YesCT’s picture

Well, 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.)

tedbow’s picture

Assigned: Unassigned » tedbow

Working on this

tedbow’s picture

Status: Needs work » Needs review
FileSize
7.1 KB
5.51 KB

Ok I completed the test @YesCT started.

I mocked the Vocabulary itself. I won't sure if I should have created an actual vocabulary.

Status: Needs review » Needs work

The last submitted patch, 16: 2808217-16.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review

Tests passed

The last submitted patch, 13: 2808217-13.patch, failed testing.

The last submitted patch, 14: 2808217-14.patch, failed testing.

Wim Leers’s picture

Hm, 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:

  1. the assertVocabularyAccess() method that YesCT added made things much more complex than necessary. Adding such assertSomeThing methods makes sense when there's a lot of reuse. But that's not the case here.
  2. Quite a bit of the stubbed behavior was simply unnecessary.
  3. Using 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.
  4. The comments were not yet updated to use @covers, @coversDefaultClass etc. This means less useful feedback from PHPUnit, and also needlessly complex/arbitrary documentation.
  5. Incomplete type documentation.
  6. "mock" should've been "stub"
  7. Stubbing multiple users (actually, accounts) does not make a ton of sense in this case. It's simpler to just stub an account and vary the return value of hasPermission('administer taxonomy'), which is the only thing this is testing.

Previous patch vs this one:

  • 43 insertions(+), 96 deletions(-)
  • The test is now 94 lines instead of 148.
Wim Leers’s picture

  1. +++ b/core/modules/taxonomy/tests/src/Unit/Views/VocabularyAccessControlHandlerTest.php
    @@ -0,0 +1,94 @@
    +namespace Drupal\Tests\taxonomy\Unit\Views;
    

    Oh, and this definitely does not belong in the "Views" subnamespace.

  2. +++ b/core/modules/taxonomy/tests/src/Unit/Views/VocabularyAccessControlHandlerTest.php
    @@ -0,0 +1,94 @@
    +    $module_handler->invokeAll('entity_access', Argument::any())->willReturn([]);
    +    $module_handler->invokeAll('taxonomy_vocabulary_access', Argument::any())->willReturn([]);
    

    Patches prior to #21 were not yet testing this, and this now is. So it's actually a more complete test too.

Wim Leers’s picture

Change record created: https://www.drupal.org/node/2830467.

Wim Leers’s picture

Issue tags: +Needs change record

Eh, 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?

Status: Needs review » Needs work

The last submitted patch, 22: 2808217-23.patch, failed testing.

Wim Leers’s picture

Assigned: tedbow » 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.

Wim Leers’s picture

I 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 the node 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.

tedbow’s picture

Hopefully YesCT & tedbow can review this. And hopefully it helps them writing simpler unit tests in the future :)

@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 ;)

+++ b/core/modules/taxonomy/tests/src/Unit/VocabularyAccessControlHandlerTest.php
@@ -0,0 +1,97 @@
+  /**
+   * @return array
+   */
+  public function vocabularyAccessProvider() {

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.

tedbow’s picture

Status: Needs review » Needs work
Wim Leers’s picture

Status: Needs work » Needs review

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()"

Some have that, some don't. It's entirely superfluous, so I prefer not adding that.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Ok thanks for clarification. back to RTBC

dawehner’s picture

  1. +++ b/core/modules/taxonomy/taxonomy.routing.yml
    @@ -74,7 +74,7 @@ entity.taxonomy_vocabulary.overview_form:
         _form: 'Drupal\taxonomy\Form\OverviewTerms'
         _title_callback: 'Drupal\taxonomy\Controller\TaxonomyController::vocabularyTitle'
       requirements:
    -    _entity_access: 'taxonomy_vocabulary.view'
    +    _permission: 'administer taxonomy'
    

    I'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?

  2. +++ b/core/modules/taxonomy/tests/src/Unit/VocabularyAccessControlHandlerTest.php
    @@ -0,0 +1,97 @@
    +    $account->hasPermission(Argument::is('access content'))->willReturn($has_access_content_permission);
    +    $account->hasPermission(Argument::is('administer taxonomy'))->willReturn($has_administer_taxonomy_permission);
    

    NoteL you don't need Argument::is(), simply passing along the value itself does the same.

Wim Leers’s picture

#32

  1. See #6 and later for the rationale. I also tried to break @tedbow's argument for this, but could not — see #10.1 for my Q and #11 for Ted's A.
  2. Good to know!
xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: 2808217-27.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

DrupalCI crashed, retesting.

Mixologic’s picture

Testing some other envs to sort out an inconsistent testbot permission issue.

dawehner’s picture

See #6 and later for the rationale. I also tried to break @tedbow's argument for this, but could not — see #10.1 for my Q and #11 for Ted's A.

Can't we introduce a new operation for that to at least not bypass the entity access system completely?

catch’s picture

Status: Reviewed & tested by the community » Needs review

That looks like CNR.

Status: Needs review » Needs work

The last submitted patch, 27: 2808217-27.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

We need feedback from the subsystem maintainer on @dawehner's suggestion in #40.

dawehner’s picture

@Wim Leers
I pinged berdir, as I guess this is sort of an entity API question.

dawehner’s picture

Status: Needs review » Needs work

Berdir pointed me to #2658956: Taxonomy vocabulary data not available as views fields ... which got reverted due to

[14:14:16] dawehner: which was reverted as it accidently allowed anon access to the term overview page

. This certainly a problem here as well.

Berdir’s picture

That 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.

Wim Leers’s picture

Status: Needs work » Needs review

#45:

This certainly a problem here as well.

#46:

That is actually fixed here, by overriding that access check.

So back to NR.

Wim Leers’s picture

This has been blocked on subsystem maintainer review (@xjm, see #34) since December 6.

Wim Leers’s picture

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Assigned: xjm » Unassigned
Issue tags: -Needs subsystem maintainer review +Needs issue summary update

Okay, 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:

+++ b/core/modules/taxonomy/taxonomy.routing.yml
@@ -74,7 +74,7 @@ entity.taxonomy_vocabulary.overview_form:
-    _entity_access: 'taxonomy_vocabulary.view'
+    _permission: 'administer taxonomy'

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.

+++ b/core/modules/taxonomy/src/VocabularyAccessControlHandler.php
@@ -0,0 +1,29 @@
+  public function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
+    // Allow all users to view vocabulary config entities.
+    if ($operation === 'view') {
+      return AccessResult::allowedIfHasPermissions($account, ['access content', 'administer taxonomy'], 'OR');
+    }
+
+    return parent::checkAccess($entity, $operation, $account);

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:

  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    if ($operation == 'delete' && $entity->isNew()) {
      return AccessResult::forbidden()->addCacheableDependency($entity);
    }
    if ($admin_permission = $this->entityType->getAdminPermission()) {
      return AccessResult::allowedIfHasPermission($account, $this->entityType->getAdminPermission());
    }
    else {
      // No opinion.                                                            
      return AccessResult::neutral();
    }
  }

So there it defines it as falling back to the admin permission for the entity type. Which is set on the entity annotation:

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -19,7 +19,8 @@
  *   admin_permission = "administer taxonomy",

Looks good to me!

It looks like a lot of the other issue is a duplicate, so how about postponing it on this one?

dawehner’s picture

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.

I am wondering whether we could maybe tackle this problem by having a custom operation. Bypassing the entity access system just makes me really nervous.

xjm’s picture

@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?

dawehner’s picture

Assigned: Unassigned » Berdir

I 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 :)

xjm’s picture

Wha? 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.

xjm’s picture

@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.

Wim Leers’s picture

I 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.

Wim Leers’s picture

It looks like a lot of the other issue is a duplicate, so how about postponing it on this one?

  1. #2599128: Allow user with edit terms permission to access vocabulary overview is about granting access to that same entity.taxonomy_vocabulary.overview_form route with less powerful permissions than administer taxonomy
  2. #1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission) is trying to do something similar, but differently again
  3. #2824408: To be able to create/update/delete Term entities via REST, one should not have to grant the 'administer taxonomy' permission is about Taxonomy terms, not vocabularies. We decided to do vocabularies first way back when.

So I don't think we can actually close any of the other issues; they're about different problems.

dawehner’s picture

e (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.

Well yeah, as wim described it in

I 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.

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.

xjm says that they're equivalent. Which is true, by default.

Oh wow, we talk about tough stuff here. Equality is non a trivial subject ;)

Berdir’s picture

I 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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Assigned: Berdir » Wim Leers
Status: Needs review » Active
Issue tags: -blocker
Related issues: +#2913518: Expose labels of referenced entities

#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 a access taxonomy overview operation for Vocabulary 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:

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).

How would we distinguish between a GET that wants to view vs view 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 the hierarchy key too. (And yes, for clients that hard code the label, they could also hard code the hierarchy 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.)

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.

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 the hal_json format, but not for the json or xml formats.
The mental model for json and xml 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.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review

So here's the alternative/new proposal I'm making: allowing Vocabulary entities to be viewed if the user has the access taxonomy overview permission (or the administer taxonomy permission, which is what's required today). Because the access taxonomy overview permission says this:

access taxonomy overview:
  title: 'Access the taxonomy vocabulary overview page'
  description: 'Get an overview of all taxonomy vocabularies.'

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.

Wim Leers’s picture

tedbow’s picture

@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

Because only there we have the concept of a ?included_fields query string to customize the response. So it can work for the hal_json format, but not for the json or xml formats.
The mental model for json and xml 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.

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

{
"uuid": "0eddcc49-3835-466f-b44b-4bbe9b43a8ce",
"langcode": "en",
"status": true,
"dependencies": [],
"_core": {
"default_config_hash": "lO5ziR5dVI1PpEeHZsSOfQ-Y7NWihSDKW8-MMf6uoms"
},
"name": "Tags",
"vid": "tags",
"description": "Use tags to group articles on similar topics into categories.",
"hierarchy": 0,
"weight": 0
}

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

access taxonomy overview:
  title: 'Access the taxonomy vocabulary overview page'
  description: 'Get an overview of all taxonomy vocabularies.'

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.

dawehner’s picture

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.

I 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.

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

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.

tedbow’s picture

@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.

Wim Leers’s picture

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

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.

Agreed. 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?

dawehner’s picture

Its funny that we have descriptions in vocabularies in the first place if we nowhere ever expose it :)

Wim Leers’s picture

Well… 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!

Wim Leers’s picture

@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.

Wim Leers’s picture

xjm’s picture

Saved credit for this issue.

Wim Leers’s picture

#72 has been more than a month, which is more than a week, so removing Needs subsystem maintainer review in favor of Needs framework manager review.

effulgentsia’s picture

I 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.

Wim Leers’s picture

Thank you!

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.

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:

$ http get http://d8/entity/taxonomy_vocabulary/tags?_format=json --body
{
    "message": "The following permissions are required: 'access taxonomy overview' OR 'administer taxonomy'."
}

wim.leers at acquia in ~/Work/d8 on 8.5.x*

(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.

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.

#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.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I wanted to reroll this patch, but there's literally nothing to do. Moving to RTBC per

@Berdir in #60
My recommendation would be to postpone this on that issue and figure out the term overview access there, then this will get fairly trivial.

That's exactly what I did!

@tedbow in #65
I like this solution.

(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.)

@effulgentsia in #75
I think #64 is an improvement to what's in HEAD and has no downside
effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

I 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.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Definite +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!

effulgentsia’s picture

Thanks. Ticking credit boxes for reviewers.

  • effulgentsia committed 730c5a9 on 8.5.x
    Issue #2808217 by Wim Leers, tedbow, YesCT, dawehner, xjm, Berdir,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.5.x.

effulgentsia’s picture

Issue tags: +8.5.0 highlights

I don't know yet if we want to include this in the "8.5.0 highlights", but tagging for now just in case.

Wim Leers’s picture

#83: I don't think this is a highlight TBH.

dawehner’s picture

I 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".

Wim Leers’s picture

Completely agreed, @dawehner!

effulgentsia’s picture

Issue tags: -8.5.0 highlights

OK :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.