diff --git a/Gemfile b/Gemfile index c85b496e..c3ba7bd3 100644 --- a/Gemfile +++ b/Gemfile @@ -4,8 +4,7 @@ gem 'rails', '4.0.0.rc1' gem 'sass-rails', '4.0.0.rc1' gem 'coffee-rails', '~>4.0' -# add these gems to help with the transition: -gem 'protected_attributes' +# todo: remove xml api gem 'actionpack-xml_parser', github: 'rails/actionpack-xml_parser' # See https://github.com/sstephenson/execjs#readme for more supported runtimes diff --git a/Gemfile.lock b/Gemfile.lock index 4a4a408d..e300d0b9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -105,8 +105,6 @@ GEM mysql2 (0.3.11) nokogiri (1.5.9) polyglot (0.3.3) - protected_attributes (1.0.1) - activemodel (>= 4.0.0.beta, < 5.0) rack (1.5.2) rack-mini-profiler (0.1.26) rack (>= 1.1.3) @@ -209,7 +207,6 @@ DEPENDENCIES jquery-rails mocha mysql2 - protected_attributes rack-mini-profiler rails (= 4.0.0.rc1) rails_autolink diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index bc2ec16a..067c16e4 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -6,8 +6,9 @@ require_dependency "login_system" require_dependency "tracks/source_view" class ApplicationController < ActionController::Base - - protect_from_forgery + # Prevent CSRF attacks by raising an exception. + # For APIs, you may want to use :null_session instead. + protect_from_forgery with: :exception include LoginSystem helper_method :current_user, :prefs, :format_date diff --git a/app/controllers/contexts_controller.rb b/app/controllers/contexts_controller.rb index 98cb3dea..f7ed32c3 100644 --- a/app/controllers/contexts_controller.rb +++ b/app/controllers/contexts_controller.rb @@ -8,7 +8,7 @@ class ContextsController < ApplicationController prepend_before_filter :login_or_feed_token_required, :only => [:index] def index - @all_contexts = current_user.contexts + @all_contexts = current_user.contexts @active_contexts = current_user.contexts.active @hidden_contexts = current_user.contexts.hidden @closed_contexts = current_user.contexts.closed @@ -69,7 +69,7 @@ class ContextsController < ApplicationController render_failure "Expected post format is valid xml like so: context name.", 400 return end - @context = current_user.contexts.build(params['context']) + @context = current_user.contexts.build(context_params) @context.hide! if params['context_state'] && params['context_state']['hide'] == '1' @saved = @context.save @context_not_done_counts = { @context.id => 0 } @@ -93,7 +93,7 @@ class ContextsController < ApplicationController def update process_params_for_update - @context.attributes = params["context"] + @context.attributes = context_params @saved = @context.save @state_saved = set_state_for_update(@new_state) @saved = @saved && @state_saved @@ -161,6 +161,12 @@ class ContextsController < ApplicationController all_done_todos_for current_user.contexts.find(params[:id]) end + private + + def context_params + params.require(:context).permit(:name, :position, :state) + end + protected def update_state_counts diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index e6a20637..71b49f73 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -24,7 +24,7 @@ class NotesController < ApplicationController def create @note = current_user.notes.build - @note.attributes = params["note"] + @note.attributes = note_params @saved = @note.save @@ -45,7 +45,7 @@ class NotesController < ApplicationController def update @note = current_user.notes.find(params['id']) - @note.attributes = params["note"] + @note.attributes = note_params @saved = @note.save respond_to do |format| format.html @@ -69,4 +69,10 @@ class NotesController < ApplicationController @source_view = params['_source_view'] || 'note' end + private + + def note_params + params.require(:note).permit(:project_id, :body) + end + end diff --git a/app/controllers/preferences_controller.rb b/app/controllers/preferences_controller.rb index 0f0d8df9..36ba8342 100644 --- a/app/controllers/preferences_controller.rb +++ b/app/controllers/preferences_controller.rb @@ -9,8 +9,8 @@ class PreferencesController < ApplicationController def update @prefs = current_user.prefs @user = current_user - user_updated = current_user.update_attributes(params['user']) - prefs_updated = current_user.preference.update_attributes(params['prefs']) + user_updated = current_user.update_attributes(user_params) + prefs_updated = current_user.preference.update_attributes(prefs_params) if (user_updated && prefs_updated) if !params['user']['password'].blank? # password updated? logout_user t('preferences.password_changed') @@ -33,6 +33,20 @@ class PreferencesController < ApplicationController private + def prefs_params + params.require(:prefs).permit( + :date_format, :week_starts, :show_number_completed, + :show_completed_projects_in_sidebar, :show_hidden_contexts_in_sidebar, + :staleness_starts, :due_style, :locale, :title_date_format, :time_zone, + :show_hidden_projects_in_sidebar, :show_project_on_todo_done, + :review_period, :refresh, :verbose_action_descriptors, + :mobile_todos_per_page, :sms_email, :sms_context_id) + end + + def user_params + params.require(:user).permit(:login, :first_name, :last_name, :password_confirmation, :password, :auth_type, :open_id_url) + end + # Display notification if preferences are successful updated def preference_updated notify :notice, t('preferences.updated') diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 2220129e..40e0dba8 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -173,7 +173,7 @@ class ProjectsController < ApplicationController render_failure "Expected post format is valid xml like so: project name.", 400 return end - @project = current_user.projects.build(params['project']) + @project = current_user.projects.build(project_params) @go_to_project = params['go_to_project'] @saved = @project.save @project_not_done_counts = { @project.id => 0 } @@ -212,7 +212,7 @@ class ProjectsController < ApplicationController params['project']['name'] = params['value'] end - @project.attributes = params['project'] + @project.attributes = project_params @saved = @project.save if @saved @project.transition_to(@new_state) if @state_changed @@ -342,4 +342,10 @@ class ProjectsController < ApplicationController end end + private + + def project_params + params.require(:project).permit(:name, :position, :user_id, :description, :state, :default_context_id, :default_tags) + end + end diff --git a/app/controllers/recurring_todos_controller.rb b/app/controllers/recurring_todos_controller.rb index 35c10872..66568e54 100644 --- a/app/controllers/recurring_todos_controller.rb +++ b/app/controllers/recurring_todos_controller.rb @@ -89,7 +89,7 @@ class RecurringTodosController < ApplicationController params["recurring_todo"]["weekly_return_"+day]=' ' if params["recurring_todo"]["weekly_return_"+day].nil? end - @saved = @recurring_todo.update_attributes params["recurring_todo"] + @saved = @recurring_todo.update_attributes recurring_todo_params respond_to do |format| format.js @@ -97,7 +97,7 @@ class RecurringTodosController < ApplicationController end def create - p = RecurringTodoCreateParamsHelper.new(params) + p = RecurringTodoCreateParamsHelper.new(params, recurring_todo_params) p.attributes['end_date']=parse_date_per_user_prefs(p.attributes['end_date']) p.attributes['start_from']=parse_date_per_user_prefs(p.attributes['start_from']) @@ -207,9 +207,9 @@ class RecurringTodosController < ApplicationController class RecurringTodoCreateParamsHelper - def initialize(params) + def initialize(params, recurring_todo_params) @params = params['request'] || params - @attributes = params['request'] && params['request']['recurring_todo'] || params['recurring_todo'] + @attributes = recurring_todo_params # make sure all selectors (recurring_period, recurrence_selector, # daily_selector, monthly_selector and yearly_selector) are first in hash @@ -259,6 +259,25 @@ class RecurringTodosController < ApplicationController private + def recurring_todo_params + params.require(:recurring_todo).permit( + # model attributes + :context_id, :project_id, :description, :notes, :state, :start_from, + :ends_on, :end_date, :number_of_occurences, :occurences_count, :target, + :show_from_delta, :recurring_period, :recurrence_selector, :every_other1, + :every_other2, :every_other3, :every_day, :only_work_days, :every_count, + :weekday, :show_always, + # form attributes + :recurring_period, :daily_selector, :monthly_selector, :yearly_selector, + :recurring_target, :daily_every_x_days, :monthly_day_of_week, + :monthly_every_x_day, :monthly_every_x_month2, :monthly_every_x_month, + :monthly_every_xth_day, :recurring_show_days_before, + :recurring_show_always, :weekly_every_x_week, :weekly_return_monday, + :yearly_day_of_week, :yearly_every_x_day, :yearly_every_xth_day, + :yearly_month_of_year2, :yearly_month_of_year + ) + end + def init @days_of_week = [] 0.upto 6 do |i| diff --git a/app/controllers/todos/todo_create_params_helper.rb b/app/controllers/todos/todo_create_params_helper.rb index 22a102df..6cd910bf 100644 --- a/app/controllers/todos/todo_create_params_helper.rb +++ b/app/controllers/todos/todo_create_params_helper.rb @@ -5,7 +5,7 @@ module Todos def initialize(params, user) set_params(params) - filter_attributes + filter_attributes(params) filter_tags filter_starred @@ -20,8 +20,12 @@ module Todos @params = params['request'] || params end - def filter_attributes - @attributes = @params['request'] && @params['request']['todo'] || @params['todo'] + def filter_attributes(params) + if params[:request] + @attributes = todo_params(params[:request]) + else + @attributes = todo_params(params) + end @attributes = {} if @attributes.nil? # make sure there is at least an empty hash end @@ -116,6 +120,24 @@ module Todos private + def todo_params(params) + # keep :predecessor_dependencies from being filterd (for XML API). + # The permit cannot handle multiple precessors + deps = params[:todo][:predecessor_dependencies][:predecessor] if params[:todo][:predecessor_dependencies] + + filtered = params.require(:todo).permit( + :context_id, :project_id, :description, :notes, + :due, :show_from, :state, + # XML API + :tags => [:tag => [:name]], + :context => [:name], + :project => [:name]) + + # add back :predecessor_dependencies + filtered[:predecessor_dependencies] = {:predecessor => deps } unless deps.nil? + filtered + end + def find_or_create_group(group_type, set, name) return set_id_by_name(group_type, set, name) if specified_by_name?(group_type) return set_id_by_id_string(group_type, set, @attributes["#{group_type}_id"]) if specified_by_id?(group_type) diff --git a/app/controllers/todos_controller.rb b/app/controllers/todos_controller.rb index 06b82d1d..f65efe72 100644 --- a/app/controllers/todos_controller.rb +++ b/app/controllers/todos_controller.rb @@ -428,7 +428,7 @@ class TodosController < ApplicationController determine_changes_by_this_update determine_remaining_in_container_count( (@context_changed || @project_changed) ? @original_item : @todo) determine_down_count - determine_deferred_tag_count(params['_tag_name']) if source_view_is(:tag) + determine_deferred_tag_count(sanitize(params['_tag_name'])) if source_view_is(:tag) @todo.touch_predecessors if @original_item_description != @todo.description @@ -1219,7 +1219,10 @@ class TodosController < ApplicationController end def update_attributes_of_todo - @todo.attributes = params["todo"] + # TODO: duplication with todo_create_params_helper + @todo.attributes = params.require(:todo).permit( + :context_id, :project_id, :description, :notes, + :due, :show_from, :state) end def determine_changes_by_this_update diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 2311aac7..afabffac 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -78,7 +78,7 @@ class UsersController < ApplicationController return end - user = User.new(params['user']) + user = User.new(user_params) unless user.valid? session['new_user'] = user @@ -108,8 +108,8 @@ class UsersController < ApplicationController render_failure "Expected post format is valid xml like so: usernameabc123.", 400 return end - user = User.new(params[:user]) - user.password_confirmation = params[:user][:password] + user = User.new(user_params) + user.password_confirmation = user_params[:password] saved = user.save unless user.new_record? render :text => t('users.user_created'), :status => 200 @@ -147,7 +147,7 @@ class UsersController < ApplicationController def update_password # is used for focing password change after sha->bcrypt upgrade - current_user.change_password(params[:user][:password], params[:user][:password_confirmation]) + current_user.change_password(user_params[:password], user_params[:password_confirmation]) notify :notice, t('users.password_updated') redirect_to preferences_path rescue Exception => error @@ -160,7 +160,7 @@ class UsersController < ApplicationController end def update_auth_type - current_user.auth_type = params[:user][:auth_type] + current_user.auth_type = user_params[:auth_type] if current_user.save notify :notice, t('users.auth_type_updated') redirect_to preferences_path @@ -179,6 +179,10 @@ class UsersController < ApplicationController private + def user_params + params.require(:user).permit(:login, :first_name, :last_name, :password_confirmation, :password, :auth_type, :open_id_url) + end + def get_new_user if session['new_user'] user = session['new_user'] diff --git a/app/models/context.rb b/app/models/context.rb index 1e56c055..339c976c 100644 --- a/app/models/context.rb +++ b/app/models/context.rb @@ -1,7 +1,6 @@ class Context < ActiveRecord::Base - has_many :todos, -> { order("todos.due IS NULL, todos.due ASC, todos.created_at ASC").includes(:project) }, :dependent => :delete_all - + has_many :todos, -> { order("todos.due IS NULL, todos.due ASC, todos.created_at ASC").includes(:project) }, :dependent => :delete_all has_many :recurring_todos, :dependent => :delete_all belongs_to :user @@ -34,8 +33,6 @@ class Context < ActiveRecord::Base end end - attr_protected :user - validates_presence_of :name, :message => "context must have a name" validates_length_of :name, :maximum => 255, :message => "context name must be less than 256 characters" validates_uniqueness_of :name, :message => "already exists", :scope => "user_id" diff --git a/app/models/note.rb b/app/models/note.rb index 575f284d..ba663114 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -2,7 +2,5 @@ class Note < ActiveRecord::Base belongs_to :user belongs_to :project - attr_protected :user - scope :with_body, lambda { |terms| where("body LIKE ?", terms) } end diff --git a/app/models/preference.rb b/app/models/preference.rb index e34bfd6e..afe2b69f 100644 --- a/app/models/preference.rb +++ b/app/models/preference.rb @@ -1,11 +1,6 @@ class Preference < ActiveRecord::Base belongs_to :user belongs_to :sms_context, :class_name => 'Context' - - attr_accessible :date_format, :week_starts, :show_number_completed, :show_completed_projects_in_sidebar, - :show_hidden_contexts_in_sidebar, :staleness_starts, :due_style, :locale, - :title_date_format, :time_zone, :show_hidden_projects_in_sidebar, :show_project_on_todo_done, :review_period, - :refresh, :verbose_action_descriptors, :mobile_todos_per_page, :sms_email, :sms_context_id def self.due_styles { :due_in_n_days => 0, :due_on => 1} diff --git a/app/models/project.rb b/app/models/project.rb index 22aaaa7b..d86785fe 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -40,7 +40,6 @@ class Project < ActiveRecord::Base end end - attr_protected :user attr_accessor :cached_note_count def self.null_object @@ -123,7 +122,11 @@ class Project < ActiveRecord::Base end def name=(value) - self[:name] = value.gsub(/\s{2,}/, " ").strip + if value + self[:name] = value.gsub(/\s{2,}/, " ").strip + else + self[:name] = nil + end end def new_record_before_save? diff --git a/app/models/recurring_todo.rb b/app/models/recurring_todo.rb index 372882ad..92b4d613 100644 --- a/app/models/recurring_todo.rb +++ b/app/models/recurring_todo.rb @@ -1,6 +1,4 @@ class RecurringTodo < ActiveRecord::Base - attr_protected :user - belongs_to :context belongs_to :project belongs_to :user diff --git a/app/models/tag.rb b/app/models/tag.rb index 4ed2d8d2..d6a3aaf5 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -10,8 +10,6 @@ class Tag < ActiveRecord::Base # rescue the ActiveRecord database constraint errors instead. validates_presence_of :name validates_uniqueness_of :name, :case_sensitive => false - - attr_accessible :name before_create :before_create diff --git a/app/models/tagging.rb b/app/models/tagging.rb index 528856b2..53cb221c 100644 --- a/app/models/tagging.rb +++ b/app/models/tagging.rb @@ -2,9 +2,7 @@ # The Tagging join model. class Tagging < ActiveRecord::Base - - attr_accessible :taggable_id, :tag, :taggable_type - + belongs_to :tag belongs_to :taggable, :polymorphic => true, :touch => true diff --git a/app/models/todo.rb b/app/models/todo.rb index 976d6b10..1c4b4823 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -66,7 +66,7 @@ class Todo < ActiveRecord::Base aasm :column => :state do - state :active #, :enter => Proc.new{|t| puts "$$$ activating #{t.aasm_current_state} - #{t.show_from} "} + state :active state :project_hidden state :completed, :before_enter => Proc.new { |t| t.completed_at = Time.zone.now }, :before_exit => Proc.new { |t| t.completed_at = nil} state :deferred, :after_exit => Proc.new { |t| t[:show_from] = nil } @@ -103,8 +103,6 @@ class Todo < ActiveRecord::Base end end - attr_protected :user - # Description field can't be empty, and must be < 100 bytes Notes must be < # 60,000 bytes (65,000 actually, but I'm being cautious) validates_presence_of :description diff --git a/app/models/user.rb b/app/models/user.rb index 48960091..5aa94f8a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,9 +4,7 @@ require 'bcrypt' class User < ActiveRecord::Base # Virtual attribute for the unencrypted password attr_accessor :password - attr_protected :is_admin # don't allow mass-assignment for this - attr_accessible :login, :first_name, :last_name, :password_confirmation, :password, :auth_type, :open_id_url #for will_paginate plugin cattr_accessor :per_page @@per_page = 5 diff --git a/test/controllers/notes_controller_test.rb b/test/controllers/notes_controller_test.rb index 84882665..a83dabf1 100644 --- a/test/controllers/notes_controller_test.rb +++ b/test/controllers/notes_controller_test.rb @@ -2,13 +2,45 @@ require File.expand_path(File.dirname(__FILE__) + '/../test_helper') class NotesControllerTest < ActionController::TestCase - def setup - end - def test_get_notes_page login_as :admin_user get :index assert_response 200 end + + def test_create_new_note + login_as :admin_user + project = users(:admin_user).projects.first + count = users(:admin_user).notes.count + + post :create, note: {body: "test note", project_id: project.id}, format: :js + + assert_response 200 + assert assigns['saved'], "@saved should be true" + assert count+1, users(:admin_user).notes.reload.count + end + + def test_update_note + login_as :admin_user + + note = users(:admin_user).notes.first + + assert_not_equal "test", note.body + post :update, id: note.id, note: {body: "test"}, format: :js + assert_equal "test", note.reload.body + end + + def test_destroy_note + login_as :admin_user + + note = users(:admin_user).notes.first + count = users(:admin_user).notes.count + + post :destroy, id: note.id, format: :js + + old_note = users(:admin_user).notes.where(id: note.id).first + assert_nil old_note + assert count-1, users(:admin_user).notes.reload.count + end end diff --git a/test/integration/context_xml_api_test.rb b/test/integration/context_xml_api_test.rb index 9de6876e..71e6abab 100644 --- a/test/integration/context_xml_api_test.rb +++ b/test/integration/context_xml_api_test.rb @@ -19,7 +19,7 @@ class ContextXmlApiTest < ActionDispatch::IntegrationTest end def test_fails_gracefully_with_invalid_xml_format - authenticated_post_xml_to_context_create "" + authenticated_post_xml_to_context_create "" assert_responses_with_error 'Name context must have a name' end diff --git a/test/integration/project_xml_api_test.rb b/test/integration/project_xml_api_test.rb index b79edf56..2b5a344b 100644 --- a/test/integration/project_xml_api_test.rb +++ b/test/integration/project_xml_api_test.rb @@ -21,7 +21,7 @@ class ProjectXmlApiTest < ActionDispatch::IntegrationTest end def test_fails_with_invalid_xml_format2 - authenticated_post_xml_to_project_create "" + authenticated_post_xml_to_project_create "" assert_responses_with_error 'Name project must have a name' end diff --git a/test/integration/todo_xml_api_test.rb b/test/integration/todo_xml_api_test.rb index 6fd405b0..8234c056 100644 --- a/test/integration/todo_xml_api_test.rb +++ b/test/integration/todo_xml_api_test.rb @@ -61,7 +61,7 @@ class TodoXmlApiTest < ActionDispatch::IntegrationTest assert_response :success todo = @user.todos.where(:description => "this will succeed 2.0").first assert_not_nil todo - assert !todo.uncompleted_predecessors.empty? + assert !todo.uncompleted_predecessors.empty?, "should have predecessors" end def test_post_create_todo_with_single_dependency