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

@@ -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]
+      if session[:auth_source_registration]
-      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
+        @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
+        @user.login = params[:user][:login]
-        if @user.save
+        @user.password, @user.password_confirmation = params[:password], params[:password_confirmation]
-          # Sends an email to the administrators
+        case Setting.self_registration
-          Mailer.deliver_account_activation_request(@user)
+        when '1'
-          flash[:notice] = l(:notice_account_pending)
+          # Email activation
-          redirect_to :action => 'login'
+          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

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 :name, :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
 
   def authenticate(login, password)
   end

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

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)
+        user = new(*attrs)
-        onthefly.login = login
+        user.login = login
-        onthefly.language = Setting.default_language
+        user.language = Setting.default_language
-        if onthefly.save
+        if user.save
-          user = find(:first, :conditions => ["login=?", login])
+          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

app/views/account/register.rhtml

@@ -5,8 +5,9 @@
 
 <div class="box">
 <!--[form:user]-->
+<% if @user.auth_source_id.nil? %>
 <p><label for="user_login"><%=l(:field_login)%> <span class="required">*</span></label>
-<%= text_field 'user', 'login', :size => 25  %></p>
+<%= text_field 'user', 'login', :size => 25 %></p>
 
 <p><label for="password"><%=l(:field_password)%> <span class="required">*</span></label>
 <%= password_field_tag 'password', nil, :size => 25  %><br />
@@ -14,6 +15,7 @@
 
 <p><label for="password_confirmation"><%=l(:field_password_confirmation)%> <span class="required">*</span></label>
 <%= password_field_tag 'password_confirmation', nil, :size => 25  %></p>
+<% end %>
 
 <p><label for="user_firstname"><%=l(:field_firstname)%> <span class="required">*</span></label>
 <%= text_field 'user', 'firstname'  %></p>

app/views/auth_sources/_form.rhtml

@@ -22,14 +22,12 @@
 
 <p><label for="auth_source_base_dn"><%=l(:field_base_dn)%> <span class="required">*</span></label>
 <%= text_field 'auth_source', 'base_dn', :size => 60 %></p>
-</div>
 
-<div class="box">
 <p><label for="auth_source_onthefly_register"><%=l(:field_onthefly)%></label>
 <%= check_box 'auth_source', 'onthefly_register' %></p>
+</div>
 
-<p>
+<fieldset class="box"><legend><%=l(:label_attribute_plural)%></legend>
-<fieldset><legend><%=l(:label_attribute_plural)%></legend>
 <p><label for="auth_source_attr_login"><%=l(:field_login)%> <span class="required">*</span></label>
 <%= text_field 'auth_source', 'attr_login', :size => 20  %></p>
 
@@ -42,7 +40,5 @@
 <p><label for="auth_source_attr_mail"><%=l(:field_mail)%></label>
 <%= text_field 'auth_source', 'attr_mail', :size => 20  %></p>
 </fieldset>
-</p>
-</div>
 <!--[eoform:auth_source]-->
 

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