do not fallback on the drives to load groups for LDAP (#20320)

if a user policy is found, avoid reading from the drives
for missing group mappings, group mappings are not mandatory
and conditional.

This PR restores the older behavior while making sure that
if a direct user policy is not found, we would still attempt
to load from the group from the drives.

cmd/iam-store.go

@@ -23,6 +23,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"path"
 	"sort"
 	"strings"
 	"time"
@@ -367,7 +368,7 @@ func (c *iamCache) removeGroupFromMembershipsMap(group string) {
 // information in IAM (i.e sys.iam*Map) - this info is stored only in the STS
 // generated credentials. Thus we skip looking up group memberships, user map,
 // and group map and check the appropriate policy maps directly.
-func (c *iamCache) policyDBGet(store *IAMStoreSys, name string, isGroup bool) ([]string, time.Time, error) {
+func (c *iamCache) policyDBGet(store *IAMStoreSys, name string, isGroup bool, policyPresent bool) ([]string, time.Time, error) {
 	if isGroup {
 		if store.getUsersSysType() == MinIOUsersSysType {
 			g, ok := c.iamGroupsMap[name]
@@ -392,105 +393,107 @@ func (c *iamCache) policyDBGet(store *IAMStoreSys, name string, isGroup bool) ([
 		if ok {
 			return policy.toSlice(), policy.UpdatedAt, nil
 		}
-		if err := store.loadMappedPolicyWithRetry(context.TODO(), name, regUser, true, c.iamGroupPolicyMap, 3); err != nil && !errors.Is(err, errNoSuchPolicy) {
+		if !policyPresent {
-			return nil, time.Time{}, err
+			if err := store.loadMappedPolicy(context.TODO(), name, regUser, true, c.iamGroupPolicyMap); err != nil && !errors.Is(err, errNoSuchPolicy) {
-		}
+				return nil, time.Time{}, err
-		policy, _ = c.iamGroupPolicyMap.Load(name)
-		return policy.toSlice(), policy.UpdatedAt, nil
-	}
-
-	// When looking for a user's policies, we also check if the user
-	// and the groups they are member of are enabled.
-	u, ok := c.iamUsersMap[name]
-	if ok {
-		if !u.Credentials.IsValid() {
-			return nil, time.Time{}, nil
-		}
-	}
-
-	// For internal IDP regular/service account user accounts, the policy
-	// mapping is iamUserPolicyMap. For STS accounts, the parent user would be
-	// passed here and we lookup the mapping in iamSTSPolicyMap.
-	mp, ok := c.iamUserPolicyMap.Load(name)
-	if !ok {
-		if err := store.loadMappedPolicyWithRetry(context.TODO(), name, regUser, false, c.iamUserPolicyMap, 3); err != nil && !errors.Is(err, errNoSuchPolicy) {
-			return nil, time.Time{}, err
-		}
-
-		mp, ok = c.iamUserPolicyMap.Load(name)
-		if !ok {
-			// Since user "name" could be a parent user of an STS account, we look up
-			// mappings for those too.
-			mp, ok = c.iamSTSPolicyMap.Load(name)
-			if !ok {
-				// Attempt to load parent user mapping for STS accounts
-				if err := store.loadMappedPolicyWithRetry(context.TODO(), name, stsUser, false, c.iamSTSPolicyMap, 3); err != nil && !errors.Is(err, errNoSuchPolicy) {
-					return nil, time.Time{}, err
-				}
-				mp, _ = c.iamSTSPolicyMap.Load(name)
 			}
+			policy, _ = c.iamGroupPolicyMap.Load(name)
+			return policy.toSlice(), policy.UpdatedAt, nil
 		}
+		return nil, time.Time{}, nil
 	}
 
 	// returned policy could be empty, we use set to de-duplicate.
-	policies := set.CreateStringSet(mp.toSlice()...)
+	var policies set.StringSet
+	var updatedAt time.Time
 
-	for _, group := range u.Credentials.Groups {
+	if store.getUsersSysType() == LDAPUsersSysType {
-		if store.getUsersSysType() == MinIOUsersSysType {
+		// For LDAP policy mapping is part of STS users, we only need to lookup
-			g, ok := c.iamGroupsMap[group]
+		// those mappings.
-			if !ok {
+		mp, ok := c.iamSTSPolicyMap.Load(name)
-				if err := store.loadGroup(context.Background(), group, c.iamGroupsMap); err != nil {
+		if !ok {
-					return nil, time.Time{}, err
+			// Attempt to load parent user mapping for STS accounts
-				}
+			if err := store.loadMappedPolicy(context.TODO(), name, stsUser, false, c.iamSTSPolicyMap); err != nil && !errors.Is(err, errNoSuchPolicy) {
-				g, ok = c.iamGroupsMap[group]
+				return nil, time.Time{}, err
-				if !ok {
-					return nil, time.Time{}, errNoSuchGroup
-				}
 			}
-
+			mp, _ = c.iamSTSPolicyMap.Load(name)
-			// Group is disabled, so we return no policy - this
+		}
-			// ensures the request is denied.
+		policies = set.CreateStringSet(mp.toSlice()...)
-			if g.Status == statusDisabled {
+		updatedAt = mp.UpdatedAt
+	} else {
+		// When looking for a user's policies, we also check if the user
+		// and the groups they are member of are enabled.
+		u, ok := c.iamUsersMap[name]
+		if ok {
+			if !u.Credentials.IsValid() {
 				return nil, time.Time{}, nil
 			}
 		}
 
-		policy, ok := c.iamGroupPolicyMap.Load(group)
+		// For internal IDP regular/service account user accounts, the policy
+		// mapping is iamUserPolicyMap. For STS accounts, the parent user would be
+		// passed here and we lookup the mapping in iamSTSPolicyMap.
+		mp, ok := c.iamUserPolicyMap.Load(name)
 		if !ok {
-			if err := store.loadMappedPolicyWithRetry(context.TODO(), group, regUser, true, c.iamGroupPolicyMap, 3); err != nil && !errors.Is(err, errNoSuchPolicy) {
+			if err := store.loadMappedPolicy(context.TODO(), name, regUser, false, c.iamUserPolicyMap); err != nil && !errors.Is(err, errNoSuchPolicy) {
 				return nil, time.Time{}, err
 			}
-			policy, _ = c.iamGroupPolicyMap.Load(group)
+			mp, ok = c.iamUserPolicyMap.Load(name)
+			if !ok {
+				// Since user "name" could be a parent user of an STS account, we look up
+				// mappings for those too.
+				mp, ok = c.iamSTSPolicyMap.Load(name)
+				if !ok {
+					// Attempt to load parent user mapping for STS accounts
+					if err := store.loadMappedPolicy(context.TODO(), name, stsUser, false, c.iamSTSPolicyMap); err != nil && !errors.Is(err, errNoSuchPolicy) {
+						return nil, time.Time{}, err
+					}
+					mp, _ = c.iamSTSPolicyMap.Load(name)
+				}
+			}
 		}
+		policies = set.CreateStringSet(mp.toSlice()...)
 
-		for _, p := range policy.toSlice() {
+		for _, group := range u.Credentials.Groups {
-			policies.Add(p)
+			g, ok := c.iamGroupsMap[group]
+			if ok {
+				// Group is disabled, so we return no policy - this
+				// ensures the request is denied.
+				if g.Status == statusDisabled {
+					return nil, time.Time{}, nil
+				}
+			}
+
+			policy, ok := c.iamGroupPolicyMap.Load(group)
+			if !ok {
+				if err := store.loadMappedPolicy(context.TODO(), group, regUser, true, c.iamGroupPolicyMap); err != nil && !errors.Is(err, errNoSuchPolicy) {
+					return nil, time.Time{}, err
+				}
+				policy, _ = c.iamGroupPolicyMap.Load(group)
+			}
+
+			for _, p := range policy.toSlice() {
+				policies.Add(p)
+			}
 		}
+		updatedAt = mp.UpdatedAt
 	}
 
 	for _, group := range c.iamUserGroupMemberships[name].ToSlice() {
 		if store.getUsersSysType() == MinIOUsersSysType {
 			g, ok := c.iamGroupsMap[group]
-			if !ok {
+			if ok {
-				if err := store.loadGroup(context.Background(), group, c.iamGroupsMap); err != nil {
+				// Group is disabled, so we return no policy - this
-					return nil, time.Time{}, err
+				// ensures the request is denied.
+				if g.Status == statusDisabled {
+					return nil, time.Time{}, nil
 				}
-				g, ok = c.iamGroupsMap[group]
-				if !ok {
-					return nil, time.Time{}, errNoSuchGroup
-				}
-			}
-
-			// Group is disabled, so we return no policy - this
-			// ensures the request is denied.
-			if g.Status == statusDisabled {
-				return nil, time.Time{}, nil
 			}
 		}
 
 		policy, ok := c.iamGroupPolicyMap.Load(group)
 		if !ok {
-			if err := store.loadMappedPolicyWithRetry(context.TODO(), group, regUser, true, c.iamGroupPolicyMap, 3); err != nil && !errors.Is(err, errNoSuchPolicy) {
+			if err := store.loadMappedPolicy(context.TODO(), group, regUser, true, c.iamGroupPolicyMap); err != nil && !errors.Is(err, errNoSuchPolicy) {
 				return nil, time.Time{}, err
 			}
 			policy, _ = c.iamGroupPolicyMap.Load(group)
@@ -501,7 +504,7 @@ func (c *iamCache) policyDBGet(store *IAMStoreSys, name string, isGroup bool) ([
 		}
 	}
 
-	return policies.ToSlice(), mp.UpdatedAt, nil
+	return policies.ToSlice(), updatedAt, nil
 }
 
 func (c *iamCache) updateUserWithClaims(key string, u UserIdentity) error {
@@ -754,13 +757,14 @@ func (store *IAMStoreSys) PolicyDBGet(name string, groups ...string) ([]string,
 	cache := store.rlock()
 	defer store.runlock()
 
-	policies, _, err := cache.policyDBGet(store, name, false)
+	policies, _, err := cache.policyDBGet(store, name, false, false)
 	if err != nil {
 		return nil, err
 	}
 
+	userPolicyPresent := len(policies) > 0
 	for _, group := range groups {
-		ps, _, err := cache.policyDBGet(store, group, true)
+		ps, _, err := cache.policyDBGet(store, group, true, userPolicyPresent)
 		if err != nil {
 			return nil, err
 		}
@@ -945,7 +949,7 @@ func (store *IAMStoreSys) GetGroupDescription(group string) (gd madmin.GroupDesc
 	cache := store.rlock()
 	defer store.runlock()
 
-	ps, updatedAt, err := cache.policyDBGet(store, group, true)
+	ps, updatedAt, err := cache.policyDBGet(store, group, true, false)
 	if err != nil {
 		return gd, err
 	}
@@ -974,34 +978,46 @@ func (store *IAMStoreSys) GetGroupDescription(group string) (gd madmin.GroupDesc
 	}, nil
 }
 
+// updateGroups updates the group from the persistent store, and also related policy mapping if any.
 func (store *IAMStoreSys) updateGroups(ctx context.Context, cache *iamCache) (res []string, err error) {
-	if store.getUsersSysType() == MinIOUsersSysType {
+	if iamOS, ok := store.IAMStorageAPI.(*IAMObjectStore); ok {
-		m := map[string]GroupInfo{}
+		listedConfigItems, err := iamOS.listAllIAMConfigItems(ctx)
-		err = store.loadGroups(ctx, m)
 		if err != nil {
-			return
+			return nil, err
 		}
-		cache.iamGroupsMap = m
+		if store.getUsersSysType() == MinIOUsersSysType {
-		cache.updatedAt = time.Now()
+			groupsList := listedConfigItems[groupsListKey]
-		for k := range cache.iamGroupsMap {
+			for _, item := range groupsList {
-			res = append(res, k)
+				group := path.Dir(item)
+				if err = iamOS.loadGroup(ctx, group, cache.iamGroupsMap); err != nil && !errors.Is(err, errNoSuchGroup) {
+					return nil, fmt.Errorf("unable to load the group: %w", err)
+				}
+				res = append(res, group)
+			}
 		}
+
+		groupPolicyMappingsList := listedConfigItems[policyDBGroupsListKey]
+		for _, item := range groupPolicyMappingsList {
+			group := strings.TrimSuffix(item, ".json")
+			if err = iamOS.loadMappedPolicy(ctx, group, regUser, true, cache.iamGroupPolicyMap); err != nil && !errors.Is(err, errNoSuchPolicy) {
+				return nil, fmt.Errorf("unable to load the policy mapping for the group: %w", err)
+			}
+			res = append(res, group)
+		}
+
+		return res, nil
 	}
 
-	if store.getUsersSysType() == LDAPUsersSysType {
+	// For etcd just return from cache.
-		m := xsync.NewMapOf[string, MappedPolicy]()
+	for k := range cache.iamGroupsMap {
-		err = store.loadMappedPolicies(ctx, stsUser, true, m)
+		res = append(res, k)
-		if err != nil {
-			return
-		}
-		cache.iamGroupPolicyMap = m
-		cache.updatedAt = time.Now()
-		cache.iamGroupPolicyMap.Range(func(k string, v MappedPolicy) bool {
-			res = append(res, k)
-			return true
-		})
 	}
 
+	cache.iamGroupPolicyMap.Range(func(k string, v MappedPolicy) bool {
+		res = append(res, k)
+		return true
+	})
+
 	return res, nil
 }
 
@@ -2800,14 +2816,8 @@ func (store *IAMStoreSys) LoadUser(ctx context.Context, accessKey string) error
 		}
 
 		if groupLoad {
-			load := len(newCache.iamGroupsMap) == 0
+			// NOTE: we are not worried about loading errors from groups.
-			if store.getUsersSysType() == LDAPUsersSysType && newCache.iamGroupPolicyMap.Size() == 0 {
+			store.updateGroups(ctx, newCache)
-				load = true
-			}
-			if load {
-				// NOTE: we are not worried about loading errors from groups.
-				store.updateGroups(ctx, newCache)
-			}
 			newCache.buildUserGroupMemberships()
 		}