From a371f1097a57da442405cf55c7675a5e1df6394d Mon Sep 17 00:00:00 2001 From: Nick Muerdter Date: Sun, 23 Apr 2017 13:22:18 -0600 Subject: [PATCH] Fix empty "regex" field in API sub settings causing problems. Similar to issue for rewrites in https://github.com/NREL/api-umbrella/issues/360 Add missing validations for the regex field, and update proxy to deal more gracefully with any existing backends where this is missing. --- .../app/components/apis/sub-settings-form.js | 4 +- .../admin-ui/app/models/api/sub-settings.js | 14 +++- .../components/apis/sub-settings-table.hbs | 1 + .../proxy/middleware/api_settings.lua | 2 +- .../web-app/app/models/api/sub_settings.rb | 2 + .../v1/apis/test_save_rewrite_validations.rb | 76 +----------------- .../test_save_sub_settings_validations.rb | 61 +++++++++++++++ test/proxy/test_sub_settings.rb | 74 ++++++++++++++++++ .../api_save_validations.rb | 78 +++++++++++++++++++ 9 files changed, 234 insertions(+), 78 deletions(-) create mode 100644 test/apis/v1/apis/test_save_sub_settings_validations.rb create mode 100644 test/proxy/test_sub_settings.rb create mode 100644 test/support/api_umbrella_shared_tests/api_save_validations.rb diff --git a/src/api-umbrella/admin-ui/app/components/apis/sub-settings-form.js b/src/api-umbrella/admin-ui/app/components/apis/sub-settings-form.js index 42082ce3c..a53c46fb8 100644 --- a/src/api-umbrella/admin-ui/app/components/apis/sub-settings-form.js +++ b/src/api-umbrella/admin-ui/app/components/apis/sub-settings-form.js @@ -1,5 +1,6 @@ import Ember from 'ember'; import BufferedProxy from 'ember-buffered-proxy/proxy'; +import SubSettings from 'api-umbrella-admin-ui/models/api/sub-settings'; export default Ember.Component.extend({ openModal: false, @@ -25,7 +26,8 @@ export default Ember.Component.extend({ }), bufferedModel: Ember.computed('model', function() { - return BufferedProxy.create({ content: this.get('model') }); + let owner = Ember.getOwner(this).ownerInjection(); + return BufferedProxy.extend(SubSettings.validationClass).create(owner, { content: this.get('model') }); }), actions: { diff --git a/src/api-umbrella/admin-ui/app/models/api/sub-settings.js b/src/api-umbrella/admin-ui/app/models/api/sub-settings.js index 1903c6ddc..e9b5a2586 100644 --- a/src/api-umbrella/admin-ui/app/models/api/sub-settings.js +++ b/src/api-umbrella/admin-ui/app/models/api/sub-settings.js @@ -1,6 +1,16 @@ import DS from 'ember-data'; +import { validator, buildValidations } from 'ember-cp-validations'; -export default DS.Model.extend({ +const Validations = buildValidations({ + httpMethod: [ + validator('presence', true), + ], + regex: [ + validator('presence', true), + ], +}); + +export default DS.Model.extend(Validations, { sortOrder: DS.attr('number'), httpMethod: DS.attr(), regex: DS.attr(), @@ -17,4 +27,6 @@ export default DS.Model.extend({ this.set('settings', this.get('store').createRecord('api/settings')); } }, +}).reopenClass({ + validationClass: Validations, }); diff --git a/src/api-umbrella/admin-ui/app/templates/components/apis/sub-settings-table.hbs b/src/api-umbrella/admin-ui/app/templates/components/apis/sub-settings-table.hbs index d9cc21d29..b242dbd62 100644 --- a/src/api-umbrella/admin-ui/app/templates/components/apis/sub-settings-table.hbs +++ b/src/api-umbrella/admin-ui/app/templates/components/apis/sub-settings-table.hbs @@ -5,6 +5,7 @@ HTTP Method URL Matcher + diff --git a/src/api-umbrella/proxy/middleware/api_settings.lua b/src/api-umbrella/proxy/middleware/api_settings.lua index 765e020ef..cb245d3e7 100644 --- a/src/api-umbrella/proxy/middleware/api_settings.lua +++ b/src/api-umbrella/proxy/middleware/api_settings.lua @@ -21,7 +21,7 @@ return function(api) local request_method = ngx.ctx.request_method local request_uri = ngx.ctx.request_uri for _, sub_settings in ipairs(api["sub_settings"]) do - if sub_settings["http_method"] == "any" or sub_settings["http_method"] == request_method then + if (sub_settings["http_method"] == "any" or sub_settings["http_method"] == request_method) and sub_settings["regex"] then local matches, match_err = ngx.re.match(request_uri, sub_settings["regex"], "ijo") if matches then local original_required_roles diff --git a/src/api-umbrella/web-app/app/models/api/sub_settings.rb b/src/api-umbrella/web-app/app/models/api/sub_settings.rb index 87b31ba0c..c65df64a3 100644 --- a/src/api-umbrella/web-app/app/models/api/sub_settings.rb +++ b/src/api-umbrella/web-app/app/models/api/sub_settings.rb @@ -13,6 +13,8 @@ class Api::SubSettings # Validations validates :http_method, :inclusion => { :in => %w(any GET POST PUT DELETE HEAD TRACE OPTIONS CONNECT PATCH) } + validates :regex, + :presence => true # Nested attributes accepts_nested_attributes_for :settings diff --git a/test/apis/v1/apis/test_save_rewrite_validations.rb b/test/apis/v1/apis/test_save_rewrite_validations.rb index 7c0a17009..c41865f31 100644 --- a/test/apis/v1/apis/test_save_rewrite_validations.rb +++ b/test/apis/v1/apis/test_save_rewrite_validations.rb @@ -2,6 +2,7 @@ class Test::Apis::V1::Apis::TestSaveRewriteValidations < Minitest::Test include ApiUmbrellaTestHelpers::AdminAuth + include ApiUmbrellaTestHelpers::ApiSaveValidations include ApiUmbrellaTestHelpers::Setup parallelize_me! @@ -97,79 +98,4 @@ def test_rejects_blank_backend_replacement ], }, ["rewrites[0].backend_replacement"]) end - - private - - def assert_valid(overrides) - assert_valid_create(overrides) - assert_valid_update(overrides) - end - - def assert_valid_create(overrides) - assert_valid_action(:create, overrides) - end - - def assert_valid_update(overrides) - assert_valid_action(:update, overrides) - end - - def assert_valid_action(action, overrides) - attributes = attributes_for(action).deep_merge(overrides.deep_stringify_keys) - - response = create_or_update(action, attributes) - if(action == :create) - assert_response_code(201, response) - elsif(action == :update) - assert_response_code(204, response) - end - end - - def assert_invalid(overrides, expected_error_fields) - assert_invalid_create(overrides, expected_error_fields) - assert_invalid_update(overrides, expected_error_fields) - end - - def assert_invalid_create(overrides, expected_error_fields) - assert_invalid_action(:create, overrides, expected_error_fields) - end - - def assert_invalid_update(overrides, expected_error_fields) - assert_invalid_action(:update, overrides, expected_error_fields) - end - - def assert_invalid_action(action, overrides, expected_error_fields) - attributes = attributes_for(action).deep_merge(overrides.deep_stringify_keys) - - response = create_or_update(action, attributes) - assert_response_code(422, response) - data = MultiJson.load(response.body) - assert_equal(["errors"], data.keys) - assert_equal(expected_error_fields.sort, data["errors"].keys.sort) - end - - def attributes_for(action) - if(action == :create) - FactoryGirl.attributes_for(:api).deep_stringify_keys - elsif(action == :update) - FactoryGirl.create(:api).serializable_hash - else - flunk("Unknown action: #{action.inspect}") - end - end - - def create_or_update(action, attributes) - if(action == :create) - Typhoeus.post("https://127.0.0.1:9081/api-umbrella/v1/apis.json", http_options.deep_merge(admin_token).deep_merge({ - :headers => { "Content-Type" => "application/json" }, - :body => MultiJson.dump(:api => attributes), - })) - elsif(action == :update) - Typhoeus.put("https://127.0.0.1:9081/api-umbrella/v1/apis/#{attributes["id"]}.json", http_options.deep_merge(admin_token).deep_merge({ - :headers => { "Content-Type" => "application/json" }, - :body => MultiJson.dump(:api => attributes), - })) - else - flunk("Unknown action: #{action.inspect}") - end - end end diff --git a/test/apis/v1/apis/test_save_sub_settings_validations.rb b/test/apis/v1/apis/test_save_sub_settings_validations.rb new file mode 100644 index 000000000..71ed91d3e --- /dev/null +++ b/test/apis/v1/apis/test_save_sub_settings_validations.rb @@ -0,0 +1,61 @@ +require_relative "../../../test_helper" + +class Test::Apis::V1::Apis::TestSaveSubSettingsValidations < Minitest::Test + include ApiUmbrellaTestHelpers::AdminAuth + include ApiUmbrellaTestHelpers::ApiSaveValidations + include ApiUmbrellaTestHelpers::Setup + parallelize_me! + + def setup + super + setup_server + end + + def test_accepts_valid_rewrite + assert_valid({ + :sub_settings => [ + FactoryGirl.attributes_for(:api_sub_setting), + ], + }) + end + + def test_rejects_null_http_method + assert_invalid({ + :sub_settings => [ + FactoryGirl.attributes_for(:api_sub_setting, :http_method => nil), + ], + }, ["sub_settings[0].http_method"]) + end + + def test_rejects_blank_http_method + assert_invalid({ + :sub_settings => [ + FactoryGirl.attributes_for(:api_sub_setting, :http_method => ""), + ], + }, ["sub_settings[0].http_method"]) + end + + def test_rejects_invalid_http_method + assert_invalid({ + :sub_settings => [ + FactoryGirl.attributes_for(:api_sub_setting, :http_method => "zzz"), + ], + }, ["sub_settings[0].http_method"]) + end + + def test_rejects_null_regex + assert_invalid({ + :sub_settings => [ + FactoryGirl.attributes_for(:api_sub_setting, :regex => nil), + ], + }, ["sub_settings[0].regex"]) + end + + def test_rejects_blank_regex + assert_invalid({ + :sub_settings => [ + FactoryGirl.attributes_for(:api_sub_setting, :regex => ""), + ], + }, ["sub_settings[0].regex"]) + end +end diff --git a/test/proxy/test_sub_settings.rb b/test/proxy/test_sub_settings.rb new file mode 100644 index 000000000..e344cf2c9 --- /dev/null +++ b/test/proxy/test_sub_settings.rb @@ -0,0 +1,74 @@ +require_relative "../test_helper" + +class Test::Proxy::TestSubSettings < Minitest::Test + include ApiUmbrellaTestHelpers::Setup + parallelize_me! + + def setup + super + setup_server + end + + def test_sub_settings + prepend_api_backends([ + { + :frontend_host => "127.0.0.1", + :backend_host => "127.0.0.1", + :servers => [{ :host => "127.0.0.1", :port => 9444 }], + :url_matches => [{ :frontend_prefix => "/#{unique_test_id}/", :backend_prefix => "/" }], + :sub_settings => [ + { + :http_method => "any", + :regex => "^/info/sub/", + :settings => { + :headers => [ + { :key => "X-Sub1", :value => "sub-value1" }, + ], + }, + }, + ], + }, + ]) do + response = Typhoeus.get("http://127.0.0.1:9080/#{unique_test_id}/info/sub/", http_options) + assert_response_code(200, response) + data = MultiJson.load(response.body) + assert_equal("sub-value1", data["headers"]["x-sub1"]) + end + end + + def test_ignores_invalid_sub_settings_without_regex + prepend_api_backends([ + { + :frontend_host => "127.0.0.1", + :backend_host => "127.0.0.1", + :servers => [{ :host => "127.0.0.1", :port => 9444 }], + :url_matches => [{ :frontend_prefix => "/#{unique_test_id}/", :backend_prefix => "/" }], + :sub_settings => [ + { + :http_method => "any", + :settings => { + :headers => [ + { :key => "X-Sub1", :value => "sub-value1" }, + ], + }, + }, + { + :http_method => "any", + :regex => "^/info/sub/", + :settings => { + :headers => [ + { :key => "X-Sub2", :value => "sub-value2" }, + ], + }, + }, + ], + }, + ]) do + response = Typhoeus.get("http://127.0.0.1:9080/#{unique_test_id}/info/sub/", http_options) + assert_response_code(200, response) + data = MultiJson.load(response.body) + assert_nil(data["headers"]["x-sub1"]) + assert_equal("sub-value2", data["headers"]["x-sub2"]) + end + end +end diff --git a/test/support/api_umbrella_shared_tests/api_save_validations.rb b/test/support/api_umbrella_shared_tests/api_save_validations.rb new file mode 100644 index 000000000..53811dc7c --- /dev/null +++ b/test/support/api_umbrella_shared_tests/api_save_validations.rb @@ -0,0 +1,78 @@ +module ApiUmbrellaTestHelpers + module ApiSaveValidations + private + + def assert_valid(overrides) + assert_valid_create(overrides) + assert_valid_update(overrides) + end + + def assert_valid_create(overrides) + assert_valid_action(:create, overrides) + end + + def assert_valid_update(overrides) + assert_valid_action(:update, overrides) + end + + def assert_valid_action(action, overrides) + attributes = attributes_for(action).deep_merge(overrides.deep_stringify_keys) + + response = create_or_update(action, attributes) + if(action == :create) + assert_response_code(201, response) + elsif(action == :update) + assert_response_code(204, response) + end + end + + def assert_invalid(overrides, expected_error_fields) + assert_invalid_create(overrides, expected_error_fields) + assert_invalid_update(overrides, expected_error_fields) + end + + def assert_invalid_create(overrides, expected_error_fields) + assert_invalid_action(:create, overrides, expected_error_fields) + end + + def assert_invalid_update(overrides, expected_error_fields) + assert_invalid_action(:update, overrides, expected_error_fields) + end + + def assert_invalid_action(action, overrides, expected_error_fields) + attributes = attributes_for(action).deep_merge(overrides.deep_stringify_keys) + + response = create_or_update(action, attributes) + assert_response_code(422, response) + data = MultiJson.load(response.body) + assert_equal(["errors"], data.keys) + assert_equal(expected_error_fields.sort, data["errors"].keys.sort) + end + + def attributes_for(action) + if(action == :create) + FactoryGirl.attributes_for(:api).deep_stringify_keys + elsif(action == :update) + FactoryGirl.create(:api).serializable_hash + else + flunk("Unknown action: #{action.inspect}") + end + end + + def create_or_update(action, attributes) + if(action == :create) + Typhoeus.post("https://127.0.0.1:9081/api-umbrella/v1/apis.json", http_options.deep_merge(admin_token).deep_merge({ + :headers => { "Content-Type" => "application/json" }, + :body => MultiJson.dump(:api => attributes), + })) + elsif(action == :update) + Typhoeus.put("https://127.0.0.1:9081/api-umbrella/v1/apis/#{attributes["id"]}.json", http_options.deep_merge(admin_token).deep_merge({ + :headers => { "Content-Type" => "application/json" }, + :body => MultiJson.dump(:api => attributes), + })) + else + flunk("Unknown action: #{action.inspect}") + end + end + end +end