From a1955531e1c0c8fad70ac2663255a06ff1efcf7a Mon Sep 17 00:00:00 2001 From: taitus Date: Mon, 9 Oct 2023 09:45:22 +0200 Subject: [PATCH 1/3] Enable devise lockable module with default values In order to the display a warn text on the last attempt before the account is locked, we need update config.paranoid to false as the devise documentation explains. Adding "config.paranoid: false" implies further changes to the code, so for now we unncomment the default value "config.last_attempt_warning = true" and update it to false. --- app/models/user.rb | 2 +- config/initializers/devise.rb | 12 ++++----- .../20231009073912_add_lockable_to_devise.rb | 11 ++++++++ db/schema.rb | 4 +++ .../users/sessions_controller_spec.rb | 25 +++++++++++++++++++ 5 files changed, 47 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20231009073912_add_lockable_to_devise.rb create mode 100644 spec/controllers/users/sessions_controller_spec.rb diff --git a/app/models/user.rb b/app/models/user.rb index 38eb7911e..87652c380 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -3,7 +3,7 @@ class User < ApplicationRecord attribute :registering_from_web, default: false devise :database_authenticatable, :registerable, :confirmable, :recoverable, :rememberable, - :trackable, :validatable, :omniauthable, :password_expirable, :secure_validatable, + :trackable, :validatable, :omniauthable, :password_expirable, :secure_validatable, :lockable, authentication_keys: [:login] acts_as_voter diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index e297bfd48..58493f5b2 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -167,27 +167,27 @@ Devise.setup do |config| # Defines which strategy will be used to lock an account. # :failed_attempts = Locks an account after a number of failed attempts to sign in. # :none = No lock strategy. You should handle locking by yourself. - # config.lock_strategy = :failed_attempts + config.lock_strategy = :failed_attempts # Defines which key will be used when locking and unlocking an account - # config.unlock_keys = [:email] + config.unlock_keys = [:email] # Defines which strategy will be used to unlock an account. # :email = Sends an unlock link to the user email # :time = Re-enables login after a certain amount of time (see :unlock_in below) # :both = Enables both strategies # :none = No unlock strategy. You should handle unlocking by yourself. - # config.unlock_strategy = :both + config.unlock_strategy = :both # Number of authentication tries before locking an account if lock_strategy # is failed attempts. - # config.maximum_attempts = 20 + config.maximum_attempts = 20 # Time interval to unlock the account if :time is enabled as unlock_strategy. - # config.unlock_in = 1.hour + config.unlock_in = 1.hour # Warn on the last attempt before the account is locked. - # config.last_attempt_warning = true + config.last_attempt_warning = false # ==> Configuration for :recoverable # diff --git a/db/migrate/20231009073912_add_lockable_to_devise.rb b/db/migrate/20231009073912_add_lockable_to_devise.rb new file mode 100644 index 000000000..35bd75d3e --- /dev/null +++ b/db/migrate/20231009073912_add_lockable_to_devise.rb @@ -0,0 +1,11 @@ +class AddLockableToDevise < ActiveRecord::Migration[6.1] + def change + # Only if lock strategy is :failed_attempts + add_column :users, :failed_attempts, :integer, default: 0, null: false + add_column :users, :locked_at, :datetime + + # Add these only if unlock strategy is :email or :both + add_column :users, :unlock_token, :string + add_index :users, :unlock_token, unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 603deabd5..db35e4c6c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1644,6 +1644,9 @@ ActiveRecord::Schema.define(version: 2023_10_12_141318) do t.boolean "recommended_debates", default: true t.boolean "recommended_proposals", default: true t.string "subscriptions_token" + t.integer "failed_attempts", default: 0, null: false + t.datetime "locked_at" + t.string "unlock_token" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["date_of_birth"], name: "index_users_on_date_of_birth" t.index ["email"], name: "index_users_on_email", unique: true @@ -1652,6 +1655,7 @@ ActiveRecord::Schema.define(version: 2023_10_12_141318) do t.index ["hidden_at"], name: "index_users_on_hidden_at" t.index ["password_changed_at"], name: "index_users_on_password_changed_at" t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true + t.index ["unlock_token"], name: "index_users_on_unlock_token", unique: true t.index ["username"], name: "index_users_on_username" end diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb new file mode 100644 index 000000000..57fecedb6 --- /dev/null +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -0,0 +1,25 @@ +require "rails_helper" + +describe Users::SessionsController do + before { request.env["devise.mapping"] = Devise.mappings[:user] } + + let!(:user) { create(:user, email: "citizen@consul.org", password: "12345678") } + + describe "Devise lock" do + context "when devise sign in maximum_attempts reached", :with_frozen_time do + it "locks the user account and sends an email to the account with an unlock link" do + user.update(failed_attempts: 19) + + expect do + post :create, params: { user: { login: "citizen@consul.org", password: "wrongpassword" }} + end.to change { user.reload.failed_attempts }.by(1) + .and change { user.reload.locked_at }.from(nil).to(Time.current) + + expect(ActionMailer::Base.deliveries.count).to eq(1) + body = ActionMailer::Base.deliveries.last.body + expect(body).to have_content "Your account has been locked" + expect(body).to have_link "Unlock my account" + end + end + end +end From 873ec84b52d36f233bffa200635d34e8f11b408f Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 5 Apr 2023 11:38:17 +0200 Subject: [PATCH 2/3] Allow disable devise lockable through secrets --- app/models/user.rb | 3 ++- config/application.rb | 3 +++ config/environments/test.rb | 2 ++ config/secrets.yml.example | 4 ++++ 4 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 87652c380..13efdf965 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -3,8 +3,9 @@ class User < ApplicationRecord attribute :registering_from_web, default: false devise :database_authenticatable, :registerable, :confirmable, :recoverable, :rememberable, - :trackable, :validatable, :omniauthable, :password_expirable, :secure_validatable, :lockable, + :trackable, :validatable, :omniauthable, :password_expirable, :secure_validatable, authentication_keys: [:login] + devise :lockable if Rails.application.config.devise_lockable acts_as_voter acts_as_paranoid column: :hidden_at diff --git a/config/application.rb b/config/application.rb index 9a37609a9..636d4552b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -138,6 +138,9 @@ module Consul config.paths["app/views"].unshift(Rails.root.join("app", "views", "custom")) + # Set to true to enable devise user lockable feature + config.devise_lockable = Rails.application.secrets.devise_lockable + # Set to true to enable managing different tenants using the same application config.multitenancy = Rails.application.secrets.multitenancy end diff --git a/config/environments/test.rb b/config/environments/test.rb index 5787afca2..530286e6c 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -81,6 +81,8 @@ Rails.application.configure do # Allow managing different tenants using the same application config.multitenancy = true + + config.devise_lockable = true end require Rails.root.join("config", "environments", "custom", "test") diff --git a/config/secrets.yml.example b/config/secrets.yml.example index c2c736321..7edfa57cd 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -18,6 +18,7 @@ http_basic_auth: &http_basic_auth development: http_basic_username: "dev" http_basic_password: "pass" + devise_lockable: false multitenancy: false security: last_sign_in: false @@ -50,6 +51,7 @@ staging: errbit_self_hosted_ssl: false http_basic_username: "" http_basic_password: "" + devise_lockable: false managers_url: "" managers_application_key: "" multitenancy: false @@ -89,6 +91,7 @@ preproduction: errbit_self_hosted_ssl: false http_basic_username: "" http_basic_password: "" + devise_lockable: false managers_url: "" managers_application_key: "" multitenancy: false @@ -133,6 +136,7 @@ production: errbit_self_hosted_ssl: false http_basic_username: "" http_basic_password: "" + devise_lockable: false managers_url: "" managers_application_key: "" multitenancy: false From d54a5c2ae0d761e6d6ea445fd04921cf38369bba Mon Sep 17 00:00:00 2001 From: taitus Date: Mon, 9 Oct 2023 09:54:42 +0200 Subject: [PATCH 3/3] Allow define maximum_attemps and unlock_in --- app/models/user.rb | 8 +++ config/initializers/devise.rb | 4 +- config/secrets.yml.example | 12 ++++ .../users/sessions_controller_spec.rb | 2 +- spec/models/user_spec.rb | 66 +++++++++++++++++++ 5 files changed, 89 insertions(+), 3 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 13efdf965..b7aa66e3f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -425,6 +425,14 @@ class User < ApplicationRecord end end + def self.maximum_attempts + (Tenant.current_secrets.dig(:security, :lockable, :maximum_attempts) || 20).to_i + end + + def self.unlock_in + (Tenant.current_secrets.dig(:security, :lockable, :unlock_in) || 1).to_f.hours + end + private def clean_document_number diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 58493f5b2..d351f06ad 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -181,10 +181,10 @@ Devise.setup do |config| # Number of authentication tries before locking an account if lock_strategy # is failed attempts. - config.maximum_attempts = 20 + config.maximum_attempts = 20 # Overwritten in User model # Time interval to unlock the account if :time is enabled as unlock_strategy. - config.unlock_in = 1.hour + config.unlock_in = 1.hour # Overwritten in User model # Warn on the last attempt before the account is locked. config.last_attempt_warning = false diff --git a/config/secrets.yml.example b/config/secrets.yml.example index 7edfa57cd..7b61e7a15 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -23,6 +23,9 @@ development: security: last_sign_in: false password_complexity: false + # lockable: + # maximum_attempts: 20 + # unlock_in: 1 # In hours secret_key_base: 56792feef405a59b18ea7db57b4777e855103882b926413d4afdfb8c0ea8aa86ea6649da4e729c5f5ae324c0ab9338f789174cf48c544173bc18fdc3b14262e4 <<: *maps @@ -58,6 +61,9 @@ staging: security: last_sign_in: false password_complexity: false + # lockable: + # maximum_attempts: 20 + # unlock_in: 1 # In hours tenants: # If you've enabled multitenancy, you can overwrite secrets for a # specific tenant with: @@ -98,6 +104,9 @@ preproduction: security: last_sign_in: false password_complexity: false + # lockable: + # maximum_attempts: 20 + # unlock_in: 1 # In hours tenants: # If you've enabled multitenancy, you can overwrite secrets for a # specific tenant with: @@ -143,6 +152,9 @@ production: security: last_sign_in: false password_complexity: false + # lockable: + # maximum_attempts: 20 + # unlock_in: 1 # In hours tenants: # If you've enabled multitenancy, you can overwrite secrets for a # specific tenant with: diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 57fecedb6..29f0ace2d 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -8,7 +8,7 @@ describe Users::SessionsController do describe "Devise lock" do context "when devise sign in maximum_attempts reached", :with_frozen_time do it "locks the user account and sends an email to the account with an unlock link" do - user.update(failed_attempts: 19) + allow(User).to receive(:maximum_attempts).and_return(1) expect do post :create, params: { user: { login: "citizen@consul.org", password: "wrongpassword" }} diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2ff3a9cb5..ed2bf468e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -882,4 +882,70 @@ describe User do end end end + + describe ".maximum_attempts" do + it "returns 20 as default when the secrets aren't configured" do + expect(User.maximum_attempts).to eq 20 + end + + context "when secrets are configured" do + before do + allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge( + security: { + lockable: { maximum_attempts: "14" } + }, + tenants: { + superstrict: { + security: { + lockable: { maximum_attempts: "1" } + } + } + } + )) + end + + it "uses the general secrets for the main tenant" do + expect(User.maximum_attempts).to eq 14 + end + + it "uses the tenant secrets for a tenant" do + allow(Tenant).to receive(:current_schema).and_return("superstrict") + + expect(User.maximum_attempts).to eq 1 + end + end + end + + describe ".unlock_in" do + it "returns 1 as default when the secrets aren't configured" do + expect(User.unlock_in).to eq 1.hour + end + + context "when secrets are configured" do + before do + allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge( + security: { + lockable: { unlock_in: "2" } + }, + tenants: { + superstrict: { + security: { + lockable: { unlock_in: "50" } + } + } + } + )) + end + + it "uses the general secrets for the main tenant" do + expect(User.unlock_in).to eq 2.hours + end + + it "uses the tenant secrets for a tenant" do + allow(Tenant).to receive(:current_schema).and_return("superstrict") + + expect(User.unlock_in).to eq 50.hours + end + end + end end