From f6bec8529697bdb89ebcd0901ba093f06aa9ac46 Mon Sep 17 00:00:00 2001 From: Norwin Date: Tue, 22 Dec 2020 02:53:37 +0000 Subject: [PATCH] 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 --- integrations/privateactivity_test.go | 6 +-- models/action.go | 78 +++++++++++++++++----------- models/fixtures/action.yml | 9 ++++ models/user_heatmap.go | 36 +++++++------ models/user_heatmap_test.go | 29 +++++++---- routers/api/v1/user/user.go | 2 +- routers/user/home.go | 2 +- routers/user/profile.go | 2 +- 8 files changed, 104 insertions(+), 60 deletions(-) diff --git a/integrations/privateactivity_test.go b/integrations/privateactivity_test.go index bfdc2ef53..381cb6b33 100644 --- a/integrations/privateactivity_test.go +++ b/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") } diff --git a/models/action.go b/models/action.go index 554640924..c39fdc397 100644 --- a/models/action.go +++ b/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 } diff --git a/models/fixtures/action.yml b/models/fixtures/action.yml index eb92aeedb..14cfd9042 100644 --- a/models/fixtures/action.yml +++ b/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 diff --git a/models/user_heatmap.go b/models/user_heatmap.go index ce3ec029c..425817e6d 100644 --- a/models/user_heatmap.go +++ b/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 } diff --git a/models/user_heatmap_test.go b/models/user_heatmap_test.go index c9d33db29..d98c4c63e 100644 --- a/models/user_heatmap_test.go +++ b/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) diff --git a/routers/api/v1/user/user.go b/routers/api/v1/user/user.go index b552c1353..07d5e9112 100644 --- a/routers/api/v1/user/user.go +++ b/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 diff --git a/routers/user/home.go b/routers/user/home.go index 46532f82b..92a913847 100644 --- a/routers/user/home.go +++ b/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 diff --git a/routers/user/profile.go b/routers/user/profile.go index 36f3d0735..bd5b35927 100644 --- a/routers/user/profile.go +++ b/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