Use DB for auth sessions (#1233)

* DB sessions

* Validations for profile image

app/controllers/concerns/authentication.rb

@@ -2,6 +2,7 @@ module Authentication
   extend ActiveSupport::Concern
 
   included do
+    before_action :set_request_details
     before_action :authenticate_user!
   end
 
@@ -12,10 +13,9 @@ module Authentication
   end
 
   private
-
     def authenticate_user!
-      if user = User.find_by(id: session[:user_id])
+      if session_record = Session.find_by_id(cookies.signed[:session_token])
-        Current.user = user
+        Current.session = session_record
       else
         if self_hosted_first_login?
           redirect_to new_registration_url
@@ -25,23 +25,18 @@ module Authentication
       end
     end
 
-    def login(user)
+    def create_session_for(user)
-      Current.user = user
+      session = user.sessions.create!
-      reset_session
+      cookies.signed.permanent[:session_token] = { value: session.id, httponly: true }
-      session[:user_id] = user.id
+      session
-      set_last_login_at
-    end
-
-    def logout
-      Current.user = nil
-      reset_session
-    end
-
-    def set_last_login_at
-      Current.user.update(last_login_at: DateTime.now)
     end
 
     def self_hosted_first_login?
       Rails.application.config.app_mode.self_hosted? && User.count.zero?
     end
+
+    def set_request_details
+      Current.user_agent = request.user_agent
+      Current.ip_address = request.ip
+    end
 end

app/controllers/registrations_controller.rb

@@ -17,7 +17,7 @@ class RegistrationsController < ApplicationController
 
     if @user.save
       Category.create_default_categories(@user.family)
-      login @user
+      @session = create_session_for(@user)
       flash[:notice] = t(".success")
       redirect_to root_path
     else

app/controllers/sessions_controller.rb

@@ -1,4 +1,5 @@
 class SessionsController < ApplicationController
+  before_action :set_session, only: :destroy
   skip_authentication only: %i[new create]
 
   layout "auth"
@@ -8,7 +9,7 @@ class SessionsController < ApplicationController
 
   def create
     if user = User.authenticate_by(email: params[:email], password: params[:password])
-      login user
+      @session = create_session_for(user)
       redirect_to root_path
     else
       flash.now[:alert] = t(".invalid_credentials")
@@ -17,7 +18,12 @@ class SessionsController < ApplicationController
   end
 
   def destroy
-    logout
+    @session.destroy
     redirect_to root_path, notice: t(".logout_successful")
   end
+
+  private
+    def set_session
+      @session = Current.user.sessions.find(params[:id])
+    end
 end

app/controllers/settings/profiles_controller.rb

@@ -23,7 +23,7 @@ class Settings::ProfilesController < SettingsController
 
   def destroy
     if Current.user.deactivate
-      logout
+      Current.session.destroy
       redirect_to root_path, notice: t(".success")
     else
       redirect_to settings_profile_path, alert: Current.user.errors.full_messages.to_sentence
@@ -31,7 +31,6 @@ class Settings::ProfilesController < SettingsController
   end
 
   private
-
     def user_params
       params.require(:user).permit(:first_name, :last_name, :profile_image,
                                    family_attributes: [ :name, :id ])

app/helpers/auth_messages_helper.rb

@@ -1,6 +0,0 @@
-module AuthMessagesHelper
-  def auth_messages(form = nil)
-    render "shared/auth_messages", flash: flash,
-      errors: form&.object&.errors&.full_messages || []
-  end
-end

app/models/current.rb

@@ -1,5 +1,7 @@
 class Current < ActiveSupport::CurrentAttributes
-  attribute :user
+  attribute :session
+  attribute :user_agent, :ip_address
 
+  delegate :user, to: :session, allow_nil: true
   delegate :family, to: :user, allow_nil: true
 end

app/models/session.rb

@@ -0,0 +1,8 @@
+class Session < ApplicationRecord
+  belongs_to :user
+
+  before_create do
+    self.user_agent = Current.user_agent
+    self.ip_address = Current.ip_address
+  end
+end

app/models/user.rb

@@ -2,9 +2,11 @@ class User < ApplicationRecord
   has_secure_password
 
   belongs_to :family
+  has_many :sessions, dependent: :destroy
   accepts_nested_attributes_for :family
 
   validates :email, presence: true, uniqueness: true
+  validate :ensure_valid_profile_image
   normalizes :email, with: ->(email) { email.strip.downcase }
 
   normalizes :first_name, :last_name, with: ->(value) { value.strip.presence }
@@ -72,6 +74,14 @@ class User < ApplicationRecord
   end
 
   private
+    def ensure_valid_profile_image
+      return unless profile_image.attached?
+
+      unless profile_image.content_type.in?(%w[image/jpeg image/png])
+        errors.add(:profile_image, "must be a JPEG or PNG")
+        profile_image.purge
+      end
+    end
 
     def last_user_in_family?
       family.users.count == 1

app/views/layouts/_sidebar.html.erb

@@ -63,7 +63,7 @@
         <% end %>
       </div>
       <div class="p-1">
-        <%= button_to session_path, method: :delete, class: "w-full text-red-400 flex gap-1 items-center hover:bg-gray-50 rounded-lg px-3 py-2" do %>
+        <%= button_to session_path(Current.session), method: :delete, class: "w-full text-red-400 flex gap-1 items-center hover:bg-gray-50 rounded-lg px-3 py-2" do %>
           <%= lucide_icon("log-out", class: "w-5 h-5 shrink-0") %>
           <span class="text-sm">Logout</span>
         <% end %>

app/views/layouts/auth.html.erb

@@ -23,7 +23,7 @@
     </div>
 
     <div class="p-8 mt-2 text-center">
-      <p class="mt-6 text-sm text-black"><%= link_to t(".privacy_policy"), "/privacy", class: "font-medium text-gray-600 hover:text-gray-400 transition" %> &bull; <%= link_to t(".terms_of_service"), "/terms", class: "font-medium text-gray-600 hover:text-gray-400 transition" %></p>
+      <p class="mt-6 text-sm text-black"><%= link_to t(".privacy_policy"), "https://maybe.co/privacy", class: "font-medium text-gray-600 hover:text-gray-400 transition" %> &bull; <%= link_to t(".terms_of_service"), "https://maybe.co/tos", class: "font-medium text-gray-600 hover:text-gray-400 transition" %></p>
     </div>
   </div>
 <% end %>

app/views/password_resets/edit.html.erb

@@ -3,8 +3,6 @@
 %>
 
 <%= styled_form_with model: @user, url: password_reset_path(token: params[:token]), method: :patch, class: "space-y-4" do |form| %>
-  <%= auth_messages form %>
-
   <div class="relative border border-gray-100 bg-gray-25 rounded-xl focus-within:bg-white focus-within:shadow focus-within:opacity-100">
     <%= form.label :password, class: "p-4 pb-0 block text-sm font-medium text-gray-700" %>
     <%= form.password_field :password, required: "required", class: "p-4 pt-1 bg-transparent border-none opacity-50 focus:outline-none focus:ring-0 focus-within:opacity-100 w-full" %>

app/views/password_resets/new.html.erb

@@ -3,8 +3,6 @@
 %>
 
 <%= styled_form_with url: password_reset_path, class: "space-y-4" do |form| %>
-  <%= auth_messages form %>
-
   <%= form.email_field :email, label: true, autofocus: false, autocomplete: "email", required: "required", placeholder: "you@example.com" %>
 
   <%= form.submit t(".submit") %>

app/views/passwords/edit.html.erb

@@ -1,8 +1,6 @@
 <h1><% t(".title") %></h1>
 
 <%= styled_form_with model: Current.user, url: password_path, class: "space-y-4" do |form| %>
-  <%= auth_messages form %>
-
   <div>
     <%= form.label :password_challenge, t(".password_challenge") %>
     <%= form.password_field :password_challenge %>

app/views/registrations/new.html.erb

@@ -10,7 +10,6 @@
 <% end %>
 
 <%= styled_form_with model: @user, url: registration_path, class: "space-y-4" do |form| %>
-  <%= auth_messages form %>
   <%= form.email_field :email, autofocus: false, autocomplete: "email", required: "required", placeholder: "you@example.com", label: true %>
   <%= form.password_field :password, autocomplete: "new-password", required: "required", label: true %>
   <%= form.password_field :password_confirmation, autocomplete: "new-password", required: "required", label: true %>

app/views/sessions/new.html.erb

@@ -2,9 +2,7 @@
   header_title t(".title")
 %>
 
-<%= styled_form_with url: session_path, class: "space-y-4" do |form| %>
+<%= styled_form_with url: sessions_path, class: "space-y-4" do |form| %>
-  <%= auth_messages form %>
-
   <%= form.email_field :email, label: t(".email"), autofocus: false, autocomplete: "email", required: "required", placeholder: t(".email_placeholder") %>
 
   <%= form.password_field :password, label: t(".password"), required: "required" %>

app/views/settings/_nav.html.erb

@@ -68,7 +68,7 @@
     </section>
 
     <section>
-      <%= button_to session_path, method: :delete, class: "flex items-center gap-2 px-3 py-2 rounded-xl border text-sm font-medium w-full text-error hover:bg-gray-100 border-transparent" do %>
+      <%= button_to session_path(Current.session), method: :delete, class: "flex items-center gap-2 px-3 py-2 rounded-xl border text-sm font-medium w-full text-error hover:bg-gray-100 border-transparent" do %>
         <%= lucide_icon("log-out", class: "w-5 h-5 shrink-0") %>
         <span><%= t(".logout") %></span>
       <% end %>

app/views/settings/profiles/show.html.erb

@@ -26,7 +26,7 @@
           <div class="space-y-3">
             <p><%= t(".profile_image_type") %></p>
             <%= form.label :profile_image, t(".profile_image_choose"), class: "inline-block cursor-pointer px-3 py-2 bg-gray-50 text-gray-900 rounded-md text-sm font-medium" %>
-            <%= form.file_field :profile_image, accept: "image/png, image/jpeg, image/gif", class: "hidden px-3 py-2 bg-gray-50 text-gray-900 rounded-md text-sm font-medium", data: {profile_image_preview_target: "fileField", action: "change->profile-image-preview#preview"} %>
+            <%= form.file_field :profile_image, accept: "image/png, image/jpeg", class: "hidden px-3 py-2 bg-gray-50 text-gray-900 rounded-md text-sm font-medium", data: {profile_image_preview_target: "fileField", action: "change->profile-image-preview#preview"} %>
             <%= form.hidden_field :delete_profile_image, value: false, data: {profile_image_preview_target: "deleteField"} %>
           </div>
         </div>

config/routes.rb

@@ -1,18 +1,14 @@
 Rails.application.routes.draw do
   mount GoodJob::Engine => "jobs"
 
-  get "changelog" => "pages#changelog", as: :changelog
+  get "changelog", to: "pages#changelog"
-  get "feedback" => "pages#feedback", as: :feedback
+  get "feedback", to: "pages#feedback"
 
   resource :registration
-  resource :session
+  resources :sessions, only: %i[new create destroy]
   resource :password_reset
   resource :password
 
-  namespace :help do
-    resources :articles, only: :show
-  end
-
   namespace :settings do
     resource :profile, only: %i[show update destroy]
     resource :preferences, only: %i[show update]

db/migrate/20241003163448_create_sessions.rb

@@ -0,0 +1,13 @@
+class CreateSessions < ActiveRecord::Migration[7.2]
+  def change
+    create_table :sessions, id: :uuid do |t|
+      t.references :user, null: false, foreign_key: true, type: :uuid
+      t.string :user_agent
+      t.string :ip_address
+
+      t.timestamps
+    end
+
+    remove_column :users, :last_login_at, :datetime
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.2].define(version: 2024_10_01_181256) do
+ActiveRecord::Schema[7.2].define(version: 2024_10_03_163448) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
   enable_extension "plpgsql"
@@ -450,6 +450,15 @@ ActiveRecord::Schema[7.2].define(version: 2024_10_01_181256) do
     t.datetime "updated_at", null: false
   end
 
+  create_table "sessions", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
+    t.uuid "user_id", null: false
+    t.string "user_agent"
+    t.string "ip_address"
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["user_id"], name: "index_sessions_on_user_id"
+  end
+
   create_table "settings", force: :cascade do |t|
     t.string "var", null: false
     t.text "value"
@@ -485,7 +494,6 @@ ActiveRecord::Schema[7.2].define(version: 2024_10_01_181256) do
     t.string "password_digest"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.datetime "last_login_at"
     t.string "last_prompted_upgrade_commit_sha"
     t.string "last_alerted_upgrade_commit_sha"
     t.enum "role", default: "member", null: false, enum_type: "user_role"
@@ -524,6 +532,7 @@ ActiveRecord::Schema[7.2].define(version: 2024_10_01_181256) do
   add_foreign_key "imports", "families"
   add_foreign_key "institutions", "families"
   add_foreign_key "merchants", "families"
+  add_foreign_key "sessions", "users"
   add_foreign_key "taggings", "tags"
   add_foreign_key "tags", "families"
   add_foreign_key "users", "families"

test/controllers/registrations_controller_test.rb

@@ -24,14 +24,6 @@ class RegistrationsControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
-  test "sets last_login_at on successful registration" do
-    post registration_url, params: { user: {
-      email: "john@example.com",
-      password: "password",
-      password_confirmation: "password" } }
-    assert_not_nil User.find_by(email: "john@example.com").last_login_at
-  end
-
   test "create when hosted requires an invite code" do
     with_env_overrides REQUIRE_INVITE_CODE: "true" do
       assert_no_difference "User.count" do

test/controllers/sessions_controller_test.rb

@@ -5,14 +5,30 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
     @user = users(:family_admin)
   end
 
-  test "can sign in" do
+  test "login page" do
-    post session_url, params: { email: @user.email, password: "password" }
+    get new_session_url
-    assert_redirected_to root_url
+    assert_response :success
   end
 
-  test "sets last_login_at on successful login" do
+  test "can sign in" do
-    assert_changes -> { @user.reload.last_login_at }, from: nil do
+    sign_in @user
-      post session_url, params: { email: @user.email, password: "password" }
+    assert_redirected_to root_url
-    end
+
+    get root_url
+    assert_response :success
+  end
+
+  test "fails to sign in with bad password" do
+    post sessions_url, params: { email: @user.email, password: "bad" }
+    assert_response :unprocessable_entity
+    assert_equal "Invalid email or password.", flash[:alert]
+  end
+
+  test "can sign out" do
+    sign_in @user
+
+    delete session_url(@user.sessions.order(:created_at).last)
+    assert_redirected_to root_url
+    assert_equal "You have signed out successfully.", flash[:notice]
   end
 end

test/fixtures/sessions.yml

@@ -0,0 +1,4 @@
+one:
+  user: family_admin
+  user_agent: MyString
+  ip_address: MyString

test/models/current_test.rb

@@ -3,7 +3,7 @@ require "test_helper"
 class CurrentTest < ActiveSupport::TestCase
   test "family returns user family" do
     user = users(:family_admin)
-    Current.user = user
+    Current.session = user.sessions.create!
     assert_equal user.family, Current.family
   end
 end

test/models/session_test.rb

@@ -0,0 +1,7 @@
+require "test_helper"
+
+class SessionTest < ActiveSupport::TestCase
+  # test "the truth" do
+  #   assert true
+  # end
+end

test/test_helper.rb

@@ -49,7 +49,7 @@ module ActiveSupport
 
     # Add more helper methods to be used by all tests here...
     def sign_in(user)
-      post session_path, params: { email: user.email, password: "password" }
+      post sessions_path, params: { email: user.email, password: "password" }
     end
 
     def with_env_overrides(overrides = {}, &block)