Add a read-only mode to JSON:API

Off to a great start!

1. +++ b/jsonapi.routing.yml
   @@ -1,2 +1,10 @@
   +  path: '/admin/config/services/jsonapi'
   

   This collides with JSON:API Extras. I think it's OK, we can work it out there. Can you open an issue to make sure that happens?

2. +++ b/jsonapi.routing.yml
   @@ -1,2 +1,10 @@
   +    _title: 'JSON:API'
   

   Let's make a not about Read-Only in the page title to show that there should not be scope creep in this config screen.

3. +++ b/src/Routing/ReadOnlyModeMethodFilter.php
   @@ -0,0 +1,66 @@
   + * Filters routes based on the HTTP method.
   

   … and the configuration flag.

4. +++ b/src/Routing/ReadOnlyModeMethodFilter.php
   @@ -0,0 +1,66 @@
   +      if ($route->hasDefault(Routes::JSON_API_ROUTE_FLAG_KEY) && $route->getMethods() !== ['GET']) {
   

   We probably want to include ['GET', 'HEAD', 'OPTIONS', 'TRACE'] here.

   $is_read_only_route = array_reduce($route->getMethods(), function ($read_only, $method) {
     return $read_only && in_array($method, ['GET', 'HEAD', 'OPTIONS', 'TRACE']);
   }, TRUE);
   if (… && $is_read_only_route)
   

Two general comments.

1. Aside from fixing the tests we also need the hook_update (just making sure we don't forget).
2. If this is a JSON:API-specific fix (still puzzled about that), doesn't it make more sense to use ResourceTypeRepository::isMutableResourceType? That'll simplify the patch quite a bit.

[/admin/config/services/jsonapi] collides with JSON:API Extras. I think it's OK, we can work it out there. Can you open an issue to make sure that happens?

What about keeping the path the same, and having JSON:API Extras form_alter the form to add all of its stuff to it? In which case,

+++ b/jsonapi.links.menu.yml
@@ -0,0 +1,5 @@
+  description: "Enable or disable JSON:API's read-only mode."

Should this description be more generic, like "Configure JSON:API"?

Alternatively, maybe we keep this description as-is, and make it JSON:API Extras's responsibility to alter it to the more generic one?

What about keeping the path the same, and having JSON:API Extras form_alter the form to add all of its stuff to it? In which case,

I thought about this, but I want this screen / setting to be dead simple and uncluttered.

Thanks @Wim Leers!

UX contributors who should be credited:
webchick, benjifisher, jhodgdon, Gábor Hojtsy, xjm, phenaproxima, ckrina, effulgentsia, wordlinemine

One thing I can't remember now whether Wim and I discussed is also potentially adding a line to the hook_help() about it, but that's less important than the in-context docs on the form.

Other things discussed with UX team that we won't implement at this point (in addition to the things Wim pushed back on above):

1. We discussed adding a status report info message (or worldlinemine was suggesting a warning), but we should scope that to a followup since the radio is the essential change.
2. We discussed listing the specific resources that were exposed and how in a table below the main form, to help the site owner understand the implications of their choice, but that should also be scoped to a followup.
3. The UX team all agreed not to add any kind of confirm form since it would be redundant "Cover Your Access-control". ;)

+++ b/src/Form/JsonApiSettingsForm.php
@@ -0,0 +1,61 @@
+      '#description' => $this->t('When writing is allowed, entities can be added, changed or removed via JSON:API. <a href=":docs">This may have security implications.</a>', [':docs' => 'https://www.drupal.org/docs/8/modules/jsonapi/security-considerations']),

Oh, one other nitpick that came up in the meeting is that we need the Oxford comma here:

[...] added, changed, or removed [...]

This collides with JSON:API Extras. I think it's OK, we can work it out there. Can you open an issue to make sure that happens?

@e0ipso, we also discussed in the UX meeting that from a usability perspective, JSON:API extras might add its config to the same page. (It's out of scope here and not in any way something that impacts core inclusion, just was the unanimous recommendation of UX people in the meeting.)

Oops, looks like #14 was also already discussed above. Sorry, crossposts!

My feeling on the screenshot in #17:
- The radio options seem very clear to me.
- I am not sure the term "entities" is clear to all users. Suggest putting something in that clarifies it, like saying instead of just "entities"
entities (content items, taxonomy terms, ...)

Or something similar.

Looks like worldlinemine also still needs to be credited as well: https://www.drupal.org/u/worldlinemine

Meanwhile, adding the followup tasks to the IS.

1. +++ b/src/Form/JsonApiSettingsForm.php
   @@ -0,0 +1,61 @@
   +        'r' => $this->t('Allow only reading'),
   +        'rw' => $this->t('Allow reading and writing'),
   

   I was thinking maybe we could use the word 'access' to make it more clear, and it also might not be clear what reading and writing is about for less technical users. So maybe something like:
   - Allow only read access to the API.
   - Allow read and write access to the API.

   The part 'access to the API' might be redundant and could be fixed by adding a field label: 'Define access to the API', but just some thoughts.

   I also think these need a dot at the end, like on the /admin/config/media/file-system page.

2. +++ b/src/Form/JsonApiSettingsForm.php
   @@ -0,0 +1,61 @@
   +      '#description' => $this->t('When writing is allowed, entities can be added, changed, or removed via JSON:API. <a href=":docs">This may have security implications.</a>', [':docs' => 'https://www.drupal.org/docs/8/modules/jsonapi/security-considerations']),
   

   I think we discussed in the UX meeting we could probably use the Warning: prefix, similar to the permissions page. So this would then be Warning: When writing is allowed, entities can be added, changed, or removed via JSON:API. This may have security implications..

Great progress! Thanks everyone for trying hard to get JSON:API to land.

It's a minor suggestion, but "This may have security implications." sounds a bit scary. I'd write it in a positive way: "When no write operations are required, setting JSON-API to read-only is recommended as it hardens your site's security.". The latter helps the user understand the why and feels more constructive. That extra explanation could incentivize more people to opt for read-only. Again, not a showstopper by any means, and it might have been debated in other places.

@Dries, we also just discussed almost exactly that in the UX channel. I think Wim will have an updated patch with some changes we proposed to address making it a warning that's actionable rather than vague and therefore scary. Thank you!

#36 Looks good to me! Thanks.

Agree that #36 looks better!

This needs a $form['read_only']['#title'], because '#type' => 'radios' is rendered as a fieldset with a legend. The patch in #36 gives us an empty legend.

It could use a configure key in the info.yml file too

1. +++ b/jsonapi.services.yml
   @@ -143,6 +143,13 @@ services:
   +  method_filter.jsonapi:
   +    public: false
   +    class: Drupal\jsonapi\Routing\ReadOnlyModeMethodFilter
   +    decorates: method_filter
   +    arguments: ['@method_filter.jsonapi.inner', '@config.factory']
   

   Why use decoration here? Why not just add it as a separate route filter?

2. +++ b/src/Routing/ReadOnlyModeMethodFilter.php
   @@ -0,0 +1,69 @@
   +      $is_read_only_route = !empty(array_diff($route->getMethods(), $read_only_methods));
   +      if ($is_jsonapi_route && $is_read_only_route) {
   +        $collection->remove($name);
   +      }
   

   Based on this logic, is_read_only_route is incorrectly named, because it's actually telling you if it's a route that allows writes. Also, it's not handling the case where $route->getMethods() returns empty, because the meaning of that is that all methods are allowed. The jsonapi module doesn't create such routes, but we shouldn't assume that here.

3. +++ b/src/Routing/ReadOnlyModeMethodFilter.php
   @@ -0,0 +1,69 @@
   +    throw new MethodNotAllowedHttpException(array_unique($all_supported_methods), sprintf("JSON:API's read-only mode is enabled. Site administrators can enable writes at %s.", Url::fromRoute('jsonapi.settings')->setAbsolute()->toString(TRUE)->getGeneratedUrl()));
   

   $all_supported_methods is always empty. We need to add the supported methods to it.

After catching up on this issue, this would be my ideal:

[ ] Accept only JSON:API read operations.
[ ] Accept all JSON:API create, read, update, and delete operations.

Warning: You should not unnecessarily accept all operations. By limiting your site to only read operations you can reduce your exposure to potential security vulnerabilities. Learn more about securing your site here.

Here are my reasonings:

1. "Allow only reading JSON:API": implies that there are access control implecations. It suggests that existing access controls won't be respected, which is of course untrue. Even if you choose "allow," many requests will often be forbidden. Therefore, I've changed this to "accept" because... once a request is accepted, access will still be checked.
2. "Allow only reading JSON:API": "only" sounds strange here because it's not paired with a similar word in the corresponding option. Therefore, I added all to balance things out.
3. "Allow only reading JSON:API": What we're allowing are read operations. Not reading JSON:API. JSON:API is a specification. Also, reading as a term is an imprecise developer-ism.
4. Warning: I changed this because the warning was describing a feature of JSON:API. It wasn't actually telling a user why that option might be undesirable. As above, the language also makes it sound like JSON:API will bypass entity access controls, which it will not.

   Let's not forget... we're adding this read-only mode because of a fear of hypothetical, as-yet-unknown bugs in preexisting entity access controls—not because we're aware of any existing or inherent flaws in JSON:API's respect for, or usage of, these public access APIs.

Regarding the follow-up:

Non-blocker, major UX task: Discuss whether the site owner should see a list or table on the page of the resources that are exposed.

When and where was it decided that either of these should exist at all?

I'm very -1 to this follow-up.

This is not information that the site owner should see. AFAIK, that information will be entirely unactionable and overwhelming. JSON:API's docs are already very clear about what data it exposes.

I'm also wondering about #6.2 or, alternatively, why unsafe methods are ever added in Routes.php. Why not just prevent the routes from being created at all? (or send them to a no-op controller method like EntityResource::getReadOnlyErrorResponse())?

I think it's because the route filter allows for the generic error response, which is a DX win?

I think we also need a configure key in our module info file.

I'm also wondering about #6.2 or, alternatively, why unsafe methods are ever added in Routes.php. Why not just prevent the routes from being created at all? (or send them to a no-op controller method like EntityResource::getReadOnlyErrorResponse())?

I think the intent is that this technique is moved out of JSON:API and generalized in core. At least my reviews have had that in mind. However you are right that I have not seen a follow-up created (or mentioned) in that direction. If that is not the intent, then I agree that we should approach this differently from a technical perspective.

Can someone, please, confirm the assumption above ☝️?

WRT the error object:

throw new MethodNotAllowedHttpException(array_unique($all_supported_methods), sprintf("JSON:API's read-only mode is enabled. Site administrators can enable writes at %s.", Url::fromRoute('jsonapi.settings')->setAbsolute()->toString(TRUE)->getGeneratedUrl()));

It would be great if we could add a Link to https://www.drupal.org/docs/8/modules/jsonapi/security-considerations and have a more detailed explanation there.

I'm glad we have got here after the long discussion.

+++ b/config/schema/jsonapi.schema.yml
@@ -0,0 +1,7 @@
+jsonapi.settings:
+  type: config_object
+  label: 'JSON:API settings'
+  mapping:
+    read_only:
+      type: boolean
+      label: 'Read-only mode enabled'

We need a hook_update_N to update the config active storage of existing sites with JSON:API installed.

For the followup issues, we can discuss them on the issues once filed and decide whether they're wontfix or a different fix or whatnot. :) Although I do think we want these left open to migrate into the core queue. We can move the feedback for them over once they're posted.

I am certain that this is the link @Wim Leers meant to link for his response to my comment on #44: #3039687: Add an API read-only mode to Drupal core

Edit: cross posted.

Feedback about the documented follow-ups:

• Docs gate core blocker: Draft a handbook page (Wim has notes on the details), which we will provide to the sec team for review.
• Non-blocker, major UX task: Discuss whether the site owner should see a list or table on the page of the resources that are exposed. This is the user-facing complement of what /jsonapi provides developers, except possibly with additional info on whether it is read-only (e.g. config entities) or writable.
• Non-blocker, major or normal UX task: Discuss adding a status report entry about the setting (helpful for site audits per several people in the meeting). TBD also: whether it's a warning or info.
• Non-blocker: core critical task for 8.8.x to provide a generalized API in core that will support this for REST etc. too: #2843147: Add JSON:API to core as a stable module.

• Sounds great ✅
• Like @gabesullice, I am very hesitant about this. If anythig, I'd be excited to use this as an excuse to start leveraging JSON:API in a progressively decoupled approach inside of core. But let's discuss this in the follow up itself since it's not blocking.
• I'm 🆗 with this
• Sounds great ✅✅

I don’t feel I can sign off on it, but the field label and radio buttons look very good to me! I like the fact that we now explicitly mention all the operations.

I do think that exposure to potential security vulnerabilities in the adding, changing or removing of entities looks a little bit scary. I would immediately think, why is there an option with potential security vulnerabilities? What are they? In the previous version of the description it stated a bit more what the risk were (adding, changing and removing content), which for some reason sound less scary to me than potential security vulnerabilities.

On the other hand, it might be that we actually want it to look scary so people won't enable the option without knowing what they are doing. In that case, it looks good to me!

+++ b/jsonapi.routing.yml
@@ -1,2 +1,10 @@
+    _permission: 'administer site configuration'

note to self: this is 'restrict access': TRUE

1. +++ b/src/Routing/ReadOnlyModeMethodFilter.php
   @@ -0,0 +1,74 @@
   +      assert(count($supported_methods) > 0, 'JSON:API routes always have a method specified.');
   +
   +      $is_jsonapi_route = $route->hasDefault(Routes::JSON_API_ROUTE_FLAG_KEY);
   

   we assert before we check the JSON_API_ROUTE_FLAG_KEY? Looking at the parent implementation those without supported methods aren't removed.

2. +++ b/tests/src/Functional/FileUploadTest.php
   @@ -367,6 +379,7 @@ class FileUploadTest extends ResourceTestBase {
   +    $this->config('jsonapi.settings')->set('read_only', FALSE)->save(TRUE);
   
   @@ -405,6 +418,7 @@ class FileUploadTest extends ResourceTestBase {
   +    $this->config('jsonapi.settings')->set('read_only', FALSE)->save(TRUE);
   
   @@ -434,6 +448,7 @@ class FileUploadTest extends ResourceTestBase {
   +    $this->config('jsonapi.settings')->set('read_only', FALSE)->save(TRUE);
   

   does this warrant adding a utility method to the base class ... future you would approve when you have to refactor

Sorry to add to the pile.

My suggestion of the UI:

1. From:
   

   'Warning: If read operations alone cover your needs, you can choose to limit JSON:API to just that to reduce your exposure to potential security vulnerabilities in the adding, changing or removing of entities (content items, taxonomy terms, …) via JSON:API. <a href=":docs">Learn more about securing your site with JSON:API.</a>'
   

   to

   'Warning: You can choose to limit JSON:API to read-only mode if you only read from Drupal using JSON:API. That will reduce potential security risks in the adding, changing or removing of content (nodes, taxonomy terms, …) via JSON:API. <a href=":docs">Learn more about securing your site with JSON:API.</a>'
   

2. Make this description #states based and only appear when read & write is selected. Having a Warning on the secure option seems confusing.
3. From:
   Accept all JSON:API create, update, and delete operations
   to
   Accept all JSON:API read operations. Accept all create, update, and delete operations for content entities as well.

The description is getting too long for people to read. I'd really suggest going back to the description we hashed out last night because it needs to be short and declarative.

Yeah, marking NW, I don't even understand the current description when I read it. =/ It's a run-on sentence and contains so much terminology. (I'm fine with the new labels; they are clearer.) But the less text, the better.

At a minimum, let's provide screenshots of both versions of the description for UX review.

To be honest, I agree with @xjm that the description we had last night was a bit better. Less scary, easy to read, and understand. This does not feel like a blocking thing to me though, just a preference.

Warning: When writing is allowed, entities (content items, taxonomy terms, …) can be added, changed, or removed via JSON:API. <a href=":docs">Read more about securing your site with JSON:API.</a>

#56 the problem is that the shorter description did not explain all the nuances. In my opinion a security flag is not the place where you want to favor succinctness vs precision.

Thanks @e0ipso. I think the key thing from a usability perspective is that the handbook page is the way to properly explain the nuances, and we want people to go to the handbook page. Any UI description is too short to provide nuances and the current one is simultaneously too long and unfamiliar for site users to actually read.

The problem with the original description is that it just described a feature of JSON:API. It was like "Warning: this module works as designed." Additionally, it's incomplete. The score security release of a few weeks ago would have been mitigated by this read-only mode even though it had nothing to do with changing entities. The attack just exploited that code path. That's why I removed the entity CRUD language.

Let's try this:

Warning: Accepting only read operations will harden the security of your site. Learn more about security hardening here.

My proposal is sort of similar to the recent ones:

Warning: You should only enable write access if your site or contributed modules require it. Learn more about securing your site with JSON:API.

I pinged @jhodgdon and @clara for their UI text expertise.

I can +1 #63 (although I slightly prefer #62, but #63 is has more consensus building). The only part that is missing and we must communicate is that regardless of the option selected, no config entities can be mutated. This is very relevant and very important to this screen.

AFAICT #states only works for form items (the #type => something level), not for descriptions of form items.

Right, but there is nothing stopping us from making that description its own #type => 'html_tag' with that description. If that is something that you think makes sense to do.

Wow, lots of action during last night (in my time zone)! And in Slack, @xjm said the help text below the radios is now proposed to be

Warning: Only enable all operations if the application requires it. Learn more about securing sites with JSON:API.

so I'm reviewing that text (not the text in #64, which I think is not as good), the rest of the screenshot in #63, and the Security page that the help text links to at https://www.drupal.org/docs/8/modules/jsonapi/security-considerations -- I have a couple of thoughts...

a) I mostly like the text on the radios. It gets across that this is only the config page for JSON:API operations, and clearly states what the two options mean.

b) On the security page at the bottom, it says that if you use read-only mode, that "completely eliminates" security problems... but is that really true? What if reading data is a possible access violation? Maybe that sentence could be changed to say that it would eliminate security problems that could affect the content of your site.

c) Regarding the help text... I am a bit uncomfortable with "the application", and also with the proposal in #64 saying "your site or contributed modules". And also that the bit explaining about entities has been omitted. Actually, entities are not being mentioned at all.

d) See also https://www.drupal.org/docs/develop/user-interface-standards/interface-text -- we're supposed to use the word "site" not "Drupal" in general, so I think "application" is also not recommended? Also it says to avoid the word "entities", which is desirable, but also in Slack yesterday we discussed that these operations only apply to entities, and people wanted to get that across.

e) Yesterday in Slack there was discussion about a module that would restrict JSON:API permissions to only certain entity types. I don't see that mentioned on the Security page or in this text.

So... Maybe this would be better:

------
Allowed operations
* Accept only JSON:API entity read operations
* Accept all JSON:API entity create, read, update, and delete operations
Warning: Use read-only mode unless the site needs write operations on entities (content items, comments, ...). Learn more about securing sites with JSON:API.
------
And then also address (b) and (e) on the Security page?

#67 discussion makes sense to me, and I think the text is clear. +1. (Have not reviewed the code, only the UI screenshots.)

+1 from me as well!

@Wim Leers, Maybe we can spin off a separate issue for the docs page. I did specifically ask for @jhodgdon's feedback. :) It's not a bikeshed; it's docs gate.

#67 looks good to me!

LGTM too!

Follow up tag added for the docs, per #70 and the docs needing sec. team review.

Gah! I hate that tag.

Completely OT: Hm. Well, "followup" is not an actual word, so I can see how tag confusion would happen. :)

Reviewed the "after" screenshot from #67: https://www.drupal.org/files/issues/2019-03-13/Screenshot%202019-03-13%2...

Looks good...

1) Uses radios vs. checkboxes to make it clear what each option will do (UX pattern borrowed from private vs. public files)
2) Includes the warning about security implications, but tones it down, per @Dries and some others.

The wording of the options is quite technical. OTOH, it's also pretty technical page, and it's targeted at User 1 vs. a site builder/content author type.

One piece of feedback from the UX call that is not addressed here is the description on the overview page still is "Enable or disable JSON:API's read-only mode." which was cited as overly specific. OTOH, this seems to be a very deliberate choice, per #8, to keep this page as simple as possible.

Changing any of this is a string fix which can happen until RC so I'm not particularly fussed about it. The current version feels "shippable" to me, so +1 from me, as well. Thanks so much for the quick turnaround on this!

+++ b/src/Form/JsonApiSettingsForm.php
@@ -0,0 +1,60 @@
+        'r' => $this->t('Accept only JSON:API read operations.'),
+        'rw' => $this->t('Accept all JSON:API create, read, update, and delete operations.'),

I like this wording! Given this wording, I wonder if we want to change any of the following:

1. +++ b/config/schema/jsonapi.schema.yml
   @@ -0,0 +1,7 @@
   +      label: 'Read-only mode enabled'
   

   Where is this label used? Depending on the answer to that, perhaps it should be something like "Restrict JSON:API to only read operations"?

2. +++ b/jsonapi.links.menu.yml
   @@ -0,0 +1,5 @@
   +  description: "Enable or disable JSON:API's read-only mode."
   

   #74 mentions the UX feedback about making this less specific. However, many of the links on admin/config are very specific. For example, "Change site name, email address, slogan, default front page, and error pages.". But given the labels of the radio buttons, perhaps this could be something like "Configure whether to accept JSON:API write operations." or "Enable or disable JSON:API write operations."?

3. +++ b/src/Form/JsonApiSettingsForm.php
   @@ -0,0 +1,60 @@
   +      '#title' => $this->t('Allowed operations'),
   

   Should this be "Accepted operations"?

4. +++ b/src/Form/JsonApiSettingsForm.php
   @@ -0,0 +1,60 @@
   +      '#description' => $this->t('Warning: Only enable all operations if the site requires it. <a href=":docs">Learn more about securing your site with JSON:API.</a>', [':docs' => 'https://www.drupal.org/docs/8/modules/jsonapi/security-considerations']),
   

   Should we change "enable" to "accept"?

5. +++ b/src/Routing/ReadOnlyModeMethodFilter.php
   @@ -0,0 +1,78 @@
   +    throw new MethodNotAllowedHttpException(array_intersect($all_supported_methods, $read_only_methods), sprintf("JSON:API's read-only mode is enabled. Site administrators can enable writes at %s.", Url::fromRoute('jsonapi.settings')->setAbsolute()->toString(TRUE)->getGeneratedUrl()));
   

   Should this be changed to something like "JSON:API is configured to accept only read operations. Site administrators can enable write operations at %s."?

Anyway, this might have all been discussed in earlier comments (I haven't read through them all). I'm fine with punting these and/or other string changes to followups.

Other than potential string changes, #79 looks great to me, so back to RTBC.

One other followup we might want:

+++ b/src/Routing/ReadOnlyModeMethodFilter.php
@@ -0,0 +1,78 @@
+    throw new MethodNotAllowedHttpException(array_intersect($all_supported_methods, $read_only_methods), sprintf("JSON:API's read-only mode is enabled. Site administrators can enable writes at %s.", Url::fromRoute('jsonapi.settings')->setAbsolute()->toString(TRUE)->getGeneratedUrl()));

I suspect it's possible that this is thrown in a situation where you're in read-only mode and try to PATCH a config entity. But in that situation, enabling writes won't make it work (even with that enabled, you still can't patch config entities). So in that case, we're giving a misleading error message. Perhaps we need to adjust the error message based on whether we actually filtered out the requested method?

Oh wait, #82 might not be an issue, because in that case, core's method filter probably throws its exception first, so perhaps we're all good there.

I will commit this in about 1h. Any other string refinement will happen in a separate issue.

Please post any additional feedback before then. Anything missed here will be fixed in another commit if necessary.

Thanks! RTBC+1.

Ha! @Wim Leers beat me to it. However, I will carry on with my plan regardless.

This issue and its predecessor were very stressful. I realize that there are a myriad of factors at play, but I think that collaborator & volunteer feelings could have been better cared for. This is often my most important value in a contrib issue queue. Hence the tradition to credit everyone that bothered to comment in an issue (yes, we've given credit to people commenting dis sux!).

Another tradition is that if an issue is particularly strenuous we'll give extra commit credit. In this case I'd like everyone to pay attention to what Wim Leers did here. He considered every nit pick, had meetings dedicated to a radio button label, posted many patches and iterations, got woken up by a bad-timing landlord after 3h of sleep… and managed to keep his spirit up and constructive while all that happened.

I know that when this is in core I won't have the chance to do this anymore with JSON:API, so here it goes…

Ugh, while I was writing this review, y'all already committed this. Ooops. :/

---

Wow, step away from the fray for a few days and lots of progress! Huzzah. :)

Mostly +1 to RTBC, with a few nits/questions/concerns:

1. +++ b/src/Form/JsonApiSettingsForm.php
   @@ -0,0 +1,60 @@
   +      '#title' => $this->t('Allowed operations'),
   

   Already mentioned, but it seems weird to use "Allowed" in this label, since the setting doesn't actually determine if the operations are allowed or not. I guess this is into "follow-up string fixes before RC" territory now, since everyone's running out of steam to make any more changes. :/

2. +++ b/src/Form/JsonApiSettingsForm.php
   @@ -0,0 +1,60 @@
   +      '#description' => $this->t('Warning: Only enable all operations if the site requires it. <a href=":docs">Learn more about securing your site with JSON:API.</a>', [':docs' => 'https://www.drupal.org/docs/8/modules/jsonapi/security-considerations']),
   

   This is the only place "enable" appears on the entire form. Can we say: "Warning: Only accept all..." to match what the choices are? This seems friendly to everyone's concerns, and is a trivial fix. Perhaps still do-able before commit?

3. +++ b/src/Routing/ReadOnlyModeMethodFilter.php
   @@ -0,0 +1,78 @@
   +    $all_supported_methods = [];
   +    foreach ($collection->all() as $name => $route) {
   +      $all_supported_methods = array_merge($all_supported_methods, $route->getMethods());
   +    }
   +
   +    $collection = $this->inner->filter($collection, $request);
   +
   +    if (!$this->readOnlyModeIsEnabled) {
   +      return $collection;
   +    }
   

   Why do we do all the work to initialize $all_supported_methods if we're not in read-only mode? Shouldn't the check for read-only and the early return be the first thing this method does?

   Can't we then avoid iterating over the entire collection twice? Can't we be building up $all_supported_methods in the later foreach?

   In short: What's the point of remembering one of the "supported" methods from a route that was filtered out already?

4. +++ b/src/Routing/ReadOnlyModeMethodFilter.php
   @@ -0,0 +1,78 @@
   +      $is_read_only_route = empty(array_diff($supported_methods, $read_only_methods));
   +      if (!$is_read_only_route) {
   +        $collection->remove($name);
   +      }
   

   Not sure we need this $is_read_only_route variable at all. Why not just:

   if (!empty(array_diff(...))) {
     $collection->remove($name);
   }
   

   ?

Thanks!

p.s. Was going to ask "Why is this specific to JSON:API at all? What about REST?" and I see that's already at #3039687: Add an API read-only mode to Drupal core. ;)

Moving to Drupal core's issue queue.

I'm working on https://www.drupal.org/project/drupal/issues/3122113