Closed (outdated)
Project:
Drupal core
Version:
8.5.x-dev
Component:
rest.module
Priority:
Major
Category:
Plan
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
15 Apr 2017 at 10:55 UTC
Updated:
10 Nov 2017 at 15:43 UTC
Jump to comment: Most recent
Comments
Comment #2
alexpottComment #3
alexpottLinking some other committed issues that made access changes under test coverage auspices.
Comment #4
Anonymous (not verified) commentedIn my worldview, the files are part of the content, hence when the user does not have
'access content'this should apply to the files too. But I'm also not against the appearance of new specialized permission like"access files". And of course, I'm absolutely ready to admit my mistakes after listening to other opinions.Comment #5
dawehnerThe file module already uses this specific permission during the file upload:
Comment #6
wim leersThe problem is that many (most) config entity types do not specify an access control handler, nor an
admin_permission.admin_permissionautomatically use the default entity access control handler (\Drupal\Core\Entity\EntityAccessControlHandler). For checking access, they do this:and:
The latter is only for creating entities of that type. The former is for all other operations.
admin_permissionfor every entity typeadmin_permission. If they don't do that, then no operations on these entities are permitted at all.Conclusion: specifying an
admin_permissionis a necessity for virtually all of them.viewoperationadmin_permissionis almost never sufficient. Most if not all config entity types are safe to be viewed by many users. There's often only risk in allowing users to modify entities.A clear example: the
Vocabularyconfig entity. Without the ability to view these, decoupled applications cannot retrieve the label or description of a vocabulary. This is a problem.The only solution (at least right now, and this has been the case since 8.0.0) is to give them the
'administer taxonomy'permission. But this permission then also allows them to modify and delete vocabularies (and terms!). That's far too much!In other words: this is a clear example for the need to have a separate
viewpermission.'access content'permission.'access content'permission is already being used to grant access tonodes,node_types andterms. Besides those entity access control handlers, it's used for many routes across many modules. Many of them notnode-related at all. Quoting myself from #2808217-27: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission, from December 1, 2016 (>4 months ago):IOW:
'access content'already is the "bare minimum permission you need to access pretty much anything on a Drupal site". So… let's continue to apply that elsewhere too: non-sensitive configuration entities such asvocabularyentities would then be easily made accessible.Comment #7
wim leers#3:
All of these are only making minor adjustments to existing entity access control handlers. They're adding support for the
viewoperation — the existing code only supported other operations.Comment #8
wim leersAlso, I forgot to make one very important point in #6.
The reason this went unnoticed all this time is rather simple: Drupal 8 is not API-first, largely because our Entity/Field API has some questionable design decisions.
We're creating config entities in forms (in their submit callbacks). We're not validating those config entities. We're not checking access to those config entities. We're only checking access to the form. Hence for config entities, we are bypassing our validation and access APIs.
This works … but as soon as you want to be able to perform those same tasks via a REST API (or GraphQL or JSON API or something else), you're then forced to finally define the hitherto undefined access control. (And for validation we have #2300677: JSON:API POST/PATCH support for fully validatable config entities.)
Comment #9
wim leersI bumped this to "major" in #6 because this is blocking 7 issues that are effectively RTBC, but they're all implementing a change that we need to agree on in this plan issue. Alternatively, this plan issue must specify a different approach.
Comment #10
wim leersThis is actually the appropriate state.
Comment #11
dawehnerWell, the admin permission seems to be more of a shortcut ... it feels better to define a custom access control handler on the longrun, given that people want specific permissions anyway.
Comment #12
wim leers#11: Agreed that you want fine-grained, well-thought-through access control handlers for every entity type … but an
admin_permissionis still useful for the coarse "be able to do anything with this entity type", which is also a valid need.Comment #13
dawehnerThat's fair, well to be honest I totally agree with your for core for now. The problem is once we add this admin permission we need to carry it around potentially, so let's better think about it before we add it.
Comment #14
wim leersWhat do you mean? It's up to every entity type individually to specify an
admin_permission. So the impact is always limited to that one entity type.Comment #15
dawehnerRight. Let's assume though we adapt the entity type in the future to have more granular permissions. In that case we still need to provide support admin_permission.
Comment #16
wim leersTrue. But for most of these entity types, there already is a module-level administrative permission!
I agree though we need to consider that long-term cost. Do you see an alternative approach to avoid that?
Comment #17
dawehnerNow that I think about it, config entities, unlike content entities, are created/changed by the same group of people (site builders).
These people use an admin role anyway, from my point of view, so given that, I think it is actually fine to add administrative permissions to every of those entity types.
What are your thoughts about that?
Comment #18
wim leersMy thoughts exactly! :)
Comment #19
wim leersJust discussed this with @catch at DrupalCon Baltimore 2017.
Outcome:
access contentis the sensible permission to use for this; because it is already being used outside the node module anyway.viewconfig entities with just theaccess contentpermission, but he agrees that doing so is a step forward compared to today (because today we grant permission always, no permissions needed). So he wants to see a major follow-up tagged to review that.I think that the next step is for Alex Pott to agree or disagree. Then:
Comment #20
alexpottI'm okay we the decision - I have a mental dissonance with
access contentgranting access to view configuration - but having this controlled by a permission definitely makes sense. I'm right in thinking that you can configure more granular permissions on the rest.resource config entity if you want?Comment #21
wim leersYep, it's not controlled by any permission right now.
No; you only get the extra permissions if you enable the BC layer that #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources left in place. What do you mean exactly when you say ? I could see it be valuable to require the
administer viewsto just view aview(heh), but most others seem 100% harmless to expose to just about anybody:DateFormat,RdfMapping,Menu, and so on.So, can you please clarify?
Comment #22
dawehnerFor me viewing a config entity as a thing is tricky outside of REST.
Comment #23
alexpott@dawehner yep that's why this is such an odd issue. Thinking about this some more I really think that viewing raw configuration should be a separate permission from access content. Taking the view example you might not want to expose your business configuration to anyone that can access content on your site. Whilst there might no harm there certainly is disclosure and you might view your view as intellectual property that you only want certain users to be be able to access.
Comment #24
wim leersBut that's not true for
Vocabularyconfig entities, which you need to be able to view to be able to know the name of the vocabulary. This is the #1 use case.Comment #25
berdirNot sure if this was mentioned already or not..
The reason node type is controlled by access content is that this used to be the only way to be able to see the node type label on /admin/content if users didn't have administer node types.
It's now possible to do this more fine-grained with the view label operation. See #2692091: Use the new 'view label' entity access check in the entity reference label formatter (specifically comment #38), #2471154: Anonymous user label can't be viewed and auth user labels are only accessible with 'access user profiles' permission and #2561331: When 'administer content types' permission is disabled, content type field is hidden in content manager.
So we could change node type now to only allow label access with access content, not sure if that's an API/something change or not?
Comment #26
dawehnerThere is a bit of a strong point here ... especially you don't want to expose some configuration entities suddenly after an update.
Comment #27
amateescu commentedFWIW, I agree with #23, #25 and #26, so I think the answer to the current issue title is pretty much 'nope' :)
Comment #28
wim leers#27 but that means we make a very strong BC-incompatible change: e.g. the
Viewconfig entity currently requires zero permissions. Requiringaccess contentis a non-BC breaking solution for that.And again:
This then needs a different answer.
To be clear: I'm perfectly happy to accept "require the
admin_permissionfor any operation on any config entity" to be the default answer. But for e.g. reading a Vocabulary config entity, we need something else, because for that config entity type, viewing the config entity is a hard requirement: we need thename, thedescriptionand thehierarchyfor building decoupled apps. Viewing those is harmless, but having all CRUD permissions (which is what granting itsadmin_permissionwould result in) is a no-go.Therefore: I'm fine with not going with
access contentas the general rule. So that e.g.viewconfig entities will require the corresponding admin permission. But then what about harmless things that are important for the client side, such asVocabulary,DateFormatandMenu?Comment #29
dawehnerI'd argue its breaking some BC in regards to access right now. Why can't we have a 'view config' permission instead?
Comment #30
wim leersI disagree, because how many sites in practice do not have the
access contentpermission granted to every role? Less than one in a thousand, I bet — probably even less than 1 in 10K.Adding a new
view configpermission would surely be more disruptive? Also, a singleview configpermission for all config entities would be quite dangerous, because there's a huge difference in security sensitivity between being able to view a Vocabulary config entity vs a View or RestResourceConfig config entity?That being said: I'd personally like that for sure.
But more importantly, you did not reply to my proposal below the
<hr>in #28. I'm wondering if you have thoughts about that?Comment #31
dawehnerMaybe I understood wrong what you've been saying. I thought you want to actually use 'access content' for all configuration entities.
I think we have a conflict of
a) Denying people access, when they used to have access
b) Allowing people to gain access, when they used to not have access
I really lean towards a), while your arguments seems to move towards b) more.
At least for content entities most of them have a specific permission right? Given that I think we should have a look how a better build system looks like. In this world I would have expected permissions to view specific entities. We can ship in standard install with permissions for the anonymous use to access vocabulary/menu/display format, but maybe not the other entity types?
I hope its okay what I write in the following paragraph ...
Total unrelated, you seem to add more and more visual glutter to structure your comments. Maybe its just me, but it makes it actually harder to actually be able to read them in a reasonable amount of time. If you read a long text, visual markers are kinda like constant distractions for your eye.
Comment #32
wim leersI'm fine with that actually (because more secure), but I was concerned about the BC break that that implies. I agree that the BC break is mostly theoretical though. So if you're fine with that, then I'm too.
+1
So then, to bring this all together in a concrete proposal again:
admin_permission(RdfMapping,Tour): add anadmin_permission.View,Menu,File,DateFormat): change them to require the entity type'sadmin_permissionor the entity-type specific "view" permission (yet to be added), so for example:AccessResult::allowedIfHasPermissions($account, ['view menu', 'administer menu'], 'OR').P.S.: I use
<hr>s to separate either thoughts, or replies to different comments. I get super confused when people reply to many things, or have many distinct thoughts, and put it all in one super long comment, with some paragraphs belong together, and some not. I guess we process/read information differently :) No offense taken in any case!Comment #33
dawehner+1 to your summary. I think when we introduce it, we should certainly back it up by having REST test coverage for them. This reduces any potential risk around it.
Sure, I just wanted to write what I think :)
Comment #34
alexpott#32 sounds like a great - we can alway relax the permissions later if we choose to for specific config entities.
Comment #35
wim leersHURRAY! We have consensus!
So the consensus is:
Now updating the 7 issues in the IS to comply with this consensus.
Comment #36
wim leersUpdated IS to reflect current state.
Comment #37
wim leersRerolled all of them:
Comment #38
Anonymous (not verified) commentedGreat news! Thanks to everyone for promoting this problem, and special thanks to @Wim Leers!
Comment #39
berdirHm, sorry for being late to the party, but I'm not quite sure that we've discussed everything here.
So, first a question:
What exactly is the purpose/use case of allowing REST access to config entities? Who is going to need it, to do what?
The conclusion here is that we basically require administrative permissions for anything on many/most config entities, even viewing them. But as far as I see, that's a problem for a number of things:
* I looked quickly at the Menu patch, it actually changes the access control handler, what if code relied on that? Conceptually, I'd say that the menu block should check view access for a menu, because then you could actually *do* something with it, like denying access to a whole menu für some users. another access example that affects many config entity is when combined with entity references, because those check view access. As mentioned in other issues, that's for example why NodeType and Vocabulary config entities allow view access with access content. We now have the view label access operation, so we could say that we at least still allow access to that for those types, but again, I'm not sure if that is an behavior change. what if people have use cases that involve referencing a menu on a node, for example to show it inside the node. then this will break with those patches as users will no longer be able to see the menu labels.
* I assume a common case for REST access for config entities is decoupled applications, so that they're capable of displaying the node type label for example, or do some rendering based on its settings. Or format a date, or show tours, or add rdf mappings. But that's impossible then unless you have an admin user, so that's obviously not a solution. But at the same time, as pointed out by myself as well, allowing full access to those config entities is also not an option, with third party settings and so, who knows how much information is in there. But how are we going to solve this use case then? should we offer certain API's as services instead of just exposing data? Is this something that sites need to do themself, with exactly the data they need? Should we for example expand content entity serialization to include things like labels of referenced (config) entities?
Yes, these questions go beyond the actual question of this issue, but I'm wondering if we solved the issue without actually solving the problem behind it?
Comment #40
wim leersI agree we should have fine-grained "view" access permissions rather than just requiring "admin" permissions. But requiring "admin" permissions is just a first step. We will be able to evolve this in the future: also allow "view" access if the user has some other permission, or if they can access some other thing. What we need to fix today is: a) unregulated access (granting access to all without any condition), b) not having access control at all (because no access control handler and no admin permission).
Comment #41
catchSorry I'm re-opening this partly due to Berdir's comment, partly due to #2843768-21: EntityResource: Provide comprehensive test coverage for Tour entity.
If we add an administer permission for every config entity type that doesn't have a user interface, then that's several permissions in the UI that are a no-op unless REST is enabled. To move forward quickly without any UI changes, could we use 'administer site configuration' to start with, then add more permissions later on when we're more sure we need them?
Comment #42
wim leers#41: hah, I proposed exactly that in #2843768-22: EntityResource: Provide comprehensive test coverage for Tour entity! So let's move forward with that. IS updated.
Comment #43
wim leersAt #2843772-20: EntityResource: Provide comprehensive test coverage for DateFormat entity, @Berdir raised this:
(emphasis mine)
To which I replied in #2843772-22: EntityResource: Provide comprehensive test coverage for DateFormat entity:
I think this makes sense. I see that Berdir mentioned this before in #39, but we overlooked/forgot that. Updated conclusion in IS with another addendum.
Comment #44
alexpottI like the updated direction in #43.
Comment #45
wim leersGreat :) Hopefully Berdir, catch and dawehner also agree.
Comment #46
dawehnerI agree with the new direction in #43 as well.
Comment #47
berdiradminister site permission: Works for me to start with that.
view label operation: I was the one who suggested that, so I can't really disagree there :) Your patch is not enough yet however, see https://www.drupal.org/node/2661092, you need to opt-in for that operation. Should we add test coverage for that? To counter your next argument, yes it is a REST test patch, but what you are changing is not rest specific :)
I don't think those two things answer all of my questions, though? If an admin permission is just a first step, fine, but we should probably at least have some kind of plan/issue to discuss what the actual use cases are for reading this data (e.g. decoupled sites) and figure out how to get there? I kind of like my idea above to enrich the content entity serializers with labels and things that might be useful for decoupled sites. Possibly based on some kind of context.
Comment #48
wim leersRE: view label operation needing
protected $viewLabelOperation = TRUE;in the entity access control handler. /me throws up hands in despair.RE: OMG… you're right of course, except the maddening and frustrating truth is that the answer should be expand test coverage, but of course, zero test coverage exists for most of these access control handlers. Definitely for
DateFormatAccessControlHandler.I won't deny it: this is extremely frustrating. This slows down progress in the API-First Initiative enormously. The API-First Initiative has to fix massive oversight after massive oversight in the Entity/Field API. In fact, the majority of the work has been outside the API-First Initiative's modules for a very long time. The majority of the work is not in the
rest,serializationandhalmodules, but in theentity system,request processing system,routing systemcomponents, plus in every module that provides an entity type.Of course, making an API-First Drupal a reality, those are exactly the things we must mature/stabilize/add test coverage for. But it's such a monotonous, mind-numbing, hard-to-find-reviewers-for, thankless, slow, frustrating, endless, boring job that I wish oh so very much that we didn't have quite as many oversights.
See #2843772-25: EntityResource: Provide comprehensive test coverage for DateFormat entity for what I then guess must be the first of many first rounds of
The actual use case is exactly what you say: making it possible at all to build decoupled sites that interact with configuration entities.
Yes, we'll need #2300677: JSON:API POST/PATCH support for fully validatable config entities for that too. But ensuring access control behaves as expected, and having the ability to prevent viewing certain configuration entities must happen first.
Without having actual CRUD capabilities for config entities, nobody is ever going to build ambitious decoupled UIs that talk to Drupal sites. Because it is literally impossible. There will still be bugs after all this, but right now it's not remotely possible, and it requires a titanic effort to get it to a "mostly-working" state. Which is exactly what I've been doing for the past year with a few other people. And as this issue demonstrates, there are still lots of basics completely wrong/missing.
?!? I have no idea what this is referring to, and it sounds very vague. So I tried to find what comment of yours you were referring to, and it seems to be this part of #39:
Well that's the thing, the consensus in this issue is to provide non-admin permissions to view these config entities, and admin permissions to modify them. If you're allowed to view a config entity, then yes, you're also allowed to view third-party settings. Surely there's no harm in that? If some third party settings require their own access control, then they shouldn't be third party settings, then they should have their own config entities (so they can have their own access control)! In the third party settings you could then still refer to these associated config entities by config entity ID, which is once again harmless.
If that's not really what you were getting at, and you were actually implying that it's impossible to strictly/sensibly validate third party settings, because there exists no proper validation system for config entities in the first place, then you're right, that's another massive, painful oversight that we need to fix now… For which I again point you to #2300677: JSON:API POST/PATCH support for fully validatable config entities.
P.S.: none of this is directed at you, Berdir, but at the overall lamentable state of things from an API-First point of view. It's unfortunately a titanic effort, if not a sisyphean effort. Once software has shipped, and BC must be maintained, it's extremely difficult to make something that's API-second actually API-first without breaking things. It's absolutely understandable why we're in the state we're in. But we need to get much better at determining/communicating what is actual supported API and what is not, because this is very much death by a thousand cuts: unmaintainability by a thousand oversights.
P.P.S.: part of the reason for writing this high-level rant is that this issue finally got to consensus after a month, but as of #47 it seems the scope of this is ever-expanding. And this after half a dozen issues were already blocked on feedback for weeks, which is how this issue was opened in the first place. And the reason those half dozen issues (out of a total of >30, see #2824572: Write EntityResourceTestBase subclasses for every other entity type.) exist in the first place, is that neither the REST module nor the entity system had the necessary test coverage in place to project sufficient trust, because anybody using D8 REST a year ago was running into weird problem after weird problem. At this fixing pace, it will take at least another year.
Comment #49
dawehnerI think we should even think about it more broadly. We want to be API first, and not API second. This sadly involves all the pain points @Wim Leers mentions in his previous post. I think the only way to resolve that is to ensure that at least the other side of the world is API first for every new thing, rather than making it an afterthought (random example: how do we post media entities).
Well, to be fair its not impossible as we speak. JSONAPI exposes config entities in its full variety already, and well, you can always easily swap out things, when you actually need it. You will never access every config entity, but rather you build UIs specific per bit, which means that you will have code for each configuration already.
As far as I understand this statement we are talking about exposing more information to the normalizers than the pure stored data.
label()is one of those examples,toUrl()might be another one. They contain information which are derived from the stored values, but still are distinct between different entity types.Comment #50
berdirYes, what I mean for example is that reference fields would include the label of the referenced entity, so you get things like term labels but also author name, node type labels, vocabulary labels and so on diretly as part of the main entity you're requesting, e.g. when you specifiy ?_reference_labels or something.
More on the other things later.
Comment #51
wim leers#49:
Thanks for acknowledging the pain.
YES YES YES to this. Not to mention normalizers etc.
Adding new things to core == integrating it with all the things. Not providing full normalization support means not providing full REST/JSON API/… support. Which means that this increases the burden on the API-First Initiative contributors.
… but because of that, huge security caveats are attached to that.
#49 RE:
label()andtoUrl(): it still isn't very clear to me I'm afraid. Are you suggesting "view modes" for normalizations? i.e. a "label normalization" for an entity, and a "URL normalization" for an entity?#50: Oh, I think you're saying "ability to normalize not just references by ID, but also already include some fields from the references, such as label and URL". I think I agree with that :) But I am also certain that's vastly out of scope here. It's only tangentially related in that "normalize labels" should use the
view labelaccess operation.But let's not forget that this is completely useless for many use cases. For example: vocabularies. You could argue you only really need the label. Well, that's not true: you also need the
hierarchyanddescriptionfor most decoupled applications!Finally: JSON API already kinda solves this for the most part, thanks to
?includeand?fieldssee http://jsonapi.org/format/#fetching-sparse-fieldsets + https://www.drupal.org/docs/8/modules/json-api/fetching-resources-get.Comment #52
dawehnerDoes that mean we should propose a new core gateway, which is called M
API first??Well, no, but at the moment there is for example no quay to get the label of an entity via rest.
Comment #54
dagmarI created #2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes few weeks ago. I would like to see some input there, seems related to this topic.
Comment #55
wim leersIn the mean time, we've worked out how to deal with every individual entity type, without falling back to using
access content.Plus, #2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes landed, which helps the last remaining core entity type without REST test coverage:
File, at #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler.We don't need this issue anymore.
Thanks, all!