Clarify Actions resources ownership (#31724)
Fix #31707. Also related to #31715. Some Actions resources could has different types of ownership. It could be: - global: all repos and orgs/users can use it. - org/user level: only the org/user can use it. - repo level: only the repo can use it. There are two ways to distinguish org/user level from repo level: 1. `{owner_id: 1, repo_id: 2}` for repo level, and `{owner_id: 1, repo_id: 0}` for org level. 2. `{owner_id: 0, repo_id: 2}` for repo level, and `{owner_id: 1, repo_id: 0}` for org level. The first way seems more reasonable, but it may not be true. The point is that although a resource, like a runner, belongs to a repo (it can be used by the repo), the runner doesn't belong to the repo's org (other repos in the same org cannot use the runner). So, the second method makes more sense. And the first way is not user-friendly to query, we must set the repo id to zero to avoid wrong results. So, #31715 should be right. And the most simple way to fix #31707 is just: ```diff - shared.GetRegistrationToken(ctx, ctx.Repo.Repository.OwnerID, ctx.Repo.Repository.ID) + shared.GetRegistrationToken(ctx, 0, ctx.Repo.Repository.ID) ``` However, it is quite intuitive to set both owner id and repo id since the repo belongs to the owner. So I prefer to be compatible with it. If we get both owner id and repo id not zero when creating or finding, it's very clear that the caller want one with repo level, but set owner id accidentally. So it's OK to accept it but fix the owner id to zero. (cherry picked from commit a33e74d40d356e8f628ac06a131cb203a3609dec)
This commit is contained in:
Jason Song
Earl Warren
parent
2302cf63c8
commit
6844258c67
5 changed files with 103 additions and 38 deletions
|
|
@ -26,14 +26,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
|
||||
|
|
@ -176,7 +187,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]
|
||||
|
|
@ -193,8 +204,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})
|
||||
|
|
@ -297,6 +307,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)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
return nil
|
||||
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
|
||||
}
|
||||
|
||||
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)})
|
||||
|
|
|
|||
|
|
@ -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})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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{
|
||||
|
|
|
|||
Loading…
Reference in a new issue