iam: restore native policy immediately after resetting overrides

Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-10-29 12:07:20 +03:00 · 2026-10-25 03:32:53 +08:00
parent aafe4f8886
commit 1e3ab56b06
3 changed files with 389 additions and 59 deletions
@@ -85,15 +85,21 @@ func TestIAMPolicyDeletionRemainsExplicit(t *testing.T) {
 			}
 			p, err := sys.store.GetPolicy("readwrite")
 			mustIAM(t, err)
+			native, err := sys.InfoPolicy("readwrite")
+			mustIAM(t, err)
 			if err := sys.DeletePolicy(ctx, "readwrite", true); err == nil {
 				t.Fatal("local pristine builtin policy became deletable")
 			}
 			_, err = sys.SetPolicy(ctx, "readwrite", p)
 			mustIAM(t, err)
 			mustIAM(t, sys.DeletePolicy(ctx, "readwrite", true))
+			assertIAMPolicyResetInfo(t, sys, "readwrite", native)
 			mustIAM(t, sys.store.LoadIAMCache(ctx, false))
-			if _, err := sys.store.GetPolicy("readwrite"); !errors.Is(err, errNoSuchPolicy) {
-				t.Fatalf("reload restored an explicitly deleted override: %v", err)
+			assertIAMPolicyResetInfo(t, sys, "readwrite", native)
+			r, err = loadIAMRevision(ctx, sys.store, getPolicyDocPath("readwrite"))
+			mustIAM(t, err)
+			if !r.Deleted {
+				t.Fatal("native policy restoration removed the override tombstone")
 			}
 			_, err = sys.SetPolicy(ctx, "readwrite", p)
 			mustIAM(t, err)
@@ -0,0 +1,305 @@
+// Copyright (c) 2026 PGSTY
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+package cmd
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"reflect"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/minio/madmin-go/v3"
+	"github.com/minio/minio/internal/grid"
+	"github.com/pgsty/silo-pkg/v3/policy"
+)
+
+func newIAMPolicyResetSibling(ctx context.Context, t *testing.T, source *IAMSys, obj ObjectLayer) *IAMSys {
+	t.Helper()
+	store := &IAMStoreSys{IAMStorageAPI: newIAMObjectStore(obj, MinIOUsersSysType)}
+	if original, ok := source.store.IAMStorageAPI.(*IAMEtcdStore); ok {
+		store.IAMStorageAPI = newIAMEtcdStore(original.client, MinIOUsersSysType)
+	}
+	mustIAM(t, store.LoadIAMCache(ctx, true))
+	return &IAMSys{store: store, usersSysType: MinIOUsersSysType}
+}
+
+func assertIAMPolicyResetInfo(t *testing.T, sys *IAMSys, name string, want *madmin.PolicyInfo) {
+	t.Helper()
+	got, err := sys.InfoPolicy(name)
+	mustIAM(t, err)
+	var gotPolicy, wantPolicy policy.Policy
+	mustIAM(t, json.Unmarshal(got.Policy, &gotPolicy))
+	mustIAM(t, json.Unmarshal(want.Policy, &wantPolicy))
+	// Actions and resources are sets, whose JSON array order is not stable.
+	// Compare their exact contents while retaining all native metadata checks.
+	gotMetadata, wantMetadata := *got, *want
+	gotMetadata.Policy, wantMetadata.Policy = nil, nil
+	if !gotPolicy.Equals(wantPolicy) || !reflect.DeepEqual(gotMetadata, wantMetadata) {
+		t.Fatalf("policy definition or native metadata changed: got %+v, want %+v", got, want)
+	}
+}
+
+// The first InfoPolicy call after Reset must work without any inventory read,
+// refresh, or retry. The durable tombstone belongs to the removed override;
+// it must still reject stale overrides without suppressing the native policy.
+func TestIAMBuiltinPolicyResetRestoresNativePolicy(t *testing.T) {
+	for _, backend := range []string{"object", "etcd"} {
+		t.Run(backend, func(t *testing.T) {
+			ctx, sys, obj := prepareIAMRevisionFixture(t, backend)
+			for _, builtin := range policy.DefaultPolicies {
+				t.Run(builtin.Name, func(t *testing.T) {
+					name := builtin.Name
+					native, err := sys.InfoPolicy(name)
+					mustIAM(t, err)
+					override := builtin.Definition
+					override.ID = "reset-override"
+					written, err := sys.SetPolicy(ctx, name, override)
+					mustIAM(t, err)
+					mustIAM(t, sys.DeletePolicy(ctx, name, true))
+					assertIAMPolicyResetInfo(t, sys, name, native)
+
+					path := getPolicyDocPath(name)
+					deleted, err := loadIAMRevision(ctx, sys.store, path)
+					mustIAM(t, err)
+					if !deleted.Deleted || !deleted.timestamp().After(written) {
+						t.Fatalf("Reset lost the override deletion revision: %+v", deleted)
+					}
+					for _, at := range []time.Time{written, deleted.timestamp()} {
+						_, err = sys.SetPolicy(withIAMReplicationTime(ctx, at), name, override)
+						if !errors.Is(err, errIAMStaleUpdate) {
+							t.Fatalf("Reset accepted a stale override at %s: %v", at, err)
+						}
+						assertIAMPolicyResetInfo(t, sys, name, native)
+					}
+					mustIAM(t, sys.store.LoadIAMCache(ctx, false))
+					assertIAMPolicyResetInfo(t, sys, name, native)
+					fresh := newIAMPolicyResetSibling(ctx, t, sys, obj)
+					assertIAMPolicyResetInfo(t, fresh, name, native)
+					for _, docs := range []bool{false, true} {
+						mustIAM(t, iamPolicyReloadCall(ctx, fresh.store, docs))
+						assertIAMPolicyResetInfo(t, fresh, name, native)
+					}
+					after, err := loadIAMRevision(ctx, fresh.store, path)
+					mustIAM(t, err)
+					if !reflect.DeepEqual(after, deleted) || !fresh.store.revisionIndex().get(path).Deleted {
+						t.Fatal("default restoration changed the durable deletion or revision index")
+					}
+					// A deliberate new edit remains possible after Reset.
+					_, err = sys.SetPolicy(ctx, name, override)
+					mustIAM(t, err)
+					current, err := sys.store.GetPolicyDoc(name)
+					mustIAM(t, err)
+					if current.Policy.ID != override.ID || current.UpdateDate.IsZero() {
+						t.Fatal("Reset prevented a subsequent explicit override")
+					}
+					mustIAM(t, sys.DeletePolicy(ctx, name, true))
+				})
+			}
+		})
+	}
+}
+
+// Peer and watch notifications carry a name, not a version. Reload committed
+// storage so delayed deletion events cannot erase a recreated policy. Built-in
+// defaults retain mappings; a genuinely deleted custom policy does not.
+func TestIAMPolicyResetNotificationLoadsCommittedState(t *testing.T) {
+	for _, backend := range []string{"object", "etcd"} {
+		for _, delivery := range []string{"peer", "watch"} {
+			for _, recreate := range []bool{false, true} {
+				for _, name := range []string{"writeonly", "reset-custom"} {
+					t.Run(fmt.Sprintf("%s/%s/recreate=%t/%s", backend, delivery, recreate, name), func(t *testing.T) {
+						ctx, source, obj := prepareIAMRevisionFixture(t, backend)
+						globalObjLayerMutex.Lock()
+						globalObjectAPI = obj
+						globalObjLayerMutex.Unlock()
+						builtin := name == "writeonly"
+						var want *madmin.PolicyInfo
+						if builtin {
+							var err error
+							want, err = source.InfoPolicy(name)
+							mustIAM(t, err)
+						}
+						p, err := source.store.GetPolicy("readwrite")
+						mustIAM(t, err)
+						p.ID = "reset-original-override"
+						_, err = source.SetPolicy(ctx, name, p)
+						mustIAM(t, err)
+						_, err = source.CreateUser(ctx, "reset-user", madmin.AddOrUpdateUserReq{SecretKey: "synthetic-reset-password", Status: madmin.AccountEnabled})
+						mustIAM(t, err)
+						_, err = source.AddUsersToGroup(ctx, "reset-group", []string{"reset-user"})
+						mustIAM(t, err)
+						for _, group := range []bool{false, true} {
+							entity := "reset-user"
+							if group {
+								entity = "reset-group"
+							}
+							_, err = source.PolicyDBSet(ctx, entity, "readonly,"+name, regUser, group)
+							mustIAM(t, err)
+						}
+						if err := source.DeletePolicy(ctx, name, true); !errors.Is(err, errPolicyInUse) {
+							t.Fatalf("Reset bypassed policy-in-use protection: %v", err)
+						}
+						sibling := newIAMPolicyResetSibling(ctx, t, source, obj)
+						mappings := make(map[bool]MappedPolicy)
+						mappings[false], _ = sibling.store.GetMappedPolicy("reset-user", false)
+						mappings[true], _ = sibling.store.GetMappedPolicy("reset-group", true)
+						// A remote site may delete before this node has received the
+						// corresponding mapping changes. Local deletion stays guarded.
+						mustIAM(t, source.DeletePolicy(withIAMReplicationTime(ctx, UTCNow()), name, true))
+						if recreate {
+							p.ID = "reset-recreated-override"
+							_, err = source.SetPolicy(ctx, name, p)
+							mustIAM(t, err)
+							want, err = source.InfoPolicy(name)
+							mustIAM(t, err)
+						}
+						before, err := loadIAMRevision(ctx, source.store, getPolicyDocPath(name))
+						mustIAM(t, err)
+						globalIAMSys = sibling
+						defer func() { globalIAMSys = source }()
+						for range 2 {
+							if delivery == "peer" {
+								_, remoteErr := (&peerRESTServer{}).DeletePolicyHandler(grid.NewMSSWith(map[string]string{peerRESTPolicy: name}))
+								if remoteErr != nil {
+									t.Fatal(remoteErr)
+								}
+							} else {
+								mustIAM(t, sibling.loadWatchedEvent(ctx, iamWatchEvent{keyPath: getPolicyDocPath(name)}))
+							}
+							if builtin || recreate {
+								assertIAMPolicyResetInfo(t, sibling, name, want)
+							} else if _, err := sibling.InfoPolicy(name); !errors.Is(err, errNoSuchPolicy) {
+								t.Fatalf("deleted custom policy returned: %v", err)
+							}
+							for group, original := range mappings {
+								entity := "reset-user"
+								if group {
+									entity = "reset-group"
+								}
+								got, _ := sibling.store.GetMappedPolicy(entity, group)
+								if builtin || recreate {
+									if !reflect.DeepEqual(got, original) {
+										t.Fatalf("notification changed surviving mappings: %+v", got)
+									}
+								} else if got.Policies != "readonly" {
+									t.Fatalf("custom deletion did not retain only the unrelated mapping: %+v", got)
+								}
+							}
+						}
+						after, err := loadIAMRevision(ctx, source.store, getPolicyDocPath(name))
+						mustIAM(t, err)
+						if !reflect.DeepEqual(after, before) {
+							t.Fatal("notification changed durable policy storage")
+						}
+					})
+				}
+			}
+		}
+	}
+}
+
+func TestIAMPolicyResetNotificationSurvivesOlderRefresh(t *testing.T) {
+	ctx, source, obj := prepareIAMRevisionFixture(t)
+	p, err := source.store.GetPolicy("readwrite")
+	mustIAM(t, err)
+	p.ID = "old-override"
+	_, err = source.SetPolicy(ctx, "writeonly", p)
+	mustIAM(t, err)
+	sibling := newIAMPolicyResetSibling(ctx, t, source, obj)
+	original := sibling.store.IAMStorageAPI
+	loading, release := make(chan struct{}), make(chan struct{})
+	unblock := sync.OnceFunc(func() { close(release) })
+	var wg sync.WaitGroup
+	t.Cleanup(func() { unblock(); wg.Wait() })
+	sibling.store.IAMStorageAPI = &iamPolicyReloadStore{
+		IAMStorageAPI: original,
+		policies:      original.loadPolicyDocs,
+		users: func(ctx context.Context, typ IAMUserType, m map[string]UserIdentity) error {
+			if typ == regUser {
+				close(loading)
+				<-release
+			}
+			return original.loadUsers(ctx, typ, m)
+		},
+	}
+	done := make(chan error, 1)
+	wg.Go(func() { done <- sibling.store.LoadIAMCache(ctx, false) })
+	awaitIAMPolicyReload(t, loading)
+	mustIAM(t, source.DeletePolicy(ctx, "writeonly", true))
+	p.ID = "newer-override"
+	_, err = source.SetPolicy(ctx, "writeonly", p)
+	mustIAM(t, err)
+	want, err := source.InfoPolicy("writeonly")
+	mustIAM(t, err)
+	// The delayed deletion notification must observe the newer edit, and
+	// prevent a refresh that captured the old override from replacing it.
+	mustIAM(t, sibling.DeletePolicy(ctx, "writeonly", false))
+	unblock()
+	mustIAM(t, <-done)
+	assertIAMPolicyResetInfo(t, sibling, "writeonly", want)
+}
+
+type iamPolicyResetFailureStore struct {
+	IAMStorageAPI
+	err error
+}
+
+func (s *iamPolicyResetFailureStore) saveIAMConfig(context.Context, any, string, ...options) error {
+	return s.err
+}
+
+func (s *iamPolicyResetFailureStore) loadPolicyDoc(context.Context, string, map[string]PolicyDoc) error {
+	return s.err
+}
+
+func TestIAMBuiltinPolicyResetStorageFailurePreservesOverride(t *testing.T) {
+	for _, backend := range []string{"object", "etcd"} {
+		t.Run(backend, func(t *testing.T) {
+			ctx, sys, _ := prepareIAMRevisionFixture(t, backend)
+			p, err := sys.store.GetPolicy("readonly")
+			mustIAM(t, err)
+			_, err = sys.SetPolicy(ctx, "writeonly", p)
+			mustIAM(t, err)
+			before, err := sys.InfoPolicy("writeonly")
+			mustIAM(t, err)
+			stored, err := loadIAMRevision(ctx, sys.store, getPolicyDocPath("writeonly"))
+			mustIAM(t, err)
+			original := sys.store.IAMStorageAPI
+			injected := errors.New("injected policy storage failure")
+			sys.store.IAMStorageAPI = &iamPolicyResetFailureStore{IAMStorageAPI: original, err: injected}
+			for _, notify := range []bool{true, false} {
+				if err := sys.DeletePolicy(ctx, "writeonly", notify); !errors.Is(err, injected) {
+					t.Fatalf("storage failure became a successful Reset, notify=%t: %v", notify, err)
+				}
+				assertIAMPolicyResetInfo(t, sys, "writeonly", before)
+			}
+			if err := sys.loadWatchedEvent(ctx, iamWatchEvent{keyPath: getPolicyDocPath("writeonly")}); !errors.Is(err, injected) {
+				t.Fatalf("watch storage failure became a default policy: %v", err)
+			}
+			assertIAMPolicyResetInfo(t, sys, "writeonly", before)
+			after, err := loadIAMRevision(ctx, original, getPolicyDocPath("writeonly"))
+			mustIAM(t, err)
+			if !reflect.DeepEqual(after, stored) {
+				t.Fatal("failed Reset changed the durable override")
+			}
+		})
+	}
+}
@@ -647,6 +647,20 @@ func setDefaultCannedPolicies(policies map[string]PolicyDoc) {
 	}
 }

+// Remove a stored policy from cache. A built-in policy's deletion removes its
+// override, so its native definition remains available with native metadata.
+// The durable tombstone is retained separately to reject stale overrides.
+func removePolicyFromCache(name string, policies map[string]PolicyDoc) (restoredDefault bool) {
+	delete(policies, name)
+	for _, p := range policy.DefaultPolicies {
+		if p.Name == name {
+			policies[name] = defaultPolicyDoc(p.Definition)
+			return true
+		}
+	}
+	return false
+}
+
 // LoadIAMCache reads all IAM items and populates a new iamCache object and
 // replaces the in-memory cache object.
 func (store *IAMStoreSys) LoadIAMCache(ctx context.Context, firstTime bool) error {
@@ -722,7 +736,7 @@ func (store *IAMStoreSys) LoadIAMCache(ctx context.Context, firstTime bool) erro

 	for name := range newCache.iamPolicyDocsMap {
 		if store.revisionIndex().get(getPolicyDocPath(name)).Deleted {
-			delete(newCache.iamPolicyDocsMap, name)
+			removePolicyFromCache(name, newCache.iamPolicyDocsMap)
 		}
 	}

@@ -1376,9 +1390,9 @@ func (store *IAMStoreSys) PolicyDBSet(ctx context.Context, name, policy string,
 }

 // PolicyNotificationHandler - loads given policy from storage. If not present,
-// deletes from cache. This notification only reads from storage, and updates
-// cache. When the notification is for a policy deletion, it updates the
-// user-policy and group-policy maps as well.
+// removes its override from cache. This notification only reads from storage,
+// and updates cache. A deleted built-in override restores its native policy;
+// a deleted custom policy is removed from user-policy and group-policy maps.
 func (store *IAMStoreSys) PolicyNotificationHandler(ctx context.Context, policy string) error {
 	if policy == "" {
 		return errInvalidArgument
@@ -1389,8 +1403,10 @@ func (store *IAMStoreSys) PolicyNotificationHandler(ctx context.Context, policy

 	err := store.loadPolicyDoc(ctx, policy, cache.iamPolicyDocsMap)
 	if errors.Is(err, errNoSuchPolicy) {
-		// policy was deleted, update cache.
-		delete(cache.iamPolicyDocsMap, policy)
+		if removePolicyFromCache(policy, cache.iamPolicyDocsMap) {
+			cache.updatedAt = time.Now()
+			return nil
+		}

 		// update user policy map
 		cache.iamUserPolicyMap.Range(func(u string, mp MappedPolicy) bool {
@@ -1426,74 +1442,77 @@ func (store *IAMStoreSys) PolicyNotificationHandler(ctx context.Context, policy
 		cache.updatedAt = time.Now()
 		return nil
 	}
+	if err == nil {
+		cache.updatedAt = time.Now()
+	}
 	return err
 }

-// DeletePolicy - deletes policy from storage and cache. When this called in
-// response to a notification (i.e. isFromNotification = true), it skips the
-// validation of policy usage and the attempt to delete in the backend as well
-// (as this is already done by the notifying node).
+// DeletePolicy - deletes a stored policy and restores the native definition
+// when it was a built-in override. Notifications reload committed storage, so
+// a delayed deletion event cannot remove a subsequently recreated policy.
 func (store *IAMStoreSys) DeletePolicy(ctx context.Context, policy string, isFromNotification bool) error {
 	if policy == "" {
 		return errInvalidArgument
 	}
+	if isFromNotification {
+		return store.PolicyNotificationHandler(ctx, policy)
+	}

 	cache := store.lock()
 	defer store.unlock()

-	if !isFromNotification {
-		if err := store.checkIAMRevision(ctx, getPolicyDocPath(policy), true); err != nil {
-			return err
+	if err := store.checkIAMRevision(ctx, getPolicyDocPath(policy), true); err != nil {
+		return err
+	}
+	if _, replicated := iamReplicationTime(ctx); !replicated {
+		if _, exists := cache.iamPolicyDocsMap[policy]; !exists {
+			// Preserve idempotent local deletion without creating a durable
+			// revocation for a misspelled/nonexistent policy name.
+			return nil
 		}
-		if _, replicated := iamReplicationTime(ctx); !replicated {
-			if _, exists := cache.iamPolicyDocsMap[policy]; !exists {
-				// Preserve idempotent local deletion without creating a durable
-				// revocation for a misspelled/nonexistent policy name.
-				return nil
+		// Check if policy is mapped to any existing user or group. If so, we do not
+		// allow deletion of the policy. If the policy is mapped to an STS account,
+		// we do allow deletion.
+		users := []string{}
+		groups := []string{}
+		cache.iamUserPolicyMap.Range(func(u string, mp MappedPolicy) bool {
+			pset := mp.policySet()
+			if store.getUsersSysType() == MinIOUsersSysType {
+				if _, ok := cache.iamUsersMap[u]; !ok {
+					// This case can happen when a temporary account is
+					// deleted or expired - remove it from userPolicyMap.
+					cache.iamUserPolicyMap.Delete(u)
+					return true
+				}
 			}
-			// Check if policy is mapped to any existing user or group. If so, we do not
-			// allow deletion of the policy. If the policy is mapped to an STS account,
-			// we do allow deletion.
-			users := []string{}
-			groups := []string{}
-			cache.iamUserPolicyMap.Range(func(u string, mp MappedPolicy) bool {
-				pset := mp.policySet()
-				if store.getUsersSysType() == MinIOUsersSysType {
-					if _, ok := cache.iamUsersMap[u]; !ok {
-						// This case can happen when a temporary account is
-						// deleted or expired - remove it from userPolicyMap.
-						cache.iamUserPolicyMap.Delete(u)
-						return true
-					}
-				}
-				if pset.Contains(policy) {
-					users = append(users, u)
-				}
-				return true
-			})
-			cache.iamGroupPolicyMap.Range(func(g string, mp MappedPolicy) bool {
-				pset := mp.policySet()
-				if pset.Contains(policy) {
-					groups = append(groups, g)
-				}
-				return true
-			})
-			if len(users) != 0 || len(groups) != 0 {
-				return errPolicyInUse
+			if pset.Contains(policy) {
+				users = append(users, u)
 			}
-		}
-
-		err := store.deletePolicyDoc(ctx, policy)
-		if errors.Is(err, errNoSuchPolicy) {
-			// Ignore error if policy is already deleted.
-			err = nil
-		}
-		if err != nil {
-			return err
+			return true
+		})
+		cache.iamGroupPolicyMap.Range(func(g string, mp MappedPolicy) bool {
+			pset := mp.policySet()
+			if pset.Contains(policy) {
+				groups = append(groups, g)
+			}
+			return true
+		})
+		if len(users) != 0 || len(groups) != 0 {
+			return errPolicyInUse
 		}
 	}

-	delete(cache.iamPolicyDocsMap, policy)
+	err := store.deletePolicyDoc(ctx, policy)
+	if errors.Is(err, errNoSuchPolicy) {
+		// Ignore error if policy is already deleted.
+		err = nil
+	}
+	if err != nil {
+		return err
+	}
+
+	removePolicyFromCache(policy, cache.iamPolicyDocsMap)
 	cache.updatedAt = time.Now()

 	return nil