Ensure permissions are correctly enforced (#1072)

It turns out we were lazy about a lot of permissions in the API since we had a small set of users and nobody with `view` permissions didn't also have `annotate`. Now that we are enabling public access, that's a problem! This makes sure we're checking appropriate permissions in all the API controllers and actions.

This also does a little work to differentiate requests with no credentials and invalid credentials, which is important now that we are enabling public view access (no credentials is OK, invalid credentials is obviously an error, but needs an API-style response rather than the default authentication error handling). Devise doesn't provide anything that differentiates those, so we have to drop down to check some Warden data.

Found as part of auditing API access and options in #1070.

app/controllers/api/v0/annotations_controller.rb

@@ -1,6 +1,6 @@
 class Api::V0::AnnotationsController < Api::V0::ApiController
   include SortingConcern
-  before_action { authorize :api, :annotate? }
+  before_action(only: [:create]) { authorize :api, :annotate? }
   before_action :set_annotation, only: [:show]
 
   def index
@@ -12,7 +12,7 @@ def index
       links: paging[:links],
       meta: paging[:meta],
       data: annotations.as_json(
-        include: { author: { only: [:id, :email] } },
+        include: inclusions,
         except: :author_id
       )
     }
@@ -27,7 +27,7 @@ def show
         from_version: api_v0_page_version_url(page, @annotation.change.from_version)
       },
       data: @annotation.as_json(
-        include: { author: { only: [:id, :email] } },
+        include: inclusions,
         except: :author_id
       )
     }
@@ -66,4 +66,13 @@ def set_annotation
   def parent_change
     @change ||= Change.find_by_api_id(params[:change_id])
   end
+
+  def inclusions
+    # Author info may be have e-mails; only allow other logged-in users to see.
+    if current_user.present?
+      { author: { only: [:id, :email] } }
+    else
+      {}
+    end
+  end
 end

app/controllers/api/v0/api_controller.rb

@@ -2,6 +2,7 @@ class Api::V0::ApiController < ApplicationController
   skip_forgery_protection
 
   include PagingConcern
+  before_action :validate_credentials!
   before_action { authorize :api, :view? }
   before_action :set_environment_header
 
@@ -18,6 +19,23 @@ def paging_url_format
     ''
   end
 
+  # Allow unauthenticated requests, but raise an error if invalid credentials
+  # are sent. (Devise does not provide a method that differentiates between
+  # these cases.)
+  # We also want to provide a nice API-style error response, but Devise's
+  # `authenticate_user!` exits this controller entirely and goes to a separate,
+  # global "failure_app" Rack handler when it fails.
+  def validate_credentials!
+    # Force Devise/Warden to try and authenticate.
+    user_signed_in?
+
+    # It seems like we have to drop down to warden to figure out whether there
+    # was actually a failure or there was no authentication info sent.
+    # `warden.result` will be one of nil (no auth), :failure, :success.
+    # `warden.errors` does not get populated in, so use our own message.
+    raise Api::AuthorizationError, 'Invalid credentials.' if warden.result == :failure
+  end
+
   def pundit_auth_error(error)
     api_error = if user_signed_in?
                   Api::ForbiddenError.new('You are not authorized to perform this action.')

app/controllers/api/v0/maintainers_controller.rb

@@ -1,6 +1,8 @@
 class Api::V0::MaintainersController < Api::V0::ApiController
   include SortingConcern
 
+  before_action(except: [:index, :show]) { authorize(:api, :annotate?) }
+
   def index
     query = maintainer_collection
     paging = pagination(query)

app/controllers/api/v0/tags_controller.rb

@@ -1,6 +1,8 @@
 class Api::V0::TagsController < Api::V0::ApiController
   include SortingConcern
 
+  before_action(except: [:index, :show]) { authorize(:api, :annotate?) }
+
   def index
     query = tag_collection
     paging = pagination(query)

app/controllers/api/v0/urls_controller.rb

@@ -1,4 +1,6 @@
 class Api::V0::UrlsController < Api::V0::ApiController
+  before_action(except: [:index, :show]) { authorize(:api, :import?) }
+
   def index
     urls = page.urls.order('page_urls.to_time DESC')
 

app/controllers/api/v0/versions_controller.rb

@@ -122,6 +122,8 @@ def raw
   end
 
   def create
+    authorize(:api, :import?)
+
     # TODO: unify this with import code in ImportVersionsJob#import_record
     @version = page.versions.new(version_params)
 

test/controllers/api/v0/annotations_controller_test.rb

@@ -3,30 +3,31 @@
 class Api::V0::AnnotationsControllerTest < ActionDispatch::IntegrationTest
   include Devise::Test::IntegrationHelpers
 
-  test 'authorizations' do
+  test 'can only list annotations without auth if configured' do
     page = pages(:home_page)
     annotations(:annotation1).touch
-    get(
-      api_v0_page_change_annotations_url(
-        page,
-        changes(:page1_change_1_2),
-        params: { sort: 'updated_at:asc' }
-      )
-    )
-    assert_response(:unauthorized)
-
-    user = users(:alice)
-    user.update permissions: (user.permissions - [User::ANNOTATE_PERMISSION])
-    sign_in user
 
-    get(
-      api_v0_page_change_annotations_url(
-        page,
-        changes(:page1_change_1_2),
-        params: { sort: 'updated_at:asc' }
+    with_rails_configuration(:allow_public_view, true) do
+      get(
+        api_v0_page_change_annotations_url(
+          page,
+          changes(:page1_change_1_2),
+          params: { sort: 'updated_at:asc' }
+        )
       )
-    )
-    assert_response(:forbidden)
+      assert_response :success
+    end
+
+    with_rails_configuration(:allow_public_view, false) do
+      get(
+        api_v0_page_change_annotations_url(
+          page,
+          changes(:page1_change_1_2),
+          params: { sort: 'updated_at:asc' }
+        )
+      )
+      assert_response :unauthorized
+    end
   end
 
   test 'can annotate a version' do

test/controllers/api/v0/maintainers_controller_test.rb

@@ -3,9 +3,16 @@
 class Api::V0::MaintainersControllerTest < ActionDispatch::IntegrationTest
   include Devise::Test::IntegrationHelpers
 
-  test 'cannot list maintainers without auth' do
-    get api_v0_maintainers_path
-    assert_response :unauthorized
+  test 'can only list maintainers without auth if configured' do
+    with_rails_configuration(:allow_public_view, true) do
+      get api_v0_maintainers_path
+      assert_response :success
+    end
+
+    with_rails_configuration(:allow_public_view, false) do
+      get api_v0_maintainers_path
+      assert_response :unauthorized
+    end
   end
 
   test 'can list maintainers' do
@@ -94,7 +101,18 @@ class Api::V0::MaintainersControllerTest < ActionDispatch::IntegrationTest
     assert_equal(maintainers(:someone).uuid, body['data'][0]['uuid'])
   end
 
+  test 'adding a maintainer requires annotate permissions' do
+    user = users(:alice)
+    user.update permissions: (user.permissions - [User::ANNOTATE_PERMISSION])
+    sign_in user
 
+    post(
+      api_v0_maintainers_path,
+      as: :json,
+      params: { name: 'EPA' }
+    )
+    assert_response(:forbidden)
+  end
 
   test 'can add a maintainer to a page' do
     sign_in users(:alice)
@@ -208,6 +226,19 @@ class Api::V0::MaintainersControllerTest < ActionDispatch::IntegrationTest
     assert_response(:bad_request)
   end
 
+  test 'editing a maintainer requires annotate permissions' do
+    user = users(:alice)
+    user.update permissions: (user.permissions - [User::ANNOTATE_PERMISSION])
+    sign_in user
+
+    patch(
+      api_v0_maintainer_path(maintainers(:epa)),
+      as: :json,
+      params: { name: 'epa' }
+    )
+    assert_response(:forbidden)
+  end
+
   test 'can edit a maintainer' do
     sign_in users(:alice)
     patch(
@@ -220,6 +251,16 @@ class Api::V0::MaintainersControllerTest < ActionDispatch::IntegrationTest
     assert_equal('epa', body['data']['name'])
   end
 
+  test 'deleting a maintainer from a page requires annotate permissions' do
+    user = users(:alice)
+    user.update permissions: (user.permissions - [User::ANNOTATE_PERMISSION])
+    sign_in user
+
+    pages(:home_page).add_maintainer(maintainers(:epa))
+    delete(api_v0_page_maintainer_path(pages(:home_page), maintainers(:epa)))
+    assert_response(:forbidden)
+  end
+
   test 'can delete a maintainer from a page' do
     pages(:home_page).add_maintainer(maintainers(:epa))
 

test/controllers/api/v0/pages_controller_test.rb

@@ -3,9 +3,16 @@
 class Api::V0::PagesControllerTest < ActionDispatch::IntegrationTest
   include Devise::Test::IntegrationHelpers
 
-  test 'cannot list pages without auth' do
-    get '/api/v0/pages/'
-    assert_response :unauthorized
+  test 'can only list pages without auth if configured' do
+    with_rails_configuration(:allow_public_view, true) do
+      get api_v0_pages_path
+      assert_response :success
+    end
+
+    with_rails_configuration(:allow_public_view, false) do
+      get api_v0_pages_path
+      assert_response :unauthorized
+    end
   end
 
   test 'can list pages' do

test/controllers/api/v0/tags_controller_test.rb

@@ -3,9 +3,16 @@
 class Api::V0::TagsControllerTest < ActionDispatch::IntegrationTest
   include Devise::Test::IntegrationHelpers
 
-  test 'cannot list tags without auth' do
-    get api_v0_tags_path
-    assert_response :unauthorized
+  test 'can only list maintainers without auth if configured' do
+    with_rails_configuration(:allow_public_view, true) do
+      get api_v0_tags_path
+      assert_response :success
+    end
+
+    with_rails_configuration(:allow_public_view, false) do
+      get api_v0_tags_path
+      assert_response :unauthorized
+    end
   end
 
   test 'can list tags' do
@@ -57,6 +64,19 @@ class Api::V0::TagsControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
+  test 'adding a tag requires annotate permissions' do
+    user = users(:alice)
+    user.update permissions: (user.permissions - [User::ANNOTATE_PERMISSION])
+    sign_in user
+
+    post(
+      api_v0_page_tags_path(pages(:home_page)),
+      as: :json,
+      params: { name: 'Page of wonderment' }
+    )
+    assert_response(:forbidden)
+  end
+
   test 'can add a tag to a page' do
     sign_in users(:alice)
     post(
@@ -125,6 +145,19 @@ class Api::V0::TagsControllerTest < ActionDispatch::IntegrationTest
     assert_response(:bad_request)
   end
 
+  test 'editing a tag requires annotate permissions' do
+    user = users(:alice)
+    user.update permissions: (user.permissions - [User::ANNOTATE_PERMISSION])
+    sign_in user
+
+    patch(
+      api_v0_tag_path(tags(:site_whatever)),
+      as: :json,
+      params: { name: 'site:wherever' }
+    )
+    assert_response(:forbidden)
+  end
+
   test 'can edit a tag' do
     sign_in users(:alice)
     patch(
@@ -137,6 +170,16 @@ class Api::V0::TagsControllerTest < ActionDispatch::IntegrationTest
     assert_equal('site:wherever', body['data']['name'])
   end
 
+  test 'deleting a tag from a page requires annotate permissions' do
+    user = users(:alice)
+    user.update permissions: (user.permissions - [User::ANNOTATE_PERMISSION])
+    sign_in user
+
+    pages(:home_page).add_tag(tags(:site_whatever))
+    delete(api_v0_page_tag_path(pages(:home_page), tags(:site_whatever)))
+    assert_response(:forbidden)
+  end
+
   test 'can delete a tag from a page' do
     pages(:home_page).add_tag(tags(:site_whatever))