Controller/routing cleanup: trailing-slash middleware, admin_only! helper, test settings isolation

Gemfile.lock

@@ -66,7 +66,7 @@ GIT
 
 GIT
   remote: https://github.com/ncbo/ontologies_linked_data.git
-  revision: 24c5a6d0a9de0ec6510d2615cc2e98c93c370a4e
+  revision: c6f83eba9c21513c60977f93f93c6f08af958921
   branch: develop
   specs:
     ontologies_linked_data (0.0.1)

app.rb

@@ -28,6 +28,7 @@
 require_relative 'lib/rack/param_translator'
 require_relative 'lib/rack/slice_detection'
 require_relative 'lib/rack/request_lang'
+require_relative 'lib/rack/trailing_slash_redirect'
 
 # Logging setup
 require_relative "config/logging"
@@ -172,6 +173,10 @@
   require_relative 'config/unicorn_workerkiller'
 end
 
+# Canonicalize trailing slashes (301/308 redirect) before Sinatra routing.
+# Innermost middleware: runs after auth/CORS but before any route or filter.
+use Rack::TrailingSlashRedirect
+
 # Add New Relic last to allow Rack middleware instrumentation
 require 'newrelic_rpm'
 

controllers/annotator_controller.rb

@@ -103,13 +103,13 @@ def process_annotation(params=nil)
     end
 
     post '/dictionary' do
-      error 403, "Access denied" unless current_user && current_user.admin?
+      admin_only!
       annotator = Annotator::Models::NcboAnnotator.new
       annotator.generate_dictionary_file()
     end
 
     post '/cache' do
-      error 403, "Access denied" unless current_user && current_user.admin?
+      admin_only!
       delete_cache = params['delete_cache'].eql?('true')
       annotator = Annotator::Models::NcboAnnotator.new
       annotator.create_term_cache(nil, delete_cache)

controllers/slices_controller.rb

@@ -17,20 +17,20 @@ class SlicesController < ApplicationController
     ##
     # Create a new slice
     post do
-      error 403, "Access denied" unless current_user && current_user.admin?
+      admin_only!
       create_slice
     end
 
     # Delete a slice
     delete '/:slice' do
-      error 403, "Access denied" unless current_user && current_user.admin?
+      admin_only!
       LinkedData::Models::Slice.find(params[:slice]).first.delete
       halt 204
     end
 
     # Update an existing slice
     patch '/:slice' do
-      error 403, "Access denied" unless current_user && current_user.admin?
+      admin_only!
       slice = LinkedData::Models::Slice.find(params[:slice]).include(LinkedData::Models::Slice.attributes(:all)).first
       populate_from_params(slice, params)
       if slice.valid?
@@ -44,7 +44,7 @@ class SlicesController < ApplicationController
 
     # Check to make sure each group has a corresponding slice (and ontologies match)
     get '/synchronize_groups' do
-      error 403, "Access denied" unless current_user && current_user.admin?
+      admin_only!
 
       groups = LinkedData::Models::Group.where.include(LinkedData::Models::Group.attributes(:all)).all
       groups.each do |g|

controllers/users_controller.rb

@@ -105,7 +105,7 @@ class UsersController < ApplicationController
 
     # Delete a user
     delete '/:username' do
-      error 403, "Access denied" unless current_user.admin?
+      admin_only!
       User.find(params[:username]).first.delete
       halt 204
     end

helpers/application_helper.rb

@@ -471,6 +471,12 @@ def current_user
         env["REMOTE_USER"] || LinkedData::Models::User.new
       end
 
+      ##
+      # Halt with 403 unless the current user is an administrator
+      def admin_only!
+        error 403, "Access denied" unless current_user && current_user.admin?
+      end
+
       def include_param_contains?(str)
         str = str.to_s unless str.is_a?(String)
         class_params_include = params["include_for_class"] && params["include_for_class"].include?(str.to_sym)

init.rb

@@ -10,38 +10,5 @@ def require_dir(dir)
 require_dir('models')
 require_dir('controllers')
 
-# Add optional trailing slash to routes
-Sinatra.register do
-  def self.registered(app)
-    app.routes.each do |verb, routes|
-      routes.each do |route|
-        pattern = route[0]
-        next if pattern.to_s.end_with?('/')
-
-        http_verb = verb.to_s.downcase
-        app.public_send(http_verb, "#{pattern}/") do
-          pass unless request.path_info.end_with?('/')
-          redirect_path = request.path_info.chomp('/')
-          redirect canonical_redirect_url(redirect_path), 301
-        end
-      end
-    end
-  end
-end
-
-helpers do
-  def canonical_redirect_url(path)
-    url = +"#{external_request_scheme}://#{request.host_with_port}#{path}"
-    url << "?#{request.query_string}" unless request.query_string.empty?
-    url
-  end
-
-  # Rack 3 no longer trusts X-Forwarded-Proto on `request.scheme`, so a
-  # TLS-terminating proxy that forwards to the app over HTTP would otherwise
-  # cause us to emit `Location: http://...` for an https:// request.
-  def external_request_scheme
-    forwarded = request.get_header('HTTP_X_FORWARDED_PROTO').to_s
-                       .split(',').first.to_s.strip.downcase
-    %w[http https].include?(forwarded) ? forwarded : request.scheme
-  end
-end
+# Trailing-slash canonicalization is handled by Rack::TrailingSlashRedirect
+# (see app.rb / lib/rack/trailing_slash_redirect.rb).

lib/rack/trailing_slash_redirect.rb

@@ -0,0 +1,54 @@
+module Rack
+  # Canonicalizes URLs by stripping a trailing slash and redirecting to the
+  # slash-less form (e.g. `GET /ontologies/` -> 301 `/ontologies`).
+  #
+  # Implemented as Rack middleware so the redirect happens before Sinatra
+  # routing and before any namespace `before` filters run -- routes therefore
+  # only ever see canonical paths, and no per-route trailing-slash handling is
+  # needed.
+  class TrailingSlashRedirect
+    def initialize(app)
+      @app = app
+    end
+
+    def call(env)
+      # PATH_INFO and QUERY_STRING are required by the Rack SPEC: always
+      # present, possibly empty, never nil.
+      path = env['PATH_INFO']
+      return @app.call(env) unless path.length > 1 && path.end_with?('/')
+
+      req = Rack::Request.new(env)
+      # PATH_INFO keeps percent-encoding (e.g. %2F in class IRIs), so building
+      # the location from it round-trips encoded segments unchanged.
+      location = +"#{external_scheme(env, req)}://#{req.host_with_port}#{path.chomp('/')}"
+      query = env['QUERY_STRING']
+      location << "?#{query}" unless query.empty?
+
+      # 301 may turn POST -> GET and drop the body; 308 preserves method + body.
+      status = %w[GET HEAD].include?(env['REQUEST_METHOD']) ? 301 : 308
+      # A HEAD response must not carry a body.
+      body = env['REQUEST_METHOD'] == 'HEAD' ? [] : ['Moved Permanently']
+      # The Location's scheme depends on X-Forwarded-Proto, but an outer
+      # Rack::Cache keys entries on path+query only. Without no-store it could
+      # serve a cached `Location: https://...` to a plain-http client (or vice
+      # versa). Keep the scheme-dependent redirect out of the shared cache.
+      headers = {
+        'location' => location,
+        'content-type' => 'text/plain',
+        'cache-control' => 'no-store'
+      }
+      [status, headers, body]
+    end
+
+    private
+
+    # Rack 3 no longer trusts X-Forwarded-Proto on `request.scheme`, so a
+    # TLS-terminating proxy that forwards to the app over HTTP would otherwise
+    # cause us to emit `Location: http://...` for an https:// request. Use the
+    # leftmost forwarded value (RFC 7239 chained proxies), else fall back.
+    def external_scheme(env, req)
+      forwarded = env['HTTP_X_FORWARDED_PROTO'].to_s.split(',').first.to_s.strip.downcase
+      %w[http https].include?(forwarded) ? forwarded : req.scheme
+    end
+  end
+end

test/controllers/test_admin_only_endpoints.rb

@@ -0,0 +1,58 @@
+require_relative '../test_case'
+
+# Verifies the `admin_only!` authorization gate (helpers/application_helper.rb):
+# every endpoint guarded by it must return 403 for an authenticated non-admin.
+# `admin_only!` is the first line of each guarded handler, so it halts before
+# any endpoint work runs -- no valid payload or target resource is required.
+#
+# The "admin is allowed through" path is left to the existing endpoint tests
+# (e.g. test_slices_controller, test_users_controller); it is not re-exercised
+# here to avoid their side effects (slice/user mutation, annotator dictionary
+# and cache rebuilds).
+class TestAdminOnlyEndpoints < TestCase
+
+  # [http verb, path] for every reachable handler that calls `admin_only!`.
+  #
+  # GET /slices/synchronize_groups also calls admin_only! but is unreachable --
+  # the earlier `get '/:slice_id'` route shadows it -- so it is omitted here.
+  ADMIN_ONLY_ENDPOINTS = [
+    [:post,   "/slices"],
+    [:patch,  "/slices/any"],
+    [:delete, "/slices/any"],
+    [:delete, "/users/any"],
+    [:post,   "/annotator/dictionary"],
+    [:post,   "/annotator/cache"]
+  ].freeze
+
+  def before_suite
+    self.class.delete_user("test-admin-gate")
+    @@user = self.class.create_user("test-admin-gate")
+  end
+
+  def after_suite
+    self.class.delete_user("test-admin-gate")
+  end
+
+  def setup
+    # Reset the role with security off: user.save runs a write permission check
+    # when security is on, which denies outside an authenticated request. The
+    # request under test enables security itself.
+    with_settings(enable_security: false) do
+      self.class.reset_to_not_admin(@@user)
+    end
+  end
+
+  def test_admin_only_endpoints_forbidden_for_non_admin
+    with_settings(enable_security: true) do
+      ADMIN_ONLY_ENDPOINTS.each do |verb, path|
+        send(verb, "#{path}?apikey=#{@@user.apikey}")
+
+        assert_equal 403, last_response.status,
+                     "expected 403 for #{verb.upcase} #{path} as a non-admin user, " \
+                     "got #{last_response.status}: #{last_response.body}"
+        assert_match(/access denied/i, last_response.body,
+                     "expected an 'Access denied' body for #{verb.upcase} #{path}")
+      end
+    end
+  end
+end

test/controllers/test_annotator_controller.rb

@@ -291,15 +291,14 @@ def test_recognizer_endpoint
     rest_recognizers = MultiJson.load(last_response.body)
     assert rest_recognizers.length > 0
 
-    default_rec_setting = Annotator.settings.supported_recognizers
-    Annotator.settings.supported_recognizers = recognizers
-
-    get "/annotator/recognizers"
-    assert last_response.ok?
-    rest_recognizers = MultiJson.load(last_response.body)
-    assert rest_recognizers.length > 0
-    assert_equal recognizers.length, rest_recognizers.length
-    assert_equal recognizers.sort, rest_recognizers.map {|r| r.to_sym}.sort
+    with_settings(Annotator.settings, supported_recognizers: recognizers) do
+      get "/annotator/recognizers"
+      assert last_response.ok?
+      rest_recognizers = MultiJson.load(last_response.body)
+      assert rest_recognizers.length > 0
+      assert_equal recognizers.length, rest_recognizers.length
+      assert_equal recognizers.sort, rest_recognizers.map {|r| r.to_sym}.sort
+    end
   end
 
   #TODO: this method is duplicated in NCBO_ANNOTATOR

test/controllers/test_ontologies_controller.rb

@@ -170,20 +170,15 @@ def test_download_ontology
     assert_equal(1, onts.length, msg="Failed to create 1 ontology?")
     ont = onts.first
     assert_instance_of(Ontology, ont, msg="ont is not a #{Ontology.class}")
-    # Clear restrictions on downloads
-    LinkedData::OntologiesAPI.settings.restrict_download = []
     # Download the latest submission (the generic ontology download)
     acronym = created_ont_acronyms.first
     get "/ontologies/#{acronym}/download"
     assert_equal(200, last_response.status, msg='failed download for ontology : ' + get_errors(last_response))
-    # Add restriction on download
-    LinkedData::OntologiesAPI.settings.restrict_download = [acronym]
-    # Try download
-    get "/ontologies/#{acronym}/download"
-    # download should fail with a 403 status
-    assert_equal(403, last_response.status, msg='failed to restrict download for ontology : ' + get_errors(last_response))
-    # Clear restrictions on downloads
-    LinkedData::OntologiesAPI.settings.restrict_download = []
+    # With the acronym restricted, the same download should fail with a 403.
+    with_settings(LinkedData::OntologiesAPI.settings, restrict_download: [acronym]) do
+      get "/ontologies/#{acronym}/download"
+      assert_equal(403, last_response.status, msg='failed to restrict download for ontology : ' + get_errors(last_response))
+    end
     # see also test_ontologies_submissions_controller::test_download_submission
   end
 
@@ -237,20 +232,19 @@ def test_download_acl_only
       ont.viewingRestriction = "private"
       ont.save
 
-      LinkedData.settings.enable_security = true
-
-      get "/ontologies/#{acronym}/download?apikey=#{allowed_user.apikey}"
-      assert_equal(200, last_response.status, msg="User who is in ACL couldn't download ontology")
+      with_settings(enable_security: true) do
+        get "/ontologies/#{acronym}/download?apikey=#{allowed_user.apikey}"
+        assert_equal(200, last_response.status, msg="User who is in ACL couldn't download ontology")
 
-      get "/ontologies/#{acronym}/download?apikey=#{blocked_user.apikey}"
-      assert_equal(403, last_response.status, msg="User who isn't in ACL could download ontology")
+        get "/ontologies/#{acronym}/download?apikey=#{blocked_user.apikey}"
+        assert_equal(403, last_response.status, msg="User who isn't in ACL could download ontology")
 
-      admin = ont.administeredBy.first
-      admin.bring(:apikey)
-      get "/ontologies/#{acronym}/download?apikey=#{admin.apikey}"
-      assert_equal(200, last_response.status, msg="Admin couldn't download ontology")
+        admin = ont.administeredBy.first
+        admin.bring(:apikey)
+        get "/ontologies/#{acronym}/download?apikey=#{admin.apikey}"
+        assert_equal(200, last_response.status, msg="Admin couldn't download ontology")
+      end
     ensure
-      LinkedData.settings.enable_security = false
       del = User.find("allowed").first
       del.delete if del
       del = User.find("blocked").first
@@ -276,23 +270,23 @@ def test_on_demand_ontology_pull
       start_server
       sub.pullLocation = RDF::IRI.new(@@server_url)
       sub.save
-      LinkedData.settings.enable_security = true
-      post "/ontologies/#{acronym}/pull?apikey=#{allowed_user.apikey}"
-      assert_equal(204, last_response.status, msg="The ontology admin was unable to execute the on-demand pull")
-
-      blocked_user = User.new({
-        username: "blocked",
-        email: "test@example.org",
-        password: "12345"
-      })
-      blocked_user.save
-      post "/ontologies/#{acronym}/pull?apikey=#{blocked_user.apikey}"
-      assert_equal(403, last_response.status, msg="An unauthorized user was able to execute the on-demand pull")
+      with_settings(enable_security: true) do
+        post "/ontologies/#{acronym}/pull?apikey=#{allowed_user.apikey}"
+        assert_equal(204, last_response.status, msg="The ontology admin was unable to execute the on-demand pull")
+
+        blocked_user = User.new({
+          username: "blocked",
+          email: "test@example.org",
+          password: "12345"
+        })
+        blocked_user.save
+        post "/ontologies/#{acronym}/pull?apikey=#{blocked_user.apikey}"
+        assert_equal(403, last_response.status, msg="An unauthorized user was able to execute the on-demand pull")
+      end
     ensure
       del = User.find("blocked").first
       del.delete if del
       stop_server
-      LinkedData.settings.enable_security = false
       del = User.find("blocked").first
       del.delete if del
     end