rework heatmap permissions (#14080)

* now uses the same permission model as for the activity feed:
  only include activities in repos, that the doer has access to.
  this might be somewhat slower.

* also improves handling of user.KeepActivityPrivate (still shows
  the heatmap to self & admins)

* extend tests

* adjust integration test to new behaviour

* add access to actions for admins

* extend heatmap unit tests

Author: noerw

integrations/privateactivity_test.go

@@ -388,7 +388,7 @@ func TestPrivateActivityYesHeatmapHasNoContentForUserItself(t *testing.T) {
 	session := loginUser(t, privateActivityTestUser)
 	hasContent := testPrivateActivityHelperHasHeatmapContentFromSession(t, session)
 
-	assert.False(t, hasContent, "user should have no heatmap content")
+	assert.True(t, hasContent, "user should see their own heatmap content")
 }
 
 func TestPrivateActivityYesHeatmapHasNoContentForOtherUser(t *testing.T) {
@@ -399,7 +399,7 @@ func TestPrivateActivityYesHeatmapHasNoContentForOtherUser(t *testing.T) {
 	session := loginUser(t, privateActivityTestOtherUser)
 	hasContent := testPrivateActivityHelperHasHeatmapContentFromSession(t, session)
 
-	assert.False(t, hasContent, "user should have no heatmap content")
+	assert.False(t, hasContent, "other user should not see heatmap content")
 }
 
 func TestPrivateActivityYesHeatmapHasNoContentForAdmin(t *testing.T) {
@@ -410,5 +410,5 @@ func TestPrivateActivityYesHeatmapHasNoContentForAdmin(t *testing.T) {
 	session := loginUser(t, privateActivityTestAdmin)
 	hasContent := testPrivateActivityHelperHasHeatmapContentFromSession(t, session)
 
-	assert.False(t, hasContent, "user should have no heatmap content")
+	assert.True(t, hasContent, "heatmap should show content for admin")
 }

models/action.go

@@ -298,32 +298,63 @@ type GetFeedsOptions struct {
 
 // GetFeeds returns actions according to the provided options
 func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {
-	cond := builder.NewCond()
+	if !activityReadable(opts.RequestedUser, opts.Actor) {
+		return make([]*Action, 0), nil
+	}
 
-	var repoIDs []int64
-	var actorID int64
+	cond, err := activityQueryCondition(opts)
+	if err != nil {
+		return nil, err
+	}
 
-	if opts.Actor != nil {
-		actorID = opts.Actor.ID
+	actions := make([]*Action, 0, setting.UI.FeedPagingNum)
+
+	if err := x.Limit(setting.UI.FeedPagingNum).Desc("id").Where(cond).Find(&actions); err != nil {
+		return nil, fmt.Errorf("Find: %v", err)
 	}
 
-	if opts.RequestedUser.IsOrganization() {
-		env, err := opts.RequestedUser.AccessibleReposEnv(actorID)
-		if err != nil {
-			return nil, fmt.Errorf("AccessibleReposEnv: %v", err)
-		}
-		if repoIDs, err = env.RepoIDs(1, opts.RequestedUser.NumRepos); err != nil {
-			return nil, fmt.Errorf("GetUserRepositories: %v", err)
+	if err := ActionList(actions).LoadAttributes(); err != nil {
+		return nil, fmt.Errorf("LoadAttributes: %v", err)
+	}
+
+	return actions, nil
+}
+
+func activityReadable(user *User, doer *User) bool {
+	var doerID int64
+	if doer != nil {
+		doerID = doer.ID
+	}
+	if doer == nil || !doer.IsAdmin {
+		if user.KeepActivityPrivate && doerID != user.ID {
+			return false
 		}
+	}
+	return true
+}
 
-		cond = cond.And(builder.In("repo_id", repoIDs))
-	} else {
-		cond = cond.And(builder.In("repo_id", AccessibleRepoIDsQuery(opts.Actor)))
+func activityQueryCondition(opts GetFeedsOptions) (builder.Cond, error) {
+	cond := builder.NewCond()
+
+	var repoIDs []int64
+	var actorID int64
+	if opts.Actor != nil {
+		actorID = opts.Actor.ID
 	}
 
+	// check readable repositories by doer/actor
 	if opts.Actor == nil || !opts.Actor.IsAdmin {
-		if opts.RequestedUser.KeepActivityPrivate && actorID != opts.RequestedUser.ID {
-			return make([]*Action, 0), nil
+		if opts.RequestedUser.IsOrganization() {
+			env, err := opts.RequestedUser.AccessibleReposEnv(actorID)
+			if err != nil {
+				return nil, fmt.Errorf("AccessibleReposEnv: %v", err)
+			}
+			if repoIDs, err = env.RepoIDs(1, opts.RequestedUser.NumRepos); err != nil {
+				return nil, fmt.Errorf("GetUserRepositories: %v", err)
+			}
+			cond = cond.And(builder.In("repo_id", repoIDs))
+		} else {
+			cond = cond.And(builder.In("repo_id", AccessibleRepoIDsQuery(opts.Actor)))
 		}
 	}
 
@@ -335,20 +366,9 @@ func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {
 	if !opts.IncludePrivate {
 		cond = cond.And(builder.Eq{"is_private": false})
 	}
-
 	if !opts.IncludeDeleted {
 		cond = cond.And(builder.Eq{"is_deleted": false})
 	}
 
-	actions := make([]*Action, 0, setting.UI.FeedPagingNum)
-
-	if err := x.Limit(setting.UI.FeedPagingNum).Desc("id").Where(cond).Find(&actions); err != nil {
-		return nil, fmt.Errorf("Find: %v", err)
-	}
-
-	if err := ActionList(actions).LoadAttributes(); err != nil {
-		return nil, fmt.Errorf("LoadAttributes: %v", err)
-	}
-
-	return actions, nil
+	return cond, nil
 }

models/fixtures/action.yml

@@ -23,3 +23,12 @@
   act_user_id: 11
   repo_id: 9
   is_private: false
+
+-
+  id: 4
+  user_id: 16
+  op_type: 12 # close issue
+  act_user_id: 16
+  repo_id: 22
+  is_private: true
+  created_unix: 1603267920

models/user_heatmap.go

@@ -16,10 +16,10 @@ type UserHeatmapData struct {
 }
 
 // GetUserHeatmapDataByUser returns an array of UserHeatmapData
-func GetUserHeatmapDataByUser(user *User) ([]*UserHeatmapData, error) {
+func GetUserHeatmapDataByUser(user *User, doer *User) ([]*UserHeatmapData, error) {
 	hdata := make([]*UserHeatmapData, 0)
 
-	if user.KeepActivityPrivate {
+	if !activityReadable(user, doer) {
 		return hdata, nil
 	}
 
@@ -37,22 +37,26 @@ func GetUserHeatmapDataByUser(user *User) ([]*UserHeatmapData, error) {
 		groupByName = groupBy
 	}
 
-	sess := x.Select(groupBy+" AS timestamp, count(user_id) as contributions").
-		Table("action").
-		Where("user_id = ?", user.ID).
-		And("created_unix > ?", (timeutil.TimeStampNow() - 31536000))
-
-	// * Heatmaps for individual users only include actions that the user themself
-	//   did.
-	// * For organizations actions by all users that were made in owned
-	//   repositories are counted.
-	if user.Type == UserTypeIndividual {
-		sess = sess.And("act_user_id = ?", user.ID)
+	cond, err := activityQueryCondition(GetFeedsOptions{
+		RequestedUser:  user,
+		Actor:          doer,
+		IncludePrivate: true, // don't filter by private, as we already filter by repo access
+		IncludeDeleted: true,
+		// * Heatmaps for individual users only include actions that the user themself did.
+		// * For organizations actions by all users that were made in owned
+		//   repositories are counted.
+		OnlyPerformedBy: !user.IsOrganization(),
+	})
+	if err != nil {
+		return nil, err
 	}
 
-	err := sess.GroupBy(groupByName).
+	return hdata, x.
+		Select(groupBy+" AS timestamp, count(user_id) as contributions").
+		Table("action").
+		Where(cond).
+		And("created_unix > ?", (timeutil.TimeStampNow() - 31536000)).
+		GroupBy(groupByName).
 		OrderBy("timestamp").
 		Find(&hdata)
-
-	return hdata, err
 }

models/user_heatmap_test.go

@@ -6,6 +6,7 @@ package models
 
 import (
 	"encoding/json"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -14,35 +15,45 @@ import (
 func TestGetUserHeatmapDataByUser(t *testing.T) {
 	testCases := []struct {
 		userID      int64
+		doerID      int64
 		CountResult int
 		JSONResult  string
 	}{
-		{2, 1, `[{"timestamp":1603152000,"contributions":1}]`},
-		{3, 0, `[]`},
+		{2, 2, 1, `[{"timestamp":1603152000,"contributions":1}]`}, // self looks at action in private repo
+		{2, 1, 1, `[{"timestamp":1603152000,"contributions":1}]`}, // admin looks at action in private repo
+		{2, 3, 0, `[]`}, // other user looks at action in private repo
+		{2, 0, 0, `[]`}, // nobody looks at action in private repo
+		{16, 15, 1, `[{"timestamp":1603238400,"contributions":1}]`}, // collaborator looks at action in private repo
+		{3, 3, 0, `[]`}, // no action action not performed by target user
 	}
 	// Prepare
 	assert.NoError(t, PrepareTestDatabase())
 
-	for _, tc := range testCases {
-
-		// Insert some action
+	for i, tc := range testCases {
 		user := AssertExistsAndLoadBean(t, &User{ID: tc.userID}).(*User)
 
+		doer := &User{ID: tc.doerID}
+		_, err := loadBeanIfExists(doer)
+		assert.NoError(t, err)
+		if tc.doerID == 0 {
+			doer = nil
+		}
+
 		// get the action for comparison
 		actions, err := GetFeeds(GetFeedsOptions{
 			RequestedUser:   user,
-			Actor:           user,
+			Actor:           doer,
 			IncludePrivate:  true,
-			OnlyPerformedBy: false,
+			OnlyPerformedBy: true,
 			IncludeDeleted:  true,
 		})
 		assert.NoError(t, err)
 
 		// Get the heatmap and compare
-		heatmap, err := GetUserHeatmapDataByUser(user)
+		heatmap, err := GetUserHeatmapDataByUser(user, doer)
 		assert.NoError(t, err)
 		assert.Equal(t, len(actions), len(heatmap), "invalid action count: did the test data became too old?")
-		assert.Equal(t, tc.CountResult, len(heatmap))
+		assert.Equal(t, tc.CountResult, len(heatmap), fmt.Sprintf("testcase %d", i))
 
 		//Test JSON rendering
 		jsonData, err := json.Marshal(heatmap)

routers/api/v1/user/user.go

@@ -166,7 +166,7 @@ func GetUserHeatmapData(ctx *context.APIContext) {
 		return
 	}
 
-	heatmap, err := models.GetUserHeatmapDataByUser(user)
+	heatmap, err := models.GetUserHeatmapDataByUser(user, ctx.User)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "GetUserHeatmapDataByUser", err)
 		return

routers/user/home.go

@@ -115,7 +115,7 @@ func Dashboard(ctx *context.Context) {
 	// no heatmap access for admins; GetUserHeatmapDataByUser ignores the calling user
 	// so everyone would get the same empty heatmap
 	if setting.Service.EnableUserHeatmap && !ctxUser.KeepActivityPrivate {
-		data, err := models.GetUserHeatmapDataByUser(ctxUser)
+		data, err := models.GetUserHeatmapDataByUser(ctxUser, ctx.User)
 		if err != nil {
 			ctx.ServerError("GetUserHeatmapDataByUser", err)
 			return

routers/user/profile.go

@@ -98,7 +98,7 @@ func Profile(ctx *context.Context) {
 	// no heatmap access for admins; GetUserHeatmapDataByUser ignores the calling user
 	// so everyone would get the same empty heatmap
 	if setting.Service.EnableUserHeatmap && !ctxUser.KeepActivityPrivate {
-		data, err := models.GetUserHeatmapDataByUser(ctxUser)
+		data, err := models.GetUserHeatmapDataByUser(ctxUser, ctx.User)
 		if err != nil {
 			ctx.ServerError("GetUserHeatmapDataByUser", err)
 			return