diff --git a/app/models/group.rb b/app/models/group.rb index 73b0f1c657242ae8dc7463a760cf78605c1ffcbf..4248e1162d85edcdd32fbae8de65db7547957d7d 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -65,7 +65,9 @@ class Group < Namespace def select_for_project_authorization if current_scope.joins_values.include?(:shared_projects) - select("members.user_id, projects.id AS project_id, project_group_links.group_access") + joins('INNER JOIN namespaces project_namespace ON project_namespace.id = projects.namespace_id') + .where('project_namespace.share_with_group_lock = ?', false) + .select("members.user_id, projects.id AS project_id, LEAST(project_group_links.group_access, members.access_level) AS access_level") else super end diff --git a/app/models/issue.rb b/app/models/issue.rb index 6e8f5d3c4226f203462d7cfc122064f46aae8c58..dd0cb75f9a8a718a7bae6a3af115f6c74eac0370 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -93,7 +93,7 @@ class Issue < ActiveRecord::Base # Check if we are scoped to a specific project's issues if owner_project - if owner_project.authorized_for_user?(user, Gitlab::Access::REPORTER) + if owner_project.team.member?(user, Gitlab::Access::REPORTER) # If the project is authorized for the user, they can see all issues in the project return all else diff --git a/app/models/member.rb b/app/models/member.rb index 7be2665bf4816480e1706767e4dab7ab335847e7..df93aaee8471f2d2695ef02b7a25be788fca919d 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -246,7 +246,7 @@ class Member < ActiveRecord::Base end def post_update_hook - # override in subclass + UserProjectAccessChangedService.new(user.id).execute if access_level_changed? end def post_destroy_hook diff --git a/app/models/namespace.rb b/app/models/namespace.rb index b67049f0f55cb6273f592c1ba4ab0244a202951c..99da26a89fb1476ef53b83f0cfece5b7ea9da837 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -27,6 +27,7 @@ class Namespace < ActiveRecord::Base delegate :name, to: :owner, allow_nil: true, prefix: true after_update :move_dir, if: :path_changed? + after_commit :refresh_access_of_projects_invited_groups, on: :update, if: -> { previous_changes.key?('share_with_group_lock') } # Save the storage paths before the projects are destroyed to use them on after destroy before_destroy(prepend: true) { @old_repository_storage_paths = repository_storage_paths } @@ -175,4 +176,11 @@ class Namespace < ActiveRecord::Base end end end + + def refresh_access_of_projects_invited_groups + Group. + joins(project_group_links: :project). + where(projects: { namespace_id: id }). + find_each(&:refresh_members_authorized_projects) + end end diff --git a/app/models/project.rb b/app/models/project.rb index 76c1fc4945d4009db5036f97be894bbf5aaaa09e..bd9fcb2f3b7854652bd739d9d025bd9356affe4a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -126,6 +126,8 @@ class Project < ActiveRecord::Base has_many :hooks, dependent: :destroy, class_name: 'ProjectHook' has_many :protected_branches, dependent: :destroy + has_many :project_authorizations, dependent: :destroy + has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User' has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source alias_method :members, :project_members has_many :users, through: :project_members @@ -1293,14 +1295,6 @@ class Project < ActiveRecord::Base end end - # Checks if `user` is authorized for this project, with at least the - # `min_access_level` (if given). - def authorized_for_user?(user, min_access_level = nil) - return false unless user - - user.authorized_project?(self, min_access_level) - end - def append_or_update_attribute(name, value) old_values = public_send(name.to_s) diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 65549d8cd08b817dc4aa367dd34ffce7a6bc99d7..8a53e974b6f6e725909370a5a83c336282102920 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -80,19 +80,19 @@ class ProjectTeam alias_method :users, :members def guests - @guests ||= fetch_members(:guests) + @guests ||= fetch_members(Gitlab::Access::GUEST) end def reporters - @reporters ||= fetch_members(:reporters) + @reporters ||= fetch_members(Gitlab::Access::REPORTER) end def developers - @developers ||= fetch_members(:developers) + @developers ||= fetch_members(Gitlab::Access::DEVELOPER) end def masters - @masters ||= fetch_members(:masters) + @masters ||= fetch_members(Gitlab::Access::MASTER) end def import(source_project, current_user = nil) @@ -141,8 +141,12 @@ class ProjectTeam max_member_access(user.id) == Gitlab::Access::MASTER end - def member?(user, min_member_access = Gitlab::Access::GUEST) - max_member_access(user.id) >= min_member_access + # Checks if `user` is authorized for this project, with at least the + # `min_access_level` (if given). + def member?(user, min_access_level = Gitlab::Access::GUEST) + return false unless user + + user.authorized_project?(project, min_access_level) end def human_max_access(user_id) @@ -165,112 +169,29 @@ class ProjectTeam # Lookup only the IDs we need user_ids = user_ids - access.keys + users_access = project.project_authorizations. + where(user: user_ids). + group(:user_id). + maximum(:access_level) - if user_ids.present? - user_ids.each { |id| access[id] = Gitlab::Access::NO_ACCESS } - - member_access = project.members.access_for_user_ids(user_ids) - merge_max!(access, member_access) - - if group - group_access = group.members.access_for_user_ids(user_ids) - merge_max!(access, group_access) - end - - # Each group produces a list of maximum access level per user. We take the - # max of the values produced by each group. - if project_shared_with_group? - project.project_group_links.each do |group_link| - invited_access = max_invited_level_for_users(group_link, user_ids) - merge_max!(access, invited_access) - end - end - end - + access.merge!(users_access) access end def max_member_access(user_id) - max_member_access_for_user_ids([user_id])[user_id] + max_member_access_for_user_ids([user_id])[user_id] || Gitlab::Access::NO_ACCESS end private - # For a given group, return the maximum access level for the user. This is the min of - # the invited access level of the group and the access level of the user within the group. - # For example, if the group has been given DEVELOPER access but the member has MASTER access, - # the user should receive only DEVELOPER access. - def max_invited_level_for_users(group_link, user_ids) - invited_group = group_link.group - capped_access_level = group_link.group_access - access = invited_group.group_members.access_for_user_ids(user_ids) - - # If the user is not in the list, assume he/she does not have access - missing_users = user_ids - access.keys - missing_users.each { |id| access[id] = Gitlab::Access::NO_ACCESS } - - # Cap the maximum access by the invited level access - access.each { |key, value| access[key] = [value, capped_access_level].min } - end - def fetch_members(level = nil) - project_members = project.members - group_members = group ? group.members : [] - - if level - project_members = project_members.public_send(level) - group_members = group_members.public_send(level) if group - end - - user_ids = project_members.pluck(:user_id) - - invited_members = fetch_invited_members(level) - user_ids.push(*invited_members.map(&:user_id)) if invited_members.any? - - user_ids.push(*group_members.pluck(:user_id)) if group + members = project.authorized_users + members = members.where(project_authorizations: { access_level: level }) if level - User.where(id: user_ids) + members end def group project.group end - - def merge_max!(first_hash, second_hash) - first_hash.merge!(second_hash) { |_key, old, new| old > new ? old : new } - end - - def project_shared_with_group? - project.invited_groups.any? && project.allowed_to_share_with_group? - end - - def fetch_invited_members(level = nil) - invited_members = [] - - return invited_members unless project_shared_with_group? - - project.project_group_links.includes(group: [:group_members]).each do |link| - invited_group_members = link.group.members - - if level - numeric_level = GroupMember.access_level_roles[level.to_s.singularize.titleize] - - # If we're asked for a level that's higher than the group's access, - # there's nothing left to do - next if numeric_level > link.group_access - - # Make sure we include everyone _above_ the requested level as well - invited_group_members = - if numeric_level == link.group_access - invited_group_members.where("access_level >= ?", link.group_access) - else - invited_group_members.public_send(level) - end - end - - invited_members << invited_group_members - end - - invited_members.flatten.compact - end end diff --git a/app/models/user.rb b/app/models/user.rb index 29fb849940a6a794fc4c7533502a900f410b9845..223d84ba9169ceedbfc66e7c898a5697046b25eb 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -926,7 +926,7 @@ class User < ActiveRecord::Base # Returns a union query of projects that the user is authorized to access def project_authorizations_union relations = [ - personal_projects.select("#{id} AS user_id, projects.id AS project_id, #{Gitlab::Access::OWNER} AS access_level"), + personal_projects.select("#{id} AS user_id, projects.id AS project_id, #{Gitlab::Access::MASTER} AS access_level"), groups_projects.select_for_project_authorization, projects.select_for_project_authorization, groups.joins(:shared_projects).select_for_project_authorization diff --git a/changelogs/unreleased/fix-drop-project-authorized-for-user.yml b/changelogs/unreleased/fix-drop-project-authorized-for-user.yml new file mode 100644 index 0000000000000000000000000000000000000000..0d11969575aa741e1973d524e08e72ab7b070cfb --- /dev/null +++ b/changelogs/unreleased/fix-drop-project-authorized-for-user.yml @@ -0,0 +1,4 @@ +--- +title: Use authorized projects in ProjectTeam +merge_request: +author: diff --git a/db/fixtures/development/06_teams.rb b/db/fixtures/development/06_teams.rb index 9739a5ac8d5a91df32437e0fb91cb28e24a35bce..04c3690e152086cecf767da56cffad0500a1b780 100644 --- a/db/fixtures/development/06_teams.rb +++ b/db/fixtures/development/06_teams.rb @@ -1,20 +1,25 @@ -Gitlab::Seeder.quiet do - Group.all.each do |group| - User.all.sample(4).each do |user| - if group.add_user(user, Gitlab::Access.values.sample).persisted? - print '.' - else - print 'F' +require 'sidekiq/testing' +require './db/fixtures/support/serialized_transaction' + +Sidekiq::Testing.inline! do + Gitlab::Seeder.quiet do + Group.all.each do |group| + User.all.sample(4).each do |user| + if group.add_user(user, Gitlab::Access.values.sample).persisted? + print '.' + else + print 'F' + end end end - end - Project.all.each do |project| - User.all.sample(4).each do |user| - if project.team << [user, Gitlab::Access.values.sample] - print '.' - else - print 'F' + Project.all.each do |project| + User.all.sample(4).each do |user| + if project.team << [user, Gitlab::Access.values.sample] + print '.' + else + print 'F' + end end end end diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb index ffca1c94da105fdcf8a66442c42591b2f1c186c9..33934cdf8b1387f98c0e7184822db3078c0f0b27 100644 --- a/spec/helpers/members_helper_spec.rb +++ b/spec/helpers/members_helper_spec.rb @@ -10,7 +10,7 @@ describe MembersHelper do end describe '#remove_member_message' do - let(:requester) { build(:user) } + let(:requester) { create(:user) } let(:project) { create(:empty_project, :public, :access_requestable) } let(:project_member) { build(:project_member, project: project) } let(:project_member_invite) { build(:project_member, project: project).tap { |m| m.generate_invite_token! } } @@ -31,7 +31,7 @@ describe MembersHelper do end describe '#remove_member_title' do - let(:requester) { build(:user) } + let(:requester) { create(:user) } let(:project) { create(:empty_project, :public, :access_requestable) } let(:project_member) { build(:project_member, project: project) } let(:project_member_request) { project.request_access(requester) } diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index fe3c39e38db9a3b972f0f54af64bae511a52ff29..7e00e214c6e5df189a98547e9ea74ace11f7d3d7 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -186,6 +186,8 @@ project: - environments - deployments - project_feature +- authorized_users +- project_authorizations award_emoji: - awardable - user diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 300425767ed9eef901fc324d47a3f47ecfffe366..89e93dce8c5c5057c8887e4eb5ddb51d4633701a 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -331,7 +331,7 @@ describe Issue, models: true do end context 'with a user' do - let(:user) { build(:user) } + let(:user) { create(:user) } let(:issue) { build(:issue) } it 'returns true when the issue is readable' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 5ba741f40eca33bef26c243cb55387fc7cc30e58..da38254d1bce2b48083fb11e0c8007edfa01736d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1507,57 +1507,6 @@ describe Project, models: true do end end - describe 'authorized_for_user' do - let(:group) { create(:group) } - let(:developer) { create(:user) } - let(:master) { create(:user) } - let(:personal_project) { create(:project, namespace: developer.namespace) } - let(:group_project) { create(:project, namespace: group) } - let(:members_project) { create(:project) } - let(:shared_project) { create(:project) } - - before do - group.add_master(master) - group.add_developer(developer) - - members_project.team << [developer, :developer] - members_project.team << [master, :master] - - create(:project_group_link, project: shared_project, group: group, group_access: Gitlab::Access::DEVELOPER) - end - - it 'returns false for no user' do - expect(personal_project.authorized_for_user?(nil)).to be(false) - end - - it 'returns true for personal projects of the user' do - expect(personal_project.authorized_for_user?(developer)).to be(true) - end - - it 'returns true for projects of groups the user is a member of' do - expect(group_project.authorized_for_user?(developer)).to be(true) - end - - it 'returns true for projects for which the user is a member of' do - expect(members_project.authorized_for_user?(developer)).to be(true) - end - - it 'returns true for projects shared on a group the user is a member of' do - expect(shared_project.authorized_for_user?(developer)).to be(true) - end - - it 'checks for the correct minimum level access' do - expect(group_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false) - expect(group_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true) - expect(members_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false) - expect(members_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true) - expect(shared_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false) - expect(shared_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(false) - expect(shared_project.authorized_for_user?(developer, Gitlab::Access::DEVELOPER)).to be(true) - expect(shared_project.authorized_for_user?(master, Gitlab::Access::DEVELOPER)).to be(true) - end - end - describe 'change_head' do let(:project) { create(:project) } diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index eb6b009c7cff98d68f06488ed6f7d809c800344a..0475cecaa2da4f9f9f0d33b0e80edb5127b5f3a7 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -37,7 +37,7 @@ describe ProjectTeam, models: true do context 'group project' do let(:group) { create(:group) } - let(:project) { create(:empty_project, group: group) } + let!(:project) { create(:empty_project, group: group) } before do group.add_master(master) @@ -118,7 +118,7 @@ describe ProjectTeam, models: true do context 'group project' do let(:group) { create(:group) } - let(:project) { create(:empty_project, group: group) } + let!(:project) { create(:empty_project, group: group) } it 'returns project members' do group_member = create(:group_member, group: group) @@ -178,9 +178,9 @@ describe ProjectTeam, models: true do it 'returns Master role' do user = create(:user) group = create(:group) - group.add_master(user) + project = create(:empty_project, namespace: group) - project = build_stubbed(:empty_project, namespace: group) + group.add_master(user) expect(project.team.human_max_access(user.id)).to eq 'Master' end @@ -188,9 +188,9 @@ describe ProjectTeam, models: true do it 'returns Owner role' do user = create(:user) group = create(:group) - group.add_owner(user) + project = create(:empty_project, namespace: group) - project = build_stubbed(:empty_project, namespace: group) + group.add_owner(user) expect(project.team.human_max_access(user.id)).to eq 'Owner' end @@ -244,7 +244,7 @@ describe ProjectTeam, models: true do context 'group project' do let(:group) { create(:group, :access_requestable) } - let(:project) { create(:empty_project, group: group) } + let!(:project) { create(:empty_project, group: group) } before do group.add_master(master) @@ -261,6 +261,57 @@ describe ProjectTeam, models: true do end end + describe '#member?' do + let(:group) { create(:group) } + let(:developer) { create(:user) } + let(:master) { create(:user) } + let(:personal_project) { create(:project, namespace: developer.namespace) } + let(:group_project) { create(:project, namespace: group) } + let(:members_project) { create(:project) } + let(:shared_project) { create(:project) } + + before do + group.add_master(master) + group.add_developer(developer) + + members_project.team << [developer, :developer] + members_project.team << [master, :master] + + create(:project_group_link, project: shared_project, group: group) + end + + it 'returns false for no user' do + expect(personal_project.team.member?(nil)).to be(false) + end + + it 'returns true for personal projects of the user' do + expect(personal_project.team.member?(developer)).to be(true) + end + + it 'returns true for projects of groups the user is a member of' do + expect(group_project.team.member?(developer)).to be(true) + end + + it 'returns true for projects for which the user is a member of' do + expect(members_project.team.member?(developer)).to be(true) + end + + it 'returns true for projects shared on a group the user is a member of' do + expect(shared_project.team.member?(developer)).to be(true) + end + + it 'checks for the correct minimum level access' do + expect(group_project.team.member?(developer, Gitlab::Access::MASTER)).to be(false) + expect(group_project.team.member?(master, Gitlab::Access::MASTER)).to be(true) + expect(members_project.team.member?(developer, Gitlab::Access::MASTER)).to be(false) + expect(members_project.team.member?(master, Gitlab::Access::MASTER)).to be(true) + expect(shared_project.team.member?(developer, Gitlab::Access::MASTER)).to be(false) + expect(shared_project.team.member?(master, Gitlab::Access::MASTER)).to be(false) + expect(shared_project.team.member?(developer, Gitlab::Access::DEVELOPER)).to be(true) + expect(shared_project.team.member?(master, Gitlab::Access::DEVELOPER)).to be(true) + end + end + shared_examples_for "#max_member_access_for_users" do |enable_request_store| describe "#max_member_access_for_users" do before do