Clarify Actions resources ownership

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)
 }
 

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)

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,39 +40,40 @@ 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
 }
 
 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)})

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,39 +77,32 @@ 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)
 }
 
 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})
 	}

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{