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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

gabesullice’s picture

Issue summary: View changes
FileSize
77.42 KB

Architecturally, 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:

  1. JSON:API has no concept of type inheritances like Drupal's entity type -> bundle concept. ResourceTypes "flatten" things out.
  2. ResourceType::isLocatable() is just a shorthand for "is the underlying entity type backed storage?" E.g. The message entity type can be POSTed to but not GETed). This shorthand keeps things clean when we define routes or check if it's appropriate to add a self hyperlink to the response.
  3. ResourceType::isVersionable(), is a shorthand for "is this entity type revisionable." But we've also used it to hardcode and limit it to node and media entities, since the necessary revision access logic isn't complete enough to feel safe about any other entity types, even though they implement RevisionableInterface.

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.

Wim Leers’s picture

Thanks for writing #2, @gabesullice! Very clear explanation :)

e0ipso’s picture

Thanks @gabesullice. That's a very detailed summary of things.

One clarification:

Where JSON:API Extras overrides these values, it's often to do the things that we consider too risky for JSON:API. For example, as of JSON:API 2.0, ResourceType::isMutable() is FALSE, for all config entity types, since Entity::validate() is not available for them.

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?

gabesullice’s picture

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.

Updated my comment. You're right, I (unintentially) misrepresented that.

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?

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.

amateescu’s picture

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.

I 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 :)

xjm’s picture

Ah 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.

effulgentsia’s picture

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?

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.

I think we have 4 cases to consider:

  1. Entity types defined in code
  2. Entity types defined in config
  3. Bundles defined in code
  4. Bundles defined in config

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.

xjm’s picture

Title: Create a public API for marking resource types internal » Create a public API for indicating resource types should not be exposed
Issue summary: View changes

(Retitling to clarify and because there is not agreement that "internal" is the right description. Also embedding the mockup.)

larowlan’s picture

I'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.

webchick’s picture

Before 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?

e0ipso’s picture

I 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.

xjm’s picture

gabesullice’s picture

Quoting myself from #2:

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.

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.

e0ipso’s picture

I'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.

effulgentsia’s picture

We 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 in hook_entity_bundle_info().

Note that even though EntityType is an object, it's not a TypedData object, just as entities themselves are also not typed data objects. However, we have an EntityAdapter for when we want to treat entities as typed data, so perhaps we could add EntityTypeAdapter and EntityBundleAdapter as well, the latter giving us a way to work with the EntityTypeBundleInfo::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.

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.

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 and api_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 setting api_readable without setting api_writable? Or would we prefer to push for the single key of internal and try to claim it back from Content Moderation and Workspaces?

gabesullice’s picture

However, internal doesn't give us the granularity of read vs write. Would it make sense to have two new keys: api_readable and api_writable?

The API-First angel (devil?) on my shoulder is saying "no".

I think we're mixing two concepts together. There's:

  1. My type makes sense to be consumed via an API
  2. My type is (un)prepared to be consumed via an API

There are entity types like menu_link_content and image_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 make api_writable TRUE.

I could be convinced I'm wrong about that though.

Wim Leers’s picture

Project: JSON:API » Drupal core
Version: 8.x-2.x-dev » 8.8.x-dev
Component: Code » ajax system

@gabesullice in #3040025-8: [meta] Decide on and implement additional security hardenings for JSON:API and other HTTP APIs:

  • #3037039: Create a public API for indicating resource types should not be exposed this one does not provide a ton of value from a security hardening perspective relative to the other issues IMO. That's because it isn't safer by default and it requires custom code to be used. That said, I think it has lots of value from a non-security perspective. I propose we move it under #3032787: [META] Start creating the public PHP API of the JSON:API module since JSON:API Extras is already using an @internal API to accomplish the same thing. This will make Drupal more stable regardless of its security outcomes so it doesn't need a priority under this meta issue IMO.
  • Also moving to core's jsonapi.module component.

    gabesullice’s picture

    Component: ajax system » jsonapi.module

    ;)

    gabesullice’s picture

    Unless 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.

    e0ipso’s picture

    I'm good with #19 @gabesullice. This should be a small patch with significant impact. Excited to see what you post!

    Wim Leers’s picture

    #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?

    gabesullice’s picture

    Status: Active » Needs review
    Issue tags: +Security improvements
    FileSize
    15.58 KB

    Here 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.

    gabesullice’s picture

    I think this cleans the concept up a bit.

    jibran’s picture

    I 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 return TRUE.

    e0ipso’s picture

    +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuilder.php
    @@ -0,0 +1,222 @@
    +  /**
    +   * Sets the event dispatcher.
    +   *
    +   * Internal use only. Subscribers should not call this method.
    +   *
    +   * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $dispatcher
    +   *   An event dispatcher with which to dispatch this event.
    +   *
    +   * @internal
    +   */
    +  public function setEventDispatcher(EventDispatcherInterface $dispatcher) {
    +    if (!isset($this->dispatcher)) {
    +      $this->dispatcher = $dispatcher;
    +    }
    +  }
    

    This should go into a generic Drupal core trait EventDispatcherAwareTrait, along with the property declaration.

    e0ipso’s picture

    Status: Needs review » Needs work

    There 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 constructor protected (or private 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.

    Wim Leers’s picture

    #24 I like the sound of ruthlessly protects against any unforeseen usage patterns while leaving the door open to future enhancements 👏

    #28

    My only requirement here is that this step happens after the configuration system is ready

    I don't also feel great about the $resource_type being passed by reference


    1. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuilder.php
      @@ -0,0 +1,222 @@
      +  protected $isLocatable;
      ...
      +  protected $isMutable;
      

      I'm concerned we're repeating all of the currently known properties of a ResourceType — perhaps that will change in the future.

    2. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
      @@ -139,7 +150,7 @@ public function all() {
      -    return new ResourceType(
      +    $resource_type_builder = ResourceTypeBuilder::createWithEntityBundleDefaults(
      
      @@ -149,6 +160,11 @@ protected function createResourceType(EntityTypeInterface $entity_type, $bundle)
      +    $resource_type_builder->setEventDispatcher($this->eventDispatcher);
      +    $resource_type = NULL;
      +    $resource_type_builder->build($resource_type);
      +    assert($resource_type instanceof ResourceType);
      +    return $resource_type;
      

      This reminds me of the ContainerBuilder vs Container 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 the ResourceType level.

      i.e. a ResourceTypeRepoositoryBuilder, not a ResourceTypeBuilder.

    gabesullice’s picture

    Status: Needs work » Needs review
    FileSize
    10.79 KB

    See 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: 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.

    Wim Leers’s picture

    #30++ for the simplified patch and sharing the full rationale. That's very helpful :)

    1. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuildEvent.php
      @@ -0,0 +1,76 @@
      +  protected $resourceTypeName;
      ...
      +  protected $disabled = FALSE;
      
      +++ b/core/modules/jsonapi/tests/modules/jsonapi_test_resource_type_building/src/EventSubscriber/ResourceTypeBuildEventSubscriber.php
      @@ -0,0 +1,36 @@
      +  public function disableResourceType(ResourceTypeBuildEvent $event) {
      +    $disabled_resource_types = \Drupal::state()->get('jsonapi_test_resource_type_builder.disabled_resource_types', []);
      +    if (in_array($event->getResourceTypeName(), $disabled_resource_types, TRUE)) {
      +      $event->setDisabled();
      +    }
      +  }
      

      👍 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!

    2. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuildEvent.php
      @@ -0,0 +1,76 @@
      +   * Whether the JSON:API resource type to be build should be disabled.
      

      🤓 s/build/built/

    3. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuildEvent.php
      @@ -0,0 +1,76 @@
      +   * @var bool $disabled
      

      🤓 Just @var bool is enough.

    4. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuildEvent.php
      @@ -0,0 +1,76 @@
      +  public function setDisabled() {
      

      🤔 Wouldn't disable() be better?

    5. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuilderEvents.php
      @@ -0,0 +1,15 @@
      +   * Emitted during the main resource type build process.
      

      🤓 Main? Implying there are non-main build processes too?

    6. +++ b/core/modules/jsonapi/tests/modules/jsonapi_test_resource_type_building/jsonapi_test_resource_type_building.info.yml
      @@ -0,0 +1,4 @@
      +name: 'JSON API test resource type building API'
      

      🤓 s/JSON API/JSON:API/

    Effectively only nitpicks, so keeping at Needs review so @e0ipso can chime in!

    e0ipso’s picture

    I 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?

    gabesullice’s picture

    I am tempted to create a follow up to disable resource fields. Thoughts?

    We're on the same page. I thought I had said this here, but I see I actually said it to you in Slack:

    I think it might be too early to convert JSON:API Extras to [use this patch], but I think with some very [small] followups, we can expand the pattern so that Extras will have a very clean and elegant public API to do all of its modifications. I'm curious to see what you think.

    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)

    gabesullice’s picture

    #31:

    1. 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).
    Exactly!

    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.

    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.

    Wim Leers’s picture

    Status: Needs review » Reviewed & tested by the community

    This 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!

    jibran’s picture

    Issue tags: +Needs change record

    As it is a new feature, let's add a change record.

    jibran’s picture

    +1 to the approach. The other day, I uninstalled the jsonapi extra and added following code to the overridden 'jsonapi.resource_type.repository' service,

    
    
      /**
       * {@inheritdoc}
       */
      protected function createResourceType(EntityTypeInterface $entity_type, $bundle) {
        $raw_fields = $this->getAllFieldNames($entity_type, $bundle);
        $entity_type_id = $entity_type->id();
        return new CountableResourceType(
          $entity_type_id,
          $bundle,
          $entity_type->getClass(),
          $entity_type->isInternal() || !in_array("$entity_type_id--$bundle", static::JSONAPI_RESOURCE_TYPE_WHITELIST),
          static::isLocatableResourceType($entity_type, $bundle),
          static::isMutableResourceType($entity_type, $bundle),
          static::isVersionableResourceType($entity_type),
          static::getFieldMapping($raw_fields, $entity_type, $bundle)
        );
      }
    

    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.

    gabesullice’s picture

    Issue tags: -Needs change record

    Created a change record: https://www.drupal.org/node/3079797

    Wim Leers’s picture

    Thanks for validating the approach in a real-world project, @jibran! 👍🙏

    xjm’s picture

    I think the IS is out of date here -- this is no longer exposed in the UI, right? I know @webchick had concerns about that.

    Wim Leers’s picture

    Assigned: Unassigned » gabesullice

    You're right!

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work

    The needs a reroll and #40 still needs to be addressed.

    gabesullice’s picture

    Issue summary: View changes

    Updating the IS. Reroll next.

    gabesullice’s picture

    Status: Needs work » Reviewed & tested by the community
    FileSize
    10.94 KB

    Rerolled. Back to RTBC.

    e0ipso’s picture

    Issue summary: View changes
    alexpott’s picture

    Status: Reviewed & tested by the community » Needs review
    +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuildEvent.php
    @@ -0,0 +1,79 @@
    +  /**
    +   * Whether the resource type to be built has been disabled.
    +   *
    +   * @return bool
    +   *   TRUE if the resource type will be disabled, FALSE otherwise.
    +   */
    +  public function wasResourceTypeDisabled() {
    +    return $this->disabled;
    +  }
    
    +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
    @@ -139,11 +150,13 @@ public function all() {
       protected function createResourceType(EntityTypeInterface $entity_type, $bundle) {
         $raw_fields = $this->getAllFieldNames($entity_type, $bundle);
    +    $event = ResourceTypeBuildEvent::create(sprintf('%s--%s', $entity_type->id(), $bundle));
    +    $this->eventDispatcher->dispatch(ResourceTypeBuildEvents::BUILD, $event);
         return new ResourceType(
           $entity_type->id(),
           $bundle,
           $entity_type->getClass(),
    -      $entity_type->isInternal(),
    +      $entity_type->isInternal() || $event->wasResourceTypeDisabled(),
           static::isLocatableResourceType($entity_type, $bundle),
           static::isMutableResourceType($entity_type, $bundle),
           static::isVersionableResourceType($entity_type),
    

    This past tense is confusing me. How come this is not isResourceTypeDisabled(). For me wasSomething() refers to something that has happened in the past but is no longer true. Or maybe this more resourceTypeShouldBeDisabled() 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.

    dww’s picture

    Issue summary: View changes
    Status: Needs review » Needs work

    +1 to #46. That's confusing. ;)

    +   * Whether the resource type to be built has been disabled.
    +   *
    +   * @return bool
    +   *   TRUE if the resource type will be disabled, FALSE otherwise.
    

    "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 match isInternal())?

    Thanks,
    -Derek

    p.s. Fixed very minor typo in the summary (s/even/event/).

    dww’s picture

    +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
    @@ -93,12 +101,15 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
    -  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityTypeBundleInfoInterface $entity_bundle_info, EntityFieldManagerInterface $entity_field_manager, CacheBackendInterface $cache) {
    +  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityTypeBundleInfoInterface $entity_bundle_info, EntityFieldManagerInterface $entity_field_manager, CacheBackendInterface $cache, EventDispatcherInterface $dispatcher) {
         $this->entityTypeManager = $entity_type_manager;
         $this->entityTypeBundleInfo = $entity_bundle_info;
         $this->entityFieldManager = $entity_field_manager;
         $this->cache = $cache;
    +    $this->eventDispatcher = $dispatcher;
    

    Also, 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...

    gabesullice’s picture

    Assigned: gabesullice » Unassigned
    Status: Needs work » Needs review
    Issue tags: -Needs issue summary update
    FileSize
    1.9 KB
    10.95 KB

    #46, I went with was 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 that matter, why have "ResourceType" in the function name at all?

    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 a shouldFieldBeDisabled($field_name) or something like that.

    #48:

    AFAIU, no. This change will not be backported into a patch release. Per the BC policy:

    The following parts of the code base should always be treated as internal and are not guaranteed to not change between minor versions... The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the first place.

    rajanvalecha12’s picture

    The 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.

    Wim Leers’s picture

    Status: Needs review » Reviewed & tested by the community

    #50: Features are only added to the next minor. The current minor only gets bugfixes.

    I think #49 addressed @alexpott's concerns in #46.

    gabesullice’s picture

    gabesullice’s picture

    Issue summary: View changes
    gabesullice’s picture

    gabesullice’s picture

    Adding 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.

    e0ipso’s picture

    JSON:API Extras needs this in order to stop depending on internal APIs

    Yay!

    Thanks @gabesullice and @Wim Leers for helping me keep up with the internal API changes.

    larowlan’s picture

    Status: Reviewed & tested by the community » Needs review
    1. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuildEvent.php
      @@ -0,0 +1,79 @@
      +   * @param string $resource_type_name
      +   *   A JSON:API resource type name.
      +   */
      +  protected function __construct($resource_type_name) {
      

      should 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)

    2. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
      @@ -139,11 +150,13 @@ public function all() {
      +    $event = ResourceTypeBuildEvent::create(sprintf('%s--%s', $entity_type->id(), $bundle));
      

      e.g. if the method was createFromEntityTypeIdAndBundle($entity_type_id, $bundle) then there would be value beyond the new keyword

    3. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
      @@ -139,11 +150,13 @@ public function all() {
      -      $entity_type->isInternal(),
      +      $entity_type->isInternal() || $event->resourceTypeShouldBeDisabled(),
      

      if the entity type is internal, do we need to dispatch the event at all? save a few cycles

    gabesullice’s picture

    1 & 2. Good call.

    3. Nice suggestion, done.

    EDIT: Don't know how I managed to upload duplicate patches 🤷🏼‍♂️

    e0ipso’s picture

    EDIT: Don't know how I managed to upload duplicate patches 🤷🏼‍♂️

    Some times a patch is SO GOOD that drupal.org can't have just one copy of it.

    Wim Leers’s picture

    Status: Needs review » Reviewed & tested by the community

    Superb suggestions, @larowlan! 🙏👍

    Some times a patch is SO GOOD that drupal.org can't have just one copy of it.

    😂

    larowlan’s picture

    larowlan’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed 9b41f7f and pushed to 8.8.x. Thanks!

    Published change record - thanks all!

    • larowlan committed 9b41f7f on 8.8.x
      Issue #3037039 by gabesullice, rajanvalecha12, e0ipso, Wim Leers,...
    xjm’s picture

    While 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

    xjm’s picture

    Wim Leers’s picture

    "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"

    Since 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.

    e0ipso’s picture

    While 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.

    8.8.0 adds an important additional security feature for JSON:API, allowing modules to indicate whether the resource types associated to their entity data should be available over JSON:API at al.l

    @Wim Leers would you be more comfortable with the adition of the bold part to @xjm's statement.

    Wim Leers’s picture

    marking entity types internal may have other unintended consequences

    The consequences affect only the REST and JSON:API modules.

    @Wim Leers would you be more comfortable with the adition of the bold part to @xjm's statement.

    Yes :)

    amateescu’s picture

    The consequences affect only the REST and JSON:API modules.

    Internal entity types can not be moderated and they can not be edited in a workspace, so it affects at least two other core modules.

    Wim Leers’s picture

    😅 I had this eerie feeling I was forgetting about something, thanks for pointing that something out, @amateescu! 🙏

    effulgentsia’s picture

    FYI, 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.

    gabesullice’s picture

    Issue summary: View changes
    gabesullice’s picture

    Status: Fixed » Needs review

    I 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.

    xjm’s picture

    Issue summary: View changes

    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.

    Well... 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.

    xjm’s picture

    Issue tags: +needs documentation updates

    https://www.drupal.org/docs/8/modules/jsonapi/security-considerations should also be updated to document that this API is available.

    gabesullice’s picture

    I like your compromise! 👍


    But for any passers by to this issue a year from now...

    <warning>

    Well... they should, if their data is not meant to be exposed.

    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:

    Them: Hi! How do I hide a URL from the API?
    
    Me: Hm, there are lots of ways. Are you trying to keep certain users from ever accessing users?
    
    Them: yeah, I only I need them to see blog posts. I don't want anyone to be able to get a list of users from /jsonapi/user/user though. How do I disable that? Is that what JSON:API Extras is for? I tried a Symfony route filter but I thought there might be a better way.
    
    Me: yeah, definitely. it sounds like you need to set some permissions up or implement hook_user_access(). That'll keep users from accessing user entities from anywhere on your Drupal site, including JSON:API, unless they're allowed to.
    
    Them: Oh, TIL. That sounds perfect. Let me try that.
    
    20 minutes pass...
    
    Them: that worked great, thanks!
    
    Me: 👍
    
    

    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.

    e0ipso’s picture

    I 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:

    Well... they should, if their data is not meant to be exposed.

    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:

    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.

    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.

    Wim Leers’s picture

    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.

    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.

    Amen. 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.

    gabesullice’s picture

    I think there is unanimity in these thoughts among the API-First co-coordinators, but I will let @Wim Leers and @gabesullice confirm that.

    Confirmed ;)

    I think we're running out of ways to say the same thing.

    💯

    jibran’s picture

    Status: Needs review » Fixed
    Issue tags: -needs documentation updates

    The 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.

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.