From a0ec497f4975dbad1394d911ecc4eb61a6e05e51 Mon Sep 17 00:00:00 2001 From: Tuan Nguyen Date: Wed, 8 Jan 2025 11:53:10 -0500 Subject: [PATCH] improve user model validation --- app/controllers/sessions_controller.rb | 2 +- app/lib/warden/ppy_auth_strategy.rb | 39 ++++++++++++++------ app/models/user.rb | 5 +-- bin/import_db.sh | 2 +- lib/tasks/admin.rake | 19 ++++++++++ lib/tasks/searchkick.rake | 10 ----- test/controllers/sessions_controller_test.rb | 20 +++++----- test/factories/users.rb | 6 +-- test/models/user_test.rb | 11 +++--- test/test_helper.rb | 5 ++- 10 files changed, 73 insertions(+), 46 deletions(-) create mode 100644 lib/tasks/admin.rake delete mode 100644 lib/tasks/searchkick.rake diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index a53f213..d1c148d 100644 --- a/app/controllers/sessions_controller.rb +++ b/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 diff --git a/app/lib/warden/ppy_auth_strategy.rb b/app/lib/warden/ppy_auth_strategy.rb index ee02470..5e9ca2d 100644 --- a/app/lib/warden/ppy_auth_strategy.rb +++ b/app/lib/warden/ppy_auth_strategy.rb @@ -10,17 +10,17 @@ 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] @@ -28,25 +28,40 @@ def authenticate! @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 diff --git a/app/models/user.rb b/app/models/user.rb index 631ee44..3916852 100644 --- a/app/models/user.rb +++ b/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 diff --git a/bin/import_db.sh b/bin/import_db.sh index 8855711..efc36e5 100755 --- a/bin/import_db.sh +++ b/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 diff --git a/lib/tasks/admin.rake b/lib/tasks/admin.rake new file mode 100644 index 0000000..3e26a9e --- /dev/null +++ b/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 diff --git a/lib/tasks/searchkick.rake b/lib/tasks/searchkick.rake deleted file mode 100644 index 3920371..0000000 --- a/lib/tasks/searchkick.rake +++ /dev/null @@ -1,10 +0,0 @@ -namespace :searchkick do - desc "Reindex all models" - task reindex_all: :environment do - User.reindex - Request.reindex - Item.reindex - Course.reindex - Location.reindex - end -end diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 3d718d3..818b46b 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/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,7 +22,7 @@ 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 @@ -34,7 +30,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest 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) @@ -42,7 +38,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest 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' => 'something@email.com', + '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 diff --git a/test/factories/users.rb b/test/factories/users.rb index 08f6138..a7f5af9 100644 --- a/test/factories/users.rb +++ b/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 diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 78d061f..22ef16a 100644 --- a/test/models/user_test.rb +++ b/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: 'woot@woot.com') + assert !build(:user, email: 'woot@woot.com').valid?, 'email must be unique' end should 'have staff and users scopes' do diff --git a/test/test_helper.rb b/test/test_helper.rb index 1261eaf..8fbb551 100644 --- a/test/test_helper.rb +++ b/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)