From 461df4244e9157126d783d71e1652676da4dedff Mon Sep 17 00:00:00 2001 From: Sergio-e <33036058+Sergio-e@users.noreply.github.com> Date: Wed, 10 Jul 2024 19:01:05 -0600 Subject: [PATCH 1/2] Fixes & improvements to user authentication --- app/controllers/application_controller.rb | 2 - app/controllers/concerns/authentication.rb | 56 ++++++++++--------- app/controllers/password_resets_controller.rb | 6 +- app/controllers/sessions_controller.rb | 1 + app/models/user.rb | 9 ++- .../20240703041238_add_index_to_user_email.rb | 5 -- db/schema.rb | 2 +- db/seeds.rb | 4 +- spec/factories/users.rb | 1 + spec/models/user_spec.rb | 1 + 10 files changed, 44 insertions(+), 43 deletions(-) delete mode 100644 db/migrate/20240703041238_add_index_to_user_email.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 658bada1..361611fe 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,5 +1,3 @@ class ApplicationController < ActionController::Base include Authentication - - before_action :authenticate_user! end diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 1bb4e35a..55400474 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -4,31 +4,35 @@ module Authentication included do helper_method :current_user, :user_signed_in? - def authenticate_user! - redirect_to root_path, alert: t("controllers.concerns.authentication.unauthorized") unless user_signed_in? - end - - def current_user - Current.user ||= authenticate_user_from_session - end - - def authenticate_user_from_session - User.find_by(id: session[:user_id]) - end - - def user_signed_in? - current_user.present? - end - - def login(user) - Current.user = user - reset_session - session[:user_id] = user.id - end - - def logout - Current.user = nil - reset_session - end + before_action :authenticate_user! + end + + def authenticate_user! + return current_user if user_signed_in? + + redirect_to root_path, alert: t("controllers.concerns.authentication.unauthorized") + end + + def current_user + Current.user ||= authenticate_user_from_session + end + + def authenticate_user_from_session + User.find_by(id: session[:user_id]) + end + + def user_signed_in? + current_user.present? + end + + def login(user) + Current.user = user + reset_session + session[:user_id] = user.id + end + + def logout + Current.user = nil + reset_session end end diff --git a/app/controllers/password_resets_controller.rb b/app/controllers/password_resets_controller.rb index b79929ac..15f94e59 100644 --- a/app/controllers/password_resets_controller.rb +++ b/app/controllers/password_resets_controller.rb @@ -1,5 +1,6 @@ class PasswordResetsController < ApplicationController skip_before_action :authenticate_user!, only: [:new, :create] + before_action :set_user_by_token, only: [:edit, :update] def new @@ -15,7 +16,7 @@ def create PasswordMailer.with( user: @user, token: @user.generate_token_for(:password_reset) - ).password_reset.deliver_now + ).password_reset.deliver_later end redirect_to root_path, notice: t("controllers.password_resets.create.notice") @@ -33,8 +34,9 @@ def update def set_user_by_token @user = User.find_by_token_for(:password_reset, params[:token]) + return if @user.present? - redirect_to new_password_reset_path alert: t("controllers.password_resets.errors.invalid_token") if @user.blank? + redirect_to new_password_reset_path alert: t("controllers.password_resets.errors.invalid_token") end def password_params diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index ad5454d5..00e79feb 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -2,6 +2,7 @@ class SessionsController < ApplicationController skip_before_action :authenticate_user! def new + @user = User.new end def create diff --git a/app/models/user.rb b/app/models/user.rb index d37580ee..86e22a32 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -6,6 +6,7 @@ # email :string not null # in_app_notifications_enabled :boolean default(TRUE), not null # mail_notifications_enabled :boolean default(TRUE), not null +# password_digest :string not null # role :string # created_at :datetime not null # updated_at :datetime not null @@ -15,10 +16,12 @@ # index_users_on_email (email) UNIQUE # class User < ApplicationRecord - PASSWORD_RESET_EXPIRATION = 15.minutes + PASSWORD_RESET_EXPIRATION = 60.minutes normalizes :email, with: ->(email) { email.strip.downcase } + has_secure_password + has_one :profile, as: :profileable, dependent: :destroy has_and_belongs_to_many :events @@ -26,10 +29,6 @@ class User < ApplicationRecord validates :email, presence: true, uniqueness: true validates :password_digest, presence: true - normalizes :email, with: ->(email) { email.strip.downcase } - - has_secure_password - generates_token_for :password_reset, expires_in: PASSWORD_RESET_EXPIRATION do password_salt&.last(10) end diff --git a/db/migrate/20240703041238_add_index_to_user_email.rb b/db/migrate/20240703041238_add_index_to_user_email.rb deleted file mode 100644 index ff03658d..00000000 --- a/db/migrate/20240703041238_add_index_to_user_email.rb +++ /dev/null @@ -1,5 +0,0 @@ -class AddIndexToUserEmail < ActiveRecord::Migration[7.1] - def change - add_index :users, :email, unique: true - end -end diff --git a/db/schema.rb b/db/schema.rb index a6e60d62..d7c97655 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_07_03_041238) do +ActiveRecord::Schema[7.1].define(version: 2024_06_28_211903) do create_table "conferences", force: :cascade do |t| t.string "name", null: false t.datetime "created_at", null: false diff --git a/db/seeds.rb b/db/seeds.rb index ab538a33..cee8acc9 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -1,7 +1,7 @@ -conference = Conference.create!(name: "RailsWorld 2024") +conference = Conference.find_or_create_by!(name: "RailsWorld 2024") # Users -user = User.create!(email: "dev@example.com") +user = User.create!(email: "dev@example.com", password: "foobar", password_confirmation: "foobar") # Tags Tag.create!(name: "Hotwire") diff --git a/spec/factories/users.rb b/spec/factories/users.rb index bebbe6aa..14539a09 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -6,6 +6,7 @@ # email :string not null # in_app_notifications_enabled :boolean default(TRUE), not null # mail_notifications_enabled :boolean default(TRUE), not null +# password_digest :string not null # role :string # created_at :datetime not null # updated_at :datetime not null diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2b251366..db4430e7 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -6,6 +6,7 @@ # email :string not null # in_app_notifications_enabled :boolean default(TRUE), not null # mail_notifications_enabled :boolean default(TRUE), not null +# password_digest :string not null # role :string # created_at :datetime not null # updated_at :datetime not null From cb97d76c3091256462c1fc6d921c6bf01b778562 Mon Sep 17 00:00:00 2001 From: Sergio-e <33036058+Sergio-e@users.noreply.github.com> Date: Thu, 11 Jul 2024 18:50:01 -0600 Subject: [PATCH 2/2] Authentication improvements --- .env.template | 1 + .../default.md => PULL_REQUEST_TEMPLATE.md} | 0 .github/workflows/github-actions.yml | 2 + app/controllers/concerns/authentication.rb | 30 ++++++----- app/controllers/main_controller.rb | 3 +- app/controllers/password_resets_controller.rb | 6 +-- app/controllers/passwords_controller.rb | 2 +- app/controllers/registrations_controller.rb | 2 +- app/controllers/sessions_controller.rb | 5 +- app/models/user.rb | 1 + app/views/passwords/edit.html.erb | 2 +- app/views/registrations/new.html.erb | 8 +-- app/views/sessions/new.html.erb | 6 +-- config/locales/en.yml | 37 ++----------- db/seeds.rb | 2 +- .../password_resets_controller_spec.rb | 17 +++--- spec/controllers/passwords_controller_spec.rb | 3 +- .../registrations_controller_spec.rb | 1 - spec/controllers/sessions_controller_spec.rb | 5 +- spec/factories/users.rb | 2 +- spec/rails_helper.rb | 3 +- spec/support/authentication_helper.rb | 17 ++++++ spec/system/test_spec.rb | 8 --- spec/system/user_sign_in_spec.rb | 37 +++++++++++++ spec/system/user_sign_up_spec.rb | 52 +++++++++++++++++++ 25 files changed, 164 insertions(+), 88 deletions(-) rename .github/{PULL_REQUEST_TEMPLATE/default.md => PULL_REQUEST_TEMPLATE.md} (100%) create mode 100644 spec/support/authentication_helper.rb delete mode 100644 spec/system/test_spec.rb create mode 100644 spec/system/user_sign_in_spec.rb create mode 100644 spec/system/user_sign_up_spec.rb diff --git a/.env.template b/.env.template index c80d0eff..0e694f27 100644 --- a/.env.template +++ b/.env.template @@ -1,5 +1,6 @@ OVERMIND_PROCFILE=Procfile.dev HIVEMIND_PROCFILE=Procfile.dev +PORT=3000 OVERMIND_ENV=./.env.local HIVEMIND_ENV=./.env.local RSPEC_RETRY_RETRY_COUNT=0 diff --git a/.github/PULL_REQUEST_TEMPLATE/default.md b/.github/PULL_REQUEST_TEMPLATE.md similarity index 100% rename from .github/PULL_REQUEST_TEMPLATE/default.md rename to .github/PULL_REQUEST_TEMPLATE.md diff --git a/.github/workflows/github-actions.yml b/.github/workflows/github-actions.yml index 4ed4df5d..e955b403 100644 --- a/.github/workflows/github-actions.yml +++ b/.github/workflows/github-actions.yml @@ -86,6 +86,8 @@ jobs: - name: Setup DB run: | bundle exec rails db:test:prepare + - name: Assets Precompile + run: bundle exec rails assets:precompile - name: Run tests run: | bundle exec rspec diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 55400474..6414e533 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -4,25 +4,31 @@ module Authentication included do helper_method :current_user, :user_signed_in? - before_action :authenticate_user! + before_action :authenticate_user, :authenticate_user! end - def authenticate_user! - return current_user if user_signed_in? - - redirect_to root_path, alert: t("controllers.concerns.authentication.unauthorized") + class_methods do + def allow_unauthenticated_access(**options) + skip_before_action :authenticate_user!, **options + end end - def current_user - Current.user ||= authenticate_user_from_session - end + private + + def current_user = Current.user - def authenticate_user_from_session - User.find_by(id: session[:user_id]) + def user_signed_in? = Current.user.present? + + def authenticate_user + Current.user = User.find_by(id: session[:user_id]) end - def user_signed_in? - current_user.present? + def authenticate_user! + authenticate_user + + if !user_signed_in? + redirect_to new_session_path, alert: t("controllers.concerns.authentication.unauthorized") + end end def login(user) diff --git a/app/controllers/main_controller.rb b/app/controllers/main_controller.rb index f6b17f11..c98a8eaa 100644 --- a/app/controllers/main_controller.rb +++ b/app/controllers/main_controller.rb @@ -1,5 +1,6 @@ class MainController < ApplicationController - skip_before_action :authenticate_user! + allow_unauthenticated_access + def index end end diff --git a/app/controllers/password_resets_controller.rb b/app/controllers/password_resets_controller.rb index 15f94e59..8e4429f4 100644 --- a/app/controllers/password_resets_controller.rb +++ b/app/controllers/password_resets_controller.rb @@ -1,5 +1,5 @@ class PasswordResetsController < ApplicationController - skip_before_action :authenticate_user!, only: [:new, :create] + allow_unauthenticated_access before_action :set_user_by_token, only: [:edit, :update] @@ -19,7 +19,7 @@ def create ).password_reset.deliver_later end - redirect_to root_path, notice: t("controllers.password_resets.create.notice") + redirect_to new_session_path, notice: t("controllers.password_resets.create.notice") end def update @@ -36,7 +36,7 @@ def set_user_by_token @user = User.find_by_token_for(:password_reset, params[:token]) return if @user.present? - redirect_to new_password_reset_path alert: t("controllers.password_resets.errors.invalid_token") + redirect_to new_password_reset_path, alert: t("controllers.password_resets.errors.invalid_token") end def password_params diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index 82969ec9..1930c97b 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -3,7 +3,7 @@ def edit end def update - if current_user.update(password_params) + if Current.user.update(password_params) redirect_to edit_password_path, notice: t("controllers.passwords.update.notice") else render :edit, status: :unprocessable_entity diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 199ba626..37651612 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -1,5 +1,5 @@ class RegistrationsController < ApplicationController - skip_before_action :authenticate_user! + allow_unauthenticated_access def new @user = User.new diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 00e79feb..fae2553e 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,5 +1,5 @@ class SessionsController < ApplicationController - skip_before_action :authenticate_user! + allow_unauthenticated_access only: [:new, :create] def new @user = User.new @@ -12,8 +12,7 @@ def create login @user redirect_to root_path, notice: t("controllers.sessions.create.notice") else - flash[:alert] = t("controllers.sessions.create.alert") - render :new, status: :unprocessable_entity + redirect_to new_session_path, alert: t("controllers.sessions.create.alert") end end diff --git a/app/models/user.rb b/app/models/user.rb index 86e22a32..8b641a70 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -28,6 +28,7 @@ class User < ApplicationRecord validates :email, presence: true, uniqueness: true validates :password_digest, presence: true + validates :password, length: {minimum: 8}, if: -> { password.present? } generates_token_for :password_reset, expires_in: PASSWORD_RESET_EXPIRATION do password_salt&.last(10) diff --git a/app/views/passwords/edit.html.erb b/app/views/passwords/edit.html.erb index 7a30272d..b1959002 100644 --- a/app/views/passwords/edit.html.erb +++ b/app/views/passwords/edit.html.erb @@ -1,6 +1,6 @@