configstore: add more validations

All the validation must be done inside the configstore since it's the source of
truth.

The gateway could also do some validation to avoid bad requests to the
configstore when needed or when the logic resides outside the configstore (like
project setup or user registration)

internal/services/configstore/command/command.go

@@ -49,6 +49,9 @@ func (s *CommandHandler) CreateProjectGroup(ctx context.Context, projectGroup *t
 	if projectGroup.Name == "" {
 		return nil, util.NewErrBadRequest(errors.Errorf("project group name required"))
 	}
+	if !util.ValidateName(projectGroup.Name) {
+		return nil, util.NewErrBadRequest(errors.Errorf("invalid project group name %q", projectGroup.Name))
+	}
 	if projectGroup.Parent.ID == "" {
 		return nil, util.NewErrBadRequest(errors.Errorf("project group parent id required"))
 	}
@@ -58,7 +61,7 @@ func (s *CommandHandler) CreateProjectGroup(ctx context.Context, projectGroup *t
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		parentProjectGroup, err := s.readDB.GetProjectGroup(tx, projectGroup.Parent.ID)
 		if err != nil {
@@ -129,19 +132,39 @@ func (s *CommandHandler) CreateProject(ctx context.Context, project *types.Proje
 	if project.Name == "" {
 		return nil, util.NewErrBadRequest(errors.Errorf("project name required"))
 	}
+	if !util.ValidateName(project.Name) {
+		return nil, util.NewErrBadRequest(errors.Errorf("invalid project name %q", project.Name))
+	}
 	if project.Parent.ID == "" {
 		return nil, util.NewErrBadRequest(errors.Errorf("project parent id required"))
 	}
+	if project.Parent.Type != types.ConfigTypeProjectGroup {
+		return nil, util.NewErrBadRequest(errors.Errorf("invalid project parent type %q", project.Parent.Type))
+	}
 	if !types.IsValidVisibility(project.Visibility) {
 		return nil, util.NewErrBadRequest(errors.Errorf("invalid project visibility"))
 	}
 	if !types.IsValidRemoteRepositoryConfigType(project.RemoteRepositoryConfigType) {
 		return nil, util.NewErrBadRequest(errors.Errorf("invalid project remote repository config type %q", project.RemoteRepositoryConfigType))
 	}
+	if project.RemoteRepositoryConfigType == types.RemoteRepositoryConfigTypeRemoteSource {
+		if project.RemoteSourceID == "" {
+			return nil, util.NewErrBadRequest(errors.Errorf("empty remote source id"))
+		}
+		if project.LinkedAccountID == "" {
+			return nil, util.NewErrBadRequest(errors.Errorf("empty linked account id"))
+		}
+		if project.RepositoryID == "" {
+			return nil, util.NewErrBadRequest(errors.Errorf("empty remote repository id"))
+		}
+		if project.RepositoryPath == "" {
+			return nil, util.NewErrBadRequest(errors.Errorf("empty remote repository path"))
+		}
+	}
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		group, err := s.readDB.GetProjectGroup(tx, project.Parent.ID)
@@ -233,7 +256,7 @@ func (s *CommandHandler) DeleteProject(ctx context.Context, projectRef string) e
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 
@@ -282,6 +305,9 @@ func (s *CommandHandler) CreateUser(ctx context.Context, req *CreateUserRequest)
 	if req.UserName == "" {
 		return nil, util.NewErrBadRequest(errors.Errorf("user name required"))
 	}
+	if !util.ValidateName(req.UserName) {
+		return nil, util.NewErrBadRequest(errors.Errorf("invalid user name %q", req.UserName))
+	}
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 	// changegroup is the username (and in future the email) to ensure no
@@ -289,7 +315,7 @@ func (s *CommandHandler) CreateUser(ctx context.Context, req *CreateUserRequest)
 	cgNames := []string{util.EncodeSha256Hex("username-" + req.UserName)}
 	var rs *types.RemoteSource
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		cgt, err = s.readDB.GetChangeGroupsUpdateTokens(tx, cgNames)
@@ -391,7 +417,7 @@ func (s *CommandHandler) DeleteUser(ctx context.Context, userRef string) error {
 	var user *types.User
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 
@@ -441,7 +467,7 @@ func (s *CommandHandler) UpdateUser(ctx context.Context, req *UpdateUserRequest)
 	cgNames := []string{}
 	var user *types.User
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		user, err = s.readDB.GetUser(tx, req.UserRef)
@@ -524,7 +550,7 @@ func (s *CommandHandler) CreateUserLA(ctx context.Context, req *CreateUserLARequ
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		user, err = s.readDB.GetUser(tx, req.UserRef)
@@ -609,7 +635,7 @@ func (s *CommandHandler) DeleteUserLA(ctx context.Context, userRef, laID string)
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		user, err = s.readDB.GetUser(tx, userRef)
@@ -679,7 +705,7 @@ func (s *CommandHandler) UpdateUserLA(ctx context.Context, req *UpdateUserLARequ
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		user, err = s.readDB.GetUser(tx, req.UserRef)
@@ -753,7 +779,7 @@ func (s *CommandHandler) CreateUserToken(ctx context.Context, userRef, tokenName
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		user, err = s.readDB.GetUser(tx, userRef)
@@ -818,7 +844,7 @@ func (s *CommandHandler) DeleteUserToken(ctx context.Context, userRef, tokenName
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		user, err = s.readDB.GetUser(tx, userRef)
@@ -870,12 +896,41 @@ func (s *CommandHandler) CreateRemoteSource(ctx context.Context, remoteSource *t
 	if remoteSource.Name == "" {
 		return nil, util.NewErrBadRequest(errors.Errorf("remotesource name required"))
 	}
+	if !util.ValidateName(remoteSource.Name) {
+		return nil, util.NewErrBadRequest(errors.Errorf("invalid remotesource name %q", remoteSource.Name))
+	}
+
+	if remoteSource.Name == "" {
+		return nil, util.NewErrBadRequest(errors.Errorf("remotesource name required"))
+	}
+	if remoteSource.APIURL == "" {
+		return nil, util.NewErrBadRequest(errors.Errorf("remotesource api url required"))
+	}
+	if remoteSource.Type == "" {
+		return nil, util.NewErrBadRequest(errors.Errorf("remotesource type required"))
+	}
+	if remoteSource.AuthType == "" {
+		return nil, util.NewErrBadRequest(errors.Errorf("remotesource auth type required"))
+	}
+
+	// validate if the remote source type supports the required auth type
+	if !types.SourceSupportsAuthType(types.RemoteSourceType(remoteSource.Type), types.RemoteSourceAuthType(remoteSource.AuthType)) {
+		return nil, util.NewErrBadRequest(errors.Errorf("remotesource type %q doesn't support auth type %q", remoteSource.Type, remoteSource.AuthType))
+	}
+	if remoteSource.AuthType == types.RemoteSourceAuthTypeOauth2 {
+		if remoteSource.Oauth2ClientID == "" {
+			return nil, util.NewErrBadRequest(errors.Errorf("remotesource oauth2clientid required for auth type %q", types.RemoteSourceAuthTypeOauth2))
+		}
+		if remoteSource.Oauth2ClientSecret == "" {
+			return nil, util.NewErrBadRequest(errors.Errorf("remotesource oauth2clientsecret required for auth type %q", types.RemoteSourceAuthTypeOauth2))
+		}
+	}
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 	// changegroup is the remotesource name
 	cgNames := []string{util.EncodeSha256Hex("remotesourcename-" + remoteSource.Name)}
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		cgt, err = s.readDB.GetChangeGroupsUpdateTokens(tx, cgNames)
@@ -924,7 +979,7 @@ func (s *CommandHandler) DeleteRemoteSource(ctx context.Context, remoteSourceNam
 	// changegroup is the remotesource id
 	cgNames := []string{util.EncodeSha256Hex("remotesourceid-" + remoteSource.ID)}
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		cgt, err = s.readDB.GetChangeGroupsUpdateTokens(tx, cgNames)
@@ -961,14 +1016,17 @@ func (s *CommandHandler) DeleteRemoteSource(ctx context.Context, remoteSourceNam
 
 func (s *CommandHandler) CreateOrg(ctx context.Context, org *types.Organization) (*types.Organization, error) {
 	if org.Name == "" {
-		return nil, util.NewErrBadRequest(errors.Errorf("org name required"))
+		return nil, util.NewErrBadRequest(errors.Errorf("organization name required"))
+	}
+	if !util.ValidateName(org.Name) {
+		return nil, util.NewErrBadRequest(errors.Errorf("invalid organization name %q", org.Name))
 	}
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 	// changegroup is the org name
 	cgNames := []string{util.EncodeSha256Hex("orgname-" + org.Name)}
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		cgt, err = s.readDB.GetChangeGroupsUpdateTokens(tx, cgNames)
@@ -1032,7 +1090,7 @@ func (s *CommandHandler) DeleteOrg(ctx context.Context, orgRef string) error {
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		// check org existance
@@ -1082,8 +1140,11 @@ func (s *CommandHandler) CreateSecret(ctx context.Context, secret *types.Secret)
 	if secret.Name == "" {
 		return nil, util.NewErrBadRequest(errors.Errorf("secret name required"))
 	}
+	if !util.ValidateName(secret.Name) {
+		return nil, util.NewErrBadRequest(errors.Errorf("invalid secret name %q", secret.Name))
+	}
 	if secret.Type != types.SecretTypeInternal {
-		return nil, util.NewErrBadRequest(errors.Errorf("wrong secret type %q", secret.Type))
+		return nil, util.NewErrBadRequest(errors.Errorf("invalid secret type %q", secret.Type))
 	}
 	switch secret.Type {
 	case types.SecretTypeInternal:
@@ -1105,7 +1166,7 @@ func (s *CommandHandler) CreateSecret(ctx context.Context, secret *types.Secret)
 	// changegroup is the secret name
 	cgNames := []string{util.EncodeSha256Hex("secretname-" + secret.Name)}
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		cgt, err = s.readDB.GetChangeGroupsUpdateTokens(tx, cgNames)
@@ -1158,7 +1219,7 @@ func (s *CommandHandler) DeleteSecret(ctx context.Context, parentType types.Conf
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		parentID, err := s.readDB.ResolveConfigID(tx, parentType, parentRef)
@@ -1204,6 +1265,9 @@ func (s *CommandHandler) CreateVariable(ctx context.Context, variable *types.Var
 	if variable.Name == "" {
 		return nil, util.NewErrBadRequest(errors.Errorf("variable name required"))
 	}
+	if !util.ValidateName(variable.Name) {
+		return nil, util.NewErrBadRequest(errors.Errorf("invalid variable name %q", variable.Name))
+	}
 	if len(variable.Values) == 0 {
 		return nil, util.NewErrBadRequest(errors.Errorf("variable values required"))
 	}
@@ -1221,7 +1285,7 @@ func (s *CommandHandler) CreateVariable(ctx context.Context, variable *types.Var
 	// changegroup is the variable name
 	cgNames := []string{util.EncodeSha256Hex("variablename-" + variable.Name)}
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		cgt, err = s.readDB.GetChangeGroupsUpdateTokens(tx, cgNames)
@@ -1274,7 +1338,7 @@ func (s *CommandHandler) DeleteVariable(ctx context.Context, parentType types.Co
 
 	var cgt *datamanager.ChangeGroupsUpdateToken
 
-	// must do all the check in a single transaction to avoid concurrent changes
+	// must do all the checks in a single transaction to avoid concurrent changes
 	err := s.readDB.Do(func(tx *db.Tx) error {
 		var err error
 		parentID, err := s.readDB.ResolveConfigID(tx, parentType, parentRef)

internal/services/configstore/configstore_test.go

@@ -420,7 +420,7 @@ func TestProjectGroupsAndProjects(t *testing.T) {
 	t.Run("create a project in user non root project group with same name as a root project", func(t *testing.T) {
 		_, err := cs.ch.CreateProject(ctx, &types.Project{Name: "project01", Parent: types.Parent{Type: types.ConfigTypeProjectGroup, ID: path.Join("user", user.Name, "projectgroup01")}, Visibility: types.VisibilityPublic, RemoteRepositoryConfigType: types.RemoteRepositoryConfigTypeManual})
 		if err != nil {
-			t.Fatalf("unexpected err: %+#v", err)
+			t.Fatalf("unexpected err: %v", err)
 		}
 	})
 	t.Run("create a project in org non root project group with same name as a root project", func(t *testing.T) {

internal/services/gateway/command/project.go

@@ -45,7 +45,7 @@ func (c *CommandHandler) CreateProject(ctx context.Context, req *CreateProjectRe
 		return nil, util.NewErrBadRequest(errors.Errorf("empty remote source name"))
 	}
 	if req.RepoPath == "" {
-		return nil, util.NewErrBadRequest(errors.Errorf("empty remote repo path name"))
+		return nil, util.NewErrBadRequest(errors.Errorf("empty remote repo path"))
 	}
 
 	user, resp, err := c.configstoreClient.GetUser(ctx, req.CurrentUserID)

internal/services/gateway/command/remotesource.go

@@ -17,7 +17,6 @@ package command
 import (
 	"context"
 
-	"github.com/sorintlab/agola/internal/services/gateway/common"
 	"github.com/sorintlab/agola/internal/services/types"
 	"github.com/sorintlab/agola/internal/util"
 
@@ -52,7 +51,7 @@ func (c *CommandHandler) CreateRemoteSource(ctx context.Context, req *CreateRemo
 	}
 
 	// validate if the remote source type supports the required auth type
-	if !common.SourceSupportsAuthType(types.RemoteSourceType(req.Type), types.RemoteSourceAuthType(req.AuthType)) {
+	if !types.SourceSupportsAuthType(types.RemoteSourceType(req.Type), types.RemoteSourceAuthType(req.AuthType)) {
 		return nil, util.NewErrBadRequest(errors.Errorf("remotesource type %q doesn't support auth type %q", req.Type, req.AuthType))
 	}
 

internal/services/gateway/common/gitsource.go

@@ -15,8 +15,6 @@
 package common
 
 import (
-	"fmt"
-
 	gitsource "github.com/sorintlab/agola/internal/gitsources"
 	"github.com/sorintlab/agola/internal/gitsources/gitea"
 	"github.com/sorintlab/agola/internal/gitsources/gitlab"
@@ -25,30 +23,6 @@ import (
 	"github.com/pkg/errors"
 )
 
-func SourceSupportedAuthTypes(rsType types.RemoteSourceType) []types.RemoteSourceAuthType {
-	switch rsType {
-	case types.RemoteSourceTypeGitea:
-		return []types.RemoteSourceAuthType{types.RemoteSourceAuthTypePassword}
-	case types.RemoteSourceTypeGithub:
-		fallthrough
-	case types.RemoteSourceTypeGitlab:
-		return []types.RemoteSourceAuthType{types.RemoteSourceAuthTypeOauth2}
-
-	default:
-		panic(fmt.Errorf("unsupported remote source type: %q", rsType))
-	}
-}
-
-func SourceSupportsAuthType(rsType types.RemoteSourceType, authType types.RemoteSourceAuthType) bool {
-	supportedAuthTypes := SourceSupportedAuthTypes(rsType)
-	for _, st := range supportedAuthTypes {
-		if st == authType {
-			return true
-		}
-	}
-	return false
-}
-
 func newGitea(rs *types.RemoteSource, accessToken string) (*gitea.Client, error) {
 	return gitea.New(gitea.Opts{
 		URL:        rs.APIURL,

internal/services/types/types.go

@@ -15,6 +15,7 @@
 package types
 
 import (
+	"fmt"
 	"regexp"
 	"time"
 )
@@ -129,6 +130,30 @@ type RemoteSource struct {
 	Oauth2ClientSecret string `json:"client_secret,omitempty"`
 }
 
+func SourceSupportedAuthTypes(rsType RemoteSourceType) []RemoteSourceAuthType {
+	switch rsType {
+	case RemoteSourceTypeGitea:
+		return []RemoteSourceAuthType{RemoteSourceAuthTypePassword}
+	case RemoteSourceTypeGithub:
+		fallthrough
+	case RemoteSourceTypeGitlab:
+		return []RemoteSourceAuthType{RemoteSourceAuthTypeOauth2}
+
+	default:
+		panic(fmt.Errorf("unsupported remote source type: %q", rsType))
+	}
+}
+
+func SourceSupportsAuthType(rsType RemoteSourceType, authType RemoteSourceAuthType) bool {
+	supportedAuthTypes := SourceSupportedAuthTypes(rsType)
+	for _, st := range supportedAuthTypes {
+		if st == authType {
+			return true
+		}
+	}
+	return false
+}
+
 type LinkedAccount struct {
 	// The type version. Increase when a breaking change is done. Usually not
 	// needed when adding fields.