fix(batch): skip deleted policies a mapping still names

Deleting a policy checks that no user or group mapping names it, but not
the STS mappings of OpenID parent users and LDAP user DNs. After such a
deletion the request path (MergePolicies) skips the confirmed-absent
policy and decides on the remaining mapped policies, while batch
authorization treated the absent document as a denial of the whole
action: jobs of such a submitter ended refused, although the same
principal's requests were allowed.

batchLocalPolicies now skips a policy document whose load reports that it
does not exist, as MergePolicies does, and still decides on the remaining
policies. Any other failure to read a document, and a load that returns
no document, still ends the attempt as unavailable, since it could hide a
Deny. With every mapped policy absent nothing allows the action, so the
job is refused as before.

TestBatchAuthorizeSkipsDeletedMappedPolicy maps an STS parent user to two
policies, deletes one, and requires the batch decision to agree with the
request decision, allowed with the other policy and denied once both are
deleted. Before this change the batch decision was a refusal while the
request was allowed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>

cmd/batch-authz-snapshot_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/minio/minio/internal/auth"
 	"github.com/pgsty/silo-pkg/v3/policy"
 	"github.com/puzpuzpuz/xsync/v3"
 )
@@ -157,3 +158,48 @@ func TestBatchAuthorizeSlowLoadKeepsCachedAuthorizationAvailable(t *testing.T) {
 		})
 	}
 }
+
+// A policy that a mapping still names after it was deleted takes no part in
+// the decision, for a batch job as for a request: the remaining mapped
+// policies decide. An STS parent user's mapping can name a deleted policy,
+// because deleting a policy checks only user and group mappings for use.
+func TestBatchAuthorizeSkipsDeletedMappedPolicy(t *testing.T) {
+	f := newBatchAuthzFixture(t)
+	batchAuthzPolicy(f.ctx, t, f.sys, "mapped-full", batchAuthzFullAccess)
+	batchAuthzPolicy(f.ctx, t, f.sys, "mapped-deleted", `{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/*"]}`)
+	parent := iamAdminUser(f.ctx, t, f.sys, "mapped-parent", "")
+	_, err := f.sys.PolicyDBSet(f.ctx, parent.AccessKey, "mapped-full,mapped-deleted", stsUser, false)
+	mustIAM(t, err)
+	cred, err := auth.GetNewCredentialsWithMetadata(map[string]any{parentClaim: parent.AccessKey, "exp": UTCNow().Add(time.Hour).Unix()}, globalActiveCred.SecretKey)
+	mustIAM(t, err)
+	cred.ParentUser = parent.AccessKey
+	_, err = f.sys.SetTempUser(f.ctx, cred.AccessKey, cred, "")
+	mustIAM(t, err)
+	sub, err := newBatchJobSubmitter(cred)
+	mustIAM(t, err)
+	req := batchAuthzRequest{action: policy.GetObjectAction, bucket: f.bucket, object: "key", versionID: mustGetUUID()}
+	if err := sub.authorize(f.ctx, req); err != nil {
+		t.Fatalf("control: %v", err)
+	}
+	p, err := sub.principal(f.ctx)
+	mustIAM(t, err)
+	args := policy.Args{AccountName: cred.AccessKey, Action: req.action, BucketName: req.bucket, ObjectName: req.object, Claims: p.claims, ConditionValues: p.conditionValues(req)}
+
+	mustIAM(t, f.sys.DeletePolicy(f.ctx, "mapped-deleted", true))
+	if !f.sys.IsAllowed(args) {
+		t.Fatal("the request path denied the remaining mapped policy")
+	}
+	if err := sub.authorize(f.ctx, req); err != nil {
+		t.Fatalf("batch authorization diverged from the request after a mapped policy was deleted: %v", err)
+	}
+
+	// With every mapped policy deleted, both deny; the job is refused, not
+	// paused, as nothing is unavailable.
+	mustIAM(t, f.sys.DeletePolicy(f.ctx, "mapped-full", true))
+	if f.sys.IsAllowed(args) {
+		t.Fatal("the request path allowed without any mapped policy")
+	}
+	if err := sub.authorize(f.ctx, req); !isBatchJobAuthzError(err) {
+		t.Fatalf("batch authorization without any mapped policy: %v", err)
+	}
+}

cmd/batch-authz.go

@@ -688,12 +688,19 @@ func (sys *IAMSys) batchLocalPolicies(p batchJobPrincipal, args policy.Args, wit
 			doc, ok := cache.iamPolicyDocsMap[name]
 			if !ok {
 				loaded := make(map[string]PolicyDoc)
-				if err := snap.loadPolicyDoc(name, loaded); err != nil {
+				err := snap.loadPolicyDoc(name, loaded)
+				if errors.Is(err, errNoSuchPolicy) {
+					// Confirmed absent, as when a mapping still names a
+					// deleted policy: it takes no part in the decision, as
+					// in MergePolicies. Any other failure could hide a Deny.
+					continue
+				}
+				if err != nil {
 					return nil, err
 				}
 				doc, ok = loaded[name]
 				if !ok {
-					return nil, errNoSuchPolicy
+					return nil, fmt.Errorf("load IAM policy %q returned no document", name)
 				}
 			}
 			selected = append(selected, doc.Policy)