From 60feff5cef9b469ca663d56671c36d9675649ea1 Mon Sep 17 00:00:00 2001 From: Simone Gotti Date: Fri, 3 May 2019 12:41:49 +0200 Subject: [PATCH] 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) --- .../services/configstore/command/command.go | 106 ++++++++++++++---- .../services/configstore/configstore_test.go | 2 +- internal/services/gateway/command/project.go | 2 +- .../services/gateway/command/remotesource.go | 3 +- internal/services/gateway/common/gitsource.go | 26 ----- internal/services/types/types.go | 25 +++++ 6 files changed, 113 insertions(+), 51 deletions(-) diff --git a/internal/services/configstore/command/command.go b/internal/services/configstore/command/command.go index 8cc6346..756eaa0 100644 --- a/internal/services/configstore/command/command.go +++ b/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) diff --git a/internal/services/configstore/configstore_test.go b/internal/services/configstore/configstore_test.go index 57e9d18..3397b40 100644 --- a/internal/services/configstore/configstore_test.go +++ b/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) { diff --git a/internal/services/gateway/command/project.go b/internal/services/gateway/command/project.go index fa8cb23..696365e 100644 --- a/internal/services/gateway/command/project.go +++ b/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) diff --git a/internal/services/gateway/command/remotesource.go b/internal/services/gateway/command/remotesource.go index f30f4f3..2d86466 100644 --- a/internal/services/gateway/command/remotesource.go +++ b/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)) } diff --git a/internal/services/gateway/common/gitsource.go b/internal/services/gateway/common/gitsource.go index 0026970..575f120 100644 --- a/internal/services/gateway/common/gitsource.go +++ b/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, diff --git a/internal/services/types/types.go b/internal/services/types/types.go index 0547f23..1ae0192 100644 --- a/internal/services/types/types.go +++ b/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.