s3: register an identity's inline account instead of collapsing it into admin (#10548)

* s3: register an identity's inline account instead of collapsing it into admin Credential stores persist an account inline on the identity and never emit a top-level accounts list, so every user created through the IAM API or the admin UI with an email hit the "non exist account ID" branch and was given the shared admin account. Distinct users then presented the same owner id, so ownership checks could not tell them apart and each passed for the others' buckets. Treat an id missing from the account map as undeclared rather than invalid: register it, keeping an email another account already claimed. Both load paths now resolve the account through one helper. * s3: refresh an undeclared account from the identity that carries it The merge path starts from the live account cache, so an identity upserted with the same account id but a new email or display name kept the cached copy: the new address never reached the email index and the replaced one still resolved. Changing a user's email through the admin UI takes exactly that path. An account registered from an inline block is only described by the identity carrying it, so refresh it and move its email claim. Accounts from a top-level list and the predefined defaults are marked declared and stay authoritative. * s3: let an account reclaim an email once its holder moves away Two identities can carry the same email, and the second to load leaves the lookup with the first. Returning early when the incoming metadata matches the cached account meant the loser never re-ran the claim, so an address freed by the holder's update resolved to nobody until the loser itself changed. Re-index on the unchanged path, which is a no-op while another account still holds the address.
2026-08-18 21:26:56 +00:00 · 2026-08-03 12:45:42 -07:00
parent 846f9f7e7d
commit cc2775d9f2
2 changed files with 248 additions and 44 deletions
@@ -7,6 +7,7 @@ import (
 	"testing"

 	"github.com/aws/aws-sdk-go/service/s3"
+	"github.com/seaweedfs/seaweedfs/weed/pb/iam_pb"
 	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
 	"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
 	"github.com/stretchr/testify/assert"
@@ -125,3 +126,167 @@ func TestUnscopedIdentityReusesConfiguredAccount(t *testing.T) {
 	assert.Equal(t, "Alice Smith", alice.Account.DisplayName,
 		"identity must reuse the configured account, not the synthesized one")
 }
+
+// Users created through the IAM API carry their account inline on the identity,
+// and no credential store emits a top-level accounts list. Such an account must
+// be registered rather than collapsed into the admin account, which gave every
+// one of those users the same owner id for ownership checks.
+func TestInlineIdentityAccountIsRegistered(t *testing.T) {
+	resetMemoryStore()
+
+	config := `{
+  "identities": [
+    {"name": "alice", "account": {"id": "100000000001", "displayName": "Alice", "emailAddress": "alice@example.com"}, "credentials": [{"accessKey": "alice_ak", "secretKey": "alice_sk"}], "actions": ["Read", "Write"]},
+    {"name": "bob", "account": {"id": "100000000002", "displayName": "Bob", "emailAddress": "bob@example.com"}, "credentials": [{"accessKey": "bob_ak", "secretKey": "bob_sk"}], "actions": ["Read", "Write"]}
+  ]
+}`
+	tmp, err := os.CreateTemp("", "s3-config-*.json")
+	require.NoError(t, err)
+	defer os.Remove(tmp.Name())
+	_, err = tmp.WriteString(config)
+	require.NoError(t, err)
+	require.NoError(t, tmp.Close())
+
+	iam := NewIdentityAccessManagementWithStore(&S3ApiServerOption{Config: tmp.Name()}, nil, "memory")
+
+	alice, _, found := iam.LookupByAccessKey("alice_ak")
+	require.True(t, found)
+	require.NotNil(t, alice.Account)
+	assert.Equal(t, "100000000001", alice.Account.Id, "an undeclared inline account must not collapse into admin")
+
+	bob, _, found := iam.LookupByAccessKey("bob_ak")
+	require.True(t, found)
+	require.NotNil(t, bob.Account)
+	assert.NotEqual(t, alice.Account.Id, bob.Account.Id, "distinct users must not share an owner id")
+
+	assert.Equal(t, "Alice", iam.GetAccountNameById("100000000001"), "the registered account must resolve for ACL/owner display")
+	assert.Equal(t, "100000000002", iam.GetAccountIdByEmail("bob@example.com"), "the registered account must resolve by email")
+}
+
+// A dynamic update carries a single identity with no accounts list, so the merge
+// path must register the inline account the same way the full load does.
+func TestInlineIdentityAccountIsRegisteredOnUpsert(t *testing.T) {
+	resetMemoryStore()
+
+	config := `{
+  "identities": [
+    {"name": "admin", "credentials": [{"accessKey": "admin_ak", "secretKey": "admin_sk"}], "actions": ["Admin"]}
+  ]
+}`
+	tmp, err := os.CreateTemp("", "s3-config-*.json")
+	require.NoError(t, err)
+	defer os.Remove(tmp.Name())
+	_, err = tmp.WriteString(config)
+	require.NoError(t, err)
+	require.NoError(t, tmp.Close())
+
+	iam := NewIdentityAccessManagementWithStore(&S3ApiServerOption{Config: tmp.Name()}, nil, "memory")
+
+	require.NoError(t, iam.UpsertIdentity(&iam_pb.Identity{
+		Name:        "alice",
+		Account:     &iam_pb.Account{Id: "100000000001", DisplayName: "Alice", EmailAddress: "alice@example.com"},
+		Credentials: []*iam_pb.Credential{{AccessKey: "alice_ak", SecretKey: "alice_sk"}},
+		Actions:     []string{"Read", "Write"},
+	}))
+
+	alice, _, found := iam.LookupByAccessKey("alice_ak")
+	require.True(t, found)
+	require.NotNil(t, alice.Account)
+	assert.Equal(t, "100000000001", alice.Account.Id, "a pushed identity must keep its own account id")
+	assert.NotEqual(t, AccountAdmin.Id, alice.Account.Id, "a pushed identity must not inherit the admin account")
+	assert.Equal(t, "Alice", iam.GetAccountNameById("100000000001"), "the registered account must resolve for ACL/owner display")
+
+	// Changing the email keeps the account id, so the merge starts from a cached
+	// account that still carries the old address.
+	require.NoError(t, iam.UpsertIdentity(&iam_pb.Identity{
+		Name:        "alice",
+		Account:     &iam_pb.Account{Id: "100000000001", DisplayName: "Alice Smith", EmailAddress: "alice.smith@example.com"},
+		Credentials: []*iam_pb.Credential{{AccessKey: "alice_ak", SecretKey: "alice_sk"}},
+		Actions:     []string{"Read", "Write"},
+	}))
+
+	assert.Equal(t, "100000000001", iam.GetAccountIdByEmail("alice.smith@example.com"), "the new email must be indexed")
+	assert.Empty(t, iam.GetAccountIdByEmail("alice@example.com"), "the replaced email must no longer resolve")
+	assert.Equal(t, "Alice Smith", iam.GetAccountNameById("100000000001"), "the display name must follow the update")
+}
+
+// Two inline accounts can carry the same email. The first to register keeps the
+// lookup, and the loser can claim it once the holder moves away — otherwise an
+// email freed by an update would resolve to nobody.
+func TestInlineAccountEmailClaimAndHandoff(t *testing.T) {
+	resetMemoryStore()
+
+	config := `{
+  "identities": [
+    {"name": "admin", "credentials": [{"accessKey": "admin_ak", "secretKey": "admin_sk"}], "actions": ["Admin"]}
+  ]
+}`
+	tmp, err := os.CreateTemp("", "s3-config-*.json")
+	require.NoError(t, err)
+	defer os.Remove(tmp.Name())
+	_, err = tmp.WriteString(config)
+	require.NoError(t, err)
+	require.NoError(t, tmp.Close())
+
+	iam := NewIdentityAccessManagementWithStore(&S3ApiServerOption{Config: tmp.Name()}, nil, "memory")
+
+	require.NoError(t, iam.UpsertIdentity(&iam_pb.Identity{
+		Name:        "alice",
+		Account:     &iam_pb.Account{Id: "100000000001", DisplayName: "Alice", EmailAddress: "shared@example.com"},
+		Credentials: []*iam_pb.Credential{{AccessKey: "alice_ak", SecretKey: "alice_sk"}},
+		Actions:     []string{"Read"},
+	}))
+	require.NoError(t, iam.UpsertIdentity(&iam_pb.Identity{
+		Name:        "bob",
+		Account:     &iam_pb.Account{Id: "100000000002", DisplayName: "Bob", EmailAddress: "shared@example.com"},
+		Credentials: []*iam_pb.Credential{{AccessKey: "bob_ak", SecretKey: "bob_sk"}},
+		Actions:     []string{"Read"},
+	}))
+
+	assert.Equal(t, "100000000001", iam.GetAccountIdByEmail("shared@example.com"), "the first account to claim an email keeps it")
+
+	// alice moves off the shared address, freeing it
+	require.NoError(t, iam.UpsertIdentity(&iam_pb.Identity{
+		Name:        "alice",
+		Account:     &iam_pb.Account{Id: "100000000001", DisplayName: "Alice", EmailAddress: "alice@example.com"},
+		Credentials: []*iam_pb.Credential{{AccessKey: "alice_ak", SecretKey: "alice_sk"}},
+		Actions:     []string{"Read"},
+	}))
+	assert.Equal(t, "100000000001", iam.GetAccountIdByEmail("alice@example.com"), "the moved account indexes its new email")
+
+	// bob re-syncs unchanged and picks up the address he never got to claim
+	require.NoError(t, iam.UpsertIdentity(&iam_pb.Identity{
+		Name:        "bob",
+		Account:     &iam_pb.Account{Id: "100000000002", DisplayName: "Bob", EmailAddress: "shared@example.com"},
+		Credentials: []*iam_pb.Credential{{AccessKey: "bob_ak", SecretKey: "bob_sk"}},
+		Actions:     []string{"Read"},
+	}))
+	assert.Equal(t, "100000000002", iam.GetAccountIdByEmail("shared@example.com"), "a freed email resolves to the account still holding it")
+}
+
+// An account declared in a top-level accounts list outranks an identity's inline
+// block, which must not rewrite its metadata or take over its email.
+func TestDeclaredAccountOutranksInlineIdentityAccount(t *testing.T) {
+	resetMemoryStore()
+
+	config := `{
+  "accounts": [
+    {"id": "100000000001", "displayName": "Alice Smith", "emailAddress": "alice@example.com"}
+  ],
+  "identities": [
+    {"name": "alice", "account": {"id": "100000000001", "displayName": "wrong", "emailAddress": "wrong@example.com"}, "credentials": [{"accessKey": "alice_ak", "secretKey": "alice_sk"}], "actions": ["Read"]}
+  ]
+}`
+	tmp, err := os.CreateTemp("", "s3-config-*.json")
+	require.NoError(t, err)
+	defer os.Remove(tmp.Name())
+	_, err = tmp.WriteString(config)
+	require.NoError(t, err)
+	require.NoError(t, tmp.Close())
+
+	iam := NewIdentityAccessManagementWithStore(&S3ApiServerOption{Config: tmp.Name()}, nil, "memory")
+
+	assert.Equal(t, "Alice Smith", iam.GetAccountNameById("100000000001"), "the declared display name must win")
+	assert.Equal(t, "100000000001", iam.GetAccountIdByEmail("alice@example.com"), "the declared email must stay indexed")
+	assert.Empty(t, iam.GetAccountIdByEmail("wrong@example.com"), "an inline block must not claim an email for a declared account")
+}
@@ -125,6 +125,11 @@ type Account struct {

 	//Id is used to identify an Account when granting cross-account access(ACLs) to buckets and objects
 	Id string
+
+	// declared marks an account from a top-level accounts list or a predefined
+	// default. An account registered from an identity's inline block is not
+	// declared, and the identity stays authoritative for its metadata.
+	declared bool
 }

 // Default account ID for all automated SeaweedFS accounts and fallback
@@ -137,6 +142,7 @@ var (
 		DisplayName:  "admin",
 		EmailAddress: "admin@example.com",
 		Id:           s3_constants.AccountAdminId,
+		declared:     true,
 	}

 	// AccountAnonymous is used to represent the account for anonymous access
@@ -144,6 +150,7 @@ var (
 		DisplayName:  "anonymous",
 		EmailAddress: "anonymous@example.com",
 		Id:           s3_constants.AccountAnonymousId,
+		declared:     true,
 	}
 )

@@ -160,6 +167,70 @@ func accountForUnscopedIdentity(name string) *Account {
 	}
 }

+// indexAccountEmail points an email at its account unless a different account
+// already claims it, so an inline block cannot take over a declared account's
+// email.
+func indexAccountEmail(emailAccount map[string]*Account, account *Account) {
+	if account.EmailAddress == "" {
+		return
+	}
+	if claimed, taken := emailAccount[account.EmailAddress]; taken && claimed.Id != account.Id {
+		return
+	}
+	emailAccount[account.EmailAddress] = account
+}
+
+// resolveIdentityAccount returns the account an identity owns resources under,
+// registering it in accounts when it is not already known so the id resolves
+// through GetAccountNameById. Credential stores persist an account inline on the
+// identity and never emit a top-level accounts list, so an id missing from
+// accounts means undeclared, not invalid: falling back to the admin account
+// would give every such identity the same owner id.
+//
+// An undeclared account is only described by the identity carrying it, so its
+// metadata is refreshed from every load — the merge path starts from the live
+// cache, and a user whose email changed would otherwise keep the old one
+// indexed. A declared account outranks the inline block and is left alone.
+func resolveIdentityAccount(ident *iam_pb.Identity, accounts map[string]*Account, emailAccount map[string]*Account) *Account {
+	if ident.Account == nil || ident.Account.Id == "" {
+		synthesized := accountForUnscopedIdentity(ident.Name)
+		if existing, ok := accounts[synthesized.Id]; ok {
+			return existing
+		}
+		accounts[synthesized.Id] = synthesized
+		return synthesized
+	}
+
+	account := &Account{
+		Id:           ident.Account.Id,
+		DisplayName:  ident.Account.DisplayName,
+		EmailAddress: ident.Account.EmailAddress,
+	}
+
+	existing, ok := accounts[account.Id]
+	if ok {
+		if existing.declared {
+			return existing
+		}
+		if existing.DisplayName == account.DisplayName && existing.EmailAddress == account.EmailAddress {
+			// an email this account lost to another one is claimable once freed
+			indexAccountEmail(emailAccount, existing)
+			return existing
+		}
+		glog.V(3).Infof("refreshing account %s from identity %s", account.Id, ident.Name)
+		// drop the email this account itself indexed; another account's claim stands
+		if claimed, indexed := emailAccount[existing.EmailAddress]; indexed && claimed.Id == existing.Id {
+			delete(emailAccount, existing.EmailAddress)
+		}
+	} else {
+		glog.V(3).Infof("registering account %s from identity %s", account.Id, ident.Name)
+	}
+
+	accounts[account.Id] = account
+	indexAccountEmail(emailAccount, account)
+	return account
+}
+
 type Credential struct {
 	AccessKey  string
 	SecretKey  string
@@ -639,6 +710,7 @@ func (iam *IdentityAccessManagement) ReplaceS3ApiConfiguration(config *iam_pb.S3
 			Id:           account.Id,
 			DisplayName:  account.DisplayName,
 			EmailAddress: account.EmailAddress,
+			declared:     true,
 		}
 		switch account.Id {
 		case AccountAdmin.Id:
@@ -655,6 +727,7 @@ func (iam *IdentityAccessManagement) ReplaceS3ApiConfiguration(config *iam_pb.S3
 			DisplayName:  AccountAdmin.DisplayName,
 			EmailAddress: AccountAdmin.EmailAddress,
 			Id:           AccountAdmin.Id,
+			declared:     true,
 		}
 		emailAccount[AccountAdmin.EmailAddress] = accounts[AccountAdmin.Id]
 	}
@@ -663,6 +736,7 @@ func (iam *IdentityAccessManagement) ReplaceS3ApiConfiguration(config *iam_pb.S3
 			DisplayName:  AccountAnonymous.DisplayName,
 			EmailAddress: AccountAnonymous.EmailAddress,
 			Id:           AccountAnonymous.Id,
+			declared:     true,
 		}
 		emailAccount[AccountAnonymous.EmailAddress] = accounts[AccountAnonymous.Id]
 	}
@@ -689,30 +763,11 @@ func (iam *IdentityAccessManagement) ReplaceS3ApiConfiguration(config *iam_pb.S3
 			Disabled:     ident.Disabled, // false (default) = enabled, true = disabled
 			PolicyNames:  ident.PolicyNames,
 		}
-		switch {
-		case ident.Name == AccountAnonymous.Id:
+		if ident.Name == AccountAnonymous.Id {
 			t.Account = &AccountAnonymous
 			identityAnonymous = t
-		case ident.Account == nil:
-			// Account-less identities own resources under a distinct id derived
-			// from their name. Reuse an explicitly-configured account with that
-			// id if one exists (preserving its display name/email); otherwise
-			// synthesize one and register it so the id resolves via
-			// GetAccountNameById (ACL grantee validation, owner display).
-			synthesized := accountForUnscopedIdentity(t.Name)
-			if existing, ok := accounts[synthesized.Id]; ok {
-				t.Account = existing
-			} else {
-				t.Account = synthesized
-				accounts[synthesized.Id] = synthesized
-			}
-		default:
-			if account, ok := accounts[ident.Account.Id]; ok {
-				t.Account = account
-			} else {
-				t.Account = &AccountAdmin
-				glog.Warningf("identity %s is associated with a non exist account ID, the association is invalid", ident.Name)
-			}
+		} else {
+			t.Account = resolveIdentityAccount(ident, accounts, emailAccount)
 		}

 		for _, action := range ident.Actions {
@@ -881,6 +936,7 @@ func (iam *IdentityAccessManagement) MergeS3ApiConfiguration(config *iam_pb.S3Ap
 				Id:           account.Id,
 				DisplayName:  account.DisplayName,
 				EmailAddress: account.EmailAddress,
+				declared:     true,
 			}
 			if account.EmailAddress != "" {
 				emailAccount[account.EmailAddress] = accounts[account.Id]
@@ -894,6 +950,7 @@ func (iam *IdentityAccessManagement) MergeS3ApiConfiguration(config *iam_pb.S3Ap
 			DisplayName:  AccountAdmin.DisplayName,
 			EmailAddress: AccountAdmin.EmailAddress,
 			Id:           AccountAdmin.Id,
+			declared:     true,
 		}
 		emailAccount[AccountAdmin.EmailAddress] = accounts[AccountAdmin.Id]
 	}
@@ -902,6 +959,7 @@ func (iam *IdentityAccessManagement) MergeS3ApiConfiguration(config *iam_pb.S3Ap
 			DisplayName:  AccountAnonymous.DisplayName,
 			EmailAddress: AccountAnonymous.EmailAddress,
 			Id:           AccountAnonymous.Id,
+			declared:     true,
 		}
 		emailAccount[AccountAnonymous.EmailAddress] = accounts[AccountAnonymous.Id]
 	}
@@ -926,30 +984,11 @@ func (iam *IdentityAccessManagement) MergeS3ApiConfiguration(config *iam_pb.S3Ap
 			IsStatic: fromStaticFile,
 		}

-		switch {
-		case ident.Name == AccountAnonymous.Id:
+		if ident.Name == AccountAnonymous.Id {
 			t.Account = &AccountAnonymous
 			identityAnonymous = t
-		case ident.Account == nil:
-			// Account-less identities own resources under a distinct id derived
-			// from their name. Reuse an explicitly-configured account with that
-			// id if one exists (preserving its display name/email); otherwise
-			// synthesize one and register it so the id resolves via
-			// GetAccountNameById (ACL grantee validation, owner display).
-			synthesized := accountForUnscopedIdentity(t.Name)
-			if existing, ok := accounts[synthesized.Id]; ok {
-				t.Account = existing
-			} else {
-				t.Account = synthesized
-				accounts[synthesized.Id] = synthesized
-			}
-		default:
-			if account, ok := accounts[ident.Account.Id]; ok {
-				t.Account = account
-			} else {
-				t.Account = &AccountAdmin
-				glog.Warningf("identity %s is associated with a non exist account ID, the association is invalid", ident.Name)
-			}
+		} else {
+			t.Account = resolveIdentityAccount(ident, accounts, emailAccount)
 		}

 		for _, action := range ident.Actions {