Skip to content

Commit bd94801

Browse files
committed
Merge develop into master
2 parents 46dcce4 + 39d00c2 commit bd94801

23 files changed

Lines changed: 463 additions & 206 deletions

Gemfile.lock

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ GIT
88

99
GIT
1010
remote: https://github.com/ncbo/goo.git
11-
revision: 4cfe719b32385e94583f58c0b57904b4e4c03e2f
11+
revision: be1300e100101118557d5d1b5b8079eaa8bb3e04
1212
branch: main
1313
specs:
1414
goo (0.0.2)
@@ -26,7 +26,7 @@ GIT
2626

2727
GIT
2828
remote: https://github.com/ncbo/ncbo_annotator.git
29-
revision: 2d689b10f1be3dc70e890b42c9a62aea8baba5f2
29+
revision: 1a5bab6a22f5dcfaf05a4e3d218e5ad4aed13bfc
3030
branch: master
3131
specs:
3232
ncbo_annotator (0.0.1)
@@ -37,7 +37,7 @@ GIT
3737

3838
GIT
3939
remote: https://github.com/ncbo/ncbo_cron.git
40-
revision: 62b9cd5678c8c2fd287cf42f55a6906eaf0276bd
40+
revision: 195fc402bd27827e3f8ef50d4b57ea5c69361b20
4141
branch: master
4242
specs:
4343
ncbo_cron (0.0.1)
@@ -55,7 +55,7 @@ GIT
5555

5656
GIT
5757
remote: https://github.com/ncbo/ncbo_ontology_recommender.git
58-
revision: 5b3f6c90cd5aa5ba0680979770e1d756fc546c72
58+
revision: 08e2cc45558b6e62ff42a4ba8e7f7c660e3247fb
5959
branch: master
6060
specs:
6161
ncbo_ontology_recommender (0.0.1)
@@ -66,7 +66,7 @@ GIT
6666

6767
GIT
6868
remote: https://github.com/ncbo/ontologies_linked_data.git
69-
revision: 4e1a2c81a96585ba7a440bf0e7f533ee2ee3c94c
69+
revision: 867ff0f5194c1593d82645792fb83b6c8725ab03
7070
branch: master
7171
specs:
7272
ontologies_linked_data (0.0.1)
@@ -77,12 +77,10 @@ GIT
7777
json
7878
libxml-ruby
7979
multi_json
80-
net-ftp
8180
oj
8281
omni_logger
8382
pony
8483
rack
85-
rack-test
8684
rsolr
8785
rubyzip (~> 3.0)
8886

app.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
require_relative 'lib/rack/param_translator'
2929
require_relative 'lib/rack/slice_detection'
3030
require_relative 'lib/rack/request_lang'
31+
require_relative 'lib/rack/trailing_slash_redirect'
3132

3233
# Logging setup
3334
require_relative "config/logging"
@@ -172,6 +173,10 @@
172173
require_relative 'config/unicorn_workerkiller'
173174
end
174175

176+
# Canonicalize trailing slashes (301/308 redirect) before Sinatra routing.
177+
# Innermost middleware: runs after auth/CORS but before any route or filter.
178+
use Rack::TrailingSlashRedirect
179+
175180
# Add New Relic last to allow Rack middleware instrumentation
176181
require 'newrelic_rpm'
177182

controllers/annotator_controller.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,13 +103,13 @@ def process_annotation(params=nil)
103103
end
104104

105105
post '/dictionary' do
106-
error 403, "Access denied" unless current_user && current_user.admin?
106+
admin_only!
107107
annotator = Annotator::Models::NcboAnnotator.new
108108
annotator.generate_dictionary_file()
109109
end
110110

111111
post '/cache' do
112-
error 403, "Access denied" unless current_user && current_user.admin?
112+
admin_only!
113113
delete_cache = params['delete_cache'].eql?('true')
114114
annotator = Annotator::Models::NcboAnnotator.new
115115
annotator.create_term_cache(nil, delete_cache)

controllers/slices_controller.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,20 @@ class SlicesController < ApplicationController
1717
##
1818
# Create a new slice
1919
post do
20-
error 403, "Access denied" unless current_user && current_user.admin?
20+
admin_only!
2121
create_slice
2222
end
2323

2424
# Delete a slice
2525
delete '/:slice' do
26-
error 403, "Access denied" unless current_user && current_user.admin?
26+
admin_only!
2727
LinkedData::Models::Slice.find(params[:slice]).first.delete
2828
halt 204
2929
end
3030

3131
# Update an existing slice
3232
patch '/:slice' do
33-
error 403, "Access denied" unless current_user && current_user.admin?
33+
admin_only!
3434
slice = LinkedData::Models::Slice.find(params[:slice]).include(LinkedData::Models::Slice.attributes(:all)).first
3535
populate_from_params(slice, params)
3636
if slice.valid?
@@ -44,7 +44,7 @@ class SlicesController < ApplicationController
4444

4545
# Check to make sure each group has a corresponding slice (and ontologies match)
4646
get '/synchronize_groups' do
47-
error 403, "Access denied" unless current_user && current_user.admin?
47+
admin_only!
4848

4949
groups = LinkedData::Models::Group.where.include(LinkedData::Models::Group.attributes(:all)).all
5050
groups.each do |g|

controllers/users_controller.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ class UsersController < ApplicationController
105105

106106
# Delete a user
107107
delete '/:username' do
108-
error 403, "Access denied" unless current_user.admin?
108+
admin_only!
109109
User.find(params[:username]).first.delete
110110
halt 204
111111
end

helpers/application_helper.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -471,6 +471,12 @@ def current_user
471471
env["REMOTE_USER"] || LinkedData::Models::User.new
472472
end
473473

474+
##
475+
# Halt with 403 unless the current user is an administrator
476+
def admin_only!
477+
error 403, "Access denied" unless current_user && current_user.admin?
478+
end
479+
474480
def include_param_contains?(str)
475481
str = str.to_s unless str.is_a?(String)
476482
class_params_include = params["include_for_class"] && params["include_for_class"].include?(str.to_sym)

init.rb

Lines changed: 2 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -10,38 +10,5 @@ def require_dir(dir)
1010
require_dir('models')
1111
require_dir('controllers')
1212

13-
# Add optional trailing slash to routes
14-
Sinatra.register do
15-
def self.registered(app)
16-
app.routes.each do |verb, routes|
17-
routes.each do |route|
18-
pattern = route[0]
19-
next if pattern.to_s.end_with?('/')
20-
21-
http_verb = verb.to_s.downcase
22-
app.public_send(http_verb, "#{pattern}/") do
23-
pass unless request.path_info.end_with?('/')
24-
redirect_path = request.path_info.chomp('/')
25-
redirect canonical_redirect_url(redirect_path), 301
26-
end
27-
end
28-
end
29-
end
30-
end
31-
32-
helpers do
33-
def canonical_redirect_url(path)
34-
url = +"#{external_request_scheme}://#{request.host_with_port}#{path}"
35-
url << "?#{request.query_string}" unless request.query_string.empty?
36-
url
37-
end
38-
39-
# Rack 3 no longer trusts X-Forwarded-Proto on `request.scheme`, so a
40-
# TLS-terminating proxy that forwards to the app over HTTP would otherwise
41-
# cause us to emit `Location: http://...` for an https:// request.
42-
def external_request_scheme
43-
forwarded = request.get_header('HTTP_X_FORWARDED_PROTO').to_s
44-
.split(',').first.to_s.strip.downcase
45-
%w[http https].include?(forwarded) ? forwarded : request.scheme
46-
end
47-
end
13+
# Trailing-slash canonicalization is handled by Rack::TrailingSlashRedirect
14+
# (see app.rb / lib/rack/trailing_slash_redirect.rb).
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
module Rack
2+
# Canonicalizes URLs by stripping a trailing slash and redirecting to the
3+
# slash-less form (e.g. `GET /ontologies/` -> 301 `/ontologies`).
4+
#
5+
# Implemented as Rack middleware so the redirect happens before Sinatra
6+
# routing and before any namespace `before` filters run -- routes therefore
7+
# only ever see canonical paths, and no per-route trailing-slash handling is
8+
# needed.
9+
class TrailingSlashRedirect
10+
def initialize(app)
11+
@app = app
12+
end
13+
14+
def call(env)
15+
# PATH_INFO and QUERY_STRING are required by the Rack SPEC: always
16+
# present, possibly empty, never nil.
17+
path = env['PATH_INFO']
18+
return @app.call(env) unless path.length > 1 && path.end_with?('/')
19+
20+
req = Rack::Request.new(env)
21+
# PATH_INFO keeps percent-encoding (e.g. %2F in class IRIs), so building
22+
# the location from it round-trips encoded segments unchanged.
23+
location = +"#{external_scheme(env, req)}://#{req.host_with_port}#{path.chomp('/')}"
24+
query = env['QUERY_STRING']
25+
location << "?#{query}" unless query.empty?
26+
27+
# 301 may turn POST -> GET and drop the body; 308 preserves method + body.
28+
status = %w[GET HEAD].include?(env['REQUEST_METHOD']) ? 301 : 308
29+
# A HEAD response must not carry a body.
30+
body = env['REQUEST_METHOD'] == 'HEAD' ? [] : ['Moved Permanently']
31+
# The Location's scheme depends on X-Forwarded-Proto, but an outer
32+
# Rack::Cache keys entries on path+query only. Without no-store it could
33+
# serve a cached `Location: https://...` to a plain-http client (or vice
34+
# versa). Keep the scheme-dependent redirect out of the shared cache.
35+
headers = {
36+
'location' => location,
37+
'content-type' => 'text/plain',
38+
'cache-control' => 'no-store'
39+
}
40+
[status, headers, body]
41+
end
42+
43+
private
44+
45+
# Rack 3 no longer trusts X-Forwarded-Proto on `request.scheme`, so a
46+
# TLS-terminating proxy that forwards to the app over HTTP would otherwise
47+
# cause us to emit `Location: http://...` for an https:// request. Use the
48+
# leftmost forwarded value (RFC 7239 chained proxies), else fall back.
49+
def external_scheme(env, req)
50+
forwarded = env['HTTP_X_FORWARDED_PROTO'].to_s.split(',').first.to_s.strip.downcase
51+
%w[http https].include?(forwarded) ? forwarded : req.scheme
52+
end
53+
end
54+
end
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
require_relative '../test_case'
2+
3+
# Verifies the `admin_only!` authorization gate (helpers/application_helper.rb):
4+
# every endpoint guarded by it must return 403 for an authenticated non-admin.
5+
# `admin_only!` is the first line of each guarded handler, so it halts before
6+
# any endpoint work runs -- no valid payload or target resource is required.
7+
#
8+
# The "admin is allowed through" path is left to the existing endpoint tests
9+
# (e.g. test_slices_controller, test_users_controller); it is not re-exercised
10+
# here to avoid their side effects (slice/user mutation, annotator dictionary
11+
# and cache rebuilds).
12+
class TestAdminOnlyEndpoints < TestCase
13+
14+
# [http verb, path] for every reachable handler that calls `admin_only!`.
15+
#
16+
# GET /slices/synchronize_groups also calls admin_only! but is unreachable --
17+
# the earlier `get '/:slice_id'` route shadows it -- so it is omitted here.
18+
ADMIN_ONLY_ENDPOINTS = [
19+
[:post, "/slices"],
20+
[:patch, "/slices/any"],
21+
[:delete, "/slices/any"],
22+
[:delete, "/users/any"],
23+
[:post, "/annotator/dictionary"],
24+
[:post, "/annotator/cache"]
25+
].freeze
26+
27+
def before_suite
28+
self.class.delete_user("test-admin-gate")
29+
@@user = self.class.create_user("test-admin-gate")
30+
end
31+
32+
def after_suite
33+
self.class.delete_user("test-admin-gate")
34+
end
35+
36+
def setup
37+
# Reset the role with security off: user.save runs a write permission check
38+
# when security is on, which denies outside an authenticated request. The
39+
# request under test enables security itself.
40+
with_settings(enable_security: false) do
41+
self.class.reset_to_not_admin(@@user)
42+
end
43+
end
44+
45+
def test_admin_only_endpoints_forbidden_for_non_admin
46+
with_settings(enable_security: true) do
47+
ADMIN_ONLY_ENDPOINTS.each do |verb, path|
48+
send(verb, "#{path}?apikey=#{@@user.apikey}")
49+
50+
assert_equal 403, last_response.status,
51+
"expected 403 for #{verb.upcase} #{path} as a non-admin user, " \
52+
"got #{last_response.status}: #{last_response.body}"
53+
assert_match(/access denied/i, last_response.body,
54+
"expected an 'Access denied' body for #{verb.upcase} #{path}")
55+
end
56+
end
57+
end
58+
end

test/controllers/test_annotator_controller.rb

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -291,15 +291,14 @@ def test_recognizer_endpoint
291291
rest_recognizers = MultiJson.load(last_response.body)
292292
assert rest_recognizers.length > 0
293293

294-
default_rec_setting = Annotator.settings.supported_recognizers
295-
Annotator.settings.supported_recognizers = recognizers
296-
297-
get "/annotator/recognizers"
298-
assert last_response.ok?
299-
rest_recognizers = MultiJson.load(last_response.body)
300-
assert rest_recognizers.length > 0
301-
assert_equal recognizers.length, rest_recognizers.length
302-
assert_equal recognizers.sort, rest_recognizers.map {|r| r.to_sym}.sort
294+
with_settings(Annotator.settings, supported_recognizers: recognizers) do
295+
get "/annotator/recognizers"
296+
assert last_response.ok?
297+
rest_recognizers = MultiJson.load(last_response.body)
298+
assert rest_recognizers.length > 0
299+
assert_equal recognizers.length, rest_recognizers.length
300+
assert_equal recognizers.sort, rest_recognizers.map {|r| r.to_sym}.sort
301+
end
303302
end
304303

305304
#TODO: this method is duplicated in NCBO_ANNOTATOR

0 commit comments

Comments
 (0)