From 9fb4ff47e1e6dc5b9b7af3dea2d5ab5ad789e5ac Mon Sep 17 00:00:00 2001 From: ale Date: Sun, 23 Jun 2019 11:21:30 +0100 Subject: [PATCH] Do not issue duplicated changes in the same LDAP "transaction" Prevents duplicate insertion of storageEncryptedSecretKey when requesting an account recovery with opportunistic encryption enabled. Add an integration test to cover this scenario. --- backend/ldap/tx.go | 18 +++++++++++++++--- integrationtest/account_mgmt_test.go | 26 ++++++++++++++++++-------- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/backend/ldap/tx.go b/backend/ldap/tx.go index a8786df..079c405 100644 --- a/backend/ldap/tx.go +++ b/backend/ldap/tx.go @@ -43,7 +43,7 @@ type ldapTX struct { cache map[string][]string newDNs map[string]struct{} // nolint (it's plural DN, not DNS) - changes []ldapAttr + changes []*ldapAttr } func newLDAPTX(conn ldapConn) *ldapTX { @@ -86,7 +86,19 @@ func (tx *ldapTX) setAttr(dn, attr string, values ...string) { if dn == "" { panic("empty dn in setAttr!") } - tx.changes = append(tx.changes, ldapAttr{dn: dn, attr: attr, values: values}) + // Reuse previous change, if any. Prevents value duplication + // in the same ModifyRequest. + found := false + for _, chg := range tx.changes { + if chg.dn == dn && chg.attr == attr { + chg.values = values + found = true + break + } + } + if !found { + tx.changes = append(tx.changes, &ldapAttr{dn: dn, attr: attr, values: values}) + } } // Commit the transaction, sending all changes to the LDAP server. @@ -154,7 +166,7 @@ func (tx *ldapTX) aggregateChanges(ctx context.Context) (map[string]*ldap.AddReq return adds, mods, dns } -func (tx *ldapTX) updateModifyRequest(ctx context.Context, mr *ldap.ModifyRequest, attr ldapAttr) { +func (tx *ldapTX) updateModifyRequest(ctx context.Context, mr *ldap.ModifyRequest, attr *ldapAttr) { old, ok := tx.cache[cacheKey(attr.dn, attr.attr)] // Pessimistic approach: if we haven't seen this attribute diff --git a/integrationtest/account_mgmt_test.go b/integrationtest/account_mgmt_test.go index c4c9bd1..0a39599 100644 --- a/integrationtest/account_mgmt_test.go +++ b/integrationtest/account_mgmt_test.go @@ -131,29 +131,39 @@ func runChangeUserPasswordTest(t *testing.T, username string, cfg as.Config) *as } func TestIntegration_AccountRecovery(t *testing.T) { - runAccountRecoveryTest(t, "uno@investici.org", false) + runAccountRecoveryTest(t, "uno@investici.org", false, false) } func TestIntegration_AccountRecovery_WithEncryptionKeys(t *testing.T) { - user := runAccountRecoveryTest(t, "due@investici.org", false) + user := runAccountRecoveryTest(t, "due@investici.org", false, false) if !user.HasEncryptionKeys { - t.Fatalf("encryption keys not enabled after account recovery") + t.Fatal("encryption keys not enabled after account recovery") + } +} + +func TestIntegration_AccountRecovery_WithOpportunisticEncryption(t *testing.T) { + user := runAccountRecoveryTest(t, "uno@investici.org", false, true) + if !user.HasEncryptionKeys { + t.Fatal("encryption keys not enabled after account recovery") } } func TestIntegration_AccountRecovery_WithCache(t *testing.T) { - runAccountRecoveryTest(t, "uno@investici.org", true) + runAccountRecoveryTest(t, "uno@investici.org", true, false) } func TestIntegration_AccountRecovery_WithEncryptionKeysAndCache(t *testing.T) { - user := runAccountRecoveryTest(t, "due@investici.org", true) + user := runAccountRecoveryTest(t, "due@investici.org", true, false) if !user.HasEncryptionKeys { - t.Fatalf("encryption keys not enabled after account recovery") + t.Fatal("encryption keys not enabled after account recovery") } } -func runAccountRecoveryTest(t *testing.T, username string, enableCache bool) *as.RawUser { - stop, be, c := startService(t) +func runAccountRecoveryTest(t *testing.T, username string, enableCache, enableOpportunisticEncryption bool) *as.RawUser { + cfg := as.Config{ + EnableOpportunisticEncryption: enableOpportunisticEncryption, + } + stop, be, c := startServiceWithConfig(t, cfg) defer stop() hint := "secret code?" -- GitLab