From 9186a6155c4a9bbfd60eddd50f3eb63db78fd07e Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Sun, 24 Feb 2019 12:47:26 +0100 Subject: [PATCH] Move the user preferences controller into the api namespace --- .rubocop_todo.yml | 2 +- .../api/user_preferences_controller.rb | 90 +++++++ .../user_preferences_controller.rb | 88 ------- config/routes.rb | 10 +- .../api/user_preferences_controller_test.rb | 246 ++++++++++++++++++ .../user_preferences_controller_test.rb | 244 ----------------- 6 files changed, 342 insertions(+), 338 deletions(-) create mode 100644 app/controllers/api/user_preferences_controller.rb delete mode 100644 app/controllers/user_preferences_controller.rb create mode 100644 test/controllers/api/user_preferences_controller_test.rb delete mode 100644 test/controllers/user_preferences_controller_test.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 35107e9b9..609e02bde 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -15,7 +15,7 @@ Lint/AssignmentInCondition: - 'app/controllers/notes_controller.rb' - 'app/controllers/traces_controller.rb' - 'app/controllers/users_controller.rb' - - 'app/controllers/user_preferences_controller.rb' + - 'app/controllers/api/user_preferences_controller.rb' - 'app/helpers/application_helper.rb' - 'app/helpers/browse_helper.rb' - 'app/helpers/browse_tags_helper.rb' diff --git a/app/controllers/api/user_preferences_controller.rb b/app/controllers/api/user_preferences_controller.rb new file mode 100644 index 000000000..82f6c6a4d --- /dev/null +++ b/app/controllers/api/user_preferences_controller.rb @@ -0,0 +1,90 @@ +# Update and read user preferences, which are arbitrayr key/val pairs +module Api + class UserPreferencesController < ApplicationController + skip_before_action :verify_authenticity_token + before_action :authorize + + authorize_resource + + around_action :api_call_handle_error + + ## + # return all the preferences as an XML document + def read + doc = OSM::API.new.get_xml_doc + + prefs = current_user.preferences + + el1 = XML::Node.new "preferences" + + prefs.each do |pref| + el1 << pref.to_xml_node + end + + doc.root << el1 + render :xml => doc.to_s + end + + ## + # return the value for a single preference + def read_one + pref = UserPreference.find([current_user.id, params[:preference_key]]) + + render :plain => pref.v.to_s + end + + # update the entire set of preferences + def update + old_preferences = current_user.preferences.each_with_object({}) do |preference, preferences| + preferences[preference.k] = preference + end + + new_preferences = {} + + doc = XML::Parser.string(request.raw_post, :options => XML::Parser::Options::NOERROR).parse + + doc.find("//preferences/preference").each do |pt| + if preference = old_preferences.delete(pt["k"]) + preference.v = pt["v"] + elsif new_preferences.include?(pt["k"]) + raise OSM::APIDuplicatePreferenceError, pt["k"] + else + preference = current_user.preferences.build(:k => pt["k"], :v => pt["v"]) + end + + new_preferences[preference.k] = preference + end + + old_preferences.each_value(&:delete) + + new_preferences.each_value(&:save!) + + render :plain => "" + end + + ## + # update the value of a single preference + def update_one + begin + pref = UserPreference.find([current_user.id, params[:preference_key]]) + rescue ActiveRecord::RecordNotFound + pref = UserPreference.new + pref.user = current_user + pref.k = params[:preference_key] + end + + pref.v = request.raw_post.chomp + pref.save! + + render :plain => "" + end + + ## + # delete a single preference + def delete_one + UserPreference.find([current_user.id, params[:preference_key]]).delete + + render :plain => "" + end + end +end diff --git a/app/controllers/user_preferences_controller.rb b/app/controllers/user_preferences_controller.rb deleted file mode 100644 index 915c847de..000000000 --- a/app/controllers/user_preferences_controller.rb +++ /dev/null @@ -1,88 +0,0 @@ -# Update and read user preferences, which are arbitrayr key/val pairs -class UserPreferencesController < ApplicationController - skip_before_action :verify_authenticity_token - before_action :authorize - - authorize_resource - - around_action :api_call_handle_error - - ## - # return all the preferences as an XML document - def read - doc = OSM::API.new.get_xml_doc - - prefs = current_user.preferences - - el1 = XML::Node.new "preferences" - - prefs.each do |pref| - el1 << pref.to_xml_node - end - - doc.root << el1 - render :xml => doc.to_s - end - - ## - # return the value for a single preference - def read_one - pref = UserPreference.find([current_user.id, params[:preference_key]]) - - render :plain => pref.v.to_s - end - - # update the entire set of preferences - def update - old_preferences = current_user.preferences.each_with_object({}) do |preference, preferences| - preferences[preference.k] = preference - end - - new_preferences = {} - - doc = XML::Parser.string(request.raw_post, :options => XML::Parser::Options::NOERROR).parse - - doc.find("//preferences/preference").each do |pt| - if preference = old_preferences.delete(pt["k"]) - preference.v = pt["v"] - elsif new_preferences.include?(pt["k"]) - raise OSM::APIDuplicatePreferenceError, pt["k"] - else - preference = current_user.preferences.build(:k => pt["k"], :v => pt["v"]) - end - - new_preferences[preference.k] = preference - end - - old_preferences.each_value(&:delete) - - new_preferences.each_value(&:save!) - - render :plain => "" - end - - ## - # update the value of a single preference - def update_one - begin - pref = UserPreference.find([current_user.id, params[:preference_key]]) - rescue ActiveRecord::RecordNotFound - pref = UserPreference.new - pref.user = current_user - pref.k = params[:preference_key] - end - - pref.v = request.raw_post.chomp - pref.save! - - render :plain => "" - end - - ## - # delete a single preference - def delete_one - UserPreference.find([current_user.id, params[:preference_key]]).delete - - render :plain => "" - end -end diff --git a/config/routes.rb b/config/routes.rb index a12490158..58158612e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -71,11 +71,11 @@ OpenStreetMap::Application.routes.draw do get "user/gpx_files" => "users#api_gpx_files" get "users" => "users#api_users", :as => :api_users - get "user/preferences" => "user_preferences#read" - get "user/preferences/:preference_key" => "user_preferences#read_one" - put "user/preferences" => "user_preferences#update" - put "user/preferences/:preference_key" => "user_preferences#update_one" - delete "user/preferences/:preference_key" => "user_preferences#delete_one" + get "user/preferences" => "api/user_preferences#read" + get "user/preferences/:preference_key" => "api/user_preferences#read_one" + put "user/preferences" => "api/user_preferences#update" + put "user/preferences/:preference_key" => "api/user_preferences#update_one" + delete "user/preferences/:preference_key" => "api/user_preferences#delete_one" post "gpx/create" => "traces#api_create" get "gpx/:id" => "traces#api_read", :id => /\d+/ diff --git a/test/controllers/api/user_preferences_controller_test.rb b/test/controllers/api/user_preferences_controller_test.rb new file mode 100644 index 000000000..99dad9597 --- /dev/null +++ b/test/controllers/api/user_preferences_controller_test.rb @@ -0,0 +1,246 @@ +require "test_helper" + +module Api + class UserPreferencesControllerTest < ActionController::TestCase + ## + # test all routes which lead to this controller + def test_routes + assert_routing( + { :path => "/api/0.6/user/preferences", :method => :get }, + { :controller => "api/user_preferences", :action => "read" } + ) + assert_routing( + { :path => "/api/0.6/user/preferences", :method => :put }, + { :controller => "api/user_preferences", :action => "update" } + ) + assert_routing( + { :path => "/api/0.6/user/preferences/key", :method => :get }, + { :controller => "api/user_preferences", :action => "read_one", :preference_key => "key" } + ) + assert_routing( + { :path => "/api/0.6/user/preferences/key", :method => :put }, + { :controller => "api/user_preferences", :action => "update_one", :preference_key => "key" } + ) + assert_routing( + { :path => "/api/0.6/user/preferences/key", :method => :delete }, + { :controller => "api/user_preferences", :action => "delete_one", :preference_key => "key" } + ) + end + + ## + # test read action + def test_read + # first try without auth + get :read + assert_response :unauthorized, "should be authenticated" + + # authenticate as a user with no preferences + basic_authorization create(:user).email, "test" + + # try the read again + get :read + assert_select "osm" do + assert_select "preferences", :count => 1 do + assert_select "preference", :count => 0 + end + end + + # authenticate as a user with preferences + user = create(:user) + user_preference = create(:user_preference, :user => user) + user_preference2 = create(:user_preference, :user => user) + basic_authorization user.email, "test" + + # try the read again + get :read + assert_response :success + assert_equal "application/xml", @response.content_type + assert_select "osm" do + assert_select "preferences", :count => 1 do + assert_select "preference", :count => 2 + assert_select "preference[k=\"#{user_preference.k}\"][v=\"#{user_preference.v}\"]", :count => 1 + assert_select "preference[k=\"#{user_preference2.k}\"][v=\"#{user_preference2.v}\"]", :count => 1 + end + end + end + + ## + # test read_one action + def test_read_one + user = create(:user) + create(:user_preference, :user => user, :k => "key", :v => "value") + + # try a read without auth + get :read_one, :params => { :preference_key => "key" } + assert_response :unauthorized, "should be authenticated" + + # authenticate as a user with preferences + basic_authorization user.email, "test" + + # try the read again + get :read_one, :params => { :preference_key => "key" } + assert_response :success + assert_equal "text/plain", @response.content_type + assert_equal "value", @response.body + + # try the read again for a non-existent key + get :read_one, :params => { :preference_key => "unknown_key" } + assert_response :not_found + end + + ## + # test update action + def test_update + user = create(:user) + create(:user_preference, :user => user, :k => "key", :v => "value") + create(:user_preference, :user => user, :k => "some_key", :v => "some_value") + + # try a put without auth + assert_no_difference "UserPreference.count" do + put :update, :body => "" + end + assert_response :unauthorized, "should be authenticated" + assert_equal "value", UserPreference.find([user.id, "key"]).v + assert_equal "some_value", UserPreference.find([user.id, "some_key"]).v + assert_raises ActiveRecord::RecordNotFound do + UserPreference.find([user.id, "new_key"]) + end + + # authenticate as a user with preferences + basic_authorization user.email, "test" + + # try the put again + assert_no_difference "UserPreference.count" do + put :update, :body => "" + end + assert_response :success + assert_equal "text/plain", @response.content_type + assert_equal "", @response.body + assert_equal "new_value", UserPreference.find([user.id, "key"]).v + assert_equal "value", UserPreference.find([user.id, "new_key"]).v + assert_raises ActiveRecord::RecordNotFound do + UserPreference.find([user.id, "some_key"]) + end + + # try a put with duplicate keys + assert_no_difference "UserPreference.count" do + put :update, :body => "" + end + assert_response :bad_request + assert_equal "text/plain", @response.content_type + assert_equal "Duplicate preferences with key key", @response.body + assert_equal "new_value", UserPreference.find([user.id, "key"]).v + + # try a put with invalid content + assert_no_difference "UserPreference.count" do + put :update, :body => "nonsense" + end + assert_response :bad_request + end + + ## + # test update_one action + def test_update_one + user = create(:user) + create(:user_preference, :user => user) + + # try a put without auth + assert_no_difference "UserPreference.count" do + put :update_one, :params => { :preference_key => "new_key" }, :body => "new_value" + end + assert_response :unauthorized, "should be authenticated" + assert_raises ActiveRecord::RecordNotFound do + UserPreference.find([user.id, "new_key"]) + end + + # authenticate as a user with preferences + basic_authorization user.email, "test" + + # try adding a new preference + assert_difference "UserPreference.count", 1 do + put :update_one, :params => { :preference_key => "new_key" }, :body => "new_value" + end + assert_response :success + assert_equal "text/plain", @response.content_type + assert_equal "", @response.body + assert_equal "new_value", UserPreference.find([user.id, "new_key"]).v + + # try changing the value of a preference + assert_no_difference "UserPreference.count" do + put :update_one, :params => { :preference_key => "new_key" }, :body => "newer_value" + end + assert_response :success + assert_equal "text/plain", @response.content_type + assert_equal "", @response.body + assert_equal "newer_value", UserPreference.find([user.id, "new_key"]).v + end + + ## + # test delete_one action + def test_delete_one + user = create(:user) + create(:user_preference, :user => user, :k => "key", :v => "value") + + # try a delete without auth + assert_no_difference "UserPreference.count" do + delete :delete_one, :params => { :preference_key => "key" } + end + assert_response :unauthorized, "should be authenticated" + assert_equal "value", UserPreference.find([user.id, "key"]).v + + # authenticate as a user with preferences + basic_authorization user.email, "test" + + # try the delete again + assert_difference "UserPreference.count", -1 do + get :delete_one, :params => { :preference_key => "key" } + end + assert_response :success + assert_equal "text/plain", @response.content_type + assert_equal "", @response.body + assert_raises ActiveRecord::RecordNotFound do + UserPreference.find([user.id, "key"]) + end + + # try the delete again for the same key + assert_no_difference "UserPreference.count" do + get :delete_one, :params => { :preference_key => "key" } + end + assert_response :not_found + assert_raises ActiveRecord::RecordNotFound do + UserPreference.find([user.id, "key"]) + end + end + + # Ensure that a valid access token with correct capabilities can be used to + # read preferences + def test_read_one_using_token + user = create(:user) + token = create(:access_token, :user => user, :allow_read_prefs => true) + create(:user_preference, :user => user, :k => "key", :v => "value") + + # Hack together an oauth request - an alternative would be to sign the request properly + @request.env["oauth.version"] = 1 + @request.env["oauth.strategies"] = [:token] + @request.env["oauth.token"] = token + + get :read_one, :params => { :preference_key => "key" } + assert_response :success + end + + # Ensure that a valid access token with incorrect capabilities can't be used + # to read preferences even, though the owner of that token could read them + # by other methods. + def test_read_one_using_token_fail + user = create(:user) + token = create(:access_token, :user => user, :allow_read_prefs => false) + create(:user_preference, :user => user, :k => "key", :v => "value") + @request.env["oauth.version"] = 1 + @request.env["oauth.strategies"] = [:token] + @request.env["oauth.token"] = token + + get :read_one, :params => { :preference_key => "key" } + assert_response :forbidden + end + end +end diff --git a/test/controllers/user_preferences_controller_test.rb b/test/controllers/user_preferences_controller_test.rb deleted file mode 100644 index 99b40d5f0..000000000 --- a/test/controllers/user_preferences_controller_test.rb +++ /dev/null @@ -1,244 +0,0 @@ -require "test_helper" - -class UserPreferencesControllerTest < ActionController::TestCase - ## - # test all routes which lead to this controller - def test_routes - assert_routing( - { :path => "/api/0.6/user/preferences", :method => :get }, - { :controller => "user_preferences", :action => "read" } - ) - assert_routing( - { :path => "/api/0.6/user/preferences", :method => :put }, - { :controller => "user_preferences", :action => "update" } - ) - assert_routing( - { :path => "/api/0.6/user/preferences/key", :method => :get }, - { :controller => "user_preferences", :action => "read_one", :preference_key => "key" } - ) - assert_routing( - { :path => "/api/0.6/user/preferences/key", :method => :put }, - { :controller => "user_preferences", :action => "update_one", :preference_key => "key" } - ) - assert_routing( - { :path => "/api/0.6/user/preferences/key", :method => :delete }, - { :controller => "user_preferences", :action => "delete_one", :preference_key => "key" } - ) - end - - ## - # test read action - def test_read - # first try without auth - get :read - assert_response :unauthorized, "should be authenticated" - - # authenticate as a user with no preferences - basic_authorization create(:user).email, "test" - - # try the read again - get :read - assert_select "osm" do - assert_select "preferences", :count => 1 do - assert_select "preference", :count => 0 - end - end - - # authenticate as a user with preferences - user = create(:user) - user_preference = create(:user_preference, :user => user) - user_preference2 = create(:user_preference, :user => user) - basic_authorization user.email, "test" - - # try the read again - get :read - assert_response :success - assert_equal "application/xml", @response.content_type - assert_select "osm" do - assert_select "preferences", :count => 1 do - assert_select "preference", :count => 2 - assert_select "preference[k=\"#{user_preference.k}\"][v=\"#{user_preference.v}\"]", :count => 1 - assert_select "preference[k=\"#{user_preference2.k}\"][v=\"#{user_preference2.v}\"]", :count => 1 - end - end - end - - ## - # test read_one action - def test_read_one - user = create(:user) - create(:user_preference, :user => user, :k => "key", :v => "value") - - # try a read without auth - get :read_one, :params => { :preference_key => "key" } - assert_response :unauthorized, "should be authenticated" - - # authenticate as a user with preferences - basic_authorization user.email, "test" - - # try the read again - get :read_one, :params => { :preference_key => "key" } - assert_response :success - assert_equal "text/plain", @response.content_type - assert_equal "value", @response.body - - # try the read again for a non-existent key - get :read_one, :params => { :preference_key => "unknown_key" } - assert_response :not_found - end - - ## - # test update action - def test_update - user = create(:user) - create(:user_preference, :user => user, :k => "key", :v => "value") - create(:user_preference, :user => user, :k => "some_key", :v => "some_value") - - # try a put without auth - assert_no_difference "UserPreference.count" do - put :update, :body => "" - end - assert_response :unauthorized, "should be authenticated" - assert_equal "value", UserPreference.find([user.id, "key"]).v - assert_equal "some_value", UserPreference.find([user.id, "some_key"]).v - assert_raises ActiveRecord::RecordNotFound do - UserPreference.find([user.id, "new_key"]) - end - - # authenticate as a user with preferences - basic_authorization user.email, "test" - - # try the put again - assert_no_difference "UserPreference.count" do - put :update, :body => "" - end - assert_response :success - assert_equal "text/plain", @response.content_type - assert_equal "", @response.body - assert_equal "new_value", UserPreference.find([user.id, "key"]).v - assert_equal "value", UserPreference.find([user.id, "new_key"]).v - assert_raises ActiveRecord::RecordNotFound do - UserPreference.find([user.id, "some_key"]) - end - - # try a put with duplicate keys - assert_no_difference "UserPreference.count" do - put :update, :body => "" - end - assert_response :bad_request - assert_equal "text/plain", @response.content_type - assert_equal "Duplicate preferences with key key", @response.body - assert_equal "new_value", UserPreference.find([user.id, "key"]).v - - # try a put with invalid content - assert_no_difference "UserPreference.count" do - put :update, :body => "nonsense" - end - assert_response :bad_request - end - - ## - # test update_one action - def test_update_one - user = create(:user) - create(:user_preference, :user => user) - - # try a put without auth - assert_no_difference "UserPreference.count" do - put :update_one, :params => { :preference_key => "new_key" }, :body => "new_value" - end - assert_response :unauthorized, "should be authenticated" - assert_raises ActiveRecord::RecordNotFound do - UserPreference.find([user.id, "new_key"]) - end - - # authenticate as a user with preferences - basic_authorization user.email, "test" - - # try adding a new preference - assert_difference "UserPreference.count", 1 do - put :update_one, :params => { :preference_key => "new_key" }, :body => "new_value" - end - assert_response :success - assert_equal "text/plain", @response.content_type - assert_equal "", @response.body - assert_equal "new_value", UserPreference.find([user.id, "new_key"]).v - - # try changing the value of a preference - assert_no_difference "UserPreference.count" do - put :update_one, :params => { :preference_key => "new_key" }, :body => "newer_value" - end - assert_response :success - assert_equal "text/plain", @response.content_type - assert_equal "", @response.body - assert_equal "newer_value", UserPreference.find([user.id, "new_key"]).v - end - - ## - # test delete_one action - def test_delete_one - user = create(:user) - create(:user_preference, :user => user, :k => "key", :v => "value") - - # try a delete without auth - assert_no_difference "UserPreference.count" do - delete :delete_one, :params => { :preference_key => "key" } - end - assert_response :unauthorized, "should be authenticated" - assert_equal "value", UserPreference.find([user.id, "key"]).v - - # authenticate as a user with preferences - basic_authorization user.email, "test" - - # try the delete again - assert_difference "UserPreference.count", -1 do - get :delete_one, :params => { :preference_key => "key" } - end - assert_response :success - assert_equal "text/plain", @response.content_type - assert_equal "", @response.body - assert_raises ActiveRecord::RecordNotFound do - UserPreference.find([user.id, "key"]) - end - - # try the delete again for the same key - assert_no_difference "UserPreference.count" do - get :delete_one, :params => { :preference_key => "key" } - end - assert_response :not_found - assert_raises ActiveRecord::RecordNotFound do - UserPreference.find([user.id, "key"]) - end - end - - # Ensure that a valid access token with correct capabilities can be used to - # read preferences - def test_read_one_using_token - user = create(:user) - token = create(:access_token, :user => user, :allow_read_prefs => true) - create(:user_preference, :user => user, :k => "key", :v => "value") - - # Hack together an oauth request - an alternative would be to sign the request properly - @request.env["oauth.version"] = 1 - @request.env["oauth.strategies"] = [:token] - @request.env["oauth.token"] = token - - get :read_one, :params => { :preference_key => "key" } - assert_response :success - end - - # Ensure that a valid access token with incorrect capabilities can't be used - # to read preferences even, though the owner of that token could read them - # by other methods. - def test_read_one_using_token_fail - user = create(:user) - token = create(:access_token, :user => user, :allow_read_prefs => false) - create(:user_preference, :user => user, :k => "key", :v => "value") - @request.env["oauth.version"] = 1 - @request.env["oauth.strategies"] = [:token] - @request.env["oauth.token"] = token - - get :read_one, :params => { :preference_key => "key" } - assert_response :forbidden - end -end -- 2.43.2