diff --git a/weed/s3api/auth_account_collapse_test.go b/weed/s3api/auth_account_collapse_test.go index 4f443e233..141c6440d 100644 --- a/weed/s3api/auth_account_collapse_test.go +++ b/weed/s3api/auth_account_collapse_test.go @@ -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") +} diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index fa066c4dd..8956a40f8 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -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 {