Keep the concurrent-save fix scoped to aura.go/credentials.go

Author: Liam-Doodson

common/clicfg/credentials/aura.go

@@ -14,6 +14,21 @@ import (
 type AuraCredentials struct {
 	DefaultCredential string            `json:"default-credential"`
 	Credentials       []*AuraCredential `json:"credentials"`
+	refresh           func() error
+	persist           func() error
+}
+
+// refreshAndPersist re-reads the on-disk state into c immediately before mutate runs, so mutate
+// always applies its delta on top of the current file rather than a snapshot that may have gone
+// stale since load. It only persists if mutate succeeds.
+func (c *AuraCredentials) refreshAndPersist(mutate func() error) error {
+	if err := c.refresh(); err != nil {
+		return err
+	}
+	if err := mutate(); err != nil {
+		return err
+	}
+	return c.persist()
 }
 
 func (c *AuraCredentials) List() []*AuraCredential {
@@ -31,6 +46,57 @@ func (config *AuraCredentials) Print(writer io.Writer) error {
 	return nil
 }
 
+func (c *AuraCredentials) Add(name string, clientId string, clientSecret string) error {
+	return c.refreshAndPersist(func() error {
+		for _, credential := range c.Credentials {
+			if credential.Name == name {
+				return clierr.NewUsageError("already have credential with name %s", name)
+			}
+		}
+
+		c.Credentials = append(c.Credentials, &AuraCredential{Name: name, ClientId: clientId, ClientSecret: clientSecret})
+		if len(c.Credentials) == 1 {
+			c.DefaultCredential = name
+		}
+		return nil
+	})
+}
+
+func (c *AuraCredentials) Remove(name string) error {
+	return c.refreshAndPersist(func() error {
+		var indexToRemove = -1
+
+		for i, credential := range c.Credentials {
+			if credential.Name == name {
+				indexToRemove = i
+				break
+			}
+		}
+
+		if indexToRemove == -1 {
+			return clierr.NewUsageError("could not find credential with name %s to remove", name)
+		}
+
+		if c.DefaultCredential == name {
+			c.DefaultCredential = ""
+		}
+
+		c.Credentials = append(c.Credentials[:indexToRemove], c.Credentials[indexToRemove+1:]...)
+		return nil
+	})
+}
+
+func (c *AuraCredentials) SetDefault(name string) error {
+	return c.refreshAndPersist(func() error {
+		if !c.credentialExists(name) {
+			return clierr.NewUsageError("could not find credential with name %s", name)
+		}
+
+		c.DefaultCredential = name
+		return nil
+	})
+}
+
 func (c *AuraCredentials) GetDefault() (*AuraCredential, error) {
 	if c.DefaultCredential == "" {
 		return nil, clierr.NewUsageError("default credential not set, please follow the instructions at https://neo4j.com/docs/aura/classic/platform/api/authentication/#_creating_credentials and use the `credential add` subcommand to add the created credentials")
@@ -47,6 +113,47 @@ func (c *AuraCredentials) Get(name string) (*AuraCredential, error) {
 	return nil, clierr.NewUsageError("could not find credential with name %s", name)
 }
 
+func (c *AuraCredentials) UpdateAccessToken(cred *AuraCredential, accessToken string, expiresInSeconds int64) *AuraCredential {
+	var credential *AuraCredential
+	err := c.refreshAndPersist(func() error {
+		var err error
+		credential, err = c.Get(cred.Name)
+		if err != nil {
+			return err
+		}
+
+		const expireToleranceSeconds = 60
+		now := time.Now().UnixMilli()
+
+		credential.TokenExpiry = now + (expiresInSeconds-expireToleranceSeconds)*1000
+		credential.AccessToken = accessToken
+		return nil
+	})
+	if err != nil {
+		panic(err)
+	}
+	return credential
+}
+
+func (c *AuraCredentials) ClearAccessToken(cred *AuraCredential) (*AuraCredential, error) {
+	var credential *AuraCredential
+	err := c.refreshAndPersist(func() error {
+		var err error
+		credential, err = c.Get(cred.Name)
+		if err != nil {
+			return err
+		}
+
+		credential.TokenExpiry = 0
+		credential.AccessToken = ""
+		return nil
+	})
+	if err != nil {
+		return nil, err
+	}
+	return credential, nil
+}
+
 func (c *AuraCredentials) credentialExists(name string) bool {
 	for _, credential := range c.Credentials {
 		if credential.Name == name {

common/clicfg/credentials/credentials.go

@@ -6,10 +6,8 @@ package credentials
 import (
 	"encoding/json"
 	"path/filepath"
-	"time"
 
 	"github.com/neo4j/cli/common/clicfg/fileutils"
-	"github.com/neo4j/cli/common/clierr"
 	"github.com/spf13/afero"
 )
 
@@ -49,164 +47,46 @@ func (c *Credentials) load() {
 	}
 
 	c.Aura = credentials.Aura
+	c.Aura.refresh = c.refreshAura
+	c.Aura.persist = c.save
 
 	if !fileHasData {
-		c.writeAura(c.Aura)
+		c.save()
 	}
 }
 
-func (c *Credentials) readAuraFresh() (*AuraCredentials, error) {
+// refreshAura re-reads the credentials file from disk into the existing c.Aura, discarding
+// whatever was loaded or mutated in memory before this call. It updates the struct in place
+// rather than replacing it, so the refresh/persist closures wired up in load stay intact.
+func (c *Credentials) refreshAura() error {
 	data := fileutils.ReadFileSafe(c.fs, c.filePath)
 
 	var credFile CredentialsFile
-	if len(data) == 0 {
-		credFile = CredentialsFile{
-			Aura: &AuraCredentials{
-				Credentials: []*AuraCredential{},
-			},
-		}
-	} else {
+	if len(data) != 0 {
 		if err := json.Unmarshal(data, &credFile); err != nil {
-			return nil, err
+			return err
 		}
 	}
 
 	if credFile.Aura == nil {
-		credFile.Aura = &AuraCredentials{
-			Credentials: []*AuraCredential{},
-		}
+		c.Aura.Credentials = []*AuraCredential{}
+		c.Aura.DefaultCredential = ""
+		return nil
 	}
 
-	return credFile.Aura, nil
+	c.Aura.Credentials = credFile.Aura.Credentials
+	c.Aura.DefaultCredential = credFile.Aura.DefaultCredential
+	return nil
 }
 
-func (c *Credentials) writeAura(aura *AuraCredentials) error {
+func (c *Credentials) save() error {
 	data, err := json.Marshal(CredentialsFile{
-		Aura: aura,
+		Aura: c.Aura,
 	})
 	if err != nil {
 		panic(err)
 	}
 
 	fileutils.WriteFile(c.fs, c.filePath, data)
-	c.Aura = aura
 	return nil
 }
-
-func (c *Credentials) Add(name string, clientId string, clientSecret string) error {
-	aura, err := c.readAuraFresh()
-	if err != nil {
-		return err
-	}
-
-	for _, credential := range aura.Credentials {
-		if credential.Name == name {
-			return clierr.NewUsageError("already have credential with name %s", name)
-		}
-	}
-
-	aura.Credentials = append(aura.Credentials, &AuraCredential{Name: name, ClientId: clientId, ClientSecret: clientSecret})
-	if len(aura.Credentials) == 1 {
-		aura.DefaultCredential = name
-	}
-
-	return c.writeAura(aura)
-}
-
-func (c *Credentials) Remove(name string) error {
-	aura, err := c.readAuraFresh()
-	if err != nil {
-		return err
-	}
-
-	var indexToRemove = -1
-	for i, credential := range aura.Credentials {
-		if credential.Name == name {
-			indexToRemove = i
-			break
-		}
-	}
-
-	if indexToRemove == -1 {
-		return clierr.NewUsageError("could not find credential with name %s to remove", name)
-	}
-
-	if aura.DefaultCredential == name {
-		aura.DefaultCredential = ""
-	}
-
-	aura.Credentials = append(aura.Credentials[:indexToRemove], aura.Credentials[indexToRemove+1:]...)
-
-	return c.writeAura(aura)
-}
-
-func (c *Credentials) SetDefault(name string) error {
-	aura, err := c.readAuraFresh()
-	if err != nil {
-		return err
-	}
-
-	if !c.credentialExists(name, aura) {
-		return clierr.NewUsageError("could not find credential with name %s", name)
-	}
-
-	aura.DefaultCredential = name
-
-	return c.writeAura(aura)
-}
-
-func (c *Credentials) UpdateAccessToken(cred *AuraCredential, accessToken string, expiresInSeconds int64) *AuraCredential {
-	aura, err := c.readAuraFresh()
-	if err != nil {
-		panic(err)
-	}
-
-	credential, err := aura.Get(cred.Name)
-	if err != nil {
-		panic(err)
-	}
-
-	const expireToleranceSeconds = 60
-	now := time.Now().UnixMilli()
-
-	credential.TokenExpiry = now + (expiresInSeconds-expireToleranceSeconds)*1000
-	credential.AccessToken = accessToken
-
-	err = c.writeAura(aura)
-	if err != nil {
-		panic(err)
-	}
-
-	return credential
-}
-
-func (c *Credentials) ClearAccessToken(cred *AuraCredential) (*AuraCredential, error) {
-	aura, err := c.readAuraFresh()
-	if err != nil {
-		return nil, err
-	}
-
-	credential, err := aura.Get(cred.Name)
-	if err != nil {
-		return nil, err
-	}
-
-	credential.TokenExpiry = 0
-	credential.AccessToken = ""
-
-	err = c.writeAura(aura)
-	if err != nil {
-		return nil, err
-	}
-
-	return credential, nil
-}
-
-func (c *Credentials) credentialExists(name string, aura *AuraCredentials) bool {
-	for _, credential := range aura.Credentials {
-		if credential.Name == name {
-			return true
-		}
-	}
-	return false
-}

common/clicfg/credentials/credentials_test.go

@@ -21,12 +21,12 @@ func TestConcurrentSaveDoesNotLoseAnEarlierWrite(t *testing.T) {
 	processB := credentials.NewCredentials(fs, configPrefix)
 
 	// Process A finishes first: `credential add --name first ...`.
-	assert.NoError(t, processA.Add("first", "client-a", "secret-a"))
+	assert.NoError(t, processA.Aura.Add("first", "client-a", "secret-a"))
 
 	// Process B finishes second — e.g. a routine token refresh triggering a
 	// save as a side effect, or a second `credential add`. It never re-read
 	// the file, so its in-memory view still thinks the store is empty.
-	assert.NoError(t, processB.Add("second", "client-b", "secret-b"))
+	assert.NoError(t, processB.Aura.Add("second", "client-b", "secret-b"))
 
 	// A fresh read of the file is what the next command would see.
 	onDisk := credentials.NewCredentials(fs, configPrefix)
@@ -48,13 +48,13 @@ func TestRemoveThenReaddSameNameWithinProcessDoesNotLoseCredential(t *testing.T)
 	cli := credentials.NewCredentials(fs, configPrefix)
 
 	// 2. Adds a credential
-	assert.NoError(t, cli.Add("test", "client-id", "client-secret"))
+	assert.NoError(t, cli.Aura.Add("test", "client-id", "client-secret"))
 
 	// 3. Removes it
-	assert.NoError(t, cli.Remove("test"))
+	assert.NoError(t, cli.Aura.Remove("test"))
 
 	// 4. Re-adds it with the same name (but new client ID/secret)
-	assert.NoError(t, cli.Add("test", "new-client-id", "new-client-secret"))
+	assert.NoError(t, cli.Aura.Add("test", "new-client-id", "new-client-secret"))
 
 	// 5. A fresh read of the file should have the credential
 	onDisk := credentials.NewCredentials(fs, configPrefix)

neo4j-cli/aura/internal/api/response.go

@@ -352,7 +352,7 @@ func formatAuthorizationError(resBody []byte, statusCode int, credential *creden
 		messages = append(messages, e.Message)
 	}
 
-	_, err = cfg.Credentials.ClearAccessToken(credential)
+	_, err = cfg.Credentials.Aura.ClearAccessToken(credential)
 	if err != nil {
 		messages = append(messages, "Request failed authorization - attempted to clear the access token but encountered an error, please report an issue in https://github.com/neo4j/cli")
 	} else {

neo4j-cli/aura/internal/api/token.go

@@ -69,6 +69,6 @@ func getToken(credential *credentials.AuraCredential, cfg *clicfg.Config) (strin
 		panic(clierr.NewFatalError("can't retrieve authentication token. %w", err))
 	}
 
-	cfg.Credentials.UpdateAccessToken(credential, grant.AccessToken, grant.ExpiresIn)
+	cfg.Credentials.Aura.UpdateAccessToken(credential, grant.AccessToken, grant.ExpiresIn)
 	return grant.AccessToken, err
 }

neo4j-cli/aura/internal/subcommands/credential/add.go

@@ -25,7 +25,7 @@ func NewAddCmd(cfg *clicfg.Config) *cobra.Command {
 		Use:   "add",
 		Short: "Adds a credential",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return cfg.Credentials.Add(name, clientId, clientSecret)
+			return cfg.Credentials.Aura.Add(name, clientId, clientSecret)
 		},
 	}
 

neo4j-cli/aura/internal/subcommands/credential/remove.go

@@ -14,7 +14,7 @@ func NewRemoveCmd(cfg *clicfg.Config) *cobra.Command {
 		Short: "Removes a credential",
 		Args:  cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return cfg.Credentials.Remove(args[0])
+			return cfg.Credentials.Aura.Remove(args[0])
 		},
 	}
 }

neo4j-cli/aura/internal/subcommands/credential/use.go

@@ -14,7 +14,7 @@ func NewUseCmd(cfg *clicfg.Config) *cobra.Command {
 		Short: "Sets the default credential to be used",
 		Args:  cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return cfg.Credentials.SetDefault(args[0])
+			return cfg.Credentials.Aura.SetDefault(args[0])
 		},
 	}
 }