FIX: Don't try to delete staged, unused admins and mods

FIX: Don’t try to delete staged, unused admins and mods

diff --git a/app/jobs/scheduled/clean_up_unused_staged_users.rb b/app/jobs/scheduled/clean_up_unused_staged_users.rb
index 292327a..ffcac8a 100644
--- a/app/jobs/scheduled/clean_up_unused_staged_users.rb
+++ b/app/jobs/scheduled/clean_up_unused_staged_users.rb
@@ -10,7 +10,7 @@ module Jobs
 
       User.joins("LEFT JOIN posts ON posts.user_id = users.id")
         .where("posts.user_id IS NULL")
-        .where(staged: true)
+        .where(staged: true, admin: false, moderator: false)
         .where("users.created_at < ?", 1.year.ago)
         .find_each do |user|
 
diff --git a/spec/jobs/clean_up_unused_staged_users_spec.rb b/spec/jobs/clean_up_unused_staged_users_spec.rb
index 2a8679d..ec5cad0 100644
--- a/spec/jobs/clean_up_unused_staged_users_spec.rb
+++ b/spec/jobs/clean_up_unused_staged_users_spec.rb
@@ -3,37 +3,52 @@
 require 'rails_helper'
 
 RSpec.describe Jobs::CleanUpUnusedStagedUsers do
-  fab!(:user) { Fabricate(:user) }
   fab!(:staged_user) { Fabricate(:user, staged: true) }
 
-  context 'when staged user is unused' do
-    it 'should clean up the staged user' do
-      user
-      staged_user.update!(created_at: 2.years.ago)
-
-      expect { described_class.new.execute({}) }.to change { User.count }.by(-1)
-      expect(User.find_by(id: staged_user.id)).to eq(nil)
+  shared_examples "does not delete" do
+    it "doesn't delete the staged user" do
+      expect { described_class.new.execute({}) }.to_not change { User.count }
+      expect(User.exists?(staged_user.id)).to eq(true)
     end
+  end
+
+  context "when staged user is unused" do
+    context "when staged user is old enough" do
+      before { staged_user.update!(created_at: 2.years.ago) }
 
-    describe 'when staged user is not old enough' do
-      it 'should not clean up the staged user' do
-        user
-        staged_user.update!(created_at: 5.months.ago)
+      context "regular staged user" do
+        it "deletes the staged user" do
+          expect { described_class.new.execute({}) }.to change { User.count }.by(-1)
+          expect(User.exists?(staged_user.id)).to eq(false)
+        end
+      end
+
+      context "staged admin" do
+        before { staged_user.update!(admin: true) }
+        include_examples "does not delete"
+      end
 
-        expect { described_class.new.execute({}) }.to_not change { User.count }
-        expect(User.find_by(id: staged_user.id)).to eq(staged_user)
+      context "staged moderator" do
+        before { staged_user.update!(moderator: true) }
+        include_examples "does not delete"
       end
     end
+
+    context 'when staged user is not old enough' do
+      before { staged_user.update!(created_at: 5.months.ago) }
+      include_examples "does not delete"
+    end
+  end
+
+  context "when staged user has posts" do
+    before { Fabricate(:post, user: staged_user) }
+    include_examples "does not delete"
   end
 
-  context 'when staged user is not unused' do
-    it 'should not clean up the staged user' do
-      user
-      Fabricate(:post, user: staged_user)
-      user.update!(created_at: 2.years.ago)
+  it "doesn't delete regular, unused user" do
+    user = Fabricate(:user, created_at: 2.years.ago)
 
-      expect { described_class.new.execute({}) }.to_not change { User.count }
-      expect(User.find_by(id: staged_user.id)).to eq(staged_user)
-    end
+    expect { described_class.new.execute({}) }.to_not change { User.count }
+    expect(User.exists?(user.id)).to eq(true)
   end
 end

GitHub sha: 00b75b4f

1 Like

Since we’re only cleaning up after inactive admins in the invalidate_inactive_admins job, should we be worried about inactive staged moderators?

Unlikely sine the big potential security problem is admins who can download the site database, change all global site settings, etc

On Wed, Aug 21, 2019 at 7:19 AM Discourse Review Bot < notifications@github.com> wrote:

Régis Hanol posted https://review.discourse.org/t/fix-dont-try-to-delete-staged-unused-admins-and-mods/5315/2 :

Since we’re only cleaning up after inactive admins in the invalidate_inactive_admins job, should we be worried about inactive moderators?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/discourse/discourse/commit/00b75b4f4eedfe3b2021a6adb2ebe019cd264566?email_source=notifications&email_token=AALTWVJ7OBXW44PQ363LHW3QFVFHZA5CNFSM4IOHTS72YY3PNVWWK3TUL52HS4DFVVBW63LNNF2EG33NNVSW45FKMNXW23LFNZ2F62LEZYBBFKRE#commitcomment-34777636, or mute the thread https://github.com/notifications/unsubscribe-auth/AALTWVKPZW2IEZDLUFSIXKLQFVFHZANCNFSM4IOHTS7Q .

1 Like