diff options
author | Dmitriy Zaporozhets <dzaporozhets@gitlab.com> | 2015-04-13 17:10:25 +0300 |
---|---|---|
committer | Dmitriy Zaporozhets <dzaporozhets@gitlab.com> | 2015-04-13 17:10:25 +0300 |
commit | ecb58dacd614de66c00c8df673abb96fafa5d452 (patch) | |
tree | 9ed48d7b39bdc67b841b58e33d40e3a4231ab207 /app | |
parent | 8cf1a6f0a3b58b299e1c63283400c05209270dc2 (diff) | |
parent | 16e1076e6f69626e1d8bf53f52dc67baee9fb51e (diff) |
Merge branch 'reference-access-control' into 'master'
Only allow users to reference groups, projects, issues, MRs, commits they have access to.
Addresses https://dev.gitlab.org/gitlab/gitlabhq/issues/2183.
See merge request !1742
Diffstat (limited to 'app')
-rw-r--r-- | app/helpers/commits_helper.rb | 7 | ||||
-rw-r--r-- | app/mailers/emails/groups.rb | 1 | ||||
-rw-r--r-- | app/mailers/emails/profile.rb | 6 | ||||
-rw-r--r-- | app/mailers/emails/projects.rb | 5 | ||||
-rw-r--r-- | app/mailers/notify.rb | 12 | ||||
-rw-r--r-- | app/models/commit.rb | 12 | ||||
-rw-r--r-- | app/models/concerns/issuable.rb | 8 | ||||
-rw-r--r-- | app/models/concerns/mentionable.rb | 37 | ||||
-rw-r--r-- | app/models/merge_request.rb | 8 | ||||
-rw-r--r-- | app/services/git_push_service.rb | 6 | ||||
-rw-r--r-- | app/services/notification_service.rb | 37 | ||||
-rw-r--r-- | app/services/projects/participants_service.rb | 19 | ||||
-rw-r--r-- | app/views/projects/issues/_discussion.html.haml | 4 | ||||
-rw-r--r-- | app/views/projects/merge_requests/show/_participants.html.haml | 4 |
14 files changed, 87 insertions, 79 deletions
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 5aae697e2f0..d13d80be293 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -134,12 +134,13 @@ module CommitsHelper # avatar: true will prepend the avatar image # size: size of the avatar image in px def commit_person_link(commit, options = {}) + user = commit.send(options[:source]) + source_name = clean(commit.send "#{options[:source]}_name".to_sym) source_email = clean(commit.send "#{options[:source]}_email".to_sym) - user = User.find_for_commit(source_email, source_name) - person_name = user.nil? ? source_name : user.name - person_email = user.nil? ? source_email : user.email + person_name = user.try(:name) || source_name + person_email = user.try(:email) || source_email text = if options[:avatar] diff --git a/app/mailers/emails/groups.rb b/app/mailers/emails/groups.rb index 26f43bf955e..626eb593d51 100644 --- a/app/mailers/emails/groups.rb +++ b/app/mailers/emails/groups.rb @@ -4,6 +4,7 @@ module Emails @group_member = GroupMember.find(group_member_id) @group = @group_member.group @target_url = group_url(@group) + @current_user = @group_member.user mail(to: @group_member.user.email, subject: subject("Access to group was granted")) end diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index ab5b0765352..3a83b083109 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -1,7 +1,7 @@ module Emails module Profile def new_user_email(user_id, token = nil) - @user = User.find(user_id) + @current_user = @user = User.find(user_id) @target_url = user_url(@user) @token = token mail(to: @user.notification_email, subject: subject("Account was created for you")) @@ -9,13 +9,13 @@ module Emails def new_email_email(email_id) @email = Email.find(email_id) - @user = @email.user + @current_user = @user = @email.user mail(to: @user.notification_email, subject: subject("Email was added to your account")) end def new_ssh_key_email(key_id) @key = Key.find(key_id) - @user = @key.user + @current_user = @user = @key.user @target_url = user_url(@user) mail(to: @user.notification_email, subject: subject("SSH key was added to your account")) end diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb index 3cd812825e2..20a863c3742 100644 --- a/app/mailers/emails/projects.rb +++ b/app/mailers/emails/projects.rb @@ -4,12 +4,13 @@ module Emails @project_member = ProjectMember.find user_project_id @project = @project_member.project @target_url = namespace_project_url(@project.namespace, @project) + @current_user = @project_member.user mail(to: @project_member.user.email, subject: subject("Access to project was granted")) end def project_was_moved_email(project_id, user_id) - @user = User.find user_id + @current_user = @user = User.find user_id @project = Project.find project_id @target_url = namespace_project_url(@project.namespace, @project) mail(to: @user.notification_email, @@ -28,7 +29,7 @@ module Emails end @project = Project.find(project_id) - @author = User.find(author_id) + @current_user = @author = User.find(author_id) @reverse_compare = reverse_compare @compare = compare @ref_name = Gitlab::Git.ref_name(ref) diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 0c186ab5866..7c8b37029d1 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -13,6 +13,9 @@ class Notify < ActionMailer::Base add_template_helper MergeRequestsHelper add_template_helper EmailsHelper + attr_accessor :current_user + helper_method :current_user, :can? + default_url_options[:host] = Gitlab.config.gitlab.host default_url_options[:protocol] = Gitlab.config.gitlab.protocol default_url_options[:port] = Gitlab.config.gitlab.port unless Gitlab.config.gitlab_on_standard_port? @@ -79,9 +82,8 @@ class Notify < ActionMailer::Base # # Returns a String containing the User's email address. def recipient(recipient_id) - if recipient = User.find(recipient_id) - recipient.notification_email - end + @current_user = User.find(recipient_id) + @current_user.notification_email end # Set the References header field @@ -154,4 +156,8 @@ class Notify < ActionMailer::Base mail(headers, &block) end + + def can? + Ability.abilities.allowed?(user, action, subject) + end end diff --git a/app/models/commit.rb b/app/models/commit.rb index e0461809e10..7a0ad137650 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -117,8 +117,8 @@ class Commit # Discover issues should be closed when this commit is pushed to a project's # default branch. - def closes_issues(project) - Gitlab::ClosingIssueExtractor.closed_by_message_in_project(safe_message, project) + def closes_issues(project, current_user = self.committer) + Gitlab::ClosingIssueExtractor.new(project, current_user).closed_by_message(safe_message) end # Mentionable override. @@ -126,6 +126,14 @@ class Commit "commit #{id}" end + def author + User.find_for_commit(author_email, author_name) + end + + def committer + User.find_for_commit(committer_email, committer_name) + end + def method_missing(m, *args, &block) @raw.send(m, *args, &block) end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 88ac83744df..478134dff68 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -118,16 +118,16 @@ module Issuable end # Return all users participating on the discussion - def participants + def participants(current_user = self.author) users = [] users << author users << assignee if is_assigned? mentions = [] - mentions << self.mentioned_users + mentions << self.mentioned_users(current_user) notes.each do |note| users << note.author - mentions << note.mentioned_users + mentions << note.mentioned_users(current_user) end users.concat(mentions.reduce([], :|)).uniq @@ -140,7 +140,7 @@ module Issuable return subscription.subscribed end - participants.include?(user) + participants(user).include?(user) end def toggle_subscription(user) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index d96e07034ec..b7882a2bb16 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -42,35 +42,22 @@ module Mentionable Note.cross_reference_exists?(target, local_reference) end - def mentioned_users - users = [] - return users if mentionable_text.blank? - has_project = self.respond_to? :project - matches = mentionable_text.scan(/@[a-zA-Z][a-zA-Z0-9_\-\.]*/) - matches.each do |match| - identifier = match.delete "@" - if identifier == "all" - users.push(*project.team.members.flatten) - elsif namespace = Namespace.find_by(path: identifier) - if namespace.is_a?(Group) - users.push(*namespace.users) - else - users << namespace.owner - end - end - end - users.uniq + def mentioned_users(current_user = nil) + return [] if mentionable_text.blank? + + ext = Gitlab::ReferenceExtractor.new(self.project, current_user) + ext.analyze(mentionable_text) + ext.users.uniq end # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference. - def references(p = project, text = mentionable_text) + def references(p = project, current_user = self.author, text = mentionable_text) return [] if text.blank? - ext = Gitlab::ReferenceExtractor.new - ext.analyze(text, p) - (ext.issues_for(p) + - ext.merge_requests_for(p) + - ext.commits_for(p)).uniq - [local_reference] + ext = Gitlab::ReferenceExtractor.new(p, current_user) + ext.analyze(text) + + (ext.issues + ext.merge_requests + ext.commits).uniq - [local_reference] end # Create a cross-reference Note for each GFM reference to another Mentionable found in +mentionable_text+. @@ -96,7 +83,7 @@ module Mentionable # Only proceed if the saved changes actually include a chance to an attr_mentionable field. return unless mentionable_changed - preexisting = references(p, original) + preexisting = references(p, self.author, original) create_cross_references!(p, a, preexisting) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5634f9a686e..35cb920d8bc 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -257,11 +257,11 @@ class MergeRequest < ActiveRecord::Base end # Return the set of issues that will be closed if this merge request is accepted. - def closes_issues + def closes_issues(current_user = self.author) if target_branch == project.default_branch - issues = commits.flat_map { |c| c.closes_issues(project) } - issues.push(*Gitlab::ClosingIssueExtractor. - closed_by_message_in_project(description, project)) + issues = commits.flat_map { |c| c.closes_issues(project, current_user) } + issues.push(*Gitlab::ClosingIssueExtractor.new(project, current_user). + closed_by_message(description)) issues.uniq.sort_by(&:id) else [] diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 1f0b29dff5e..31e0167d247 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -70,7 +70,7 @@ class GitPushService # Close issues if these commits were pushed to the project's default branch and the commit message matches the # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to # a different branch. - issues_to_close = commit.closes_issues(project) + issues_to_close = commit.closes_issues(project, user) # Load commit author only if needed. # For push with 1k commits it prevents 900+ requests in database @@ -87,7 +87,7 @@ class GitPushService # Create cross-reference notes for any other references. Omit any issues that were referenced in an # issue-closing phrase, or have already been mentioned from this commit (probably from this commit # being pushed to a different branch). - refs = commit.references(project) - issues_to_close + refs = commit.references(project, user) - issues_to_close refs.reject! { |r| commit.has_mentioned?(r) } if refs.present? @@ -127,6 +127,6 @@ class GitPushService end def commit_user(commit) - User.find_for_commit(commit.author_email, commit.author_name) || user + commit.author || user end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index cc5853144c5..42547f6f481 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -123,32 +123,29 @@ class NotificationService return true if note.note.start_with?('Status changed to closed') return true if note.cross_reference? && note.system == true - opts = { noteable_type: note.noteable_type, project_id: note.project_id } - target = note.noteable - if target.respond_to?(:participants) - recipients = target.participants - else - recipients = note.mentioned_users - end + recipients = [] if note.commit_id.present? - opts.merge!(commit_id: note.commit_id) recipients << note.commit_author - else - opts.merge!(noteable_id: note.noteable_id) end # Get users who left comment in thread - recipients = recipients.concat(User.where(id: Note.where(opts).pluck(:author_id))) + recipients = recipients.concat(noteable_commenters(note)) # Merge project watchers recipients = recipients.concat(project_watchers(note.project)).compact.uniq - # Reject mention users unless mentioned in comment - recipients = reject_mention_users(recipients - note.mentioned_users, note.project) - recipients = recipients + note.mentioned_users + # Reject users with Mention notification level + recipients = reject_mention_users(recipients, note.project) + + # Add explicitly mentioned users + if target.respond_to?(:participants) + recipients = recipients.concat(target.participants) + else + recipients = recipients.concat(note.mentioned_users) + end # Reject mutes users recipients = reject_muted_users(recipients, note.project) @@ -195,6 +192,18 @@ class NotificationService protected + def noteable_commenters(note) + opts = { noteable_type: note.noteable_type, project_id: note.project_id } + + if note.commit_id.present? + opts.merge!(commit_id: note.commit_id) + else + opts.merge!(noteable_id: note.noteable_id) + end + + User.where(id: Note.where(opts).pluck(:author_id)) + end + # Get project users with WATCH notification level def project_watchers(project) project_members = project_member_notification(project) diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb index bcbacbff562..ae6260bcdab 100644 --- a/app/services/projects/participants_service.rb +++ b/app/services/projects/participants_service.rb @@ -1,10 +1,5 @@ module Projects class ParticipantsService < BaseService - def initialize(project, user) - @project = project - @user = user - end - def execute(note_type, note_id) participating = if note_type && note_id @@ -12,7 +7,7 @@ module Projects else [] end - project_members = sorted(@project.team.members) + project_members = sorted(project.team.members) participants = all_members + groups + project_members + participating participants.uniq end @@ -20,11 +15,11 @@ module Projects def participants_in(type, id) users = case type when "Issue" - issue = @project.issues.find_by_iid(id) - issue ? issue.participants : [] + issue = project.issues.find_by_iid(id) + issue ? issue.participants(current_user) : [] when "MergeRequest" - merge_request = @project.merge_requests.find_by_iid(id) - merge_request ? merge_request.participants : [] + merge_request = project.merge_requests.find_by_iid(id) + merge_request ? merge_request.participants(current_user) : [] when "Commit" author_ids = Note.for_commit_id(id).pluck(:author_id).uniq User.where(id: author_ids) @@ -41,14 +36,14 @@ module Projects end def groups - @user.authorized_groups.sort_by(&:path).map do |group| + current_user.authorized_groups.sort_by(&:path).map do |group| count = group.users.count { username: group.path, name: "#{group.name} (#{count})" } end end def all_members - count = @project.team.members.flatten.count + count = project.team.members.flatten.count [{ username: "all", name: "All Project and Group Members (#{count})" }] end end diff --git a/app/views/projects/issues/_discussion.html.haml b/app/views/projects/issues/_discussion.html.haml index 0d3028d50b4..288b48f4583 100644 --- a/app/views/projects/issues/_discussion.html.haml +++ b/app/views/projects/issues/_discussion.html.haml @@ -9,8 +9,8 @@ .votes-holder.pull-right #votes= render 'votes/votes_block', votable: @issue .participants - %span= pluralize(@issue.participants.count, 'participant') - - @issue.participants.each do |participant| + %span= pluralize(@issue.participants(current_user).count, 'participant') + - @issue.participants(current_user).each do |participant| = link_to_member(@project, participant, name: false, size: 24) .voting_notes#notes= render "projects/notes/notes_with_form" %aside.col-md-3 diff --git a/app/views/projects/merge_requests/show/_participants.html.haml b/app/views/projects/merge_requests/show/_participants.html.haml index 4f34af1737d..9c93fa55fe6 100644 --- a/app/views/projects/merge_requests/show/_participants.html.haml +++ b/app/views/projects/merge_requests/show/_participants.html.haml @@ -1,4 +1,4 @@ .participants - %span #{@merge_request.participants.count} participants - - @merge_request.participants.each do |participant| + %span #{@merge_request.participants(current_user).count} participants + - @merge_request.participants(current_user).each do |participant| = link_to_member(@project, participant, name: false, size: 24) |