From: Andy Allan Date: Sun, 24 Feb 2019 13:34:15 +0000 (+0100) Subject: Move the user api methods into a separate controller in the api namespace X-Git-Tag: live~2669^2~4 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/4b4c5aac2f6e338fb043cd4d2a031c4bd6ee1b05 Move the user api methods into a separate controller in the api namespace --- diff --git a/app/controllers/api/users_controller.rb b/app/controllers/api/users_controller.rb new file mode 100644 index 000000000..70ad93f65 --- /dev/null +++ b/app/controllers/api/users_controller.rb @@ -0,0 +1,66 @@ +module Api + class UsersController < ApplicationController + layout "site", :except => [:api_details] + + skip_before_action :verify_authenticity_token + before_action :disable_terms_redirect, :only => [:api_details] + before_action :authorize, :only => [:api_details, :api_gpx_files] + before_action :api_deny_access_handler + + authorize_resource + + before_action :check_api_readable + around_action :api_call_handle_error + before_action :lookup_user_by_id, :only => [:api_read] + + def api_read + if @user.visible? + render :action => :api_read, :content_type => "text/xml" + else + head :gone + end + end + + def api_details + @user = current_user + render :action => :api_read, :content_type => "text/xml" + end + + def api_users + raise OSM::APIBadUserInput, "The parameter users is required, and must be of the form users=id[,id[,id...]]" unless params["users"] + + ids = params["users"].split(",").collect(&:to_i) + + raise OSM::APIBadUserInput, "No users were given to search for" if ids.empty? + + @users = User.visible.find(ids) + + render :action => :api_users, :content_type => "text/xml" + end + + def api_gpx_files + doc = OSM::API.new.get_xml_doc + current_user.traces.reload.each do |trace| + doc.root << trace.to_xml_node + end + render :xml => doc.to_s + end + + private + + ## + # ensure that there is a "user" instance variable + def lookup_user_by_id + @user = User.find(params[:id]) + end + + ## + # + def disable_terms_redirect + # this is necessary otherwise going to the user terms page, when + # having not agreed already would cause an infinite redirect loop. + # it's .now so that this doesn't propagate to other pages. + flash.now[:skip_terms] = true + end + end +end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index d1a94b9f6..edb2b1df5 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,22 +1,17 @@ class UsersController < ApplicationController - layout "site", :except => [:api_details] + layout "site" - skip_before_action :verify_authenticity_token, :only => [:api_read, :api_users, :api_details, :api_gpx_files, :auth_success] - before_action :disable_terms_redirect, :only => [:terms, :save, :logout, :api_details] - before_action :authorize, :only => [:api_details, :api_gpx_files] - before_action :authorize_web, :except => [:api_read, :api_users, :api_details, :api_gpx_files] - before_action :set_locale, :except => [:api_read, :api_users, :api_details, :api_gpx_files] - before_action :api_deny_access_handler, :only => [:api_read, :api_users, :api_details, :api_gpx_files] + skip_before_action :verify_authenticity_token, :only => [:auth_success] + before_action :disable_terms_redirect, :only => [:terms, :save, :logout] + before_action :authorize_web + before_action :set_locale authorize_resource before_action :require_self, :only => [:account] - before_action :check_database_readable, :except => [:login, :api_read, :api_users, :api_details, :api_gpx_files] + before_action :check_database_readable, :except => [:login] before_action :check_database_writable, :only => [:new, :account, :confirm, :confirm_email, :lost_password, :reset_password, :go_public, :make_friend, :remove_friend] - before_action :check_api_readable, :only => [:api_read, :api_users, :api_details, :api_gpx_files] before_action :require_cookies, :only => [:new, :login, :confirm] - around_action :api_call_handle_error, :only => [:api_read, :api_users, :api_details, :api_gpx_files] - before_action :lookup_user_by_id, :only => [:api_read] before_action :lookup_user_by_name, :only => [:set_status, :delete] before_action :allow_thirdparty_images, :only => [:show, :account] @@ -376,39 +371,6 @@ class UsersController < ApplicationController end end - def api_read - if @user.visible? - render :action => :api_read, :content_type => "text/xml" - else - head :gone - end - end - - def api_details - @user = current_user - render :action => :api_read, :content_type => "text/xml" - end - - def api_users - raise OSM::APIBadUserInput, "The parameter users is required, and must be of the form users=id[,id[,id...]]" unless params["users"] - - ids = params["users"].split(",").collect(&:to_i) - - raise OSM::APIBadUserInput, "No users were given to search for" if ids.empty? - - @users = User.visible.find(ids) - - render :action => :api_users, :content_type => "text/xml" - end - - def api_gpx_files - doc = OSM::API.new.get_xml_doc - current_user.traces.reload.each do |trace| - doc.root << trace.to_xml_node - end - render :xml => doc.to_s - end - def show @user = User.find_by(:display_name => params[:display_name]) @@ -755,12 +717,6 @@ class UsersController < ApplicationController head :forbidden if params[:display_name] != current_user.display_name end - ## - # ensure that there is a "user" instance variable - def lookup_user_by_id - @user = User.find(params[:id]) - end - ## # ensure that there is a "user" instance variable def lookup_user_by_name diff --git a/app/views/users/_api_user.builder b/app/views/api/users/_api_user.builder similarity index 100% rename from app/views/users/_api_user.builder rename to app/views/api/users/_api_user.builder diff --git a/app/views/users/api_read.builder b/app/views/api/users/api_read.builder similarity index 100% rename from app/views/users/api_read.builder rename to app/views/api/users/api_read.builder diff --git a/app/views/users/api_users.builder b/app/views/api/users/api_users.builder similarity index 100% rename from app/views/users/api_users.builder rename to app/views/api/users/api_users.builder diff --git a/config/routes.rb b/config/routes.rb index 654dc48fd..d6e28b736 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -66,10 +66,10 @@ OpenStreetMap::Application.routes.draw do get "relations/search" => "api/search#search_relations" get "nodes/search" => "api/search#search_nodes" - get "user/:id" => "users#api_read", :id => /\d+/ - get "user/details" => "users#api_details" - get "user/gpx_files" => "users#api_gpx_files" - get "users" => "users#api_users", :as => :api_users + get "user/:id" => "api/users#api_read", :id => /\d+/ + get "user/details" => "api/users#api_details" + get "user/gpx_files" => "api/users#api_gpx_files" + get "users" => "api/users#api_users", :as => :api_users get "user/preferences" => "api/user_preferences#read" get "user/preferences/:preference_key" => "api/user_preferences#read_one" diff --git a/test/controllers/api/users_controller.rb b/test/controllers/api/users_controller.rb new file mode 100644 index 000000000..5483731db --- /dev/null +++ b/test/controllers/api/users_controller.rb @@ -0,0 +1,199 @@ +require "test_helper" + +module Api + class UsersControllerTest < ActionController::TestCase + def setup + stub_hostip_requests + end + + ## + # test all routes which lead to this controller + def test_routes + assert_routing( + { :path => "/api/0.6/user/1", :method => :get }, + { :controller => "api/users", :action => "api_read", :id => "1" } + ) + assert_routing( + { :path => "/api/0.6/user/details", :method => :get }, + { :controller => "api/users", :action => "api_details" } + ) + assert_routing( + { :path => "/api/0.6/user/gpx_files", :method => :get }, + { :controller => "api/users", :action => "api_gpx_files" } + ) + assert_routing( + { :path => "/api/0.6/users", :method => :get }, + { :controller => "api/users", :action => "api_users" } + ) + end + + def test_api_read + user = create(:user, :description => "test", :terms_agreed => Date.yesterday) + # check that a visible user is returned properly + get :api_read, :params => { :id => user.id } + assert_response :success + assert_equal "text/xml", response.content_type + + # check the data that is returned + assert_select "description", :count => 1, :text => "test" + assert_select "contributor-terms", :count => 1 do + assert_select "[agreed='true']" + end + assert_select "img", :count => 0 + assert_select "roles", :count => 1 do + assert_select "role", :count => 0 + end + assert_select "changesets", :count => 1 do + assert_select "[count='0']" + end + assert_select "traces", :count => 1 do + assert_select "[count='0']" + end + assert_select "blocks", :count => 1 do + assert_select "received", :count => 1 do + assert_select "[count='0'][active='0']" + end + assert_select "issued", :count => 0 + end + + # check that we aren't revealing private information + assert_select "contributor-terms[pd]", false + assert_select "home", false + assert_select "languages", false + assert_select "messages", false + + # check that a suspended user is not returned + get :api_read, :params => { :id => create(:user, :suspended).id } + assert_response :gone + + # check that a deleted user is not returned + get :api_read, :params => { :id => create(:user, :deleted).id } + assert_response :gone + + # check that a non-existent user is not returned + get :api_read, :params => { :id => 0 } + assert_response :not_found + end + + def test_api_details + user = create(:user, :description => "test", :terms_agreed => Date.yesterday, :home_lat => 12.1, :home_lon => 12.1, :languages => ["en"]) + create(:message, :read, :recipient => user) + create(:message, :sender => user) + + # check that nothing is returned when not logged in + get :api_details + assert_response :unauthorized + + # check that we get a response when logged in + basic_authorization user.email, "test" + get :api_details + assert_response :success + assert_equal "text/xml", response.content_type + + # check the data that is returned + assert_select "description", :count => 1, :text => "test" + assert_select "contributor-terms", :count => 1 do + assert_select "[agreed='true'][pd='false']" + end + assert_select "img", :count => 0 + assert_select "roles", :count => 1 do + assert_select "role", :count => 0 + end + assert_select "changesets", :count => 1 do + assert_select "[count='0']", :count => 1 + end + assert_select "traces", :count => 1 do + assert_select "[count='0']", :count => 1 + end + assert_select "blocks", :count => 1 do + assert_select "received", :count => 1 do + assert_select "[count='0'][active='0']" + end + assert_select "issued", :count => 0 + end + assert_select "home", :count => 1 do + assert_select "[lat='12.1'][lon='12.1'][zoom='3']" + end + assert_select "languages", :count => 1 do + assert_select "lang", :count => 1, :text => "en" + end + assert_select "messages", :count => 1 do + assert_select "received", :count => 1 do + assert_select "[count='1'][unread='0']" + end + assert_select "sent", :count => 1 do + assert_select "[count='1']" + end + end + end + + def test_api_users + user1 = create(:user, :description => "test1", :terms_agreed => Date.yesterday) + user2 = create(:user, :description => "test2", :terms_agreed => Date.yesterday) + user3 = create(:user, :description => "test3", :terms_agreed => Date.yesterday) + + get :api_users, :params => { :users => user1.id } + assert_response :success + assert_equal "text/xml", response.content_type + assert_select "user", :count => 1 do + assert_select "user[id='#{user1.id}']", :count => 1 + assert_select "user[id='#{user2.id}']", :count => 0 + assert_select "user[id='#{user3.id}']", :count => 0 + end + + get :api_users, :params => { :users => user2.id } + assert_response :success + assert_equal "text/xml", response.content_type + assert_select "user", :count => 1 do + assert_select "user[id='#{user1.id}']", :count => 0 + assert_select "user[id='#{user2.id}']", :count => 1 + assert_select "user[id='#{user3.id}']", :count => 0 + end + + get :api_users, :params => { :users => "#{user1.id},#{user3.id}" } + assert_response :success + assert_equal "text/xml", response.content_type + assert_select "user", :count => 2 do + assert_select "user[id='#{user1.id}']", :count => 1 + assert_select "user[id='#{user2.id}']", :count => 0 + assert_select "user[id='#{user3.id}']", :count => 1 + end + + get :api_users, :params => { :users => create(:user, :suspended).id } + assert_response :not_found + + get :api_users, :params => { :users => create(:user, :deleted).id } + assert_response :not_found + + get :api_users, :params => { :users => 0 } + assert_response :not_found + end + + def test_api_gpx_files + user = create(:user) + trace1 = create(:trace, :user => user) do |trace| + create(:tracetag, :trace => trace, :tag => "London") + end + trace2 = create(:trace, :user => user) do |trace| + create(:tracetag, :trace => trace, :tag => "Birmingham") + end + # check that nothing is returned when not logged in + get :api_gpx_files + assert_response :unauthorized + + # check that we get a response when logged in + basic_authorization user.email, "test" + get :api_gpx_files + assert_response :success + assert_equal "application/xml", response.content_type + + # check the data that is returned + assert_select "gpx_file[id='#{trace1.id}']", 1 do + assert_select "tag", "London" + end + assert_select "gpx_file[id='#{trace2.id}']", 1 do + assert_select "tag", "Birmingham" + end + end + end +end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index b85dcf65b..1b875ca97 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -8,23 +8,6 @@ class UsersControllerTest < ActionController::TestCase ## # test all routes which lead to this controller def test_routes - assert_routing( - { :path => "/api/0.6/user/1", :method => :get }, - { :controller => "users", :action => "api_read", :id => "1" } - ) - assert_routing( - { :path => "/api/0.6/user/details", :method => :get }, - { :controller => "users", :action => "api_details" } - ) - assert_routing( - { :path => "/api/0.6/user/gpx_files", :method => :get }, - { :controller => "users", :action => "api_gpx_files" } - ) - assert_routing( - { :path => "/api/0.6/users", :method => :get }, - { :controller => "users", :action => "api_users" } - ) - assert_routing( { :path => "/login", :method => :get }, { :controller => "users", :action => "login" } @@ -1123,175 +1106,6 @@ class UsersControllerTest < ActionController::TestCase end end - def test_api_read - user = create(:user, :description => "test", :terms_agreed => Date.yesterday) - # check that a visible user is returned properly - get :api_read, :params => { :id => user.id } - assert_response :success - assert_equal "text/xml", response.content_type - - # check the data that is returned - assert_select "description", :count => 1, :text => "test" - assert_select "contributor-terms", :count => 1 do - assert_select "[agreed='true']" - end - assert_select "img", :count => 0 - assert_select "roles", :count => 1 do - assert_select "role", :count => 0 - end - assert_select "changesets", :count => 1 do - assert_select "[count='0']" - end - assert_select "traces", :count => 1 do - assert_select "[count='0']" - end - assert_select "blocks", :count => 1 do - assert_select "received", :count => 1 do - assert_select "[count='0'][active='0']" - end - assert_select "issued", :count => 0 - end - - # check that we aren't revealing private information - assert_select "contributor-terms[pd]", false - assert_select "home", false - assert_select "languages", false - assert_select "messages", false - - # check that a suspended user is not returned - get :api_read, :params => { :id => create(:user, :suspended).id } - assert_response :gone - - # check that a deleted user is not returned - get :api_read, :params => { :id => create(:user, :deleted).id } - assert_response :gone - - # check that a non-existent user is not returned - get :api_read, :params => { :id => 0 } - assert_response :not_found - end - - def test_api_details - user = create(:user, :description => "test", :terms_agreed => Date.yesterday, :home_lat => 12.1, :home_lon => 12.1, :languages => ["en"]) - create(:message, :read, :recipient => user) - create(:message, :sender => user) - - # check that nothing is returned when not logged in - get :api_details - assert_response :unauthorized - - # check that we get a response when logged in - basic_authorization user.email, "test" - get :api_details - assert_response :success - assert_equal "text/xml", response.content_type - - # check the data that is returned - assert_select "description", :count => 1, :text => "test" - assert_select "contributor-terms", :count => 1 do - assert_select "[agreed='true'][pd='false']" - end - assert_select "img", :count => 0 - assert_select "roles", :count => 1 do - assert_select "role", :count => 0 - end - assert_select "changesets", :count => 1 do - assert_select "[count='0']", :count => 1 - end - assert_select "traces", :count => 1 do - assert_select "[count='0']", :count => 1 - end - assert_select "blocks", :count => 1 do - assert_select "received", :count => 1 do - assert_select "[count='0'][active='0']" - end - assert_select "issued", :count => 0 - end - assert_select "home", :count => 1 do - assert_select "[lat='12.1'][lon='12.1'][zoom='3']" - end - assert_select "languages", :count => 1 do - assert_select "lang", :count => 1, :text => "en" - end - assert_select "messages", :count => 1 do - assert_select "received", :count => 1 do - assert_select "[count='1'][unread='0']" - end - assert_select "sent", :count => 1 do - assert_select "[count='1']" - end - end - end - - def test_api_users - user1 = create(:user, :description => "test1", :terms_agreed => Date.yesterday) - user2 = create(:user, :description => "test2", :terms_agreed => Date.yesterday) - user3 = create(:user, :description => "test3", :terms_agreed => Date.yesterday) - - get :api_users, :params => { :users => user1.id } - assert_response :success - assert_equal "text/xml", response.content_type - assert_select "user", :count => 1 do - assert_select "user[id='#{user1.id}']", :count => 1 - assert_select "user[id='#{user2.id}']", :count => 0 - assert_select "user[id='#{user3.id}']", :count => 0 - end - - get :api_users, :params => { :users => user2.id } - assert_response :success - assert_equal "text/xml", response.content_type - assert_select "user", :count => 1 do - assert_select "user[id='#{user1.id}']", :count => 0 - assert_select "user[id='#{user2.id}']", :count => 1 - assert_select "user[id='#{user3.id}']", :count => 0 - end - - get :api_users, :params => { :users => "#{user1.id},#{user3.id}" } - assert_response :success - assert_equal "text/xml", response.content_type - assert_select "user", :count => 2 do - assert_select "user[id='#{user1.id}']", :count => 1 - assert_select "user[id='#{user2.id}']", :count => 0 - assert_select "user[id='#{user3.id}']", :count => 1 - end - - get :api_users, :params => { :users => create(:user, :suspended).id } - assert_response :not_found - - get :api_users, :params => { :users => create(:user, :deleted).id } - assert_response :not_found - - get :api_users, :params => { :users => 0 } - assert_response :not_found - end - - def test_api_gpx_files - user = create(:user) - trace1 = create(:trace, :user => user) do |trace| - create(:tracetag, :trace => trace, :tag => "London") - end - trace2 = create(:trace, :user => user) do |trace| - create(:tracetag, :trace => trace, :tag => "Birmingham") - end - # check that nothing is returned when not logged in - get :api_gpx_files - assert_response :unauthorized - - # check that we get a response when logged in - basic_authorization user.email, "test" - get :api_gpx_files - assert_response :success - assert_equal "application/xml", response.content_type - - # check the data that is returned - assert_select "gpx_file[id='#{trace1.id}']", 1 do - assert_select "tag", "London" - end - assert_select "gpx_file[id='#{trace2.id}']", 1 do - assert_select "tag", "Birmingham" - end - end - def test_make_friend # Get users to work with user = create(:user)