FIX: allow existing users to be invited to topic/message when must_approve_users is enabled

app/assets/javascripts/discourse/controllers/invite.js.es6

@@ -27,19 +27,22 @@ export default Ember.Controller.extend(ModalFunctionality, {
     return Discourse.User.currentProp("admin");
   }.property(),
 
-  disabled: function() {
-    if (this.get('model.saving')) return true;
-    if (Ember.isEmpty(this.get('emailOrUsername'))) return true;
-    const emailOrUsername = this.get('emailOrUsername').trim();
+  @computed('isAdmin', 'emailOrUsername', 'invitingToTopic', 'isPrivateTopic', 'model.groupNames', 'model.saving', 'model.details.can_invite_to')
+  disabled(isAdmin, emailOrUsername, invitingToTopic, isPrivateTopic, groupNames, saving, can_invite_to) {
+    if (saving) return true;
+    if (Ember.isEmpty(emailOrUsername)) return true;
+    const emailTrimmed = emailOrUsername.trim();
+
     // when inviting to forum, email must be valid
-    if (!this.get('invitingToTopic') && !emailValid(emailOrUsername)) return true;
+    if (!invitingToTopic && !emailValid(emailTrimmed)) return true;
     // normal users (not admin) can't invite users to private topic via email
-    if (!this.get('isAdmin') && this.get('isPrivateTopic') && emailValid(emailOrUsername)) return true;
+    if (!isAdmin && isPrivateTopic && emailValid(emailTrimmed)) return true;
     // when inviting to private topic via email, group name must be specified
-    if (this.get('isPrivateTopic') && Ember.isEmpty(this.get('model.groupNames')) && emailValid(emailOrUsername)) return true;
-    if (this.get('model.details.can_invite_to')) return false;
+    if (isPrivateTopic && Ember.isEmpty(groupNames) && emailValid(emailTrimmed)) return true;
+
+    if (can_invite_to) return false;
     return false;
-  }.property('isAdmin', 'emailOrUsername', 'invitingToTopic', 'isPrivateTopic', 'model.groupNames', 'model.saving'),
+  },
 
   disabledCopyLink: function() {
     if (this.get('hasCustomMessage')) return true;
@@ -65,9 +68,10 @@ export default Ember.Controller.extend(ModalFunctionality, {
     return this.get('model') !== this.currentUser;
   }.property('model'),
 
-  showCopyInviteButton: function() {
-    return (!Discourse.SiteSettings.enable_sso && !this.get('isMessage'));
-  }.property('isMessage'),
+  @computed('isMessage', 'model.details.can_invite_via_email')
+  showCopyInviteButton(isMessage, can_invite_via_email) {
+    return (can_invite_via_email && !isMessage);
+  },
 
   topicId: Ember.computed.alias('model.id'),
 
@@ -83,32 +87,38 @@ export default Ember.Controller.extend(ModalFunctionality, {
   }.property('invitingToTopic'),
 
   // Show Groups? (add invited user to private group)
-  showGroups: function() {
-    return this.get('isAdmin') && (emailValid(this.get('emailOrUsername')) || this.get('isPrivateTopic') || !this.get('invitingToTopic')) && !Discourse.SiteSettings.enable_sso && Discourse.SiteSettings.enable_local_logins && !this.get('isMessage');
-  }.property('isAdmin', 'emailOrUsername', 'isPrivateTopic', 'isMessage', 'invitingToTopic'),
+  @computed('isAdmin', 'emailOrUsername', 'isPrivateTopic', 'isMessage', 'invitingToTopic', 'model.details.can_invite_via_email')
+  showGroups(isAdmin, emailOrUsername, isPrivateTopic, isMessage, invitingToTopic, can_invite_via_email) {
+    return isAdmin &&
+           can_invite_via_email &&
+           !isMessage &&
+           (emailValid(emailOrUsername) || isPrivateTopic || !invitingToTopic);
+  },
 
-  showCustomMessage: function() {
-    return (this.get('model') === this.currentUser || emailValid(this.get('emailOrUsername')));
-  }.property('emailOrUsername'),
+  @computed('emailOrUsername')
+  showCustomMessage(emailOrUsername) {
+    return (this.get('model') === this.currentUser || emailValid(emailOrUsername));
+  },
 
   // Instructional text for the modal.
-  inviteInstructions: function() {
-    if (Discourse.SiteSettings.enable_sso || !Discourse.SiteSettings.enable_local_logins) {
-      // inviting existing user when SSO enabled
+  @computed('isMessage', 'invitingToTopic', 'emailOrUsername', 'isPrivateTopic', 'isAdmin', 'model.details.can_invite_via_email')
+  inviteInstructions(isMessage, invitingToTopic, emailOrUsername, isPrivateTopic, isAdmin, can_invite_via_email) {
+    if (!can_invite_via_email) {
+      // can't invite via email, only existing users
       return I18n.t('topic.invite_reply.sso_enabled');
-    } else if (this.get('isMessage')) {
+    } else if (isMessage) {
       // inviting to a message
       return I18n.t('topic.invite_private.email_or_username');
-    } else if (this.get('invitingToTopic')) {
+    } else if (invitingToTopic) {
       // inviting to a private/public topic
-      if (this.get('isPrivateTopic') && !this.get('isAdmin')) {
+      if (isPrivateTopic && !isAdmin) {
         // inviting to a private topic and is not admin
         return I18n.t('topic.invite_reply.to_username');
       } else {
         // when inviting to a topic, display instructions based on provided entity
-        if (Ember.isEmpty(this.get('emailOrUsername'))) {
+        if (Ember.isEmpty(emailOrUsername)) {
           return I18n.t('topic.invite_reply.to_topic_blank');
-        } else if (emailValid(this.get('emailOrUsername'))) {
+        } else if (emailValid(emailOrUsername)) {
           this.set("inviteIcon", "envelope");
           return I18n.t('topic.invite_reply.to_topic_email');
         } else {
@@ -120,7 +130,7 @@ export default Ember.Controller.extend(ModalFunctionality, {
       // inviting to forum
       return I18n.t('topic.invite_reply.to_forum');
     }
-  }.property('isMessage', 'invitingToTopic', 'emailOrUsername'),
+  },
 
   showGroupsClass: function() {
     return this.get('isPrivateTopic') ? 'required' : 'optional';
@@ -147,11 +157,12 @@ export default Ember.Controller.extend(ModalFunctionality, {
     return this.get('isMessage') ? I18n.t('topic.invite_private.error') : I18n.t('topic.invite_reply.error');
   }.property('isMessage'),
 
-  placeholderKey: function() {
-    return (Discourse.SiteSettings.enable_sso || !Discourse.SiteSettings.enable_local_logins) ?
-            'topic.invite_reply.username_placeholder' :
-            'topic.invite_private.email_or_username_placeholder';
-  }.property(),
+  @computed('model.details.can_invite_via_email')
+  placeholderKey(can_invite_via_email) {
+    return (can_invite_via_email) ?
+            'topic.invite_private.email_or_username_placeholder' :
+            'topic.invite_reply.username_placeholder';
+  },
 
   customMessagePlaceholder: function() {
     return I18n.t('invite.custom_message_placeholder');

app/models/invite.rb

@@ -59,7 +59,7 @@ class Invite < ActiveRecord::Base
     if topic.private_message?
       topic.grant_permission_to_user(user.email)
     elsif topic.category && topic.category.groups.any?
-      if Guardian.new(invited_by).can_invite_to?(topic) && !SiteSetting.enable_sso
+      if Guardian.new(invited_by).can_invite_via_email?(topic)
         (topic.category.groups - user.groups).each do |group|
           group.add(user)
           GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)

app/models/topic.rb

@@ -744,7 +744,7 @@ SQL
       end
     end
 
-    if username_or_email =~ /^.+@.+$/ && !SiteSetting.enable_sso && SiteSetting.enable_local_logins
+    if username_or_email =~ /^.+@.+$/ && Guardian.new(invited_by).can_invite_via_email?(self)
       # rate limit topic invite
       RateLimiter.new(invited_by, "topic-invitations-per-day", SiteSetting.max_topic_invitations_per_day, 1.day.to_i).performed!
 

app/serializers/topic_view_serializer.rb

@@ -115,6 +115,7 @@ class TopicViewSerializer < ApplicationSerializer
     result[:can_recover] = true if scope.can_recover_topic?(object.topic)
     result[:can_remove_allowed_users] = true if scope.can_remove_allowed_users?(object.topic)
     result[:can_invite_to] = true if scope.can_invite_to?(object.topic)
+    result[:can_invite_via_email] = true if scope.can_invite_via_email?(object.topic)
     result[:can_create_post] = true if scope.can_create?(Post, object.topic)
     result[:can_reply_as_new_topic] = true if scope.can_reply_as_new_topic?(object.topic)
     result[:can_flag_topic] = actions_summary.any? { |a| a[:can_act] }

lib/guardian.rb

@@ -229,7 +229,6 @@ class Guardian
 
   def can_invite_to?(object, group_ids=nil)
     return false if ! authenticated?
-    return false unless (!SiteSetting.must_approve_users? || is_staff?)
     return true if is_admin?
     return false if (SiteSetting.max_invites_per_day.to_i == 0 && !is_staff?)
     return false if ! can_see?(object)
@@ -244,6 +243,11 @@ class Guardian
     user.has_trust_level?(TrustLevel[2])
   end
 
+  def can_invite_via_email?(object)
+    return false unless can_invite_to?(object)
+    !SiteSetting.enable_sso && SiteSetting.enable_local_logins && (!SiteSetting.must_approve_users? || is_staff?)
+  end
+
   def can_bulk_invite_to_forum?(user)
     user.admin?
   end

spec/components/guardian_spec.rb

@@ -341,16 +341,6 @@ describe Guardian do
       expect(Guardian.new(moderator).can_invite_to?(topic)).to be_truthy
     end
 
-    it 'returns true when the site requires approving users and is mod' do
-      SiteSetting.expects(:must_approve_users?).returns(true)
-      expect(Guardian.new(moderator).can_invite_to?(topic)).to be_truthy
-    end
-
-    it 'returns false when the site requires approving users and is regular' do
-      SiteSetting.expects(:must_approve_users?).returns(true)
-      expect(Guardian.new(coding_horror).can_invite_to?(topic)).to be_falsey
-    end
-
     it 'returns false for normal user on private topic' do
       expect(Guardian.new(user).can_invite_to?(private_topic)).to be_falsey
     end
@@ -364,6 +354,38 @@ describe Guardian do
     end
   end
 
+  describe 'can_invite_via_email?' do
+    it 'returns true for all (tl2 and above) users when sso is disabled, local logins are enabled, user approval is not required' do
+      expect(Guardian.new(trust_level_2).can_invite_via_email?(topic)).to be_truthy
+      expect(Guardian.new(moderator).can_invite_via_email?(topic)).to be_truthy
+      expect(Guardian.new(admin).can_invite_via_email?(topic)).to be_truthy
+    end
+
+    it 'returns false for all users when sso is enabled' do
+      SiteSetting.enable_sso = true
+
+      expect(Guardian.new(trust_level_2).can_invite_via_email?(topic)).to be_falsey
+      expect(Guardian.new(moderator).can_invite_via_email?(topic)).to be_falsey
+      expect(Guardian.new(admin).can_invite_via_email?(topic)).to be_falsey
+    end
+
+    it 'returns false for all users when local logins are disabled' do
+      SiteSetting.enable_local_logins = false
+
+      expect(Guardian.new(trust_level_2).can_invite_via_email?(topic)).to be_falsey
+      expect(Guardian.new(moderator).can_invite_via_email?(topic)).to be_falsey
+      expect(Guardian.new(admin).can_invite_via_email?(topic)).to be_falsey
+    end
+
+    it 'returns correct valuse when user approval is required' do
+      SiteSetting.must_approve_users = true
+
+      expect(Guardian.new(trust_level_2).can_invite_via_email?(topic)).to be_falsey
+      expect(Guardian.new(moderator).can_invite_via_email?(topic)).to be_truthy
+      expect(Guardian.new(admin).can_invite_via_email?(topic)).to be_truthy
+    end
+  end
+
   describe 'can_see?' do
 
     it 'returns false with a nil object' do

spec/mailers/invite_mailer_spec.rb

@@ -73,7 +73,8 @@ describe InviteMailer do
 
 
     context "invite to topic" do
-      let(:topic) { Fabricate(:topic, excerpt: "Topic invite support is now available in Discourse!") }
+      let(:trust_level_2) { build(:user, trust_level: 2) }
+      let(:topic) { Fabricate(:topic, excerpt: "Topic invite support is now available in Discourse!", user: trust_level_2) }
       let(:invite) { topic.invite(topic.user, 'name@example.com') }
 
       context "default invite message" do

spec/models/invite_spec.rb

@@ -330,25 +330,24 @@ describe Invite do
     end
 
     context 'invited to topics' do
-      let!(:topic) { Fabricate(:private_message_topic) }
+      let(:tl2_user) { Fabricate(:user, trust_level: 2) }
+      let!(:topic) { Fabricate(:private_message_topic, user: tl2_user) }
       let!(:invite) {
         topic.invite(topic.user, 'jake@adventuretime.ooo')
       }
 
       context 'redeem topic invite' do
-
         it 'adds the user to the topic_users' do
           user = invite.redeem
           topic.reload
           expect(topic.allowed_users.include?(user)).to eq(true)
           expect(Guardian.new(user).can_see?(topic)).to eq(true)
         end
-
       end
 
       context 'invited by another user to the same topic' do
-        let(:coding_horror) { User.find_by(username: "CodingHorror") }
-        let!(:another_invite) { topic.invite(coding_horror, 'jake@adventuretime.ooo') }
+        let(:another_tl2_user) { Fabricate(:user, trust_level: 2) }
+        let!(:another_invite) { topic.invite(another_tl2_user, 'jake@adventuretime.ooo') }
         let!(:user) { invite.redeem }
 
         it 'adds the user to the topic_users' do
@@ -358,25 +357,14 @@ describe Invite do
       end
 
       context 'invited by another user to a different topic' do
-        let!(:another_invite) { another_topic.invite(coding_horror, 'jake@adventuretime.ooo') }
         let!(:user) { invite.redeem }
-
-        let(:coding_horror) { User.find_by(username: "CodingHorror") }
-        let(:another_topic) { Fabricate(:topic, category_id: nil, archetype: "private_message", user: coding_horror) }
+        let(:another_tl2_user) { Fabricate(:user, trust_level: 2) }
+        let(:another_topic) { Fabricate(:topic, user: another_tl2_user) }
 
         it 'adds the user to the topic_users of the first topic' do
+          expect(another_topic.invite(another_tl2_user, user.username)).to be_truthy # invited via username
           expect(topic.allowed_users.include?(user)).to eq(true)
           expect(another_topic.allowed_users.include?(user)).to eq(true)
-          duplicate_invite = Invite.find_by(id: another_invite.id)
-          expect(duplicate_invite).to be_nil
-        end
-
-        context 'if they redeem the other invite afterwards' do
-
-          it 'wont redeem a duplicate invite' do
-            expect(another_invite.redeem).to be_blank
-          end
-
         end
       end
     end

spec/models/topic_spec.rb

@@ -502,28 +502,33 @@ describe Topic do
 
   end
 
-  it "rate limits topic invitations" do
-    SiteSetting.stubs(:max_topic_invitations_per_day).returns(2)
-    RateLimiter.stubs(:disabled?).returns(false)
-    RateLimiter.clear_all!
+  context 'rate limits' do
 
-    start = Time.now.tomorrow.beginning_of_day
-    freeze_time(start)
+    it "rate limits topic invitations" do
+      SiteSetting.stubs(:max_topic_invitations_per_day).returns(2)
+      RateLimiter.stubs(:disabled?).returns(false)
+      RateLimiter.clear_all!
 
-    user = Fabricate(:user)
-    topic = Fabricate(:topic)
+      start = Time.now.tomorrow.beginning_of_day
+      freeze_time(start)
 
-    freeze_time(start + 10.minutes)
-    topic.invite(topic.user, user.username)
+      user = Fabricate(:user)
+      trust_level_2 = Fabricate(:user, trust_level: 2)
+      topic = Fabricate(:topic, user: trust_level_2)
 
-    freeze_time(start + 20.minutes)
-    topic.invite(topic.user, "walter@white.com")
+      freeze_time(start + 10.minutes)
+      topic.invite(topic.user, user.username)
 
-    freeze_time(start + 30.minutes)
+      freeze_time(start + 20.minutes)
+      topic.invite(topic.user, "walter@white.com")
+
+      freeze_time(start + 30.minutes)
+
+      expect {
+        topic.invite(topic.user, "user@example.com")
+      }.to raise_error(RateLimiter::LimitExceeded)
+    end
 
-    expect {
-      topic.invite(topic.user, "user@example.com")
-    }.to raise_error(RateLimiter::LimitExceeded)
   end
 
   context 'bumping topics' do