enhance policy managment

pkg/api/policy/api.go

@@ -182,18 +182,21 @@ func (a *API) listAllPolicies() ([]policyTY.Policy, error) {
 // Save persists a user authored policy.
 //
 // System policies are code: their statements are rewritten on every start, so
-// accepting edits here would silently discard them. The System flag itself is never
-// taken from the request - otherwise a client could mark its own policy
-// undeletable.
+// accepting edits here would silently discard them. The System flag itself is
+// never taken from the request - a client cannot mark a custom policy undeletable
+// or create a policy with a built-in id.
 func (a *API) Save(policy *policyTY.Policy) error {
 	if policy.ID == "" {
 		policy.ID = utils.RandID()
 	}
 	existing, err := a.loadPolicyFromStorage(policy.ID)
 	isExisting := err == nil && existing.ID != ""
 
-	if isExisting && existing.System {
-		return fmt.Errorf("cannot modify system policy: %s", policy.ID)
+	if IsBuiltInPolicyID(policy.ID) || (isExisting && existing.System) {
+		return fmt.Errorf("%w: %s", ErrSystemPolicyImmutable, policy.ID)
+	}
+	if policy.System {
+		return fmt.Errorf("%w: %s", ErrSystemFlagNotAllowed, policy.ID)
 	}
 	policy.System = false
 
@@ -259,15 +262,11 @@ func (a *API) Import(data interface{}) error {
 	if !ok {
 		return fmt.Errorf("invalid type:%T", data)
 	}
-	if input.ID == "" {
-		input.ID = utils.RandID()
-	}
-	filters := []storageTY.Filter{{Key: types.KeyID, Value: input.ID}}
-	if err := a.storage.Upsert(types.EntityPolicy, &input, filters); err != nil {
-		return err
+	if IsBuiltInPolicyID(input.ID) {
+		return nil
 	}
-	a.cache.PutPolicy(&input)
-	return nil
+	input.System = false
+	return a.Save(&input)
 }
 
 func (a *API) GetEntityInterface() interface{} {
@@ -312,4 +311,8 @@ func (a *API) ValidatePoliciesExist(ids []string) error {
 	return nil
 }
 
-var ErrNotFound = errors.New("not found")
+var (
+	ErrNotFound              = errors.New("not found")
+	ErrSystemPolicyImmutable = errors.New("cannot modify system policy")
+	ErrSystemFlagNotAllowed  = errors.New("system flag is not allowed on custom policies")
+)

pkg/api/policy/api_save_test.go

@@ -0,0 +1,139 @@
+package policy
+
+import (
+	"errors"
+	"testing"
+
+	types "github.com/mycontroller-org/server/v2/pkg/types"
+	policyTY "github.com/mycontroller-org/server/v2/pkg/types/policy"
+	svcTokenTY "github.com/mycontroller-org/server/v2/pkg/types/service_token"
+	userTY "github.com/mycontroller-org/server/v2/pkg/types/user"
+	storageTY "github.com/mycontroller-org/server/v2/plugin/database/storage/types"
+)
+
+type policyMemStore struct {
+	policies map[string]policyTY.Policy
+}
+
+func newPolicyMemStore() *policyMemStore {
+	return &policyMemStore{policies: map[string]policyTY.Policy{}}
+}
+
+func (s *policyMemStore) Name() string { return "test" }
+func (s *policyMemStore) Ping() error  { return nil }
+func (s *policyMemStore) Close() error { return nil }
+func (s *policyMemStore) Insert(string, interface{}) error {
+	return errors.New("not implemented")
+}
+func (s *policyMemStore) Update(string, interface{}, []storageTY.Filter) error {
+	return errors.New("not implemented")
+}
+func (s *policyMemStore) Find(string, interface{}, []storageTY.Filter, *storageTY.Pagination) (*storageTY.Result, error) {
+	return nil, errors.New("not implemented")
+}
+func (s *policyMemStore) Delete(string, []storageTY.Filter) (int64, error) {
+	return 0, errors.New("not implemented")
+}
+func (s *policyMemStore) Pause() error                                 { return nil }
+func (s *policyMemStore) Resume() error                                { return nil }
+func (s *policyMemStore) ClearDatabase() error                         { return nil }
+func (s *policyMemStore) DoStartupImport() (bool, string, string)      { return false, "", "" }
+
+func (s *policyMemStore) FindOne(entityName string, out interface{}, filters []storageTY.Filter) error {
+	if entityName != types.EntityPolicy || len(filters) == 0 {
+		return storageTY.ErrNoDocuments
+	}
+	id, _ := filters[0].Value.(string)
+	p, ok := s.policies[id]
+	if !ok {
+		return storageTY.ErrNoDocuments
+	}
+	*(out.(*policyTY.Policy)) = p
+	return nil
+}
+
+func (s *policyMemStore) Upsert(entityName string, data interface{}, _ []storageTY.Filter) error {
+	p, ok := data.(*policyTY.Policy)
+	if !ok || entityName != types.EntityPolicy {
+		return errors.New("invalid upsert")
+	}
+	s.policies[p.ID] = *p
+	return nil
+}
+
+func testPolicyAPI(store *policyMemStore) *API {
+	c := newCache()
+	c.setLoaders(
+		func(id string) (*userTY.User, error) { return nil, ErrUserNotFound },
+		func(id string) (*policyTY.Policy, error) { return nil, ErrUserNotFound },
+		func(id string) (*svcTokenTY.ServiceToken, error) { return nil, ErrTokenNotFound },
+		func() ([]policyTY.Policy, error) { return nil, nil },
+	)
+	return &API{storage: store, cache: c}
+}
+
+func TestSaveRejectsSystemPolicyEdit(t *testing.T) {
+	store := newPolicyMemStore()
+	store.policies["admin"] = policyTY.Policy{ID: "admin", System: true}
+	a := testPolicyAPI(store)
+
+	err := a.Save(&policyTY.Policy{ID: "admin", Description: "hacked", System: false})
+	if !errors.Is(err, ErrSystemPolicyImmutable) {
+		t.Fatalf("Save system policy: got %v, want %v", err, ErrSystemPolicyImmutable)
+	}
+}
+
+func TestSaveRejectsBuiltInID(t *testing.T) {
+	a := testPolicyAPI(newPolicyMemStore())
+	err := a.Save(&policyTY.Policy{ID: policyTY.PolicyReadOnly, Description: "custom readonly"})
+	if !errors.Is(err, ErrSystemPolicyImmutable) {
+		t.Fatalf("Save built-in id: got %v, want %v", err, ErrSystemPolicyImmutable)
+	}
+}
+
+func TestSaveRejectsSystemFlagOnCustomPolicy(t *testing.T) {
+	a := testPolicyAPI(newPolicyMemStore())
+	err := a.Save(&policyTY.Policy{ID: "living-room", System: true})
+	if !errors.Is(err, ErrSystemFlagNotAllowed) {
+		t.Fatalf("Save custom with system: got %v, want %v", err, ErrSystemFlagNotAllowed)
+	}
+}
+
+func TestSavePersistsCustomPolicyWithoutSystem(t *testing.T) {
+	store := newPolicyMemStore()
+	a := testPolicyAPI(store)
+	if err := a.Save(&policyTY.Policy{ID: "living-room", Description: "ok"}); err != nil {
+		t.Fatalf("Save custom: %v", err)
+	}
+	got := store.policies["living-room"]
+	if got.System {
+		t.Fatal("expected system=false on stored custom policy")
+	}
+	if got.Description != "ok" {
+		t.Fatalf("description=%q", got.Description)
+	}
+}
+
+func TestImportSkipsBuiltInAndStripsSystemFlag(t *testing.T) {
+	store := newPolicyMemStore()
+	store.policies["admin"] = policyTY.Policy{ID: "admin", System: true, Description: "original"}
+	a := testPolicyAPI(store)
+
+	if err := a.Import(policyTY.Policy{ID: "admin", Description: "from backup", System: true}); err != nil {
+		t.Fatalf("Import built-in: %v", err)
+	}
+	if store.policies["admin"].Description != "original" {
+		t.Fatalf("built-in was overwritten: %+v", store.policies["admin"])
+	}
+
+	if err := a.Import(policyTY.Policy{ID: "custom-1", System: true, Description: "restored"}); err != nil {
+		t.Fatalf("Import custom: %v", err)
+	}
+	got := store.policies["custom-1"]
+	if got.System {
+		t.Fatal("import left system=true on custom policy")
+	}
+	if got.Description != "restored" {
+		t.Fatalf("description=%q", got.Description)
+	}
+}

pkg/api/policy/defaults.go

@@ -5,6 +5,19 @@ import (
 	settingsTY "github.com/mycontroller-org/server/v2/pkg/types/settings"
 )
 
+// IsBuiltInPolicyID reports whether id is a code-owned system policy.
+func IsBuiltInPolicyID(id string) bool {
+	if id == "" {
+		return false
+	}
+	for _, p := range BuiltInPolicies() {
+		if p.ID == id {
+			return true
+		}
+	}
+	return false
+}
+
 // BuiltInPolicies returns system policies that should always exist.
 func BuiltInPolicies() []policyTY.Policy {
 	allActions := []string{policyTY.ActionAll}

pkg/http_router/routes/policy.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"net/http"
 
+	policyAPI "github.com/mycontroller-org/server/v2/pkg/api/policy"
 	types "github.com/mycontroller-org/server/v2/pkg/types"
 	policyTY "github.com/mycontroller-org/server/v2/pkg/types/policy"
 	handlerUtils "github.com/mycontroller-org/server/v2/pkg/utils/http_handler"
@@ -41,7 +42,11 @@ func (h *Routes) updatePolicy(w http.ResponseWriter, r *http.Request) {
 	}
 	err = h.api.Policy().Save(entity)
 	if err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+		status := http.StatusInternalServerError
+		if errors.Is(err, policyAPI.ErrSystemPolicyImmutable) || errors.Is(err, policyAPI.ErrSystemFlagNotAllowed) {
+			status = http.StatusBadRequest
+		}
+		http.Error(w, err.Error(), status)
 		return
 	}
 }