Exclude some pending or inactivated rows in Member scopes

An unapproved access request should not give access rights, and blocked users
should not be considered members of anything.

One visible outcome of this behaviour is that owners and masters of a group or
project may be blocked, yet still receive notification emails for access
requests. This commit prevents this from happening.

2 files changed, 64 insertions, 11 deletions

diff --git a/app/models/member.rb b/app/models/member.rb
index 64e0d33fb20..69406379948 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -28,17 +28,34 @@ class Member < ActiveRecord::Base
       allow_nil: true
     }
 
+  # This scope encapsulates (most of) the conditions a row in the member table
+  # must satisfy if it is a valid permission. Of particular note:
+  #
+  #   * Access requests must be excluded
+  #   * Blocked users must be excluded
+  #   * Invitations take effect immediately
+  #   * expires_at is not implemented. A background worker purges expired rows
+  scope :active, -> do
+    is_external_invite = arel_table[:user_id].eq(nil).and(arel_table[:invite_token].not_eq(nil))
+    user_is_active = User.arel_table[:state].eq(:active)
+
+    includes(:user).references(:users)
+      .where(is_external_invite.or(user_is_active))
+      .where(requested_at: nil)
+  end
+
   scope :invite, -> { where.not(invite_token: nil) }
   scope :non_invite, -> { where(invite_token: nil) }
   scope :request, -> { where.not(requested_at: nil) }
-  scope :has_access, -> { where('access_level > 0') }
-
-  scope :guests, -> { where(access_level: GUEST) }
-  scope :reporters, -> { where(access_level: REPORTER) }
-  scope :developers, -> { where(access_level: DEVELOPER) }
-  scope :masters,  -> { where(access_level: MASTER) }
-  scope :owners,  -> { where(access_level: OWNER) }
-  scope :owners_and_masters,  -> { where(access_level: [OWNER, MASTER]) }
+
+  scope :has_access, -> { active.where('access_level > 0') }
+
+  scope :guests, -> { active.where(access_level: GUEST) }
+  scope :reporters, -> { active.where(access_level: REPORTER) }
+  scope :developers, -> { active.where(access_level: DEVELOPER) }
+  scope :masters,  -> { active.where(access_level: MASTER) }
+  scope :owners,  -> { active.where(access_level: OWNER) }
+  scope :owners_and_masters,  -> { active.where(access_level: [OWNER, MASTER]) }
 
   before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? }
 
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index fef90d9b5cb..0b1634f654a 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -57,7 +57,7 @@ describe Member, models: true do
 
   describe 'Scopes & finders' do
     before do
-      project = create(:project)
+      project = create(:empty_project)
       group = create(:group)
       @owner_user = create(:user).tap { |u| group.add_owner(u) }
       @owner = group.members.find_by(user_id: @owner_user.id)
@@ -65,6 +65,15 @@ describe Member, models: true do
       @master_user = create(:user).tap { |u| project.team << [u, :master] }
       @master = project.members.find_by(user_id: @master_user.id)
 
+      @blocked_user = create(:user).tap do |u|
+        project.team << [u, :master]
+        project.team << [u, :developer]
+
+        u.block!
+      end
+      @blocked_master = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::MASTER)
+      @blocked_developer = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::DEVELOPER)
+
       Member.add_user(
         project.members,
         'toto1@example.com',
@@ -73,7 +82,7 @@ describe Member, models: true do
       )
       @invited_member = project.members.invite.find_by_invite_email('toto1@example.com')
 
-      accepted_invite_user = build(:user)
+      accepted_invite_user = build(:user, state: :active)
       Member.add_user(
         project.members,
         'toto2@example.com',
@@ -91,7 +100,7 @@ describe Member, models: true do
 
     describe '.access_for_user_ids' do
       it 'returns the right access levels' do
-        users = [@owner_user.id, @master_user.id]
+        users = [@owner_user.id, @master_user.id, @blocked_user.id]
         expected = {
           @owner_user.id => Gitlab::Access::OWNER,
           @master_user.id => Gitlab::Access::MASTER
@@ -125,6 +134,19 @@ describe Member, models: true do
       it { expect(described_class.request).not_to include @accepted_request_member }
     end
 
+    describe '.developers' do
+      subject { described_class.developers.to_a }
+
+      it { is_expected.not_to include @owner }
+      it { is_expected.not_to include @master }
+      it { is_expected.to include @invited_member }
+      it { is_expected.to include @accepted_invite_member }
+      it { is_expected.not_to include @requested_member }
+      it { is_expected.to include @accepted_request_member }
+      it { is_expected.not_to include @blocked_master }
+      it { is_expected.not_to include @blocked_developer }
+    end
+
     describe '.owners_and_masters' do
       it { expect(described_class.owners_and_masters).to include @owner }
       it { expect(described_class.owners_and_masters).to include @master }
@@ -132,6 +154,20 @@ describe Member, models: true do
       it { expect(described_class.owners_and_masters).not_to include @accepted_invite_member }
       it { expect(described_class.owners_and_masters).not_to include @requested_member }
       it { expect(described_class.owners_and_masters).not_to include @accepted_request_member }
+      it { expect(described_class.owners_and_masters).not_to include @blocked_master }
+    end
+
+    describe '.has_access' do
+      subject { described_class.has_access.to_a }
+
+      it { is_expected.to include @owner }
+      it { is_expected.to include @master }
+      it { is_expected.to include @invited_member }
+      it { is_expected.to include @accepted_invite_member }
+      it { is_expected.not_to include @requested_member }
+      it { is_expected.to include @accepted_request_member }
+      it { is_expected.not_to include @blocked_master }
+      it { is_expected.not_to include @blocked_developer }
     end
   end