From b0088b527eacd16773a85ad8f88e49de7c646cf1 Mon Sep 17 00:00:00 2001
From: Robert Speicher <robert@gitlab.com>
Date: Fri, 4 Nov 2016 14:15:43 +0000
Subject: [PATCH] Merge branch '23403-fix-events-for-private-project-features'
into 'security'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
app/controllers/users_controller.rb | 3 +-
app/models/event.rb | 3 +-
lib/gitlab/contributions_calendar.rb | 74 ++++++++-----
.../lib/gitlab/contributions_calendar_spec.rb | 104 ++++++++++++++++++
spec/models/event_spec.rb | 5 +-
5 files changed, 159 insertions(+), 30 deletions(-)
create mode 100644 spec/lib/gitlab/contributions_calendar_spec.rb
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 6a881b271d7..c4508ccc3b9 100644
--- a/app/controllers/users_controller.rb
+++ b/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
diff --git a/app/models/event.rb b/app/models/event.rb
index 43e67069b70..c76d88b1c7b 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -49,6 +49,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"],
@@ -62,7 +63,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?
diff --git a/lib/gitlab/contributions_calendar.rb b/lib/gitlab/contributions_calendar.rb
index b164f5a2eea..7e3d5647b39 100644
--- a/lib/gitlab/contributions_calendar.rb
+++ b/lib/gitlab/contributions_calendar.rb
@@ -1,45 +1,44 @@
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
diff --git a/spec/lib/gitlab/contributions_calendar_spec.rb b/spec/lib/gitlab/contributions_calendar_spec.rb
new file mode 100644
index 00000000000..01b2a55b63c
--- /dev/null
+++ b/spec/lib/gitlab/contributions_calendar_spec.rb
@@ -0,0 +1,104 @@
+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
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index aca49be2942..29a3af68a9b 100644
--- a/spec/models/event_spec.rb
+++ b/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)
--
GitLab