Topic: Too much in my controller?

Been seeing this concept come up a bit lately on here, and I was wondering if perhaps some of my controller methods might have too much going on. Heres my create method for the sessions controller, basically verifies entered user information and if its correct adds the user id to the session.

  def create
    user = User.authenticate(params[:email], params[:password])
    # if an error msg is being sent back, display it
    if user.class == String
      respond_to do |format|
        flash[:error] = user
        format.html { redirect_to user_home_path(user) }
      end
    # user is set to false, add an attempt
    elsif !user
      # manage the number of attempts appropriately
      if session[:attempts]
        session[:attempts] = session[:attempts] + 1
      else
        session[:attempts] = 1
      end

      # redirect appropriately
      if session[:attempts] == 5
        respond_to do |format|
          flash[:error] = 'Maximum number of attempts taken. Provide your security answer to recieve a new password'
          format.html { redirect_to forgot_password_path }
        end
      else
        respond_to do |format|
          flash[:error] = 'Incorrect email or password entered. Please try again'
          format.html { redirect_to root_path }
        end
      end
    # if the user is otherwise set
    elsif user
      # if the user is not active
      if !user.activated
        respond_to do |format|
          flash[:error] = "Your account is not activated, please check your e-mail for an activation link or click " +
            "<a href=\"#{resend_activation_path}\">here</a> to resend the e-mail. " +
            "<a href=\"#{activate_path(user.activation_code)}\">activate</a>"
          format.html { redirect_to root_path }
        end
      # if the user is disabled
      elsif user.disabled
        respond_to do |format|
          format.html { redirect_to root_path }
        end
      # all is well, set the user to session
      else
        session[:user_id] = user.id
        # if attempts have been set, set it to nil
        if session[:attempts]
          session[:attempts] = nil
        end
        respond_to do |format|
          format.html { redirect_to user_home_path }
        end
      end
    # otherwise its not found, let the user know
    else
      respond_to do |format|
        flash[:error] = 'Incorrect email or password entered. Please try again'
        format.html { redirect_to root_path }
      end
    end
  end

Some of the methods used are in the user model, so heres that file as well:

require 'digest/sha1'

# coding: utf-8

class User < ActiveRecord::Base
  # include Authentication::ByPassword
  attr_accessor               :password_confirmation
  validates_presence_of       :email, :fname, :lname, :security_q, :security_a, :hashed_password,
                              :permissions, :postalcode
  validates_presence_of       :institute_id, :campus_id
  validates_uniqueness_of     :email
  validates_length_of         :password, :in => 8 .. 14, :on => :create
  validates_length_of         :postalcode, :is => 6

  validates_confirmation_of   :password

  validates_format_of         :email,
                              :with => /^([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})$/i,
                              :message => 'email must be valid'
  attr_accessible             :email, :fname, :lname, :security_q, :security_a, :password, :password_confirmation,
                              :institute_id, :area_of_study_id, :addrl, :city, :province, :postalcode,
                              :birthday, :sex, :profile_picture, :campus_id

  # loads the different user types into a hash
  def load_user_types
    types = {
      'Student' => 0,
      'Instructor' => 14
    }
    types.sort { |a, b| a[1] <=> b[1] }
  end

  # loads the gender types into a hash
  def load_sex_types
    types = {
      'Please Choose...' => '',
      'M' => 'M',
      'F' => 'F'
    }
    types.sort { |a, b| a[1] <=> b[1] }
  end

  # loads the provinces into a hash
  def load_provinces
    provinces = {
      'Please Choose...' => '',
      'Alberta' => 'AB',
      'British Columbia' => 'BC',
      'Manitoba' => 'MB',
      'Newfoundland' => 'NF',
      'Nova Scotia' => 'NS',
      'Northwest Territories' => 'NT',
      'Nunavut' => 'NU',
      'Ontario' => 'ON',
      'Prince Edward Island' => 'PE',
      'Quebec' => 'QC',
      'Saskatchewan' => 'SK',
      'Yukon Territory' => 'YT'
    }
    provinces.sort { |a, b| a[1] <=> b[1] }
  end

  # authenticates given login information
  def self.authenticate(email, password)
    user = self.find_by_email(email)
    if user
      expected_password = encrypted_password(password, user.salt)
      if user.hashed_password != expected_password
        user = false # set to false, so the controller knows the password is invalid
      end
    else
      user = nil
    end
    user
  end

  # returns the password variable
  def password
    @password
  end

  # accepts the sent password, and hashes it
  def password=(pwd)
    @password = pwd
    return if pwd.blank?
    create_new_salt
    self.hashed_password = User.encrypted_password(self.password, self.salt)
  end

  def secure_new_pw(password)
    create_new_salt
    self.hashed_password = User.encrypted_password(password, self.salt)
  end

  # sets and returns a randomly generated activation code
  def self.activation_code
    @activation_code = User.random_string(15)
  end

  def self.generate_password
    return User.random_string(9)
  end

  # keeping for paginate example
  def self.member_list(page)
    paginate :all,
             :per_page => 50, :page => page,
             :conditions => ['enabled = ? and activated_at IS NOT NULL', true],
             :order => 'login'
  end

  def is_admin?
    return true if @permissions == 33
  end

  def is_teacher?
    return true if @permissions == 14
  end

  def self.load_user_hash
    users = Hash.new
    user_a = User.find(:all, :order => 'fname, lname')
    user_a.each { |u| users["#{u.fname}  #{u.lname}"] = u.id}
    users
  end

private

  def self.encrypted_password(password, salt)
    string_to_hash = password + "wibble" + salt
    Digest::SHA1.hexdigest(string_to_hash)
  end

  def create_new_salt
    self.salt = self.object_id.to_s + rand.to_s
  end

  def self.random_string(max)
    o =  [('a'..'z'),('A'..'Z'), (0 .. 9)].map{|i| i.to_a}.flatten
    return (0 .. max).map{ o[rand(o.length)]  }.join
  end
end

Thanks guys.

Re: Too much in my controller?

Seems to me that the password retry code/count needs to be in the model, not the controller. If you did this, then you could persist the count in the DB instead of in the session, and it would be more secure. Just reset it each time you successfully authenticate the user.

Instead of User.authenticate() returning either a String error message, a User object, nil, or false, why not just either return an authenticated user object or raise an appropriate exception. Then your controller code would basically be a call to User.authenticate and a couple of rescue clauses to handle the exceptions.

Re: Too much in my controller?

specious wrote:

Seems to me that the password retry code/count needs to be in the model, not the controller. If you did this, then you could persist the count in the DB instead of in the session, and it would be more secure. Just reset it each time you successfully authenticate the user.

Instead of User.authenticate() returning either a String error message, a User object, nil, or false, why not just either return an authenticated user object or raise an appropriate exception. Then your controller code would basically be a call to User.authenticate and a couple of rescue clauses to handle the exceptions.

Those are some good ideas. I really like the idea of saving number of tries in the database, not really difficult to implement either.

Re: Too much in my controller?

on a side note, I would also point out that you have several model methods which are all performing the exact same sort...  i'd throw that into a private method (or make your own method and share it amongst your methods..

   def load_user_types
     type_sort({'Student' => 0, 'Instructor' => 14})
  end

  def load_sex_types
    type_sort({'Please Choose...' => nil, 'M' => 'M', 'F' => 'F'})
  end

  # .. etc

private

  def type_sort(h)
    h.sort { |a, b| a.last <=> b.last }
  end

-patrick