From 01fffd56a34d991a8d38aa9f25035df2573b20e9 Mon Sep 17 00:00:00 2001 From: Vistaar Juneja Date: Thu, 10 Aug 2023 14:33:02 +0100 Subject: [PATCH] address comments --- cmd/gitness/wire_gen.go | 8 +- internal/api/controller/secret/controller.go | 4 + internal/api/controller/secret/create.go | 33 ++++ internal/api/controller/secret/delete.go | 4 +- internal/api/controller/secret/find.go | 15 +- internal/api/controller/secret/update.go | 16 +- internal/api/controller/secret/wire.go | 4 +- .../api/controller/space/list_pipelines.go | 20 +-- internal/api/controller/space/list_secrets.go | 20 +-- internal/api/handler/secret/delete.go | 3 - internal/api/handler/secret/update.go | 3 - internal/store/database.go | 6 +- internal/store/database/execution.go | 105 ++++-------- .../database/migrate/ci/ci_migrations.sql | 116 ++++++------- internal/store/database/pipeline.go | 100 ++++++----- internal/store/database/secret.go | 158 +++++++----------- internal/store/database/wire.go | 4 +- mocks/mock_client.go | 3 +- mocks/mock_store.go | 3 +- 19 files changed, 297 insertions(+), 328 deletions(-) diff --git a/cmd/gitness/wire_gen.go b/cmd/gitness/wire_gen.go index 81aca6116..a9d129906 100644 --- a/cmd/gitness/wire_gen.go +++ b/cmd/gitness/wire_gen.go @@ -91,14 +91,14 @@ func initSystem(ctx context.Context, config *types.Config) (*server.System, erro executionStore := database.ProvideExecutionStore(db) pipelineStore := database.ProvidePipelineStore(db) executionController := execution.ProvideController(db, authorizer, executionStore, pipelineStore, spaceStore) + secretStore := database.ProvideSecretStore(db) + spaceController := space.ProvideController(db, provider, pathUID, authorizer, pathStore, pipelineStore, secretStore, spaceStore, repoStore, principalStore, repoController, membershipStore) + pipelineController := pipeline.ProvideController(db, pathUID, pathStore, repoStore, authorizer, pipelineStore, spaceStore) encrypter, err := database.ProvideEncryptor(databaseConfig) if err != nil { return nil, err } - secretStore := database.ProvideSecretStore(encrypter, db) - spaceController := space.ProvideController(db, provider, pathUID, authorizer, pathStore, pipelineStore, secretStore, spaceStore, repoStore, principalStore, repoController, membershipStore) - pipelineController := pipeline.ProvideController(db, pathUID, pathStore, repoStore, authorizer, pipelineStore, spaceStore) - secretController := secret.ProvideController(db, pathUID, pathStore, secretStore, authorizer, spaceStore) + secretController := secret.ProvideController(db, pathUID, pathStore, encrypter, secretStore, authorizer, spaceStore) pullReqStore := database.ProvidePullReqStore(db, principalInfoCache) pullReqActivityStore := database.ProvidePullReqActivityStore(db, principalInfoCache) codeCommentView := database.ProvideCodeCommentView(db) diff --git a/internal/api/controller/secret/controller.go b/internal/api/controller/secret/controller.go index c979b6dcb..5a62633b2 100644 --- a/internal/api/controller/secret/controller.go +++ b/internal/api/controller/secret/controller.go @@ -5,6 +5,7 @@ package secret import ( + "github.com/harness/gitness/encrypt" "github.com/harness/gitness/internal/auth/authz" "github.com/harness/gitness/internal/store" "github.com/harness/gitness/types/check" @@ -16,6 +17,7 @@ type Controller struct { db *sqlx.DB uidCheck check.PathUID pathStore store.PathStore + encrypter encrypt.Encrypter secretStore store.SecretStore authorizer authz.Authorizer spaceStore store.SpaceStore @@ -26,6 +28,7 @@ func NewController( uidCheck check.PathUID, authorizer authz.Authorizer, pathStore store.PathStore, + encrypter encrypt.Encrypter, secretStore store.SecretStore, spaceStore store.SpaceStore, ) *Controller { @@ -33,6 +36,7 @@ func NewController( db: db, uidCheck: uidCheck, pathStore: pathStore, + encrypter: encrypter, secretStore: secretStore, authorizer: authorizer, spaceStore: spaceStore, diff --git a/internal/api/controller/secret/create.go b/internal/api/controller/secret/create.go index fe6da13f6..63b4ee881 100644 --- a/internal/api/controller/secret/create.go +++ b/internal/api/controller/secret/create.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/harness/gitness/encrypt" apiauth "github.com/harness/gitness/internal/api/auth" "github.com/harness/gitness/internal/api/usererror" "github.com/harness/gitness/internal/auth" @@ -66,6 +67,10 @@ func (c *Controller) Create(ctx context.Context, session *auth.Session, in *Crea Updated: now, Version: 0, } + secret, err = enc(c.encrypter, secret) + if err != nil { + return fmt.Errorf("could not encrypt secret: %w", err) + } err = c.secretStore.Create(ctx, secret) if err != nil { return fmt.Errorf("secret creation failed: %w", err) @@ -97,3 +102,31 @@ func (c *Controller) sanitizeCreateInput(in *CreateInput) error { return nil } + +// helper function returns the same secret with encrypted data. +func enc(encrypt encrypt.Encrypter, secret *types.Secret) (*types.Secret, error) { + if secret == nil { + return nil, fmt.Errorf("cannot encrypt a nil secret") + } + s := *secret + ciphertext, err := encrypt.Encrypt(secret.Data) + if err != nil { + return nil, err + } + s.Data = string(ciphertext) + return &s, nil +} + +// helper function returns the same secret with decrypted data. +func dec(encrypt encrypt.Encrypter, secret *types.Secret) (*types.Secret, error) { + if secret == nil { + return nil, fmt.Errorf("cannot decrypt a nil secret") + } + s := *secret + plaintext, err := encrypt.Decrypt([]byte(secret.Data)) + if err != nil { + return nil, err + } + s.Data = plaintext + return &s, nil +} diff --git a/internal/api/controller/secret/delete.go b/internal/api/controller/secret/delete.go index 8e42ede62..bcb31e1be 100644 --- a/internal/api/controller/secret/delete.go +++ b/internal/api/controller/secret/delete.go @@ -16,12 +16,12 @@ import ( func (c *Controller) Delete(ctx context.Context, session *auth.Session, spaceRef string, uid string) error { space, err := c.spaceStore.FindByRef(ctx, spaceRef) if err != nil { - return err + return fmt.Errorf("could not find space: %w", err) } err = apiauth.CheckSecret(ctx, c.authorizer, session, space.Path, uid, enum.PermissionSecretDelete) if err != nil { - return err + return fmt.Errorf("failed to authorize: %w", err) } err = c.secretStore.DeleteByUID(ctx, space.ID, uid) if err != nil { diff --git a/internal/api/controller/secret/find.go b/internal/api/controller/secret/find.go index 91d2e492f..34dcc9f3d 100644 --- a/internal/api/controller/secret/find.go +++ b/internal/api/controller/secret/find.go @@ -6,6 +6,7 @@ package secret import ( "context" + "fmt" apiauth "github.com/harness/gitness/internal/api/auth" "github.com/harness/gitness/internal/auth" @@ -21,11 +22,19 @@ func (c *Controller) Find( ) (*types.Secret, error) { space, err := c.spaceStore.FindByRef(ctx, spaceRef) if err != nil { - return nil, err + return nil, fmt.Errorf("could not find space: %w", err) } err = apiauth.CheckSecret(ctx, c.authorizer, session, space.Path, uid, enum.PermissionSecretView) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to authorize: %w", err) } - return c.secretStore.FindByUID(ctx, space.ID, uid) + secret, err := c.secretStore.FindByUID(ctx, space.ID, uid) + if err != nil { + return nil, fmt.Errorf("could not find secret: %w", err) + } + secret, err = dec(c.encrypter, secret) + if err != nil { + return nil, fmt.Errorf("could not decrypt secret: %w", err) + } + return secret, nil } diff --git a/internal/api/controller/secret/update.go b/internal/api/controller/secret/update.go index 3d283b11d..2ec2de78b 100644 --- a/internal/api/controller/secret/update.go +++ b/internal/api/controller/secret/update.go @@ -6,6 +6,7 @@ package secret import ( "context" + "fmt" apiauth "github.com/harness/gitness/internal/api/auth" "github.com/harness/gitness/internal/auth" @@ -25,20 +26,21 @@ func (c *Controller) Update( session *auth.Session, spaceRef string, uid string, - in *UpdateInput) (*types.Secret, error) { + in *UpdateInput, +) (*types.Secret, error) { space, err := c.spaceStore.FindByRef(ctx, spaceRef) if err != nil { - return nil, err + return nil, fmt.Errorf("could not find space: %w", err) } err = apiauth.CheckSecret(ctx, c.authorizer, session, space.Path, uid, enum.PermissionSecretEdit) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to authorize: %w", err) } secret, err := c.secretStore.FindByUID(ctx, space.ID, uid) if err != nil { - return nil, err + return nil, fmt.Errorf("could not find secret: %w", err) } return c.secretStore.UpdateOptLock(ctx, secret, func(original *types.Secret) error { @@ -46,7 +48,11 @@ func (c *Controller) Update( original.Description = in.Description } if in.Data != "" { - original.Data = in.Data // will get encrypted at db layer + data, err := c.encrypter.Encrypt(original.Data) + if err != nil { + return err + } + original.Data = string(data) } if in.UID != "" { original.UID = in.UID diff --git a/internal/api/controller/secret/wire.go b/internal/api/controller/secret/wire.go index 8922a5dc1..807cac0d8 100644 --- a/internal/api/controller/secret/wire.go +++ b/internal/api/controller/secret/wire.go @@ -5,6 +5,7 @@ package secret import ( + "github.com/harness/gitness/encrypt" "github.com/harness/gitness/internal/auth/authz" "github.com/harness/gitness/internal/store" "github.com/harness/gitness/types/check" @@ -21,9 +22,10 @@ var WireSet = wire.NewSet( func ProvideController(db *sqlx.DB, uidCheck check.PathUID, pathStore store.PathStore, + encrypter encrypt.Encrypter, secretStore store.SecretStore, authorizer authz.Authorizer, spaceStore store.SpaceStore, ) *Controller { - return NewController(db, uidCheck, authorizer, pathStore, secretStore, spaceStore) + return NewController(db, uidCheck, authorizer, pathStore, encrypter, secretStore, spaceStore) } diff --git a/internal/api/controller/space/list_pipelines.go b/internal/api/controller/space/list_pipelines.go index 6a70162c9..276106671 100644 --- a/internal/api/controller/space/list_pipelines.go +++ b/internal/api/controller/space/list_pipelines.go @@ -35,21 +35,21 @@ func (c *Controller) ListPipelines( var pipelines []types.Pipeline err = dbtx.New(c.db).WithTx(ctx, func(ctx context.Context) (err error) { - var dbErr error - count, dbErr = c.pipelineStore.Count(ctx, space.ID, pagination) - if dbErr != nil { - return fmt.Errorf("failed to count child executions: %w", err) + count, err = c.pipelineStore.Count(ctx, space.ID, pagination) + if err != nil { + err = fmt.Errorf("failed to count child executions: %w", err) + return } - pipelines, dbErr = c.pipelineStore.List(ctx, space.ID, pagination) - if dbErr != nil { - return fmt.Errorf("failed to list child executions: %w", err) + pipelines, err = c.pipelineStore.List(ctx, space.ID, pagination) + if err != nil { + err = fmt.Errorf("failed to count child executions: %w", err) + return } - - return dbErr + return }, dbtx.TxDefaultReadOnly) if err != nil { - return pipelines, count, err + return pipelines, count, fmt.Errorf("failed to list pipelines: %w", err) } return pipelines, count, nil diff --git a/internal/api/controller/space/list_secrets.go b/internal/api/controller/space/list_secrets.go index ff49b06d4..63db20f8e 100644 --- a/internal/api/controller/space/list_secrets.go +++ b/internal/api/controller/space/list_secrets.go @@ -35,21 +35,21 @@ func (c *Controller) ListSecrets( var secrets []types.Secret err = dbtx.New(c.db).WithTx(ctx, func(ctx context.Context) (err error) { - var dbErr error - count, dbErr = c.secretStore.Count(ctx, space.ID, pagination) - if dbErr != nil { - return fmt.Errorf("failed to count child executions: %w", err) + count, err = c.secretStore.Count(ctx, space.ID, pagination) + if err != nil { + err = fmt.Errorf("failed to count child executions: %w", err) + return } - secrets, dbErr = c.secretStore.List(ctx, space.ID, pagination) - if dbErr != nil { - return fmt.Errorf("failed to list child executions: %w", err) + secrets, err = c.secretStore.List(ctx, space.ID, pagination) + if err != nil { + err = fmt.Errorf("failed to list child executions: %w", err) + return } - - return dbErr + return }, dbtx.TxDefaultReadOnly) if err != nil { - return secrets, count, err + return secrets, count, fmt.Errorf("failed to list secrets: %w", err) } return secrets, count, nil diff --git a/internal/api/handler/secret/delete.go b/internal/api/handler/secret/delete.go index 681f6bd34..968a9d171 100644 --- a/internal/api/handler/secret/delete.go +++ b/internal/api/handler/secret/delete.go @@ -13,9 +13,6 @@ import ( "github.com/harness/gitness/internal/paths" ) -/* - * Deletes a secret. - */ func HandleDelete(secretCtrl *secret.Controller) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() diff --git a/internal/api/handler/secret/update.go b/internal/api/handler/secret/update.go index 6722ab14c..618319265 100644 --- a/internal/api/handler/secret/update.go +++ b/internal/api/handler/secret/update.go @@ -14,9 +14,6 @@ import ( "github.com/harness/gitness/internal/paths" ) -/* - * Updates an existing secret. - */ func HandleUpdate(secretCtrl *secret.Controller) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() diff --git a/internal/store/database.go b/internal/store/database.go index 3d066f3fa..3d4c6bd24 100644 --- a/internal/store/database.go +++ b/internal/store/database.go @@ -450,7 +450,7 @@ type ( Create(ctx context.Context, pipeline *types.Pipeline) error // Update tries to update a pipeline in the datastore - Update(ctx context.Context, pipeline *types.Pipeline) (*types.Pipeline, error) + Update(ctx context.Context, pipeline *types.Pipeline) error // List lists the pipelines present in a parent space ID in the datastore. List(ctx context.Context, spaceID int64, pagination types.Pagination) ([]types.Pipeline, error) @@ -490,7 +490,7 @@ type ( mutateFn func(secret *types.Secret) error) (*types.Secret, error) // Update tries to update a secret. - Update(ctx context.Context, secret *types.Secret) (*types.Secret, error) + Update(ctx context.Context, secret *types.Secret) error // Delete deletes a secret given an ID. Delete(ctx context.Context, id int64) error @@ -510,7 +510,7 @@ type ( Create(ctx context.Context, execution *types.Execution) error // Update tries to update an execution. - Update(ctx context.Context, execution *types.Execution) (*types.Execution, error) + Update(ctx context.Context, execution *types.Execution) error // UpdateOptLock updates the execution using the optimistic locking mechanism. UpdateOptLock(ctx context.Context, exectuion *types.Execution, diff --git a/internal/store/database/execution.go b/internal/store/database/execution.go index 29c8f53bf..8edc51338 100644 --- a/internal/store/database/execution.go +++ b/internal/store/database/execution.go @@ -33,10 +33,6 @@ type executionStore struct { } const ( - executionQueryBase = ` - SELECT` + executionColumns + ` - FROM executions` - executionColumns = ` execution_id ,execution_pipeline_id @@ -74,8 +70,26 @@ const ( ,execution_updated ,execution_version ` +) - executionInsertStmt = ` +// Find returns an execution given a pipeline ID and an execution number. +func (s *executionStore) Find(ctx context.Context, pipelineID int64, executionNum int64) (*types.Execution, error) { + const findQueryStmt = ` + SELECT` + executionColumns + ` + FROM executions + WHERE execution_pipeline_id = $1 AND execution_number = $2` + db := dbtx.GetAccessor(ctx, s.db) + + dst := new(types.Execution) + if err := db.GetContext(ctx, dst, findQueryStmt, pipelineID, executionNum); err != nil { + return nil, database.ProcessSQLErrorf(err, "Failed to find execution") + } + return dst, nil +} + +// Create creates a new execution in the datastore. +func (s *executionStore) Create(ctx context.Context, execution *types.Execution) error { + const executionInsertStmt = ` INSERT INTO executions ( execution_pipeline_id ,execution_repo_id @@ -147,58 +161,6 @@ const ( ,:execution_updated ,:execution_version ) RETURNING execution_id` - - executionUpdateStmt = ` - UPDATE executions - SET - execution_trigger = :execution_trigger - ,execution_parent = :execution_parent - ,execution_status = :execution_status - ,execution_error = :execution_error - ,execution_event = :execution_event - ,execution_action = :execution_action - ,execution_link = :execution_link - ,execution_timestamp = :execution_timestamp - ,execution_title = :execution_title - ,execution_message = :execution_message - ,execution_before = :execution_before - ,execution_after = :execution_after - ,execution_ref = :execution_ref - ,execution_source_repo = :execution_source_repo - ,execution_source = :execution_source - ,execution_target = :execution_target - ,execution_author = :execution_author - ,execution_author_name = :execution_author_name - ,execution_author_email = :execution_author_email - ,execution_author_avatar = :execution_author_avatar - ,execution_sender = :execution_sender - ,execution_params = :execution_params - ,execution_cron = :execution_cron - ,execution_deploy = :execution_deploy - ,execution_deploy_id = :execution_deploy_id - ,execution_debug = :execution_debug - ,execution_started = :execution_started - ,execution_finished = :execution_finished - ,execution_updated = :execution_updated - ,execution_version = :execution_version - WHERE execution_id = :execution_id AND execution_version = :execution_version - 1` -) - -// Find returns an execution given a pipeline ID and an execution number. -func (s *executionStore) Find(ctx context.Context, pipelineID int64, executionNum int64) (*types.Execution, error) { - const findQueryStmt = executionQueryBase + ` - WHERE execution_pipeline_id = $1 AND execution_number = $2` - db := dbtx.GetAccessor(ctx, s.db) - - dst := new(types.Execution) - if err := db.GetContext(ctx, dst, findQueryStmt, pipelineID, executionNum); err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to find execution") - } - return dst, nil -} - -// Create creates a new execution in the datastore. -func (s *executionStore) Create(ctx context.Context, execution *types.Execution) error { db := dbtx.GetAccessor(ctx, s.db) query, arg, err := db.BindNamed(executionInsertStmt, execution) @@ -214,7 +176,18 @@ func (s *executionStore) Create(ctx context.Context, execution *types.Execution) } // Update tries to update an execution in the datastore with optimistic locking. -func (s *executionStore) Update(ctx context.Context, execution *types.Execution) (*types.Execution, error) { +func (s *executionStore) Update(ctx context.Context, execution *types.Execution) error { + const executionUpdateStmt = ` + UPDATE executions + SET + ,execution_status = :execution_status + ,execution_error = :execution_error + ,execution_event = :execution_event + ,execution_started = :execution_started + ,execution_finished = :execution_finished + ,execution_updated = :execution_updated + ,execution_version = :execution_version + WHERE execution_id = :execution_id AND execution_version = :execution_version - 1` updatedAt := time.Now() execution.Version++ @@ -224,24 +197,24 @@ func (s *executionStore) Update(ctx context.Context, execution *types.Execution) query, arg, err := db.BindNamed(executionUpdateStmt, execution) if err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to bind execution object") + return database.ProcessSQLErrorf(err, "Failed to bind execution object") } result, err := db.ExecContext(ctx, query, arg...) if err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to update execution") + return database.ProcessSQLErrorf(err, "Failed to update execution") } count, err := result.RowsAffected() if err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to get number of updated rows") + return database.ProcessSQLErrorf(err, "Failed to get number of updated rows") } if count == 0 { - return nil, gitness_store.ErrVersionConflict + return gitness_store.ErrVersionConflict } - return execution, nil + return nil } // UpdateOptLock updates the pipeline using the optimistic locking mechanism. @@ -251,16 +224,12 @@ func (s *executionStore) UpdateOptLock(ctx context.Context, for { dup := *execution - fmt.Println(dup.Status) - err := mutateFn(&dup) if err != nil { return nil, err } - fmt.Println("dup.Status after: ", dup.Status) - - execution, err = s.Update(ctx, &dup) + err = s.Update(ctx, &dup) if err == nil { return &dup, nil } diff --git a/internal/store/database/migrate/ci/ci_migrations.sql b/internal/store/database/migrate/ci/ci_migrations.sql index 57df6eaf5..89975c98a 100644 --- a/internal/store/database/migrate/ci/ci_migrations.sql +++ b/internal/store/database/migrate/ci/ci_migrations.sql @@ -1,17 +1,17 @@ CREATE TABLE IF NOT EXISTS pipelines ( - pipeline_id INTEGER PRIMARY KEY AUTOINCREMENT, - pipeline_description TEXT, - pipeline_space_id INTEGER NOT NULL, - pipeline_uid TEXT NOT NULL, - pipeline_seq INTEGER NOT NULL DEFAULT 0, - pipeline_repo_id INTEGER, - pipeline_repo_type TEXT NOT NULL, - pipeline_repo_name TEXT, - pipeline_default_branch TEXT, - pipeline_config_path TEXT NOT NULL, - pipeline_created INTEGER NOT NULL, - pipeline_updated INTEGER NOT NULL, - pipeline_version INTEGER NOT NULL, + pipeline_id INTEGER PRIMARY KEY AUTOINCREMENT + ,pipeline_description TEXT NOT NULL + ,pipeline_space_id INTEGER NOT NULL + ,pipeline_uid TEXT NOT NULL + ,pipeline_seq INTEGER NOT NULL DEFAULT 0 + ,pipeline_repo_id INTEGER + ,pipeline_repo_type TEXT NOT NULL + ,pipeline_repo_name TEXT + ,pipeline_default_branch TEXT + ,pipeline_config_path TEXT NOT NULL + ,pipeline_created INTEGER NOT NULL + ,pipeline_updated INTEGER NOT NULL + ,pipeline_version INTEGER NOT NULL -- Ensure unique combination of UID and ParentID UNIQUE (pipeline_space_id, pipeline_uid), @@ -30,67 +30,67 @@ CREATE TABLE IF NOT EXISTS pipelines ( ); CREATE TABLE IF NOT EXISTS executions ( - execution_id INTEGER PRIMARY KEY AUTOINCREMENT, - execution_pipeline_id INTEGER NOT NULL, - execution_repo_id INTEGER, - execution_trigger TEXT, - execution_number INTEGER NOT NULL, - execution_parent INTEGER, - execution_status TEXT, - execution_error TEXT, - execution_event TEXT, - execution_action TEXT, - execution_link TEXT, - execution_timestamp INTEGER, - execution_title TEXT, - execution_message TEXT, - execution_before TEXT, - execution_after TEXT, - execution_ref TEXT, - execution_source_repo TEXT, - execution_source TEXT, - execution_target TEXT, - execution_author TEXT, - execution_author_name TEXT, - execution_author_email TEXT, - execution_author_avatar TEXT, - execution_sender TEXT, - execution_params TEXT, - execution_cron TEXT, - execution_deploy TEXT, - execution_deploy_id INTEGER, - execution_debug BOOLEAN NOT NULL DEFAULT 0, - execution_started INTEGER, - execution_finished INTEGER, - execution_created INTEGER NOT NULL, - execution_updated INTEGER NOT NULL, - execution_version INTEGER NOT NULL, + execution_id INTEGER PRIMARY KEY AUTOINCREMENT + ,execution_pipeline_id INTEGER NOT NULL + ,execution_repo_id INTEGER + ,execution_trigger TEXT + ,execution_number INTEGER NOT NULL + ,execution_parent INTEGER + ,execution_status TEXT + ,execution_error TEXT + ,execution_event TEXT + ,execution_action TEXT + ,execution_link TEXT + ,execution_timestamp INTEGER + ,execution_title TEXT + ,execution_message TEXT + ,execution_before TEXT + ,execution_after TEXT + ,execution_ref TEXT + ,execution_source_repo TEXT + ,execution_source TEXT + ,execution_target TEXT + ,execution_author TEXT + ,execution_author_name TEXT + ,execution_author_email TEXT + ,execution_author_avatar TEXT + ,execution_sender TEXT + ,execution_params TEXT + ,execution_cron TEXT + ,execution_deploy TEXT + ,execution_deploy_id INTEGER + ,execution_debug BOOLEAN NOT NULL DEFAULT 0 + ,execution_started INTEGER + ,execution_finished INTEGER + ,execution_created INTEGER NOT NULL + ,execution_updated INTEGER NOT NULL + ,execution_version INTEGER NOT NULL -- Ensure unique combination of pipeline ID and number UNIQUE (execution_pipeline_id, execution_number), -- Foreign key to pipelines table - CONSTRAINT fk_executions_pipeline_id FOREIGN KEY (execution_pipeline_id) + CONSTRAINT fk_executions_pipeline_id FOREIGN KEY (,execution_pipeline_id) REFERENCES pipelines (pipeline_id) MATCH SIMPLE ON UPDATE NO ACTION ON DELETE CASCADE -- Foreign key to repositories table - CONSTRAINT fk_executions_repo_id FOREIGN KEY (execution_repo_id) + CONSTRAINT fk_executions_repo_id FOREIGN KEY (,execution_repo_id) REFERENCES repositories (repo_id) MATCH SIMPLE ON UPDATE NO ACTION ON DELETE CASCADE ); CREATE TABLE IF NOT EXISTS secrets ( - secret_id INTEGER PRIMARY KEY AUTOINCREMENT, - secret_uid TEXT NOT NULL, - secret_space_id INTEGER NOT NULL, - secret_description TEXT, - secret_data BLOB NOT NULL, - secret_created INTEGER NOT NULL, - secret_updated INTEGER NOT NULL, - secret_version INTEGER NOT NULL, + secret_id INTEGER PRIMARY KEY AUTOINCREMENT + ,secret_uid TEXT NOT NULL + ,secret_space_id INTEGER NOT NULL + ,secret_description TEXT NOT NULL + ,secret_data BLOB NOT NULL + ,secret_created INTEGER NOT NULL + ,secret_updated INTEGER NOT NULL + ,secret_version INTEGER NOT NULL -- Ensure unique combination of space ID and UID UNIQUE (secret_space_id, secret_uid), diff --git a/internal/store/database/pipeline.go b/internal/store/database/pipeline.go index a09320fdb..5b83426f8 100644 --- a/internal/store/database/pipeline.go +++ b/internal/store/database/pipeline.go @@ -43,51 +43,6 @@ const ( ,pipeline_updated ,pipeline_version ` - - pipelineInsertStmt = ` - INSERT INTO pipelines ( - pipeline_description - ,pipeline_space_id - ,pipeline_uid - ,pipeline_seq - ,pipeline_repo_id - ,pipeline_repo_type - ,pipeline_repo_name - ,pipeline_default_branch - ,pipeline_config_path - ,pipeline_created - ,pipeline_updated - ,pipeline_version - ) VALUES ( - :pipeline_description, - :pipeline_space_id, - :pipeline_uid, - :pipeline_seq, - :pipeline_repo_id, - :pipeline_repo_type, - :pipeline_repo_name, - :pipeline_default_branch, - :pipeline_config_path, - :pipeline_created, - :pipeline_updated, - :pipeline_version - ) RETURNING pipeline_id` - - pipelineUpdateStmt = ` - UPDATE pipelines - SET - pipeline_description = :pipeline_description, - pipeline_space_id = :pipeline_space_id, - pipeline_uid = :pipeline_uid, - pipeline_seq = :pipeline_seq, - pipeline_repo_id = :pipeline_repo_id, - pipeline_repo_type = :pipeline_repo_type, - pipeline_repo_name = :pipeline_repo_name, - pipeline_default_branch = :pipeline_default_branch, - pipeline_config_path = :pipeline_config_path, - pipeline_updated = :pipeline_updated, - pipeline_version = :pipeline_version - WHERE pipeline_id = :pipeline_id AND pipeline_version = :pipeline_version - 1` ) // NewPipelineStore returns a new PipelineStore. @@ -129,6 +84,34 @@ func (s *pipelineStore) FindByUID(ctx context.Context, spaceID int64, uid string // Create creates a pipeline. func (s *pipelineStore) Create(ctx context.Context, pipeline *types.Pipeline) error { + const pipelineInsertStmt = ` + INSERT INTO pipelines ( + pipeline_description + ,pipeline_space_id + ,pipeline_uid + ,pipeline_seq + ,pipeline_repo_id + ,pipeline_repo_type + ,pipeline_repo_name + ,pipeline_default_branch + ,pipeline_config_path + ,pipeline_created + ,pipeline_updated + ,pipeline_version + ) VALUES ( + :pipeline_description, + :pipeline_space_id, + :pipeline_uid, + :pipeline_seq, + :pipeline_repo_id, + :pipeline_repo_type, + :pipeline_repo_name, + :pipeline_default_branch, + :pipeline_config_path, + :pipeline_created, + :pipeline_updated, + :pipeline_version + ) RETURNING pipeline_id` db := dbtx.GetAccessor(ctx, s.db) query, arg, err := db.BindNamed(pipelineInsertStmt, pipeline) @@ -144,7 +127,18 @@ func (s *pipelineStore) Create(ctx context.Context, pipeline *types.Pipeline) er } // Update updates a pipeline. -func (s *pipelineStore) Update(ctx context.Context, pipeline *types.Pipeline) (*types.Pipeline, error) { +func (s *pipelineStore) Update(ctx context.Context, pipeline *types.Pipeline) error { + const pipelineUpdateStmt = ` + UPDATE pipelines + SET + pipeline_description = :pipeline_description, + pipeline_uid = :pipeline_uid, + pipeline_seq = :pipeline_seq, + pipeline_default_branch = :pipeline_default_branch, + pipeline_config_path = :pipeline_config_path, + pipeline_updated = :pipeline_updated, + pipeline_version = :pipeline_version + WHERE pipeline_id = :pipeline_id AND pipeline_version = :pipeline_version - 1` updatedAt := time.Now() pipeline.Version++ @@ -154,24 +148,24 @@ func (s *pipelineStore) Update(ctx context.Context, pipeline *types.Pipeline) (* query, arg, err := db.BindNamed(pipelineUpdateStmt, pipeline) if err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to bind pipeline object") + return database.ProcessSQLErrorf(err, "Failed to bind pipeline object") } result, err := db.ExecContext(ctx, query, arg...) if err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to update pipeline") + return database.ProcessSQLErrorf(err, "Failed to update pipeline") } count, err := result.RowsAffected() if err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to get number of updated rows") + return database.ProcessSQLErrorf(err, "Failed to get number of updated rows") } if count == 0 { - return nil, gitness_store.ErrVersionConflict + return gitness_store.ErrVersionConflict } - return pipeline, nil + return nil } // List lists all the pipelines present in a space. @@ -219,7 +213,7 @@ func (s *pipelineStore) UpdateOptLock(ctx context.Context, return nil, err } - pipeline, err = s.Update(ctx, &dup) + err = s.Update(ctx, &dup) if err == nil { return &dup, nil } @@ -296,7 +290,7 @@ func (s *pipelineStore) IncrementSeqNum(ctx context.Context, pipeline *types.Pip for { var err error pipeline.Seq++ - pipeline, err = s.Update(ctx, pipeline) + err = s.Update(ctx, pipeline) if err == nil { return pipeline, nil } else if !errors.Is(err, gitness_store.ErrVersionConflict) { diff --git a/internal/store/database/secret.go b/internal/store/database/secret.go index 2ce4f5bf9..5761a468b 100644 --- a/internal/store/database/secret.go +++ b/internal/store/database/secret.go @@ -10,7 +10,6 @@ import ( "strings" "time" - "github.com/harness/gitness/encrypt" "github.com/harness/gitness/internal/store" gitness_store "github.com/harness/gitness/store" "github.com/harness/gitness/store/database" @@ -38,8 +37,48 @@ const ( secret_updated, secret_version ` +) - secretInsertStmt = ` +// NewSecretStore returns a new SecretStore. +func NewSecretStore(db *sqlx.DB) *secretStore { + return &secretStore{ + db: db, + } +} + +type secretStore struct { + db *sqlx.DB +} + +// Find returns a secret given a secret ID. +func (s *secretStore) Find(ctx context.Context, id int64) (*types.Secret, error) { + const findQueryStmt = secretQueryBase + ` + WHERE secret_id = $1` + db := dbtx.GetAccessor(ctx, s.db) + + dst := new(types.Secret) + if err := db.GetContext(ctx, dst, findQueryStmt, id); err != nil { + return nil, database.ProcessSQLErrorf(err, "Failed to find secret") + } + return dst, nil +} + +// FindByUID returns a secret in a given space with a given UID. +func (s *secretStore) FindByUID(ctx context.Context, spaceID int64, uid string) (*types.Secret, error) { + const findQueryStmt = secretQueryBase + ` + WHERE secret_space_id = $1 AND secret_uid = $2` + db := dbtx.GetAccessor(ctx, s.db) + + dst := new(types.Secret) + if err := db.GetContext(ctx, dst, findQueryStmt, spaceID, uid); err != nil { + return nil, database.ProcessSQLErrorf(err, "Failed to find secret") + } + return dst, nil +} + +// Create creates a secret. +func (s *secretStore) Create(ctx context.Context, secret *types.Secret) error { + const secretInsertStmt = ` INSERT INTO secrets ( secret_description, secret_space_id, @@ -57,67 +96,8 @@ const ( :secret_updated, :secret_version ) RETURNING secret_id` - - secretUpdateStmt = ` - UPDATE secrets - SET - secret_description = :secret_description, - secret_space_id = :secret_space_id, - secret_uid = :secret_uid, - secret_data = :secret_data, - secret_updated = :secret_updated, - secret_version = :secret_version - WHERE secret_id = :secret_id AND secret_version = :secret_version - 1` -) - -// NewSecretStore returns a new SecretStore. -func NewSecretStore(enc encrypt.Encrypter, db *sqlx.DB) *secretStore { - return &secretStore{ - db: db, - enc: enc, - } -} - -type secretStore struct { - db *sqlx.DB - enc encrypt.Encrypter -} - -// Find returns a secret given a secret ID. -func (s *secretStore) Find(ctx context.Context, id int64) (*types.Secret, error) { - const findQueryStmt = secretQueryBase + ` - WHERE secret_id = $1` db := dbtx.GetAccessor(ctx, s.db) - dst := new(types.Secret) - if err := db.GetContext(ctx, dst, findQueryStmt, id); err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to find secret") - } - return dec(s.enc, dst) -} - -// FindByUID returns a secret in a given space with a given UID. -func (s *secretStore) FindByUID(ctx context.Context, spaceID int64, uid string) (*types.Secret, error) { - const findQueryStmt = secretQueryBase + ` - WHERE secret_space_id = $1 AND secret_uid = $2` - db := dbtx.GetAccessor(ctx, s.db) - - dst := new(types.Secret) - if err := db.GetContext(ctx, dst, findQueryStmt, spaceID, uid); err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to find secret") - } - return dec(s.enc, dst) -} - -// Create creates a secret. -func (s *secretStore) Create(ctx context.Context, secret *types.Secret) error { - db := dbtx.GetAccessor(ctx, s.db) - - secret, err := enc(s.enc, secret) - if err != nil { - return err - } - query, arg, err := db.BindNamed(secretInsertStmt, secret) if err != nil { return database.ProcessSQLErrorf(err, "Failed to bind secret object") @@ -130,7 +110,16 @@ func (s *secretStore) Create(ctx context.Context, secret *types.Secret) error { return nil } -func (s *secretStore) Update(ctx context.Context, secret *types.Secret) (*types.Secret, error) { +func (s *secretStore) Update(ctx context.Context, secret *types.Secret) error { + const secretUpdateStmt = ` + UPDATE secrets + SET + secret_description = :secret_description, + secret_uid = :secret_uid, + secret_data = :secret_data, + secret_updated = :secret_updated, + secret_version = :secret_version + WHERE secret_id = :secret_id AND secret_version = :secret_version - 1` updatedAt := time.Now() secret.Version++ @@ -138,31 +127,26 @@ func (s *secretStore) Update(ctx context.Context, secret *types.Secret) (*types. db := dbtx.GetAccessor(ctx, s.db) - secret, err := enc(s.enc, secret) - if err != nil { - return nil, err - } - query, arg, err := db.BindNamed(secretUpdateStmt, secret) if err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to bind secret object") + return database.ProcessSQLErrorf(err, "Failed to bind secret object") } result, err := db.ExecContext(ctx, query, arg...) if err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to update secret") + return database.ProcessSQLErrorf(err, "Failed to update secret") } count, err := result.RowsAffected() if err != nil { - return nil, database.ProcessSQLErrorf(err, "Failed to get number of updated rows") + return database.ProcessSQLErrorf(err, "Failed to get number of updated rows") } if count == 0 { - return nil, gitness_store.ErrVersionConflict + return gitness_store.ErrVersionConflict } - return secret, nil + return nil } // UpdateOptLock updates the pipeline using the optimistic locking mechanism. @@ -177,7 +161,7 @@ func (s *secretStore) UpdateOptLock(ctx context.Context, return nil, err } - secret, err = s.Update(ctx, &dup) + err = s.Update(ctx, &dup) if err == nil { return &dup, nil } @@ -276,31 +260,3 @@ func (s *secretStore) Count(ctx context.Context, parentID int64, filter types.Pa } return count, nil } - -// helper function returns the same secret with encrypted data. -func enc(encrypt encrypt.Encrypter, secret *types.Secret) (*types.Secret, error) { - if secret == nil { - return nil, fmt.Errorf("cannot encrypt a nil secret") - } - s := *secret - ciphertext, err := encrypt.Encrypt(secret.Data) - if err != nil { - return nil, err - } - s.Data = string(ciphertext) - return &s, nil -} - -// helper function returns the same secret with decrypted data. -func dec(encrypt encrypt.Encrypter, secret *types.Secret) (*types.Secret, error) { - if secret == nil { - return nil, fmt.Errorf("cannot decrypt a nil secret") - } - s := *secret - plaintext, err := encrypt.Decrypt([]byte(secret.Data)) - if err != nil { - return nil, err - } - s.Data = plaintext - return &s, nil -} diff --git a/internal/store/database/wire.go b/internal/store/database/wire.go index bd4b64cc1..04179824a 100644 --- a/internal/store/database/wire.go +++ b/internal/store/database/wire.go @@ -105,8 +105,8 @@ func ProvidePipelineStore(db *sqlx.DB) store.PipelineStore { } // ProvideSecretStore provides a secret store. -func ProvideSecretStore(enc encrypt.Encrypter, db *sqlx.DB) store.SecretStore { - return NewSecretStore(enc, db) +func ProvideSecretStore(db *sqlx.DB) store.SecretStore { + return NewSecretStore(db) } // ProvideExecutionStore provides an execution store. diff --git a/mocks/mock_client.go b/mocks/mock_client.go index b16a6490e..3cb746055 100644 --- a/mocks/mock_client.go +++ b/mocks/mock_client.go @@ -8,9 +8,10 @@ import ( context "context" reflect "reflect" - gomock "github.com/golang/mock/gomock" user "github.com/harness/gitness/internal/api/controller/user" types "github.com/harness/gitness/types" + + gomock "github.com/golang/mock/gomock" ) // MockClient is a mock of Client interface. diff --git a/mocks/mock_store.go b/mocks/mock_store.go index 0af0bbc34..9310f3729 100644 --- a/mocks/mock_store.go +++ b/mocks/mock_store.go @@ -8,9 +8,10 @@ import ( context "context" reflect "reflect" - gomock "github.com/golang/mock/gomock" types "github.com/harness/gitness/types" enum "github.com/harness/gitness/types/enum" + + gomock "github.com/golang/mock/gomock" ) // MockPrincipalStore is a mock of PrincipalStore interface.