Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Enhancement to support fine-grained settings whitelisting #13948

Merged
merged 4 commits into from
Mar 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions app/controllers/api/settings_controller.rb
Original file line number Diff line number Diff line change
@@ -1,22 +1,36 @@
module Api
class SettingsController < BaseController
def index
render_resource :settings, slice_settings(exposed_settings)
render_resource :settings, whitelist_settings(settings_hash)
end

def show
raise NotFoundError, "Settings category #{@req.c_id} not found" unless exposed_settings.include?(@req.c_id)
render_resource :settings, slice_settings(@req.c_id)
settings_value = entry_value(whitelist_settings(settings_hash), @req.c_suffix)

raise NotFoundError, "Settings entry #{@req.c_suffix} not found" if settings_value.nil?
render_resource :settings, settings_entry_to_hash(@req.c_suffix, settings_value)
end

private

def exposed_settings
ApiConfig.collections[:settings][:categories]
def whitelist_settings(settings)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems (a) sufficiently complicated, and (b) already abstracted away from details of the request to warrant a service object that could be perhaps better tested in isolation. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is coded as it's needed as such by a following PR. Some of this code will be moving to an API Settings mixin, will be used by servers, regions and zones.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow. Is there a problem with delegating?

result_hash = {}
ApiConfig.collections[:settings][:categories].each do |category_path|
result_hash.deep_merge!(settings_entry_to_hash(category_path, entry_value(settings, category_path)))
end
result_hash
end

def settings_hash
@settings_hash ||= Settings.to_hash.deep_stringify_keys
end

def entry_value(settings, path)
settings.fetch_path(path.split('/'))
end

def slice_settings(keys)
::Settings.to_hash.slice(*Array(keys).collect(&:to_sym))
def settings_entry_to_hash(path, value)
{}.tap { |h| h.store_path(path.split("/"), value) }
end
end
end
107 changes: 107 additions & 0 deletions spec/requests/api/settings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,111 @@
expect(response).to have_http_status(:not_found)
end
end

context "Fine-Grained Settings Queries" do
let(:sample_settings) do
JSON.parse('{
"product": {
"maindb": "ExtManagementSystem",
"container_deployment_wizard": false,
"datawarehouse_manager": false
},
"server": {
"role": "database_operations,event,reporting,scheduler,smartstate,ems_operations,ems_inventory,user_interface,websocket,web_services,automate",
"worker_monitor": {
"kill_algorithm": {
"name": "used_swap_percent_gt_value",
"value": 80
},
"miq_server_time_threshold": "2.minutes",
"nice_delta": 1,
"poll": "2.seconds",
"sync_interval": "30.minutes",
"wait_for_started_timeout": "10.minutes"
}
},
"authentication": {
"bind_timeout": 30,
"follow_referrals": false,
"get_direct_groups": true,
"group_memberships_max_depth": 2,
"ldapport": "389",
"mode": "database",
"search_timeout": 30,
"user_type": "userprincipalname"
}
}')
end

# TODO: as the api.yml settings/categories expand in the future to include
# different types of categories, full, partial and single entries, we would no
# longer need this stubbing logic or the above sample_settings as we can
# test the different cases with the default api.yml categories provided.
#
def stub_api_settings_categories(value)
settings_config = Api::ApiConfig.collections["settings"].dup
settings_config["categories"] = value
allow(Api::ApiConfig.collections).to receive("settings") { settings_config }
end

before do
stub_settings_merge(sample_settings)
end

it "supports multiple categories" do
stub_api_settings_categories(%w(product authentication server))
api_basic_authorize action_identifier(:settings, :read, :resource_actions, :get)

run_get settings_url

expect(response.parsed_body).to match(
"product" => sample_settings["product"],
"authentication" => sample_settings["authentication"],
"server" => sample_settings["server"]
)
end

it "supports partial categories" do
stub_api_settings_categories(%w(product server/role))
api_basic_authorize action_identifier(:settings, :read, :resource_actions, :get)

run_get settings_url

expect(response.parsed_body).to match(
"product" => sample_settings["product"],
"server" => { "role" => sample_settings["server"]["role"] }
)
end

it "supports second level partial categories" do
stub_api_settings_categories(%w(product server/role server/worker_monitor/sync_interval))
api_basic_authorize action_identifier(:settings, :read, :resource_actions, :get)

run_get settings_url

expect(response.parsed_body).to match(
"product" => sample_settings["product"],
"server" => {
"role" => sample_settings["server"]["role"],
"worker_monitor" => { "sync_interval" => sample_settings["server"]["worker_monitor"]["sync_interval"] }
}
)
end

it "supports multiple and partial categories" do
stub_api_settings_categories(%w(product server/role server/worker_monitor authentication))
api_basic_authorize action_identifier(:settings, :read, :resource_actions, :get)

run_get settings_url

expect(response.parsed_body).to match(
"product" => sample_settings["product"],
"server" => {
"role" => sample_settings["server"]["role"],
"worker_monitor" => sample_settings["server"]["worker_monitor"],
},
"authentication" => sample_settings["authentication"]
)
end
end
end