Add a option "--user-type bot" to admin user create, improve role display (#27885)

Partially solve #13044

Fix #33295

---------

Co-authored-by: wxiaoguang <[email protected]>

Author: mscherer

cmd/admin_user_create.go

@@ -31,6 +31,11 @@ var microcmdUserCreate = &cli.Command{
 			Name:  "username",
 			Usage: "Username",
 		},
+		&cli.StringFlag{
+			Name:  "user-type",
+			Usage: "Set user's type: individual or bot",
+			Value: "individual",
+		},
 		&cli.StringFlag{
 			Name:  "password",
 			Usage: "User password",
@@ -77,6 +82,22 @@ func runCreateUser(c *cli.Context) error {
 		return err
 	}
 
+	userTypes := map[string]user_model.UserType{
+		"individual": user_model.UserTypeIndividual,
+		"bot":        user_model.UserTypeBot,
+	}
+	userType, ok := userTypes[c.String("user-type")]
+	if !ok {
+		return fmt.Errorf("invalid user type: %s", c.String("user-type"))
+	}
+	if userType != user_model.UserTypeIndividual {
+		// Some other commands like "change-password" also only support individual users.
+		// It needs to clarify the "password" behavior for bot users in the future.
+		// At the moment, we do not allow setting password for bot users.
+		if c.IsSet("password") || c.IsSet("random-password") {
+			return errors.New("password can only be set for individual users")
+		}
+	}
 	if c.IsSet("name") && c.IsSet("username") {
 		return errors.New("cannot set both --name and --username flags")
 	}
@@ -118,16 +139,19 @@ func runCreateUser(c *cli.Context) error {
 			return err
 		}
 		fmt.Printf("generated random password is '%s'\n", password)
-	} else {
+	} else if userType == user_model.UserTypeIndividual {
 		return errors.New("must set either password or random-password flag")
 	}
 
 	isAdmin := c.Bool("admin")
 	mustChangePassword := true // always default to true
 	if c.IsSet("must-change-password") {
+		if userType != user_model.UserTypeIndividual {
+			return errors.New("must-change-password flag can only be set for individual users")
+		}
 		// if the flag is set, use the value provided by the user
 		mustChangePassword = c.Bool("must-change-password")
-	} else {
+	} else if userType == user_model.UserTypeIndividual {
 		// check whether there are users in the database
 		hasUserRecord, err := db.IsTableNotEmpty(&user_model.User{})
 		if err != nil {
@@ -151,8 +175,9 @@ func runCreateUser(c *cli.Context) error {
 	u := &user_model.User{
 		Name:               username,
 		Email:              c.String("email"),
-		Passwd:             password,
 		IsAdmin:            isAdmin,
+		Type:               userType,
+		Passwd:             password,
 		MustChangePassword: mustChangePassword,
 		Visibility:         visibility,
 	}

cmd/admin_user_create_test.go

@@ -13,32 +13,54 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestAdminUserCreate(t *testing.T) {
 	app := NewMainApp(AppVersion{})
 
 	reset := func() {
-		assert.NoError(t, db.TruncateBeans(db.DefaultContext, &user_model.User{}))
-		assert.NoError(t, db.TruncateBeans(db.DefaultContext, &user_model.EmailAddress{}))
+		require.NoError(t, db.TruncateBeans(db.DefaultContext, &user_model.User{}))
+		require.NoError(t, db.TruncateBeans(db.DefaultContext, &user_model.EmailAddress{}))
 	}
 
-	type createCheck struct{ IsAdmin, MustChangePassword bool }
-	createUser := func(name, args string) createCheck {
-		assert.NoError(t, app.Run(strings.Fields(fmt.Sprintf("./gitea admin user create --username %s --email %[email protected] %s --password foobar", name, name, args))))
-		u := unittest.AssertExistsAndLoadBean(t, &user_model.User{LowerName: name})
-		return createCheck{u.IsAdmin, u.MustChangePassword}
-	}
-	reset()
-	assert.Equal(t, createCheck{IsAdmin: false, MustChangePassword: false}, createUser("u", ""), "first non-admin user doesn't need to change password")
-
-	reset()
-	assert.Equal(t, createCheck{IsAdmin: true, MustChangePassword: false}, createUser("u", "--admin"), "first admin user doesn't need to change password")
-
-	reset()
-	assert.Equal(t, createCheck{IsAdmin: true, MustChangePassword: true}, createUser("u", "--admin --must-change-password"))
-	assert.Equal(t, createCheck{IsAdmin: true, MustChangePassword: true}, createUser("u2", "--admin"))
-	assert.Equal(t, createCheck{IsAdmin: true, MustChangePassword: false}, createUser("u3", "--admin --must-change-password=false"))
-	assert.Equal(t, createCheck{IsAdmin: false, MustChangePassword: true}, createUser("u4", ""))
-	assert.Equal(t, createCheck{IsAdmin: false, MustChangePassword: false}, createUser("u5", "--must-change-password=false"))
+	t.Run("MustChangePassword", func(t *testing.T) {
+		type check struct {
+			IsAdmin            bool
+			MustChangePassword bool
+		}
+		createCheck := func(name, args string) check {
+			require.NoError(t, app.Run(strings.Fields(fmt.Sprintf("./gitea admin user create --username %s --email %[email protected] %s --password foobar", name, name, args))))
+			u := unittest.AssertExistsAndLoadBean(t, &user_model.User{LowerName: name})
+			return check{IsAdmin: u.IsAdmin, MustChangePassword: u.MustChangePassword}
+		}
+		reset()
+		assert.Equal(t, check{IsAdmin: false, MustChangePassword: false}, createCheck("u", ""), "first non-admin user doesn't need to change password")
+
+		reset()
+		assert.Equal(t, check{IsAdmin: true, MustChangePassword: false}, createCheck("u", "--admin"), "first admin user doesn't need to change password")
+
+		reset()
+		assert.Equal(t, check{IsAdmin: true, MustChangePassword: true}, createCheck("u", "--admin --must-change-password"))
+		assert.Equal(t, check{IsAdmin: true, MustChangePassword: true}, createCheck("u2", "--admin"))
+		assert.Equal(t, check{IsAdmin: true, MustChangePassword: false}, createCheck("u3", "--admin --must-change-password=false"))
+		assert.Equal(t, check{IsAdmin: false, MustChangePassword: true}, createCheck("u4", ""))
+		assert.Equal(t, check{IsAdmin: false, MustChangePassword: false}, createCheck("u5", "--must-change-password=false"))
+	})
+
+	t.Run("UserType", func(t *testing.T) {
+		createUser := func(name, args string) error {
+			return app.Run(strings.Fields(fmt.Sprintf("./gitea admin user create --username %s --email %[email protected] %s", name, name, args)))
+		}
+
+		reset()
+		assert.ErrorContains(t, createUser("u", "--user-type invalid"), "invalid user type")
+		assert.ErrorContains(t, createUser("u", "--user-type bot --password 123"), "can only be set for individual users")
+		assert.ErrorContains(t, createUser("u", "--user-type bot --must-change-password"), "can only be set for individual users")
+
+		assert.NoError(t, createUser("u", "--user-type bot"))
+		u := unittest.AssertExistsAndLoadBean(t, &user_model.User{LowerName: "u"})
+		assert.Equal(t, user_model.UserTypeBot, u.Type)
+		assert.Equal(t, "", u.Passwd)
+	})
 }

models/user/user.go

@@ -385,11 +385,12 @@ func (u *User) ValidatePassword(passwd string) bool {
 }
 
 // IsPasswordSet checks if the password is set or left empty
+// TODO: It's better to clarify the "password" behavior for different types (individual, bot)
 func (u *User) IsPasswordSet() bool {
-	return len(u.Passwd) != 0
+	return u.Passwd != ""
 }
 
-// IsOrganization returns true if user is actually a organization.
+// IsOrganization returns true if user is actually an organization.
 func (u *User) IsOrganization() bool {
 	return u.Type == UserTypeOrganization
 }
@@ -399,13 +400,14 @@ func (u *User) IsIndividual() bool {
 	return u.Type == UserTypeIndividual
 }
 
-func (u *User) IsUser() bool {
-	return u.Type == UserTypeIndividual || u.Type == UserTypeBot
+// IsTypeBot returns whether the user is of type bot
+func (u *User) IsTypeBot() bool {
+	return u.Type == UserTypeBot
 }
 
-// IsBot returns whether or not the user is of type bot
-func (u *User) IsBot() bool {
-	return u.Type == UserTypeBot
+// IsTokenAccessAllowed returns whether the user is an individual or a bot (which allows for token access)
+func (u *User) IsTokenAccessAllowed() bool {
+	return u.Type == UserTypeIndividual || u.Type == UserTypeBot
 }
 
 // DisplayName returns full name if it's not empty,

models/user/user_system.go

@@ -56,7 +56,7 @@ func NewActionsUser() *User {
 		Email:                   ActionsUserEmail,
 		KeepEmailPrivate:        true,
 		LoginName:               ActionsUserName,
-		Type:                    UserTypeIndividual,
+		Type:                    UserTypeBot,
 		AllowCreateOrganization: true,
 		Visibility:              structs.VisibleTypePublic,
 	}

routers/api/v1/api.go

@@ -268,12 +268,12 @@ func checkTokenPublicOnly() func(ctx *context.APIContext) {
 				return
 			}
 		case auth_model.ContainsCategory(requiredScopeCategories, auth_model.AccessTokenScopeCategoryUser):
-			if ctx.ContextUser != nil && ctx.ContextUser.IsUser() && ctx.ContextUser.Visibility != api.VisibleTypePublic {
+			if ctx.ContextUser != nil && ctx.ContextUser.IsTokenAccessAllowed() && ctx.ContextUser.Visibility != api.VisibleTypePublic {
 				ctx.Error(http.StatusForbidden, "reqToken", "token scope is limited to public users")
 				return
 			}
 		case auth_model.ContainsCategory(requiredScopeCategories, auth_model.AccessTokenScopeCategoryActivityPub):
-			if ctx.ContextUser != nil && ctx.ContextUser.IsUser() && ctx.ContextUser.Visibility != api.VisibleTypePublic {
+			if ctx.ContextUser != nil && ctx.ContextUser.IsTokenAccessAllowed() && ctx.ContextUser.Visibility != api.VisibleTypePublic {
 				ctx.Error(http.StatusForbidden, "reqToken", "token scope is limited to public activitypub")
 				return
 			}

routers/web/repo/issue_view.go

@@ -4,7 +4,6 @@
 package repo
 
 import (
-	stdCtx "context"
 	"fmt"
 	"math/big"
 	"net/http"
@@ -40,86 +39,80 @@ import (
 )
 
 // roleDescriptor returns the role descriptor for a comment in/with the given repo, poster and issue
-func roleDescriptor(ctx stdCtx.Context, repo *repo_model.Repository, poster *user_model.User, permsCache map[int64]access_model.Permission, issue *issues_model.Issue, hasOriginalAuthor bool) (issues_model.RoleDescriptor, error) {
-	roleDescriptor := issues_model.RoleDescriptor{}
-
+func roleDescriptor(ctx *context.Context, repo *repo_model.Repository, poster *user_model.User, permsCache map[int64]access_model.Permission, issue *issues_model.Issue, hasOriginalAuthor bool) (roleDesc issues_model.RoleDescriptor, err error) {
 	if hasOriginalAuthor {
-		return roleDescriptor, nil
+		// the poster is a migrated user, so no need to detect the role
+		return roleDesc, nil
 	}
 
-	var perm access_model.Permission
-	var err error
-	if permsCache != nil {
-		var ok bool
-		perm, ok = permsCache[poster.ID]
-		if !ok {
-			perm, err = access_model.GetUserRepoPermission(ctx, repo, poster)
-			if err != nil {
-				return roleDescriptor, err
-			}
-		}
-		permsCache[poster.ID] = perm
-	} else {
+	if poster.IsGhost() || !poster.IsIndividual() {
+		return roleDesc, nil
+	}
+
+	roleDesc.IsPoster = issue.IsPoster(poster.ID) // check whether the comment's poster is the issue's poster
+
+	// Guess the role of the poster in the repo by permission
+	perm, hasPermCache := permsCache[poster.ID]
+	if !hasPermCache {
 		perm, err = access_model.GetUserRepoPermission(ctx, repo, poster)
 		if err != nil {
-			return roleDescriptor, err
+			return roleDesc, err
 		}
 	}
-
-	// If the poster is the actual poster of the issue, enable Poster role.
-	roleDescriptor.IsPoster = issue.IsPoster(poster.ID)
+	if permsCache != nil {
+		permsCache[poster.ID] = perm
+	}
 
 	// Check if the poster is owner of the repo.
 	if perm.IsOwner() {
-		// If the poster isn't an admin, enable the owner role.
+		// If the poster isn't a site admin, then is must be the repo's owner
 		if !poster.IsAdmin {
-			roleDescriptor.RoleInRepo = issues_model.RoleRepoOwner
-			return roleDescriptor, nil
+			roleDesc.RoleInRepo = issues_model.RoleRepoOwner
+			return roleDesc, nil
 		}
-
-		// Otherwise check if poster is the real repo admin.
-		ok, err := access_model.IsUserRealRepoAdmin(ctx, repo, poster)
+		// Otherwise (poster is site admin), check if poster is the real repo admin.
+		isRealRepoAdmin, err := access_model.IsUserRealRepoAdmin(ctx, repo, poster)
 		if err != nil {
-			return roleDescriptor, err
+			return roleDesc, err
 		}
-		if ok {
-			roleDescriptor.RoleInRepo = issues_model.RoleRepoOwner
-			return roleDescriptor, nil
+		if isRealRepoAdmin {
+			roleDesc.RoleInRepo = issues_model.RoleRepoOwner
+			return roleDesc, nil
 		}
 	}
 
 	// If repo is organization, check Member role
-	if err := repo.LoadOwner(ctx); err != nil {
-		return roleDescriptor, err
+	if err = repo.LoadOwner(ctx); err != nil {
+		return roleDesc, err
 	}
 	if repo.Owner.IsOrganization() {
 		if isMember, err := organization.IsOrganizationMember(ctx, repo.Owner.ID, poster.ID); err != nil {
-			return roleDescriptor, err
+			return roleDesc, err
 		} else if isMember {
-			roleDescriptor.RoleInRepo = issues_model.RoleRepoMember
-			return roleDescriptor, nil
+			roleDesc.RoleInRepo = issues_model.RoleRepoMember
+			return roleDesc, nil
 		}
 	}
 
 	// If the poster is the collaborator of the repo
 	if isCollaborator, err := repo_model.IsCollaborator(ctx, repo.ID, poster.ID); err != nil {
-		return roleDescriptor, err
+		return roleDesc, err
 	} else if isCollaborator {
-		roleDescriptor.RoleInRepo = issues_model.RoleRepoCollaborator
-		return roleDescriptor, nil
+		roleDesc.RoleInRepo = issues_model.RoleRepoCollaborator
+		return roleDesc, nil
 	}
 
 	hasMergedPR, err := issues_model.HasMergedPullRequestInRepo(ctx, repo.ID, poster.ID)
 	if err != nil {
-		return roleDescriptor, err
+		return roleDesc, err
 	} else if hasMergedPR {
-		roleDescriptor.RoleInRepo = issues_model.RoleRepoContributor
+		roleDesc.RoleInRepo = issues_model.RoleRepoContributor
 	} else if issue.IsPull {
 		// only display first time contributor in the first opening pull request
-		roleDescriptor.RoleInRepo = issues_model.RoleRepoFirstTimeContributor
+		roleDesc.RoleInRepo = issues_model.RoleRepoFirstTimeContributor
 	}
 
-	return roleDescriptor, nil
+	return roleDesc, nil
 }
 
 func getBranchData(ctx *context.Context, issue *issues_model.Issue) {

templates/shared/user/authorlink.tmpl

@@ -1 +1 @@
-<a class="author text black tw-font-semibold muted"{{if gt .ID 0}} href="{{.HomeLink}}"{{end}}>{{.GetDisplayName}}</a>{{if .IsBot}}<span class="ui basic label tw-p-1 tw-align-baseline">bot</span>{{end}}
+<a class="author text black tw-font-semibold muted"{{if gt .ID 0}} href="{{.HomeLink}}"{{end}}>{{.GetDisplayName}}</a>{{if .IsTypeBot}}<span class="ui basic label tw-p-1 tw-align-baseline">bot</span>{{end}}