fix(iam): edit users and service accounts on the stored record, not a cached copy

UpdateServiceAccount, SetUserStatus and UpdateUserSecretKey read the identity from the serving server's cache, changed it and wrote it back whole. On a server that missed a notification, an ordinary edit (a description, a status, a secret) undid a rotation made through another server and brought back a user or service account deleted meanwhile, with its old secret. Edits now go through iamIdentityEdit, inside the identity's cross-node revision lock: the stored record is loaded and normalized as the loaders do (access key from the path, claims from the token, a deprecated comment for an empty description), the edit applies to it, and an absent or deleted identity is refused and dropped from the cache of the server that tried. The LDAP group refresh of a0de92fcb uses the same mechanism. - The admin handler binds a service-account edit to the parent it authorized: an account deleted and created again for another parent meanwhile is not edited (parentUser in updateServiceAccountOpts; replication does not set it). - A replicated edit older than a deletion this site applied is answered as stale, so site replication acknowledges it instead of retrying. - SetUserStatus refuses a service or temporary credential as before, and a refused edit drops only a cache entry of the addressed kind. - A session-policy edit stores the claims of the token it signs; the token is signed once. - UpdateUserSecretKey addressed to a service-account key finds no user instead of writing the account to the regular-user path. Tests use a second IAM store over the same storage whose cache missed the change: rotation, deletion, status and secret edits of users, parent binding, legacy records, replicated edits around a deletion, and status calls on a service account. Against the cache-based writers every case fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-11-09 17:21:51 +03:00 · 2026-11-08 13:56:04 +08:00
parent a0de92fcbe
commit 70914759e5
5 changed files with 396 additions and 101 deletions
@@ -903,6 +903,9 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re
 		description:   updateReq.NewDescription,
 		expiration:    updateReq.NewExpiration,
 		sessionPolicy: sp,
+		// The edit applies to the stored account only while it still has
+		// the parent the checks above authorized.
+		parentUser: svcAccount.ParentUser,
 	}
 	updatedAt, err := globalIAMSys.UpdateServiceAccount(ctx, accessKey, opts)
 	if err != nil {
@@ -16,7 +16,6 @@ import (
 	"time"

 	"github.com/minio/madmin-go/v3"
-	"github.com/minio/minio-go/v7/pkg/set"
 	"github.com/minio/minio/internal/auth"
 	etcd "go.etcd.io/etcd/client/v3"
 	"go.etcd.io/etcd/client/v3/concurrency"
@@ -46,22 +45,26 @@ type iamGroupRevocation struct {
 // its revision so an older non-expiring credential cannot return.
 type iamExpireIdentity struct{}

-// An LDAP group refresh sets the groups of the credential as it is stored
-// now. The refresh computes the groups from a snapshot of a server's cache
-// taken before a slow directory lookup; writing that snapshot back would
-// undo a rotation or an edit made meanwhile on any server, and bring back a
-// deleted credential. The refresh therefore changes only the groups, under
-// the identity's revision lock and on the stored record, and leaves alone a
-// credential that is gone, expired, or now has another parent, whose groups
-// the lookup did not compute.
-type iamGroupRefresh struct {
+// An edit of an identity applies to the identity as it is stored now, under
+// its revision lock, not to a server's cached copy. A copy written back
+// whole would undo a rotation or an edit made meanwhile through this or
+// another server, whose notification this one may have missed, and bring
+// back an identity deleted meanwhile. edit receives the stored record and
+// changes it in place; it returns errIAMIdentityUnchanged to write nothing,
+// or any other error to refuse. An identity that is absent or deleted is
+// refused with errIAMIdentityGone.
+type iamIdentityEdit struct {
 	UserIdentity // the record written, or found unchanged
 	accessKey    string
-	parentUser   string
-	groups       []string
-	unchanged    bool // the stored record already has the groups
+	edit         func(u *UserIdentity) error
+	unchanged    bool
 }

+var (
+	errIAMIdentityGone      = errors.New("IAM identity is absent or deleted in storage")
+	errIAMIdentityUnchanged = errors.New("IAM identity edit changes nothing")
+)
+
 // The authoritative revocation is durable even if dependent cleanup fails.
 // Callers must publish it to sibling caches before returning the error.
 type iamCommittedCleanupError struct {
@@ -269,27 +272,45 @@ func saveIAMRevision(ctx context.Context, store IAMStorageAPI, path string, item
 	if err != nil {
 		return err
 	}
-	if op, ok := item.(*iamGroupRefresh); ok {
+	if op, ok := item.(*iamIdentityEdit); ok {
 		var current UserIdentity
 		err := store.loadIAMConfig(ctx, &current, path)
 		if errors.Is(err, errConfigNotFound) {
-			return errIAMStaleUpdate
+			return errIAMIdentityGone
 		}
 		if err != nil {
 			return err
 		}
+		if current.Deleted {
+			if origin, replicated := iamReplicationTime(ctx); replicated && !origin.After(previous.timestamp()) {
+				// A peer's edit from before this deletion: acknowledged.
+				return errIAMStaleUpdate
+			}
+			return errIAMIdentityGone
+		}
+		// Normalize the record as the identity loaders do: the path names the
+		// access key, the claims are the token's, and a deprecated comment
+		// stands in for an empty description.
 		if current.Credentials.AccessKey == "" {
-			// As the identity loaders do: the path names the access key.
 			current.Credentials.AccessKey = op.accessKey
 		}
-		if current.Deleted || current.Credentials.ParentUser != op.parentUser || current.Credentials.IsExpired() {
-			return errIAMStaleUpdate
+		if current.Credentials.SessionToken != "" {
+			claims, err := extractJWTClaims(current)
+			if err != nil {
+				return err
+			}
+			current.Credentials.Claims = claims.Map()
 		}
-		if set.CreateStringSet(current.Credentials.Groups...).Equals(set.CreateStringSet(op.groups...)) {
-			op.UserIdentity, op.unchanged = current, true
-			return nil
+		if current.Credentials.Description == "" {
+			current.Credentials.Description = current.Credentials.Comment
+		}
+		if err := op.edit(&current); err != nil {
+			if errors.Is(err, errIAMIdentityUnchanged) {
+				op.UserIdentity, op.unchanged = current, true
+				return nil
+			}
+			return err
 		}
-		current.Credentials.Groups = op.groups
 		current.UpdatedAt = time.Time{} // a new revision, as for any local edit
 		op.UserIdentity = current
 		item = &op.UserIdentity
@@ -2778,37 +2778,26 @@ func (store *IAMStoreSys) SetUserStatus(ctx context.Context, accessKey string, s
 	cache := store.lock()
 	defer store.unlock()

-	ui, ok := cache.iamUsersMap[accessKey]
-	if !ok {
-		return updatedAt, errNoSuchUser
-	}
-	cred := ui.Credentials
-
-	if cred.IsTemp() || cred.IsServiceAccount() {
+	if ui, ok := cache.iamUsersMap[accessKey]; ok && (ui.Credentials.IsTemp() || ui.Credentials.IsServiceAccount()) {
 		return updatedAt, errIAMActionNotAllowed
 	}
-
-	uinfo := newUserIdentity(auth.Credentials{
-		AccessKey: accessKey,
-		SecretKey: cred.SecretKey,
-		Status: func() string {
-			switch string(status) {
-			case string(madmin.AccountEnabled), string(auth.AccountOn):
-				return auth.AccountOn
-			}
-			return auth.AccountOff
-		}(),
+	u, _, err := store.editStoredIdentity(ctx, cache, accessKey, regUser, func(stored *UserIdentity) error {
+		if stored.Credentials.IsTemp() || stored.Credentials.IsServiceAccount() {
+			return errIAMActionNotAllowed
+		}
+		stored.Credentials.Status = auth.AccountOff
+		if status == madmin.AccountEnabled {
+			stored.Credentials.Status = auth.AccountOn
+		}
+		return nil
 	})
-
-	if err := store.saveUserIdentity(ctx, accessKey, regUser, &uinfo); err != nil {
+	if errors.Is(err, errIAMIdentityGone) {
+		return updatedAt, errNoSuchUser
+	}
+	if err != nil {
 		return updatedAt, err
 	}
-
-	if err := cache.updateUserWithClaims(accessKey, uinfo); err != nil {
-		return updatedAt, err
-	}
-
-	return uinfo.UpdatedAt, nil
+	return u.UpdatedAt, nil
 }

 // AddServiceAccount - add a new service account
@@ -2863,15 +2852,39 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st
 	cache := store.lock()
 	defer store.unlock()

-	ui, ok := cache.iamUsersMap[accessKey]
-	if !ok || !ui.Credentials.IsServiceAccount() {
+	u, _, err := store.editStoredIdentity(ctx, cache, accessKey, svcUser, func(stored *UserIdentity) error {
+		if !stored.Credentials.IsServiceAccount() {
+			return errNoSuchServiceAccount
+		}
+		if opts.parentUser != "" && stored.Credentials.ParentUser != opts.parentUser {
+			// Not the account the caller was authorized for: it was
+			// deleted and created again for another parent meanwhile.
+			return errNoSuchServiceAccount
+		}
+		cr, err := updatedServiceAccountCredentials(ctx, accessKey, stored.Credentials, opts)
+		if err != nil {
+			return err
+		}
+		stored.Credentials = cr
+		return nil
+	})
+	if errors.Is(err, errIAMIdentityGone) {
 		return updatedAt, errNoSuchServiceAccount
 	}
-	cr := ui.Credentials
+	if err != nil {
+		return updatedAt, err
+	}
+	return u.UpdatedAt, nil
+}
+
+// updatedServiceAccountCredentials returns cr, the stored credential of
+// service account accessKey, with the edits of opts and a session token
+// signed again for them.
+func updatedServiceAccountCredentials(ctx context.Context, accessKey string, cr auth.Credentials, opts updateServiceAccountOpts) (auth.Credentials, error) {
 	currentSecretKey := cr.SecretKey
 	if opts.secretKey != "" {
 		if !auth.IsSecretKeyValid(opts.secretKey) {
-			return updatedAt, auth.ErrInvalidSecretKeyLength
+			return cr, auth.ErrInvalidSecretKeyLength
 		}
 		cr.SecretKey = opts.secretKey
 	}
@@ -2887,7 +2900,7 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st
 	if opts.expiration != nil {
 		expirationInUTC := opts.expiration.UTC()
 		if err := validateSvcExpirationInUTC(ctx, expirationInUTC); err != nil {
-			return updatedAt, err
+			return cr, err
 		}
 		cr.Expiration = expirationInUTC
 	}
@@ -2903,12 +2916,12 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st
 	case auth.AccountOn, auth.AccountOff:
 		cr.Status = opts.status
 	default:
-		return updatedAt, errors.New("unknown account status value")
+		return cr, errors.New("unknown account status value")
 	}

 	m, err := getClaimsFromTokenWithSecret(cr.SessionToken, currentSecretKey)
 	if err != nil {
-		return updatedAt, fmt.Errorf("unable to get svc acc claims: %v", err)
+		return cr, fmt.Errorf("unable to get svc acc claims: %v", err)
 	}

 	// Extracted session policy name string can be removed as its not useful
@@ -2926,17 +2939,17 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st

 	if opts.sessionPolicy != nil { // session policies is being updated
 		if err := opts.sessionPolicy.Validate(); err != nil {
-			return updatedAt, err
+			return cr, err
 		}

 		if opts.sessionPolicy.Version != "" && len(opts.sessionPolicy.Statements) > 0 {
 			policyBuf, err := json.Marshal(opts.sessionPolicy)
 			if err != nil {
-				return updatedAt, err
+				return cr, err
 			}

 			if len(policyBuf) > maxSVCSessionPolicySize {
-				return updatedAt, errSessionPolicyTooLarge
+				return cr, errSessionPolicyTooLarge
 			}

 			// Overwrite session policy claims.
@@ -2947,21 +2960,11 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st

 	cr.SessionToken, err = auth.JWTSignWithAccessKey(accessKey, m.Map(), cr.SecretKey)
 	if err != nil {
-		return updatedAt, err
+		return cr, err
 	}
-
-	u := newUserIdentity(cr)
-	if err := store.saveUserIdentity(ctx, u.Credentials.AccessKey, svcUser, &u); err != nil {
-		return updatedAt, err
-	}
-
-	if err := cache.updateUserWithClaims(u.Credentials.AccessKey, u); err != nil {
-		delete(cache.iamUsersMap, accessKey)
-		cache.updatedAt = UTCNow()
-		return updatedAt, err
-	}
-
-	return u.UpdatedAt, nil
+	// The stored claims are those of the new token; the save extracts them.
+	cr.Claims = nil
+	return cr, nil
 }

 // ListTempAccounts - lists only temporary accounts from the cache.
@@ -3105,20 +3108,14 @@ func (store *IAMStoreSys) UpdateUserSecretKey(ctx context.Context, accessKey, se
 	cache := store.lock()
 	defer store.unlock()

-	cache.updatedAt = time.Now()
-
-	ui, ok := cache.iamUsersMap[accessKey]
-	if !ok {
+	_, _, err := store.editStoredIdentity(ctx, cache, accessKey, regUser, func(stored *UserIdentity) error {
+		stored.Credentials.SecretKey = secretKey
+		return nil
+	})
+	if errors.Is(err, errIAMIdentityGone) {
 		return errNoSuchUser
 	}
-	cred := ui.Credentials
-	cred.SecretKey = secretKey
-	u := newUserIdentity(cred)
-	if err := store.saveUserIdentity(ctx, accessKey, regUser, &u); err != nil {
-		return err
-	}
-
-	return cache.updateUserWithClaims(accessKey, u)
+	return err
 }

 // GetSTSAndServiceAccounts - returns all STS and Service account credentials.
@@ -3146,10 +3143,10 @@ func (store *IAMStoreSys) getSTSAndServiceAccounts(cache *iamCache) []auth.Crede

 // RefreshUserGroups sets the groups of the STS or service-account
 // credential that cred was read from to groups, on the record stored now
-// and under its revision lock (see iamGroupRefresh). Nothing else of the
-// record changes. A credential that is gone, expired or now has another
-// parent is left alone in storage and dropped from the cache, which then
-// loads what is stored on its next use. It reports whether it wrote.
+// (see editStoredIdentity). Nothing else of the record changes. A
+// credential that is gone, expired or now has another parent, whose groups
+// the directory lookup did not compute, is left alone in storage and
+// dropped from the cache. It reports whether it wrote.
 func (store *IAMStoreSys) RefreshUserGroups(ctx context.Context, cred auth.Credentials, groups []string) (bool, error) {
 	cache := store.lock()
 	defer store.unlock()
@@ -3161,26 +3158,64 @@ func (store *IAMStoreSys) RefreshUserGroups(ctx context.Context, cred auth.Crede
 		}
 		userType = stsUser
 	}
-	op := &iamGroupRefresh{accessKey: cred.AccessKey, parentUser: cred.ParentUser, groups: groups}
-	err := store.saveIAMRevision(ctx, getUserIdentityPath(cred.AccessKey, userType), op)
-	if errors.Is(err, errIAMStaleUpdate) {
-		// The cache still holds the stale record; storage says otherwise.
-		delete(cache.iamUsersMap, cred.AccessKey)
-		delete(cache.iamSTSAccountsMap, cred.AccessKey)
-		cache.updatedAt = time.Now()
-		if store.group != nil {
-			store.group.Forget(cred.AccessKey)
+	_, unchanged, err := store.editStoredIdentity(ctx, cache, cred.AccessKey, userType, func(stored *UserIdentity) error {
+		if stored.Credentials.ParentUser != cred.ParentUser || stored.Credentials.IsExpired() {
+			return errIAMIdentityGone
 		}
+		if set.CreateStringSet(stored.Credentials.Groups...).Equals(set.CreateStringSet(groups...)) {
+			return errIAMIdentityUnchanged
+		}
+		stored.Credentials.Groups = groups
+		return nil
+	})
+	if errors.Is(err, errIAMIdentityGone) {
 		return false, nil
 	}
 	if err != nil {
 		return false, err
 	}
-	// The stored record is authoritative, also when it had the groups.
-	if err := cache.updateUserWithClaims(cred.AccessKey, op.UserIdentity); err != nil {
-		return false, err
+	return !unchanged, nil
+}
+
+// editStoredIdentity applies edit to identity accessKey of userType as it
+// is stored now, under its revision lock (see iamIdentityEdit), and caches
+// the result. The caller holds the cache lock. When the identity is absent
+// or deleted in storage, or edit returns errIAMIdentityGone because it is
+// no longer the identity the edit addressed, the cached copy is dropped, so
+// a lookup loads what is stored, and errIAMIdentityGone is returned.
+func (store *IAMStoreSys) editStoredIdentity(ctx context.Context, cache *iamCache, accessKey string, userType IAMUserType, edit func(u *UserIdentity) error) (UserIdentity, bool, error) {
+	op := &iamIdentityEdit{accessKey: accessKey, edit: edit}
+	err := store.saveIAMRevision(ctx, getUserIdentityPath(accessKey, userType), op)
+	if errors.Is(err, errIAMIdentityGone) {
+		store.forgetCachedIdentity(cache, accessKey, userType)
+	}
+	if err != nil {
+		return UserIdentity{}, false, err
+	}
+	if err := cache.updateUserWithClaims(accessKey, op.UserIdentity); err != nil {
+		// Stored but not cached: a lookup loads it.
+		store.forgetCachedIdentity(cache, accessKey, userType)
+		return UserIdentity{}, false, err
+	}
+	return op.UserIdentity, op.unchanged, nil
+}
+
+// forgetCachedIdentity drops identity accessKey of userType from the cache;
+// a lookup then loads it from storage. Users and service accounts share a
+// map, so an entry of the other kind under the same key is kept.
+func (store *IAMStoreSys) forgetCachedIdentity(cache *iamCache, accessKey string, userType IAMUserType) {
+	switch userType {
+	case stsUser:
+		delete(cache.iamSTSAccountsMap, accessKey)
+	default:
+		if ui, ok := cache.iamUsersMap[accessKey]; ok && ui.Credentials.IsServiceAccount() == (userType == svcUser) {
+			delete(cache.iamUsersMap, accessKey)
+		}
+	}
+	cache.updatedAt = time.Now()
+	if store.group != nil {
+		store.group.Forget(accessKey)
 	}
-	return !op.unchanged, nil
 }

 // LoadUser - attempts to load user info from storage and updates cache.
@@ -0,0 +1,233 @@
+// Copyright (c) 2026 PGSTY
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+package cmd
+
+import (
+	"errors"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/minio/madmin-go/v3"
+	"github.com/minio/minio/internal/auth"
+	"github.com/pgsty/silo-pkg/v3/policy"
+)
+
+// An administrator edits an identity through a server whose cache missed
+// a rotation or a deletion made through another. The edit applies to what
+// is stored and must not write the stale copy back.
+
+func TestIAMStaleServiceAccountEdit(t *testing.T) {
+	t.Run("rotated", func(t *testing.T) {
+		f := newGroupRefreshFixture(t)
+		snapshot := f.service(t, "edit-rotated")
+		other := f.sibling(t)
+		_, err := f.sys.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{secretKey: "edit-rotated-secret"})
+		mustIAM(t, err)
+		if cached, _ := other.GetUser(snapshot.AccessKey); cached.Credentials.SecretKey != snapshot.SecretKey {
+			t.Fatal("control: the stale server must not have heard of the rotation")
+		}
+		_, err = other.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{description: "edited on a stale server"})
+		mustIAM(t, err)
+		for where, get := range map[string]func() (UserIdentity, bool){
+			"storage":      func() (UserIdentity, bool) { return f.stored(t, snapshot.AccessKey) },
+			"stale server": func() (UserIdentity, bool) { return other.GetUser(snapshot.AccessKey) },
+		} {
+			u, ok := get()
+			if !ok || u.Credentials.SecretKey != "edit-rotated-secret" || u.Credentials.Description != "edited on a stale server" {
+				t.Errorf("%s: present=%v rotated=%v description=%q: the edit must apply to the rotated credential",
+					where, ok, u.Credentials.SecretKey == "edit-rotated-secret", u.Credentials.Description)
+			}
+		}
+	})
+	t.Run("deleted", func(t *testing.T) {
+		f := newGroupRefreshFixture(t)
+		snapshot := f.service(t, "edit-deleted")
+		other := f.sibling(t)
+		mustIAM(t, f.sys.DeleteServiceAccount(f.ctx, snapshot.AccessKey, false))
+		_, err := other.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{description: "edited on a stale server"})
+		if !errors.Is(err, errNoSuchServiceAccount) {
+			t.Errorf("an edit of a deleted account must fail with no such service account, got %v", err)
+		}
+		if _, ok := f.stored(t, snapshot.AccessKey); ok {
+			t.Error("an edit on a stale server brought a deleted account back")
+		}
+		if _, ok := other.GetUser(snapshot.AccessKey); ok {
+			t.Error("the stale server still caches the deleted account")
+		}
+	})
+}
+
+// A session policy edit stores the claims of the token it signs.
+func TestIAMServiceAccountEditStoresNewClaims(t *testing.T) {
+	f := newGroupRefreshFixture(t)
+	snapshot := f.service(t, "edit-claims")
+	sp, err := policy.ParseConfig(strings.NewReader(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::edit/*"]}]}`))
+	mustIAM(t, err)
+	_, err = f.sys.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{sessionPolicy: sp})
+	mustIAM(t, err)
+	var raw UserIdentity
+	mustIAM(t, f.sys.store.loadIAMConfig(f.ctx, &raw, getUserIdentityPath(snapshot.AccessKey, svcUser)))
+	claims, err := getClaimsFromTokenWithSecret(raw.Credentials.SessionToken, raw.Credentials.SecretKey)
+	mustIAM(t, err)
+	if raw.Credentials.Claims[iamPolicyClaimNameSA()] != embeddedPolicyType || claims.MapClaims[iamPolicyClaimNameSA()] != embeddedPolicyType {
+		t.Errorf("stored claims %v, token claims %v: both must name the embedded policy", raw.Credentials.Claims[iamPolicyClaimNameSA()], claims.MapClaims[iamPolicyClaimNameSA()])
+	}
+}
+
+func TestIAMStaleUserEdit(t *testing.T) {
+	for _, tc := range []struct {
+		name   string
+		change func(t *testing.T, f *groupRefreshFixture, user auth.Credentials)
+		edit   func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error
+		check  func(t *testing.T, u UserIdentity, ok bool, err error)
+	}{
+		{"status-after-delete", func(t *testing.T, f *groupRefreshFixture, user auth.Credentials) {
+			mustIAM(t, f.sys.DeleteUser(f.ctx, user.AccessKey, false))
+		}, func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error {
+			_, err := s.SetUserStatus(f.ctx, user.AccessKey, madmin.AccountEnabled)
+			return err
+		}, func(t *testing.T, u UserIdentity, ok bool, err error) {
+			if !errors.Is(err, errNoSuchUser) || ok {
+				t.Errorf("err=%v present=%v: a status change on a stale server must not bring a deleted user back", err, ok)
+			}
+		}},
+		{"status-after-rotation", func(t *testing.T, f *groupRefreshFixture, user auth.Credentials) {
+			mustIAM(t, f.sys.SetUserSecretKey(f.ctx, user.AccessKey, "edit-user-rotated"))
+		}, func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error {
+			_, err := s.SetUserStatus(f.ctx, user.AccessKey, madmin.AccountDisabled)
+			return err
+		}, func(t *testing.T, u UserIdentity, ok bool, err error) {
+			if err != nil || !ok || u.Credentials.SecretKey != "edit-user-rotated" || u.Credentials.Status != auth.AccountOff {
+				t.Errorf("err=%v present=%v rotated=%v status=%q: the status change must apply to the rotated user",
+					err, ok, u.Credentials.SecretKey == "edit-user-rotated", u.Credentials.Status)
+			}
+		}},
+		{"secret-after-delete", func(t *testing.T, f *groupRefreshFixture, user auth.Credentials) {
+			mustIAM(t, f.sys.DeleteUser(f.ctx, user.AccessKey, false))
+		}, func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error {
+			return s.UpdateUserSecretKey(f.ctx, user.AccessKey, "edit-user-new-secret")
+		}, func(t *testing.T, u UserIdentity, ok bool, err error) {
+			if !errors.Is(err, errNoSuchUser) || ok {
+				t.Errorf("err=%v present=%v: a secret change on a stale server must not bring a deleted user back", err, ok)
+			}
+		}},
+		{"secret-after-disable", func(t *testing.T, f *groupRefreshFixture, user auth.Credentials) {
+			_, err := f.sys.SetUserStatus(f.ctx, user.AccessKey, madmin.AccountDisabled)
+			mustIAM(t, err)
+		}, func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error {
+			return s.UpdateUserSecretKey(f.ctx, user.AccessKey, "edit-user-new-secret")
+		}, func(t *testing.T, u UserIdentity, ok bool, err error) {
+			if err != nil || !ok || u.Credentials.SecretKey != "edit-user-new-secret" || u.Credentials.Status != auth.AccountOff {
+				t.Errorf("err=%v present=%v status=%q: a secret change on a stale server must keep the user disabled", err, ok, u.Credentials.Status)
+			}
+		}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			f := newGroupRefreshFixture(t)
+			user := iamAdminUser(f.ctx, t, f.sys, "edit-user", "refresh-parent")
+			other := f.sibling(t)
+			tc.change(t, f, user)
+			if cached, ok := other.GetUser(user.AccessKey); !ok || cached.Credentials.SecretKey != user.SecretKey {
+				t.Fatal("control: the stale server must not have heard of the change")
+			}
+			err := tc.edit(other, f, user)
+			u, ok := f.stored(t, user.AccessKey)
+			tc.check(t, u, ok, err)
+		})
+	}
+}
+
+// An edit that the caller was authorized for under one parent does not
+// apply to an account stored under another parent: the account was deleted
+// and created again for it meanwhile.
+func TestIAMServiceAccountEditKeepsAuthorizedParent(t *testing.T) {
+	f := newGroupRefreshFixture(t)
+	snapshot := f.service(t, "edit-parent")
+	other := iamAdminUser(f.ctx, t, f.sys, "edit-other-parent", "refresh-parent")
+	mustIAM(t, f.sys.DeleteServiceAccount(f.ctx, snapshot.AccessKey, false))
+	_, _, err := f.sys.NewServiceAccount(f.ctx, other.AccessKey, nil, newServiceAccountOpts{accessKey: snapshot.AccessKey, secretKey: "edit-other-secret"})
+	mustIAM(t, err)
+	_, err = f.sys.store.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{secretKey: "edit-chosen-secret", parentUser: snapshot.ParentUser})
+	if !errors.Is(err, errNoSuchServiceAccount) {
+		t.Errorf("an edit authorized for another parent must fail, got %v", err)
+	}
+	if u, ok := f.stored(t, snapshot.AccessKey); !ok || u.Credentials.SecretKey != "edit-other-secret" {
+		t.Errorf("present=%v: the account of the other parent must keep its secret", ok)
+	}
+	// Without a bound parent (replication), the edit applies.
+	_, err = f.sys.store.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{description: "replicated"})
+	mustIAM(t, err)
+}
+
+// A record stored without claims, or with only a deprecated comment, is
+// edited as the loaders present it.
+func TestIAMServiceAccountEditLegacyRecord(t *testing.T) {
+	f := newGroupRefreshFixture(t)
+	snapshot := f.service(t, "edit-legacy")
+	path := getUserIdentityPath(snapshot.AccessKey, svcUser)
+	var raw UserIdentity
+	mustIAM(t, f.sys.store.loadIAMConfig(f.ctx, &raw, path))
+	raw.Credentials.Claims = nil
+	raw.Credentials.Description, raw.Credentials.Comment = "", "legacy comment"
+	mustIAM(t, f.sys.store.saveIAMConfig(f.ctx, &raw, path))
+	_, err := f.sys.store.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{status: string(madmin.AccountDisabled)})
+	mustIAM(t, err)
+	for where, get := range map[string]func() (UserIdentity, bool){
+		"storage": func() (UserIdentity, bool) { return f.stored(t, snapshot.AccessKey) },
+		"cache":   func() (UserIdentity, bool) { return f.sys.store.GetUser(snapshot.AccessKey) },
+	} {
+		u, ok := get()
+		if !ok || u.Credentials.Status != auth.AccountOff || u.Credentials.Description != "legacy comment" {
+			t.Errorf("%s: present=%v status=%q description=%q", where, ok, u.Credentials.Status, u.Credentials.Description)
+		}
+	}
+}
+
+// A peer's edit from before a deletion this site applied is acknowledged as
+// stale; one from after it is not found.
+func TestIAMReplicatedEditAfterDeletion(t *testing.T) {
+	f := newGroupRefreshFixture(t)
+	snapshot := f.service(t, "edit-repl")
+	user := iamAdminUser(f.ctx, t, f.sys, "edit-repl-user", "refresh-parent")
+	before := UTCNow()
+	mustIAM(t, f.sys.DeleteServiceAccount(f.ctx, snapshot.AccessKey, false))
+	mustIAM(t, f.sys.DeleteUser(f.ctx, user.AccessKey, false))
+	after := UTCNow().Add(time.Second)
+
+	_, err := f.sys.store.UpdateServiceAccount(withIAMReplicationTime(f.ctx, before), snapshot.AccessKey, updateServiceAccountOpts{description: "late"})
+	if iamReplicationError(err) != nil {
+		t.Errorf("an older service-account edit must be acknowledged, got %v", err)
+	}
+	_, err = f.sys.store.SetUserStatus(withIAMReplicationTime(f.ctx, before), user.AccessKey, madmin.AccountDisabled)
+	if iamReplicationError(err) != nil {
+		t.Errorf("an older status edit must be acknowledged, got %v", err)
+	}
+	if _, err = f.sys.store.UpdateServiceAccount(withIAMReplicationTime(f.ctx, after), snapshot.AccessKey, updateServiceAccountOpts{description: "late"}); !errors.Is(err, errNoSuchServiceAccount) {
+		t.Errorf("a newer edit of a deleted account must not be acknowledged as stale, got %v", err)
+	}
+	if _, ok := f.stored(t, snapshot.AccessKey); ok {
+		t.Error("a replicated edit brought a deleted account back")
+	}
+}
+
+// A status change addressed to a service account is refused as before, and
+// the account stays cached.
+func TestIAMUserStatusOnServiceAccount(t *testing.T) {
+	f := newGroupRefreshFixture(t)
+	snapshot := f.service(t, "edit-sa-status")
+	_, err := f.sys.store.SetUserStatus(f.ctx, snapshot.AccessKey, madmin.AccountDisabled)
+	if !errors.Is(err, errIAMActionNotAllowed) {
+		t.Errorf("got %v, want errIAMActionNotAllowed", err)
+	}
+	if _, ok := f.sys.store.GetUser(snapshot.AccessKey); !ok {
+		t.Error("a refused status change dropped the service account from the cache")
+	}
+	if err := f.sys.store.UpdateUserSecretKey(f.ctx, snapshot.AccessKey, "edit-not-a-user"); !errors.Is(err, errNoSuchUser) {
+		t.Errorf("a user secret change of a service account must find no user, got %v", err)
+	}
+	if u, ok := f.sys.store.GetUser(snapshot.AccessKey); !ok || u.Credentials.SecretKey != snapshot.SecretKey {
+		t.Error("a user secret change of a service account touched the account")
+	}
+}
@@ -1211,6 +1211,9 @@ type updateServiceAccountOpts struct {
 	status            string
 	name, description string
 	expiration        *time.Time
+	// parentUser, when set, is the parent the caller was authorized for; an
+	// account stored under another parent is refused as absent.
+	parentUser string
 }

 // UpdateServiceAccount - edit a service account