diff options
| -rw-r--r-- | models/actions/runner.go | 25 | ||||
| -rw-r--r-- | models/actions/runner_token.go | 28 | ||||
| -rw-r--r-- | models/actions/variable.go | 36 | ||||
| -rw-r--r-- | models/secret/secret.go | 46 | ||||
| -rw-r--r-- | tests/integration/api_user_variables_test.go | 4 |
5 files changed, 102 insertions, 37 deletions
diff --git a/models/actions/runner.go b/models/actions/runner.go index 9192925d5a..2023ba4f99 100644 --- a/models/actions/runner.go +++ b/models/actions/runner.go @@ -23,14 +23,25 @@ import ( ) // ActionRunner represents runner machines +// +// It can be: +// 1. global runner, OwnerID is 0 and RepoID is 0 +// 2. org/user level runner, OwnerID is org/user ID and RepoID is 0 +// 3. repo level runner, OwnerID is 0 and RepoID is repo ID +// +// Please note that it's not acceptable to have both OwnerID and RepoID to be non-zero, +// or it will be complicated to find runners belonging to a specific owner. +// For example, conditions like `OwnerID = 1` will also return runner {OwnerID: 1, RepoID: 1}, +// but it's a repo level runner, not an org/user level runner. +// To avoid this, make it clear with {OwnerID: 0, RepoID: 1} for repo level runners. type ActionRunner struct { ID int64 UUID string `xorm:"CHAR(36) UNIQUE"` Name string `xorm:"VARCHAR(255)"` Version string `xorm:"VARCHAR(64)"` - OwnerID int64 `xorm:"index"` // org level runner, 0 means system + OwnerID int64 `xorm:"index"` Owner *user_model.User `xorm:"-"` - RepoID int64 `xorm:"index"` // repo level runner, if OwnerID also is zero, then it's a global + RepoID int64 `xorm:"index"` Repo *repo_model.Repository `xorm:"-"` Description string `xorm:"TEXT"` Base int // 0 native 1 docker 2 virtual machine @@ -157,7 +168,7 @@ func init() { type FindRunnerOptions struct { db.ListOptions RepoID int64 - OwnerID int64 + OwnerID int64 // it will be ignored if RepoID is set Sort string Filter string IsOnline optional.Option[bool] @@ -174,8 +185,7 @@ func (opts FindRunnerOptions) ToConds() builder.Cond { c = c.Or(builder.Eq{"repo_id": 0, "owner_id": 0}) } cond = cond.And(c) - } - if opts.OwnerID > 0 { + } else if opts.OwnerID > 0 { // OwnerID is ignored if RepoID is set c := builder.NewCond().And(builder.Eq{"owner_id": opts.OwnerID}) if opts.WithAvailable { c = c.Or(builder.Eq{"repo_id": 0, "owner_id": 0}) @@ -263,6 +273,11 @@ func DeleteRunner(ctx context.Context, id int64) error { // CreateRunner creates new runner. func CreateRunner(ctx context.Context, t *ActionRunner) error { + if t.OwnerID != 0 && t.RepoID != 0 { + // It's trying to create a runner that belongs to a repository, but OwnerID has been set accidentally. + // Remove OwnerID to avoid confusion; it's not worth returning an error here. + t.OwnerID = 0 + } return db.Insert(ctx, t) } diff --git a/models/actions/runner_token.go b/models/actions/runner_token.go index ccd9bbccb3..fd6ba7ecad 100644 --- a/models/actions/runner_token.go +++ b/models/actions/runner_token.go @@ -15,12 +15,23 @@ import ( ) // ActionRunnerToken represents runner tokens +// +// It can be: +// 1. global token, OwnerID is 0 and RepoID is 0 +// 2. org/user level token, OwnerID is org/user ID and RepoID is 0 +// 3. repo level token, OwnerID is 0 and RepoID is repo ID +// +// Please note that it's not acceptable to have both OwnerID and RepoID to be non-zero, +// or it will be complicated to find tokens belonging to a specific owner. +// For example, conditions like `OwnerID = 1` will also return token {OwnerID: 1, RepoID: 1}, +// but it's a repo level token, not an org/user level token. +// To avoid this, make it clear with {OwnerID: 0, RepoID: 1} for repo level tokens. type ActionRunnerToken struct { ID int64 Token string `xorm:"UNIQUE"` - OwnerID int64 `xorm:"index"` // org level runner, 0 means system + OwnerID int64 `xorm:"index"` Owner *user_model.User `xorm:"-"` - RepoID int64 `xorm:"index"` // repo level runner, if orgid also is zero, then it's a global + RepoID int64 `xorm:"index"` Repo *repo_model.Repository `xorm:"-"` IsActive bool // true means it can be used @@ -58,7 +69,14 @@ func UpdateRunnerToken(ctx context.Context, r *ActionRunnerToken, cols ...string } // NewRunnerToken creates a new active runner token and invalidate all old tokens +// ownerID will be ignored and treated as 0 if repoID is non-zero. func NewRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerToken, error) { + if ownerID != 0 && repoID != 0 { + // It's trying to create a runner token that belongs to a repository, but OwnerID has been set accidentally. + // Remove OwnerID to avoid confusion; it's not worth returning an error here. + ownerID = 0 + } + token, err := util.CryptoRandomString(40) if err != nil { return nil, err @@ -84,6 +102,12 @@ func NewRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerTo // GetLatestRunnerToken returns the latest runner token func GetLatestRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerToken, error) { + if ownerID != 0 && repoID != 0 { + // It's trying to get a runner token that belongs to a repository, but OwnerID has been set accidentally. + // Remove OwnerID to avoid confusion; it's not worth returning an error here. + ownerID = 0 + } + var runnerToken ActionRunnerToken has, err := db.GetEngine(ctx).Where("owner_id=? AND repo_id=?", ownerID, repoID). OrderBy("id DESC").Get(&runnerToken) diff --git a/models/actions/variable.go b/models/actions/variable.go index 8aff844659..d0f917d923 100644 --- a/models/actions/variable.go +++ b/models/actions/variable.go @@ -5,7 +5,6 @@ package actions import ( "context" - "errors" "strings" "code.gitea.io/gitea/models/db" @@ -15,6 +14,18 @@ import ( "xorm.io/builder" ) +// ActionVariable represents a variable that can be used in actions +// +// It can be: +// 1. global variable, OwnerID is 0 and RepoID is 0 +// 2. org/user level variable, OwnerID is org/user ID and RepoID is 0 +// 3. repo level variable, OwnerID is 0 and RepoID is repo ID +// +// Please note that it's not acceptable to have both OwnerID and RepoID to be non-zero, +// or it will be complicated to find variables belonging to a specific owner. +// For example, conditions like `OwnerID = 1` will also return variable {OwnerID: 1, RepoID: 1}, +// but it's a repo level variable, not an org/user level variable. +// To avoid this, make it clear with {OwnerID: 0, RepoID: 1} for repo level variables. type ActionVariable struct { ID int64 `xorm:"pk autoincr"` OwnerID int64 `xorm:"UNIQUE(owner_repo_name)"` @@ -29,30 +40,26 @@ func init() { db.RegisterModel(new(ActionVariable)) } -func (v *ActionVariable) Validate() error { - if v.OwnerID != 0 && v.RepoID != 0 { - return errors.New("a variable should not be bound to an owner and a repository at the same time") +func InsertVariable(ctx context.Context, ownerID, repoID int64, name, data string) (*ActionVariable, error) { + if ownerID != 0 && repoID != 0 { + // It's trying to create a variable that belongs to a repository, but OwnerID has been set accidentally. + // Remove OwnerID to avoid confusion; it's not worth returning an error here. + ownerID = 0 } - return nil -} -func InsertVariable(ctx context.Context, ownerID, repoID int64, name, data string) (*ActionVariable, error) { variable := &ActionVariable{ OwnerID: ownerID, RepoID: repoID, Name: strings.ToUpper(name), Data: data, } - if err := variable.Validate(); err != nil { - return variable, err - } return variable, db.Insert(ctx, variable) } type FindVariablesOpts struct { db.ListOptions - OwnerID int64 RepoID int64 + OwnerID int64 // it will be ignored if RepoID is set Name string } @@ -60,8 +67,13 @@ func (opts FindVariablesOpts) ToConds() builder.Cond { cond := builder.NewCond() // Since we now support instance-level variables, // there is no need to check for null values for `owner_id` and `repo_id` - cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) + if opts.RepoID != 0 { // if RepoID is set + // ignore OwnerID and treat it as 0 + cond = cond.And(builder.Eq{"owner_id": 0}) + } else { + cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) + } if opts.Name != "" { cond = cond.And(builder.Eq{"name": strings.ToUpper(opts.Name)}) diff --git a/models/secret/secret.go b/models/secret/secret.go index 35bed500b9..ce0ad65a79 100644 --- a/models/secret/secret.go +++ b/models/secret/secret.go @@ -5,7 +5,6 @@ package secret import ( "context" - "errors" "fmt" "strings" @@ -22,6 +21,19 @@ import ( ) // Secret represents a secret +// +// It can be: +// 1. org/user level secret, OwnerID is org/user ID and RepoID is 0 +// 2. repo level secret, OwnerID is 0 and RepoID is repo ID +// +// Please note that it's not acceptable to have both OwnerID and RepoID to be non-zero, +// or it will be complicated to find secrets belonging to a specific owner. +// For example, conditions like `OwnerID = 1` will also return secret {OwnerID: 1, RepoID: 1}, +// but it's a repo level secret, not an org/user level secret. +// To avoid this, make it clear with {OwnerID: 0, RepoID: 1} for repo level secrets. +// +// Please note that it's not acceptable to have both OwnerID and RepoID to zero, global secrets are not supported. +// It's for security reasons, admin may be not aware of that the secrets could be stolen by any user when setting them as global. type Secret struct { ID int64 OwnerID int64 `xorm:"INDEX UNIQUE(owner_repo_name) NOT NULL"` @@ -46,6 +58,15 @@ func (err ErrSecretNotFound) Unwrap() error { // InsertEncryptedSecret Creates, encrypts, and validates a new secret with yet unencrypted data and insert into database func InsertEncryptedSecret(ctx context.Context, ownerID, repoID int64, name, data string) (*Secret, error) { + if ownerID != 0 && repoID != 0 { + // It's trying to create a secret that belongs to a repository, but OwnerID has been set accidentally. + // Remove OwnerID to avoid confusion; it's not worth returning an error here. + ownerID = 0 + } + if ownerID == 0 && repoID == 0 { + return nil, fmt.Errorf("%w: ownerID and repoID cannot be both zero, global secrets are not supported", util.ErrInvalidArgument) + } + encrypted, err := secret_module.EncryptSecret(setting.SecretKey, data) if err != nil { return nil, err @@ -56,9 +77,6 @@ func InsertEncryptedSecret(ctx context.Context, ownerID, repoID int64, name, dat Name: strings.ToUpper(name), Data: encrypted, } - if err := secret.Validate(); err != nil { - return secret, err - } return secret, db.Insert(ctx, secret) } @@ -66,29 +84,25 @@ func init() { db.RegisterModel(new(Secret)) } -func (s *Secret) Validate() error { - if s.OwnerID == 0 && s.RepoID == 0 { - return errors.New("the secret is not bound to any scope") - } - return nil -} - type FindSecretsOptions struct { db.ListOptions - OwnerID int64 RepoID int64 + OwnerID int64 // it will be ignored if RepoID is set SecretID int64 Name string } func (opts FindSecretsOptions) ToConds() builder.Cond { cond := builder.NewCond() - if opts.OwnerID > 0 { + + cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) + if opts.RepoID != 0 { // if RepoID is set + // ignore OwnerID and treat it as 0 + cond = cond.And(builder.Eq{"owner_id": 0}) + } else { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } - if opts.RepoID > 0 { - cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) - } + if opts.SecretID != 0 { cond = cond.And(builder.Eq{"id": opts.SecretID}) } diff --git a/tests/integration/api_user_variables_test.go b/tests/integration/api_user_variables_test.go index dd5501f0b9..9fd84ddf81 100644 --- a/tests/integration/api_user_variables_test.go +++ b/tests/integration/api_user_variables_test.go @@ -19,7 +19,7 @@ func TestAPIUserVariables(t *testing.T) { session := loginUser(t, "user1") token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteUser) - t.Run("CreateRepoVariable", func(t *testing.T) { + t.Run("CreateUserVariable", func(t *testing.T) { cases := []struct { Name string ExpectedStatus int @@ -70,7 +70,7 @@ func TestAPIUserVariables(t *testing.T) { } }) - t.Run("UpdateRepoVariable", func(t *testing.T) { + t.Run("UpdateUserVariable", func(t *testing.T) { variableName := "test_update_var" url := fmt.Sprintf("/api/v1/user/actions/variables/%s", variableName) req := NewRequestWithJSON(t, "POST", url, api.CreateVariableOption{ |