Problem/Motivation
Many sites are using the JSON:API Extras module to refine the API that JSON:API exposes by default. One such refinement is the ability to "disable" resource types. To do that, JSON:API Extras is forced to depend on potentially unstable @internal
PHP code. Also, custom module developers aren't able to write logic that could automatically disable/enable resource types according to rules that suit their application (like a whitelist or "all content entity types"). Without a way to create whitelists or logic in code, every time a new bundle is created, a site builder must go disable the associated resource type as an extra step.
Proposed resolution
Add a public, stable PHP API in the form of an event subscriber so that JSON:API Extras and other modules can write logic to enable/disable resource types.
User interface changes
None.
API changes
There will be a new event to which a module can subscribe.
Data model changes
None.
Release notes snippet
This release contains a security hardening feature to better support sites using the JSON:API module. Modules may now indicate that certain resources and fields representing their entity data should never be available over JSON:API. This can be implemented in a custom module for a specific site, or by a module that supplies an entity type. It is recommended to implement this API for any entity data that should never be exposed over JSON:API. See the JSON:API security considerations handbook page for more information on securing your site with JSON:API.
Comment | File | Size | Author |
---|---|---|---|
#58 | 3037039-58.patch | 11.51 KB | gabesullice |
#58 | 3037039-58.patch | 11.51 KB | gabesullice |
#58 | interdiff.txt | 3.06 KB | gabesullice |
#52 | 3037039-52.patch | 10.95 KB | gabesullice |
#50 | 3037039-50.patch | 19.81 KB | rajanvalecha12 |
Comments
Comment #2
gabesulliceArchitecturally, I think the soundest place for this is really in the Entity API, not JSON:API. JSON:API does not define a data model. It only exposes it.
JSON:API's ResourceType concept is, for the most part, just an adapter to bridge the divide between Drupalisms and JSON:APIisms. IOW, it provides a layer of indirection between Drupal's entity, field and typed data APIs to keep our code cleaner than it would otherwise have to be. They give us a way to shim buggy or missing behavior while things are fixed/created "upstream".
A few examples:
ResourceType::isLocatable()
is just a shorthand for "is the underlying entity type backed storage?" E.g. The message entity type can bePOST
ed to but notGET
ed). This shorthand keeps things clean when we define routes or check if it's appropriate to add aself
hyperlink to the response.ResourceType::isVersionable()
, is a shorthand for "is this entity type revisionable." But we've also used it to hardcode and limit it tonode
andmedia
entities, since the necessary revision access logic isn't complete enough to feel safe about any other entity types, even though they implementRevisionableInterface
.In one instance, where JSON:API Extras overrides on e of these values, it's to do something that we consider too risky for JSON:API. That is, as of JSON:API 2.0,
ResourceType::isMutable()
is FALSE, for all config entity types, since Entity::validate() is not available for them. Extras overrides this for applications willing to accept the risk of unvalidated config states in order to accomplish their business needs.What ResourceTypes have never been, is a facility for creating a divergent data model from the one already defined through Drupal's entity types/bundles. They've always been more of a lens through which we "view" the real, underlying model. This has prevented us from being blocked by hairy, slow moving entity/field API shortcomings and missing features. The goal has always been to work toward a 1:1 mapping with the underlying data model, not to override it or create our own concepts. We've just used it to "fill the gaps".
So! What does that mean?
It means that ideally, "no" we would not have a public API for making ResourceTypes internal. The Entity API would.
And, it already does! See
hook_entity_type_alter
.So, why then, does Extras not use this API? Because, AFAIK, these can't be altered with per bundle granularity. IMO, that's the missing piece.
If Drupal would simply have a typed data definition that's granularized per-bundle and that was alterable/configurable, the API would already be entirely sufficient for JSON:API and our users.
Finally, this would actually provide a better site builder UX than we have today, because Drupal could add a checkbox for this setting on the bundle config page:
/admin/structure/types/manage/article
That'd look something like this:
Ideally, our users wouldn't have to configure their data model in two completely separate places.
Comment #3
Wim LeersThanks for writing #2, @gabesullice! Very clear explanation :)
Comment #4
e0ipsoThanks @gabesullice. That's a very detailed summary of things.
One clarification:
Where it says it's often to do it should say there is one instance where we do. Unless I'm misremembering the example you provide is the only risky one.
One comment:
I am not +1 with the UI's proposal. That would imply that there is an existing UI for all entity type bundles. Since building a UI is not a requirement for creating a new bundle, I'd say that's not the case. Do you have anything in mind for those cases?
Comment #5
gabesulliceUpdated my comment. You're right, I (unintentially) misrepresented that.
I don't think it implies that—my mock-up was not meant to be applied to every case. It was meant to illustrate that further baking 'internal' into the data modelling cake could be more intuitive and beneficial for users.
I don't think we can or should make every entity type/bundle user-configurable. There are lots of entity types that should be marked internal in module code. Like image styles and views, which make little sense to be exposed via any web services. However, when/if they do need to be exposed, it's most likely going to be for some tightly coupled admin UI, which can be expected to use a hook to override the default in code.
OTOH, comments probably make sense to be public by default. However, when a site builder creates a new comment type, perhaps one that's meant to only be seen by content editor's, that user should be able to make a decision on the screen right after the comment type is created.
The point being that it's the author of the data type's responsibility to both choose a sensible default and to decide if that default makes sense as a configurable option.
In general, the guiding principle I'm trying to follow is that JSON:API does not define a data model. It only exposes it.
So, if the architect of the data type decided that the data type should not be configurable, it's not our place to override that decision.
Comment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI just wrote the same argument in #3035979-54: [DISCUSSION] Should a facility be added to JSON:API which limits automatic exposure of all entities, but for individual fields instead of comment types :)
Comment #7
xjmAh yep, I also just commented to the same effect as #6.
I'll c/p my comment from the other issue:
This seems an interesting approach to me, although I might call the resources something other than "internal". One other difficulty is that there are entity types and bundles that don't have a page in the core UI like the node article in the screenshot does. A central configuration page OTOH could display all those random things we've discussed above on the thread.
Comment #8
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think we have 4 cases to consider:
Core only has the first and last. There are 0 core entity types defined in config and 0 core bundles that are coming from somewhere other than a config entity (which gets mapped to the bundle via the
bundle_entity_type
annotation).That's why in #3035979-93: [DISCUSSION] Should a facility be added to JSON:API which limits automatic exposure of all entities I'm proposing a centralized config page for just entity types (to handle case 1), and not bundles.
For bundles defined in config (case 4), I agree with #2 about the benefit of colocating the "Web services" setting with the place where the bundle is created.
Case 2 (entity types defined in config) is rare. I don't know of any module that lets you do it other than ECK and that one is not yet stable.
Case 3 (bundles defined in some way other than via the
bundle_entity_type
config entities) probably exists in contrib. Anyone know of any examples? Looking at some specific examples might help inform what the best UI for those would be.Comment #9
xjm(Retitling to clarify and because there is not agreement that "internal" is the right description. Also embedding the mockup.)
Comment #10
larowlanI'm happy to work on this in the next fortnight.
We discussed it on the committers call and felt like it was possible, but we'd have to do the translation/workflow style approach where we had a single form with bundle checkboxes instead of making it on each node-type, because obviously not all entity-types have bundle edit forms.
Comment #11
webchickBefore continuing down that road, which then passes burden for knowing how to create a secure HTTP API up to less-technical site builders (vs. developers) and forces us to rush in a UI that could very well cause people to accidentally break their decoupled sites...
Can we please double-check that #3035979-98: [DISCUSSION] Should a facility be added to JSON:API which limits automatic exposure of all entities will not work as a basic mitigation strategy to get us through 8.7, with this issue coming to later enhance it?
Comment #12
e0ipsoI believe that this should be part of a broader effort that puts the API under the site builders scope. Maybe an api_first_ui module to do UI alterations. Among these alterations we would add configuration screens like the ones in this issue, but also warning about breaking HTTP API BC if a field is deleted, or made required, etc.
Comment #13
xjmRe-parenting to the HTTP API security hardening meta issue specifically.
Comment #14
gabesulliceQuoting myself from #2:
That would mean we could mark data as 'internal' on the bundle level. That would make a JSON: API-specific solution redundant.
I'd like to propose we close this issue (or rescope it) to have an issue titled "Allow entity+bundle TypedData definitions to be marked 'internal'"
I'm aware of the Content Moderation confusion with internal on entity type annotations, but I think that's something we should probably clean up in the Content Moderation module.
Comment #15
e0ipsoI'm +1 with #14. An API-First Drupal should define API interactions (aside from implicit ones) with the same API and UI tools used in content modelling.
Comment #16
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWe already have
EntityTypeBundleInfo::getBundleInfo()
which invokes hooks and alters. Converting this from an array to an object might have lots of benefits, but doing it in a BC way might be tricky. Even without converting it to an object, we could add'internal'
as a new key and document that inhook_entity_bundle_info()
.Note that even though
EntityType
is an object, it's not aTypedData
object, just as entities themselves are also not typed data objects. However, we have anEntityAdapter
for when we want to treat entities as typed data, so perhaps we could addEntityTypeAdapter
andEntityBundleAdapter
as well, the latter giving us a way to work with theEntityTypeBundleInfo::getBundleInfo()
array as a typed data object.Finally, we also have
ConfigEntityBundleBase
as a base class for config entities that act as a bundles of other entity types (like NodeType entities defining the bundles of Node). Maybe we want to add something there as well? Though that only covers #8.4, not #8.3.Yeah, maybe it's worth doing that. However,
internal
doesn't give us the granularity of read vs write. Would it make sense to have two new keys:api_readable
andapi_writable
? Not sure if that's valuable on a bundle level, but at EntityType and FieldType levels, I could see it being useful. For example, if an entity type or field type maintainer is confident about the accuracy of their access control handler for the 'view' operation, but isn't confident about the completeness/security of their validation constraints, might that be a good use-case for settingapi_readable
without settingapi_writable
? Or would we prefer to push for the single key ofinternal
and try to claim it back from Content Moderation and Workspaces?Comment #17
gabesulliceThe API-First angel (devil?) on my shoulder is saying "no".
I think we're mixing two concepts together. There's:
There are entity types like
menu_link_content
andimage_style
config entities which I'd argue fall into the first bucket (as long as there isn't a decoupled admin UI, at least. Also, I'm becoming more convinced that JSON-RPC is the way to go for a lot of decoupled config type things (like adding a new field to a bundle) rather than jamming that into a RESTful pattern). Then there are types in contrib which aren't prepared for the API world but make sense to be exposed. Metatag comes to mind here (because of how it uses a field to store overrides alone and doesn't have a computed property with a complete set of tags).So, I think 'internal' as a concept is valid even in a world where APIs are a universal first-class feature of Drupal.
We could have another issue to deal with the second case. But I'll share some initial thoughts here:
From the preparedness point of view, I do see the benefit of creating an incremental upgrade path (read then write), but I fear it might actually be counter-productive by making it too easy to ignore doing "the right thing".
An all-or-nothing flag (I think we called this
api_consumable
), raises the cost of upgrading a bit, but it also creates an incentive for contributors to upgrade the entity/field type in all respects.How?
We know that reads are a far more common use case than writes. Therefore, I can plausibly believe contributors will file issues and submit patches to make this flag
TRUE
and, while they're at it, also cover the write case. OTOH, I think it less plausible that a contributor will come along and dedicate time purely to makeapi_writable
TRUE
.I could be convinced I'm wrong about that though.
Comment #18
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOpened #3040354: Introduce entity type annotation key for indicating that an entity type is *safe* for HTTP API usage for more discussion about #17.
Comment #19
Wim Leers@gabesullice in #3040025-8: [meta] Decide on and implement additional security hardenings for JSON:API and other HTTP APIs:
Also moving to core's
jsonapi.module
component.Comment #20
gabesullice;)
Comment #21
gabesulliceUnless anyone is against #19 in the next 2 weeks, I'll change this to be a child of #3032787: [META] Start creating the public PHP API of the JSON:API module.
Comment #22
e0ipsoI'm good with #19 @gabesullice. This should be a small patch with significant impact. Excited to see what you post!
Comment #23
Wim Leers#20 LOL 😇
#22: I feel like I missed something or misunderstood something, because I don't immediately see why this should be a small patch?
Comment #24
gabesulliceHere ya go! The patch isn't super small, but it's well contained/scoped and has the advantage of being decoupled from the entity system.
The pattern might be a little odd to read at first, but it's for a reason... It ruthlessly protects against any unforeseen usage patterns while leaving the door open to future enhancements like field aliasing, custom resource type names, etc.
Comment #25
gabesulliceI think this cleans the concept up a bit.
Comment #26
jibranI think this event will help to fix #2996638: Allow resource types to include count. We just need to add a new method to
ResourceTypeBuilder
so that\Drupal\jsonapi\ResourceType\ResourceType::includeCount
can condtionally returnTRUE
.Comment #27
e0ipsoThis should go into a generic Drupal core trait
EventDispatcherAwareTrait
, along with the property declaration.Comment #28
e0ipsoThere are a few things about the implementation that I'd like to explore further.
I like the idea of the build step to generate the resource type objects. My only requirement here is that this step happens after the configuration system is ready, because there will be a UI to do this and configuration is a great way to deal with all that. This is already the case 💯.
I'm not sure about the idea of creating a builder that contains the same properties that the resource type contains so they can be modified and (ultimately) set into the resource type. I don't also feel great about the
$resource_type
being passed by reference as a means of altering its internal properties. Side effects of this is an awkward public method marked as@internal
along with the$resource_type = NULL;
declaration. I would much rather using function composition rather than parameter mutation.I believe that all this is to ensure that the
ResourceType
class creates only semi-immutable objects that cannot be modified after the all the events have been processed (rather than pure immutable objects that cannot be altered after the object creation).My suggestion is to have the
ResourceTypeRepository
build the resource types just like it is now. Then the event dispatcher / hook system allows altering that resource type. Finally the resource type repository locks mutation on the resource type object and returns it.In addition to that, we may make the
ResourceType
's constructorprotected
(orprivate
even). The goal is to enforce all resource types to be created via the factory class (ResourceTypeRepository
).Probably in a related issue we could consider adding
immutablephp/immutable
in order to Avoiding Quasi-Immutable Objects in PHP, if that is a concern.Comment #29
Wim Leers#24 I like the sound of 👏
#28
➕
➕
I'm concerned we're repeating all of the currently known properties of a
ResourceType
— perhaps that will change in the future.This reminds me of the
ContainerBuilder
vsContainer
pattern that Symfony's dependency injection container uses.I do like that!
But the
ContainerBuilder
operates at the level of a collection of services, not at the level of individual services.I think this patch would feel different (and would also address @e0ipso's concerns) if it were to apply this pattern at the
ResourceTypeRepository
level instead of theResourceType
level.i.e. a
ResourceTypeRepoositoryBuilder
, not aResourceTypeBuilder
.Comment #30
gabesulliceSee attached patch, it gets rid of the builder entirely. The API surface really remains the same, there's much less superfluous stuff behind it.
Frankly, you can just forget about #25, but if you're curious why there was a bunch of weird contortions in the #25 code, read on...
I started out by trying to implement the builder pattern and by trying to hide the details of dispatching events from the resource type repository.
What happened was that I ended up bending over backward to ensure that get the event dispatcher where it needed to be and to prevent the event subscriber from having access the anything it shouldn't. I also wanted to do my best to ensure that we would be able to make future changes without worrying about BC on a "best effort" even though things were @internal. Thus, you'll see all the protected constructors, the final classes, that the builder was staticly created (instead of using service injection), etc.
The end goal was guarantee that the only public API would have been the
ResourceTypeBuildEvents::BUILD
constant and the event itself.After a weekend and a few days to think about it and after reading both of your concerns about the weird things like the argument passed by reference (which I didn't really like either), I realized that the simple solution was staring me in the face. I started with biases and was letting the implementation drive the solution:
Comment #31
Wim Leers#30++ for the simplified patch and sharing the full rationale. That's very helpful :)
👍 Ah, so now the event only contains two things: the resource type name it applies to (immutable), and one flag to affect how the actual
ResourceType
object will be generated (mutable).That indeed allows the API surface to be expanded in the future (think
setFoo()
).Still, it does mean we need to add a method for every thing we want to make customizable, which is what I raised in #29.1. But if we want to keep
ResourceType
objects immutable like they are today, that's necessary.At least this allows us to gradually expand which customizations we want contrib modules to be able to make. If we'd be exposing all of the properties of resource type objects, we wouldn't have that control over API surface and prudence.
So: +1!
🤓 s/build/built/
🤓 Just
@var bool
is enough.🤔 Wouldn't
disable()
be better?🤓 Main? Implying there are non-main build processes too?
🤓 s/JSON API/JSON:API/
Effectively only nitpicks, so keeping at
so @e0ipso can chime in!Comment #32
e0ipsoI echo the +1. This addresses all my previous concerns. I also agree with the feedback provided by @Wim Leers in #31.
I am tempted to create a follow up to disable resource fields. Thoughts?
Comment #33
gabesulliceWe're on the same page. I thought I had said this here, but I see I actually said it to you in Slack:
I was thinking about disabling fields, aliasing field and resource type names, etc when I said this ^
So, yes, we can definitely create followups to do that once this is RTBC/committed!
(I'm thinking we should postpone the field-level modifications on #3014277: ResourceTypes should know about their fields though)
Comment #34
gabesullice#31:
1.
Exactly!
Precisely :)
2. Fixed.
3. Whoops, copy pasta problems. Fixed.
4. Yes! I think
disableResourceType
would be even better. Updated.5. Hm, when I wrote that I was thinking there might be multiple phases (to handle
setRelatableResourceTypes
for example). I think writing that in the docs is premature though. Removed.6. Fixed.
Comment #35
Wim LeersThis now looks great to me! An exciting first step in creating a public API for the JSON:API module! And we've got sign-off from all three JSON:API maintainers (@gabesullice, @e0ipso and myself). Great!
Comment #36
jibranAs it is a new feature, let's add a change record.
Comment #37
jibran+1 to the approach. The other day, I uninstalled the jsonapi extra and added following code to the overridden
'jsonapi.resource_type.repository'
service,After this issue, I don't have to override this service anymore and the only missing piece will be #2996638: Allow resource types to include count for me as stated in #26.
Thank you for working on this. It looks great.
Comment #38
gabesulliceCreated a change record: https://www.drupal.org/node/3079797
Comment #39
Wim LeersThanks for validating the approach in a real-world project, @jibran! 👍🙏
Comment #40
xjmI think the IS is out of date here -- this is no longer exposed in the UI, right? I know @webchick had concerns about that.
Comment #41
Wim LeersYou're right!
Comment #42
alexpottThe needs a reroll and #40 still needs to be addressed.
Comment #43
gabesulliceUpdating the IS. Reroll next.
Comment #44
gabesulliceRerolled. Back to RTBC.
Comment #45
e0ipsoComment #46
alexpottThis past tense is confusing me. How come this is not
isResourceTypeDisabled()
. For mewasSomething()
refers to something that has happened in the past but is no longer true. Or maybe this moreresourceTypeShouldBeDisabled()
as this more refers to something that is going to happen in the future - at least with respect to how the event is fired and then the disabled resource type is built.Setting to needs review to get some opinions.
Comment #47
dww+1 to #46. That's confusing. ;)
"has been" vs. "will be" -- huh?
For that matter, why have "ResourceType" in the function name at all? The event is specific to a certain entity type / bundle combo, anyway. Why not just
isDisabled()
(to matchisInternal()
)?Thanks,
-Derek
p.s. Fixed very minor typo in the summary (s/even/event/).
Comment #48
dwwAlso, don't we need the constructor arguments BC deprecation song/dance here? E.g.
$dispatcher
should have a default value of NULL and if it's actually NULL, we trigger a deprecation warning that the event dispatcher is required in D9, and use\Drupal::
for it...Comment #49
gabesullice#46, I went with because of when/where the method is called. If you're confused, then it's confusing. @dww +1's that. I changed it to
resourceTypeShouldBeDisabled
per your suggestion.#47:
For the future! Once this lands, I hope to add more methods to this event, one of which might be
disableField($field_name)
and then we'll need ashouldFieldBeDisabled($field_name)
or something like that.#48:
AFAIU, no. This change will not be backported into a patch release. Per the BC policy:
Comment #50
rajanvalecha12 CreditAttribution: rajanvalecha12 as a volunteer commentedThe recent patch in #49 doesn't apply to latest drupal release 8.7.7
Here is the patch that I re-rolled for this purpose and it's working for me.
Comment #51
Wim Leers#50: Features are only added to the next minor. The current minor only gets bugfixes.
I think #49 addressed @alexpott's concerns in #46.
Comment #52
gabesulliceRerolled after #3014277: ResourceTypes should know about their fields
Comment #53
gabesulliceComment #54
gabesulliceCreated #3085035: Add a public API for aliasing and disabling JSON:API resource type fields as a follow-up per #32 and #33.
Comment #55
gabesulliceAdding the blocker tag for #3085035: Add a public API for aliasing and disabling JSON:API resource type fields an a contributed project soft blocker tag because JSON:API Extras needs this in order to stop depending on internal APIs.
Comment #56
e0ipsoYay!
Thanks @gabesullice and @Wim Leers for helping me keep up with the internal API changes.
Comment #57
larowlanshould we document this is protected by design and use the static method?
is there a reason for the static method (it doesn't seem to contain any logic that isn't already available by using the new keyword)
e.g. if the method was
createFromEntityTypeIdAndBundle($entity_type_id, $bundle)
then there would be value beyond thenew
keywordif the entity type is internal, do we need to dispatch the event at all? save a few cycles
Comment #58
gabesullice1 & 2. Good call.
3. Nice suggestion, done.
EDIT: Don't know how I managed to upload duplicate patches 🤷🏼♂️
Comment #59
e0ipsoSome times a patch is SO GOOD that drupal.org can't have just one copy of it.
Comment #60
Wim LeersSuperb suggestions, @larowlan! 🙏👍
😂
Comment #61
larowlanComment #62
larowlanCommitted 9b41f7f and pushed to 8.8.x. Thanks!
Published change record - thanks all!
Comment #64
xjmWhile this isn't in itself a disruptive change, as an important security hardening for contrib to implement, @larowlan and I think it should still be mentioned in the release notes since it's something more than your average CR.
I'm not sure whether this would be something that would get added to the frontpage post, but tagging just in case. The pitch would be something like "8.8.0 adds an important additional security feature for JSON:API, allowing modules to indicate whether their entity data should be available over JSON:API at al.l" Or something like that
Comment #65
xjmI posted #3087525: [meta] Determine which core entity types should not expose their data to JSON:API (for DX or security reasons) for implementing this for core entity types as appropriate.
Comment #66
Wim LeersSince the moment that JSON:API was committed to Drupal core, it's been possible for modules to choose to not expose "their entity data" via JSON:API: by marking the entity type "internal". This API addition is to allow modules like https://www.drupal.org/project/jsonapi_extras (or custom modules to act at a more granular level than entity types.
Comment #67
e0ipsoWhile I agree with the technical precision of #66 I also see that #64 has merit. Additionally, marking entity types internal may have other unintended consequences that's why it's always been a tough recommendation.
@Wim Leers would you be more comfortable with the adition of the bold part to @xjm's statement.
Comment #68
Wim LeersThe consequences affect only the REST and JSON:API modules.
Yes :)
Comment #69
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedInternal entity types can not be moderated and they can not be edited in a workspace, so it affects at least two other core modules.
Comment #70
Wim Leers😅 I had this eerie feeling I was forgetting about something, thanks for pointing that something out, @amateescu! 🙏
Comment #71
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFYI, I committed #3085035: Add a public API for aliasing and disabling JSON:API resource type fields, which requires an update to this issue's CR and may also require an update to this issue's release note and/or highlight entry. I set that issue to Needs Work for those reasons.
Comment #72
gabesulliceComment #73
gabesulliceI took a stab at the release notes to encompass #3085035: Add a public API for aliasing and disabling JSON:API resource type fields.
I took the proposed notes and modified it. Notably, I changed "important" to "optional" because I think "important" makes it seem a little too much like sites ought to go out an implement the API right away because there's some known vulnerability.
Comment #74
xjmWell... they should, if their data is not meant to be exposed. :P I just left it as a "security hardening feature" as a compromise, but did add a recommendation at the end to implement the API if the data should never be exposed.
Comment #75
xjmhttps://www.drupal.org/docs/8/modules/jsonapi/security-considerations should also be updated to document that this API is available.
Comment #76
gabesulliceI like your compromise! 👍
But for any passers by to this issue a year from now...
<warning>
Before doing this, you should be carefully configuring permissions and/or implementing custom access hooks if you don't want your data exposed in any way, including JSON:API.
This security hardening feature might mitigate some future unknown access bypass vulnerability by locking down a JSON:API-specific attack vector., but it's not a replacement for proper access control.
There are many other vectors by which your data might be exposed that are not via JSON:API if you don't have proper access controls in place.
There are access APIs that cover all of them, including JSON:API.
</warning>
Sorry, I had to say it and to keep beating this dead horse.
I can't stop myself from worrying that if we mess up our messaging for this, new users coming to Drupal just for JSON:API (I see this on a nearly daily basis in Slack) will misinterpret this feature and end up merrily disabling resource types, completely unaware that while their
/jsonapi/node/private_bundle/{uuid}
route is disabled, that same entity is still sitting out in plain sight at/node/42
(albeit in HTML form instead of JSON).I've had too many conversations with new Drupal users in #contenta that go pretty much like this:
Now, if they Google "disable users JSON:API Drupal", this API might be the first thing they'll get and they might never think more deeply about it.
Comment #77
e0ipsoI very much agree with #76. It was very brilliantly articulated.
Security hardening also encompasses not giving a false sense of security. This feature is very important for API design reasons, additionally it may serve as a protection of some
unk-edge-case
attack vectors.But this is not what this API's goal is:
Making it seem like this is the API's goal, may even be giving a false sense of security. If some data is sensitive (and therefore not meant to be exposed), it must be protected with entity access. Protecting it with this API is NOT the recommended way.
I think this is what @gabesullice alluded to in #76:
That being said, I agree with tagging this as a security hardening (against the
unk-edge-case
attack vectors).I think there is unanimity in these thoughts among the API-First co-coordinators, but I will let @Wim Leers and @gabesullice confirm that.
Comment #78
Wim LeersAmen. This is what we've had to repeat over and over again. I think we're running out of ways to say the same thing.
Comment #79
gabesulliceConfirmed ;)
💯
Comment #80
jibranThe docs changes requested in #75 has been made see https://www.drupal.org/node/3039599/revisions/view/11572419/11608018 so removing the tag and marking this fixed.