Refactor usage of check_api_readable/writable

[gravitystorm]

The review of #4605 made me look more closely at the usage of check_api_readable and check_api_writable in existing controllers, and I realised that it can be a bit confusing.

The two helpers return true or false depending on api_status, which itself returns one of three states.

api_status	check_api_readable	check_api_writable
online	✔️ true	✔️ true
readonly	✔️ true	✖️ false
offline	✖️ false	✖️ false

Note that it's not possible (nor is it logical) for the api to be _writable but not _readable.

---

There are therefore two approaches that we could standardise on, either:

• Every method that needs some kind of API access either checks _readable or _writable, but not both
• Every method that needs write access checks both (read actions only check _readable, of course).

The first approach seems logical, but leads to long lists of :except exclusions e.g.

    before_action :check_api_writable, :only => [:create, :update, :delete]
    before_action :check_api_readable, :except => [:create, :update, :delete]

The second approach leads to only maintaining one list of actions, e.g.

    before_action :check_api_readable
    before_action :check_api_writable, :only => [:create, :update, :delete]

Technically this is redundant, since _writable also ensures that the api_status is in some kind of read mode. But I prefer this approach since it is easier to read and, I think, to maintain.

---

I also made a quick review of existing controllers, and it seems some are a bit of a mess:

class ChangesetCommentsController < ApiController
    before_action :check_api_writable
    before_action :check_api_readable, :except => [:create]

This makes no sense, if all actions need write access then why would one action be excluded from the readable check?

  class ChangesetsController < ApiController
    before_action :check_api_writable, :only => [:create, :update, :upload, :subscribe, :unsubscribe]
    before_action :check_api_readable, :except => [:index, :create, :update, :upload, :download, :subscribe, :unsubscribe]

Why does changesets#index and changesets#download not have any checks on the api_status being online/readonly? They aren't covered by either check.

I think both of these show the difficulties in trying to maintain :except lists.

[gravitystorm]

Fixed by #4859

[gravitystorm]

Just as a passing comment here - in #4859 I managed to extract check_api_readable into api_controller (since it applies by default to most controllers).

I thought about doing something similar with check_api_writable, since there's a frequent pattern of:

before_action :check_api_writable, :only => [:create, :update, :destroy]

I think this could apply by default to all api controllers - for those action names, you probably want to check the database is writeable by default.

What stands in the way is that many of our api controllers aren't (yet) refactored to use resourceful routing - they have idiosyncratic action names like :subscribe or :update_all. This is another place where resourceful routing refactoring, although a substantial amount of work, would prove useful in making the code easier to maintain and to build new features.