From ee3c9146e4bbe93ec1f00ee45386a82ec4363c4d Mon Sep 17 00:00:00 2001 From: Azul Date: Fri, 23 Nov 2012 12:11:11 +0100 Subject: identify user by id so rerendering the form does not use new invalid login --- users/app/controllers/users_controller.rb | 15 +++++++++++---- users/app/models/user.rb | 8 ++------ users/lib/warden/strategies/secure_remote_password.rb | 2 +- users/test/functional/users_controller_test.rb | 4 +++- users/test/unit/user_test.rb | 8 ++++---- 5 files changed, 21 insertions(+), 16 deletions(-) diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index ecab53b..3913d0d 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -1,6 +1,8 @@ class UsersController < ApplicationController - skip_before_filter :verify_authenticity_token + skip_before_filter :verify_authenticity_token, :only => [:create] + + before_filter :fetch_user, :only => [:edit, :update] respond_to :json, :html @@ -17,12 +19,17 @@ class UsersController < ApplicationController end def edit - @user = current_user end def update - @user = current_user - @user.update(params[:user]) + @user.update_attributes(params[:user]) respond_with(@user, :location => edit_user_path(@user)) end + + protected + + def fetch_user + @user = User.find_by_param(params[:id]) + access_denied unless @user == current_user + end end diff --git a/users/app/models/user.rb b/users/app/models/user.rb index 507eda5..624754b 100644 --- a/users/app/models/user.rb +++ b/users/app/models/user.rb @@ -29,9 +29,7 @@ class User < CouchRest::Model::Base end class << self - def find_by_param(login) - return find_by_login(login) || raise(RECORD_NOT_FOUND) - end + alias_method :find_by_param, :find # valid set of attributes for testing def valid_attributes_hash @@ -42,9 +40,7 @@ class User < CouchRest::Model::Base end - def to_param - self.login - end + alias_method :to_param, :id def to_json(options={}) { diff --git a/users/lib/warden/strategies/secure_remote_password.rb b/users/lib/warden/strategies/secure_remote_password.rb index 95570e0..953e2e9 100644 --- a/users/lib/warden/strategies/secure_remote_password.rb +++ b/users/lib/warden/strategies/secure_remote_password.rb @@ -30,7 +30,7 @@ module Warden end def initialize! - user = User.find_by_param(id) + user = User.find_by_login(id) session[:handshake] = user.initialize_auth(params['A'].hex) custom! json_response(session[:handshake]) rescue RECORD_NOT_FOUND diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb index 4318928..e39869f 100644 --- a/users/test/functional/users_controller_test.rb +++ b/users/test/functional/users_controller_test.rb @@ -32,6 +32,7 @@ class UsersControllerTest < ActionController::TestCase test "should get edit view" do user = stub_record User + User.expects(:find_by_param).with(user.id.to_s).returns(user) login user get :edit, :id => user.id assert_equal user, assigns[:user] @@ -39,7 +40,8 @@ class UsersControllerTest < ActionController::TestCase test "should process updated params" do user = stub_record User - user.expects(:update).with(user.params).returns(user) + user.expects(:update_attributes).with(user.params).returns(true) + User.expects(:find_by_param).with(user.id.to_s).returns(user) login user post :update, :user => user.params, :id => user.id assert_equal user, assigns[:user] diff --git a/users/test/unit/user_test.rb b/users/test/unit/user_test.rb index f057ca7..92c1463 100644 --- a/users/test/unit/user_test.rb +++ b/users/test/unit/user_test.rb @@ -23,14 +23,14 @@ class UserTest < ActiveSupport::TestCase assert !@user.valid? end - test "find_by_param gets User by login" do + test "find_by_param gets User by id" do @user.save - assert_equal @user, User.find_by_param(@user.login) + assert_equal @user, User.find_by_param(@user.id) @user.destroy end - test "to_param gives user login" do - assert_equal @user.login, @user.to_param + test "to_param gives user id" do + assert_equal @user.id, @user.to_param end test "verifier returns number for the hex in password_verifier" do -- cgit v1.2.3 From 76a3b91ad78d12ef82a0c01ca702720a510f1e22 Mon Sep 17 00:00:00 2001 From: Azul Date: Sun, 25 Nov 2012 13:21:23 +0100 Subject: basic changing of password and login working --- users/app/assets/javascripts/srp | 2 +- users/app/assets/javascripts/users.js.coffee | 36 ++++++---------------------- users/app/views/users/_form.html.haml | 3 ++- 3 files changed, 10 insertions(+), 31 deletions(-) diff --git a/users/app/assets/javascripts/srp b/users/app/assets/javascripts/srp index 076d6e2..fff770a 160000 --- a/users/app/assets/javascripts/srp +++ b/users/app/assets/javascripts/srp @@ -1 +1 @@ -Subproject commit 076d6e251e4caf826787d87b11434e535960455c +Subproject commit fff770a866b44abce6fe0fc5d5ffde034225436d diff --git a/users/app/assets/javascripts/users.js.coffee b/users/app/assets/javascripts/users.js.coffee index d0ec32f..5663161 100644 --- a/users/app/assets/javascripts/users.js.coffee +++ b/users/app/assets/javascripts/users.js.coffee @@ -1,40 +1,17 @@ preventDefault = (event) -> event.preventDefault() -validOrAbort = (event) -> - errors = {} - - abortIfErrors = -> - return if $.isEmptyObject(errors) - # we're relying on client_side_validations here instead of printing - # our own errors. This gets us translatable error messages. - $('.control-group.error input, .control-group.error select, control-group.error textarea').first().focus() - event.stopImmediatePropagation() - - validatePassword = -> - password = $('#srp_password').val() - confirmation = $('#srp_password_confirmation').val() - login = $('#srp_username').val() - - if password != confirmation - errors.password_confirmation = "Confirmation does not match!" - if password == login - errors.password = "Password and Login may not match!" - if password.length < 8 - errors.password = "Password needs to be at least 8 characters long!" - - validatePassword() - abortIfErrors() - - - srp.session = new srp.Session() srp.signedUp = -> - window.location = '/' + srp.login srp.loggedIn = -> window.location = '/' +#// TODO: not sure this is what we want. +srp.updated = -> + window.location = '/' + srp.error = (message) -> if $.isPlainObject(message) && message.errors for field, error of message.errors @@ -46,8 +23,9 @@ srp.error = (message) -> $(document).ready -> $('#new_user').submit preventDefault - $('#new_user').submit validOrAbort $('#new_user').submit srp.signup $('#new_session').submit preventDefault $('#new_session').submit srp.login + $('.user.form.edit').submit srp.update + $('.user.form.edit').submit preventDefault diff --git a/users/app/views/users/_form.html.haml b/users/app/views/users/_form.html.haml index 8914241..d26d17d 100644 --- a/users/app/views/users/_form.html.haml +++ b/users/app/views/users/_form.html.haml @@ -1,4 +1,5 @@ -= simple_form_for @user, :validate => true, :html => {:class => 'form-horizontal'} do |f| +- html = {:class => 'form-horizontal user form ' + (@user.new_record? ? 'new' : 'edit')} += simple_form_for @user, :validate => true, :html => html do |f| %legend = @user.new_record? ? t(:signup_message) : t(:edit_settings) = f.input :login, :input_html => { :id => :srp_username } -- cgit v1.2.3 From ce0999ead0d61db1f6534ee9d8114c4551542e80 Mon Sep 17 00:00:00 2001 From: Azul Date: Mon, 26 Nov 2012 10:59:50 +0100 Subject: minor: client side validations fixed + .json request --- config/initializers/client_side_validations.rb | 2 +- users/app/assets/javascripts/users.js.coffee | 2 +- users/app/models/user.rb | 8 +++++++- users/app/views/users/_form.html.haml | 2 +- 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/config/initializers/client_side_validations.rb b/config/initializers/client_side_validations.rb index 2c73fa3..252aded 100644 --- a/config/initializers/client_side_validations.rb +++ b/config/initializers/client_side_validations.rb @@ -1,7 +1,7 @@ # ClientSideValidations Initializer # Uncomment to disable uniqueness validator, possible security issue -# ClientSideValidations::Config.disabled_validators = [:uniqueness] +ClientSideValidations::Config.disabled_validators = [:uniqueness] # Uncomment the following block if you want each input field to have the validation messages attached. ActionView::Base.field_error_proc = Proc.new do |html_tag, instance| diff --git a/users/app/assets/javascripts/users.js.coffee b/users/app/assets/javascripts/users.js.coffee index 5663161..f0bb3dd 100644 --- a/users/app/assets/javascripts/users.js.coffee +++ b/users/app/assets/javascripts/users.js.coffee @@ -15,7 +15,7 @@ srp.updated = -> srp.error = (message) -> if $.isPlainObject(message) && message.errors for field, error of message.errors - element = $('form input[name="session['+field+']"]') + element = $('form input[name$="['+field+']"]') next unless element element.trigger('element:validate:fail.ClientSideValidations', error).data('valid', false) else diff --git a/users/app/models/user.rb b/users/app/models/user.rb index 624754b..39d079a 100644 --- a/users/app/models/user.rb +++ b/users/app/models/user.rb @@ -9,7 +9,8 @@ class User < CouchRest::Model::Base :presence => true validates :login, - :uniqueness => true + :uniqueness => true, + :if => :serverside? validates :login, :format => { :with => /\A[A-Za-z\d_]+\z/, @@ -74,4 +75,9 @@ class User < CouchRest::Model::Base def password password_verifier end + + # used as a condition for validations that are server side only + def serverside? + true + end end diff --git a/users/app/views/users/_form.html.haml b/users/app/views/users/_form.html.haml index d26d17d..fc835af 100644 --- a/users/app/views/users/_form.html.haml +++ b/users/app/views/users/_form.html.haml @@ -1,5 +1,5 @@ - html = {:class => 'form-horizontal user form ' + (@user.new_record? ? 'new' : 'edit')} -= simple_form_for @user, :validate => true, :html => html do |f| += simple_form_for @user, :validate => true, :format => :json, :html => html do |f| %legend = @user.new_record? ? t(:signup_message) : t(:edit_settings) = f.input :login, :input_html => { :id => :srp_username } -- cgit v1.2.3 From 595518684b9c4364f96c97a84cc481b5ae0da981 Mon Sep 17 00:00:00 2001 From: Azul Date: Mon, 26 Nov 2012 11:54:11 +0100 Subject: simplified controller and adjusted tests Also added #assert_json_error to tests. --- core/lib/extensions/testing.rb | 12 +++++++++-- users/app/controllers/users_controller.rb | 9 +++----- users/test/functional/sessions_controller_test.rb | 4 ++-- users/test/functional/users_controller_test.rb | 25 +++++++++++----------- users/test/support/stub_record_helper.rb | 1 + users/test/unit/user_test.rb | 1 + .../warden_strategy_secure_remote_password_test.rb | 4 ++-- 7 files changed, 31 insertions(+), 25 deletions(-) diff --git a/core/lib/extensions/testing.rb b/core/lib/extensions/testing.rb index 86a059f..925c023 100644 --- a/core/lib/extensions/testing.rb +++ b/core/lib/extensions/testing.rb @@ -15,10 +15,18 @@ module LeapWebCore end def assert_json_response(object) - object.stringify_keys! if object.respond_to? :stringify_keys! - assert_equal object, JSON.parse(get_response.body) + if object.is_a? Hash + object.stringify_keys! if object.respond_to? :stringify_keys! + assert_equal object, JSON.parse(get_response.body) + else + assert_equal object.to_json, get_response.body + end end + def assert_json_error(object) + object.stringify_keys! if object.respond_to? :stringify_keys! + assert_json_response :errors => object + end end end diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index 3913d0d..5be1fa9 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -11,11 +11,8 @@ class UsersController < ApplicationController end def create - @user = User.create!(params[:user]) - respond_with(@user, :location => root_url, :notice => "Signed up!") - rescue VALIDATION_FAILED => e - @user = e.document - respond_with(@user, :location => new_user_path) + @user = User.create(params[:user]) + respond_with @user end def edit @@ -23,7 +20,7 @@ class UsersController < ApplicationController def update @user.update_attributes(params[:user]) - respond_with(@user, :location => edit_user_path(@user)) + respond_with @user end protected diff --git a/users/test/functional/sessions_controller_test.rb b/users/test/functional/sessions_controller_test.rb index 93cc032..9df4455 100644 --- a/users/test/functional/sessions_controller_test.rb +++ b/users/test/functional/sessions_controller_test.rb @@ -22,7 +22,7 @@ class SessionsControllerTest < ActionController::TestCase request.env['warden'].expects(:winning_strategy) get :new, :format => :json assert_response :success - assert_json_response :errors => nil + assert_json_error nil end test "renders warden errors" do @@ -31,7 +31,7 @@ class SessionsControllerTest < ActionController::TestCase I18n.expects(:t).with(:translate_me).at_least_once.returns("translation stub") get :new, :format => :json assert_response 422 - assert_json_response :errors => {"field" => "translation stub"} + assert_json_error :field => "translation stub" end # Warden takes care of parsing the params and diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb index e39869f..ced8ee9 100644 --- a/users/test/functional/users_controller_test.rb +++ b/users/test/functional/users_controller_test.rb @@ -11,23 +11,22 @@ class UsersControllerTest < ActionController::TestCase test "should create new user" do user = stub_record User - User.expects(:create!).with(user.params).returns(user) - post :create, :user => user.params + User.expects(:create).with(user.params).returns(user) + post :create, :user => user.params, :format => :json assert_nil session[:user_id] - assert_response :redirect - assert_redirected_to root_url + assert_json_response user + assert_response :success end test "should redirect to signup form on failed attempt" do params = User.valid_attributes_hash.slice(:login) user = User.new(params) params.stringify_keys! - User.expects(:create!).with(params).raises(VALIDATION_FAILED.new(user)) - post :create, :user => params - assert_nil session[:user_id] - assert_equal user, assigns[:user] - assert_response :redirect - assert_redirected_to new_user_path + assert !user.valid? + User.expects(:create).with(params).returns(user) + post :create, :user => params, :format => :json + assert_json_error user.errors.messages + assert_response 422 end test "should get edit view" do @@ -43,9 +42,9 @@ class UsersControllerTest < ActionController::TestCase user.expects(:update_attributes).with(user.params).returns(true) User.expects(:find_by_param).with(user.id.to_s).returns(user) login user - post :update, :user => user.params, :id => user.id + put :update, :user => user.params, :id => user.id, :format => :json assert_equal user, assigns[:user] - assert_response :redirect - assert_redirected_to edit_user_path(user) + assert_equal " ", @response.body + assert_response 204 end end diff --git a/users/test/support/stub_record_helper.rb b/users/test/support/stub_record_helper.rb index 95b9d63..e744ad7 100644 --- a/users/test/support/stub_record_helper.rb +++ b/users/test/support/stub_record_helper.rb @@ -10,6 +10,7 @@ module StubRecordHelper params.reverse_merge! :id => 123, :class => klass, :to_key => ['123'], + :to_json => %Q({"stub":"#{klass.name}"}), :new_record? => !persisted, :persisted? => persisted stub params diff --git a/users/test/unit/user_test.rb b/users/test/unit/user_test.rb index 92c1463..cce11c2 100644 --- a/users/test/unit/user_test.rb +++ b/users/test/unit/user_test.rb @@ -5,6 +5,7 @@ class UserTest < ActiveSupport::TestCase include SRP::Util setup do @attribs = User.valid_attributes_hash + User.find_by_login(@attribs[:login]).try(:destroy) @user = User.new(@attribs) end diff --git a/users/test/unit/warden_strategy_secure_remote_password_test.rb b/users/test/unit/warden_strategy_secure_remote_password_test.rb index 79480f0..319809a 100644 --- a/users/test/unit/warden_strategy_secure_remote_password_test.rb +++ b/users/test/unit/warden_strategy_secure_remote_password_test.rb @@ -32,7 +32,7 @@ class WardenStrategySecureRemotePasswordTest < ActiveSupport::TestCase User.expects(:find_by_param).with(unknown).raises(RECORD_NOT_FOUND) post :create, :login => unknown assert_response :success - assert_json_response :errors => {"login" => ["unknown user"]} + assert_json_error "login" => ["unknown user"] end test "should authorize" do @@ -56,7 +56,7 @@ class WardenStrategySecureRemotePasswordTest < ActiveSupport::TestCase post :update, :id => @user.login, :client_auth => @client_hex assert_nil session[:handshake] assert_nil session[:user_id] - assert_json_response :errors => {"password" => ["wrong password"]} + assert_json_error "password" => ["wrong password"] end =end -- cgit v1.2.3 From cdda8f095d49cdda94c3527ecb92cb15c300327b Mon Sep 17 00:00:00 2001 From: Azul Date: Mon, 26 Nov 2012 12:15:54 +0100 Subject: fixed login error message on wrong username --- users/lib/warden/strategies/secure_remote_password.rb | 11 ++++++----- users/test/integration/api/account_flow_test.rb | 3 ++- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/users/lib/warden/strategies/secure_remote_password.rb b/users/lib/warden/strategies/secure_remote_password.rb index 953e2e9..594e27e 100644 --- a/users/lib/warden/strategies/secure_remote_password.rb +++ b/users/lib/warden/strategies/secure_remote_password.rb @@ -30,11 +30,12 @@ module Warden end def initialize! - user = User.find_by_login(id) - session[:handshake] = user.initialize_auth(params['A'].hex) - custom! json_response(session[:handshake]) - rescue RECORD_NOT_FOUND - fail! :login => "user_not_found" + if user = User.find_by_login(id) + session[:handshake] = user.initialize_auth(params['A'].hex) + custom! json_response(session[:handshake]) + else + fail! :login => "user_not_found" + end end def json_response(object) diff --git a/users/test/integration/api/account_flow_test.rb b/users/test/integration/api/account_flow_test.rb index 4135485..add12fe 100644 --- a/users/test/integration/api/account_flow_test.rb +++ b/users/test/integration/api/account_flow_test.rb @@ -65,8 +65,8 @@ class AccountFlowTest < ActiveSupport::TestCase test "signup and wrong password login attempt" do srp = SRP::Client.new(@login, "wrong password") server_auth = srp.authenticate(self) + assert_json_error :password => "wrong password" assert !last_response.successful? - assert_equal "wrong password", server_auth["errors"]['password'] assert_nil server_auth["M2"] end @@ -76,6 +76,7 @@ class AccountFlowTest < ActiveSupport::TestCase assert_raises RECORD_NOT_FOUND do server_auth = srp.authenticate(self) end + assert_json_error :login => "could not be found" assert !last_response.successful? assert_nil server_auth end -- cgit v1.2.3