Merge remote-tracking branch 'giteaofficial/main'

* giteaofficial/main:
  Prevent 500 when there is an error during new auth source post (go-gitea#19041)
  Update the webauthn_credential_id_sequence in Postgres (go-gitea#19048)
  If rendering has failed due to a net.OpError stop rendering (attempt 2) (go-gitea#19049)
  use xorm builder for models.getReviewers() (go-gitea#19033)
  RSS/Atom support for Orgs (go-gitea#17714)
  Fix flag validation (go-gitea#19046)
  Improve SyncMirrors logging (go-gitea#19045)

cmd/cmd.go

@@ -31,7 +31,7 @@ func argsSet(c *cli.Context, args ...string) error {
 			return errors.New(a + " is not set")
 		}
 
-		if util.IsEmptyString(a) {
+		if util.IsEmptyString(c.String(a)) {
 			return errors.New(a + " is required")
 		}
 	}

docs/content/doc/developers/guidelines-backend.md

@@ -42,7 +42,7 @@ To maintain understandable code and avoid circular dependencies it is important
   - `modules/setting`: Store all system configurations read from ini files and has been referenced by everywhere. But they should be used as function parameters when possible.
   - `modules/git`: Package to interactive with `Git` command line or Gogit package.
 - `public`: Compiled frontend files (javascript, images, css, etc.)
-- `routers`: Handling of server requests. As it uses other Gitea packages to serve the request, other packages (models, modules or services) shall not depend on routers.
+- `routers`: Handling of server requests. As it uses other Gitea packages to serve the request, other packages (models, modules or services) must not depend on routers.
   - `routers/api` Contains routers for `/api/v1` aims to handle RESTful API requests. 
   - `routers/install` Could only respond when system is in INSTALL mode (INSTALL_LOCK=false). 
   - `routers/private` will only be invoked by internal sub commands, especially `serv` and `hooks`. 
@@ -106,10 +106,20 @@ i.e. `servcies/user`, `models/repository`.
 Since there are some packages which use the same package name, it is possible that you find packages like `modules/user`, `models/user`, and `services/user`. When these packages are imported in one Go file, it's difficult to know which package we are using and if it's a variable name or an import name. So, we always recommend to use import aliases. To differ from package variables which are commonly in camelCase, just use **snake_case** for import aliases.
 i.e. `import user_service "code.gitea.io/gitea/services/user"`
 
+### Important Gotchas
+
+- Never write `x.Update(exemplar)` without an explicit `WHERE` clause:
+  - This will cause all rows in the table to be updated with the non-zero values of the exemplar - including IDs.
+  - You should usually write `x.ID(id).Update(exemplar)`.
+- If during a migration you are inserting into a table using `x.Insert(exemplar)` where the ID is preset:
+  - You will need to ``SET IDENTITY_INSERT `table` ON`` for the MSSQL variant (the migration will fail otherwise)
+  - However, you will also need to update the id sequence for postgres - the migration will silently pass here but later insertions will fail:
+    ``SELECT setval('table_name_id_seq', COALESCE((SELECT MAX(id)+1 FROM `table_name`), 1), false)``
+
 ### Future Tasks
 
 Currently, we are creating some refactors to do the following things:
 
 - Correct that codes which doesn't follow the rules.
 - There are too many files in `models`, so we are moving some of them into a sub package `models/xxx`.
-- Some `modules` sub packages should be moved to `services` because they depends on `models`.
+- Some `modules` sub packages should be moved to `services` because they depend on `models`.

models/action.go

@@ -22,6 +22,7 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/util"
 
@@ -315,29 +316,36 @@ func (a *Action) GetIssueContent() string {
 
 // GetFeedsOptions options for retrieving feeds
 type GetFeedsOptions struct {
-	RequestedUser   *user_model.User // the user we want activity for
-	RequestedTeam   *Team            // the team we want activity for
-	Actor           *user_model.User // the user viewing the activity
-	IncludePrivate  bool             // include private actions
-	OnlyPerformedBy bool             // only actions performed by requested user
-	IncludeDeleted  bool             // include deleted actions
-	Date            string           // the day we want activity for: YYYY-MM-DD
+	db.ListOptions
+	RequestedUser   *user_model.User       // the user we want activity for
+	RequestedTeam   *Team                  // the team we want activity for
+	RequestedRepo   *repo_model.Repository // the repo we want activity for
+	Actor           *user_model.User       // the user viewing the activity
+	IncludePrivate  bool                   // include private actions
+	OnlyPerformedBy bool                   // only actions performed by requested user
+	IncludeDeleted  bool                   // include deleted actions
+	Date            string                 // the day we want activity for: YYYY-MM-DD
 }
 
 // GetFeeds returns actions according to the provided options
 func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {
-	if !activityReadable(opts.RequestedUser, opts.Actor) {
-		return make([]*Action, 0), nil
+	if opts.RequestedUser == nil && opts.RequestedTeam == nil && opts.RequestedRepo == nil {
+		return nil, fmt.Errorf("need at least one of these filters: RequestedUser, RequestedTeam, RequestedRepo")
 	}
 
 	cond, err := activityQueryCondition(opts)
 	if err != nil {
 		return nil, err
 	}
 
-	actions := make([]*Action, 0, setting.UI.FeedPagingNum)
+	sess := db.GetEngine(db.DefaultContext).Where(cond)
 
-	if err := db.GetEngine(db.DefaultContext).Limit(setting.UI.FeedPagingNum).Desc("created_unix").Where(cond).Find(&actions); err != nil {
+	opts.SetDefaultValues()
+	sess = db.SetSessionPagination(sess, &opts)
+
+	actions := make([]*Action, 0, opts.PageSize)
+
+	if err := sess.Desc("created_unix").Find(&actions); err != nil {
 		return nil, fmt.Errorf("Find: %v", err)
 	}
 
@@ -349,41 +357,44 @@ func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {
 }
 
 func activityReadable(user, doer *user_model.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
+	return !user.KeepActivityPrivate ||
+		doer != nil && (doer.IsAdmin || user.ID == doer.ID)
 }
 
 func activityQueryCondition(opts GetFeedsOptions) (builder.Cond, error) {
 	cond := builder.NewCond()
 
-	var repoIDs []int64
-	var actorID int64
-	if opts.Actor != nil {
-		actorID = opts.Actor.ID
+	if opts.RequestedTeam != nil && opts.RequestedUser == nil {
+		org, err := user_model.GetUserByID(opts.RequestedTeam.OrgID)
+		if err != nil {
+			return nil, err
+		}
+		opts.RequestedUser = org
+	}
+
+	// check activity visibility for actor ( similar to activityReadable() )
+	if opts.Actor == nil {
+		cond = cond.And(builder.In("act_user_id",
+			builder.Select("`user`.id").Where(
+				builder.Eq{"keep_activity_private": false, "visibility": structs.VisibleTypePublic},
+			).From("`user`"),
+		))
+	} else if !opts.Actor.IsAdmin {
+		cond = cond.And(builder.In("act_user_id",
+			builder.Select("`user`.id").Where(
+				builder.Eq{"keep_activity_private": false}.
+					And(builder.In("visibility", structs.VisibleTypePublic, structs.VisibleTypeLimited))).
+				Or(builder.Eq{"id": opts.Actor.ID}).From("`user`"),
+		))
 	}
 
 	// check readable repositories by doer/actor
 	if opts.Actor == nil || !opts.Actor.IsAdmin {
-		if opts.RequestedUser.IsOrganization() {
-			env, err := OrgFromUser(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)))
-		}
+		cond = cond.And(builder.In("repo_id", AccessibleRepoIDsQuery(opts.Actor)))
+	}
+
+	if opts.RequestedRepo != nil {
+		cond = cond.And(builder.Eq{"repo_id": opts.RequestedRepo.ID})
 	}
 
 	if opts.RequestedTeam != nil {
@@ -395,11 +406,14 @@ func activityQueryCondition(opts GetFeedsOptions) (builder.Cond, error) {
 		cond = cond.And(builder.In("repo_id", teamRepoIDs))
 	}
 
-	cond = cond.And(builder.Eq{"user_id": opts.RequestedUser.ID})
+	if opts.RequestedUser != nil {
+		cond = cond.And(builder.Eq{"user_id": opts.RequestedUser.ID})
 
-	if opts.OnlyPerformedBy {
-		cond = cond.And(builder.Eq{"act_user_id": opts.RequestedUser.ID})
+		if opts.OnlyPerformedBy {
+			cond = cond.And(builder.Eq{"act_user_id": opts.RequestedUser.ID})
+		}
 	}
+
 	if !opts.IncludePrivate {
 		cond = cond.And(builder.Eq{"is_private": false})
 	}

models/action_test.go

@@ -93,6 +93,46 @@ func TestGetFeeds2(t *testing.T) {
 	assert.Len(t, actions, 0)
 }
 
+func TestActivityReadable(t *testing.T) {
+	tt := []struct {
+		desc   string
+		user   *user_model.User
+		doer   *user_model.User
+		result bool
+	}{{
+		desc:   "user should see own activity",
+		user:   &user_model.User{ID: 1},
+		doer:   &user_model.User{ID: 1},
+		result: true,
+	}, {
+		desc:   "anon should see activity if public",
+		user:   &user_model.User{ID: 1},
+		result: true,
+	}, {
+		desc:   "anon should NOT see activity",
+		user:   &user_model.User{ID: 1, KeepActivityPrivate: true},
+		result: false,
+	}, {
+		desc:   "user should see own activity if private too",
+		user:   &user_model.User{ID: 1, KeepActivityPrivate: true},
+		doer:   &user_model.User{ID: 1},
+		result: true,
+	}, {
+		desc:   "other user should NOT see activity",
+		user:   &user_model.User{ID: 1, KeepActivityPrivate: true},
+		doer:   &user_model.User{ID: 2},
+		result: false,
+	}, {
+		desc:   "admin should see activity",
+		user:   &user_model.User{ID: 1, KeepActivityPrivate: true},
+		doer:   &user_model.User{ID: 2, IsAdmin: true},
+		result: true,
+	}}
+	for _, test := range tt {
+		assert.Equal(t, test.result, activityReadable(test.user, test.doer), test.desc)
+	}
+}
+
 func TestNotifyWatchers(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 

models/lfs.go

@@ -193,12 +193,13 @@ func LFSAutoAssociate(metas []*LFSMetaObject, user *user_model.User, repoID int6
 		// admin can associate any LFS object to any repository, and we do not care about errors (eg: duplicated unique key),
 		// even if error occurs, it won't hurt users and won't make things worse
 		for i := range metas {
+			p := lfs.Pointer{Oid: metas[i].Oid, Size: metas[i].Size}
 			_, err = sess.Insert(&LFSMetaObject{
-				Pointer:      lfs.Pointer{Oid: metas[i].Oid, Size: metas[i].Size},
+				Pointer:      p,
 				RepositoryID: repoID,
 			})
 			if err != nil {
-				log.Warn("failed to insert LFS meta object into database, err=%v", err)
+				log.Warn("failed to insert LFS meta object %-v for repo_id: %d into database, err=%v", p, repoID, err)
 			}
 		}
 	}

models/migrations/v210.go

@@ -174,5 +174,11 @@ func remigrateU2FCredentials(x *xorm.Engine) error {
 		regs = regs[:0]
 	}
 
+	if x.Dialect().URI().DBType == schemas.POSTGRES {
+		if _, err := x.Exec("SELECT setval('webauthn_credential_id_seq', COALESCE((SELECT MAX(id)+1 FROM `webauthn_credential`), 1), false)"); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }

models/repo.go

@@ -218,50 +218,44 @@ func getReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post
 		return nil, err
 	}
 
-	var users []*user_model.User
-	e := db.GetEngine(ctx)
+	cond := builder.And(builder.Neq{"`user`.id": posterID})
 
 	if repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate {
 		// This a private repository:
 		// Anyone who can read the repository is a requestable reviewer
-		if err := e.
-			SQL("SELECT * FROM `user` WHERE id in ("+
-				"SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id != ?"+ // private org repos
-				") ORDER BY name",
-				repo.ID, perm.AccessModeRead,
-				posterID).
-			Find(&users); err != nil {
-			return nil, err
-		}
+
+		cond = cond.And(builder.In("`user`.id",
+			builder.Select("user_id").From("access").Where(
+				builder.Eq{"repo_id": repo.ID}.
+					And(builder.Gte{"mode": perm.AccessModeRead}),
+			),
+		))
 
 		if repo.Owner.Type == user_model.UserTypeIndividual && repo.Owner.ID != posterID {
 			// as private *user* repos don't generate an entry in the `access` table,
 			// the owner of a private repo needs to be explicitly added.
-			users = append(users, repo.Owner)
+			cond = cond.Or(builder.Eq{"`user`.id": repo.Owner.ID})
 		}
 
-		return users, nil
-	}
-
-	// This is a "public" repository:
-	// Any user that has read access, is a watcher or organization member can be requested to review
-	if err := e.
-		SQL("SELECT * FROM `user` WHERE id IN ( "+
-			"SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? "+
-			"UNION "+
-			"SELECT user_id FROM `watch` WHERE repo_id = ? AND mode IN (?, ?) "+
-			"UNION "+
-			"SELECT uid AS user_id FROM `org_user` WHERE org_id = ? "+
-			") AND id != ? ORDER BY name",
-			repo.ID, perm.AccessModeRead,
-			repo.ID, repo_model.WatchModeNormal, repo_model.WatchModeAuto,
-			repo.OwnerID,
-			posterID).
-		Find(&users); err != nil {
-		return nil, err
-	}
-
-	return users, nil
+	} else {
+		// This is a "public" repository:
+		// Any user that has read access, is a watcher or organization member can be requested to review
+		cond = cond.And(builder.And(builder.In("`user`.id",
+			builder.Select("user_id").From("access").
+				Where(builder.Eq{"repo_id": repo.ID}.
+					And(builder.Gte{"mode": perm.AccessModeRead})),
+		).Or(builder.In("`user`.id",
+			builder.Select("user_id").From("watch").
+				Where(builder.Eq{"repo_id": repo.ID}.
+					And(builder.In("mode", repo_model.WatchModeNormal, repo_model.WatchModeAuto))),
+		).Or(builder.In("`user`.id",
+			builder.Select("uid").From("org_user").
+				Where(builder.Eq{"org_id": repo.OwnerID}),
+		)))))
+	}
+
+	users := make([]*user_model.User, 0, 8)
+	return users, db.GetEngine(ctx).Where(cond).OrderBy("name").Find(&users)
 }
 
 // GetReviewers get all users can be requested to review: