Merge branch '23403-fix-events-for-private-project-features' into 'security'

Respect project visibility settings in the contributions calendar

This MR fixes a number of bugs relating to access controls and date selection of events for the contributions calendar

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/23403

See merge request !2019

app/controllers/users_controller.rb

@@ -104,8 +104,7 @@ class UsersController < ApplicationController
   end
 
   def contributions_calendar
-    @contributions_calendar ||= Gitlab::ContributionsCalendar.
-      new(contributed_projects, user)
+    @contributions_calendar ||= Gitlab::ContributionsCalendar.new(user, current_user)
   end
 
   def load_events

app/models/event.rb

@@ -48,6 +48,7 @@ class Event < ActiveRecord::Base
         update_all(updated_at: Time.now)
     end
 
+    # Update Gitlab::ContributionsCalendar#activity_dates if this changes
     def contributions
       where("action = ? OR (target_type in (?) AND action in (?))",
             Event::PUSHED, ["MergeRequest", "Issue"],
@@ -61,7 +62,7 @@ class Event < ActiveRecord::Base
 
   def visible_to_user?(user = nil)
     if push?
-      true
+      Ability.allowed?(user, :download_code, project)
     elsif membership_changed?
       true
     elsif created_project?

changelogs/unreleased/23403-fix-events-for-private-project-features.yml

+---
+title: Respect event visibility in Gitlab::ContributionsCalendar
+merge_request: 
+author: 

lib/gitlab/contributions_calendar.rb

 module Gitlab
   class ContributionsCalendar
-    attr_reader :activity_dates, :projects, :user
+    attr_reader :contributor
+    attr_reader :current_user
+    attr_reader :projects
 
-    def initialize(projects, user)
-      @projects = projects
-      @user = user
+    def initialize(contributor, current_user = nil)
+      @contributor = contributor
+      @current_user = current_user
+      @projects = ContributedProjectsFinder.new(contributor).execute(current_user)
     end
 
     def activity_dates
       return @activity_dates if @activity_dates.present?
 
-      @activity_dates = {}
+      # Can't use Event.contributions here because we need to check 3 different
+      # project_features for the (currently) 3 different contribution types
       date_from = 1.year.ago
+      repo_events = event_counts(date_from, :repository).
+        having(action: Event::PUSHED)
+      issue_events = event_counts(date_from, :issues).
+        having(action: [Event::CREATED, Event::CLOSED], target_type: "Issue")
+      mr_events = event_counts(date_from, :merge_requests).
+        having(action: [Event::MERGED, Event::CREATED, Event::CLOSED], target_type: "MergeRequest")
 
-      events = Event.reorder(nil).contributions.where(author_id: user.id).
-        where("created_at > ?", date_from).where(project_id: projects).
-        group('date(created_at)').
-        select('date(created_at) as date, count(id) as total_amount').
-        map(&:attributes)
+      union = Gitlab::SQL::Union.new([repo_events, issue_events, mr_events])
+      events = Event.find_by_sql(union.to_sql).map(&:attributes)
 
-      activity_dates = (1.year.ago.to_date..Date.today).to_a
-
-      activity_dates.each do |date|
-        day_events = events.find { |day_events| day_events["date"] == date }
-
-        if day_events
-          @activity_dates[date] = day_events["total_amount"]
-        end
+      @activity_events = events.each_with_object(Hash.new {|h, k| h[k] = 0 }) do |event, activities|
+        activities[event["date"]] += event["total_amount"]
       end
-
-      @activity_dates
     end
 
     def events_by_date(date)
-      events = Event.contributions.where(author_id: user.id).
-        where("created_at > ? AND created_at < ?", date.beginning_of_day, date.end_of_day).
+      events = Event.contributions.where(author_id: contributor.id).
+        where(created_at: date.beginning_of_day..date.end_of_day).
         where(project_id: projects)
 
-      events.select do |event|
-        event.push? || event.issue? || event.merge_request?
-      end
+      # Use visible_to_user? instead of the complicated logic in activity_dates
+      # because we're only viewing the events for a single day.
+      events.select {|event| event.visible_to_user?(current_user) }
     end
 
     def starting_year
@@ -49,5 +48,30 @@ module Gitlab
     def starting_month
       Date.today.month
     end
+
+    private
+
+    def event_counts(date_from, feature)
+      t = Event.arel_table
+
+      # re-running the contributed projects query in each union is expensive, so
+      # use IN(project_ids...) instead. It's the intersection of two users so
+      # the list will be (relatively) short
+      @contributed_project_ids ||= projects.uniq.pluck(:id)
+      authed_projects = Project.where(id: @contributed_project_ids).
+        with_feature_available_for_user(feature, current_user).
+        reorder(nil).
+        select(:id)
+
+      conditions = t[:created_at].gteq(date_from.beginning_of_day).
+        and(t[:created_at].lteq(Date.today.end_of_day)).
+        and(t[:author_id].eq(contributor.id))
+
+      Event.reorder(nil).
+        select(t[:project_id], t[:target_type], t[:action], 'date(created_at) AS date', 'count(id) as total_amount').
+        group(t[:project_id], t[:target_type], t[:action], 'date(created_at)').
+        where(conditions).
+        having(t[:project_id].in(Arel::Nodes::SqlLiteral.new(authed_projects.to_sql)))
+    end
   end
 end

spec/lib/gitlab/contributions_calendar_spec.rb

+require 'spec_helper'
+
+describe Gitlab::ContributionsCalendar do
+  let(:contributor) { create(:user) }
+  let(:user) { create(:user) }
+
+  let(:private_project) do
+    create(:empty_project, :private) do |project|
+      create(:project_member, user: contributor, project: project)
+    end
+  end
+
+  let(:public_project) do
+    create(:empty_project, :public) do |project|
+      create(:project_member, user: contributor, project: project)
+    end
+  end
+
+  let(:feature_project) do
+    create(:empty_project, :public, issues_access_level: ProjectFeature::PRIVATE) do |project|
+      create(:project_member, user: contributor, project: project).project
+    end
+  end
+
+  let(:today) { Time.now.to_date }
+  let(:last_week) { today - 7.days }
+  let(:last_year) { today - 1.year }
+
+  before do
+    travel_to today
+  end
+
+  after do
+    travel_back
+  end
+
+  def calendar(current_user = nil)
+    described_class.new(contributor, current_user)
+  end
+
+  def create_event(project, day)
+    @targets ||= {}
+    @targets[project] ||= create(:issue, project: project, author: contributor)
+
+    Event.create!(
+      project: project,
+      action: Event::CREATED,
+      target: @targets[project],
+      author: contributor,
+      created_at: day,
+    )
+  end
+
+  describe '#activity_dates' do
+    it "returns a hash of date => count" do
+      create_event(public_project, last_week)
+      create_event(public_project, last_week)
+      create_event(public_project, today)
+
+      expect(calendar.activity_dates).to eq(last_week => 2, today => 1)
+    end
+
+    it "only shows private events to authorized users" do
+      create_event(private_project, today)
+      create_event(feature_project, today)
+
+      expect(calendar.activity_dates[today]).to eq(0)
+      expect(calendar(user).activity_dates[today]).to eq(0)
+      expect(calendar(contributor).activity_dates[today]).to eq(2)
+    end
+  end
+
+  describe '#events_by_date' do
+    it "returns all events for a given date" do
+      e1 = create_event(public_project, today)
+      e2 = create_event(public_project, today)
+      create_event(public_project, last_week)
+
+      expect(calendar.events_by_date(today)).to contain_exactly(e1, e2)
+    end
+
+    it "only shows private events to authorized users" do
+      e1 = create_event(public_project, today)
+      e2 = create_event(private_project, today)
+      e3 = create_event(feature_project, today)
+      create_event(public_project, last_week)
+
+      expect(calendar.events_by_date(today)).to contain_exactly(e1)
+      expect(calendar(contributor).events_by_date(today)).to contain_exactly(e1, e2, e3)
+    end
+  end
+
+  describe '#starting_year' do
+    it "should be the start of last year" do
+      expect(calendar.starting_year).to eq(last_year.year)
+    end
+  end
+
+  describe '#starting_month' do
+    it "should be the start of this month" do
+      expect(calendar.starting_month).to eq(today.month)
+    end
+  end
+end

spec/models/event_spec.rb

@@ -27,13 +27,14 @@ describe Event, models: true do
   end
 
   describe "Push event" do
-    let(:project) { create(:project) }
+    let(:project) { create(:project, :private) }
     let(:user) { project.owner }
     let(:event) { create_event(project, user) }
 
     it do
       expect(event.push?).to be_truthy
-      expect(event.visible_to_user?).to be_truthy
+      expect(event.visible_to_user?(user)).to be_truthy
+      expect(event.visible_to_user?(nil)).to be_falsey
       expect(event.tag?).to be_falsey
       expect(event.branch_name).to eq("master")
       expect(event.author).to eq(user)