From 6e85d0d6892f369e9ce7e302911bba9946944e74 Mon Sep 17 00:00:00 2001 From: arandomandy Date: Mon, 19 Aug 2019 21:13:11 -0400 Subject: [PATCH 01/12] Fix typo in the flash message shown when wiped user tries to reset password --- app/controllers/login_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/login_controller.rb b/app/controllers/login_controller.rb index 110f86254..9185089d1 100644 --- a/app/controllers/login_controller.rb +++ b/app/controllers/login_controller.rb @@ -116,7 +116,7 @@ def reset_password end if @found_user.is_wiped? - flash.now[:error] = "It's not possible to reest your password " << + flash.now[:error] = "It's not possible to reset your password " << "because your account was deleted before the site changed admins " << "and your email address was wiped for privacy." return forgot_password From c336fb55dfc2fffdf417fa9aec21098b799ba0a2 Mon Sep 17 00:00:00 2001 From: arandomandy Date: Mon, 19 Aug 2019 21:42:05 -0400 Subject: [PATCH 02/12] Remove inactive-user from BANNED_USERNAMES The following validation *validates_each :username* was causing multiple actions that operate on a user to fail, throwing a 500 error. This was preventing the inactive-user user account from being created by *rake db:seed* as well as preventing the inactive-user user to reset their password. --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 07091cb9d..454e69491 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -110,7 +110,7 @@ class User < ApplicationRecord end BANNED_USERNAMES = ["admin", "administrator", "contact", "fraud", "guest", - "help", "hostmaster", "inactive-user", "lobster", "lobsters", "mailer-daemon", "moderator", + "help", "hostmaster", "lobster", "lobsters", "mailer-daemon", "moderator", "moderators", "nobody", "postmaster", "root", "security", "support", "sysop", "webmaster", "enable", "new", "signup",].freeze From e897d2caa75e11bbb0b305a344a62a17f823d4fa Mon Sep 17 00:00:00 2001 From: arandomandy Date: Wed, 21 Aug 2019 22:08:13 -0400 Subject: [PATCH 03/12] Fix bug letting already logged in users see login and forgot password forms --- app/controllers/login_controller.rb | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/app/controllers/login_controller.rb b/app/controllers/login_controller.rb index 9185089d1..d136926ca 100644 --- a/app/controllers/login_controller.rb +++ b/app/controllers/login_controller.rb @@ -17,9 +17,13 @@ def logout end def index - @title = "Login" - @referer ||= request.referer - render :action => "index" + if @user + redirect_to "/" + else + @title = "Login" + @referer ||= request.referer + render :action => "index" + end end def login @@ -98,8 +102,12 @@ def login end def forgot_password - @title = "Reset Password" - render :action => "forgot_password" + if @user + redirect_to "/" + else + @title = "Reset Password" + render :action => "forgot_password" + end end def reset_password From 1a19bef0371fda4d61a5ec0a2790a8a759d5e1f1 Mon Sep 17 00:00:00 2001 From: arandomandy Date: Wed, 21 Aug 2019 22:10:39 -0400 Subject: [PATCH 04/12] Fix broken test The error was caused by *user1* trying to log in while *user* was already logged in. --- spec/features/comment_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/features/comment_spec.rb b/spec/features/comment_spec.rb index b07151486..03f03d514 100644 --- a/spec/features/comment_spec.rb +++ b/spec/features/comment_spec.rb @@ -83,8 +83,9 @@ created_at: 90.days.ago, comment: "Cool story.", ) + reset_session! - stub_login_as user1 + stub_login_as user1 visit "/s/#{story.short_id}" expect(page.find(:css, ".comment .comment_text")).to have_content('Cool story.') expect(comment.upvotes).to eq(1) From f80b5f03b2e320474ca7f40e64d811ba188fd50e Mon Sep 17 00:00:00 2001 From: arandomandy Date: Wed, 21 Aug 2019 22:11:08 -0400 Subject: [PATCH 05/12] Add tests for when a logged in user attempts to go to /login or /login/forgot_password --- spec/controllers/login_controller_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/controllers/login_controller_spec.rb b/spec/controllers/login_controller_spec.rb index 4f7a0d29f..4c1d52d95 100644 --- a/spec/controllers/login_controller_spec.rb +++ b/spec/controllers/login_controller_spec.rb @@ -68,6 +68,12 @@ expect(flash[:error]).to match(/wiped/) end end + + it "doesn't show login form if user is already logged in" do + post :login, params: { email: user.email, password: 'asdf' } + get :login + expect(response).to redirect_to('/') + end end describe "/login/reset_password" do @@ -98,6 +104,12 @@ expect(flash[:success]).to be_nil }.not_to(change { User.find(deleted_wiped.id).password_reset_token }) end + + it "doesn't show reset form if user is already logged in" do + post :login, params: { email: user.email, password: 'asdf' } + get :login + expect(response).to redirect_to('/') + end end describe "/login/set_new_password" do From a40acaf5254635fe268e7ba57757ad77e9d127b0 Mon Sep 17 00:00:00 2001 From: arandomandy Date: Wed, 21 Aug 2019 22:30:39 -0400 Subject: [PATCH 06/12] Fix inconsistent identation --- spec/features/comment_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/comment_spec.rb b/spec/features/comment_spec.rb index 03f03d514..fe8909f0e 100644 --- a/spec/features/comment_spec.rb +++ b/spec/features/comment_spec.rb @@ -83,9 +83,9 @@ created_at: 90.days.ago, comment: "Cool story.", ) - reset_session! + reset_session! - stub_login_as user1 + stub_login_as user1 visit "/s/#{story.short_id}" expect(page.find(:css, ".comment .comment_text")).to have_content('Cool story.') expect(comment.upvotes).to eq(1) From 4a8b75e81b909550086ede9e8865e5e5c5204bb9 Mon Sep 17 00:00:00 2001 From: arandomandy Date: Wed, 4 Sep 2019 07:20:10 -0400 Subject: [PATCH 07/12] Make mail_new_activity no-op silently --- script/mail_new_activity.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/script/mail_new_activity.rb b/script/mail_new_activity.rb index ccb02bbc4..d101c6b6a 100755 --- a/script/mail_new_activity.rb +++ b/script/mail_new_activity.rb @@ -66,7 +66,7 @@ def story_subject(story, prefix = "") mailing_list_users = User.where("mailing_list_mode > 0").select(&:is_active?) - last_story_id = (Keystore.value_for(LAST_STORY_KEY) || Story.last.id).to_i + last_story_id = (Keystore.value_for(LAST_STORY_KEY) || Story.last && Story.last.id).to_i Story.where("id > ? AND is_expired = ?", last_story_id, false).order(:id).each do |s| s.fetch_story_cache! @@ -142,8 +142,7 @@ def story_subject(story, prefix = "") # repeat for comments - last_comment_id = (Keystore.value_for(LAST_COMMENT_KEY) || - Comment.last.id).to_i + last_comment_id = (Keystore.value_for(LAST_COMMENT_KEY) || Comment.last && Comment.last.id).to_i Comment.where( "id > ? AND (is_deleted = ? AND is_moderated = ?)", From fccaf109884af84684753a4386b4bd56dc488eae Mon Sep 17 00:00:00 2001 From: arandomandy Date: Wed, 4 Sep 2019 07:47:19 -0400 Subject: [PATCH 08/12] Revert "Make mail_new_activity no-op silently" This reverts commit 4a8b75e81b909550086ede9e8865e5e5c5204bb9. --- script/mail_new_activity.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/script/mail_new_activity.rb b/script/mail_new_activity.rb index d101c6b6a..ccb02bbc4 100755 --- a/script/mail_new_activity.rb +++ b/script/mail_new_activity.rb @@ -66,7 +66,7 @@ def story_subject(story, prefix = "") mailing_list_users = User.where("mailing_list_mode > 0").select(&:is_active?) - last_story_id = (Keystore.value_for(LAST_STORY_KEY) || Story.last && Story.last.id).to_i + last_story_id = (Keystore.value_for(LAST_STORY_KEY) || Story.last.id).to_i Story.where("id > ? AND is_expired = ?", last_story_id, false).order(:id).each do |s| s.fetch_story_cache! @@ -142,7 +142,8 @@ def story_subject(story, prefix = "") # repeat for comments - last_comment_id = (Keystore.value_for(LAST_COMMENT_KEY) || Comment.last && Comment.last.id).to_i + last_comment_id = (Keystore.value_for(LAST_COMMENT_KEY) || + Comment.last.id).to_i Comment.where( "id > ? AND (is_deleted = ? AND is_moderated = ?)", From e0f66f1a8703ae75bdda24b8f430526d44cd23ff Mon Sep 17 00:00:00 2001 From: arandomandy Date: Sun, 8 Sep 2019 16:08:48 -0400 Subject: [PATCH 09/12] Fix tests calling wrong methods --- spec/controllers/login_controller_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/controllers/login_controller_spec.rb b/spec/controllers/login_controller_spec.rb index 4c1d52d95..62afae35d 100644 --- a/spec/controllers/login_controller_spec.rb +++ b/spec/controllers/login_controller_spec.rb @@ -71,7 +71,7 @@ it "doesn't show login form if user is already logged in" do post :login, params: { email: user.email, password: 'asdf' } - get :login + get :index expect(response).to redirect_to('/') end end @@ -105,9 +105,9 @@ }.not_to(change { User.find(deleted_wiped.id).password_reset_token }) end - it "doesn't show reset form if user is already logged in" do + it "doesn't show forgot password form if user is already logged in" do post :login, params: { email: user.email, password: 'asdf' } - get :login + get :forgot_password expect(response).to redirect_to('/') end end From c76d08f54abb27490d6cc1088daa45f9d79e3555 Mon Sep 17 00:00:00 2001 From: arandomandy Date: Wed, 11 Sep 2019 21:02:03 -0400 Subject: [PATCH 10/12] Disable caching when running tests --- config/environments/test.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/config/environments/test.rb b/config/environments/test.rb index 0a4a64377..e5ca07a51 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -7,6 +7,9 @@ # and recreated between test runs. Don't rely on the data there! config.cache_classes = true + # Disable caching when running tests + config.cache_store = :null_store + # Do not eager load code on boot. This avoids loading your whole application # just for the purpose of running a single test. If you are using a tool that # preloads Rails for running tests, you may have to set it to true. From 95f8a7f75ac88e3b4538c7831fb539afba0f3fff Mon Sep 17 00:00:00 2001 From: arandomandy Date: Wed, 11 Sep 2019 21:06:06 -0400 Subject: [PATCH 11/12] Visit the logout path rather than resetting the session --- spec/features/comment_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/features/comment_spec.rb b/spec/features/comment_spec.rb index fe8909f0e..3b6c74631 100644 --- a/spec/features/comment_spec.rb +++ b/spec/features/comment_spec.rb @@ -83,7 +83,8 @@ created_at: 90.days.ago, comment: "Cool story.", ) - reset_session! + visit "/settings" + click_on "Logout" stub_login_as user1 visit "/s/#{story.short_id}" From e824bb5ac7b1e297ba3154791378897b92ef3d87 Mon Sep 17 00:00:00 2001 From: arandomandy Date: Wed, 11 Sep 2019 22:03:07 -0400 Subject: [PATCH 12/12] Replace if statements with guard-clauses --- app/controllers/login_controller.rb | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/app/controllers/login_controller.rb b/app/controllers/login_controller.rb index 288483809..96f0ddaf9 100644 --- a/app/controllers/login_controller.rb +++ b/app/controllers/login_controller.rb @@ -17,13 +17,11 @@ def logout end def index - if @user - redirect_to "/" - else - @title = "Login" - @referer ||= request.referer - render :action => "index" - end + return redirect_to "/" if @user + + @title = "Login" + @referer ||= request.referer + render :action => "index" end def login @@ -102,12 +100,10 @@ def login end def forgot_password - if @user - redirect_to "/" - else - @title = "Reset Password" - render :action => "forgot_password" - end + return redirect_to "/" if @user + + @title = "Reset Password" + render :action => "forgot_password" end def reset_password