From 28839fd1504004bd10f30eeba3d7212c77adfbb4 Mon Sep 17 00:00:00 2001 From: Matt Amos Date: Mon, 17 Nov 2008 14:32:15 +0000 Subject: [PATCH] Fixed some bugs in changeset query code. Added more test cases. --- app/controllers/application.rb | 11 ++++- app/controllers/changeset_controller.rb | 20 +++++++--- lib/osm.rb | 3 ++ test/fixtures/changesets.yml | 9 ++++- test/functional/changeset_controller_test.rb | 42 +++++++++++++++++++- 5 files changed, 75 insertions(+), 10 deletions(-) diff --git a/app/controllers/application.rb b/app/controllers/application.rb index 579c50e95..f5ea0063d 100644 --- a/app/controllers/application.rb +++ b/app/controllers/application.rb @@ -22,7 +22,11 @@ class ApplicationController < ActionController::Base redirect_to :controller => 'user', :action => 'login', :referer => request.request_uri unless @user end - def authorize(realm='Web Password', errormessage="Couldn't authenticate you") + ## + # sets up the @user object for use by other methods. this is mostly called + # from the authorize method, but can be called elsewhere if authorisation + # is optional. + def setup_user_auth username, passwd = get_auth_data # parse from headers # authenticate per-scheme if username.nil? @@ -32,6 +36,11 @@ class ApplicationController < ActionController::Base else @user = User.authenticate(:username => username, :password => passwd) # basic auth end + end + + def authorize(realm='Web Password', errormessage="Couldn't authenticate you") + # make the @user object from any auth sources we have + setup_user_auth # handle authenticate pass/fail unless @user diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb index 1e6a44189..5a0be3588 100644 --- a/app/controllers/changeset_controller.rb +++ b/app/controllers/changeset_controller.rb @@ -236,9 +236,9 @@ class ChangesetController < ApplicationController # create the conditions that the user asked for. some or all of # these may be nil. conditions = conditions_bbox(params['bbox']) - cond_merge conditions, conditions_user(params['user']) - cond_merge conditions, conditions_time(params['time']) - cond_merge conditions, conditions_open(params['open']) + conditions = cond_merge conditions, conditions_user(params['user']) + conditions = cond_merge conditions, conditions_time(params['time']) + conditions = cond_merge conditions, conditions_open(params['open']) # create the results document results = OSM::API.new.get_xml_doc @@ -333,7 +333,15 @@ class ChangesetController < ApplicationController def conditions_user(user) unless user.nil? u = User.find(user.to_i) - raise OSM::APINotFoundError unless u.data_public? + # should be able to get changesets of public users only, or + # our own changesets regardless of public-ness. + unless u.data_public? + # get optional user auth stuff so that users can see their own + # changesets if they're non-public + setup_user_auth + + raise OSM::APINotFoundError if @user.nil? or @user.id != u.id + end return ['user_id = ?', u.id] else return nil @@ -347,11 +355,11 @@ class ChangesetController < ApplicationController # if there is a range, i.e: comma separated, then the first is # low, second is high - same as with bounding boxes. if time.count(',') == 1 - from, to = time.split(/,/).collect { |t| Date.parse(t) } + from, to = time.split(/,/).collect { |t| DateTime.parse(t) } return ['created_at > ? and created_at < ?', from, to] else # if there is no comma, assume its a lower limit on time - return ['created_at > ?', Date.parse(time)] + return ['created_at > ?', DateTime.parse(time)] end else return nil diff --git a/lib/osm.rb b/lib/osm.rb index c132ff25c..128e65aac 100644 --- a/lib/osm.rb +++ b/lib/osm.rb @@ -17,6 +17,9 @@ module OSM # Raised when an API object is not found. class APINotFoundError < APIError + def render_opts + { :nothing => true, :status => :not_found } + end end # Raised when a precondition to an API action fails sanity check. diff --git a/test/fixtures/changesets.yml b/test/fixtures/changesets.yml index 2047af8d5..ed0df0228 100644 --- a/test/fixtures/changesets.yml +++ b/test/fixtures/changesets.yml @@ -30,12 +30,17 @@ normal_user_version_change: user_id: 1 created_at: "2008-01-01 00:00:00" open: true + min_lon: <%= 1 * SCALE %> + min_lat: <%= 1 * SCALE %> + max_lon: <%= 4 * SCALE %> + max_lat: <%= 4 * SCALE %> # changeset to contain all the invalid stuff that is in the -# fixtures (nodes outside the world, etc...) +# fixtures (nodes outside the world, etc...), but needs to have +# a valid user... invalid_changeset: id: 5 - user_id: 0 + user_id: 3 created_at: "2008-01-01 00:00:00" open: false \ No newline at end of file diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb index 6cbee1eb8..2b8fa360a 100644 --- a/test/functional/changeset_controller_test.rb +++ b/test/functional/changeset_controller_test.rb @@ -595,7 +595,37 @@ EOF def test_changeset_by_bbox get :query, :bbox => "-10,-10, 10, 10" assert_response :success, "can't get changesets in bbox" - # FIXME: write the actual test bit after fixing the fixtures! + assert_changesets [1,4] + + get :query, :bbox => "4.5,4.5,4.6,4.6" + assert_response :success, "can't get changesets in bbox" + assert_changesets [1] + + # can't get changesets of user 1 without authenticating + get :query, :user => users(:normal_user).id + assert_response :not_found, "shouldn't be able to get changesets by non-public user" + + # but this should work + basic_authorization "test@openstreetmap.org", "test" + get :query, :user => users(:normal_user).id + assert_response :success, "can't get changesets by user" + assert_changesets [1,3,4] + + get :query, :user => users(:normal_user).id, :open => true + assert_response :success, "can't get changesets by user and open" + assert_changesets [1,4] + + get :query, :time => '2007-12-31' + assert_response :success, "can't get changesets by time-since" + assert_changesets [2,4,5] + + get :query, :time => '2008-01-01T12:34Z' + assert_response :success, "can't get changesets by time-since with hour" + assert_changesets [2] + + get :query, :time => '2007-12-31T23:59Z,2008-01-01T00:01Z' + assert_response :success, "can't get changesets by time-range" + assert_changesets [4,5] end ## @@ -641,6 +671,16 @@ EOF # utility functions #------------------------------------------------------------ + ## + # boilerplate for checking that certain changesets exist in the + # output. + def assert_changesets(ids) + assert_select "osm>changeset", ids.size + ids.each do |id| + assert_select "osm>changeset[id=#{id}]", 1 + end + end + ## # call the include method and assert properties of the bbox def check_after_include(changeset_id, lon, lat, bbox) -- 2.43.2