Problem/Motivation
On bulk forms no access checking is performed. This means that once you got access to admin/people you can pretty much do whatever you want
Entity access checks are not performed when executing a bulk operation from entity overview pages, such as the content overview page (admin/content) or the user overview page (admin/people).
Proposed resolution
- Add a new method on the ActionInterface:
+ public function access($operation, $object, AccountInterface $account = NULL, $return_as_object = FALSE);
- Each action plugin has to implement based upon the object, whether the account can perform actions on it
- We rely on the field access checking, when we just update
$node->status
, but we rely on
entity access checking for things deleting nodes - In the actual bulk operation code we check access for the chosen action on each row, and execute it, if access was granted
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Reroll the patch if it no longer applies. | Instructions | ||
(after a reroll) Create a patch that is fixed so installs and tests run. Include an interdiff | Instructions | ||
Add automated tests | Instructions |
User interface changes
None
API changes
See beta evaluation
Beta phase evaluation
Issue category | It is a bug, because not checking access on a misconfigured view could allow people to add roles, for example and so get any permission |
---|---|
Issue priority | Critical, because of security |
Prioritized changes | The main goal of this issue is security. |
Disruption | Somehow disruptive because of the following reasons:
|
Original issue
How to reproduce
For user admin view
-
Create a user (useradmin with uid 2) with the following permissions:
- administer users
- access user profiles
- Implement
hook_user_access()
in a custom module and deny delete access on any user entity (see code example below). - Create an authenticated user (dummy with uid 3).
- Login as useradmin.
- Try to go the cancel page of the dummy user (user/3/cancel). You should get an access denied page, so you are not allowed to cancel the user.
- Go the user overview page (admin/people).
- Select the dummy user from the overview and try to apply the action "Cancel the selected user account(s)" on it.
You will go to admin/people/cancel and you are able to select a cancel method. If you choose, for example, "Delete the account and its content." the user is deleted although you shouldn't be able to do that.$entity->access('delete')
will returnFALSE
for user 3.
For node admin view
The steps to reproduce for nodes are similar, though if the user does not have the permission "administer nodes", access to admin/content/node/delete is denied even if the user is allowed to delete the selected node.
Implementation of hook_user_access()
/**
* Implements hook_ENTITY_TYPE_access() for entity type "user".
*/
function mymodule_user_access($entity, $operation, $account) {
if ($operation == 'delete') {
return FALSE;
}
}
Comment | File | Size | Author |
---|---|---|---|
#132 | 2172017-132.patch | 43.97 KB | dawehner |
#130 | interdiff.txt | 979 bytes | dawehner |
#130 | 2172017-130.patch | 44.47 KB | dawehner |
#126 | interdiff.txt | 1.88 KB | dawehner |
#126 | 2172017-126.patch | 44.2 KB | dawehner |
Comments
Comment #1
xjmThanks @MegaChriz, good find!
@digiv, @larowlan, @dawehner and I discussed this issue today and followed similar steps for nodes in D7 with VBO. From @digiv:
@larowlan confirmed with similar steps and a different node access setup. So, it looks like VBO is properly prevented from peforming the operation on rows the user doesn't have access to. Therefore, this issue can continue as a D8-only public issue.
Comment #2
tim.plunkettThe confirm form ones are easy enough, not sure about the rest of them...
That said, when someone has 'administer users', I'm not really worried about hook_user_access().
Comment #4
dawehner@tim.plunkett
I would assume that every action basically has an entity operation, so we need to check access using this. Therefore we basically need to add a method on the AccessInterface?
Comment #5
MegaChriz CreditAttribution: MegaChriz commentedMaybe a good start would be to write a test for this case. I've some experience with writing tests for d7 contrib modules and I'm in the process of learning PHPUnit, so I think I should be able to write a test for this one.
Now how should we test this? And should it be done via SimpleTest or PHPUnit?
Possible things to test:
Comment #6
MegaChriz CreditAttribution: MegaChriz commentedI made a start with writing a test for this issue. I used SimpleTest for now, because I've more experience with that and also because the related \Drupal\user\Tests\Views\BulkFormTest is also based on SimpleTest.
The test tests the following:
The test should have two failed assertions because the bug is not fixed yet.
Details
hook_user_access()
.hook_user_access()
needed to be added.I would like some feedback on this test, before I make a similar one for the node entity.
Comment #7
MegaChriz CreditAttribution: MegaChriz commentedSide note: I found out that any user who has access to an user view that provides the operation "Block the selected user(s)" can block any user (except perhaps user 1, not tested). I would consider this a misconfiguration, so I didn't include this in the tests.
Comment #9
EclipseGc CreditAttribution: EclipseGc commentedSo, fwiw, I really oppose tying any sort of access check to actions. Drupal has done similar historically and I've provided a number of patches over the past couple version to undo this in the worst places. A great example is node_add. At one time there was a check beyond what the menu item had on it meaning if you menu_altered that you'd get a white screen if you didn't have the permission to create nodes of that type, which is counter intuitive since you altered the menu to explicitly allow that. I fixed this in either d6 or d7, I don't recall.
This same logical problem exists for forms and is why you should never define an if statement around an element of form definition, always use #access.
In short, this is more of a "profile" problem than a Drupal problem. We continue to approach access with the idea that we should provide some sort of access paradigm beyond "menu" and while I acknowledge the general need for such a thing, we should loosely couple that as much as possible and fall back to depending on menu based access whenever possible. If you build a view with actions and give a user access to that view, then the assumption should follow that they can perform those actions. This is far more useful than the opposite which comes with mounds of code undoing Drupal's basic assumptions and is the primary problem in creating really great new install profiles and also has historically been the primary complaint from people performing that task. We all talk about Drupal as a platform, but the coupling of permissions and actions (as just one example) is a really good example of how we really prevent greater flexibility of Drupal in that space. I'd really encourage us to NOT add an access restriction component to actions.
Eclipse
Comment #10
MegaChriz CreditAttribution: MegaChriz commented@EclipseGc
You have a point. Adding an access layer to actions just feels conceptually wrong. I'm reading about the plugin API lately (I'm still in the process of exploring the D8 concepts) and I read the following about it:
So, yeah, an action plugin should not have to bother about access.
However, I don't agree with you that access checks should be limited to menu's/router paths. In that light you could also argue that the node access table shouldn't be queried when building a list of nodes.
My thought about solving this issue is that the list of entities should be filtered *before* it is handed over to the action plugin. So, that should be Views' responsability I guess. Additionally, there should be an option in Views to skip access checks for bulk operations (just as you can now with disabling SQL writing to skip node view access checks) to address use cases where access checks are undesired. I'm not sure yet if this should be set per bulk operation or for the complete display only, though I think for the complete display makes more sense.
Comment #11
BerdirYes. But the action needs to say what kind of access is required for it. You might have a action that just flags a node and another one deletes it.
This is also how VBO works, see _views_bulk_operations_entity_access(), which verifies entity access based on an access mask that the action returns.
And yes, when it's then outside of the plugin, it could be possible to not check it and execute it anyway, that's up to the caller. But we have an entity access API and anyone who is working with entities in the context of a user must respect it unless told otherwise, everything else is a bug.
Comment #12
EclipseGc CreditAttribution: EclipseGc commentedI'm not going to fight on this, I said my peace and I'll leave this be, but all of my points still stand. Treating Drupal this way is prioritizing the needs of the Standard install profile over the needs of all other install profiles, which has been a common theme going on for forever now and is one of the biggest contributing factors to making install profiles more difficult than necessary. Do what you will, but just make it as easy to override as possible, cause you are putting assumptions in place that people are going to want to undo (more often then not). Also keep in mind the override behavior for this has to be simple both inside and outside of VBO.
Eclipse
Comment #13
tim.plunkettI agree with @Berdir.
We can add a new access method to action plugins.
The "simple VBO" can choose to check it.
Other code can choose to bypass it.
Comment #14
MegaChriz CreditAttribution: MegaChriz commentedI'm not sure. This makes action plugins dependent on Drupal\Core\Access\AccessibleInterface and Drupal\Core\Session\AccountInterface. I read about plugins that they should be focussed on one task and they eventually should be reusable outside of Drupal. It seems to me that adding these dependencies would defeat that thought.
I can see the benefits of adding an access layer to action plugins, though. That way it's easier for code interacting with actions to perform access checks instead of figuring out themselves how to check access, thus the chance for security issues decreases.
Still, that would make an action plugin make assumptions about in which context it is run and because a plugin shouldn't be bothered with in which context it is run (so I understand), I'm not sure if we should add knowledge about entity access to action plugins, even when calling the access() method is optional.
Patch
I now attempted to fix this issue using a similar approach as in Views Bulk Operation 7.x-3.x. I also added a test for for the node bulk operation form. Both probably still need some work.
The fix
hook_action_info()
.checkAccess()
andexecuteAction()
to BulkForm as an attempt to break things up in smaller pieces and to hopefully make BulkForm easier overrideable.Note that I've not added this option yet.
Node: Bulk form entity access
Access is tested using node access records and the the private node feature from the node_access_test module. This may not be the ideal way of testing this bug, because the node access records and the the private node feature were originally designed for an other test. Also, the test testNodeDeleteAccess() currently would also pass if only "update" access checks are performed. So, this test would still needs some work.
Attached are two patches:
Comment #17
dawehnerWell, your approach makes actions, which are not bound to entities yet, bound to entity operations.
Given that I fear that we need to add an access method.
Comment #18
MegaChriz CreditAttribution: MegaChriz commentedYeah, I can see why that's wrong.
Just checking access for update/delete might be to limited
I've been playing with ideas of how to solve this issue. Now that I have ported the User protect module to Drupal 8 (which implements
hook_user_access()
), I came to the conclusion that just checking for one of the known entity operations (create, view, update, delete) might not be enough. Modules (like User protect) may want to block certain properties from being edited. Say that there is an operation that changes user names. It should then respect the permission "Change own username". Same would count if a module invents a permission for publishing content, that module should then be able somehow to get access checks performed for the "node publish" action. If access checks for actions would be limited to update and delete, then how can such module make the distinction between which actions are allowed?Action as operation?
The following idea popped up in my mind: what if actions themselves are the operation? So to check if a certain action is permitted on an entity, one would simply do this:
Modules that provide actions for entities would then implement
hook_entity_access()
to provide a fallback access check for each action:I've done something similar in the User protect module. Check
userprotect_user_access()
for an example:http://cgit.drupalcode.org/userprotect/tree/userprotect.module?h=8.x-1.x
Caveats
Comment #19
fagoFor Rules, actions definitely need access control - that's a pre-existing feature in d7.
In d7, there are actually two kind of access control mechanism what can be confusing and should probably be avoided in d8. Firts off, there is the 'access callback' which you can add to your action_info, it's used for determining access to configure an action in the first place. Then, there is also an access() method on the action obejct (and a respective _access callback for action implementors) which allow you to determine access control for a configured action object.
For d8, this reminds me of entities, which have create access + access for acting on entity objects as well. However, the problem of which plugins are available for a given user to configure in terms of access control seems to be a plugin-generic problem to me.
Limiting action access to certain entity operations is certainly to limited, in particular given #2011038: Use Context system for actions.
Comment #20
fagoTagging d8rules as this is related.
Comment #21
dawehnerThank you for your feedback?
Indeed, you could think of providing different plugins for different users, based, for example on permissions. On the block side, allow
different people to place different kind of blocks, which is totally different from accessing the block itself.
So if we want to add access, we have to add both a "listAccess" and an "access".
While access is pretty obvious, I wonder whether "list access" depends on the interiors of the plugin itself. Won't this be
something people might want to "configure" depending on their editorial workflow of the page? Using one permission
per plugin does not seem to be scalable at all for the UI.
Comment #22
EclipseGc CreditAttribution: EclipseGc commentedWe should NOT be considering any sort of default functionality of this sort of plugins at large. If your plugin type needs something like this, fine. If you want that in a trait so you can reuse it elsewhere, fine, but don't put this on all plugins everywhere. And while we're on the topic, I don't want to be permission bound on use of plugins. I've said for a very very long time now that allowing access of a particular action within a particular vbo is completely kosher. The view has access controls already and bypassing some other set of access controls by giving access to the view is a perfectly relevant use case. Could it cause problems? absolutely, but it will cause far fewer problems than the development pain of undoing ridiculous access checks.
I said it 6 months ago, I'll say it again, I think even considering this line of reasoning is folly. You will, of course, do whatever it is that you will do, but keeping the needs of install profiles OTHER than standard in mind would be a really great way to operate going forward into the future.
Eclipse
Comment #23
dawehnerSo, what about this kind of interface as first step?
Comment #24
Sharique CreditAttribution: Sharique commentedSubmitting patch in last comment for testing.
Comment #26
tim.plunkettI like @dawehner's proposal to keep it simple and let each plugin handle things. But each plugin might not have enough data to make a decision when this is called? That needs more investigation.
That said, as far as Actions go I'd probably defer to @fago, as I'd very much like to prevent Rules from having to fork core actions if possible.
Here are some notes about the patch itself.
Does this need to be plugin specific? We have \Drupal\Core\Access\AccessibleInterface... Also not sure how #2287071: Add cacheability metadata to access checks might affect this.
Oh I guess this is less complicated (no NULL)
Comment #27
dawehnerIn case this is true, some wrapper should care about that. The plugin should just know about itself.
Let's just get the patch pass to not demotivate people to discuss it here.
In general there are cases where the access actually depends on the object itself, what should we do in this case?
Comment #29
fagoThanks! :)
Agreed. The plugin can take its configuration and other set stuff into account if necessary, but the interface used for checking should stay clean. Howsoever, this should probably be just AccessibleInterface ?
Comment #30
dawehnerAgreed.
Comment #31
xjmComment #32
Désiré CreditAttribution: Désiré commentedComment #33
Désiré CreditAttribution: Désiré commentedHere I rerolled the last patch (seems it's broken), and created a test case for it.
Comment #36
Antti J. Salminen CreditAttribution: Antti J. Salminen commentedWorking a bit with the patch from #27. Am I missing something or is the one posted in #33 not based on that one at all?
Comment #37
Antti J. Salminen CreditAttribution: Antti J. Salminen commentedHopefully this is better, it should make use of AccessibleInterface and the install pass but not much more.
Comment #38
marthinal CreditAttribution: marthinal commentedComment #40
dawehnerDoes someone know whether we will be able to change the API here?
Its not such a distruptive change to be honest.
Comment #41
EclipseGc CreditAttribution: EclipseGc commentedWhy? What is the use case?
Eclipse
Comment #42
Sam Hermans CreditAttribution: Sam Hermans commentedre-rolled without the unused $operation
Comment #43
Sam Hermans CreditAttribution: Sam Hermans commentedinstalls correctly now
Comment #44
tim.plunkettComment #48
YesCT CreditAttribution: YesCT commentedalso needs a reroll.
added remaining tasks (the dreditor insert tasks button is useful to do that). It is important to keep the issue summary up to date (especially if this issue is to be used for the Friday critical office hours).
Comment #49
larowlanComment #50
larowlanComment #52
larowlanComment #54
larowlanPatch at 42 made incorrect changes, reverting
Comment #55
larowlanRe-roll of 37
Comment #56
larowlanComment #58
larowlanComment #60
larowlanComment #62
larowlanComment #64
larowlanComment #65
larowlangreen - anyone care to review?
Comment #66
MegaChriz CreditAttribution: MegaChriz commentedI put reviewing the patch on my todo list. First glance, it seems to me that checking for a permission for using a particular operation is too limited. I think it should be possible to override access with
hook_entity_access()
. Also, I see that the tests I wrote in #14 are no longer included in the latest patch.Note that I haven't followed this issue for a while, so I should take a closer look at the patch first to know if what I'm saying here still makes sense.
Comment #67
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe changes in
Drupal\views\Plugin\views\access\AccessPluginBase
andDrupal\Core\Block\BlockPluginInterface
, while they might be desirable are orthogonal to this issue and are API breaks. I think they should belong to a separate issue.Also, I don't understand why the new
AccessibleInterface
added toDrupal\Core\Action\ActionInterface
is not used anywhere. Isn't that the whole point of this issue?^ Most of those are bogus:
$account
is optional and$return_as_object
is not respected.Comment #68
dawehnerI have to agree ... #2393041: Use AccessibleInterface for views access plugins. #2393045: Use AccessibleInterface for block plugins.
Well yeah that issue got started with just some discussion about how the API looked like and than implementation started, stopped for a while,
so knowledge about it got totally forgotten.
At lesat we return booleans everywhere, much better than objects.
This interdiff should reflect this problem, hopefully.
Adapt the code to do so, and also changed the existing test, which just totally ignores access checking ...
Let's just allow everything!
Comment #70
dawehnerThis could be it.
Comment #71
jibranOver all patch look good to me. Just a minor doc issue. I think we can remove needs test tag now. Other then that it is RTBC.
We are throwing an exception in the function now so function doc needs update.
Comment #72
dawehnerAlright
Comment #73
jibranThanks. I think it is ready.
PS: Love the horse playing piano gif.
Comment #74
dawehnerWe most probably need a change record and beta evaluation!
Comment #75
MegaChriz CreditAttribution: MegaChriz commentedI will the test the patch in #72 in a few days.
Comment #76
dawehnerComment #77
dawehnerComment #78
tstoecklerExtraneous $f
Yes, let's actually fix this :-)
I would suggest 'execute' as we have $action->execute()
Comment #79
dawehnerAgreed
Comment #82
dawehnerMh, so its actually green.
Comment #83
larowlan@jibran if you like the horse playing piano - you can watch *and* listen thanks to this handy 10 hour mix https://www.youtube.com/watch?v=OWFBqiUgspg
Slightly nsfw.
I like to play this via the office speakers every now and again to make sure people are paying attention.
;)
Comment #84
jibranOther then this minor nit it is RTBC
This can be removed now.
PS: @larowlan 10 hrs that is crazy.
Comment #85
tstoecklerNot downgrading, because I *think* the current code is OK, but want to make sure: Should we be checking for the 'execute' operation here?
Comment #86
catchShouldn't this (and elsewhere) be checking 'bypass node access'?
Or does it assume that the user already has access to the node itself before it gets this far? Not sure why we don't check actual entity access here rather than the permission at all, and this isn't clear from the issue summary either.
That probably needs to be fixed?
Comment #87
dawehnerThis is yet another of these cases where things get tricky:
For the listing we have to fallback on the current way of doing
Does that sound sane?
Comment #88
xjmComment #89
MegaChriz CreditAttribution: MegaChriz commentedTwo issues in one
So there actually two issues in one here:
I originally reported this issue for the second one: for example that you could delete an entity via a bulk operation even though entity access for deleting that same entity returns false.
Maybe issue 2 can not be fixed before fixing issue 1, but the title of this issue (and summary) still reflects issue 2.
Patch in #79
Entity access is ignored
I tested the patch in #79 specifically on entity access by implementing
hook_ENTITY_TYPE_access()
and returningDrupal\Core\Access\AccessResult::forbidden();
. Logged in as an user with the permission "administer users", this resulted in an access denied page for user/x/cancel, but the user could still be deleted via the bulk form.Empty bulk form field
I edited the user admin view to be viewable by everyone and logged as an user with just the "access content" permission. This resulted into an "empty" bulk form field as seen in the image below.
I think the bulk form field should be hidden when there are no operations to choose. Maybe fixing this should be handled in a follow-up issue so to keep the focus on fixing entity access.
New patch
The attached patch adds in back the tests from #14 (which I updated to work with the latest core):
Tests if someone with the "administer nodes" permission can not edit/delete nodes in bulk that are protected by node access records. Note that the node access handler specifically returns
FALSE
when asked if the node may be edited or deleted.Tests if someone with the "administer users" permission can not edit/delete users in bulk that are protected by the module user_access_test which implements
hook_ENTITY_TYPE_access()
for entity type "user". Note that user/x/edit and user/x/cancel result into a 403 page.These tests should report 4 failed assertions. Moving state to "Needs review" so the testbot will evaluate these tests, though the issue status should still be "Needs work".
Comment #91
dawehnerThank you a lot to add the tests, really helpful.
After talking with @bojanz and @tim.plunkett we agreed that we just check access on the row level but not for the list of available plugins.
Comment #93
dawehnerFixed the test failure, partially caused by new test coverage introduced earlier.
Comment #95
dawehnerSo using 'administer nodes' as 'bypass node edit access' seems to be a bad idea.
After talking with @berdir on IRC, we realized that maybe we could use
checkFieldAccess
and not directlyentity access. This would directly has the advantage that 'administer nodes' will work for those actions, given that field access has specific code for 'edit' which
uses 'administer nodes'.
Comment #96
dawehnerIterated on top of the idea in #95
Comment #97
damiankloip CreditAttribution: damiankloip commentedNice, I like this!!
Rogue spaces
Should we assert the response from the submitted form? What is the expected behaviour there? Nothing? error message?
Comment #98
dawehnerCan't find that in the recent patch ...
We test later that we have blocked the user.
Comment #99
MegaChriz CreditAttribution: MegaChriz commentedFor the empty bulk form issue (see #89) I created a new issue: #2402155: Empty bulk form field when no actions are available. I noticed though that that issue no longer occurs with the patch from #98.
@dawehner Is access for available actions postponed or completely out of the question? Anyway, great that the issue is focussing now on the thing I originally created it for.
I've shortly tested the patch in #98 and I noticed the following:
Comment #100
dawehnerI'd kind of prefer fixing that in another issue, given that its a different problem space and not, as you said, the actual described problem we have.
That is a good question why.
UserAccessControlHandler::checkFieldAccess()
has the following piece of code:so this seems to be intended ... asked berdir about those kind of issues in the node module, and it was totally not clear whether we have a strategy here.
What would you expect to happen? Maybe getting a 403 in case you could not update any item (and send the message on top of it?)
Comment #101
dawehnerWorked quite a lot of the issue summary to bring it up to date.
Comment #102
dawehnerComment #103
Wim LeersThat sounds wrong. Field access checking for updating individual fields is great, but it should also check access to the entity. Quick Edit does this also for example. See
EditEntityFieldAccessCheck
.s/data value/object/
Can an action ever have multiple operations? I don't think so?
Then why always pass in
'execute'
instead of just omitting the$operation
parameter?- s/login/log in/
- 80 cols
See my first remark. If we check this here, we should check if *everywhere*. Hence it makes more sense IMHO to just omit
$operation
altogether.Should also check access to the Node
'update'
operation. SeeEditEntityFieldAccessCheck
.s/node bulk form/node bulk operations form/
?
Node::load()
Don't we want to inform the user about which objects he could not modify, due to insufficient permissions?
Useless 'use' statements, because these are the only changes to these files :) Probably a remnant from earlier patch iterations.
User::load()
Comment #104
kim.pepperTaking a look.
Comment #105
larowlanComment #106
kim.pepperFixes issues in #103 except 10. Not sure how we do this? drupal_set_message?
Also, the entity_load() changes were resetting cache. Assume this isn't needed.
Comment #107
kim.pepper...and the patches.
Comment #108
larowlanI think for interface parity.
So this might blow up.
Comment #110
dawehnerSome works ...
Comment #112
dawehnerComment #114
dawehnerThere we go.
Comment #115
kim.pepperRTBC +1 from me.
Comment #116
kim.pepperComment #117
larowlanLeftover? other than that I think this is ready (and really this could stay if pressed)
Back to Wim for a final look.
Comment #118
Wim LeersLooking great!
s/$result/$access/
wherever you're dealing with anAccessResult
object would make the code more legible.s/node/node status/
Like larowlan, I think this is a debug leftover, but like larowlan I think the committer can probably remove this on commit.
Leaving at NR because of point 1. dawehner, if you (or others) agree with point 1, I could reroll it for you if you want.
Comment #119
dawehnerLet's fix those points quickly.
Comment #120
Wim LeersComment #121
catchCouple of actual questions. Couple of very minor nits.
Is there somewhere we can document the requirement to have both entity and field access. It seems a bit fragile having to call both from within the actions each time.
Why does the sticky/promoted etc. access check also check status access?
s/had/have/
nit - a user.
nit 'administer users'
Comment #122
Berdir1. Is documented on EntityAccessControlHandlerInterface::fieldAccess(). The reason is that we do not want to call entity access 20 times if we are checking access to twenty different fields. It would also not always be clear how to map between the two exactly, as they work a bit different, especially for edit/create/update.
Comment #123
dawehnerI think checking both makes sense here, because the corresponding code action changes both:
Fixed the nits.
Comment #124
dawehnerAfaik all feedback from @catch got adressed
Comment #125
catchThe action itself looks wrong to me. If you have access to set something sticky/promoted (or unsticky/unpromote) that should work without requiring extra permissions that are unrelated. The action is trying to be 'helpful' by also publishing but that's causing inconsistent behaviour. Not introduced here we don't unpublish if un-promoting or un-stickying content, so that action would have a different access check despite being for exactly the same field. It's minor in relation to the rest of the patch but people will use these as examples for their own code so I think it's worth trying to get right here.
On the entity vs. field access, those docs are OK I think at least given the limitations of the current actions API.
Comment #126
dawehnerAlright, let's update them. It's right, the intention of the user is to just promote them, so let's adapt access for it.
Comment #127
Wim Leers#125: great catch!
#126 almost looks good… but I read #125 as saying that
PromoteNode
should no longer do$entity->setPublished(TRUE);
, andStickyNode
should no longer$entity->status = NODE_PUBLISHED;
.Comment #128
dawehnerI do not read it like that, seems like an out of scope change for me personally.
For me the access should be coupled to the concept of the action not to the congret implementation.
Comment #129
Wim LeersIn my reading, that's why catch said
Comment #130
dawehnerAlright, asked @catch on IRC. He meant it to change it.
On top of that I did a bit of research:
get the impress that tim did not changed the actual code or misread it
Based upon that, let's change it.
Comment #132
dawehnerJust a rebase
Comment #133
Wim LeersLooks perfect now!
Comment #135
catchOK. I still don't feel 100% comfortable with this - simply because I feel like we ought to be able to generically check entity access and not have to individually implement that for every actions access check. Easy for a module to add an action and get this wrong.
However that's more of a complaint about the actions API than this patch, so committed/pushed to 8.0.x, thanks!
Comment #137
TR CreditAttribution: TR commentedComment #138
quietone CreditAttribution: quietone at PreviousNext commentedPublish change record