improve user model validation

app/controllers/sessions_controller.rb

@@ -34,7 +34,7 @@ def destroy_session
 
   def destroy
     destroy_session
-    redirect_to Warden::PpyAuthStrategy::LOGOUT_URL, allow_other_host: true
+    redirect_to Warden::PpyAuthStrategy::PPY_LOGOUT_URL, allow_other_host: true
   end
 
   def login_as

app/lib/warden/ppy_auth_strategy.rb

@@ -10,43 +10,58 @@ class Warden::PpyAuthStrategy < Devise::Strategies::Authenticatable
   TYPE = 'HTTP_PYORK_TYPE'
 
   LOGIN = ['/login']
-  LOGOUT_URL = 'https://passportyork.yorku.ca/ppylogin/ppylogout'
+  PPY_LOGOUT_URL = 'https://passportyork.yorku.ca/ppylogin/ppylogout'
 
   def authenticate!
     if valid?
-      resource = User.find_by('uid = ?', request.headers[USER])
+      resource = User.find_by('username = ?', request.headers[USER])
       if !resource.present?
         @user = User.new
         @user.password = Digest::SHA256.hexdigest(rand().to_s)
         @user.admin = false
         @user.active = true
-        @user.user_type = User::UNKNOWN if @user.user_type.nil?
+        @user.user_type = request.headers[TYPE]
         @user.role = User::INSTRUCTOR_ROLE
         @user.uid = request.headers[USER]
         @user.username = request.headers[USER]
         @user.name = "#{request.headers[FIRSTNAME]} #{request.headers[SURNAME]}"
         @user.email = request.headers[EMAIL]
         @user.univ_id = request.headers[CYIN]
         @user.audit_comment = 'PpyAuthStrategy created new user from authenticated PYORK headers'
-        @user.save(validate: false)
+
+        if !@user.valid?
+          fail!('Not authenticated. User validation failed.')
+          return false
+        end
+
+        @user.save
         UserMailer.welcome(@user).deliver_later if @user.email.present?
         resource = @user
       end
 
-      if resource.update_external_alma(request.headers[CYIN])
-        resource.audit_comment = 'Updated user information from ALMA' if resource.changed?
-      end
+      resource.user_type = request.headers[TYPE]
+      resource.uid = request.headers[USER]
+      resource.username = request.headers[USER]
+      resource.email = request.headers[EMAIL]
+      resource.univ_id = request.headers[CYIN]
 
-      if resource.univ_id.nil?
-        resource.univ_id = request.headers[CYIN]
+      if resource.changed?
+        resource.audit_comment = 'Updated user information from PYORK headers'
+        resource.save
       end
 
-      resource.save(validate: false) if resource.changed?
+      if resource.role == User::INSTRUCTOR_ROLE
+        if resource.update_external_alma(request.headers[CYIN])
+          if resource.changed?
+            resource.audit_comment = 'Updated user information from ALMA'
+            resource.save
+          end
+        end
+      end
 
       success!(resource)
     else
-      Rails.logger.debug "not valid"
-      fail!('Not authenticated')
+      fail!('Not authenticated. Warden::PpyAuthStrategy not valid.')
     end
   end
 

app/models/user.rb

@@ -30,9 +30,8 @@ class User < ApplicationRecord
 
   ## VALIDATIONS
   validates_presence_of :name, :email, :uid, :user_type, :role, :username
-  validates_presence_of :department, :phone, :office, unless: proc { |u| u.admin? }
-  validates_presence_of :location, if: proc { |u| u.admin? }
-  validates :email, presence: true, uniqueness: true, format: { with: /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i }
+
+  validates :email, uniqueness: true, format: { with: /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i }
 
   validates_uniqueness_of :uid
   validates_uniqueness_of :username

bin/import_db.sh

@@ -7,4 +7,4 @@ cat import.sql | rails db -p
 
 rails db:migrate
 
-rake searchkick:reindex_all
+rake admin:reindex_all

lib/tasks/admin.rake

@@ -0,0 +1,19 @@
+namespace :admin do
+  desc "Reindex all models"
+  task reindex_all: :environment do
+      User.reindex
+      Request.reindex
+      Item.reindex
+      Course.reindex
+      Location.reindex
+  end
+
+  desc "List invalid users"
+  task invalid_user_records: :environment do
+    User.all.each do |u|
+      if !u.valid?
+        puts "#{u.id},#{u.username},#{u.univ_id},#{u.email},#{u.role}"
+      end
+    end
+  end
+end

test/controllers/sessions_controller_test.rb

@@ -3,10 +3,6 @@
 require 'test_helper'
 
 class SessionsControllerTest < ActionDispatch::IntegrationTest
-  setup do
-    @cas_header = 'HTTP_PYORK_USER'
-    @cas_alt_header = 'HTTP_PYORK_CYIN'
-  end
 
   should 'not throw authorization not performed error when using the controller' do
     assert_nothing_raised do
@@ -17,7 +13,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
   should 'create a new session information if user logs in' do
     user = create(:user, uid: 'test', admin: true, role: User::MANAGER_ROLE)
 
-    get login_url, headers: { @cas_header.to_s => user.uid }
+    log_user_in(user)
 
     assert_equal user.id, session[:user_id]
     assert_redirected_to root_url
@@ -26,23 +22,23 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
 
   should 'test redirection to a dashboard if the staff is logged in' do
     staff = create(:user, uid: '123123123', admin: true, role: User::STAFF_ROLE)
-    get login_url, headers: { @cas_header.to_s => staff.uid }
+    log_user_in(staff)
 
     assert_equal staff.id, session[:user_id]
     assert_redirected_to root_url
   end
 
   should 'test redirection to a requests_user_url if the regulard user is logged in' do
     user = create(:user, uid: '123123123', admin: false)
-    get login_url, headers: { @cas_header.to_s => user.uid }
+    log_user_in(user)
 
     assert_equal user.id, session[:user_id]
     assert_redirected_to requests_user_url(user)
   end
 
   should 'show error 401 if user is not active' do
     user = create(:user, uid: '123123123', admin: false, active: false)
-    get login_url, headers: { @cas_header.to_s =>  user.uid }
+    log_user_in(user)
     assert_equal 401, response.status
     assert_not_nil flash[:alert]
     assert_equal 'User not active.', flash[:alert]
@@ -51,7 +47,11 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
   should "NEW USER, redirect to new user signup if user doesn't exist, no email should be sent yet" do
     ActionMailer::Base.deliveries.clear
     get logout_url
-    get login_url, headers: { @cas_header.to_s => 'something_or_other', 'HTTP_PYORK_TYPE' => User::FACULTY }
+    get login_url, headers: { 
+      'HTTP_PYORK_USER' => 'something_or_other', 'HTTP_PYORK_EMAIL' => '[email protected]', 
+      'HTTP_PYORK_CYIN' => '999999999', 'HTTP_PYORK_TYPE' => User::FACULTY,
+      'HTTP_PYORK_SURNAME' => 'doe', 'HTTP_PYORK_FIRSTNAME' => 'john'
+    }
     assert ActionMailer::Base.deliveries.empty?, "Should be empty, since we didn't get any details about user"
     assert_not_nil session[:user_id], 'Make sure user is logged in'
 
@@ -62,7 +62,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
     get logout_url
 
     assert_nil session[:user_id]
-    assert_redirected_to Warden::PpyAuthStrategy::LOGOUT_URL
+    assert_redirected_to Warden::PpyAuthStrategy::PPY_LOGOUT_URL
   end
 
   context 'logged in actions' do

test/factories/users.rb

@@ -5,14 +5,14 @@
 FactoryGirl.define do
   factory :user do
     name 'Jeremy Clarkson'
-    sequence(:email, 1000) { |n| "jereymy#{n}@yorku.ca" }
+    sequence(:email, 1000) { |n| "e#{n}@yorku.ca" }
     phone '124'
     user_type User::FACULTY
     role User::INSTRUCTOR_ROLE
     department 'History'
     office 'Some where'
-    sequence(:uid, '20900') { |n| "12#{n}" }
-    sequence(:username, '20900') { |n| "12#{n}" }
+    sequence(:uid, 1000) { |n| "uid#{n}" }
+    sequence(:username, 1000) { |n| "username#{n}" }
     active true
     admin false
 

test/models/user_test.rb

@@ -14,19 +14,20 @@ class UserTest < ActiveSupport::TestCase
   should 'not create an invalid user' do
     # common validations
     assert !build(:user, email: nil).valid?, 'Should have email'
+    assert !build(:user, email: 'whwoow').valid?, 'Email should follow proper format'
     assert !build(:user, uid: nil).valid?, 'Should have uid'
     assert !build(:user, name: nil).valid?, 'Should have a name'
     assert !build(:user, user_type: nil).valid?, 'Should have user type'
     assert !build(:user, role: nil).valid?, 'Staff must have a role set'
     assert !build(:user, username: nil).valid?, 'Should have a username'
 
-    # staff specific validations
-    assert !build(:user, admin: true, location: nil).valid?, 'Staff must have a location set'
-    assert !build(:user, email: 'whwoow').valid?, 'Email should follow proper format'
-
-    # ensure uid is unique
+    # uniqueness
     create(:user, uid: 'woot')
     assert !build(:user, uid: 'woot').valid?, 'UID must be unique'
+    create(:user, username: 'woot2')
+    assert !build(:user, username: 'woot2').valid?, 'username must be unique'
+    create(:user, email: '[email protected]')
+    assert !build(:user, email: '[email protected]').valid?, 'email must be unique'
   end
 
   should 'have staff and users scopes' do

test/test_helper.rb

@@ -61,7 +61,10 @@ class ActionDispatch::IntegrationTest
   Warden.test_mode!
 
   def log_user_in(user)
-    get login_url, headers: { 'HTTP_PYORK_USER' => user.uid }
+    get login_url, headers: { 
+      'HTTP_PYORK_USER' => user.username, 'HTTP_PYORK_EMAIL' => user.email, 
+      'HTTP_PYORK_CYIN' => user.univ_id, 'HTTP_PYORK_TYPE' => user.user_type
+    }
   end
 
   def get_instance_var(what)