diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-09-14 11:23:07 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-09-14 15:23:32 +0200 |
commit | 2edbafab1f7a669d35a23b608a496f87bb919717 (patch) | |
tree | f7c2b540378149aa11899fc367253fd915370a41 | |
parent | 9df6a2296e1e95fb7027d402aec57289fc79d59a (diff) | |
download | gitlab-ce-2edbafab1f7a669d35a23b608a496f87bb919717.tar.gz |
Merge branch '21650-only-active-users-can-be-members' into 'master'
Exclude some pending or inactivated rows in Member scopes
An unapproved request or not-yet-accepted invite should not give access rights. Neither should a blocked user be considered a member 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.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/21650
See merge request !1994
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r-- | CHANGELOG | 3 | ||||
-rw-r--r-- | app/models/member.rb | 33 | ||||
-rw-r--r-- | spec/models/member_spec.rb | 54 |
3 files changed, 78 insertions, 12 deletions
diff --git a/CHANGELOG b/CHANGELOG index 2a1413cf3c4..205a3c5b75e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,8 @@ Please view this file on the master branch, on stable branches it's out of date. +v 8.10.9 + - Exclude some pending or inactivated rows in Member scopes. + v 8.10.8 - Fix broken gitlab:backup:restore because of bad permissions on repo storage !6098 (Dirk Hörner) diff --git a/app/models/member.rb b/app/models/member.rb index 44db3d977fa..c58bf79b603 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -27,17 +27,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 40181a8b906..fa1261db8c2 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,11 +65,30 @@ 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) - ProjectMember.add_user(project.members, 'toto1@example.com', Gitlab::Access::DEVELOPER, @master_user) + @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', + Gitlab::Access::DEVELOPER, + current_user: @master_user + ) @invited_member = project.members.invite.find_by_invite_email('toto1@example.com') - accepted_invite_user = build(:user) - ProjectMember.add_user(project.members, 'toto2@example.com', Gitlab::Access::DEVELOPER, @master_user) + accepted_invite_user = build(:user, state: :active) + Member.add_user( + project.members, + 'toto2@example.com', + Gitlab::Access::DEVELOPER, + current_user: @master_user + ) @accepted_invite_member = project.members.invite.find_by_invite_email('toto2@example.com').tap { |u| u.accept_invite!(accepted_invite_user) } requested_user = create(:user).tap { |u| project.request_access(u) } @@ -103,6 +122,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 } @@ -110,6 +142,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 |