Commit 9521edb4 authored by Nick Thomas's avatar Nick Thomas

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.
parent 8aa025bb
...@@ -28,17 +28,34 @@ class Member < ActiveRecord::Base ...@@ -28,17 +28,34 @@ class Member < ActiveRecord::Base
allow_nil: true 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 :invite, -> { where.not(invite_token: nil) }
scope :non_invite, -> { where(invite_token: nil) } scope :non_invite, -> { where(invite_token: nil) }
scope :request, -> { where.not(requested_at: nil) } scope :request, -> { where.not(requested_at: nil) }
scope :has_access, -> { where('access_level > 0') }
scope :has_access, -> { active.where('access_level > 0') }
scope :guests, -> { where(access_level: GUEST) }
scope :reporters, -> { where(access_level: REPORTER) } scope :guests, -> { active.where(access_level: GUEST) }
scope :developers, -> { where(access_level: DEVELOPER) } scope :reporters, -> { active.where(access_level: REPORTER) }
scope :masters, -> { where(access_level: MASTER) } scope :developers, -> { active.where(access_level: DEVELOPER) }
scope :owners, -> { where(access_level: OWNER) } scope :masters, -> { active.where(access_level: MASTER) }
scope :owners_and_masters, -> { where(access_level: [OWNER, 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? } before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? }
......
...@@ -57,7 +57,7 @@ describe Member, models: true do ...@@ -57,7 +57,7 @@ describe Member, models: true do
describe 'Scopes & finders' do describe 'Scopes & finders' do
before do before do
project = create(:project) project = create(:empty_project)
group = create(:group) group = create(:group)
@owner_user = create(:user).tap { |u| group.add_owner(u) } @owner_user = create(:user).tap { |u| group.add_owner(u) }
@owner = group.members.find_by(user_id: @owner_user.id) @owner = group.members.find_by(user_id: @owner_user.id)
...@@ -65,6 +65,15 @@ describe Member, models: true do ...@@ -65,6 +65,15 @@ describe Member, models: true do
@master_user = create(:user).tap { |u| project.team << [u, :master] } @master_user = create(:user).tap { |u| project.team << [u, :master] }
@master = project.members.find_by(user_id: @master_user.id) @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( Member.add_user(
project.members, project.members,
'toto1@example.com', 'toto1@example.com',
...@@ -73,7 +82,7 @@ describe Member, models: true do ...@@ -73,7 +82,7 @@ describe Member, models: true do
) )
@invited_member = project.members.invite.find_by_invite_email('toto1@example.com') @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( Member.add_user(
project.members, project.members,
'toto2@example.com', 'toto2@example.com',
...@@ -91,7 +100,7 @@ describe Member, models: true do ...@@ -91,7 +100,7 @@ describe Member, models: true do
describe '.access_for_user_ids' do describe '.access_for_user_ids' do
it 'returns the right access levels' 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 = { expected = {
@owner_user.id => Gitlab::Access::OWNER, @owner_user.id => Gitlab::Access::OWNER,
@master_user.id => Gitlab::Access::MASTER @master_user.id => Gitlab::Access::MASTER
...@@ -125,6 +134,19 @@ describe Member, models: true do ...@@ -125,6 +134,19 @@ describe Member, models: true do
it { expect(described_class.request).not_to include @accepted_request_member } it { expect(described_class.request).not_to include @accepted_request_member }
end 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 describe '.owners_and_masters' do
it { expect(described_class.owners_and_masters).to include @owner } it { expect(described_class.owners_and_masters).to include @owner }
it { expect(described_class.owners_and_masters).to include @master } it { expect(described_class.owners_and_masters).to include @master }
...@@ -132,6 +154,20 @@ describe Member, models: true do ...@@ -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 @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 @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 @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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment