From 30b13942f09e8d92a4ff2e3a5feb3123941bbb47 Mon Sep 17 00:00:00 2001 From: Job Date: Sat, 15 Mar 2025 18:49:06 +0100 Subject: [PATCH] Give organisation members access to organisation feeds (#33508) Currently the organisation feed only includes items for public repositories (for non-administrators). This pull requests adds notifications from private repositories to the organisation-feed (for accounts that have access to the organisation). Feed-items only get shown for repositories where the users team(s) should have access to, this filtering seems to get done by some existing code. Needs some tests, but am unsure where/how to add them. Before: ![image](https://github.com/user-attachments/assets/8b63f430-227a-4b19-ad1a-f6f5175de301) After: ![image](https://github.com/user-attachments/assets/b439ce0e-4946-421c-a399-421806c7a6d8) --------- Co-authored-by: wxiaoguang --- models/unittest/fixtures_loader.go | 82 +++++++++++++-------- routers/web/feed/profile.go | 14 +++- routers/web/feed/profile_test.go | 38 ++++++++++ tests/integration/actions_job_test.go | 17 +---- tests/integration/actions_log_test.go | 7 +- tests/integration/api_repo_file_get_test.go | 2 - 6 files changed, 110 insertions(+), 50 deletions(-) create mode 100644 routers/web/feed/profile_test.go diff --git a/models/unittest/fixtures_loader.go b/models/unittest/fixtures_loader.go index 14686caf63..674f5cbe54 100644 --- a/models/unittest/fixtures_loader.go +++ b/models/unittest/fixtures_loader.go @@ -12,13 +12,17 @@ import ( "slices" "strings" + "code.gitea.io/gitea/models/db" + "gopkg.in/yaml.v3" "xorm.io/xorm" "xorm.io/xorm/schemas" ) -type fixtureItem struct { - tableName string +type FixtureItem struct { + fileFullPath string + tableName string + tableNameQuoted string sqlInserts []string sqlInsertArgs [][]any @@ -27,10 +31,11 @@ type fixtureItem struct { } type fixturesLoaderInternal struct { + xormEngine *xorm.Engine + xormTableNames map[string]bool db *sql.DB dbType schemas.DBType - files []string - fixtures map[string]*fixtureItem + fixtures map[string]*FixtureItem quoteObject func(string) string paramPlaceholder func(idx int) string } @@ -59,29 +64,27 @@ func (f *fixturesLoaderInternal) preprocessFixtureRow(row []map[string]any) (err return nil } -func (f *fixturesLoaderInternal) prepareFixtureItem(file string) (_ *fixtureItem, err error) { - fixture := &fixtureItem{} - fixture.tableName, _, _ = strings.Cut(filepath.Base(file), ".") +func (f *fixturesLoaderInternal) prepareFixtureItem(fixture *FixtureItem) (err error) { fixture.tableNameQuoted = f.quoteObject(fixture.tableName) if f.dbType == schemas.MSSQL { fixture.mssqlHasIdentityColumn, err = f.mssqlTableHasIdentityColumn(f.db, fixture.tableName) if err != nil { - return nil, err + return err } } - data, err := os.ReadFile(file) + data, err := os.ReadFile(fixture.fileFullPath) if err != nil { - return nil, fmt.Errorf("failed to read file %q: %w", file, err) + return fmt.Errorf("failed to read file %q: %w", fixture.fileFullPath, err) } var rows []map[string]any if err = yaml.Unmarshal(data, &rows); err != nil { - return nil, fmt.Errorf("failed to unmarshal yaml data from %q: %w", file, err) + return fmt.Errorf("failed to unmarshal yaml data from %q: %w", fixture.fileFullPath, err) } if err = f.preprocessFixtureRow(rows); err != nil { - return nil, fmt.Errorf("failed to preprocess fixture rows from %q: %w", file, err) + return fmt.Errorf("failed to preprocess fixture rows from %q: %w", fixture.fileFullPath, err) } var sqlBuf []byte @@ -107,16 +110,14 @@ func (f *fixturesLoaderInternal) prepareFixtureItem(file string) (_ *fixtureItem sqlBuf = sqlBuf[:0] sqlArguments = sqlArguments[:0] } - return fixture, nil + return nil } -func (f *fixturesLoaderInternal) loadFixtures(tx *sql.Tx, file string) (err error) { - fixture := f.fixtures[file] - if fixture == nil { - if fixture, err = f.prepareFixtureItem(file); err != nil { +func (f *fixturesLoaderInternal) loadFixtures(tx *sql.Tx, fixture *FixtureItem) (err error) { + if fixture.tableNameQuoted == "" { + if err = f.prepareFixtureItem(fixture); err != nil { return err } - f.fixtures[file] = fixture } _, err = tx.Exec(fmt.Sprintf("DELETE FROM %s", fixture.tableNameQuoted)) // sqlite3 doesn't support truncate @@ -147,15 +148,26 @@ func (f *fixturesLoaderInternal) Load() error { } defer func() { _ = tx.Rollback() }() - for _, file := range f.files { - if err := f.loadFixtures(tx, file); err != nil { - return fmt.Errorf("failed to load fixtures from %s: %w", file, err) + for _, fixture := range f.fixtures { + if !f.xormTableNames[fixture.tableName] { + continue + } + if err := f.loadFixtures(tx, fixture); err != nil { + return fmt.Errorf("failed to load fixtures from %s: %w", fixture.fileFullPath, err) } } - return tx.Commit() + if err = tx.Commit(); err != nil { + return err + } + for xormTableName := range f.xormTableNames { + if f.fixtures[xormTableName] == nil { + _, _ = f.xormEngine.Exec("DELETE FROM `" + xormTableName + "`") + } + } + return nil } -func FixturesFileFullPaths(dir string, files []string) ([]string, error) { +func FixturesFileFullPaths(dir string, files []string) (map[string]*FixtureItem, error) { if files != nil && len(files) == 0 { return nil, nil // load nothing } @@ -169,20 +181,25 @@ func FixturesFileFullPaths(dir string, files []string) ([]string, error) { files = append(files, e.Name()) } } - for i, file := range files { - if !filepath.IsAbs(file) { - files[i] = filepath.Join(dir, file) + fixtureItems := map[string]*FixtureItem{} + for _, file := range files { + fileFillPath := file + if !filepath.IsAbs(fileFillPath) { + fileFillPath = filepath.Join(dir, file) } + tableName, _, _ := strings.Cut(filepath.Base(file), ".") + fixtureItems[tableName] = &FixtureItem{fileFullPath: fileFillPath, tableName: tableName} } - return files, nil + return fixtureItems, nil } func NewFixturesLoader(x *xorm.Engine, opts FixturesOptions) (FixturesLoader, error) { - files, err := FixturesFileFullPaths(opts.Dir, opts.Files) + fixtureItems, err := FixturesFileFullPaths(opts.Dir, opts.Files) if err != nil { return nil, fmt.Errorf("failed to get fixtures files: %w", err) } - f := &fixturesLoaderInternal{db: x.DB().DB, dbType: x.Dialect().URI().DBType, files: files, fixtures: map[string]*fixtureItem{}} + + f := &fixturesLoaderInternal{xormEngine: x, db: x.DB().DB, dbType: x.Dialect().URI().DBType, fixtures: fixtureItems} switch f.dbType { case schemas.SQLITE: f.quoteObject = func(s string) string { return fmt.Sprintf(`"%s"`, s) } @@ -197,5 +214,12 @@ func NewFixturesLoader(x *xorm.Engine, opts FixturesOptions) (FixturesLoader, er f.quoteObject = func(s string) string { return fmt.Sprintf("[%s]", s) } f.paramPlaceholder = func(idx int) string { return "?" } } + + xormBeans, _ := db.NamesToBean() + f.xormTableNames = map[string]bool{} + for _, bean := range xormBeans { + f.xormTableNames[db.TableName(bean)] = true + } + return f, nil } diff --git a/routers/web/feed/profile.go b/routers/web/feed/profile.go index 4ec46e302a..cecf9d409a 100644 --- a/routers/web/feed/profile.go +++ b/routers/web/feed/profile.go @@ -7,6 +7,7 @@ import ( "time" activities_model "code.gitea.io/gitea/models/activities" + "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/renderhelper" "code.gitea.io/gitea/modules/markup/markdown" "code.gitea.io/gitea/services/context" @@ -28,12 +29,23 @@ func ShowUserFeedAtom(ctx *context.Context) { // showUserFeed show user activity as RSS / Atom feed func showUserFeed(ctx *context.Context, formatType string) { includePrivate := ctx.IsSigned && (ctx.Doer.IsAdmin || ctx.Doer.ID == ctx.ContextUser.ID) + isOrganisation := ctx.ContextUser.IsOrganization() + if ctx.IsSigned && isOrganisation && !includePrivate { + // When feed is requested by a member of the organization, + // include the private repo's the member has access to. + isOrgMember, err := organization.IsOrganizationMember(ctx, ctx.ContextUser.ID, ctx.Doer.ID) + if err != nil { + ctx.ServerError("IsOrganizationMember", err) + return + } + includePrivate = isOrgMember + } actions, _, err := feed_service.GetFeeds(ctx, activities_model.GetFeedsOptions{ RequestedUser: ctx.ContextUser, Actor: ctx.Doer, IncludePrivate: includePrivate, - OnlyPerformedBy: !ctx.ContextUser.IsOrganization(), + OnlyPerformedBy: !isOrganisation, IncludeDeleted: false, Date: ctx.FormString("date"), }) diff --git a/routers/web/feed/profile_test.go b/routers/web/feed/profile_test.go new file mode 100644 index 0000000000..a0f1509269 --- /dev/null +++ b/routers/web/feed/profile_test.go @@ -0,0 +1,38 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT +package feed_test + +import ( + "testing" + + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/routers/web/feed" + "code.gitea.io/gitea/services/contexttest" + + "github.com/stretchr/testify/assert" +) + +func TestMain(m *testing.M) { + unittest.MainTest(m) +} + +func TestCheckGetOrgFeedsAsOrgMember(t *testing.T) { + unittest.PrepareTestEnv(t) + t.Run("OrgMember", func(t *testing.T) { + ctx, resp := contexttest.MockContext(t, "org3.atom") + ctx.ContextUser = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + contexttest.LoadUser(t, ctx, 2) + ctx.IsSigned = true + feed.ShowUserFeedAtom(ctx) + assert.Contains(t, resp.Body.String(), "") // Should contain 1 private entry + }) + t.Run("NonOrgMember", func(t *testing.T) { + ctx, resp := contexttest.MockContext(t, "org3.atom") + ctx.ContextUser = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + contexttest.LoadUser(t, ctx, 5) + ctx.IsSigned = true + feed.ShowUserFeedAtom(ctx) + assert.NotContains(t, resp.Body.String(), "") // Should not contain any entries + }) +} diff --git a/tests/integration/actions_job_test.go b/tests/integration/actions_job_test.go index caab215cee..89c93e7a75 100644 --- a/tests/integration/actions_job_test.go +++ b/tests/integration/actions_job_test.go @@ -166,9 +166,6 @@ jobs: } }) } - - httpContext := NewAPITestContext(t, user2.Name, apiRepo.Name, auth_model.AccessTokenScopeWriteRepository) - doAPIDeleteRepository(httpContext)(t) }) } @@ -348,9 +345,6 @@ jobs: } }) } - - httpContext := NewAPITestContext(t, user2.Name, apiRepo.Name, auth_model.AccessTokenScopeWriteRepository) - doAPIDeleteRepository(httpContext)(t) }) } @@ -434,8 +428,6 @@ jobs: assert.Equal(t, setting.Actions.DefaultActionsURL.URL(), gtCtx["gitea_default_actions_url"].GetStringValue()) token := gtCtx["token"].GetStringValue() assert.Equal(t, actionTask.TokenLastEight, token[len(token)-8:]) - - doAPIDeleteRepository(user2APICtx)(t) }) } @@ -543,12 +535,14 @@ jobs: err = actions_service.CleanupEphemeralRunners(t.Context()) assert.NoError(t, err) - runner.client.runnerServiceClient.UpdateTask(t.Context(), connect.NewRequest(&runnerv1.UpdateTaskRequest{ + _, err = runner.client.runnerServiceClient.UpdateTask(t.Context(), connect.NewRequest(&runnerv1.UpdateTaskRequest{ State: &runnerv1.TaskState{ Id: actionTask.ID, Result: runnerv1.Result_RESULT_SUCCESS, }, })) + assert.NoError(t, err) + resp, err = runner.client.runnerServiceClient.FetchTask(t.Context(), connect.NewRequest(&runnerv1.FetchTaskRequest{ TasksVersion: 0, })) @@ -561,7 +555,7 @@ jobs: assert.Error(t, err) assert.Nil(t, resp) - // create an runner that picks a job and get force cancelled + // create a runner that picks a job and get force cancelled runnerToBeRemoved := newMockRunner() runnerToBeRemoved.registerAsRepoRunner(t, baseRepo.OwnerName, baseRepo.Name, "mock-runner-to-be-removed", []string{"ubuntu-latest"}, true) @@ -583,9 +577,6 @@ jobs: assert.NoError(t, err) unittest.AssertNotExistsBean(t, &actions_model.ActionRunner{ID: runnerToRemove.ID}) - - // this cleanup is required to allow further tests to pass - doAPIDeleteRepository(user2APICtx)(t) }) } diff --git a/tests/integration/actions_log_test.go b/tests/integration/actions_log_test.go index 8f10226721..5e99e5026d 100644 --- a/tests/integration/actions_log_test.go +++ b/tests/integration/actions_log_test.go @@ -35,7 +35,7 @@ func TestDownloadTaskLogs(t *testing.T) { { treePath: ".gitea/workflows/download-task-logs-zstd.yml", fileContent: `name: download-task-logs-zstd -on: +on: push: paths: - '.gitea/workflows/download-task-logs-zstd.yml' @@ -67,7 +67,7 @@ jobs: { treePath: ".gitea/workflows/download-task-logs-no-zstd.yml", fileContent: `name: download-task-logs-no-zstd -on: +on: push: paths: - '.gitea/workflows/download-task-logs-no-zstd.yml' @@ -152,8 +152,5 @@ jobs: resetFunc() }) } - - httpContext := NewAPITestContext(t, user2.Name, repo.Name, auth_model.AccessTokenScopeWriteRepository) - doAPIDeleteRepository(httpContext)(t) }) } diff --git a/tests/integration/api_repo_file_get_test.go b/tests/integration/api_repo_file_get_test.go index 2f897093ee..379851b689 100644 --- a/tests/integration/api_repo_file_get_test.go +++ b/tests/integration/api_repo_file_get_test.go @@ -44,8 +44,6 @@ func TestAPIGetRawFileOrLFS(t *testing.T) { reqLFS := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/media/"+lfs) respLFS := MakeRequestNilResponseRecorder(t, reqLFS, http.StatusOK) assert.Equal(t, testFileSizeSmall, respLFS.Length) - - doAPIDeleteRepository(httpContext) }) }) }