Thoughts about API controllers

In https://github.com/openstreetmap/openstreetmap-website/issues/2057 we had a problem with authenticity tokens and tests. All authenticity token checks are disabled in tests, which is normal for rails tests and saves huge amounts of faff in the test suite. However, they should remain enabled for all real web requests, yet need be disabled for api requests. So if you get the skip_before_filters wrong, the test suite doesn't pick it up, leaving two potential problems:

An API method fails in production, because you forgot to skip the check for that method
A website POST endpoint is vulnerable to CSRF, because you are skipping that method by mistake

Because the skip_before_actions give either a list of includes or excludes, and none of this is tested, both failures are plausible e.g.

skip_before_action :verify_authenticity_token, :except => [:index] <- add a new "create" method for web requests, silent CSRF failure
skip_before_action :verify_authenticity_token, :only => [:api_read, :api_users, :api_details, :api_gpx_files, :auth_success] <- add a new "create" method for API requests, method will fail in production

The silent CSRF failures are the most insidious, so that argues against using skip...except. But all of this is gnarly, particularly for adding new features (like notes, changeset comments, issues) which could be done as part of a GSoC or other first-big-change situations.

API methods unification

For a long time I've planned to unify the API and normal web methods, and just respond differently using the same API methods. So for example, users#api_read should be combined with usersr#show since it's doing the same thing (viewing a user) and just respond to different formats e.g.

def show
  @user = User.find(params[:id])

  respond_to do |format|
    format.html
    format.js
    format.xml { render xml: @people }
  end
end

This would also allow us to kill off the browse controller, where e.g. browse#changeset and changesets#read combine into changesets#show

The problems with unified creates

However, I think this changes entirely when we consider POST/PUT requests. The CSRF stuff above shines some light into this. The way that API POSTs work is quite different from web posts. For the API we support HTTP Basic Auth and OAuth tokens, and remove the CSRF protections. For the website we use standard cookie-based auth and standard Rails CSRF protections.

Currently we have a mishmash of options - you can only create diary entries via the web, and notes only via the API (the website, almost unbelievably, has to create and sign oauth requests to itself to create a new note), and it would be good to allow both without having massive controllers doing create and api_create and complex before actions removing filters for some methods and adding filters to others.

So I'm starting to lean towards having a separate set of API controllers. These could inherit from a base api controller that skips the authenticity tokens, adds only API auth before_actions, adds api call handle errors filters and so on. Then the regular controllers can ignore all of that and focus on regular web-related auth filters.

Other work

I've looked around a bit to see if there's any conventions for mixed rails web-and-api apps, but I haven't found any. In the RFG 2018 PR there was an effort to split api controllers out from non-api controllers, but it doesn't come with much explanation and I was instinctively opposed (as per unification above). But now I'm coming around to the idea.

API 0.x

I think the last remaining thing to check is how we would handle multiple API versions running in parallel. I'm quite keen to get 0.7 out the door in 2019, with whatever we choose to do (e.g. json, more resourceful urls) and ignoring things that are nowhere near ready (areas). But I want to make sure that it's fairly logical to have e.g. a 0.7 notes controller that behaves differently from a 0.6 notes controller, without making a huge tangle. Or maybe it's best to push on with the separation now, and revisit 0.7 related matters closer to the time.

gravitystorm / openstreetmap-website