Enhance routers for the Actions variable operations (#33547)

- Find the variable before updating or deleting
- Move the main logic from `routers/web/repo/setting/variables.go` to
`routers/web/shared/actions/variables.go`.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Giteabot <teabot@gitea.io>

models/actions/variable.go

@@ -58,6 +58,7 @@ func InsertVariable(ctx context.Context, ownerID, repoID int64, name, data strin
 
 type FindVariablesOpts struct {
 	db.ListOptions
+	IDs     []int64
 	RepoID  int64
 	OwnerID int64 // it will be ignored if RepoID is set
 	Name    string
@@ -65,6 +66,15 @@ type FindVariablesOpts struct {
 
 func (opts FindVariablesOpts) ToConds() builder.Cond {
 	cond := builder.NewCond()
+
+	if len(opts.IDs) > 0 {
+		if len(opts.IDs) == 1 {
+			cond = cond.And(builder.Eq{"id": opts.IDs[0]})
+		} else {
+			cond = cond.And(builder.In("id", opts.IDs))
+		}
+	}
+
 	// 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{"repo_id": opts.RepoID})
@@ -85,12 +95,12 @@ func FindVariables(ctx context.Context, opts FindVariablesOpts) ([]*ActionVariab
 	return db.Find[ActionVariable](ctx, opts)
 }
 
-func UpdateVariable(ctx context.Context, variable *ActionVariable) (bool, error) {
+func UpdateVariableCols(ctx context.Context, variable *ActionVariable, cols ...string) (bool, error) {
-	count, err := db.GetEngine(ctx).ID(variable.ID).Cols("name", "data").
+	variable.Name = strings.ToUpper(variable.Name)
-		Update(&ActionVariable{
+	count, err := db.GetEngine(ctx).
-			Name: variable.Name,
+		ID(variable.ID).
-			Data: variable.Data,
+		Cols(cols...).
-		})
+		Update(variable)
 	return count != 0, err
 }
 

routers/api/v1/org/action.go

@@ -450,7 +450,11 @@ func (Action) UpdateVariable(ctx *context.APIContext) {
 	if opt.Name == "" {
 		opt.Name = ctx.PathParam("variablename")
 	}
-	if _, err := actions_service.UpdateVariable(ctx, v.ID, opt.Name, opt.Value); err != nil {
+
+	v.Name = opt.Name
+	v.Data = opt.Value
+
+	if _, err := actions_service.UpdateVariableNameData(ctx, v); err != nil {
 		if errors.Is(err, util.ErrInvalidArgument) {
 			ctx.Error(http.StatusBadRequest, "UpdateVariable", err)
 		} else {

routers/api/v1/repo/action.go

@@ -417,7 +417,11 @@ func (Action) UpdateVariable(ctx *context.APIContext) {
 	if opt.Name == "" {
 		opt.Name = ctx.PathParam("variablename")
 	}
-	if _, err := actions_service.UpdateVariable(ctx, v.ID, opt.Name, opt.Value); err != nil {
+
+	v.Name = opt.Name
+	v.Data = opt.Value
+
+	if _, err := actions_service.UpdateVariableNameData(ctx, v); err != nil {
 		if errors.Is(err, util.ErrInvalidArgument) {
 			ctx.Error(http.StatusBadRequest, "UpdateVariable", err)
 		} else {

routers/api/v1/user/action.go

@@ -212,7 +212,11 @@ func UpdateVariable(ctx *context.APIContext) {
 	if opt.Name == "" {
 		opt.Name = ctx.PathParam("variablename")
 	}
-	if _, err := actions_service.UpdateVariable(ctx, v.ID, opt.Name, opt.Value); err != nil {
+
+	v.Name = opt.Name
+	v.Data = opt.Value
+
+	if _, err := actions_service.UpdateVariableNameData(ctx, v); err != nil {
 		if errors.Is(err, util.ErrInvalidArgument) {
 			ctx.Error(http.StatusBadRequest, "UpdateVariable", err)
 		} else {

routers/web/repo/setting/variables.go

@@ -1,140 +0,0 @@
-// Copyright 2023 The Gitea Authors. All rights reserved.
-// SPDX-License-Identifier: MIT
-
-package setting
-
-import (
-	"errors"
-	"net/http"
-
-	"code.gitea.io/gitea/modules/setting"
-	"code.gitea.io/gitea/modules/templates"
-	shared "code.gitea.io/gitea/routers/web/shared/actions"
-	shared_user "code.gitea.io/gitea/routers/web/shared/user"
-	"code.gitea.io/gitea/services/context"
-)
-
-const (
-	tplRepoVariables  templates.TplName = "repo/settings/actions"
-	tplOrgVariables   templates.TplName = "org/settings/actions"
-	tplUserVariables  templates.TplName = "user/settings/actions"
-	tplAdminVariables templates.TplName = "admin/actions"
-)
-
-type variablesCtx struct {
-	OwnerID           int64
-	RepoID            int64
-	IsRepo            bool
-	IsOrg             bool
-	IsUser            bool
-	IsGlobal          bool
-	VariablesTemplate templates.TplName
-	RedirectLink      string
-}
-
-func getVariablesCtx(ctx *context.Context) (*variablesCtx, error) {
-	if ctx.Data["PageIsRepoSettings"] == true {
-		return &variablesCtx{
-			OwnerID:           0,
-			RepoID:            ctx.Repo.Repository.ID,
-			IsRepo:            true,
-			VariablesTemplate: tplRepoVariables,
-			RedirectLink:      ctx.Repo.RepoLink + "/settings/actions/variables",
-		}, nil
-	}
-
-	if ctx.Data["PageIsOrgSettings"] == true {
-		err := shared_user.LoadHeaderCount(ctx)
-		if err != nil {
-			ctx.ServerError("LoadHeaderCount", err)
-			return nil, nil
-		}
-		return &variablesCtx{
-			OwnerID:           ctx.ContextUser.ID,
-			RepoID:            0,
-			IsOrg:             true,
-			VariablesTemplate: tplOrgVariables,
-			RedirectLink:      ctx.Org.OrgLink + "/settings/actions/variables",
-		}, nil
-	}
-
-	if ctx.Data["PageIsUserSettings"] == true {
-		return &variablesCtx{
-			OwnerID:           ctx.Doer.ID,
-			RepoID:            0,
-			IsUser:            true,
-			VariablesTemplate: tplUserVariables,
-			RedirectLink:      setting.AppSubURL + "/user/settings/actions/variables",
-		}, nil
-	}
-
-	if ctx.Data["PageIsAdmin"] == true {
-		return &variablesCtx{
-			OwnerID:           0,
-			RepoID:            0,
-			IsGlobal:          true,
-			VariablesTemplate: tplAdminVariables,
-			RedirectLink:      setting.AppSubURL + "/-/admin/actions/variables",
-		}, nil
-	}
-
-	return nil, errors.New("unable to set Variables context")
-}
-
-func Variables(ctx *context.Context) {
-	ctx.Data["Title"] = ctx.Tr("actions.variables")
-	ctx.Data["PageType"] = "variables"
-	ctx.Data["PageIsSharedSettingsVariables"] = true
-
-	vCtx, err := getVariablesCtx(ctx)
-	if err != nil {
-		ctx.ServerError("getVariablesCtx", err)
-		return
-	}
-
-	shared.SetVariablesContext(ctx, vCtx.OwnerID, vCtx.RepoID)
-	if ctx.Written() {
-		return
-	}
-
-	ctx.HTML(http.StatusOK, vCtx.VariablesTemplate)
-}
-
-func VariableCreate(ctx *context.Context) {
-	vCtx, err := getVariablesCtx(ctx)
-	if err != nil {
-		ctx.ServerError("getVariablesCtx", err)
-		return
-	}
-
-	if ctx.HasError() { // form binding validation error
-		ctx.JSONError(ctx.GetErrMsg())
-		return
-	}
-
-	shared.CreateVariable(ctx, vCtx.OwnerID, vCtx.RepoID, vCtx.RedirectLink)
-}
-
-func VariableUpdate(ctx *context.Context) {
-	vCtx, err := getVariablesCtx(ctx)
-	if err != nil {
-		ctx.ServerError("getVariablesCtx", err)
-		return
-	}
-
-	if ctx.HasError() { // form binding validation error
-		ctx.JSONError(ctx.GetErrMsg())
-		return
-	}
-
-	shared.UpdateVariable(ctx, vCtx.RedirectLink)
-}
-
-func VariableDelete(ctx *context.Context) {
-	vCtx, err := getVariablesCtx(ctx)
-	if err != nil {
-		ctx.ServerError("getVariablesCtx", err)
-		return
-	}
-	shared.DeleteVariable(ctx, vCtx.RedirectLink)
-}

routers/web/shared/actions/variables.go

@@ -4,31 +4,127 @@
 package actions
 
 import (
+	"errors"
+	"net/http"
+
 	actions_model "code.gitea.io/gitea/models/actions"
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/templates"
 	"code.gitea.io/gitea/modules/web"
+	shared_user "code.gitea.io/gitea/routers/web/shared/user"
 	actions_service "code.gitea.io/gitea/services/actions"
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/forms"
 )
 
-func SetVariablesContext(ctx *context.Context, ownerID, repoID int64) {
+const (
+	tplRepoVariables  templates.TplName = "repo/settings/actions"
+	tplOrgVariables   templates.TplName = "org/settings/actions"
+	tplUserVariables  templates.TplName = "user/settings/actions"
+	tplAdminVariables templates.TplName = "admin/actions"
+)
+
+type variablesCtx struct {
+	OwnerID           int64
+	RepoID            int64
+	IsRepo            bool
+	IsOrg             bool
+	IsUser            bool
+	IsGlobal          bool
+	VariablesTemplate templates.TplName
+	RedirectLink      string
+}
+
+func getVariablesCtx(ctx *context.Context) (*variablesCtx, error) {
+	if ctx.Data["PageIsRepoSettings"] == true {
+		return &variablesCtx{
+			OwnerID:           0,
+			RepoID:            ctx.Repo.Repository.ID,
+			IsRepo:            true,
+			VariablesTemplate: tplRepoVariables,
+			RedirectLink:      ctx.Repo.RepoLink + "/settings/actions/variables",
+		}, nil
+	}
+
+	if ctx.Data["PageIsOrgSettings"] == true {
+		err := shared_user.LoadHeaderCount(ctx)
+		if err != nil {
+			ctx.ServerError("LoadHeaderCount", err)
+			return nil, nil
+		}
+		return &variablesCtx{
+			OwnerID:           ctx.ContextUser.ID,
+			RepoID:            0,
+			IsOrg:             true,
+			VariablesTemplate: tplOrgVariables,
+			RedirectLink:      ctx.Org.OrgLink + "/settings/actions/variables",
+		}, nil
+	}
+
+	if ctx.Data["PageIsUserSettings"] == true {
+		return &variablesCtx{
+			OwnerID:           ctx.Doer.ID,
+			RepoID:            0,
+			IsUser:            true,
+			VariablesTemplate: tplUserVariables,
+			RedirectLink:      setting.AppSubURL + "/user/settings/actions/variables",
+		}, nil
+	}
+
+	if ctx.Data["PageIsAdmin"] == true {
+		return &variablesCtx{
+			OwnerID:           0,
+			RepoID:            0,
+			IsGlobal:          true,
+			VariablesTemplate: tplAdminVariables,
+			RedirectLink:      setting.AppSubURL + "/-/admin/actions/variables",
+		}, nil
+	}
+
+	return nil, errors.New("unable to set Variables context")
+}
+
+func Variables(ctx *context.Context) {
+	ctx.Data["Title"] = ctx.Tr("actions.variables")
+	ctx.Data["PageType"] = "variables"
+	ctx.Data["PageIsSharedSettingsVariables"] = true
+
+	vCtx, err := getVariablesCtx(ctx)
+	if err != nil {
+		ctx.ServerError("getVariablesCtx", err)
+		return
+	}
+
 	variables, err := db.Find[actions_model.ActionVariable](ctx, actions_model.FindVariablesOpts{
-		OwnerID: ownerID,
+		OwnerID: vCtx.OwnerID,
-		RepoID:  repoID,
+		RepoID:  vCtx.RepoID,
 	})
 	if err != nil {
 		ctx.ServerError("FindVariables", err)
 		return
 	}
 	ctx.Data["Variables"] = variables
+
+	ctx.HTML(http.StatusOK, vCtx.VariablesTemplate)
 }
 
-func CreateVariable(ctx *context.Context, ownerID, repoID int64, redirectURL string) {
+func VariableCreate(ctx *context.Context) {
+	vCtx, err := getVariablesCtx(ctx)
+	if err != nil {
+		ctx.ServerError("getVariablesCtx", err)
+		return
+	}
+
+	if ctx.HasError() { // form binding validation error
+		ctx.JSONError(ctx.GetErrMsg())
+		return
+	}
+
 	form := web.GetForm(ctx).(*forms.EditVariableForm)
 
-	v, err := actions_service.CreateVariable(ctx, ownerID, repoID, form.Name, form.Data)
+	v, err := actions_service.CreateVariable(ctx, vCtx.OwnerID, vCtx.RepoID, form.Name, form.Data)
 	if err != nil {
 		log.Error("CreateVariable: %v", err)
 		ctx.JSONError(ctx.Tr("actions.variables.creation.failed"))
@@ -36,30 +132,92 @@ func CreateVariable(ctx *context.Context, ownerID, repoID int64, redirectURL str
 	}
 
 	ctx.Flash.Success(ctx.Tr("actions.variables.creation.success", v.Name))
-	ctx.JSONRedirect(redirectURL)
+	ctx.JSONRedirect(vCtx.RedirectLink)
 }
 
-func UpdateVariable(ctx *context.Context, redirectURL string) {
+func VariableUpdate(ctx *context.Context) {
-	id := ctx.PathParamInt64("variable_id")
+	vCtx, err := getVariablesCtx(ctx)
-	form := web.GetForm(ctx).(*forms.EditVariableForm)
+	if err != nil {
+		ctx.ServerError("getVariablesCtx", err)
+		return
+	}
 
-	if ok, err := actions_service.UpdateVariable(ctx, id, form.Name, form.Data); err != nil || !ok {
+	if ctx.HasError() { // form binding validation error
+		ctx.JSONError(ctx.GetErrMsg())
+		return
+	}
+
+	id := ctx.PathParamInt64("variable_id")
+
+	variable := findActionsVariable(ctx, id, vCtx)
+	if ctx.Written() {
+		return
+	}
+
+	form := web.GetForm(ctx).(*forms.EditVariableForm)
+	variable.Name = form.Name
+	variable.Data = form.Data
+
+	if ok, err := actions_service.UpdateVariableNameData(ctx, variable); err != nil || !ok {
 		log.Error("UpdateVariable: %v", err)
 		ctx.JSONError(ctx.Tr("actions.variables.update.failed"))
 		return
 	}
 	ctx.Flash.Success(ctx.Tr("actions.variables.update.success"))
-	ctx.JSONRedirect(redirectURL)
+	ctx.JSONRedirect(vCtx.RedirectLink)
 }
 
-func DeleteVariable(ctx *context.Context, redirectURL string) {
+func findActionsVariable(ctx *context.Context, id int64, vCtx *variablesCtx) *actions_model.ActionVariable {
+	opts := actions_model.FindVariablesOpts{
+		IDs: []int64{id},
+	}
+	switch {
+	case vCtx.IsRepo:
+		opts.RepoID = vCtx.RepoID
+		if opts.RepoID == 0 {
+			panic("RepoID is 0")
+		}
+	case vCtx.IsOrg, vCtx.IsUser:
+		opts.OwnerID = vCtx.OwnerID
+		if opts.OwnerID == 0 {
+			panic("OwnerID is 0")
+		}
+	case vCtx.IsGlobal:
+		// do nothing
+	default:
+		panic("invalid actions variable")
+	}
+
+	got, err := actions_model.FindVariables(ctx, opts)
+	if err != nil {
+		ctx.ServerError("FindVariables", err)
+		return nil
+	} else if len(got) == 0 {
+		ctx.NotFound("FindVariables", nil)
+		return nil
+	}
+	return got[0]
+}
+
+func VariableDelete(ctx *context.Context) {
+	vCtx, err := getVariablesCtx(ctx)
+	if err != nil {
+		ctx.ServerError("getVariablesCtx", err)
+		return
+	}
+
 	id := ctx.PathParamInt64("variable_id")
 
-	if err := actions_service.DeleteVariableByID(ctx, id); err != nil {
+	variable := findActionsVariable(ctx, id, vCtx)
+	if ctx.Written() {
+		return
+	}
+
+	if err := actions_service.DeleteVariableByID(ctx, variable.ID); err != nil {
 		log.Error("Delete variable [%d] failed: %v", id, err)
 		ctx.JSONError(ctx.Tr("actions.variables.deletion.failed"))
 		return
 	}
 	ctx.Flash.Success(ctx.Tr("actions.variables.deletion.success"))
-	ctx.JSONRedirect(redirectURL)
+	ctx.JSONRedirect(vCtx.RedirectLink)
 }

routers/web/web.go

@@ -37,6 +37,7 @@ import (
 	"code.gitea.io/gitea/routers/web/repo"
 	"code.gitea.io/gitea/routers/web/repo/actions"
 	repo_setting "code.gitea.io/gitea/routers/web/repo/setting"
+	shared_actions "code.gitea.io/gitea/routers/web/shared/actions"
 	"code.gitea.io/gitea/routers/web/shared/project"
 	"code.gitea.io/gitea/routers/web/user"
 	user_setting "code.gitea.io/gitea/routers/web/user/setting"
@@ -449,10 +450,10 @@ func registerRoutes(m *web.Router) {
 
 	addSettingsVariablesRoutes := func() {
 		m.Group("/variables", func() {
-			m.Get("", repo_setting.Variables)
+			m.Get("", shared_actions.Variables)
-			m.Post("/new", web.Bind(forms.EditVariableForm{}), repo_setting.VariableCreate)
+			m.Post("/new", web.Bind(forms.EditVariableForm{}), shared_actions.VariableCreate)
-			m.Post("/{variable_id}/edit", web.Bind(forms.EditVariableForm{}), repo_setting.VariableUpdate)
+			m.Post("/{variable_id}/edit", web.Bind(forms.EditVariableForm{}), shared_actions.VariableUpdate)
-			m.Post("/{variable_id}/delete", repo_setting.VariableDelete)
+			m.Post("/{variable_id}/delete", shared_actions.VariableDelete)
 		})
 	}
 

services/actions/variables.go

@@ -6,7 +6,6 @@ package actions
 import (
 	"context"
 	"regexp"
-	"strings"
 
 	actions_model "code.gitea.io/gitea/models/actions"
 	"code.gitea.io/gitea/modules/log"
@@ -31,20 +30,18 @@ func CreateVariable(ctx context.Context, ownerID, repoID int64, name, data strin
 	return v, nil
 }
 
-func UpdateVariable(ctx context.Context, variableID int64, name, data string) (bool, error) {
+func UpdateVariableNameData(ctx context.Context, variable *actions_model.ActionVariable) (bool, error) {
-	if err := secret_service.ValidateName(name); err != nil {
+	if err := secret_service.ValidateName(variable.Name); err != nil {
 		return false, err
 	}
 
-	if err := envNameCIRegexMatch(name); err != nil {
+	if err := envNameCIRegexMatch(variable.Name); err != nil {
 		return false, err
 	}
 
-	return actions_model.UpdateVariable(ctx, &actions_model.ActionVariable{
+	variable.Data = util.ReserveLineBreakForTextarea(variable.Data)
-		ID:   variableID,
+
-		Name: strings.ToUpper(name),
+	return actions_model.UpdateVariableCols(ctx, variable, "name", "data")
-		Data: util.ReserveLineBreakForTextarea(data),
-	})
 }
 
 func DeleteVariableByID(ctx context.Context, variableID int64) error {

tests/integration/actions_variables_test.go

@@ -0,0 +1,149 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"testing"
+
+	actions_model "code.gitea.io/gitea/models/actions"
+	"code.gitea.io/gitea/models/db"
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/tests"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestActionsVariables(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	ctx := context.Background()
+
+	require.NoError(t, db.DeleteAllRecords("action_variable"))
+
+	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	_, _ = actions_model.InsertVariable(ctx, user2.ID, 0, "VAR", "user2-var")
+	user2Var := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{OwnerID: user2.ID, Name: "VAR"})
+	userWebURL := "/user/settings/actions/variables"
+
+	org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization})
+	_, _ = actions_model.InsertVariable(ctx, org3.ID, 0, "VAR", "org3-var")
+	org3Var := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{OwnerID: org3.ID, Name: "VAR"})
+	orgWebURL := "/org/org3/settings/actions/variables"
+
+	repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+	_, _ = actions_model.InsertVariable(ctx, 0, repo1.ID, "VAR", "repo1-var")
+	repo1Var := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{RepoID: repo1.ID, Name: "VAR"})
+	repoWebURL := "/user2/repo1/settings/actions/variables"
+
+	_, _ = actions_model.InsertVariable(ctx, 0, 0, "VAR", "global-var")
+	globalVar := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{Name: "VAR", Data: "global-var"})
+	adminWebURL := "/-/admin/actions/variables"
+
+	sessionAdmin := loginUser(t, "user1")
+	sessionUser2 := loginUser(t, user2.Name)
+
+	doUpdate := func(t *testing.T, sess *TestSession, baseURL string, id int64, data string, expectedStatus int) {
+		req := NewRequestWithValues(t, "POST", fmt.Sprintf("%s/%d/edit", baseURL, id), map[string]string{
+			"_csrf": GetUserCSRFToken(t, sess),
+			"name":  "VAR",
+			"data":  data,
+		})
+		sess.MakeRequest(t, req, expectedStatus)
+	}
+
+	doDelete := func(t *testing.T, sess *TestSession, baseURL string, id int64, expectedStatus int) {
+		req := NewRequestWithValues(t, "POST", fmt.Sprintf("%s/%d/delete", baseURL, id), map[string]string{
+			"_csrf": GetUserCSRFToken(t, sess),
+		})
+		sess.MakeRequest(t, req, expectedStatus)
+	}
+
+	assertDenied := func(t *testing.T, sess *TestSession, baseURL string, id int64) {
+		doUpdate(t, sess, baseURL, id, "ChangedData", http.StatusNotFound)
+		doDelete(t, sess, baseURL, id, http.StatusNotFound)
+		v := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{ID: id})
+		assert.Contains(t, v.Data, "-var")
+	}
+
+	assertSuccess := func(t *testing.T, sess *TestSession, baseURL string, id int64) {
+		doUpdate(t, sess, baseURL, id, "ChangedData", http.StatusOK)
+		v := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{ID: id})
+		assert.Equal(t, "ChangedData", v.Data)
+		doDelete(t, sess, baseURL, id, http.StatusOK)
+		unittest.AssertNotExistsBean(t, &actions_model.ActionVariable{ID: id})
+	}
+
+	t.Run("UpdateUserVar", func(t *testing.T) {
+		theVar := user2Var
+		t.Run("FromOrg", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, orgWebURL, theVar.ID)
+		})
+		t.Run("FromRepo", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, repoWebURL, theVar.ID)
+		})
+		t.Run("FromAdmin", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, adminWebURL, theVar.ID)
+		})
+	})
+
+	t.Run("UpdateOrgVar", func(t *testing.T) {
+		theVar := org3Var
+		t.Run("FromRepo", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, repoWebURL, theVar.ID)
+		})
+		t.Run("FromUser", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, userWebURL, theVar.ID)
+		})
+		t.Run("FromAdmin", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, adminWebURL, theVar.ID)
+		})
+	})
+
+	t.Run("UpdateRepoVar", func(t *testing.T) {
+		theVar := repo1Var
+		t.Run("FromOrg", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, orgWebURL, theVar.ID)
+		})
+		t.Run("FromUser", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, userWebURL, theVar.ID)
+		})
+		t.Run("FromAdmin", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, adminWebURL, theVar.ID)
+		})
+	})
+
+	t.Run("UpdateGlobalVar", func(t *testing.T) {
+		theVar := globalVar
+		t.Run("FromOrg", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, orgWebURL, theVar.ID)
+		})
+		t.Run("FromUser", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, userWebURL, theVar.ID)
+		})
+		t.Run("FromRepo", func(t *testing.T) {
+			assertDenied(t, sessionAdmin, repoWebURL, theVar.ID)
+		})
+	})
+
+	t.Run("UpdateSuccess", func(t *testing.T) {
+		t.Run("User", func(t *testing.T) {
+			assertSuccess(t, sessionUser2, userWebURL, user2Var.ID)
+		})
+		t.Run("Org", func(t *testing.T) {
+			assertSuccess(t, sessionAdmin, orgWebURL, org3Var.ID)
+		})
+		t.Run("Repo", func(t *testing.T) {
+			assertSuccess(t, sessionUser2, repoWebURL, repo1Var.ID)
+		})
+		t.Run("Admin", func(t *testing.T) {
+			assertSuccess(t, sessionAdmin, adminWebURL, globalVar.ID)
+		})
+	})
+}