Commit ba0d4877 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'fix/drop-project-authorized-for-user' into 'master'

Use authorized projects in ProjectTeam

Closes #23938 and #23636

See merge request !7586
parents 6f7ce372 2ea5ef0b
...@@ -65,7 +65,9 @@ class Group < Namespace ...@@ -65,7 +65,9 @@ class Group < Namespace
def select_for_project_authorization def select_for_project_authorization
if current_scope.joins_values.include?(:shared_projects) 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 else
super super
end end
......
...@@ -93,7 +93,7 @@ class Issue < ActiveRecord::Base ...@@ -93,7 +93,7 @@ class Issue < ActiveRecord::Base
# Check if we are scoped to a specific project's issues # Check if we are scoped to a specific project's issues
if owner_project 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 # If the project is authorized for the user, they can see all issues in the project
return all return all
else else
......
...@@ -246,7 +246,7 @@ class Member < ActiveRecord::Base ...@@ -246,7 +246,7 @@ class Member < ActiveRecord::Base
end end
def post_update_hook def post_update_hook
# override in subclass UserProjectAccessChangedService.new(user.id).execute if access_level_changed?
end end
def post_destroy_hook def post_destroy_hook
......
...@@ -27,6 +27,7 @@ class Namespace < ActiveRecord::Base ...@@ -27,6 +27,7 @@ class Namespace < ActiveRecord::Base
delegate :name, to: :owner, allow_nil: true, prefix: true delegate :name, to: :owner, allow_nil: true, prefix: true
after_update :move_dir, if: :path_changed? 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 # 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 } before_destroy(prepend: true) { @old_repository_storage_paths = repository_storage_paths }
...@@ -175,4 +176,11 @@ class Namespace < ActiveRecord::Base ...@@ -175,4 +176,11 @@ class Namespace < ActiveRecord::Base
end end
end 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 end
...@@ -126,6 +126,8 @@ class Project < ActiveRecord::Base ...@@ -126,6 +126,8 @@ class Project < ActiveRecord::Base
has_many :hooks, dependent: :destroy, class_name: 'ProjectHook' has_many :hooks, dependent: :destroy, class_name: 'ProjectHook'
has_many :protected_branches, dependent: :destroy 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 has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source
alias_method :members, :project_members alias_method :members, :project_members
has_many :users, through: :project_members has_many :users, through: :project_members
...@@ -1293,14 +1295,6 @@ class Project < ActiveRecord::Base ...@@ -1293,14 +1295,6 @@ class Project < ActiveRecord::Base
end end
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) def append_or_update_attribute(name, value)
old_values = public_send(name.to_s) old_values = public_send(name.to_s)
......
...@@ -80,19 +80,19 @@ class ProjectTeam ...@@ -80,19 +80,19 @@ class ProjectTeam
alias_method :users, :members alias_method :users, :members
def guests def guests
@guests ||= fetch_members(:guests) @guests ||= fetch_members(Gitlab::Access::GUEST)
end end
def reporters def reporters
@reporters ||= fetch_members(:reporters) @reporters ||= fetch_members(Gitlab::Access::REPORTER)
end end
def developers def developers
@developers ||= fetch_members(:developers) @developers ||= fetch_members(Gitlab::Access::DEVELOPER)
end end
def masters def masters
@masters ||= fetch_members(:masters) @masters ||= fetch_members(Gitlab::Access::MASTER)
end end
def import(source_project, current_user = nil) def import(source_project, current_user = nil)
...@@ -141,8 +141,12 @@ class ProjectTeam ...@@ -141,8 +141,12 @@ class ProjectTeam
max_member_access(user.id) == Gitlab::Access::MASTER max_member_access(user.id) == Gitlab::Access::MASTER
end end
def member?(user, min_member_access = Gitlab::Access::GUEST) # Checks if `user` is authorized for this project, with at least the
max_member_access(user.id) >= min_member_access # `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 end
def human_max_access(user_id) def human_max_access(user_id)
...@@ -165,112 +169,29 @@ class ProjectTeam ...@@ -165,112 +169,29 @@ class ProjectTeam
# Lookup only the IDs we need # Lookup only the IDs we need
user_ids = user_ids - access.keys 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? access.merge!(users_access)
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 access
end end
def max_member_access(user_id) 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 end
private 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) def fetch_members(level = nil)
project_members = project.members members = project.authorized_users
group_members = group ? group.members : [] members = members.where(project_authorizations: { access_level: level }) if level
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
User.where(id: user_ids) members
end end
def group def group
project.group project.group
end 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 end
...@@ -926,7 +926,7 @@ class User < ActiveRecord::Base ...@@ -926,7 +926,7 @@ class User < ActiveRecord::Base
# Returns a union query of projects that the user is authorized to access # Returns a union query of projects that the user is authorized to access
def project_authorizations_union def project_authorizations_union
relations = [ 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, groups_projects.select_for_project_authorization,
projects.select_for_project_authorization, projects.select_for_project_authorization,
groups.joins(:shared_projects).select_for_project_authorization groups.joins(:shared_projects).select_for_project_authorization
......
---
title: Use authorized projects in ProjectTeam
merge_request:
author:
Gitlab::Seeder.quiet do require 'sidekiq/testing'
require './db/fixtures/support/serialized_transaction'
Sidekiq::Testing.inline! do
Gitlab::Seeder.quiet do
Group.all.each do |group| Group.all.each do |group|
User.all.sample(4).each do |user| User.all.sample(4).each do |user|
if group.add_user(user, Gitlab::Access.values.sample).persisted? if group.add_user(user, Gitlab::Access.values.sample).persisted?
...@@ -18,4 +22,5 @@ Gitlab::Seeder.quiet do ...@@ -18,4 +22,5 @@ Gitlab::Seeder.quiet do
end end
end end
end end
end
end end
...@@ -10,7 +10,7 @@ describe MembersHelper do ...@@ -10,7 +10,7 @@ describe MembersHelper do
end end
describe '#remove_member_message' do describe '#remove_member_message' do
let(:requester) { build(:user) } let(:requester) { create(:user) }
let(:project) { create(:empty_project, :public, :access_requestable) } let(:project) { create(:empty_project, :public, :access_requestable) }
let(:project_member) { build(:project_member, project: project) } let(:project_member) { build(:project_member, project: project) }
let(:project_member_invite) { build(:project_member, project: project).tap { |m| m.generate_invite_token! } } let(:project_member_invite) { build(:project_member, project: project).tap { |m| m.generate_invite_token! } }
...@@ -31,7 +31,7 @@ describe MembersHelper do ...@@ -31,7 +31,7 @@ describe MembersHelper do
end end
describe '#remove_member_title' do describe '#remove_member_title' do
let(:requester) { build(:user) } let(:requester) { create(:user) }
let(:project) { create(:empty_project, :public, :access_requestable) } let(:project) { create(:empty_project, :public, :access_requestable) }
let(:project_member) { build(:project_member, project: project) } let(:project_member) { build(:project_member, project: project) }
let(:project_member_request) { project.request_access(requester) } let(:project_member_request) { project.request_access(requester) }
......
...@@ -186,6 +186,8 @@ project: ...@@ -186,6 +186,8 @@ project:
- environments - environments
- deployments - deployments
- project_feature - project_feature
- authorized_users
- project_authorizations
award_emoji: award_emoji:
- awardable - awardable
- user - user
......
...@@ -331,7 +331,7 @@ describe Issue, models: true do ...@@ -331,7 +331,7 @@ describe Issue, models: true do
end end
context 'with a user' do context 'with a user' do
let(:user) { build(:user) } let(:user) { create(:user) }
let(:issue) { build(:issue) } let(:issue) { build(:issue) }
it 'returns true when the issue is readable' do it 'returns true when the issue is readable' do
......
...@@ -1507,57 +1507,6 @@ describe Project, models: true do ...@@ -1507,57 +1507,6 @@ describe Project, models: true do
end end
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 describe 'change_head' do
let(:project) { create(:project) } let(:project) { create(:project) }
......
...@@ -37,7 +37,7 @@ describe ProjectTeam, models: true do ...@@ -37,7 +37,7 @@ describe ProjectTeam, models: true do
context 'group project' do context 'group project' do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:project) { create(:empty_project, group: group) } let!(:project) { create(:empty_project, group: group) }
before do before do
group.add_master(master) group.add_master(master)
...@@ -118,7 +118,7 @@ describe ProjectTeam, models: true do ...@@ -118,7 +118,7 @@ describe ProjectTeam, models: true do
context 'group project' do context 'group project' do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:project) { create(:empty_project, group: group) } let!(:project) { create(:empty_project, group: group) }
it 'returns project members' do it 'returns project members' do
group_member = create(:group_member, group: group) group_member = create(:group_member, group: group)
...@@ -178,9 +178,9 @@ describe ProjectTeam, models: true do ...@@ -178,9 +178,9 @@ describe ProjectTeam, models: true do
it 'returns Master role' do it 'returns Master role' do
user = create(:user) user = create(:user)
group = create(:group) 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' expect(project.team.human_max_access(user.id)).to eq 'Master'
end end
...@@ -188,9 +188,9 @@ describe ProjectTeam, models: true do ...@@ -188,9 +188,9 @@ describe ProjectTeam, models: true do
it 'returns Owner role' do it 'returns Owner role' do
user = create(:user) user = create(:user)
group = create(:group) 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' expect(project.team.human_max_access(user.id)).to eq 'Owner'
end end
...@@ -244,7 +244,7 @@ describe ProjectTeam, models: true do ...@@ -244,7 +244,7 @@ describe ProjectTeam, models: true do
context 'group project' do context 'group project' do
let(:group) { create(:group, :access_requestable) } let(:group) { create(:group, :access_requestable) }
let(:project) { create(:empty_project, group: group) } let!(:project) { create(:empty_project, group: group) }
before do before do
group.add_master(master) group.add_master(master)
...@@ -261,6 +261,57 @@ describe ProjectTeam, models: true do ...@@ -261,6 +261,57 @@ describe ProjectTeam, models: true do
end end
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| shared_examples_for "#max_member_access_for_users" do |enable_request_store|
describe "#max_member_access_for_users" do describe "#max_member_access_for_users" do
before do before do
......
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