From f7ee6ce6c57ab9cb8dfdc3b7badd77b948b29730 Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Thu, 18 Jun 2020 14:31:50 +0000 Subject: [PATCH 01/11] [Security] Bump devise from 2.2.8 to 3.5.10 Bumps [devise](https://github.com/plataformatec/devise) from 2.2.8 to 3.5.10. **This update includes a security fix.** - [Release notes](https://github.com/plataformatec/devise/releases) - [Changelog](https://github.com/plataformatec/devise/blob/v3.5.10/CHANGELOG.md) - [Commits](https://github.com/plataformatec/devise/compare/v2.2.8...v3.5.10) Signed-off-by: dependabot-preview[bot] --- Gemfile | 2 +- Gemfile.lock | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Gemfile b/Gemfile index 0c442ca2c6..13beb57c1e 100644 --- a/Gemfile +++ b/Gemfile @@ -49,7 +49,7 @@ gem 'stripe' # which is needed for Pin Payments (and possibly others). gem 'activemerchant', '~> 1.78.0' -gem 'devise', '~> 3.0.1' +gem 'devise', '~> 3.5.10' gem 'devise-encryptable' gem 'jwt', '~> 2.2' gem 'oauth2', '~> 1.4.4' # Used for Stripe Connect diff --git a/Gemfile.lock b/Gemfile.lock index fa904aedfc..68f8b9dfe1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -148,8 +148,6 @@ GEM nokogiri (>= 1.4.4) uuidtools (~> 2.1) bcrypt (3.1.13) - bcrypt-ruby (3.1.5) - bcrypt (>= 3.1.3) bugsnag (6.14.0) concurrent-ruby (~> 1.0) builder (3.1.4) @@ -219,10 +217,12 @@ GEM delayed_job (> 2.0.3) rack-protection (>= 1.5.5) sinatra (>= 1.4.4) - devise (3.0.4) - bcrypt-ruby (~> 3.0) + devise (3.5.10) + bcrypt (~> 3.0) orm_adapter (~> 0.1) railties (>= 3.2.6, < 5) + responders + thread_safe (~> 0.1) warden (~> 1.2.3) devise-encryptable (0.2.0) devise (>= 2.1.0) @@ -555,6 +555,8 @@ GEM redcarpet (3.5.0) request_store (1.4.1) rack (>= 1.4) + responders (1.1.2) + railties (>= 3.2, < 4.2) rexml (3.2.4) roadie (3.4.0) css_parser (~> 1.4) @@ -735,7 +737,7 @@ DEPENDENCIES debugger-linecache delayed_job_active_record delayed_job_web - devise (~> 3.0.1) + devise (~> 3.5.10) devise-encryptable dfc_provider! diffy From 40e065eadaff112442ce2411c0638c42a11f0231 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Thu, 18 Jun 2020 15:58:39 +0100 Subject: [PATCH 02/11] Add gem for Devise::TokenAuthenticatable and configure it --- Gemfile | 3 ++- Gemfile.lock | 3 +++ config/initializers/devise.rb | 9 +++++---- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index 13beb57c1e..88ad191231 100644 --- a/Gemfile +++ b/Gemfile @@ -49,8 +49,9 @@ gem 'stripe' # which is needed for Pin Payments (and possibly others). gem 'activemerchant', '~> 1.78.0' -gem 'devise', '~> 3.5.10' +gem 'devise', '~> 3.5.10' # v4.0.0 needs rails 4.1 gem 'devise-encryptable' +gem 'devise-token_authenticatable', '~> 0.4.10' # v0.5.0 needs devise v4 gem 'jwt', '~> 2.2' gem 'oauth2', '~> 1.4.4' # Used for Stripe Connect diff --git a/Gemfile.lock b/Gemfile.lock index 68f8b9dfe1..f0678f9a11 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -226,6 +226,8 @@ GEM warden (~> 1.2.3) devise-encryptable (0.2.0) devise (>= 2.1.0) + devise-token_authenticatable (0.4.10) + devise (>= 3.5.2, < 4.0.0) diff-lcs (1.3) diffy (3.3.0) docile (1.3.2) @@ -739,6 +741,7 @@ DEPENDENCIES delayed_job_web devise (~> 3.5.10) devise-encryptable + devise-token_authenticatable (~> 0.4.10) dfc_provider! diffy eventmachine (>= 1.2.3) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index e20b748148..5a101fbe4d 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -90,10 +90,6 @@ Devise.setup do |config| # Time interval to unlock the account if :time is enabled as unlock_strategy. # config.unlock_in = 1.hour - # ==> Configuration for :token_authenticatable - # Defines name of the authentication token params key - config.token_authentication_key = :auth_token - # ==> Scopes configuration # Turn scoped views on. Before rendering 'sessions/new', it will first check for # 'users/sessions/new'. It's turned off by default because it's slower if you @@ -141,3 +137,8 @@ Devise.setup do |config| config.case_insensitive_keys = [:email] end + +Devise::TokenAuthenticatable.setup do |config| + # Defines name of the authentication token params key + config.token_authentication_key = :auth_token +end From 26ca374a76a1d8702a89d5b542ad22017aa140ed Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Thu, 18 Jun 2020 16:52:14 +0100 Subject: [PATCH 03/11] Adpat user mailer to devise v3 --- app/mailers/spree/user_mailer.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/mailers/spree/user_mailer.rb b/app/mailers/spree/user_mailer.rb index ba25809b5d..13acb321dc 100644 --- a/app/mailers/spree/user_mailer.rb +++ b/app/mailers/spree/user_mailer.rb @@ -25,8 +25,9 @@ module Spree end # Overrides `Devise::Mailer.confirmation_instructions` - def confirmation_instructions(user, _opts) + def confirmation_instructions(user, token, _opts = {}) @user = user + @token = token @instance = Spree::Config[:site_name] @contact = ContentConfig.footer_email From a2ae78bde9faffc84d1b873f6883f181b9545426 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Thu, 18 Jun 2020 18:11:49 +0100 Subject: [PATCH 04/11] Replay commit from spree_auth_devise that upgrades to devise 3 https://github.com/spree/spree_auth_devise/commit/fe7941f67426ac17aa894b3b73c45577e4bb2513 --- app/mailers/spree/user_mailer.rb | 7 +++---- app/models/spree/user.rb | 4 ---- spec/mailers/user_mailer_spec.rb | 4 ++-- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/app/mailers/spree/user_mailer.rb b/app/mailers/spree/user_mailer.rb index 13acb321dc..a93aff05a5 100644 --- a/app/mailers/spree/user_mailer.rb +++ b/app/mailers/spree/user_mailer.rb @@ -5,12 +5,11 @@ module Spree include I18nHelper # Overrides `Devise::Mailer.reset_password_instructions` - def reset_password_instructions(user) - recipient = user.respond_to?(:id) ? user : Spree.user_class.find(user) + def reset_password_instructions(user, token, _opts = {}) @edit_password_reset_url = spree. - edit_spree_user_password_url(reset_password_token: recipient.reset_password_token) + edit_spree_user_password_url(reset_password_token: token) - mail(to: recipient.email, from: from_address, + mail(to: user.email, from: from_address, subject: Spree::Config[:site_name] + ' ' + I18n.t(:subject, scope: [:devise, :mailer, :reset_password_instructions])) end diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index 3df57a9cf8..e602c172de 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -49,10 +49,6 @@ module Spree has_spree_role?('admin') end - def send_reset_password_instructions - generate_reset_password_token! - UserMailer.reset_password_instructions(id).deliver - end # handle_asynchronously will define send_reset_password_instructions_with_delay. # If handle_asynchronously is called twice, we get an infinite job loop. handle_asynchronously :send_reset_password_instructions unless method_defined? :send_reset_password_instructions_with_delay diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index 9b4790e5ad..642ffd1406 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/spec/mailers/user_mailer_spec.rb @@ -49,7 +49,7 @@ describe Spree::UserMailer do describe '#reset_password_instructions' do describe 'message contents' do before do - @message = described_class.reset_password_instructions(user) + @message = described_class.reset_password_instructions(user, nil) end context 'subject includes' do @@ -72,7 +72,7 @@ describe Spree::UserMailer do describe 'legacy support for User object' do it 'sends an email' do expect do - Spree::UserMailer.reset_password_instructions(user).deliver + Spree::UserMailer.reset_password_instructions(user, nil).deliver end.to change(ActionMailer::Base.deliveries, :size).by(1) end end From c0f9f8c8bfa87e63633c10db231eed717349064e Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 24 Jun 2020 21:09:06 +0100 Subject: [PATCH 05/11] Remove comment refering to old spree upgrade --- spec/support/email_helper.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/support/email_helper.rb b/spec/support/email_helper.rb index f622785425..4291844575 100644 --- a/spec/support/email_helper.rb +++ b/spec/support/email_helper.rb @@ -2,8 +2,6 @@ module OpenFoodNetwork module EmailHelper # Some specs trigger actions that send emails, for example creating an order. # But sending emails doesn't work out-of-the-box. This code sets it up. - # It's here in a single place to allow an easy upgrade to Spree 2 which - # needs a different implementation of this method. def setup_email Spree::Config[:mails_from] = "test@ofn.example.org" end From 0f2980619860ed1a24ff0ad63a508c4954c7c2cb Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 24 Jun 2020 21:12:19 +0100 Subject: [PATCH 06/11] Adapt code to devise 3.2 where the reset_password_token stored in the db is a encrypted version of the token sent in the email In this particular case, the user confirmations controller is redirecting to the reset password page but it doesnt know what is the raw reset_password_token So we regenerate the reset password token so that it can know what's the raw value for the redirect The method User#regenerate_reset_password_token is a proxy to the protected method in Devise::Recoverable --- app/controllers/user_confirmations_controller.rb | 3 ++- app/models/spree/user.rb | 4 ++++ spec/controllers/user_confirmations_controller_spec.rb | 3 ++- spec/features/consumer/confirm_invitation_spec.rb | 6 +++--- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/controllers/user_confirmations_controller.rb b/app/controllers/user_confirmations_controller.rb index 1e15a6a296..f4d1ef3925 100644 --- a/app/controllers/user_confirmations_controller.rb +++ b/app/controllers/user_confirmations_controller.rb @@ -45,8 +45,9 @@ class UserConfirmationsController < DeviseController end if resource.reset_password_token.present? + raw_reset_password_token = resource.regenerate_reset_password_token return spree.edit_spree_user_password_path( - reset_password_token: resource.reset_password_token + reset_password_token: raw_reset_password_token ) end diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index e602c172de..d14694de9b 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -53,6 +53,10 @@ module Spree # If handle_asynchronously is called twice, we get an infinite job loop. handle_asynchronously :send_reset_password_instructions unless method_defined? :send_reset_password_instructions_with_delay + def regenerate_reset_password_token + set_reset_password_token + end + def known_users if admin? Spree::User.where(nil) diff --git a/spec/controllers/user_confirmations_controller_spec.rb b/spec/controllers/user_confirmations_controller_spec.rb index 9920330161..9fb6340bd1 100644 --- a/spec/controllers/user_confirmations_controller_spec.rb +++ b/spec/controllers/user_confirmations_controller_spec.rb @@ -52,7 +52,8 @@ describe UserConfirmationsController, type: :controller do unconfirmed_user.reset_password_token = Devise.friendly_token unconfirmed_user.save! spree_get :show, confirmation_token: unconfirmed_user.confirmation_token - expect(response).to redirect_to spree.edit_spree_user_password_path(reset_password_token: unconfirmed_user.reset_password_token) + expect(response).to be_redirect + expect(response.body).to include spree.edit_spree_user_password_path end end end diff --git a/spec/features/consumer/confirm_invitation_spec.rb b/spec/features/consumer/confirm_invitation_spec.rb index bfdd4baddc..43645a9e31 100644 --- a/spec/features/consumer/confirm_invitation_spec.rb +++ b/spec/features/consumer/confirm_invitation_spec.rb @@ -1,7 +1,7 @@ require "spec_helper" feature "Confirm invitation as manager" do - include UIComponentHelper # for be_logged_in_as + include UIComponentHelper include OpenFoodNetwork::EmailHelper describe "confirm email and set password" do @@ -15,8 +15,8 @@ feature "Confirm invitation as manager" do user.save! end - it "allows you to set a password" do - visit spree.spree_user_confirmation_url(confirmation_token: user.confirmation_token) + it "lets the user set a password" do + visit spree.spree_user_confirmation_path(confirmation_token: user.confirmation_token) expect(user.reload.confirmed?).to be true expect(page).to have_text I18n.t(:change_my_password) From f31d790714c82fdae8ec51a18f17b0bea9843b57 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 24 Jun 2020 21:13:13 +0100 Subject: [PATCH 07/11] Add auth spec to cover case where user tries to reset password before confirming their email --- spec/features/consumer/authentication_spec.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/features/consumer/authentication_spec.rb b/spec/features/consumer/authentication_spec.rb index c172f40f38..ad1da5cd77 100644 --- a/spec/features/consumer/authentication_spec.rb +++ b/spec/features/consumer/authentication_spec.rb @@ -28,6 +28,7 @@ feature "Authentication", js: true do browse_as_large open_login_modal end + scenario "showing login" do expect(page).to have_login_modal end @@ -106,8 +107,31 @@ feature "Authentication", js: true do end.to enqueue_job Delayed::PerformableMethod expect(Delayed::Job.last.payload_object.method_name).to eq(:send_reset_password_instructions_without_delay) end + + context "user with unconfirmed email" do + let(:email) { "test@example.org" } + let!(:user) { Spree::User.create(email: email, unconfirmed_email: email, password: "secret") } + + scenario "cannot reset password before confirming email" do + fill_in "Your email", with: email + click_reset_password_button + expect(page).to have_content I18n.t('email_unconfirmed') + page.find("a", text: I18n.t('devise.confirmations.resend_confirmation_email')).click + expect(page).to have_content I18n.t('devise.confirmations.send_instructions') + + visit spree.spree_user_confirmation_path(confirmation_token: user.confirmation_token) + expect(user.reload.confirmed?).to be true + expect(page).to have_text I18n.t('devise.confirmations.confirmed') + + select_login_tab "Forgot Password?" + fill_in "Your email", with: email + click_reset_password_button + expect(page).to have_reset_password + end + end end end + describe "as medium" do before do browse_as_medium From ca9898839a243533190eeeb08b81cabda2a8da9d Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 24 Jun 2020 21:17:09 +0100 Subject: [PATCH 08/11] Confirm! is deprecated and only redirects to confirm now in devise 3.5 --- spec/controllers/user_confirmations_controller_spec.rb | 2 +- spec/models/spree/user_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/user_confirmations_controller_spec.rb b/spec/controllers/user_confirmations_controller_spec.rb index 9fb6340bd1..335b46af79 100644 --- a/spec/controllers/user_confirmations_controller_spec.rb +++ b/spec/controllers/user_confirmations_controller_spec.rb @@ -11,7 +11,7 @@ describe UserConfirmationsController, type: :controller do before do @request.env["devise.mapping"] = Devise.mappings[:spree_user] - confirmed_user.confirm! + confirmed_user.confirm end context "confirming a user" do diff --git a/spec/models/spree/user_spec.rb b/spec/models/spree/user_spec.rb index ba5b4cd488..fc89796e3c 100644 --- a/spec/models/spree/user_spec.rb +++ b/spec/models/spree/user_spec.rb @@ -109,7 +109,7 @@ describe Spree.user_class do setup_email expect do - create(:user, confirmed_at: nil).confirm! + create(:user, confirmed_at: nil).confirm end.to enqueue_job ConfirmSignupJob end end From 86afa6f4132e9f803d27edbd2a9e8f07eba3f7ab Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 24 Jun 2020 21:28:40 +0100 Subject: [PATCH 09/11] Adapt to devise 3.2 and use after_confirmation callback to send welcome email --- app/models/spree/user.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index d14694de9b..da276ec936 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -34,10 +34,6 @@ module Spree # We use the same options as Spree and add :confirmable devise :confirmable, reconfirmable: true - # TODO: Later versions of devise have a dedicated after_confirmation callback, so use that - after_update :welcome_after_confirm, if: lambda { - confirmation_token_changed? && confirmation_token.nil? - } class DestroyWithOrdersError < StandardError; end @@ -82,9 +78,9 @@ module Spree customers.find_by(enterprise_id: enterprise) end - def welcome_after_confirm - # Send welcome email if we are confirming an user's email - # Note: this callback only runs on email confirmation + # This is a Devise Confirmable callback that runs on email confirmation + # It sends a welcome email after the user email is confirmed + def after_confirmation return unless confirmed? && unconfirmed_email.nil? && !unconfirmed_email_changed? send_signup_confirmation From 7c498a573cd84850d8cf98f533d2d959c9a02308 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Mon, 6 Jul 2020 17:51:19 +0100 Subject: [PATCH 10/11] Make shopfront redirect work when logging out by storing it outside session data --- app/controllers/application_controller.rb | 6 +++++- app/controllers/spree/user_sessions_controller.rb | 10 ++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 17585a7237..e9cb17480b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -62,11 +62,15 @@ class ApplicationController < ActionController::Base end def after_sign_out_path_for(_resource_or_scope) - session[:shopfront_redirect] || main_app.root_path + shopfront_redirect || main_app.root_path end private + def shopfront_redirect + session[:shopfront_redirect] + end + def restrict_iframes response.headers['X-Frame-Options'] = 'DENY' response.headers['Content-Security-Policy'] = "frame-ancestors 'none'" diff --git a/app/controllers/spree/user_sessions_controller.rb b/app/controllers/spree/user_sessions_controller.rb index df42f622e1..7ff26a3437 100644 --- a/app/controllers/spree/user_sessions_controller.rb +++ b/app/controllers/spree/user_sessions_controller.rb @@ -39,8 +39,18 @@ module Spree end end + def destroy + # Logout will clear session data including shopfront_redirect + # Here we store it before actually logging out so that the redirect works correctly + @shopfront_redirect = session[:shopfront_redirect] + + super + end + private + attr_reader :shopfront_redirect + def accurate_title Spree.t(:login) end From d052a7b796317b3efcb3f678441afb04377a7c0e Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 31 Jul 2020 09:08:48 +0100 Subject: [PATCH 11/11] Verify the user is confirmed before returning a reset password token Co-authored-by: Maikel --- app/controllers/user_confirmations_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/user_confirmations_controller.rb b/app/controllers/user_confirmations_controller.rb index f4d1ef3925..0915447bc5 100644 --- a/app/controllers/user_confirmations_controller.rb +++ b/app/controllers/user_confirmations_controller.rb @@ -44,7 +44,7 @@ class UserConfirmationsController < DeviseController 'not_confirmed' end - if resource.reset_password_token.present? + if result == 'confirmed' && resource.reset_password_token.present? raw_reset_password_token = resource.regenerate_reset_password_token return spree.edit_spree_user_password_path( reset_password_token: raw_reset_password_token