Problem/Motivation
Facts for Drupal 8.0 & Drupal 8.1's REST:
- REST module's
EntityResource
respects Entity Access & Field Access. - REST module requires every entity type to be explicitly enabled in
rest.settings.yml
.
Therefore it is very confusing that every entity type gets another permission, per verb, to be able to actually use that REST resource.
Proposed resolution
@klausi at #1816354-1: Add a REST module, starting with DELETE:
Access control is also a problem right now, we don't have that in the routing system yet (see #1793520: Add access control mechanism for new router system for work on that). And we don't have something like entity_access() or access controllers in core (see #1696660: Add an entity access API for single entity access for that). I think I will just expose my own permissions right now ("read resourceXY over REST API", "create resourceXY over REST API" etc.) and check for that.
That is the issue that brought REST to core. It was not at all discussed further. No follow-up was created. In other words: very little scrutiny was applied to the REST module before it could be committed to Drupal 8. I'm certain such a patch would not have been able to land in the past few years (say, since 2014) — it looks like we were far more lax in 2012.
It's totally possible that access handling in general and permission handling in particular were discussed elsewhere, but if so, I could not find it (I looked at all these issues).
The only other issue that was remotely related is #1866908: Honor entity and field access control in REST Services. In there, two commits happened:
the first marked \Drupal\rest\Plugin\rest\resource\EntityResource::permissions()
's permissions as "restricted" (this method no longer exists), as a stopgap. The second commit removed that, and instead added proper entity access checking. But … it did in fact NOT remove the permissions for entities! It just kept them!
In other words, the only reason permissions still exist, at least for EntityResource
, is simply that this was overlooked. No follow-up issue was created, and thus it was simply forgotten. It makes sense to allow @RestResource
to opt in to having a permission, for example in the case of DbLogResource
. But it does not make sense to always have this, without even the ability to opt out!
We (which includes dawehner, Crell, I, and others) also discussed this at DrupalCon New Orleans and unanimously agreed that:
- it does not make sense for Drupal 8 to continue to have permissions enabled by default for
EntityResource
- we should not break BC
- we should therefore make sure that newly installed Drupal 8.2 sites don't need to enable these permissions for
EntityResource
, but existing Drupal 8 sites will continue to use that
This issue/patch therefore proposes:
- we keep permissions enabled by default for
@RestResource
plugins: this guarantees BC for all custom/contrib REST resources - we remove permissions from
EntityResource
in Drupal 8.2.x by default, but keep them enabled for existing Drupal installations - we document A) why we have permissions, B) how you can opt out from permissions for a particular REST resource
Remaining tasks
None.
User interface changes
None.
API changes
- The
EntityResource
@RestResource
opts out from generating the permissions thatResourceBase
generates by default, but only for new sites. - Other
@RestResource
s continue to generate permissions. This means all contrib/custom ones continue to work as they do today. - For details, see the change record: https://www.drupal.org/node/2733435.
Data model changes
New key in rest.settings
configuration: bc_entity_resource_permissions
. Upgrade path provided: rest_update_8203()
.
Comment | File | Size | Author |
---|---|---|---|
#79 | rest_permissions-2664780-79.patch | 32.88 KB | Wim Leers |
Comments
Comment #2
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedBecause not all resources are entities. In practice the bulk of them are, but REST module supports any arbitrary resource model you want to throw at it. Entities are the special case in that they have an extra access control layer of their own. But even then, you'd get a 403 error when you really want a 405 error.
Comment #3
Wim LeersSo then why don't we only add permissions for non-entity resources?
I.e. why don't we allow the
@RestResource
annotation to opt out by specifyingneeds_permissions = FALSE
?Comment #4
dawehnerWell, its a good question whether we want all users which can edit any kind of content to also change things via a REST api. Just imagine Drupal.org, do you want to update content using some REST api vs. on the website?
Comment #5
Wim LeersBut you first have to expose a certain REST resource in the first place. Per verb even.
On top of that, you must also use the authentication mechanism specified.
Comment #6
klausidifferent permissions per verb: you want to grant GET access to all users, but you only want POST access for a special group of API users. Users can still create nodes via HTML forms, but you might not want to allow that over the REST API - what dawehner said.
Comment #7
Wim LeersBut isn't that an edge case that should be addressed by contrib? Right now, actually doing a single REST request requires so much non-obvious, Drupal-y steps.
If you only want to grant POST access to a special group of API users… then implement entity access hooks! That's what they're here for.
Comment #9
dawehnerSo should we create a subissues just for entities to turn it off for them?
Comment #10
Wim LeersSounds good!
Comment #11
Wim LeersWow, look what I just found: @klausi at #1816354-1: Add a REST module, starting with DELETE:
That is the issue that brought REST to core. It was not at all discussed further. No follow-up was created. In other words: very little scrutiny was applied to the REST module before it could be committed to Drupal 8. I'm certain such a patch would not have been able to land in the past few years (say, since 2014) — it looks like we were far more lax in 2012.
It's totally possible that access handling in general and permission handling in particular were discussed elsewhere, but if so, I could not find it (I looked at all these issues).
The only other issue that was remotely related is #1866908: Honor entity and field access control in REST Services. In there, two commits happened:
the first marked
\Drupal\rest\Plugin\rest\resource\EntityResource::permissions()
's permissions as "restricted" (this method no longer exists), as a stopgap. The second commit removed that, and instead added proper entity access checking. But … it did in fact NOT remove the permissions for entities! It just kept them!In other words, the only reason permissions still exist, at least for
EntityResource
, is simply that this was overlooked. No follow-up issue was created, and thus it was simply forgotten. It makes sense to allow@RestResource
to opt in to having a permission, for example in the case ofDbLogResource
. But it does not make sense to always have this, without even the ability to opt out!We (which includes dawehner, Crell, I, and others) also discussed this at DrupalCon New Orleans and unanimously agreed that:
EntityResource
EntityResource
, but existing Drupal 8 sites will continue to use thatI therefore propose that :
@RestResource
plugins: this guarantees BC for all custom/contrib REST resourcesEntityResource
in Drupal 8.2.x by default, but keep them enabled for existing Drupal installationsAttached patch does all this.
Comment #12
dawehnerNice!
I'm wondering whether this could be also a post update ... I guess it doesn't really make a difference.
Its so weird that
permissions()
returns always all of them, but really not the problem of this issue.Well, we at least need some update test, don't we?
Comment #13
klausiWim +1. Once you enable a specific entity:* resource in rest.settings.yml you already want it to work because we have custom entity and field access handling.
I think the permissions are important for other resources, and the issue title tells me we are addressing this :)
It is a bit sad that we cannot remove the permissions for EntityResource completely, or can we? How can a site break if a permission is just gone?
Comment #14
Wim LeersThey may start to expose data that wasn't exposed before.
Comment #15
dawehnerOn one hand this is true, but on the other hand, these entities could be available via the normal HTML based interface already. In general though I agree, there is some level of risk involved here.
Comment #16
Wim LeersYep, there is some risk, and having this BC flag set by default for existing sites means we avoid all that risk.
Comment #17
klausiRight, it could be a security risk if people do custom stuff with EntityResource.
Can we avoid using \Drupal:: in ResourceBase? But changing the constructor of a plugin base class is probably a BC break :-(Accessing config with \Drupal should at least go into its own method getRestSettings() or similar.
We could create a new base class BetterResourceBase or EightTwoResourceBase, which is also weird. And that's why you should not write base classes for plugins and always use traits instead. ResourceTrait would have been the way to go.
Comment #18
klausiOh forget what I said, the change is in EntityResource anyway. We can override the constructor there and don't need to use \Drupal.
Comment #20
Wim Leers#12:
ResourceInterface
.#17:
Indeed!
#18: also true. Done!
Next reroll will address #12.3.
Comment #22
Wim LeersWow, this took ages to debug. The reason for the test failures in
PageCacheTest::testQueryParameterFormatRequests()
… is that the current patch allows the routes forEntityResource
to not have any requirements. And without requirements, no access checks. Without access checks, our routing system assumes you forgot to specify access checks, and hence helpfully denies access. That's what's happening.The fix is trivial, if somewhat confusing.
Comment #23
dawehnerWhen we
In an ideal world we would move this requirement building into its own subclass, so entityBase could simply override the behaviour. Here we just add more complexity to the already existing method.
Comment #24
Wim LeersThis removes the stopgap that #11 added to allow all
RESTTestBase
-based tests to still pass. In other words: it updates the tests to test the new reality. We have one test that verifies the old behavior still works.So, this fixes #12.3 because that code is now gone, but we still need upgrade path tests.
Finally, this will have a fail in
AuthTest
. Analysis for that to follow.Comment #26
Wim LeersAlso fails in
ResponseGeneratorTest
, but that's not something tricky, it's just something I overlooked. Fixing that first, then addressingAuthTest
.This should now have a single fail, in
AuthTest
.Comment #28
Wim Leers#24 had a fail in
AuthTest
, because:In the BC case,
AuthTest
will throw a 403 HTTP exception because the anonymous user does not have the permission, which\Drupal\Core\EventSubscriber\AuthenticationSubscriber::onExceptionSendChallenge()
catches, which in turn causes\Drupal\basic_auth\Authentication\Provider\BasicAuth::challengeException()
to be called, which converts that into a 401 exception.In the non-BC case,
AuthTest
will NOT throw a 403 HTTP exception because the anonymous user is not denied access, since the permission is no longer required. However, the anonymous user still cannot access the entity, becauseEntityResource::get()
calls$entity_access = $entity->access('view', NULL, TRUE);
, which calls\Drupal\entity_test\EntityTestAccessControlHandler::checkAccess()
, which denies access, which results inEntityResource::get()
throwing a 403 HTTP exception. Sounds like we're good? No, we're not! Because this time, the 403 exception does not bubble up to the HTTP kernel level, but is caught much earlier, inRequestHandler::handle()
: since #1865594: Better REST error messages in case of exceptions, we catch any HTTP exceptions there and generate a response with an error message. Which means we still send a 403 response, but not a 403 exception. Since there is no 403 HTTP exception, theBasicAuth
authentication provider (called by the HTTP kernel) also can't convert it into a 401 exception, which means … the anonymous user gets to access our response without being authenticated!!!There are
threetwo possible solutions:remove the "better error messages" stuff that #1865594: Better REST error messages in case of exceptions added, but then we lose our helpful error messages_entity_access: '<entity type>:<operation>'
route requirements, e.g._entity_access: 'node.view'
— this will result in the access checking happening beforeEntityResource::get()
is called. This was simply not considered in #1866908: Honor entity and field access control in REST Services because_entity_access
did not yet exist then: it was only added many months later, in #1947432: Add a generic EntityAccessCheck to replace entity_page_access().However… upon closer inspection, this causes failures for the REST routes that allow users to create entities:
_entity_access
only actually works for routes that have a route parameter that is that entity, which obviously can't work for POST routes. Patch attached for completeness, but expect failures.So, there are not two possible solutions, but just one: #2 in the list above. (Even though I would have very much preferred the third option that uses
_entity_access
, it feels the most elegant to me personally.) We have variants A and B to choose from, and variant B seems to be the most reliable one.P.S.: this is only somewhat related, but I want to point out that this seems like a very fragile design: we don't actually require authentication:
_auth
is a route option, not a route requirement. We only send a 401 if a 403 happens to occur, and that 403 is actually an unhandled exception: a 403 response won't be converted into a 401. This weakness was introduced in #1865594: Better REST error messages in case of exceptions.Comment #29
Wim LeersForgot the interdiff for
rest_permissions-2664780-28-3.patch
.Comment #30
Wim LeersThis also needs a change record. Created one: https://www.drupal.org/node/2733435.
Also updated https://www.drupal.org/developing/api/8/rest, changes: https://www.drupal.org/node/2667736/revisions/view/9386550/9713245.
Comment #31
Wim LeersSo, running with option #28.2.b. Added documentation.
Interdiff relative to #26.
#23:
I disagree. The whole point of doing this in
ResourceBase
rather than inEntityResource
is to allow any resource plugin that extendsResourceBase
to overridepermissions()
to not specify any permissions, and then have it work.The alternative is to specifically document
ResourceBase
to only be used for resources that need to have permissions generated, that don't have their own access control mechanism. But doing so would requireEntityResource
to no longer extendResourceBase
, which means it'd need to duplicate all of the complexity inResourceBase::routes()
.Given that
::permissions()
is in fact defined by theResourceInterface
, and thus every REST resource plugin must implement it, it seems more appropriate to letResourceBase::getBaseRoute()
be smart enough to deal with apermissions()
implementation that simply returns no permissions. And that's exactly what that code that you cited does.Comment #34
Wim LeersAfter fighting my way through many of the WTFs that one encounters when writing an upgrade test, here is what I believe to be a very solid update test.
IMHO this patch is ready.
Comment #35
Wim LeersUpdated IS to summarize the entire issue.
Comment #36
tedbow+1 for this issue
I do find the rest permission confusing.
Would it make sense after this block to have a have an else block that would
unset($requirements['_access']);
I know technically it would behave no differently but if I was developer debugging and altering this route in a subscriber I know I would be confused if I saw '_access' == true and '_permission' == 'restful update node'
The comment block explains why effectively it still works but if someone is doing an alter they are likely not going to find this comment.
Comment #37
tedbowCould we check to see if any permissions defined by any rest resources that extend EntityResource has actually been assigned to a role?
If not would it make sense not to set this flag?
Just thinking of a site admin who thought they were going to use rest but never ended up using it or hadn't start before 8.2. In that case when did start using rest they would be stuck with the old permissions behavior unless they updated via command line.
Of course they would also even have to know they were using a deprecated permission system. To update via the command line.
This would probably be more likely to happen to sites that installed rest shortly before 8.2.x came out.
Comment #38
Wim LeersThanks for the review!
#36: I think we should keep it as it is, precisely to make it the least dynamic possible. Every developer should learn that all access checks must grant access to be allowed access a route. Hopefully, if a developer sees both, they'll learn that. Furthermore, if they see those two requirements, then they can also see the route name. Given the route name, they will surely find their way back here. And then they'll find the comment documenting this. That's exactly why I have that comment in the first place: to make sure all developers implementing or debugging
@RestResource
plugins based onResourceBase
are explicitly aware of this.#37: Ohhh! That's very interesting :) But it also makes the BC-vs-new behavior far more difficult to understand: . Also, however much I like your very clever idea, I'm afraid we cannot actually rely on it: users with the
administrator
role are automatically granted every permission, yet theuser.role.administrator
configuration lists no permissions at all. So, it's perfectly possible that a site actually is relying on permission-based REST access, even if noEntityResource
permissions are listed explicitly!So, AFAICT your suggestion is:
is_admin
not listing which permissions they haveComment #39
dawehnerOh damnit I actually wanted to write submethod.
Comment #40
Wim LeersOh, a helper method. that is totally reasonable.
Comment #41
Wim LeersDone. Extracted that logic from
getBaseRoute()
intogetBaseRouteRequirements()
.Comment #43
Wim LeersHah, #2626298: REST module must cache only responses to GET requests just landed, causing this to fail. Rebased. One tiny conflict to resolve.
Comment #45
Wim Leers#2626298: REST module must cache only responses to GET requests also expanded some test coverage in a non-conflicting way, which explains the fail in #43. Fixed.
Comment #46
dawehnerHere is a question. Why does
\Drupal\rest\Plugin\rest\resource\EntityResource::getBaseRoute
not specify_entity_access: $type.view
. Wouldn't that allow us to not deal with it?Would it make more sense to catch AccessDeniedHttpExceptions separately?
Comment #47
Wim Leers#46:
No, that doesn't work. I tried! See #28, option 3 for the details + patch.
I'd love to! But REST has lots of
throw new HttpException(STATUSCODE, …)
, which means it's more likely than not that contrib plugins will do the same. Which means we can't 100% rely on catching that particular exception. If you think we can, that'd be fine for me.Comment #48
dawehnerRight, this is why we
_entity_create_access
. On the other hand I see what you mean. Arbitrary code could still execute so we want to have a special behaviour for it.Ah too bad, yeah nevermind.
Comment #49
Wim LeersExactly!
(Thanks for the pointer though, I should have found that myself.)
So … what's left here?
Comment #50
dawehnerWorks for me now
Comment #52
dawehnerha, this is this time an actual failing test ...
Comment #53
Wim LeersYep, one of the recent commits introduced another usage of this permission. I'll fix this.
Comment #54
Wim LeersConfirmed, #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it introduced another use of the permission that this patch now disables by default. Simply removing that permission should do the trick.
So, it does the trick in that it solves the fail we saw above, but … then we start to see new fails (i.e. this patch will fail), triggered by this hunk in the patch:
Those additions mean that a 403 exception no longer gets such an
{error: "ERROR MESSAGE"}
response. Which was fine so far, but #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it added test coverage for that. And so now it turns out this solution isn't good enough anymore.This is just yet another example for #2737751: Refactor REST routing to address its brittleness. :(
Comment #55
Wim LeersClearly, the real problem is that this catching of all exceptions in the controller and transforming it into a response is something that:
KernelEvents::EXCEPTION
event to address this\Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase
So, let's get rid of all that logic in
RequestHandler
and instead use that established pattern. That means we need a subclass ofHttpExceptionSubscriberBase
that can handle all serialization formats. The RTBC patch at #2403307: RPC endpoints for user authentication: log in, check login status, log out already has that, so we can import that from there (and when that lands, we can remove it from this patch).That's what this patch does.
Comment #56
dawehnerNice!
Comment #59
Wim LeersSo, we made progress in #54, but we're still far from where we need to be… there's still lots of fails :/
The first problem is that we're now getting error (HTTP 4xx) responses in HTML, not in JSON/HAL+JSON/… Why? Because none of the PATCH or POST tests actually specify
?_format=SOME_FORMAT
in the URL. Which means we don't really request any format, and hence\Drupal\Core\StackMiddleware\NegotiationMiddleware::getContentType()
defaults tohtml
.Possible solutions:
?_format=SOME_FORMAT
to all POST and PATCH requests.So, I went with solution 1.
After the above is solved, the second problem is that in HEAD, the error message lives in an
error
key, but as of #55, it lives in amessage
key. Is this a BC break? I don't think so: it's only intended for developers anyway. So I went with that for now.EDIT: Alternatively, we could change the new exception subscriber to put the message at
'error'
instead of at'message'
.And a third problem is that
HttpExceptionSubscriberBase
itself is poorly designed: it requires every HTTP status to have its own callback, and it's missing theon422()
callback by default. So, added thaton422()
method.Comment #60
dawehnerWell, it was designed to solve a specific problem, but there is no reason to not solve that: #2739617: Make it easier to write on4xx() exception subscribers
Given that something is wrong on an exception anyway I don't think this BC break is a real BC break.
Does that mean we commit this issue after the issue on #2662284: Return complete entity after successful PATCH and call it a day?
Comment #61
Wim LeersThanks for opening #2739617: Make it easier to write on4xx() exception subscribers!
Yes! Committing this patch after that one will make this patch a fair bit simpler. I just went with option 1 for now, to prove that this patch can in fact be green.
And in fact, if #2403307: RPC endpoints for user authentication: log in, check login status, log out lands first, we'll also be able to remove most of what #55 changed.
So, let's postpone this on both #2662284 and #2403307.
Comment #62
Wim Leers#2662284: Return complete entity after successful PATCH landed, so one less blocker!
Comment #63
dawehnerIt feels weird to block on a conceptual different issue, just because it shares some code. Maybe I don't get it, but this one shared class feels not like a big deal when a potential reroll needs to happen.
Comment #64
Wim LeersI agree. But that's due to the unfortunate state that the
rest.module
codebase is in. The fix in #55 (see the interdiff) is about 20% of the entire patch size. There's been many more eyeballs on #2403307: RPC endpoints for user authentication: log in, check login status, log out, so I'd rather see that hunk landing there than here.Furthermore, #2403307: RPC endpoints for user authentication: log in, check login status, log out has been ready for almost 2 months at this point, so I expected it to be committed a long time ago. Hopefully any day now.
Comment #65
Wim LeersThe changes made in #55 actually aren't being introduced in #2403307: RPC endpoints for user authentication: log in, check login status, log out anymore, they already were introduced in #2308745: Remove rest.settings.yml, use rest_resource config entities! So, no need for this to be postponed anymore.
Rerolling…
Comment #66
Wim Leersrest_update_8201()
now exists already, so renamed the update function in this patch torest_update_8203()
(this is what the interdiff shows). Updated the patch + IS accordingly. The CR is still accurate.Comment #67
Wim LeersAfter #59 was rolled, #2308745: Remove rest.settings.yml, use rest_resource config entities landed, which added a
administer rest resources
permission. Hence the assumption that was made in this issue/patch that\Drupal\rest\Tests\Update\EntityResourcePermissionsUpdateTest
makes about REST module having no permissions by default is no longer true.This reroll/interdiff makes the minor changes necessary to no longer have that assumption, so this patch should come back green.
(I also noticed that the test method name was silly/wrong, it was a copy/paste remnant. Fixed that too.)
Comment #69
tedbowLooking at #67 interdiff. It handles the new default permission for REST, ''administer rest resources'" correctly in the test. That was the only test fail in #66
Comment #70
tedbowok just went through comments in #51 and it looks like we are back to RTBC.
Comment #72
tedbowHere is just automatic re-roll no changes. If tests pass I think RTBC
Comment #73
tedbowRTBC b/c patch not changed since #70
Comment #74
alexpottDiscussed with @catch, @effulgentsia, and @cottser we agreed that this needs to be done by the time 8.2.x beta is cut - but it is also an important patch because the last piece of a jigsaw that we want to complete in 8.2.0.
Comment #75
Wim LeersNice way of putting it! :)
Comment #77
Wim LeersConflicted with #2403307: RPC endpoints for user authentication: log in, check login status, log out, trivial resolution.
(Rebased from #67, because #72 lost the upgrade path test.)
Comment #78
alexpottOne of my major concern's is that everything probably works great right now but with minimum of test coverage future us's might break this.
Comment #79
Wim LeersThis fixes that.
Actually, we do have test coverage for that, in
\Drupal\rest\Tests\Update\EntityResourcePermissionsUpdateTest::testBcEntityResourcePermissionSettingAdded()
:So, AFAICT we were really just missing that one bit?
Comment #80
xjm(Retitling for grammar nitpicking by @prestonso and myself. Carry on.)
Comment #81
alexpottLess permissions - simpler. Nice. Committed 85d755a and pushed to 8.2.x. Thanks!
Unused uses fixed on commit.
Comment #83
Wim LeersWoot!
Comment #85
dawehnerNice! Nice work @Wim Leers and @tedbow
Comment #86
Wim LeersThanks :)
Comment #87
tedbowThanks!
Comment #88
xjmIn before the bell, so untagging. Yay!
Comment #89
xjmAlso.