From eb1d969237a9ed1bb41c6e10d5a9eb073f297e95 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sat, 19 Jul 2008 10:47:19 +0000 Subject: [PATCH] Improved on-the-fly account creation. If some attributes are missing (eg. not present in the LDAP) or are invalid, the registration form is displayed so that the user is able to fill or fix these attributes. git-svn-id: http://redmine.rubyforge.org/svn/trunk@1678 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/account_controller.rb | 68 +++++++++++++++++---------- app/models/auth_source.rb | 5 +- app/models/auth_source_ldap.rb | 5 +- app/models/user.rb | 15 +++--- app/views/account/register.rhtml | 4 +- app/views/auth_sources/_form.rhtml | 8 +--- test/integration/account_test.rb | 48 +++++++++++++++++++ 7 files changed, 107 insertions(+), 46 deletions(-) diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb index a9b8a1b8..1fe99000 100644 --- a/app/controllers/account_controller.rb +++ b/app/controllers/account_controller.rb @@ -44,7 +44,16 @@ class AccountController < ApplicationController else # Authenticate user user = User.try_to_login(params[:username], params[:password]) - if user + if user.nil? + # Invalid credentials + flash.now[:error] = l(:notice_account_invalid_creditentials) + elsif user.new_record? + # Onthefly creation failed, display the registration form to fill/fix attributes + @user = user + session[:auth_source_registration] = {:login => user.login, :auth_source_id => user.auth_source_id } + render :action => 'register' + else + # Valid user self.logged_user = user # generate a key and set cookie if autologin if params[:autologin] && Setting.autologin? @@ -52,12 +61,8 @@ class AccountController < ApplicationController cookies[:autologin] = { :value => token.value, :expires => 1.year.from_now } end redirect_back_or_default :controller => 'my', :action => 'page' - else - flash.now[:error] = l(:notice_account_invalid_creditentials) end end - rescue User::OnTheFlyCreationFailure - flash.now[:error] = 'Redmine could not retrieve the required information from the LDAP to create your account. Please, contact your Redmine administrator.' end # Log out current user and redirect to welcome page @@ -107,39 +112,52 @@ class AccountController < ApplicationController # User self-registration def register - redirect_to(home_url) && return unless Setting.self_registration? + redirect_to(home_url) && return unless Setting.self_registration? || session[:auth_source_registration] if request.get? + session[:auth_source_registration] = nil @user = User.new(:language => Setting.default_language) else @user = User.new(params[:user]) @user.admin = false - @user.login = params[:user][:login] @user.status = User::STATUS_REGISTERED - @user.password, @user.password_confirmation = params[:password], params[:password_confirmation] - case Setting.self_registration - when '1' - # Email activation - token = Token.new(:user => @user, :action => "register") - if @user.save and token.save - Mailer.deliver_register(token) - flash[:notice] = l(:notice_account_register_done) - redirect_to :action => 'login' - end - when '3' - # Automatic activation + if session[:auth_source_registration] @user.status = User::STATUS_ACTIVE + @user.login = session[:auth_source_registration][:login] + @user.auth_source_id = session[:auth_source_registration][:auth_source_id] if @user.save + session[:auth_source_registration] = nil self.logged_user = @user flash[:notice] = l(:notice_account_activated) redirect_to :controller => 'my', :action => 'account' end else - # Manual activation by the administrator - if @user.save - # Sends an email to the administrators - Mailer.deliver_account_activation_request(@user) - flash[:notice] = l(:notice_account_pending) - redirect_to :action => 'login' + @user.login = params[:user][:login] + @user.password, @user.password_confirmation = params[:password], params[:password_confirmation] + case Setting.self_registration + when '1' + # Email activation + token = Token.new(:user => @user, :action => "register") + if @user.save and token.save + Mailer.deliver_register(token) + flash[:notice] = l(:notice_account_register_done) + redirect_to :action => 'login' + end + when '3' + # Automatic activation + @user.status = User::STATUS_ACTIVE + if @user.save + self.logged_user = @user + flash[:notice] = l(:notice_account_activated) + redirect_to :controller => 'my', :action => 'account' + end + else + # Manual activation by the administrator + if @user.save + # Sends an email to the administrators + Mailer.deliver_account_activation_request(@user) + flash[:notice] = l(:notice_account_pending) + redirect_to :action => 'login' + end end end end diff --git a/app/models/auth_source.rb b/app/models/auth_source.rb index 47c121a1..a0a2cdc5 100644 --- a/app/models/auth_source.rb +++ b/app/models/auth_source.rb @@ -20,10 +20,7 @@ class AuthSource < ActiveRecord::Base validates_presence_of :name validates_uniqueness_of :name - validates_length_of :name, :host, :maximum => 60 - validates_length_of :account_password, :maximum => 60, :allow_nil => true - validates_length_of :account, :base_dn, :maximum => 255 - validates_length_of :attr_login, :attr_firstname, :attr_lastname, :attr_mail, :maximum => 30 + validates_length_of :name, :maximum => 60 def authenticate(login, password) end diff --git a/app/models/auth_source_ldap.rb b/app/models/auth_source_ldap.rb index a438bd3c..655ffd6d 100644 --- a/app/models/auth_source_ldap.rb +++ b/app/models/auth_source_ldap.rb @@ -20,7 +20,10 @@ require 'iconv' class AuthSourceLdap < AuthSource validates_presence_of :host, :port, :attr_login - validates_presence_of :attr_firstname, :attr_lastname, :attr_mail, :if => Proc.new { |a| a.onthefly_register? } + validates_length_of :name, :host, :account_password, :maximum => 60, :allow_nil => true + validates_length_of :account, :base_dn, :maximum => 255, :allow_nil => true + validates_length_of :attr_login, :attr_firstname, :attr_lastname, :attr_mail, :maximum => 30, :allow_nil => true + validates_numericality_of :port, :only_integer => true def after_initialize self.port = 389 if self.port == 0 diff --git a/app/models/user.rb b/app/models/user.rb index 55fe3ac0..5a839721 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -103,19 +103,16 @@ class User < ActiveRecord::Base # user is not yet registered, try to authenticate with available sources attrs = AuthSource.authenticate(login, password) if attrs - onthefly = new(*attrs) - onthefly.login = login - onthefly.language = Setting.default_language - if onthefly.save - user = find(:first, :conditions => ["login=?", login]) + user = new(*attrs) + user.login = login + user.language = Setting.default_language + if user.save + user.reload logger.info("User '#{user.login}' created from the LDAP") if logger - else - logger.error("User '#{onthefly.login}' found in LDAP but could not be created (#{onthefly.errors.full_messages.join(', ')})") if logger - raise OnTheFlyCreationFailure.new end end end - user.update_attribute(:last_login_on, Time.now) if user + user.update_attribute(:last_login_on, Time.now) if user && !user.new_record? user rescue => text raise text diff --git a/app/views/account/register.rhtml b/app/views/account/register.rhtml index 4e2b5adf..755a7ad4 100644 --- a/app/views/account/register.rhtml +++ b/app/views/account/register.rhtml @@ -5,8 +5,9 @@
+<% if @user.auth_source_id.nil? %>

-<%= text_field 'user', 'login', :size => 25 %>

+<%= text_field 'user', 'login', :size => 25 %>

<%= password_field_tag 'password', nil, :size => 25 %>
@@ -14,6 +15,7 @@

<%= password_field_tag 'password_confirmation', nil, :size => 25 %>

+<% end %>

<%= text_field 'user', 'firstname' %>

diff --git a/app/views/auth_sources/_form.rhtml b/app/views/auth_sources/_form.rhtml index 3d148c11..9ffffafc 100644 --- a/app/views/auth_sources/_form.rhtml +++ b/app/views/auth_sources/_form.rhtml @@ -22,14 +22,12 @@

<%= text_field 'auth_source', 'base_dn', :size => 60 %>

-
-

<%= check_box 'auth_source', 'onthefly_register' %>

+
-

-

<%=l(:label_attribute_plural)%> +
<%=l(:label_attribute_plural)%>

<%= text_field 'auth_source', 'attr_login', :size => 20 %>

@@ -42,7 +40,5 @@

<%= text_field 'auth_source', 'attr_mail', :size => 20 %>

-

- diff --git a/test/integration/account_test.rb b/test/integration/account_test.rb index a01a3ba0..c349200d 100644 --- a/test/integration/account_test.rb +++ b/test/integration/account_test.rb @@ -17,6 +17,12 @@ require "#{File.dirname(__FILE__)}/../test_helper" +begin + require 'mocha' +rescue + # Won't run some tests +end + class AccountTest < ActionController::IntegrationTest fixtures :users @@ -102,4 +108,46 @@ class AccountTest < ActionController::IntegrationTest assert_redirected_to 'account/login' log_user('newuser', 'newpass') end + + if Object.const_defined?(:Mocha) + + def test_onthefly_registration + # disable registration + Setting.self_registration = '0' + AuthSource.expects(:authenticate).returns([:login => 'foo', :firstname => 'Foo', :lastname => 'Smith', :mail => 'foo@bar.com', :auth_source_id => 66]) + + post 'account/login', :username => 'foo', :password => 'bar' + assert_redirected_to 'my/page' + + user = User.find_by_login('foo') + assert user.is_a?(User) + assert_equal 66, user.auth_source_id + assert user.hashed_password.blank? + end + + def test_onthefly_registration_with_invalid_attributes + # disable registration + Setting.self_registration = '0' + AuthSource.expects(:authenticate).returns([:login => 'foo', :lastname => 'Smith', :auth_source_id => 66]) + + post 'account/login', :username => 'foo', :password => 'bar' + assert_response :success + assert_template 'account/register' + assert_tag :input, :attributes => { :name => 'user[firstname]', :value => '' } + assert_tag :input, :attributes => { :name => 'user[lastname]', :value => 'Smith' } + assert_no_tag :input, :attributes => { :name => 'user[login]' } + assert_no_tag :input, :attributes => { :name => 'user[password]' } + + post 'account/register', :user => {:firstname => 'Foo', :lastname => 'Smith', :mail => 'foo@bar.com'} + assert_redirected_to 'my/account' + + user = User.find_by_login('foo') + assert user.is_a?(User) + assert_equal 66, user.auth_source_id + assert user.hashed_password.blank? + end + + else + puts 'Mocha is missing. Skipping tests.' + end end