Refactor secrets modification logic (#26873)

- Share code between web and api
- Add some tests

5 files changed, 96 insertions, 113 deletions

diff --git a/routers/api/v1/org/action.go b/routers/api/v1/org/action.go
index a04058be19..e50a77f362 100644
--- a/routers/api/v1/org/action.go
+++ b/routers/api/v1/org/action.go
@@ -4,14 +4,16 @@
 package org
 
 import (
+	"errors"
 	"net/http"
 
 	secret_model "code.gitea.io/gitea/models/secret"
 	"code.gitea.io/gitea/modules/context"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/v1/utils"
-	"code.gitea.io/gitea/routers/web/shared/actions"
+	secret_service "code.gitea.io/gitea/services/secrets"
 )
 
 // ListActionsSecrets list an organization's actions secrets
@@ -39,11 +41,6 @@ func ListActionsSecrets(ctx *context.APIContext) {
 	//   "200":
 	//     "$ref": "#/responses/SecretList"
 
-	listActionsSecrets(ctx)
-}
-
-// listActionsSecrets list an organization's actions secrets
-func listActionsSecrets(ctx *context.APIContext) {
 	opts := &secret_model.FindSecretsOptions{
 		OwnerID:     ctx.Org.Organization.ID,
 		ListOptions: utils.GetListOptions(ctx),
@@ -104,25 +101,28 @@ func CreateOrUpdateSecret(ctx *context.APIContext) {
 	//     description: response when updating a secret
 	//   "400":
 	//     "$ref": "#/responses/error"
-	//   "403":
-	//     "$ref": "#/responses/forbidden"
-	secretName := ctx.Params(":secretname")
-	if err := actions.NameRegexMatch(secretName); err != nil {
-		ctx.Error(http.StatusBadRequest, "CreateOrUpdateSecret", err)
-		return
-	}
+	//   "404":
+	//     "$ref": "#/responses/notFound"
+
 	opt := web.GetForm(ctx).(*api.CreateOrUpdateSecretOption)
-	isCreated, err := secret_model.CreateOrUpdateSecret(ctx, ctx.Org.Organization.ID, 0, secretName, opt.Data)
+
+	_, created, err := secret_service.CreateOrUpdateSecret(ctx, ctx.Org.Organization.ID, 0, ctx.Params("secretname"), opt.Data)
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "CreateOrUpdateSecret", err)
+		if errors.Is(err, util.ErrInvalidArgument) {
+			ctx.Error(http.StatusBadRequest, "CreateOrUpdateSecret", err)
+		} else if errors.Is(err, util.ErrNotExist) {
+			ctx.Error(http.StatusNotFound, "CreateOrUpdateSecret", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "CreateOrUpdateSecret", err)
+		}
 		return
 	}
-	if isCreated {
+
+	if created {
 		ctx.Status(http.StatusCreated)
-		return
+	} else {
+		ctx.Status(http.StatusNoContent)
 	}
-
-	ctx.Status(http.StatusNoContent)
 }
 
 // DeleteSecret delete one secret of the organization
@@ -148,22 +148,20 @@ func DeleteSecret(ctx *context.APIContext) {
 	// responses:
 	//   "204":
 	//     description: delete one secret of the organization
-	//   "403":
-	//     "$ref": "#/responses/forbidden"
-	secretName := ctx.Params(":secretname")
-	if err := actions.NameRegexMatch(secretName); err != nil {
-		ctx.Error(http.StatusBadRequest, "DeleteSecret", err)
-		return
-	}
-	err := secret_model.DeleteSecret(
-		ctx, ctx.Org.Organization.ID, 0, secretName,
-	)
-	if secret_model.IsErrSecretNotFound(err) {
-		ctx.NotFound(err)
-		return
-	}
+	//   "400":
+	//     "$ref": "#/responses/error"
+	//   "404":
+	//     "$ref": "#/responses/notFound"
+
+	err := secret_service.DeleteSecretByName(ctx, ctx.Org.Organization.ID, 0, ctx.Params("secretname"))
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "DeleteSecret", err)
+		if errors.Is(err, util.ErrInvalidArgument) {
+			ctx.Error(http.StatusBadRequest, "DeleteSecret", err)
+		} else if errors.Is(err, util.ErrNotExist) {
+			ctx.Error(http.StatusNotFound, "DeleteSecret", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "DeleteSecret", err)
+		}
 		return
 	}
 
diff --git a/routers/api/v1/repo/action.go b/routers/api/v1/repo/action.go
index b7642b6af9..039cdadac9 100644
--- a/routers/api/v1/repo/action.go
+++ b/routers/api/v1/repo/action.go
@@ -4,13 +4,14 @@
 package repo
 
 import (
+	"errors"
 	"net/http"
 
-	secret_model "code.gitea.io/gitea/models/secret"
 	"code.gitea.io/gitea/modules/context"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
-	"code.gitea.io/gitea/routers/web/shared/actions"
+	secret_service "code.gitea.io/gitea/services/secrets"
 )
 
 // create or update one secret of the repository
@@ -49,29 +50,31 @@ func CreateOrUpdateSecret(ctx *context.APIContext) {
 	//     description: response when updating a secret
 	//   "400":
 	//     "$ref": "#/responses/error"
-	//   "403":
-	//     "$ref": "#/responses/forbidden"
+	//   "404":
+	//     "$ref": "#/responses/notFound"
 
 	owner := ctx.Repo.Owner
 	repo := ctx.Repo.Repository
 
-	secretName := ctx.Params(":secretname")
-	if err := actions.NameRegexMatch(secretName); err != nil {
-		ctx.Error(http.StatusBadRequest, "CreateOrUpdateSecret", err)
-		return
-	}
 	opt := web.GetForm(ctx).(*api.CreateOrUpdateSecretOption)
-	isCreated, err := secret_model.CreateOrUpdateSecret(ctx, owner.ID, repo.ID, secretName, opt.Data)
+
+	_, created, err := secret_service.CreateOrUpdateSecret(ctx, owner.ID, repo.ID, ctx.Params("secretname"), opt.Data)
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "CreateOrUpdateSecret", err)
+		if errors.Is(err, util.ErrInvalidArgument) {
+			ctx.Error(http.StatusBadRequest, "CreateOrUpdateSecret", err)
+		} else if errors.Is(err, util.ErrNotExist) {
+			ctx.Error(http.StatusNotFound, "CreateOrUpdateSecret", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "CreateOrUpdateSecret", err)
+		}
 		return
 	}
-	if isCreated {
+
+	if created {
 		ctx.Status(http.StatusCreated)
-		return
+	} else {
+		ctx.Status(http.StatusNoContent)
 	}
-
-	ctx.Status(http.StatusNoContent)
 }
 
 // DeleteSecret delete one secret of the repository
@@ -102,26 +105,23 @@ func DeleteSecret(ctx *context.APIContext) {
 	// responses:
 	//   "204":
 	//     description: delete one secret of the organization
-	//   "403":
-	//     "$ref": "#/responses/forbidden"
+	//   "400":
+	//     "$ref": "#/responses/error"
+	//   "404":
+	//     "$ref": "#/responses/notFound"
 
 	owner := ctx.Repo.Owner
 	repo := ctx.Repo.Repository
 
-	secretName := ctx.Params(":secretname")
-	if err := actions.NameRegexMatch(secretName); err != nil {
-		ctx.Error(http.StatusBadRequest, "DeleteSecret", err)
-		return
-	}
-	err := secret_model.DeleteSecret(
-		ctx, owner.ID, repo.ID, secretName,
-	)
-	if secret_model.IsErrSecretNotFound(err) {
-		ctx.NotFound(err)
-		return
-	}
+	err := secret_service.DeleteSecretByName(ctx, owner.ID, repo.ID, ctx.Params("secretname"))
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "DeleteSecret", err)
+		if errors.Is(err, util.ErrInvalidArgument) {
+			ctx.Error(http.StatusBadRequest, "DeleteSecret", err)
+		} else if errors.Is(err, util.ErrNotExist) {
+			ctx.Error(http.StatusNotFound, "DeleteSecret", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "DeleteSecret", err)
+		}
 		return
 	}
 
diff --git a/routers/api/v1/user/action.go b/routers/api/v1/user/action.go
index 885e411462..cbe332a779 100644
--- a/routers/api/v1/user/action.go
+++ b/routers/api/v1/user/action.go
@@ -4,13 +4,14 @@
 package user
 
 import (
+	"errors"
 	"net/http"
 
-	secret_model "code.gitea.io/gitea/models/secret"
 	"code.gitea.io/gitea/modules/context"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
-	"code.gitea.io/gitea/routers/web/shared/actions"
+	secret_service "code.gitea.io/gitea/services/secrets"
 )
 
 // create or update one secret of the user scope
@@ -42,23 +43,25 @@ func CreateOrUpdateSecret(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
-	secretName := ctx.Params(":secretname")
-	if err := actions.NameRegexMatch(secretName); err != nil {
-		ctx.Error(http.StatusBadRequest, "CreateOrUpdateSecret", err)
-		return
-	}
 	opt := web.GetForm(ctx).(*api.CreateOrUpdateSecretOption)
-	isCreated, err := secret_model.CreateOrUpdateSecret(ctx, ctx.Doer.ID, 0, secretName, opt.Data)
+
+	_, created, err := secret_service.CreateOrUpdateSecret(ctx, ctx.Doer.ID, 0, ctx.Params("secretname"), opt.Data)
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "CreateOrUpdateSecret", err)
+		if errors.Is(err, util.ErrInvalidArgument) {
+			ctx.Error(http.StatusBadRequest, "CreateOrUpdateSecret", err)
+		} else if errors.Is(err, util.ErrNotExist) {
+			ctx.Error(http.StatusNotFound, "CreateOrUpdateSecret", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "CreateOrUpdateSecret", err)
+		}
 		return
 	}
-	if isCreated {
+
+	if created {
 		ctx.Status(http.StatusCreated)
-		return
+	} else {
+		ctx.Status(http.StatusNoContent)
 	}
-
-	ctx.Status(http.StatusNoContent)
 }
 
 // DeleteSecret delete one secret of the user scope
@@ -84,20 +87,15 @@ func DeleteSecret(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
-	secretName := ctx.Params(":secretname")
-	if err := actions.NameRegexMatch(secretName); err != nil {
-		ctx.Error(http.StatusBadRequest, "DeleteSecret", err)
-		return
-	}
-	err := secret_model.DeleteSecret(
-		ctx, ctx.Doer.ID, 0, secretName,
-	)
-	if secret_model.IsErrSecretNotFound(err) {
-		ctx.NotFound(err)
-		return
-	}
+	err := secret_service.DeleteSecretByName(ctx, ctx.Doer.ID, 0, ctx.Params("secretname"))
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "DeleteSecret", err)
+		if errors.Is(err, util.ErrInvalidArgument) {
+			ctx.Error(http.StatusBadRequest, "DeleteSecret", err)
+		} else if errors.Is(err, util.ErrNotExist) {
+			ctx.Error(http.StatusNotFound, "DeleteSecret", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "DeleteSecret", err)
+		}
 		return
 	}
 
diff --git a/routers/web/shared/actions/variables.go b/routers/web/shared/actions/variables.go
index 8d1516c91c..341c18f589 100644
--- a/routers/web/shared/actions/variables.go
+++ b/routers/web/shared/actions/variables.go
@@ -14,6 +14,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/services/forms"
+	secret_service "code.gitea.io/gitea/services/secrets"
 )
 
 func SetVariablesContext(ctx *context.Context, ownerID, repoID int64) {
@@ -33,20 +34,9 @@ func SetVariablesContext(ctx *context.Context, ownerID, repoID int64) {
 // https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables
 // https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets
 var (
-	nameRx            = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$")
-	forbiddenPrefixRx = regexp.MustCompile("(?i)^GIT(EA|HUB)_")
-
 	forbiddenEnvNameCIRx = regexp.MustCompile("(?i)^CI")
 )
 
-func NameRegexMatch(name string) error {
-	if !nameRx.MatchString(name) || forbiddenPrefixRx.MatchString(name) {
-		log.Error("Name %s, regex match error", name)
-		return errors.New("name has invalid character")
-	}
-	return nil
-}
-
 func envNameCIRegexMatch(name string) error {
 	if forbiddenEnvNameCIRx.MatchString(name) {
 		log.Error("Env Name cannot be ci")
@@ -58,7 +48,7 @@ func envNameCIRegexMatch(name string) error {
 func CreateVariable(ctx *context.Context, ownerID, repoID int64, redirectURL string) {
 	form := web.GetForm(ctx).(*forms.EditVariableForm)
 
-	if err := NameRegexMatch(form.Name); err != nil {
+	if err := secret_service.ValidateName(form.Name); err != nil {
 		ctx.JSONError(err.Error())
 		return
 	}
@@ -82,7 +72,7 @@ func UpdateVariable(ctx *context.Context, redirectURL string) {
 	id := ctx.ParamsInt64(":variable_id")
 	form := web.GetForm(ctx).(*forms.EditVariableForm)
 
-	if err := NameRegexMatch(form.Name); err != nil {
+	if err := secret_service.ValidateName(form.Name); err != nil {
 		ctx.JSONError(err.Error())
 		return
 	}
diff --git a/routers/web/shared/secrets/secrets.go b/routers/web/shared/secrets/secrets.go
index c09ce51499..875cb0cfec 100644
--- a/routers/web/shared/secrets/secrets.go
+++ b/routers/web/shared/secrets/secrets.go
@@ -4,13 +4,13 @@
 package secrets
 
 import (
-	"code.gitea.io/gitea/models/db"
 	secret_model "code.gitea.io/gitea/models/secret"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/web/shared/actions"
 	"code.gitea.io/gitea/services/forms"
+	secret_service "code.gitea.io/gitea/services/secrets"
 )
 
 func SetSecretsContext(ctx *context.Context, ownerID, repoID int64) {
@@ -26,14 +26,9 @@ func SetSecretsContext(ctx *context.Context, ownerID, repoID int64) {
 func PerformSecretsPost(ctx *context.Context, ownerID, repoID int64, redirectURL string) {
 	form := web.GetForm(ctx).(*forms.AddSecretForm)
 
-	if err := actions.NameRegexMatch(form.Name); err != nil {
-		ctx.JSONError(ctx.Tr("secrets.creation.failed"))
-		return
-	}
-
-	s, err := secret_model.InsertEncryptedSecret(ctx, ownerID, repoID, form.Name, actions.ReserveLineBreakForTextarea(form.Data))
+	s, _, err := secret_service.CreateOrUpdateSecret(ctx, ownerID, repoID, form.Name, actions.ReserveLineBreakForTextarea(form.Data))
 	if err != nil {
-		log.Error("InsertEncryptedSecret: %v", err)
+		log.Error("CreateOrUpdateSecret failed: %v", err)
 		ctx.JSONError(ctx.Tr("secrets.creation.failed"))
 		return
 	}
@@ -45,11 +40,13 @@ func PerformSecretsPost(ctx *context.Context, ownerID, repoID int64, redirectURL
 func PerformSecretsDelete(ctx *context.Context, ownerID, repoID int64, redirectURL string) {
 	id := ctx.FormInt64("id")
 
-	if _, err := db.DeleteByBean(ctx, &secret_model.Secret{ID: id, OwnerID: ownerID, RepoID: repoID}); err != nil {
-		log.Error("Delete secret %d failed: %v", id, err)
+	err := secret_service.DeleteSecretByID(ctx, ownerID, repoID, id)
+	if err != nil {
+		log.Error("DeleteSecretByID(%d) failed: %v", id, err)
 		ctx.JSONError(ctx.Tr("secrets.deletion.failed"))
 		return
 	}
+
 	ctx.Flash.Success(ctx.Tr("secrets.deletion.success"))
 	ctx.JSONRedirect(redirectURL)
 }