search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Fixed: Taxonomy terms: createAccess() should include per-vocabulary check. #5481

Closed indigoxela closed 2 years ago

indigoxela commented 2 years ago

Description of the bug

Function createAccess() in taxonomy.entity.inc does a very simple check - only if user has 'administer taxonomy' permission. The individual permissions per vocab aren't considered ("create terms in XXX").

This leads to misleading results when using entity_access().

Expected behavior

A complete permission check, not only admin permissions.

Additional information

indigoxela commented 2 years ago

A PR is available for testing and review.

argiepiano commented 2 years ago

I did some testing.

I applied the patch, as well as https://github.com/backdrop/backdrop/pull/3926.

I tried running:

$u = user_load(2);
dpm(entity_access('create', 'taxonomy_term', NULL, $u));

I'm getting a fatal error:

Error: Using $this when not in object context in TaxonomyTerm::createAccess() (line 123...

If I revert this patch, things work (but see my comments about the results in https://github.com/backdrop/backdrop-issues/issues/5474)

[EDIT: for this error to happen, the user must NOT have the "Administer vocabularies and terms" permission)

argiepiano commented 2 years ago

I believe the error I reported above is due to the fact that createAccess is a static method, and you can't use $this in that context.

indigoxela commented 2 years ago

Oops, right, that's a static. PR updated.

This issue is currently sort of blocked by #5474. Without the changes in function entity_access the fix here won't help much.

herbdool commented 2 years ago

I don't understand how it's blocked. Won't this help even without that?

argiepiano commented 2 years ago

I've looked at the code and tested. LGTM 👍🏽

And yes, this PR will help even without the entity_access fix. It's used by taxonomy_term_access()

indigoxela commented 2 years ago

Won't this help even without that?

Ah, you're both right, the create check in entity_access does already exist, so it will be helpful. Also in taxonomy_term_access().

quicksketch commented 2 years ago

Thanks folks! I confirmed this was not a security issue, since it's a permissions expansion, something that was introduced in #382.

I merged https://github.com/backdrop/backdrop/pull/3930 into 1.x and 1.21.x.