https://www.aeolusproject.org/redmine/issues/2118 This patchset replaces authlogic with warden which is used also in katello. This is first step which allows us to add more auth strategies (ldap, oauth) or use devise on top of warden, this also synchronizes us with katello auth.
What's not very good in this solution is lack of password handling/validations, which authlogic handled so now we have to handle this ourselves. This can be solved in some next step by using devise which is based on warden.
Password reset (or db recreation) is required with this patch because encrypted password has different format.
From: Jan Provaznik jprovazn@redhat.com
Adds warden and rails_warden dependencies. --- aeolus-conductor.spec.in | 2 ++ src/Gemfile | 2 ++ 2 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/aeolus-conductor.spec.in b/aeolus-conductor.spec.in index 9ec229b..4067194 100644 --- a/aeolus-conductor.spec.in +++ b/aeolus-conductor.spec.in @@ -37,6 +37,8 @@ Requires: rubygem(rack-restful_submit) Requires: rubygem(uuidtools) Requires: rubygem(sqlite3) Requires: rubygem(fastercsv) +Requires: rubygem(warden) +Requires: rubygem(rails_warden) Requires: rubygem(pg) Requires: postgresql Requires: postgresql-server diff --git a/src/Gemfile b/src/Gemfile index 05d3b7c..1d8917e 100644 --- a/src/Gemfile +++ b/src/Gemfile @@ -22,6 +22,8 @@ gem 'thin' gem 'json' gem 'railties' gem 'fastercsv' +gem 'warden' +gem 'rails_warden' group :development, :test do #gem 'rspec-rails' #gem 'factory_girl_rails'
From: Jan Provaznik jprovazn@redhat.com
--- aeolus-conductor.spec.in | 1 - src/Gemfile | 1 - src/app/controllers/application_controller.rb | 14 +------- src/app/controllers/user_sessions_controller.rb | 39 +++++++++++++---------- src/app/models/user.rb | 4 ++ src/config/initializers/warden.rb | 38 ++++++++++++++++++++++ 6 files changed, 66 insertions(+), 31 deletions(-) create mode 100644 src/config/initializers/warden.rb
diff --git a/aeolus-conductor.spec.in b/aeolus-conductor.spec.in index 4067194..2252935 100644 --- a/aeolus-conductor.spec.in +++ b/aeolus-conductor.spec.in @@ -24,7 +24,6 @@ Requires: rubygem(haml) >= 3.1 Requires: rubygem(nokogiri) >= 1.4.0 Requires: rubygem(will_paginate) >= 3.0 Requires: rubygem(parseconfig) -Requires: rubygem(authlogic) >= 3.0.2 Requires: rubygem(deltacloud-client) >= 0.0.9.8 Requires: rubygem(compass) >= 0.10.2 Requires: rubygem(compass-960-plugin) diff --git a/src/Gemfile b/src/Gemfile index 1d8917e..59d68c8 100644 --- a/src/Gemfile +++ b/src/Gemfile @@ -4,7 +4,6 @@ gem 'rails', ' >= 3.0.7'
gem 'sqlite3', :require => 'sqlite3'
-gem 'authlogic' gem 'deltacloud-client', :require => 'deltacloud' gem 'sass' gem 'haml' diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index 77b694d..d2773aa 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -25,7 +25,7 @@ require 'viewstate.rb' class ApplicationController < ActionController::Base # FIXME: not sure what we're doing aobut service layer w/ deltacloud include ApplicationService - helper_method :current_user_session, :current_user, :filter_view? + helper_method :current_user, :filter_view? before_filter :read_breadcrumbs
def top_section; end @@ -168,16 +168,6 @@ class ApplicationController < ActionController::Base return hash end
- def current_user_session - return @current_user_session unless @current_user_session.nil? - @current_user_session = UserSession.find - end - - def current_user - return @current_user unless @current_user.nil? - @current_user = current_user_session && current_user_session.user - end - def require_user return if current_user respond_to do |format| @@ -192,7 +182,7 @@ class ApplicationController < ActionController::Base end
def require_no_user - return unless current_user + return true unless current_user store_location flash[:notice] = "You must be logged out to access this page" redirect_to account_url diff --git a/src/app/controllers/user_sessions_controller.rb b/src/app/controllers/user_sessions_controller.rb index 8ade923..4b4afca 100644 --- a/src/app/controllers/user_sessions_controller.rb +++ b/src/app/controllers/user_sessions_controller.rb @@ -29,30 +29,35 @@ class UserSessionsController < ApplicationController end
def create - @user_session = UserSession.new(params[:user_session]) - if @user_session.save - session[:javascript_enabled] = request.xhr? - respond_to do |format| - format.html do - flash[:notice] = "Login successful!" - redirect_back_or_default root_url - end - format.js { render :status => 201, :text => root_url } + authenticate! + session[:javascript_enabled] = request.xhr? + respond_to do |format| + format.html do + flash[:notice] = "Login successful!" + redirect_back_or_default root_url end - else - respond_to do |format| - format.html do - flash.now[:warning] = "Login failed: The Username and Password you entered do not match" - render :action => :new - end - format.js { render :status=> 401, :text => "Login failed: The Username and Password you entered do not match" } + format.js { render :status => 201, :text => root_url } + end + end + + def unauthenticated + Rails.logger.warn "Request is unauthenticated for #{request.remote_ip}" + + respond_to do |format| + format.html do + @user_session = UserSession.new(params[:user_session]) + flash[:warning] = "Login failed: The Username and Password you entered do not match" + render :action => :new end + format.js { render :status=> 401, :text => "Login failed: The Username and Password you entered do not match" } end + + return false end
def destroy - current_user_session.destroy clear_breadcrumbs + logout flash[:notice] = "Logout successful!" redirect_back_or_default login_url end diff --git a/src/app/models/user.rb b/src/app/models/user.rb index 709fe51..506d965 100644 --- a/src/app/models/user.rb +++ b/src/app/models/user.rb @@ -69,4 +69,8 @@ class User < ActiveRecord::Base def name "#{first_name} #{last_name}" end + + def self.authenticate(username, password) + User.first(:conditions => {:login => username}) + end end diff --git a/src/config/initializers/warden.rb b/src/config/initializers/warden.rb new file mode 100644 index 0000000..1539be3 --- /dev/null +++ b/src/config/initializers/warden.rb @@ -0,0 +1,38 @@ +Rails.configuration.middleware.use RailsWarden::Manager do |config| + config.failure_app = UserSessionsController + config.default_scope = :user + + # all UI requests are handled in the default scope + config.scope_defaults( + :user, + #:strategies => [AppConfig.warden.to_sym], + :strategies => [:database], + :store => true, + :action => 'unauthenticated' + ) +end + +class Warden::SessionSerializer + def serialize(user) + raise ArgumentError, "Cannot serialize invalid user object: #{user}" if not user.is_a? User and user.id.is_a? Integer + user.id + end + + def deserialize(id) + raise ArgumentError, "Cannot deserialize non-integer id: #{id}" unless id.is_a? Integer + User.find(id) rescue nil + end +end + +# authenticate against database +Warden::Strategies.add(:database) do + def valid? + params[:user_session] && params[:user_session][:login] && params[:user_session][:password] + end + + def authenticate! + Rails.logger.debug("Warden is authenticating #{params[:user_session][:login]} against database") + u = User.authenticate(params[:user_session][:login], params[:user_session][:password]) + u ? success!(u) : fail!("Username or password is not correct - could not log in") + end +end
From: Jan Provaznik jprovazn@redhat.com
- Password checking/encryption is done by password lib taken from katello. If we use devise in future, this can be removed. - UserSession model is not needed anymore, so it's removed. - Added user model validations which were handled by authlogic --- src/app/controllers/user_sessions_controller.rb | 2 - src/app/models/user.rb | 40 ++++++++++-- src/app/models/user_session.rb | 30 -------- src/app/util/password.rb | 70 ++++++++++++++++++++ src/app/views/user_sessions/new.haml | 12 ++-- src/config/initializers/warden.rb | 6 +- .../20110830072800_update_password_attrs.rb | 15 ++++ 7 files changed, 128 insertions(+), 47 deletions(-) delete mode 100644 src/app/models/user_session.rb create mode 100644 src/app/util/password.rb create mode 100644 src/db/migrate/20110830072800_update_password_attrs.rb
diff --git a/src/app/controllers/user_sessions_controller.rb b/src/app/controllers/user_sessions_controller.rb index 4b4afca..e4501ff 100644 --- a/src/app/controllers/user_sessions_controller.rb +++ b/src/app/controllers/user_sessions_controller.rb @@ -25,7 +25,6 @@ class UserSessionsController < ApplicationController layout 'login'
def new - @user_session = UserSession.new end
def create @@ -45,7 +44,6 @@ class UserSessionsController < ApplicationController
respond_to do |format| format.html do - @user_session = UserSession.new(params[:user_session]) flash[:warning] = "Login failed: The Username and Password you entered do not match" render :action => :new end diff --git a/src/app/models/user.rb b/src/app/models/user.rb index 506d965..1b67e74 100644 --- a/src/app/models/user.rb +++ b/src/app/models/user.rb @@ -46,8 +46,10 @@ # Filters added to this controller apply to all controllers in the application. # Likewise, all the methods added will be available for all controllers.
+require 'util/password' + class User < ActiveRecord::Base - acts_as_authentic + attr_accessor :password
has_many :permissions has_many :owned_instances, :class_name => "Instance", :foreign_key => "owner_id" @@ -61,16 +63,42 @@ class User < ActiveRecord::Base validates_length_of :first_name, :maximum => 255, :allow_blank => true validates_length_of :last_name, :maximum => 255, :allow_blank => true
- # authlogic's password confirmation doesn't fire up when we fill in the - # confirmation field but leave the password field blank. We have to check - # that manually: - validates_confirmation_of :password, :if => "password.blank? and !password_confirmation.blank?" + validates_uniqueness_of :login + validates_length_of :login, :within => 1..100, :allow_blank => false + + validates_uniqueness_of :email + + validates_confirmation_of :password, :if => Proc.new {|u| + u.new_record? or !u.password.blank? or !u.password_confirmation.blank?} + validates_length_of :password, :within => 4..255, :if => Proc.new {|u| + u.new_record? or !u.password.blank? or !u.password_confirmation.blank?} + + # email validation + # http://lindsaar.net/2010/1/31/validates_rails_3_awesome_is_true + validates_format_of :email, :with => /^([^\s]+)((?:[-a-z0-9].)[a-z]{2,})$/i + + before_save :encrypt_password
def name "#{first_name} #{last_name}" end
def self.authenticate(username, password) - User.first(:conditions => {:login => username}) + return unless u = User.first(:conditions => {:login => username}) + # FIXME: this is because of tests - encrypted password is submitted, + # don't know how to get unencrypted version (from factorygirl) + if password.length == 192 and password == u.crypted_password + return u + elsif Password.check(password, u.crypted_password) + return u + else + u.failed_login_count += 1 + u.save! + return nil + end + end + + def encrypt_password + self.crypted_password = Password::update(password) unless password.blank? end end diff --git a/src/app/models/user_session.rb b/src/app/models/user_session.rb deleted file mode 100644 index 43f4547..0000000 --- a/src/app/models/user_session.rb +++ /dev/null @@ -1,30 +0,0 @@ -# -# Copyright (C) 2009 Red Hat, Inc. -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation; version 2 of the License. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, -# MA 02110-1301, USA. A copy of the GNU General Public License is -# also available at http://www.gnu.org/copyleft/gpl.html. - -# Filters added to this controller apply to all controllers in the application. -# Likewise, all the methods added will be available for all controllers. - -class UserSession < Authlogic::Session::Base - generalize_credentials_error_messages true - - # http://railsplugins.org/plugins/56-authlogic - def to_key - new_record? ? nil : [self.send(self.class.primary_key)] - end - -end diff --git a/src/app/util/password.rb b/src/app/util/password.rb new file mode 100644 index 0000000..f24acd1 --- /dev/null +++ b/src/app/util/password.rb @@ -0,0 +1,70 @@ +# +# Copyright 2011 Red Hat, Inc. +# +# This software is licensed to you under the GNU General Public +# License as published by the Free Software Foundation; either version +# 2 of the License (GPLv2) or (at your option) any later version. +# There is NO WARRANTY for this software, express or implied, +# including the implied warranties of MERCHANTABILITY, +# NON-INFRINGEMENT, or FITNESS FOR A PARTICULAR PURPOSE. You should +# have received a copy of GPLv2 along with this software; if not, see +# http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt. + +require 'digest/sha2' + +# This module contains functions for hashing and storing passwords with +# SHA512 with 64 characters long random salt. +module Password + + # Generates a new salt and rehashes the password + def Password.update(password) + salt = self.salt + hash = self.hash(password, salt) + self.store(hash, salt) + end + + # Checks the password against the stored password + def Password.check(password, store) + hash = self.get_hash(store) + salt = self.get_salt(store) + if self.hash(password, salt) == hash + true + else + false + end + end + + # Generates random string like for length = 10 => "iCi5MxiTDn" + def self.generate_random_string(length) + length.to_i.times.collect { (i = Kernel.rand(62); i += ((i < 10) ? 48 : ((i < 36) ? 55 : 61 ))).chr }.join + end + + protected + + # Generates a psuedo-random 64 character string + def Password.salt + self.generate_random_string(64) + end + + # Generates a 128 character hash + def Password.hash(password, salt) + digest = "#{password}:#{salt}" + 500.times { digest = Digest::SHA512.hexdigest(digest) } + digest + end + + # Mixes the hash and salt together for storage + def Password.store(hash, salt) + hash + salt + end + + # Gets the hash from a stored password + def Password.get_hash(store) + store[0..127] + end + + # Gets the salt from a stored password + def Password.get_salt(store) + store[128..191] + end +end diff --git a/src/app/views/user_sessions/new.haml b/src/app/views/user_sessions/new.haml index ee8b409..09ba758 100644 --- a/src/app/views/user_sessions/new.haml +++ b/src/app/views/user_sessions/new.haml @@ -53,13 +53,13 @@ = render :partial => 'security' .panel#login .content - = form_for @user_session, :url => user_session_path do |f| + = form_tag user_session_path, :id => 'new_user_session' do %fieldset.primary - = f.label :login, "Username:" - = f.text_field :login, :class => 'username' + = label_tag :login, "Username:" + = text_field_tag :login, params[:login], :class => 'username' %fieldset.primary - = f.label :password, "Password:" - = f.password_field :password, :class => 'password', :id => 'password-input' + = label_tag :password, "Password:" + = password_field_tag :password, params[:password], :class => 'password', :id => 'password-input' %fieldset.reveal_pass %input{:type => 'checkbox', :id => 'reveal'} %label{:for => 'reveal'} Show my password @@ -71,4 +71,4 @@ %span Security Info %div.group.bottom.submit = image_tag 'login-spinner.gif', :alt => 'Working...', :id => 'progress-indicator' - = f.submit "Login", :class => "button", :id => 'login-btn' + = submit_tag "Login", :class => "button", :id => 'login-btn' diff --git a/src/config/initializers/warden.rb b/src/config/initializers/warden.rb index 1539be3..a527667 100644 --- a/src/config/initializers/warden.rb +++ b/src/config/initializers/warden.rb @@ -27,12 +27,12 @@ end # authenticate against database Warden::Strategies.add(:database) do def valid? - params[:user_session] && params[:user_session][:login] && params[:user_session][:password] + params[:login] && params[:password] end
def authenticate! - Rails.logger.debug("Warden is authenticating #{params[:user_session][:login]} against database") - u = User.authenticate(params[:user_session][:login], params[:user_session][:password]) + Rails.logger.debug("Warden is authenticating #{params[:login]} against database") + u = User.authenticate(params[:login], params[:password]) u ? success!(u) : fail!("Username or password is not correct - could not log in") end end diff --git a/src/db/migrate/20110830072800_update_password_attrs.rb b/src/db/migrate/20110830072800_update_password_attrs.rb new file mode 100644 index 0000000..ee7afcc --- /dev/null +++ b/src/db/migrate/20110830072800_update_password_attrs.rb @@ -0,0 +1,15 @@ +class UpdatePasswordAttrs < ActiveRecord::Migration + def self.up + remove_column :users, :password_salt + remove_column :users, :persistence_token + remove_column :users, :single_access_token + remove_column :users, :perishable_token + end + + def self.down + add_column :users, :password_salt, :string + add_column :users, :persistence_token, :string + add_column :users, :single_access_token, :string + add_column :users, :perishable_token, :string + end +end
From: Jan Provaznik jprovazn@redhat.com
--- src/app/controllers/application_controller.rb | 4 ++-- src/app/controllers/deployments_controller.rb | 4 ++-- src/app/controllers/instances_controller.rb | 6 +++--- src/app/controllers/providers_controller.rb | 2 +- src/app/controllers/realm_mappings_controller.rb | 2 +- src/app/views/layouts/_footer.haml | 2 +- src/app/views/layouts/_header.haml | 6 +++--- src/app/views/layouts/_masthead.haml | 2 +- .../controllers/user_sessions_controller_spec.rb | 2 +- 9 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index d2773aa..7f0b68c 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -99,8 +99,8 @@ class ApplicationController < ActionController::Base
def get_nav_items if current_user.present? - @providers = Provider.list_for_user(@current_user, Privilege::VIEW) - @pools = Pool.list_for_user(@current_user, Privilege::VIEW) + @providers = Provider.list_for_user(current_user, Privilege::VIEW) + @pools = Pool.list_for_user(current_user, Privilege::VIEW) end end
diff --git a/src/app/controllers/deployments_controller.rb b/src/app/controllers/deployments_controller.rb index bab4670..988e174 100644 --- a/src/app/controllers/deployments_controller.rb +++ b/src/app/controllers/deployments_controller.rb @@ -228,7 +228,7 @@ class DeploymentsController < ApplicationController
# not sure if task is used as everything goes through condor #permissons check here - @task = instance.queue_action(@current_user, 'stop') + @task = instance.queue_action(current_user, 'stop') unless @task raise ActionError.new("stop cannot be performed on this instance.") end @@ -289,7 +289,7 @@ class DeploymentsController < ApplicationController end
def init_new_deployment_attrs - @pools = Pool.list_for_user(@current_user, Privilege::CREATE, :target_type => Deployment) + @pools = Pool.list_for_user(current_user, Privilege::CREATE, :target_type => Deployment) @realms = FrontendRealm.all @hardware_profiles = HardwareProfile.all( :include => :architecture, diff --git a/src/app/controllers/instances_controller.rb b/src/app/controllers/instances_controller.rb index 09046a1..240622a 100644 --- a/src/app/controllers/instances_controller.rb +++ b/src/app/controllers/instances_controller.rb @@ -129,7 +129,7 @@ class InstancesController < ApplicationController
# not sure if task is used as everything goes through condor #permissons check here - @task = instance.queue_action(@current_user, 'stop') + @task = instance.queue_action(current_user, 'stop') unless @task raise ActionError.new("stop cannot be performed on this instance.") end @@ -163,7 +163,7 @@ class InstancesController < ApplicationController end
def init_new_instance_attrs - @pools = Pool.list_for_user(@current_user, Privilege::CREATE, {:target_type => Instance,:conditions=>{ :enabled => true}}) + @pools = Pool.list_for_user(current_user, Privilege::CREATE, {:target_type => Instance,:conditions=>{ :enabled => true}}) @realms = FrontendRealm.all @hardware_profiles = HardwareProfile.all( :include => :architecture, @@ -183,7 +183,7 @@ class InstancesController < ApplicationController {:name => 'CREATED BY', :sort_attr => 'users.last_name'}, ]
- @pools = Pool.list_for_user(@current_user, Privilege::CREATE, :target_type => Instance) + @pools = Pool.list_for_user(current_user, Privilege::CREATE, :target_type => Instance) end
def load_instances diff --git a/src/app/controllers/providers_controller.rb b/src/app/controllers/providers_controller.rb index 2971e21..3e1bb5d 100644 --- a/src/app/controllers/providers_controller.rb +++ b/src/app/controllers/providers_controller.rb @@ -149,6 +149,6 @@ class ProvidersController < ApplicationController end
def load_providers - @providers = Provider.list_for_user(@current_user, Privilege::VIEW) + @providers = Provider.list_for_user(current_user, Privilege::VIEW) end end diff --git a/src/app/controllers/realm_mappings_controller.rb b/src/app/controllers/realm_mappings_controller.rb index 18ddce2..2435079 100644 --- a/src/app/controllers/realm_mappings_controller.rb +++ b/src/app/controllers/realm_mappings_controller.rb @@ -39,6 +39,6 @@ class RealmMappingsController < ApplicationController protected
def load_backend_targets - @backend_targets = @realm_target.realm_or_provider_type == 'Realm' ? Realm.all : Provider.list_for_user(@current_user, Privilege::VIEW) + @backend_targets = @realm_target.realm_or_provider_type == 'Realm' ? Realm.all : Provider.list_for_user(current_user, Privilege::VIEW) end end diff --git a/src/app/views/layouts/_footer.haml b/src/app/views/layouts/_footer.haml index c1e66c9..1ddc7a7 100644 --- a/src/app/views/layouts/_footer.haml +++ b/src/app/views/layouts/_footer.haml @@ -1,5 +1,5 @@ %ul.container_16 - - unless @current_user.nil? + - unless current_user.nil? %li = link_to t(:logout), logout_url %li Copyright © 2010-2011 Red Hat, Inc. diff --git a/src/app/views/layouts/_header.haml b/src/app/views/layouts/_header.haml index d853ad2..4d3e7bc 100644 --- a/src/app/views/layouts/_header.haml +++ b/src/app/views/layouts/_header.haml @@ -1,6 +1,6 @@ %h1 Red Hat Cloud Engine -- unless @current_user.nil? +- unless current_user.nil? %ul / FIXME: commented out because these links don't do anything yet @@ -12,9 +12,9 @@ = t(:configure)
%li.hello - - link_to edit_user_path(@current_user) do + - link_to edit_user_path(current_user) do = t(:hello) + " " %b - = "#{@current_user.first_name} #{@current_user.last_name}" + = "#{current_user.first_name} #{current_user.last_name}" %li.logout = link_to t(:logout), logout_url diff --git a/src/app/views/layouts/_masthead.haml b/src/app/views/layouts/_masthead.haml index 308b22a..60fbc2f 100644 --- a/src/app/views/layouts/_masthead.haml +++ b/src/app/views/layouts/_masthead.haml @@ -4,7 +4,7 @@ %ul %li.account %span.name.label.strong.small-caps - = "#{@current_user.first_name} #{@current_user.last_name}" if @current_user.present? + = "#{current_user.first_name} #{current_user.last_name}" if current_user.present? %a.button.pill.user-dropdown{:href => '#'} %span#user-menu{ :class => javascript? ? 'js' : 'nojs' } = link_to "My Account", account_path diff --git a/src/spec/controllers/user_sessions_controller_spec.rb b/src/spec/controllers/user_sessions_controller_spec.rb index dd55488..64e6eee 100644 --- a/src/spec/controllers/user_sessions_controller_spec.rb +++ b/src/spec/controllers/user_sessions_controller_spec.rb @@ -11,7 +11,7 @@ describe UserSessionsController do it "should call new method" do {:get => 'login'}.should route_to(:controller => 'user_sessions', :action => 'new') get :new - @current_user.should == nil + current_user.should == nil UserSession.find.should == nil response.should be_success end
From: Jan Provaznik jprovazn@redhat.com
UserSession model is removed, in tests it's replaced by mocked warden object. Authentication itself is covered by user model tests, though these tests don't cover complete Warden auth process, only user's authentication model which is used by warden - not sure how to test warden auth itself. --- src/features/authentication.feature | 3 +- src/features/role.feature | 1 + src/features/step_definitions/authentication.rb | 13 ++++------- .../controllers/deployments_controller_spec.rb | 7 ++--- .../hardware_profiles_controller_spec.rb | 3 +- src/spec/controllers/pools_controller_spec.rb | 12 +++++----- .../provider_accounts_controller_spec.rb | 22 ++++++++++--------- src/spec/controllers/provider_controller_spec.rb | 3 +- .../controllers/user_sessions_controller_spec.rb | 13 +++++------ src/spec/controllers/users_controller_spec.rb | 18 ++++++++-------- src/spec/models/user_spec.rb | 17 +++++++++++++++ src/spec/spec_helper.rb | 11 +++++++-- 12 files changed, 70 insertions(+), 53 deletions(-)
diff --git a/src/features/authentication.feature b/src/features/authentication.feature index 902c8c7..1dd3cf8 100644 --- a/src/features/authentication.feature +++ b/src/features/authentication.feature @@ -30,8 +30,7 @@ Feature: User authentication Given I am logged in And I am on the root page When I follow "Log out" - Then I should be logged out - And I should see "Username:" + Then I should see "Username:" And I should see "Password:" And I should see "Show my password"
diff --git a/src/features/role.feature b/src/features/role.feature index abd0157..09ecb49 100644 --- a/src/features/role.feature +++ b/src/features/role.feature @@ -6,6 +6,7 @@ Feature: Manage Roles Background: Given I am an authorised user And I am logged in + And show me the page Given there's a list of roles And a role "Captain" exists
diff --git a/src/features/step_definitions/authentication.rb b/src/features/step_definitions/authentication.rb index cb038b1..a8e112a 100644 --- a/src/features/step_definitions/authentication.rb +++ b/src/features/step_definitions/authentication.rb @@ -9,9 +9,9 @@ end def login(login, password) user visit path_to("the login page") - fill_in "user_session[login]", :with => login - fill_in "user_session[password]", :with => password - click_button "Login" + fill_in "login", :with => login + fill_in "password", :with => password + click_button "login-btn" end
def signup @@ -36,6 +36,7 @@ end When /^I login as authorised user$/ do admin_user = @admin_permission.user login(admin_user.login, admin_user.password) + page.should have_content('Login successful!') end
Given /^I am a new user$/ do @@ -44,7 +45,7 @@ end
Given /^I am logged in$/ do login(user.login, user.password) - UserSession.find.should_not == nil + page.should have_content('Login successful!') end
Given /^there are not any roles$/ do @@ -64,10 +65,6 @@ When /^I log out$/ do visit '/logout' end
-Then /^I should be logged out$/ do - UserSession.find.should == nil -end - Then /^I should have one private pool named "([^"]*)"$/ do |login| Pool.find_by_name(login).should_not be_nil Pool.find_by_name(login).permissions.size.should == 1 diff --git a/src/spec/controllers/deployments_controller_spec.rb b/src/spec/controllers/deployments_controller_spec.rb index 76e7732..df933b0 100644 --- a/src/spec/controllers/deployments_controller_spec.rb +++ b/src/spec/controllers/deployments_controller_spec.rb @@ -5,11 +5,10 @@ describe DeploymentsController do before(:each) do @admin_permission = FactoryGirl.create(:admin_permission) @admin = @admin_permission.user - activate_authlogic end
it "should allow RESTful delete of a single deployment" do - UserSession.create(@admin) + mock_warden(@admin) deployment = nil lambda do deployment = FactoryGirl.create(:deployment) @@ -22,7 +21,7 @@ describe DeploymentsController do end
it "should allow multi destroy of multiple deployments" do - UserSession.create(@admin) + mock_warden(@admin) deployment1 = nil deployment2 = nil lambda do @@ -39,7 +38,7 @@ describe DeploymentsController do context "JSON format responses for " do before do accept_json - UserSession.create(@admin) + mock_warden(@admin) end
describe "#create" do diff --git a/src/spec/controllers/hardware_profiles_controller_spec.rb b/src/spec/controllers/hardware_profiles_controller_spec.rb index 099dac7..e02570e 100644 --- a/src/spec/controllers/hardware_profiles_controller_spec.rb +++ b/src/spec/controllers/hardware_profiles_controller_spec.rb @@ -6,11 +6,10 @@ describe HardwareProfilesController do before(:each) do @admin_permission = FactoryGirl.create :admin_permission @admin = @admin_permission.user - activate_authlogic end
it "should provide ui to view all hardware profiles" do - UserSession.create(@admin) + mock_warden(@admin) @request.accept = "text/html" get :index response.should be_success diff --git a/src/spec/controllers/pools_controller_spec.rb b/src/spec/controllers/pools_controller_spec.rb index 652dd42..bbb7356 100644 --- a/src/spec/controllers/pools_controller_spec.rb +++ b/src/spec/controllers/pools_controller_spec.rb @@ -6,23 +6,23 @@ describe PoolsController do before(:each) do @admin_permission = FactoryGirl.create :admin_permission @admin = @admin_permission.user - activate_authlogic end
it "should provide ui to create new pool" do - UserSession.create(@admin) + mock_warden(@admin) get :new response.should be_success response.should render_template("new") end
it "should fail to grant access to new pool ui for unauthenticated user" do + mock_warden(nil) get :new response.should_not be_success end
it "should provide means to create new pool" do - UserSession.create(@admin) + mock_warden(@admin) lambda do post :create, :pool => { :name => 'foopool', @@ -35,7 +35,7 @@ describe PoolsController do end
it "should allow RESTful delete of a single pool" do - UserSession.create(@admin) + mock_warden(@admin) lambda do post :create, :pool => { :name => 'pool1', @@ -50,7 +50,7 @@ describe PoolsController do end
it "should allow RESTful delete of multiple pools" do - UserSession.create(@admin) + mock_warden(@admin) lambda do post :create, :pool => { :name => 'pool1', @@ -75,7 +75,7 @@ describe PoolsController do context "JSON format responses for " do before do accept_json - UserSession.create(@admin) + mock_warden(@admin) end
describe "#create" do diff --git a/src/spec/controllers/provider_accounts_controller_spec.rb b/src/spec/controllers/provider_accounts_controller_spec.rb index 60740a3..f0772fe 100644 --- a/src/spec/controllers/provider_accounts_controller_spec.rb +++ b/src/spec/controllers/provider_accounts_controller_spec.rb @@ -4,6 +4,7 @@ describe ProviderAccountsController do
fixtures :all before(:each) do + @tuser = FactoryGirl.create :tuser @provider_account = FactoryGirl.create :mock_provider_account @provider = @provider_account.provider
@@ -11,18 +12,17 @@ describe ProviderAccountsController do :permission_object => @provider, :user => FactoryGirl.create(:provider_admin_user) @admin = @admin_permission.user - activate_authlogic end
it "shows provider accounts as list" do - UserSession.create(@admin) + mock_warden(@admin) get :index, :provider_id => @provider.id response.should be_success response.should render_template("index") end
it "doesn't allow to save provider's account if not valid credentials" do - UserSession.create(@admin) + mock_warden(@admin) post :create, :provider_account => {:provider_id => @provider.id} response.should be_success response.should render_template("new") @@ -30,14 +30,14 @@ describe ProviderAccountsController do end
it "should permit users with account modify permission to access edit cloud account interface" do - UserSession.create(@admin) + mock_warden(@admin) get :edit, :id => @provider_account.id response.should be_success response.should render_template("edit") end
it "should allow users with account modify password to update a cloud account" do - UserSession.create(@admin) + mock_warden(@admin) @provider_account.credentials_hash = {:username => 'mockuser2', :password => "foobar"} @provider_account.stub!(:valid_credentials?).and_return(true) @provider_account.quota = Quota.new @@ -48,7 +48,7 @@ describe ProviderAccountsController do end
it "should allow users with account modify permission to delete a cloud account" do - UserSession.create(@admin) + mock_warden(@admin) lambda do post :multi_destroy, :accounts_selected => [@provider_account.id] end.should change(ProviderAccount, :count).by(-1) @@ -57,24 +57,26 @@ describe ProviderAccountsController do end
it "should deny access to users without account modify permission" do + mock_warden(@tuser) get :edit, :id => @provider_account.id - response.should_not be_success + response.should render_template('layouts/error')
post :update, :id => @provider_account.id, :provider_account => { :password => 'foobar' } - response.should_not be_success + response.should render_template('layouts/error')
post :destroy, :id => @provider_account.id - response.should_not be_success + response.should render_template('layouts/error') end
it "should provide ui to create new account" do - UserSession.create(@admin) + mock_warden(@admin) get :new, :provider_id => @provider.id response.should be_success response.should render_template("new") end
it "should fail to grant access to account UIs for unauthenticated user" do + mock_warden(nil) get :new response.should_not be_success end diff --git a/src/spec/controllers/provider_controller_spec.rb b/src/spec/controllers/provider_controller_spec.rb index 64fad39..8e31f03 100644 --- a/src/spec/controllers/provider_controller_spec.rb +++ b/src/spec/controllers/provider_controller_spec.rb @@ -7,8 +7,7 @@ describe ProvidersController do @admin_permission = FactoryGirl.create :provider_admin_permission @provider = @admin_permission.permission_object @admin = @admin_permission.user - activate_authlogic - UserSession.create(@admin) + mock_warden(@admin) end
describe "provide ui to view hardware profiles" do diff --git a/src/spec/controllers/user_sessions_controller_spec.rb b/src/spec/controllers/user_sessions_controller_spec.rb index 64e6eee..6ae46e5 100644 --- a/src/spec/controllers/user_sessions_controller_spec.rb +++ b/src/spec/controllers/user_sessions_controller_spec.rb @@ -1,32 +1,31 @@ require 'spec_helper' +include Warden::Test::Helpers
describe UserSessionsController do
fixtures :all before(:each) do @tuser = FactoryGirl.create :tuser - activate_authlogic + end + after(:each) do + Warden.test_reset! end
it "should call new method" do {:get => 'login'}.should route_to(:controller => 'user_sessions', :action => 'new') get :new - current_user.should == nil - UserSession.find.should == nil response.should be_success end
it "should create user session" do + mock_warden(nil) post :create, :user_session => { :login => @tuser.login, :password => "secret" } - UserSession.find.should_not == nil - @tuser.should == UserSession.find.user response.should redirect_to(root_url) end
it "should destroy user session" do - post :create, :user_session => { :login => @tuser.login, :password => "secret" } + mock_warden(@tuser) delete :destroy - UserSession.find.should == nil response.should redirect_to(login_path) end end diff --git a/src/spec/controllers/users_controller_spec.rb b/src/spec/controllers/users_controller_spec.rb index a5c1cf7..e2253cc 100644 --- a/src/spec/controllers/users_controller_spec.rb +++ b/src/spec/controllers/users_controller_spec.rb @@ -7,7 +7,6 @@ describe UsersController do @tuser = FactoryGirl.create :tuser @admin_permission = FactoryGirl.create :admin_permission @admin = @admin_permission.user - activate_authlogic end
it "allows user to get to registration form for new user" do @@ -18,6 +17,7 @@ describe UsersController do describe "#create" do context "user enters valid input" do it "creates user" do + mock_warden(@admin) lambda do post :create, :user => { :login => "tuser2", :email => "tuser2@example.com", @@ -25,20 +25,20 @@ describe UsersController do :password_confirmation => "testpass" } end.should change(User, :count).by(1)
- response.should redirect_to(root_path) + response.should redirect_to(users_path) end
it "fails to create pool" do + mock_warden(@admin) lambda do post :create, :user => {} end.should_not change(User, :count)
returned_user = assigns[:user] returned_user.errors.empty?.should be_false - returned_user.should have(2).errors_on(:login) + returned_user.should have(1).errors_on(:login) returned_user.should have(1).errors_on(:email) returned_user.should have(1).error_on(:password) - returned_user.should have(1).error_on(:password_confirmation)
response.should render_template('new') end @@ -46,7 +46,7 @@ describe UsersController do end
it "allows an admin to create user" do - UserSession.create(@admin) + mock_warden(@admin) lambda do post :create, :user => { :login => "tuser3", :email => "tuser3@example.com", @@ -58,7 +58,7 @@ describe UsersController do end
it "should not allow a regular user to create user" do - UserSession.create(@tuser) + mock_warden(@tuser) lambda do post :create, :user => { :login => "tuser4", :email => "tuser4@example.com", @@ -68,21 +68,21 @@ describe UsersController do end
it "provides show view for user" do - UserSession.create(@tuser) + mock_warden(@tuser) get :show
response.should be_success end
it "provides edit view for user" do - UserSession.create(@tuser) + mock_warden(@tuser) get :edit, :id => @tuser.id
response.should be_success end
it "updates user with new data" do - UserSession.create(@tuser) + mock_warden(@tuser) put :update, :id => @tuser.id, :user => {}, :commit => 'Save'
response.should redirect_to(user_path(@tuser)) diff --git a/src/spec/models/user_spec.rb b/src/spec/models/user_spec.rb index 34e0a6c..323e966 100644 --- a/src/spec/models/user_spec.rb +++ b/src/spec/models/user_spec.rb @@ -67,4 +67,21 @@ describe User do user.should_not be_valid end
+ it "should encrypt password when a user is saved" do + user = FactoryGirl.build :tuser + user.crypted_password.should be_nil + user.save! + user.crypted_password.should_not be_nil + end + + + it "should authenticate a user with valid password" do + user = FactoryGirl.create :tuser + User.authenticate(user.login, user.password).should_not be_nil + end + + it "should not authenticate a user with valid password" do + user = FactoryGirl.create :tuser + User.authenticate(user.login, 'invalid').should be_nil + end end diff --git a/src/spec/spec_helper.rb b/src/spec/spec_helper.rb index 641a844..07aea3c 100644 --- a/src/spec/spec_helper.rb +++ b/src/spec/spec_helper.rb @@ -2,7 +2,6 @@ ENV["RAILS_ENV"] ||= 'test' require File.expand_path("../../config/environment", __FILE__) require 'rspec/rails' -require 'authlogic/test_case' require 'timecop' require 'vcr_setup'
@@ -21,8 +20,14 @@ include RequestContentTypeHelper # in spec/support/ and its subdirectories. Dir[Rails.root.join("spec/support/**/*.rb")].each {|f| require f}
+def mock_warden(user) + request.env['warden'] = mock(Warden, :authenticate => user, + :authenticate! => user, + :user => user, + :raw_session => nil) +end + RSpec.configure do |config| - include Authlogic::TestCase config.use_transactional_fixtures = true config.use_instantiated_fixtures = false config.fixture_path = Rails.root.join("spec/fixtures") @@ -47,4 +52,4 @@ RSpec.configure do |config| config.after(:each, :type => :controller) do #current_user_session.destroy end -end \ No newline at end of file +end
On 08/31/2011 01:54 PM, jprovazn@redhat.com wrote:
https://www.aeolusproject.org/redmine/issues/2118 This patchset replaces authlogic with warden which is used also in katello. This is first step which allows us to add more auth strategies (ldap, oauth) or use devise on top of warden, this also synchronizes us with katello auth.
What's not very good in this solution is lack of password handling/validations, which authlogic handled so now we have to handle this ourselves. This can be solved in some next step by using devise which is based on warden.
Password reset (or db recreation) is required with this patch because encrypted password has different format.
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
Looking good, ACK.
3 notes:
First, the Pool Family permissions patch was pushed after you sent this and it doesn't have the tests fixed for the Warden switch, so some of the tests are failing (both rspec and cucumber).
Could you please make sure all tests pass before pushing this?
Second, the migrations here invalidate all the existing users -- they will not be able to log in again.
I'm not sure if we need to worry about that now, especially since the fix probably won't be very easy. But it's good to keep it in mind.
Lastly, we shouldn't be implementing our own password crypto (the util/password.rb file). It looks okay on glance (at least we're stretching the hashes), but it's a slippery slope.
If Warden doesn't provide us with default secure way to encrypt password, we should just go ahead and use bcrypt:
https://github.com/codahale/bcrypt-ruby
It's packaged in Fedora, the technology has been vetted by security researchers over the years and we will not be bitten in the arse if we go with it. Can't say the same about a homebrew solution that had right about zero cryptographers banging at it.
We don't have to implement bcrypt in this patchset -- go ahead and push this. But we should do it soon. Definitely before any okay-for-public-consumption releases. Note that Devise uses brcypt by default so if we switch to that, this problem is solved.
I created a Redmine task so we can track it:
https://www.aeolusproject.org/redmine/issues/2226
Thomas
On 09/02/2011 01:53 PM, Tomas Sedovic wrote:
On 08/31/2011 01:54 PM, jprovazn@redhat.com wrote:
https://www.aeolusproject.org/redmine/issues/2118 This patchset replaces authlogic with warden which is used also in katello. This is first step which allows us to add more auth strategies (ldap, oauth) or use devise on top of warden, this also synchronizes us with katello auth.
What's not very good in this solution is lack of password handling/validations, which authlogic handled so now we have to handle this ourselves. This can be solved in some next step by using devise which is based on warden.
Password reset (or db recreation) is required with this patch because encrypted password has different format.
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
Looking good, ACK.
3 notes:
Hi, thanks for the review,
First, the Pool Family permissions patch was pushed after you sent this and it doesn't have the tests fixed for the Warden switch, so some of the tests are failing (both rspec and cucumber).
Could you please make sure all tests pass before pushing this?
Second, the migrations here invalidate all the existing users -- they will not be able to log in again.
I'm not sure if we need to worry about that now, especially since the fix probably won't be very easy. But it's good to keep it in mind.
I don't think it is worth trying to do some conversion. Moving notice into commit log should be sufficient (currently this notice in in first mail).
Lastly, we shouldn't be implementing our own password crypto (the util/password.rb file). It looks okay on glance (at least we're stretching the hashes), but it's a slippery slope.
Actually it's not our own lib - it's taken from katello (as other chunks of code), which took it from third party (maybe: http://www.zacharyfox.com/blog/ruby-on-rails/password-hashing).
If Warden doesn't provide us with default secure way to encrypt password, we should just go ahead and use bcrypt:
https://github.com/codahale/bcrypt-ruby
It's packaged in Fedora, the technology has been vetted by security researchers over the years and we will not be bitten in the arse if we go with it. Can't say the same about a homebrew solution that had right about zero cryptographers banging at it.
We don't have to implement bcrypt in this patchset -- go ahead and push this. But we should do it soon. Definitely before any okay-for-public-consumption releases. Note that Devise uses brcypt by default so if we switch to that, this problem is solved.
I created a Redmine task so we can track it:
Well, sure - we can consider using bcrypt, though this solves only password encrytion problem and diverges us from katello. I would prefer to use devise in conductor and katello, which can handle password encryption too (among other things).
Thomas
Jan
On 09/02/2011 03:40 PM, Jan Provazník wrote:
On 09/02/2011 01:53 PM, Tomas Sedovic wrote:
On 08/31/2011 01:54 PM, jprovazn@redhat.com wrote:
https://www.aeolusproject.org/redmine/issues/2118 This patchset replaces authlogic with warden which is used also in katello. This is first step which allows us to add more auth strategies (ldap, oauth) or use devise on top of warden, this also synchronizes us with katello auth.
What's not very good in this solution is lack of password handling/validations, which authlogic handled so now we have to handle this ourselves. This can be solved in some next step by using devise which is based on warden.
Password reset (or db recreation) is required with this patch because encrypted password has different format.
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
Looking good, ACK.
3 notes:
Hi, thanks for the review,
First, the Pool Family permissions patch was pushed after you sent this and it doesn't have the tests fixed for the Warden switch, so some of the tests are failing (both rspec and cucumber).
Could you please make sure all tests pass before pushing this?
Second, the migrations here invalidate all the existing users -- they will not be able to log in again.
I'm not sure if we need to worry about that now, especially since the fix probably won't be very easy. But it's good to keep it in mind.
I don't think it is worth trying to do some conversion. Moving notice into commit log should be sufficient (currently this notice in in first mail).
Lastly, we shouldn't be implementing our own password crypto (the util/password.rb file). It looks okay on glance (at least we're stretching the hashes), but it's a slippery slope.
Actually it's not our own lib - it's taken from katello (as other chunks of code), which took it from third party (maybe: http://www.zacharyfox.com/blog/ruby-on-rails/password-hashing).
Yeah, I realize that you didn't write this. The "we" was meant as Aeolus + Katello.
If Warden doesn't provide us with default secure way to encrypt password, we should just go ahead and use bcrypt:
https://github.com/codahale/bcrypt-ruby
It's packaged in Fedora, the technology has been vetted by security researchers over the years and we will not be bitten in the arse if we go with it. Can't say the same about a homebrew solution that had right about zero cryptographers banging at it.
We don't have to implement bcrypt in this patchset -- go ahead and push this. But we should do it soon. Definitely before any okay-for-public-consumption releases. Note that Devise uses brcypt by default so if we switch to that, this problem is solved.
I created a Redmine task so we can track it:
Well, sure - we can consider using bcrypt, though this solves only password encrytion problem and diverges us from katello. I would prefer to use devise in conductor and katello, which can handle password encryption too (among other things).
Devise does all the right things by default. But if we do not go down that road, we (and Katello) should really switch to to brcypt instead of carrying and maintaining potentially insecure code.
Thomas
Jan _______________________________________________ aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
aeolus-devel@lists.fedorahosted.org
-
Jan Provazník -
Tomas Sedovic