From ac6c1acbed7817f2769810abf8d8f47080799151 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Fri, 2 Jun 2017 23:15:57 +0900
Subject: [PATCH] FIX: Groups that do not have any owners should not allow
 membership requests.

---
 app/models/group.rb                           |  7 +++++
 config/locales/server.en.yml                  |  1 +
 ...735_fix_group_allow_membership_requests.rb | 13 ++++++++
 .../admin/groups_controller_spec.rb           | 30 +++++++++++++------
 spec/models/group_spec.rb                     | 20 +++++++++++++
 5 files changed, 62 insertions(+), 9 deletions(-)
 create mode 100644 db/migrate/20170602132735_fix_group_allow_membership_requests.rb

diff --git a/app/models/group.rb b/app/models/group.rb
index dc4de2d1338..ecfab9ec29b 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -36,6 +36,7 @@ class Group < ActiveRecord::Base
   validates_uniqueness_of :name, case_sensitive: false
   validate :automatic_membership_email_domains_format_validator
   validate :incoming_email_validator
+  validate :can_allow_membership_requests
   validates :flair_url, url: true, if: Proc.new { |g| g.flair_url && g.flair_url[0,3] != 'fa-' }
 
   AUTO_GROUPS = {
@@ -511,6 +512,12 @@ SQL
 
   private
 
+    def can_allow_membership_requests
+      if self.allow_membership_requests && !self.group_users.where(owner: true).exists?
+        self.errors.add(:base, I18n.t('groups.errors.cant_allow_membership_requests'))
+      end
+    end
+
     def enqueue_update_mentions_job
       Jobs.enqueue(:update_group_mentions,
         previous_name: self.name_was,
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index a1998eeb084..b46d1483cef 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -264,6 +264,7 @@ en:
       invalid_incoming_email: "'%{email}' is not a valid email address."
       email_already_used_in_group: "'%{email}' is already used by the group '%{group_name}'."
       email_already_used_in_category: "'%{email}' is already used by the category '%{category_name}'."
+      cant_allow_membership_requests: "You cannot allow membership requests for a group without any owners."
     default_names:
       everyone: "everyone"
       admins: "admins"
diff --git a/db/migrate/20170602132735_fix_group_allow_membership_requests.rb b/db/migrate/20170602132735_fix_group_allow_membership_requests.rb
new file mode 100644
index 00000000000..1f3a5ff803f
--- /dev/null
+++ b/db/migrate/20170602132735_fix_group_allow_membership_requests.rb
@@ -0,0 +1,13 @@
+class FixGroupAllowMembershipRequests < ActiveRecord::Migration
+  def up
+    execute <<~SQL
+    UPDATE groups g
+    SET allow_membership_requests = 'f'
+    WHERE NOT EXISTS (SELECT 1 FROM group_users gu WHERE gu.owner = 't' AND gu.group_id = g.id LIMIT 1)
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
index bb7ebbf71a3..d24aaaa5e2b 100644
--- a/spec/controllers/admin/groups_controller_spec.rb
+++ b/spec/controllers/admin/groups_controller_spec.rb
@@ -1,6 +1,8 @@
 require 'rails_helper'
 
 describe Admin::GroupsController do
+  let(:user) { Fabricate(:user) }
+  let(:group) { Fabricate(:group) }
 
   before do
     @admin = log_in(:admin)
@@ -77,7 +79,7 @@ describe Admin::GroupsController do
     end
   end
 
-  context ".create" do
+  context "#create" do
 
     it "strip spaces on the group name" do
       xhr :post, :create, { group: { name: " bob " } }
@@ -92,23 +94,33 @@ describe Admin::GroupsController do
 
   end
 
-  context ".update" do
+  context "#update" do
+    it 'should update a group' do
+      group.add_owner(user)
+
+      expect do
+        xhr :put, :update, { id: group.id, group: {
+          visible: "false",
+          allow_membership_requests: "true"
+        } }
+      end.to change { GroupHistory.count }.by(2)
+
+      expect(response).to be_success
+
+      group.reload
+      expect(group.visible).to eq(false)
+      expect(group.allow_membership_requests).to eq(true)
+    end
 
     it "ignore name change on automatic group" do
       expect do
-        xhr :put, :update, { id: 1, group: {
-          name: "WAT",
-          visible: "true",
-          allow_membership_requests: "true"
-        } }
+        xhr :put, :update, { id: 1, group: { name: "WAT" } }
       end.to change { GroupHistory.count }.by(1)
 
       expect(response).to be_success
 
       group = Group.find(1)
       expect(group.name).not_to eq("WAT")
-      expect(group.visible).to eq(true)
-      expect(group.allow_membership_requests).to eq(true)
     end
 
     it "doesn't launch the 'automatic group membership' job when it's not retroactive" do
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 1455f098157..b066566c113 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -77,6 +77,26 @@ describe Group do
       group.incoming_email = "foo@bar.org"
       expect(group.valid?).to eq(true)
     end
+
+    context 'when a group has no owners' do
+      it 'should not allow membership requests' do
+        group.allow_membership_requests = true
+
+        expect(group.valid?).to eq(false)
+
+        expect(group.errors.full_messages).to include(I18n.t(
+          "groups.errors.cant_allow_membership_requests"
+        ))
+
+        group.allow_membership_requests = false
+        group.save!
+
+        group.add_owner(user)
+        group.allow_membership_requests = true
+
+        expect(group.valid?).to eq(true)
+      end
+    end
   end
 
   def real_admins