From e104b183d8e115695b19309fa4b2920380635829 Mon Sep 17 00:00:00 2001
From: Aditya Manthramurthy <donatello@users.noreply.github.com>
Date: Wed, 7 Feb 2024 20:39:53 -0800
Subject: [PATCH] fix: skip policy usage validation for cache update (#19008)

When updating the policy cache, we do not need to validate policy usage
as the policy has already been deleted by the node sending the
notification.
---
 cmd/iam-store.go | 71 ++++++++++++++++++++++++++----------------------
 cmd/iam.go       |  6 ++--
 2 files changed, 42 insertions(+), 35 deletions(-)

diff --git a/cmd/iam-store.go b/cmd/iam-store.go
index 324cd8799..e93a89e9e 100644
--- a/cmd/iam-store.go
+++ b/cmd/iam-store.go
@@ -1166,8 +1166,11 @@ func (store *IAMStoreSys) PolicyNotificationHandler(ctx context.Context, policy
 	return err
 }
 
-// DeletePolicy - deletes policy from storage and cache.
-func (store *IAMStoreSys) DeletePolicy(ctx context.Context, policy string) error {
+// 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).
+func (store *IAMStoreSys) DeletePolicy(ctx context.Context, policy string, isFromNotification bool) error {
 	if policy == "" {
 		return errInvalidArgument
 	}
@@ -1175,42 +1178,44 @@ func (store *IAMStoreSys) DeletePolicy(ctx context.Context, policy string) error
 	cache := store.lock()
 	defer store.unlock()
 
-	// 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{}
-	for u, mp := range cache.iamUserPolicyMap {
-		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.
-				delete(cache.iamUserPolicyMap, u)
-				continue
+	if !isFromNotification {
+		// 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{}
+		for u, mp := range cache.iamUserPolicyMap {
+			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.
+					delete(cache.iamUserPolicyMap, u)
+					continue
+				}
+			}
+			if pset.Contains(policy) {
+				users = append(users, u)
 			}
 		}
-		if pset.Contains(policy) {
-			users = append(users, u)
+		for g, mp := range cache.iamGroupPolicyMap {
+			pset := mp.policySet()
+			if pset.Contains(policy) {
+				groups = append(groups, g)
+			}
 		}
-	}
-	for g, mp := range cache.iamGroupPolicyMap {
-		pset := mp.policySet()
-		if pset.Contains(policy) {
-			groups = append(groups, g)
+		if len(users) != 0 || len(groups) != 0 {
+			return errPolicyInUse
 		}
-	}
-	if len(users) != 0 || len(groups) != 0 {
-		return errPolicyInUse
-	}
 
-	err := store.deletePolicyDoc(ctx, policy)
-	if errors.Is(err, errNoSuchPolicy) {
-		// Ignore error if policy is already deleted.
-		err = nil
-	}
-	if err != nil {
-		return err
+		err := store.deletePolicyDoc(ctx, policy)
+		if errors.Is(err, errNoSuchPolicy) {
+			// Ignore error if policy is already deleted.
+			err = nil
+		}
+		if err != nil {
+			return err
+		}
 	}
 
 	delete(cache.iamPolicyDocsMap, policy)
diff --git a/cmd/iam.go b/cmd/iam.go
index c18db6f78..03726ead5 100644
--- a/cmd/iam.go
+++ b/cmd/iam.go
@@ -526,7 +526,9 @@ func (sys *IAMSys) GetRolePolicy(arnStr string) (arn.ARN, string, error) {
 	return roleArn, rolePolicy, nil
 }
 
-// DeletePolicy - deletes a canned policy from backend or etcd.
+// DeletePolicy - deletes a canned policy from backend. `notifyPeers` is true
+// whenever this is called via the API. It is false when called via a
+// notification from another peer. This is to avoid infinite loops.
 func (sys *IAMSys) DeletePolicy(ctx context.Context, policyName string, notifyPeers bool) error {
 	if !sys.Initialized() {
 		return errServerNotInitialized
@@ -540,7 +542,7 @@ func (sys *IAMSys) DeletePolicy(ctx context.Context, policyName string, notifyPe
 		}
 	}
 
-	err := sys.store.DeletePolicy(ctx, policyName)
+	err := sys.store.DeletePolicy(ctx, policyName, !notifyPeers)
 	if err != nil {
 		return err
 	}