From 3002fcf2f1e72eddd02b0b2e8e5dca1fbdb72e17 Mon Sep 17 00:00:00 2001 From: lukemelia Date: Fri, 25 Aug 2006 02:25:18 +0000 Subject: [PATCH] Created tests for backend_controller to cover security concerns, including #372 "user can add an action to another user's context via API". Modified backend_controller to close that hole and make the tests pass. Added UserController#create to provide RESTful API for the admin user to create a new user in the system. This may be useful for the folks who have generously opened their Tracks installs to others. I have plans to document the RESTful API stuff at some point and write a Ruby wrapper. Created a class method User.get_salt that wraps all calls to the SALT constant so that unit-tests can be always run with the default salt (I was previously needing to modify my environment.rb to run tests). Replaced usages of assert_success in tests with assert_response :success because assert_success is deprecated. git-svn-id: http://www.rousette.org.uk/svn/tracks-repos/trunk@313 a4c988fc-2ded-0310-b66e-134b36920a42 --- tracks/app/controllers/backend_controller.rb | 28 +++-- tracks/app/controllers/user_controller.rb | 56 ++++++++- tracks/app/helpers/backend_helper.rb | 2 + tracks/app/models/user.rb | 7 +- tracks/test/fixtures/users.yml | 4 +- .../functional/backend_controller_test.rb | 38 ++++++ .../test/functional/login_controller_test.rb | 24 ++-- .../test/functional/user_controller_test.rb | 14 +-- .../test/integration/create_user_api_test.rb | 115 ++++++++++++++++++ tracks/test/integration/stories_test.rb | 2 +- tracks/test/test_helper.rb | 6 + tracks/test/unit/user_test.rb | 12 +- 12 files changed, 264 insertions(+), 44 deletions(-) create mode 100644 tracks/app/helpers/backend_helper.rb create mode 100644 tracks/test/functional/backend_controller_test.rb create mode 100644 tracks/test/integration/create_user_api_test.rb diff --git a/tracks/app/controllers/backend_controller.rb b/tracks/app/controllers/backend_controller.rb index 6746bcca..51ec3e1c 100644 --- a/tracks/app/controllers/backend_controller.rb +++ b/tracks/app/controllers/backend_controller.rb @@ -4,9 +4,8 @@ class BackendController < ApplicationController web_service_scaffold :invoke def new_todo(username, token, context_id, description) - if !check_token_against_user_word(username, token) - raise "invalid token" - end + check_token_against_user_word(username, token) + check_context_belongs_to_user(context_id) item = @user.todos.build item.description = description @@ -17,31 +16,34 @@ class BackendController < ApplicationController end def list_contexts(username, token) - if !check_token_against_user_word(username, token) - raise "invalid token" - end + check_token_against_user_word(username, token) @user.contexts end def list_projects(username, token) - if !check_token_against_user_word(username, token) - raise "invalid token" - end + check_token_against_user_word(username, token) @user.projects end - protected + private # Check whether the token in the URL matches the word in the User's table def check_token_against_user_word(username, token) @user = User.find_by_login( username ) unless ( token == @user.word) - render :text => "Sorry, you don't have permission to perform this action." - return false + raise (InvalidToken, "Sorry, you don't have permission to perform this action.") + end + end + + def check_context_belongs_to_user(context_id) + unless @user.contexts.exists? context_id + raise (CannotAccessContext, "Cannot access a context that does not belong to this user.") end - true end end + +class InvalidToken < RuntimeError; end +class CannotAccessContext < RuntimeError; end diff --git a/tracks/app/controllers/user_controller.rb b/tracks/app/controllers/user_controller.rb index 24e6351f..1e8d6e68 100644 --- a/tracks/app/controllers/user_controller.rb +++ b/tracks/app/controllers/user_controller.rb @@ -1,6 +1,6 @@ class UserController < ApplicationController layout 'standard' - before_filter :login_required + prepend_before_filter :login_required def index render_text "This will be our jumping-off point for managing user functions!" @@ -10,6 +10,44 @@ class UserController < ApplicationController render_text "You'll only be allowed to go here if you're an administrator." end + verify :method => :post, + :only => %w( create_user ), + :render => { :text => '403 Forbidden: Only POST requests on this resource are allowed.', + :status => 403 } + + # Example usage: curl -H 'Accept: application/xml' -H 'Content-Type: application/xml' + # -u admin:up2n0g00d + # -d 'usernameabc123' + # http://our.tracks.host/cpa/create_user + # + def create + admin = User.find_admin + #render_text "user is " + session["user_id"].to_s + " and admin is " + a.id.to_s + unless session["user_id"].to_i == admin.id.to_i + access_denied + return + end + unless request.content_type == "application/xml" + render_failure "Content Type must be application/xml." + return + end + unless check_create_user_params + render_failure "Expected post format is xml like so: usernameabc123." + return + end + user = User.new(params[:request]) + user.password_confirmation = params[:request][:password] + unless user.valid? + render_failure user.errors.full_messages.join(', ') + return + end + if user.save + render :text => "User created.", :status => 200 + else + render_failure "Failed to create user." + end + end + def preferences @page_title = "TRACKS::Preferences" @prefs = @user.preferences @@ -68,5 +106,21 @@ class UserController < ApplicationController return false end end + + private + + def render_failure message, status = 404 + render :text => message, :status => status + end + + def check_create_user_params + return false unless params.has_key?(:request) + return false unless params[:request].has_key?(:login) + return false if params[:request][:login].empty? + return false unless params[:request].has_key?(:password) + return false if params[:request][:password].empty? + return true + end + end \ No newline at end of file diff --git a/tracks/app/helpers/backend_helper.rb b/tracks/app/helpers/backend_helper.rb new file mode 100644 index 00000000..2414c125 --- /dev/null +++ b/tracks/app/helpers/backend_helper.rb @@ -0,0 +1,2 @@ +module BackendHelper +end diff --git a/tracks/app/models/user.rb b/tracks/app/models/user.rb index 72b04f7a..b5607d0a 100644 --- a/tracks/app/models/user.rb +++ b/tracks/app/models/user.rb @@ -27,12 +27,15 @@ class User < ActiveRecord::Base def crypt_word write_attribute("word", self.class.sha1(login + Time.now.to_i.to_s + rand.to_s)) end + + def self.get_salt + SALT + end protected def self.sha1(pass) - # SALT is set in RAILS_ROOT/config/environment.rb - Digest::SHA1.hexdigest("#{SALT}--#{pass}--") + Digest::SHA1.hexdigest("#{get_salt}--#{pass}--") end before_create :crypt_password, :crypt_word diff --git a/tracks/test/fixtures/users.yml b/tracks/test/fixtures/users.yml index 32403a2e..388c0aa2 100644 --- a/tracks/test/fixtures/users.yml +++ b/tracks/test/fixtures/users.yml @@ -2,7 +2,7 @@ admin_user: id: 1 login: admin - password: <%= Digest::SHA1.hexdigest("#{SALT}--abracadabra--") %> + password: <%= Digest::SHA1.hexdigest("#{User.get_salt()}--abracadabra--") %> word: <%= Digest::SHA1.hexdigest("adminSat Feb 25 17:14:00 GMT 20060.236961325863376") %> is_admin: true preferences: @@ -17,7 +17,7 @@ admin_user: other_user: id: 2 login: jane - password: <%= Digest::SHA1.hexdigest("#{SALT}--sesame--") %> + password: <%= Digest::SHA1.hexdigest("#{User.get_salt()}--sesame--") %> word: <%= Digest::SHA1.hexdigest("janeSun Feb 19 14:42:45 GMT 20060.408173979260027") %> is_admin: false preferences: diff --git a/tracks/test/functional/backend_controller_test.rb b/tracks/test/functional/backend_controller_test.rb new file mode 100644 index 00000000..e88d0b7d --- /dev/null +++ b/tracks/test/functional/backend_controller_test.rb @@ -0,0 +1,38 @@ +require File.dirname(__FILE__) + '/../test_helper' +require 'backend_controller' + +# Re-raise errors caught by the controller. +class BackendController; def rescue_action(e) raise e end; end + +class BackendControllerTest < Test::Unit::TestCase + fixtures :users, :projects, :contexts, :todos, :notes + + def setup + @controller = BackendController.new + request, response = ActionController::TestRequest.new, ActionController::TestResponse.new + assert_equal "change-me", User.get_salt() + end + + def test_new_todo_fails_with_incorrect_token + assert_raises_invalid_token { @controller.new_todo('admin', 'notthecorrecttoken', contexts('agenda').id, 'test') } + end + + def test_new_todo_fails_with_context_that_does_not_belong_to_user + assert_raise(CannotAccessContext, "Cannot access a context that does not belong to this user.") { @controller.new_todo(users('other_user').login, users('other_user').word, contexts('agenda').id, 'test') } + end + + def test_list_projects_fails_with_incorrect_token + assert_raises_invalid_token { @controller.list_projects('admin', 'notthecorrecttoken') } + end + + def test_list_contexts_fails_with_incorrect_token + assert_raises_invalid_token { @controller.list_contexts('admin', 'notthecorrecttoken') } + end + + private + + def assert_raises_invalid_token + assert_raise(InvalidToken, "Sorry, you don't have permission to perform this action.") { yield } + end + +end diff --git a/tracks/test/functional/login_controller_test.rb b/tracks/test/functional/login_controller_test.rb index 6100b81a..dbda2c47 100644 --- a/tracks/test/functional/login_controller_test.rb +++ b/tracks/test/functional/login_controller_test.rb @@ -10,15 +10,15 @@ class LoginControllerTest < Test::Unit::TestCase def setup assert_equal "test", ENV['RAILS_ENV'] - assert_equal "change-me", SALT + assert_equal "change-me", User.get_salt() @controller = LoginController.new @request = ActionController::TestRequest.new @response = ActionController::TestResponse.new end - # ============================================ - # Login and logout - # ============================================ + #============================================ + #Login and logout + #============================================ def test_invalid_login post :login, {:user_login => 'cracker', :user_password => 'secret', :user_noexpiry => 'on'} @@ -26,7 +26,7 @@ class LoginControllerTest < Test::Unit::TestCase assert_session_has_no :user_id assert_template "login" end - + def test_login_with_valid_admin_user @request.session['return-to'] = "/bogus/location" user = login('admin', 'abracadabra', 'on') @@ -36,8 +36,8 @@ class LoginControllerTest < Test::Unit::TestCase assert_equal "Login successful: session will not expire.", flash['notice'] assert_redirect_url "http://#{@request.host}/bogus/location" end - - + + def test_login_with_valid_standard_user user = login('jane','sesame', 'off') assert_equal user.id, @response.session['user_id'] @@ -46,28 +46,28 @@ class LoginControllerTest < Test::Unit::TestCase assert_equal "Login successful: session will expire after 1 hour of inactivity.", flash['notice'] assert_redirected_to :controller => 'todo', :action => 'index' end - + def test_logout user = login('admin','abracadabra', 'on') get :logout assert_nil(session['user_id']) assert_redirected_to :controller => 'login', :action => 'login' end - + # Test login with a bad password for existing user # def test_login_bad_password post :login, {:user_login => 'jane', :user_password => 'wrong', :user_noexpiry => 'on'} assert_session_has_no :user assert_equal "Login unsuccessful", flash['warning'] - assert_success + assert_response :success end def test_login_bad_login post :login, {:user_login => 'blah', :user_password => 'sesame', :user_noexpiry => 'on'} assert_session_has_no :user assert_equal "Login unsuccessful", flash['warning'] - assert_success + assert_response :success end # ============================================ @@ -103,7 +103,7 @@ class LoginControllerTest < Test::Unit::TestCase assert non_admin.is_admin == false || non_admin.is_admin == 0 post :signup, :user => {:login => 'newbie2', :password => 'newbiepass2', :password_confirmation => 'newbiepass2'} assert_template 'login/nosignup' - + num_users = User.find(:all) assert_equal num_users.length, 2 end diff --git a/tracks/test/functional/user_controller_test.rb b/tracks/test/functional/user_controller_test.rb index 07c67b63..6283612a 100644 --- a/tracks/test/functional/user_controller_test.rb +++ b/tracks/test/functional/user_controller_test.rb @@ -10,7 +10,7 @@ class UserControllerTest < Test::Unit::TestCase def setup assert_equal "test", ENV['RAILS_ENV'] - assert_equal "change-me", SALT + assert_equal "change-me", User.get_salt() @controller = UserController.new @request = ActionController::TestRequest.new @response = ActionController::TestResponse.new @@ -23,7 +23,7 @@ class UserControllerTest < Test::Unit::TestCase assert_redirected_to :controller => 'login', :action => 'login' @request.session['user_id'] = users(:admin_user).id # log in the admin user get :index - assert_success + assert_response :success end # Test admin with and without login @@ -33,7 +33,7 @@ class UserControllerTest < Test::Unit::TestCase assert_redirected_to :controller => 'login', :action => 'login' @request.session['user_id'] = users(:admin_user).id # log in the admin user get :admin - assert_success + assert_response :success end def test_preferences @@ -41,7 +41,7 @@ class UserControllerTest < Test::Unit::TestCase assert_redirected_to :controller => 'login', :action => 'login' @request.session['user_id'] = users(:admin_user).id # log in the admin user get :preferences - assert_success + assert_response :success assert_equal assigns['page_title'], "TRACKS::Preferences" assert_not_nil assigns['prefs'] assert_equal assigns['prefs'].length, 7 @@ -52,7 +52,7 @@ class UserControllerTest < Test::Unit::TestCase assert_redirected_to :controller => 'login', :action => 'login' @request.session['user_id'] = users(:admin_user).id # log in the admin user get :edit_preferences - assert_success + assert_response :success assert_equal assigns['page_title'], "TRACKS::Edit Preferences" assert_not_nil assigns['prefs'] assert_equal assigns['prefs'].length, 7 @@ -76,12 +76,12 @@ class UserControllerTest < Test::Unit::TestCase @request.session['user_id'] = users(:admin_user).id # log in the admin user @user = @request.session['user_id'] get :change_password # should now pass because we're logged in - assert_success + assert_response :success assert_equal assigns['page_title'], "TRACKS::Change password" post :update_password, :updateuser => {:password => 'newpassword', :password_confirmation => 'newpassword'} assert_redirected_to :controller => 'user', :action => 'preferences' @updated_user = User.find(users(:admin_user).id) - assert_equal @updated_user.password, Digest::SHA1.hexdigest("#{SALT}--newpassword--") + assert_equal @updated_user.password, Digest::SHA1.hexdigest("#{User.get_salt()}--newpassword--") assert_equal flash['notice'], "Password updated." end diff --git a/tracks/test/integration/create_user_api_test.rb b/tracks/test/integration/create_user_api_test.rb new file mode 100644 index 00000000..00106b7c --- /dev/null +++ b/tracks/test/integration/create_user_api_test.rb @@ -0,0 +1,115 @@ +require File.dirname(__FILE__) + '/../test_helper' +require 'user_controller' +require 'user' +require 'action_controller/integration' + +# Re-raise errors caught by the controller. +class UserController; def rescue_action(e) raise e end; end + +class CreateUserControllerTest < ActionController::IntegrationTest + fixtures :users + + def setup + assert_equal "test", ENV['RAILS_ENV'] + assert_equal "change-me", User.get_salt() + @foobar_postdata = "foobar" + @john_postdata = "johnbarracuda" + end + + def test_fails_with_401_if_not_authorized_user + authenticated_post_xml '/user/create', 'nobody', 'nohow', @foobar_postdata + assert_401_unauthorized + end + + def test_fails_with_401_if_not_admin_user + authenticated_post_xml '/user/create', users(:other_user).login, 'sesame', @foobar_postdata + assert_401_unauthorized + end + + def test_content_type_must_be_xml + authenticated_post_xml "/user/create", users(:admin_user).login, 'abracadabra', @foobar_postdata, {'CONTENT_TYPE' => "application/x-www-form-urlencoded"} + assert_response_and_body 404, "Content Type must be application/xml." + end + + def test_fails_with_invalid_xml_format + invalid_postdata = "" + authenticated_post_xml "/user/create", users(:admin_user).login, 'abracadabra', invalid_postdata + assert_404_invalid_xml + end + + def test_fails_with_invalid_xml_format2 + invalid_postdata = "foo" + authenticated_post_xml "/user/create", users(:admin_user).login, 'abracadabra', invalid_postdata + assert_404_invalid_xml + end + + def test_xml_simple_param_parsing + authenticated_post_xml "/user/create", users(:admin_user).login, 'abracadabra', @foobar_postdata + assert @controller.params.has_key?(:request) + assert @controller.params[:request].has_key?(:login) + assert @controller.params[:request].has_key?(:password) + assert_equal 'foo', @controller.params[:request][:login] + assert_equal 'bar', @controller.params[:request][:password] + end + + def test_fails_with_too_short_password + authenticated_post_xml "/user/create", users(:admin_user).login, 'abracadabra', @foobar_postdata + assert_response_and_body 404, "Password is too short (minimum is 5 characters)" + end + + def test_fails_with_nonunique_login + existing_login = users(:other_user).login + data = "#{existing_login}barracuda" + authenticated_post_xml "/user/create", users(:admin_user).login, 'abracadabra', data + assert_response_and_body 404, "Login has already been taken" + end + + def test_creates_new_user + authenticated_post_xml '/user/create', users(:admin_user).login, 'abracadabra', @john_postdata + assert_response_and_body 200, "User created." + assert_equal 3, User.count + john1 = User.find_by_login('john') + assert_not_nil john1, "expected user john to be created" + john2 = User.authenticate('john','barracuda') + assert_not_nil john2, "expected user john to be created" + end + + def test_fails_with_get_verb + authenticated_get_xml "/user/create", users(:admin_user).login, 'abracadabra', {} + end + + private + + def authenticated_post_xml(url, username, password, parameters, headers = {}) + post url, + parameters, + {'AUTHORIZATION' => "Basic " + Base64.encode64("#{username}:#{password}"), + 'ACCEPT' => 'application/xml', + 'CONTENT_TYPE' => 'application/xml' + }.merge(headers) + end + + def authenticated_get_xml(url, username, password, parameters, headers = {}) + get url, + parameters, + {'AUTHORIZATION' => "Basic " + Base64.encode64("#{username}:#{password}"), + 'ACCEPT' => 'application/xml', + 'CONTENT_TYPE' => 'application/xml' + }.merge(headers) + end + + def assert_401_unauthorized + assert_response_and_body 401, "401 Unauthorized: You are not authorized to interact with Tracks." + end + + def assert_404_invalid_xml + assert_response_and_body 404, "Expected post format is xml like so: usernameabc123." + end + + def assert_response_and_body (type, body, message = nil) + #puts @response.body + assert_response type, message + assert_equal body, @response.body, message + end + +end \ No newline at end of file diff --git a/tracks/test/integration/stories_test.rb b/tracks/test/integration/stories_test.rb index 95df9c7e..7e1a20b9 100644 --- a/tracks/test/integration/stories_test.rb +++ b/tracks/test/integration/stories_test.rb @@ -4,7 +4,7 @@ class StoriesTest < ActionController::IntegrationTest fixtures :users, :projects, :contexts, :todos, :notes def setup - assert_equal "change-me", SALT + assert_equal "change-me", User.get_salt end # #################################################### diff --git a/tracks/test/test_helper.rb b/tracks/test/test_helper.rb index f5fe6df6..1825dc06 100644 --- a/tracks/test/test_helper.rb +++ b/tracks/test/test_helper.rb @@ -2,6 +2,12 @@ ENV["RAILS_ENV"] = "test" require File.expand_path(File.dirname(__FILE__) + "/../config/environment") require 'test_help' +class User < ActiveRecord::Base + def self.get_salt + "change-me" + end +end + class Test::Unit::TestCase # Turn off transactional fixtures if you're working with MyISAM tables in MySQL self.use_transactional_fixtures = true diff --git a/tracks/test/unit/user_test.rb b/tracks/test/unit/user_test.rb index 560a17df..1cf3e463 100644 --- a/tracks/test/unit/user_test.rb +++ b/tracks/test/unit/user_test.rb @@ -5,7 +5,7 @@ class UserTest < Test::Unit::TestCase def setup assert_equal "test", ENV['RAILS_ENV'] - assert_equal "change-me", SALT + assert_equal "change-me", User.get_salt() @admin_user = User.find(1) @other_user = User.find(2) end @@ -16,7 +16,7 @@ class UserTest < Test::Unit::TestCase assert_kind_of User, @admin_user assert_equal 1, @admin_user.id assert_equal "admin", @admin_user.login - assert_equal "#{Digest::SHA1.hexdigest("#{SALT}--abracadabra--")}", @admin_user.password + assert_equal "#{Digest::SHA1.hexdigest("#{User.get_salt()}--abracadabra--")}", @admin_user.password assert_not_nil @admin_user.word assert @admin_user.is_admin end @@ -26,7 +26,7 @@ class UserTest < Test::Unit::TestCase assert_kind_of User, @other_user assert_equal 2, @other_user.id assert_equal "jane", @other_user.login - assert_equal "#{Digest::SHA1.hexdigest("#{SALT}--sesame--")}", @other_user.password + assert_equal "#{Digest::SHA1.hexdigest("#{User.get_salt()}--sesame--")}", @other_user.password assert_not_nil @other_user.word assert @other_user.is_admin == false || @other_user.is_admin == 0 end @@ -38,7 +38,7 @@ class UserTest < Test::Unit::TestCase # Test a password shorter than 5 characters # def test_validate_short_password - assert_equal "#{Digest::SHA1.hexdigest("#{SALT}--sesame--")}", @other_user.password + assert_equal "#{Digest::SHA1.hexdigest("#{User.get_salt()}--sesame--")}", @other_user.password @other_user.password = "four" assert !@other_user.save assert_equal 1, @other_user.errors.count @@ -48,7 +48,7 @@ class UserTest < Test::Unit::TestCase # Test a password longer than 40 characters # def test_validate_long_password - assert_equal "#{Digest::SHA1.hexdigest("#{SALT}--sesame--")}", @other_user.password + assert_equal "#{Digest::SHA1.hexdigest("#{User.get_salt()}--sesame--")}", @other_user.password @other_user.password = generate_random_string(41) assert !@other_user.save assert_equal 1, @other_user.errors.count @@ -58,7 +58,7 @@ class UserTest < Test::Unit::TestCase # Test that correct length password is valid # def test_validate_correct_length_password - assert_equal "#{Digest::SHA1.hexdigest("#{SALT}--sesame--")}", @other_user.password + assert_equal "#{Digest::SHA1.hexdigest("#{User.get_salt()}--sesame--")}", @other_user.password @other_user.password = generate_random_string(6) assert @other_user.save end