[chore] Admin CLI + new account creation refactoring (#2008)

* set maxPasswordLength to 72 bytes, rename validate function

* refactor NewSignup

* refactor admin account CLI commands

* refactor oidc create user

* refactor processor create

* tweak password change, check old != new password

internal/processing/account/create.go

@@ -26,61 +26,73 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/config"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
-	"github.com/superseriousbusiness/gotosocial/internal/log"
 	"github.com/superseriousbusiness/gotosocial/internal/messages"
 	"github.com/superseriousbusiness/gotosocial/internal/text"
 	"github.com/superseriousbusiness/oauth2/v4"
 )
 
-// Create processes the given form for creating a new account, returning an oauth token for that account if successful.
-func (p *Processor) Create(ctx context.Context, applicationToken oauth2.TokenInfo, application *gtsmodel.Application, form *apimodel.AccountCreateRequest) (*apimodel.Token, gtserror.WithCode) {
+// Create processes the given form for creating a new account,
+// returning an oauth token for that account if successful.
+//
+// Fields on the form should have already been validated by the
+// caller, before this function is called.
+func (p *Processor) Create(
+	ctx context.Context,
+	appToken oauth2.TokenInfo,
+	app *gtsmodel.Application,
+	form *apimodel.AccountCreateRequest,
+) (*apimodel.Token, gtserror.WithCode) {
 	emailAvailable, err := p.state.DB.IsEmailAvailable(ctx, form.Email)
 	if err != nil {
-		return nil, gtserror.NewErrorBadRequest(err)
+		err := fmt.Errorf("db error checking email availability: %w", err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 	if !emailAvailable {
-		return nil, gtserror.NewErrorConflict(fmt.Errorf("email address %s is not available", form.Email))
+		err := fmt.Errorf("email address %s is not available", form.Email)
+		return nil, gtserror.NewErrorConflict(err, err.Error())
 	}
 
 	usernameAvailable, err := p.state.DB.IsUsernameAvailable(ctx, form.Username)
 	if err != nil {
-		return nil, gtserror.NewErrorBadRequest(err)
+		err := fmt.Errorf("db error checking username availability: %w", err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 	if !usernameAvailable {
-		return nil, gtserror.NewErrorConflict(fmt.Errorf("username %s in use", form.Username))
+		err := fmt.Errorf("username %s is not available", form.Username)
+		return nil, gtserror.NewErrorConflict(err, err.Error())
 	}
 
-	reasonRequired := config.GetAccountsReasonRequired()
-	approvalRequired := config.GetAccountsApprovalRequired()
-
-	// don't store a reason if we don't require one
-	reason := form.Reason
-	if !reasonRequired {
-		reason = ""
+	// Only store reason if one is required.
+	var reason string
+	if config.GetAccountsReasonRequired() {
+		reason = form.Reason
 	}
 
-	log.Trace(ctx, "creating new username and account")
-	user, err := p.state.DB.NewSignup(ctx, form.Username, text.SanitizePlaintext(reason), approvalRequired, form.Email, form.Password, form.IP, form.Locale, application.ID, false, "", false)
+	user, err := p.state.DB.NewSignup(ctx, gtsmodel.NewSignup{
+		Username:    form.Username,
+		Email:       form.Email,
+		Password:    form.Password,
+		Reason:      text.SanitizePlaintext(reason),
+		PreApproved: !config.GetAccountsApprovalRequired(), // Mark as approved if no approval required.
+		SignUpIP:    form.IP,
+		Locale:      form.Locale,
+		AppID:       app.ID,
+	})
 	if err != nil {
-		return nil, gtserror.NewErrorInternalError(fmt.Errorf("error creating new signup in the database: %s", err))
+		err := fmt.Errorf("db error creating new signup: %w", err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	log.Tracef(ctx, "generating a token for user %s with account %s and application %s", user.ID, user.AccountID, application.ID)
-	accessToken, err := p.oauthServer.GenerateUserAccessToken(ctx, applicationToken, application.ClientSecret, user.ID)
+	// Generate access token *before* doing side effects; we
+	// don't want to process side effects if something borks.
+	accessToken, err := p.oauthServer.GenerateUserAccessToken(ctx, appToken, app.ClientSecret, user.ID)
 	if err != nil {
-		return nil, gtserror.NewErrorInternalError(fmt.Errorf("error creating new access token for user %s: %s", user.ID, err))
+		err := fmt.Errorf("error creating new access token for user %s: %w", user.ID, err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	if user.Account == nil {
-		a, err := p.state.DB.GetAccountByID(ctx, user.AccountID)
-		if err != nil {
-			return nil, gtserror.NewErrorInternalError(fmt.Errorf("error getting new account from the database: %s", err))
-		}
-		user.Account = a
-	}
-
-	// there are side effects for creating a new account (sending confirmation emails etc)
-	// so pass a message to the processor so that it can do it asynchronously
+	// There are side effects for creating a new account
+	// (confirmation emails etc), perform these async.
 	p.state.Workers.EnqueueClientAPI(ctx, messages.FromClientAPI{
 		APObjectType:   ap.ObjectProfile,
 		APActivityType: ap.ActivityCreate,

internal/processing/user/password.go

@@ -28,22 +28,41 @@ import (
 
 // PasswordChange processes a password change request for the given user.
 func (p *Processor) PasswordChange(ctx context.Context, user *gtsmodel.User, oldPassword string, newPassword string) gtserror.WithCode {
+	// Ensure provided oldPassword is the correct current password.
 	if err := bcrypt.CompareHashAndPassword([]byte(user.EncryptedPassword), []byte(oldPassword)); err != nil {
+		err := gtserror.Newf("%w", err)
 		return gtserror.NewErrorUnauthorized(err, "old password was incorrect")
 	}
 
-	if err := validate.NewPassword(newPassword); err != nil {
+	// Ensure new password is strong enough.
+	if err := validate.Password(newPassword); err != nil {
 		return gtserror.NewErrorBadRequest(err, err.Error())
 	}
 
-	newPasswordHash, err := bcrypt.GenerateFromPassword([]byte(newPassword), bcrypt.DefaultCost)
-	if err != nil {
-		return gtserror.NewErrorInternalError(err, "error hashing password")
+	// Ensure new password is different from old password.
+	if newPassword == oldPassword {
+		const help = "new password cannot be the same as previous password"
+		err := gtserror.New(help)
+		return gtserror.NewErrorBadRequest(err, help)
 	}
 
-	user.EncryptedPassword = string(newPasswordHash)
+	// Hash the new password.
+	encryptedPassword, err := bcrypt.GenerateFromPassword(
+		[]byte(newPassword),
+		bcrypt.DefaultCost,
+	)
+	if err != nil {
+		err := gtserror.Newf("%w", err)
+		return gtserror.NewErrorInternalError(err)
+	}
 
-	if err := p.state.DB.UpdateUser(ctx, user, "encrypted_password"); err != nil {
+	// Set new password on user.
+	user.EncryptedPassword = string(encryptedPassword)
+	if err := p.state.DB.UpdateUser(
+		ctx, user,
+		"encrypted_password",
+	); err != nil {
+		err := gtserror.Newf("db error updating user: %w", err)
 		return gtserror.NewErrorInternalError(err)
 	}
 

internal/processing/user/password_test.go

@@ -54,7 +54,7 @@ func (suite *ChangePasswordTestSuite) TestChangePasswordIncorrectOld() {
 	user := suite.testUsers["local_account_1"]
 
 	errWithCode := suite.user.PasswordChange(context.Background(), user, "ooooopsydoooopsy", "verygoodnewpassword")
-	suite.EqualError(errWithCode, "crypto/bcrypt: hashedPassword is not the hash of the given password")
+	suite.EqualError(errWithCode, "PasswordChange: crypto/bcrypt: hashedPassword is not the hash of the given password")
 	suite.Equal(http.StatusUnauthorized, errWithCode.Code())
 	suite.Equal("Unauthorized: old password was incorrect", errWithCode.Safe())