Make LDAP "transactions" self-consistent

The data written will be available for reading even before calling
Commit on the transaction. This allows constructs such as loops that
repeatedly look up and modify the same attribute to work, which fixes
the behavior of deleteAllApplicationSpecificPasswords() which
previously would only succeed in deleting one of them.

Fixes issue ai3/prod#233.

backend/ldap/tx.go

@@ -41,15 +41,28 @@ type ldapAttr struct {
 type ldapTX struct {
 	conn ldapConn
 
-	cache   map[string][]string
-	newDNs  map[string]struct{} // nolint (it's plural DN, not DNS)
+	// Read cache, containing data from the db. Only used to
+	// figure out if we need to issue a ModifyRequest or an
+	// AddRequest at commit time.
+	rcache map[string][]string
+
+	// Write cache, used to store modified attributes. Allows the
+	// transaction to be self-consistent: you can read the data
+	// that you've just written even before calling Commit().
+	wcache map[string][]string
+
+	// List of the new DNs that need to be created.
+	// nolint: it's plural DN, not DNS.
+	newDNs map[string]struct{}
+
 	changes []*ldapAttr
 }
 
 func newLDAPTX(conn ldapConn) *ldapTX {
 	return &ldapTX{
 		conn:   conn,
-		cache:  make(map[string][]string),
+		rcache: make(map[string][]string),
+		wcache: make(map[string][]string),
 		newDNs: make(map[string]struct{}),
 	}
 }
@@ -67,7 +80,7 @@ func (tx *ldapTX) search(ctx context.Context, req *ldap.SearchRequest) (*ldap.Se
 
 	for _, entry := range res.Entries {
 		for _, attr := range entry.Attributes {
-			tx.cache[cacheKey(entry.DN, attr.Name)] = attr.Values
+			tx.rcache[cacheKey(entry.DN, attr.Name)] = attr.Values
 		}
 	}
 
@@ -86,6 +99,10 @@ func (tx *ldapTX) setAttr(dn, attr string, values ...string) {
 	if dn == "" {
 		panic("empty dn in setAttr!")
 	}
+
+	// Set the value in the transaction write cache.
+	tx.wcache[cacheKey(dn, attr)] = values
+
 	// Reuse previous change, if any. Prevents value duplication
 	// in the same ModifyRequest.
 	found := false
@@ -133,6 +150,7 @@ func (tx *ldapTX) Commit(ctx context.Context) error {
 	// Cleanup
 	tx.changes = nil
 	tx.newDNs = make(map[string]struct{})
+	tx.wcache = make(map[string][]string)
 
 	return nil
 }
@@ -167,14 +185,13 @@ func (tx *ldapTX) aggregateChanges(ctx context.Context) (map[string]*ldap.AddReq
 }
 
 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
 	// before, try to fetch it from LDAP so we know if we need to
 	// perform an Add or a Replace.
+	old, ok := tx.rcache[cacheKey(attr.dn, attr.attr)]
 	if !ok {
 		log.Printf("tx: pessimistic fallback for %s %s", attr.dn, attr.attr)
-		oldFromLDAP := tx.readAttributeValues(ctx, attr.dn, attr.attr)
+		oldFromLDAP := tx.readAttributeValuesNoCache(ctx, attr.dn, attr.attr)
 		if len(oldFromLDAP) > 0 {
 			ok = true
 			old = oldFromLDAP
@@ -191,7 +208,7 @@ func (tx *ldapTX) updateModifyRequest(ctx context.Context, mr *ldap.ModifyReques
 	}
 }
 
-func (tx *ldapTX) readAttributeValues(ctx context.Context, dn, attr string) []string {
+func (tx *ldapTX) readAttributeValuesNoCache(ctx context.Context, dn, attr string) []string {
 	result, err := tx.search(ctx, ldap.NewSearchRequest(
 		dn,
 		ldap.ScopeBaseObject,
@@ -209,6 +226,13 @@ func (tx *ldapTX) readAttributeValues(ctx context.Context, dn, attr string) []st
 	return nil
 }
 
+func (tx *ldapTX) readAttributeValues(ctx context.Context, dn, attr string) []string {
+	if values, ok := tx.wcache[cacheKey(dn, attr)]; ok {
+		return values
+	}
+	return tx.readAttributeValuesNoCache(ctx, dn, attr)
+}
+
 func isEmptyModifyRequest(mr *ldap.ModifyRequest) bool {
 	return len(mr.Changes) == 0
 }

integrationtest/account_mgmt_test.go

@@ -159,6 +159,13 @@ func TestIntegration_AccountRecovery_WithEncryptionKeysAndCache(t *testing.T) {
 	}
 }
 
+func TestIntegration_AccountRecovery_ClearsAppSpecificPasswords(t *testing.T) {
+	user := runAccountRecoveryTest(t, "tre@investici.org", false, false)
+	if len(user.AppSpecificPasswords) > 0 {
+		t.Fatal("app-specific passwords were not cleared after account recovery")
+	}
+}
+
 func runAccountRecoveryTest(t *testing.T, username string, enableCache, enableOpportunisticEncryption bool) *as.RawUser {
 	cfg := as.Config{
 		EnableOpportunisticEncryption: enableOpportunisticEncryption,
@@ -247,7 +254,14 @@ func TestIntegration_AppSpecificPassword(t *testing.T) {
 	if err != nil {
 		t.Fatalf("GetUser error: %v", err)
 	}
-	if len(user.AppSpecificPasswords) == 0 {
-		t.Errorf("no ASPs were retrieved: %+v", user)
+	found := false
+	for _, asp := range user.AppSpecificPasswords {
+		if asp.Service == "service" {
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Errorf("could not find the ASPs that was just created: %+v", user)
 	}
 }

integrationtest/testdata/test3.ldif

@@ -20,6 +20,8 @@ uid: tre@investici.org
 uidNumber: 256799
 userPassword:: JGEyJDQkMzI3NjgkMSQwZDgyMzU1YjQ0Mzg0M2NmZDY4MjU1MzE4ZTVjYTdiZSRmNTQ0ODkxOTFiNWZlYzk2MDRlNWQ2ODZjMDQxZjJkNTFmOTgxOGY4ZTFmM2E4MDYzY2U3ZTEwMTE3OTc2OGI0
 totpSecret: ABCDEF
+appSpecificPassword: id1:email:encryptedpassword:comment
+appSpecificPassword: id2:jabber:encryptedpassword:comment
 status: active
 host: host2