From 0da10e23e715d237a586e02bbc7ff8979e3e7b04 Mon Sep 17 00:00:00 2001 From: Steve Polito Date: Fri, 28 Jan 2022 15:39:21 -0500 Subject: [PATCH] Store session in the database Issues ------- - Closes #68 --- README.md | 63 ++++++++++--------- app/controllers/concerns/authentication.rb | 12 ++-- app/models/active_session.rb | 3 + app/models/user.rb | 3 +- ...211217184706_add_session_token_to_users.rb | 6 -- .../20220129144819_create_active_sessions.rb | 9 +++ db/schema.rb | 12 +++- test/controllers/sessions_controller_test.rb | 36 ++++------- test/fixtures/active_sessions.yml | 1 + test/models/active_session_test.rb | 18 ++++++ test/models/user_test.rb | 21 +++---- test/test_helper.rb | 4 +- 12 files changed, 107 insertions(+), 81 deletions(-) create mode 100644 app/models/active_session.rb delete mode 100644 db/migrate/20211217184706_add_session_token_to_users.rb create mode 100644 db/migrate/20220129144819_create_active_sessions.rb create mode 100644 test/fixtures/active_sessions.yml create mode 100644 test/models/active_session_test.rb diff --git a/README.md b/README.md index c854210..9ed83f2 100644 --- a/README.md +++ b/README.md @@ -1182,7 +1182,7 @@ end <% end %> ``` -## Step 15: Add Friendly Redirects +## Step 17: Add Friendly Redirects 1. Update Authentication Concern. @@ -1301,47 +1301,54 @@ end > > - We refactor the `create` method to always start by finding and authenticating the user. Not only does this prevent timing attacks, but it also prevents accidentally leaking email addresses. This is because we were originally checking if a user was confirmed before authenticating them. That means a bad actor could try and sign in with an email address to see if it exists on the system without needing to know the password. -## Step 18: Account for Session Replay Attacks +## Step 18: Store Session in the Database -**Note that this refactor prevents a user from being logged into multiple devices and browsers at one time.** +We're currently setting the user's ID in the session. Even though that value is encrypted, the encrypted value doesn't change since it's based on the user id which doesn't change. This means that if a bad actor were to get a copy of the session they would have access to a victim's account in perpetuity. One solution is to [rotate encrypted and signed cookie configurations](https://guides.rubyonrails.org/security.html#rotating-encrypted-and-signed-cookies-configurations). Another option is to configure the [Rails session store](https://guides.rubyonrails.org/configuring.html#config-session-store) to use `mem_cache_store` to store session data. -We're currently setting the user's ID in the session. Even though that value is encrypted, the encrypted value doesn't change since it's based on the user id which doesn't change. This means that if a bad actor were to get a copy of the session they would have access to a victim's account in perpetuity. One solution is to [rotate encrypted and signed cookie configurations](https://guides.rubyonrails.org/security.html#rotating-encrypted-and-signed-cookies-configurations). Another solution is to use a rotating value to identify the user (which is what we'll be doing). A third option is to configure the [Rails session store](https://guides.rubyonrails.org/configuring.html#config-session-store) to use `mem_cache_store` to store session data. +The solution we will implement is to set a rotating value to identify the user and store that value in the database. -You can read more about session replay attacks [here](https://binarysolo.chapter24.blog/avoiding-session-replay-attacks-in-rails/). - -1. Add a session_token column to the users table. +1. Generate ActiveSession model. ```bash -rails g migration add_session_token_to_users session_token:string +rails g model active_session user:references ``` -2. Update migration. +2. Update the migration. + ```ruby -# db/migrate/[timestamp]_add_session_token_to_users.rb -class AddSessionTokenToUsers < ActiveRecord::Migration[6.1] +class CreateActiveSessions < ActiveRecord::Migration[6.1] def change - add_column :users, :session_token, :string, null: false - add_index :users, :session_token, unique: true + create_table :active_sessions do |t| + t.references :user, null: false, foreign_key: {on_delete: :cascade} + + t.timestamps + end end end ``` > **What's Going On Here?** > -> - Similar to the `remember_token` column, we prevent the `session_token` from being null and enforce that it has a unique value. +> - We update the `foreign_key` option from `true` to `{on_delete: :cascade}`. The [on_delete](https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_foreign_key-label-Creating+a+cascading+foreign+key) option will delete any `active_session` record if its associated `user` is deleted from the database. + +3. Run migration. + +```bash +rails db:migrate +``` -3. Update User Model. +4. Update User model. ```ruby # app/models/user.rb class User < ApplicationRecord ... - has_secure_token :session_token + has_many :active_sessions, dependent: :destroy ... end ``` -4. Update Authentication Concern. +5. Update Authentication Concern ```ruby # app/controllers/concerns/authentication.rb @@ -1349,21 +1356,21 @@ module Authentication ... def login(user) reset_session - user.regenerate_session_token - session[:current_user_session_token] = user.reload.session_token + active_session = user.active_sessions.create! + session[:current_active_session_id] = active_session.id end ... def logout - user = current_user + active_session = ActiveSession.find_by(id: session[:current_active_session_id]) reset_session - user.regenerate_session_token + active_session.destroy! if active_session.present? end ... private def current_user - Current.user ||= if session[:current_user_session_token].present? - User.find_by(session_token: session[:current_user_session_token]) + Current.user = if session[:current_active_session_id].present? + ActiveSession.find_by(id: session[:current_active_session_id]).user elsif cookies.permanent.encrypted[:remember_token].present? User.find_by(remember_token: cookies.permanent.encrypted[:remember_token]) end @@ -1374,11 +1381,11 @@ end > **What's Going On Here?** > -> - We update the `login` method by adding a call to `user.regenerate_session_token`. This will reset the value of the `session_token` through the [has_secure_token](https://api.rubyonrails.org/classes/ActiveRecord/SecureToken/ClassMethods.html#method-i-has_secure_token) API. We then store that value in the session. -> - We updated the `logout` method by first setting the `current_user` as a variable. This is because once we call `reset_session`, we lose access to the `current_user`. We then call `user.regenerate_session_token` which will update the value of the `session_token` on the user that just signed out. -> - Finally we update the `current_user` method to look for the `session[:current_user_session_token]` instead of the `session[:current_user_id]` and to query for the User by the `session_token` value. +> - We update the `login` method by creating a new `active_session` record and then storing it's ID in the `session`. Note that we replaced `session[:current_user_id]` with `session[:current_active_session_id]`. +> - We update the `logout` method by first finding the `active_session` record from the `session`. After we call `reset_session` we then delete the `active_session` record if it exists. We need to check if it exists because in a future section we will allow a user to log out all current active sessions. +> - We update the `current_user` method by finding the `active_session` record from the `session`, and then returning its associated `user`. Note that we've replaced all instances of `session[:current_user_id]` with `session[:current_active_session_id]`. -5. Force SSL. +6. Force SSL. ```ruby # config/environments/production.rb @@ -1390,4 +1397,4 @@ end > **What's Going On Here?** > -> - We force SSL in production to prevent [session hijacking](https://guides.rubyonrails.org/security.html#session-hijacking). Even though the session is encrypted we want to prevent the cookie from being exposed through an insecure network. If it were exposed, a bad actor could sign in as the victim. \ No newline at end of file +> - We force SSL in production to prevent [session hijacking](https://guides.rubyonrails.org/security.html#session-hijacking). Even though the session is encrypted we want to prevent the cookie from being exposed through an insecure network. If it were exposed, a bad actor could sign in as the victim. diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 451536c..40f3d8d 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -14,8 +14,8 @@ def authenticate_user! def login(user) reset_session - user.regenerate_session_token - session[:current_user_session_token] = user.reload.session_token + active_session = user.active_sessions.create! + session[:current_active_session_id] = active_session.id end def forget(user) @@ -24,9 +24,9 @@ def forget(user) end def logout - user = current_user + active_session = ActiveSession.find_by(id: session[:current_active_session_id]) reset_session - user.regenerate_session_token + active_session.destroy! if active_session.present? end def redirect_if_authenticated @@ -45,8 +45,8 @@ def store_location private def current_user - Current.user ||= if session[:current_user_session_token].present? - User.find_by(session_token: session[:current_user_session_token]) + Current.user = if session[:current_active_session_id].present? + ActiveSession.find_by(id: session[:current_active_session_id]).user elsif cookies.permanent.encrypted[:remember_token].present? User.find_by(remember_token: cookies.permanent.encrypted[:remember_token]) end diff --git a/app/models/active_session.rb b/app/models/active_session.rb new file mode 100644 index 0000000..4e570f2 --- /dev/null +++ b/app/models/active_session.rb @@ -0,0 +1,3 @@ +class ActiveSession < ApplicationRecord + belongs_to :user +end diff --git a/app/models/user.rb b/app/models/user.rb index 3e42202..3fdb708 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -7,7 +7,8 @@ class User < ApplicationRecord has_secure_password has_secure_token :remember_token - has_secure_token :session_token + + has_many :active_sessions, dependent: :destroy before_save :downcase_email before_save :downcase_unconfirmed_email diff --git a/db/migrate/20211217184706_add_session_token_to_users.rb b/db/migrate/20211217184706_add_session_token_to_users.rb deleted file mode 100644 index 4f55e39..0000000 --- a/db/migrate/20211217184706_add_session_token_to_users.rb +++ /dev/null @@ -1,6 +0,0 @@ -class AddSessionTokenToUsers < ActiveRecord::Migration[6.1] - def change - add_column :users, :session_token, :string, null: false - add_index :users, :session_token, unique: true - end -end diff --git a/db/migrate/20220129144819_create_active_sessions.rb b/db/migrate/20220129144819_create_active_sessions.rb new file mode 100644 index 0000000..1a36b77 --- /dev/null +++ b/db/migrate/20220129144819_create_active_sessions.rb @@ -0,0 +1,9 @@ +class CreateActiveSessions < ActiveRecord::Migration[6.1] + def change + create_table :active_sessions do |t| + t.references :user, null: false, foreign_key: {on_delete: :cascade} + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index c1b52bc..b8cbf10 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,14 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_12_17_184706) do +ActiveRecord::Schema.define(version: 2022_01_29_144819) do + + create_table "active_sessions", force: :cascade do |t| + t.integer "user_id", null: false + t.datetime "created_at", precision: 6, null: false + t.datetime "updated_at", precision: 6, null: false + t.index ["user_id"], name: "index_active_sessions_on_user_id" + end create_table "users", force: :cascade do |t| t.string "email", null: false @@ -20,10 +27,9 @@ t.string "password_digest", null: false t.string "unconfirmed_email" t.string "remember_token", null: false - t.string "session_token", null: false t.index ["email"], name: "index_users_on_email", unique: true t.index ["remember_token"], name: "index_users_on_remember_token", unique: true - t.index ["session_token"], name: "index_users_on_session_token", unique: true end + add_foreign_key "active_sessions", "users", on_delete: :cascade end diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 27e7671..e19390c 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -18,15 +18,17 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to root_path end - test "should login if confirmed" do - post login_path, params: { - user: { - email: @confirmed_user.email, - password: @confirmed_user.password + test "should login and create active session if confirmed" do + assert_difference("@confirmed_user.active_sessions.count") do + post login_path, params: { + user: { + email: @confirmed_user.email, + password: @confirmed_user.password + } } - } + end assert_redirected_to root_path - assert_equal @confirmed_user.email, current_user.email + assert_equal @confirmed_user, current_user end test "should remember user when logging in" do @@ -82,10 +84,12 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert_nil current_user end - test "should logout if authenticated" do + test "should logout and delete current active session if authenticated" do login @confirmed_user - delete logout_path + assert_difference("@confirmed_user.active_sessions.count", -1) do + delete logout_path + end assert_nil current_user assert_redirected_to root_path @@ -98,18 +102,4 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest delete logout_path assert_redirected_to root_path end - - test "should reset session_token when logging out" do - login @confirmed_user - - assert_changes "@confirmed_user.reload.session_token" do - delete logout_path - end - end - - test "should reset session_token when logging in" do - assert_changes "@confirmed_user.reload.session_token" do - login @confirmed_user - end - end end diff --git a/test/fixtures/active_sessions.yml b/test/fixtures/active_sessions.yml new file mode 100644 index 0000000..f5a7b58 --- /dev/null +++ b/test/fixtures/active_sessions.yml @@ -0,0 +1 @@ +# Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html \ No newline at end of file diff --git a/test/models/active_session_test.rb b/test/models/active_session_test.rb new file mode 100644 index 0000000..3867f6a --- /dev/null +++ b/test/models/active_session_test.rb @@ -0,0 +1,18 @@ +require "test_helper" + +class ActiveSessionTest < ActiveSupport::TestCase + setup do + @user = User.new(email: "unique_email@example.com", password: "password", password_confirmation: "password") + @active_session = @user.active_sessions.build + end + + test "should be valid" do + assert @active_session.valid? + end + + test "should have a user" do + @active_session.user = nil + + assert_not @active_session.valid? + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 616e6c3..93fda46 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -164,23 +164,20 @@ class UserTest < ActiveSupport::TestCase end end - test "should set session_token on create" do + test "should create active session" do @user.save! - assert_not_nil @user.reload.session_token - end - - test "should generate confirmation token" do - @user.save! - confirmation_token = @user.generate_confirmation_token - - assert_equal @user, User.find_signed(confirmation_token, purpose: :confirm_email) + assert_difference("@user.active_sessions.count", 1) do + @user.active_sessions.create! + end end - test "should generate password reset token" do + test "should destroy associated active session when destryoed" do @user.save! - password_reset_token = @user.generate_password_reset_token + @user.active_sessions.create! - assert_equal @user, User.find_signed(password_reset_token, purpose: :reset_password) + assert_difference("@user.active_sessions.count", -1) do + @user.destroy! + end end end diff --git a/test/test_helper.rb b/test/test_helper.rb index b8c08c0..7133707 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -11,7 +11,7 @@ class ActiveSupport::TestCase # Add more helper methods to be used by all tests here... def current_user - session[:current_user_session_token] && User.find_by(session_token: session[:current_user_session_token]) + session[:current_active_session_id] && ActiveSession.find_by(id: session[:current_active_session_id]).user end def login(user, remember_user: nil) @@ -25,6 +25,6 @@ def login(user, remember_user: nil) end def logout - session.delete(:current_user_id) + session.delete(:current_active_session_id) end end