Introduce entity type annotation key for indicating that an entity type is *safe* for HTTP API usage

I'm the first to advocate for stricter rules on what is allowed to be exposed, but this seems quite problematic :)

First thoughts:

a) I think in general, it is more of an issue with field types than entity types, because entity types are by default locked down, you have to at least specify an admin permission to make it accessible at all. Then again, e.g. json_api tells the whole world about any content entity type on your site, even if you set access at all.

b) This would break all existing sites that rely on any custom entity types/field types in their APIs. So we'd need a BC layer for that? E.g. not *actually* respecting that property in 8.x, but triggering a deprecated message if any entity type without that flag is explicitly accessed? Or having a flag to opt-in to respecting that, with a status report warning as long as it is off? Changing the default value in 9.x, e.g. in 8.x, we'd add it and allow entity types to opt-out, in 9.x, we'd default to FALSE and they are forced to opt in. A combination of those things? Last and first is pretty much the same actually.

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

1. #3040372: Allow for API-First content without requiring all configuration to also be API-first would make JSON:API easier to use while at the same time making it safer by default. It isn't end-user actionable, but I think it would dramatically reduce the attack surface area of JSON:API. I think its one drawback is that it is quite blunt. I suspect that it will lead us directly to...
2. #3040354 should be merged with the previous issue or made a required followup to it.

I am not sure that this should be merged. I think both approaches can be considered orthogonal. Because for #3040372, it's possible to add a reference to a config entity type that was not previously reachable from content, and that should cause it to be exposed automatically.

@Wim Leers, I thought about this more and I think I agree that these can be considered orthogonal, but not by the same reasoning.

My original thinking was that #3040372 would need this issue in order to be overridable. As you pointed out, creating a new reference in the other issue would make this possible without this issue, but... I just posted a comment on that issue where I tried to make the case that "referenced by a content entity" probably isn't an ideal heuristic. And because I don't think we should use that rule, I thought this would become necessary.

The key distinction that makes these two issue orthogonal IMO is this quote from the IS:

Per #3037039-17: Create a public API for indicating resource types should not be exposed, there's a semantic difference between "My type makes sense to be consumed via an API" vs. "My type is (un)prepared to be consumed via an API". internal has the former semantics, while api_consumable would have the latter semantics.

Therefore, the mechanism to use for exposing a config entity after we implement the other issue can simply be hook_entity_type_alter to set the config entity type's internal value to FALSE.

Once this issue is implemented, that internal = FALSE setting would have no effect unless the entity was also api_consumable = TRUE.

I'm not sure that having both this and #3037039: Create a public API for indicating resource types should not be exposed makes sense from a DX perspective. As an entity type could presumably at any time turn off its own disableResourceType() (and thereby expose the resource), I think the API from #3037039: Create a public API for indicating resource types should not be exposed still serves both usecases. I think having both would confuse developers and degrade the DX for it. So I would actually consider marking this one as a duplicate, unless the annotation key itself would implement this under the hood.

This is an architectural question that would affect essentially every entity type, so tagging for Framework Manager review.

#2:

1. @Berdir:

I think in general, it is more of an issue with field types than entity types, because entity types are by default locked down

I'm not sure that I agree that this is more of a field issue. IMO, the primary motivation for this issue is that some EntityAccessControlerHandlerInterface::access method might not be securely implemented since the providing module is relying on a route access requirements for their list builder or form controller, instead of entity-level access. It's hard for me to imagine how a EntityAccessControlerHandlerInterface::fieldAccess could be left unimplemented without being insecure under non-HTTP API circumstances too. I guess form builders and alterers could be checking permissions, but that just sounds like a cumbersome poorly written module.

Then again, e.g. json_api tells the whole world about any content entity type on your site, even if you set access at all.

You've pinpointed one reason why this issue would be a valuable addition to core :) It'd mean we would have a reliable/uniform way to detect whether a core or contrib entity type is ready/intended to be exposed over an HTTP API.

2. I think we can address BC by defining a config value that determines the default for when the annotation key is left undefined. If the annotation key is explicitly set to TRUE/FALSE, we should always respect it. Only in the case where it is not defined should we have a BC-safe value. I.e. on existing sites the default will be FALSE, but on new installs the default will be TRUE.

#8:

@xjm, I disagree that these issues are duplicates and I also disagree that they should reuse the same code paths.

#3037039: Create a public API for indicating resource types should not be exposed is an API for disabling resource types that you don't need for your particular site. It's a simple, granular API meant to be implemented like a hook would have been implemented in D7 in a custom module and it's JSON:API-specific. It's about shrinking the size of your site's attack surface when it's unnecessarily broad. It's almost an editorial decision that one makes for one's site (like disabling or renaming menu links). For example, your site might use comments to make editorial notes about a piece of content before its published to a decoupled frontend via the admin UI, but those comments are never meant to be part of the decoupled frontend. In that case, you'd want to disable them.

OTOH, this API is for something much more generally applicable. It's a way for entity type providers to declare "yes, I've carefully reviewed my EntityAccessControlHandler and this entity is *safe* to be exposed over HTTP APIs, like REST module, GraphQL or JSON:API." Paragraphs module should do this, media module should do it too. Comment module should also do it—even though some particular sites may want comments to work of an HTTP API and others may not, comment module should still say "if you want to use comments over an HTTP API, it's safe to expose it AFAICT 👍".

Here's an initial implementation, it's definitely going to fail tests because no test coverage has been updated.

> It's hard for me to imagine how a EntityAccessControlerHandlerInterface::fieldAccess could be left unimplemented without being insecure under non-HTTP API circumstances too. I guess form builders and alterers could be checking permissions, but that just sounds like a cumbersome poorly written module.

Entity level access defaults to access denied. You need to explicitly set up at least an admin permission or a more specific access logic.

A (configurable) field however is by default accessible to anyone if you have access to edit/view a given entity. It is very common to just hide on the form/view display settings, or custom twig templates to show/hide things.

I've provided examples for those kind of things for years in varous API/acess related issues. A paywall for a newspaper for example is typically not implemented with entity access, but some logic in preprocess/twig because you want users to be able to see the teaser and access the detail page and then show them a paywall, probably even show part of the content. I've used alter hooks to change the view mode to another one for example to show things differently.

Another is some internal fields on users that code uses to track things, e.g. an expiration date of some paid membership or other data that shouldn't be visible to the user or even others. Maybe you actually bothered to implement field access, but often that's only the case if you want to make such fields accessible to admins.

@Berdir, I think the key distinction between what you're describing and what I'm describing is that you're talking about certain field instances being private while I'm talking about an API for entity and field types.

e.g. an expiration date of some paid membership or other data that shouldn't be visible to the user or even others.

Since this issue is nominally about entity and field annotations, this example would mean that the implementor would mark the datetime field as insecure by default. I don't think that's what you're arguing for.

Is the practical outcome of your argument that we should ignore doing anything on the entity/field type level and focus on something else?

#3085035: Add a public API for aliasing and disabling JSON:API resource type fields introduced an API for disabling fields in JSON:API only, without affecting field access (see the change record). I think that has some symmetry with the display-oriented approaches you described. Does that API address part of your concerns or is there more to the story?

Finally, ignoring fields for a moment, what do you think of the entity-level annotation that says yes, I've carefully reviewed my EntityAccessControlHandler and this entity is *safe* to be exposed over HTTP APIs, like REST module, GraphQL or JSON:API?

> @Berdir, I think the key distinction between what you're describing and what I'm describing is that you're talking about certain field instances being private while I'm talking about an API for entity and field types.

Yeah, that's correct, we mixed that up.

@yched always said that field types are structure, not business logic. We partially already violate that with things like created/changed fields. In that sense, as you said, saying that a datetime or a string field type is either save or not save is not very meaningful. If anything, it would need to be on the field level (hint: we don't use the term instance anymore, field instance in 7.x = field in 8.x, field in 7.x = field storage in 8.x). But that isn't something that is done in code.

I've been pondering before on whether we can bring how Drupal renders entities with fields on the site and through the API closer together. Specifically, whether we could somehow introduce the concept of API view modes, that as a first step would just allow to control which fields are enabled/disabled when you go to jsonapi/node/ID (I don't know how the actual URL works, this is just an abstract example).

Entity types are different though, even generic things like the node entity type have enough meaning as in "that's a thing that you view and expose", will check the patch but in general I think that makes sense.

Entity types are different though, even generic things like the node entity type have enough meaning as in "that's a thing that you view and expose", will check the patch but in general I think that makes sense.

Based on this and my own thoughts, I think we should descope this issue to only cover the entity level. On their own, I think field types rarely have enough semantic meaning for developers to make a meaningful statement about how safe it is to use them in an API context.

1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeManager.php
   @@ -114,6 +114,13 @@ public function processDefinition(&$definition, $plugin_id) {
   +    // If the `api_consumable` annotation key is not defined, then set a default
   +    // determined by a configuration value.
   +    if (is_null($definition->get('http_api_consumable'))) {
   +      $strict = (bool) \Drupal::config('system.http_api')->get('strictly_expose_api_consumable_entities');
   +      $definition->set('http_api_consumable', !$strict);
   +    }
   

   I'm not sure if the entity type manager is the best place to do this. My thinking was that it's better to do a config lookup while the plugin definitions are being processed (during a cache rebuild), then to do it at runtime. Maybe there's an even better, earlier place that this could be done? Maybe it could be done in an alter hook?

2. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -53,6 +53,14 @@ system.maintenance:
   +system.http_api:
   

   System module is the only place where this really makes sense IMO. This config isn't specific to JSON:API or REST.

3. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -53,6 +53,14 @@ system.maintenance:
   +    strictly_expose_api_consumable_entities:
   +      type: boolean
   +      label: 'Whether the http_api_consumable entity annotation should be strictly enforced.'
   

   I'm open to a better key name than strictly_expose_api_consumable_entities. I also think that this label could be improved. The way BC works isn't really about "enforcement" it's about a sensible default.

@Berdir Have you had a chance to review the patch? It'd be good to have an entity API maintainer perspective on the implementation and #15.

Some review of #15:

I'm not sure if the entity type manager is the best place to do this [...]

It seems alright to me, otherwise the system module could implement hook_entity_type_build but in practice both implementations would alter definitions while they're processed and not at runtime.

I'm open to a better key name than strictly_expose_api_consumable_entities [...]

Since it's already nested inside an api/http related key, I think this could be strictly_expose_entities.

If there's a sense that this implementation is desirable I can jump in and address any review and write tests, but I want to wait until there's some sign off.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.