fix: resolve default/profile client credential conflict from env vars

internal/cac/client/config.go

@@ -1,6 +1,10 @@
 package client
 
-import acpclient "github.com/cloudentity/acp-client-go"
+import (
+	"net/url"
+
+	acpclient "github.com/cloudentity/acp-client-go"
+)
 
 type Configuration struct {
 	// nolint
@@ -9,6 +13,41 @@ type Configuration struct {
 	Insecure bool `json:"insecure"`
 }
 
+// Clone returns a deep copy of the configuration. The embedded acpclient.Config
+// holds reference fields (the *url.URL endpoints and the Scopes slice), so a
+// plain struct copy would still alias them with the original. Callers rely on
+// Clone to obtain a fully independent value that can be mutated in isolation.
+func (c *Configuration) Clone() *Configuration {
+	if c == nil {
+		return nil
+	}
+
+	clone := *c
+
+	if c.Scopes != nil {
+		clone.Scopes = append([]string(nil), c.Scopes...)
+	}
+
+	clone.RedirectURL = cloneURL(c.RedirectURL)
+	clone.IssuerURL = cloneURL(c.IssuerURL)
+	clone.TokenURL = cloneURL(c.TokenURL)
+	clone.AuthorizeURL = cloneURL(c.AuthorizeURL)
+	clone.PushedAuthorizationRequestEndpoint = cloneURL(c.PushedAuthorizationRequestEndpoint)
+	clone.UserinfoURL = cloneURL(c.UserinfoURL)
+
+	return &clone
+}
+
+func cloneURL(u *url.URL) *url.URL {
+	if u == nil {
+		return nil
+	}
+
+	clone := *u
+
+	return &clone
+}
+
 var DefaultConfig = func() *Configuration {
 	return &Configuration{
 		Insecure: false,

internal/cac/config/config.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"os"
 	"strings"
 
 	"github.com/cloudentity/cac/internal/cac/client"
@@ -56,16 +57,22 @@ func (c *Configuration) SetImplicitValues(name string, defaultConfig Configurati
 		c.Name = name
 	}
 
-	if c.Logging == nil {
-		c.Logging = defaultConfig.Logging
+	// Fall back to the default config, but as an independent copy so that
+	// profiles never share (and accidentally mutate) the default's objects.
+	// Logging only holds value fields, so a plain struct copy is enough; the
+	// client and storage configs carry reference fields and are deep-copied
+	// via their Clone methods.
+	if c.Logging == nil && defaultConfig.Logging != nil {
+		clone := *defaultConfig.Logging
+		c.Logging = &clone
 	}
 
-	if c.Client == nil {
-		c.Client = defaultConfig.Client
+	if c.Client == nil && defaultConfig.Client != nil {
+		c.Client = defaultConfig.Client.Clone()
 	}
 
-	if c.Storage == nil {
-		c.Storage = defaultConfig.Storage
+	if c.Storage == nil && defaultConfig.Storage != nil {
+		c.Storage = defaultConfig.Storage.Clone()
 	}
 }
 
@@ -82,9 +89,10 @@ func InitConfig(path string) (_ *RootConfiguration, err error) {
 	config.Default.SetImplicitValues("default", DefaultConfig())
 
 	utils.ConfigureDecoder(&dconf)
-	v := viper.GetViper()
+	v := viper.New()
+	envKeyReplacer := strings.NewReplacer(".", "_")
 	v.AutomaticEnv()
-	v.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
+	v.SetEnvKeyReplacer(envKeyReplacer)
 
 	if decoder, err = mapstructure.NewDecoder(&dconf); err != nil {
 		return nil, err
@@ -98,6 +106,10 @@ func InitConfig(path string) (_ *RootConfiguration, err error) {
 		v.SetDefault(k, val)
 	}
 
+	// Leaf keys of the (squashed) default config, e.g. "client.client_id".
+	// These are the keys that can also be set per profile via env variables.
+	defaultKeys := v.AllKeys()
+
 	if path != "" {
 		v.SetConfigFile(path)
 
@@ -106,6 +118,32 @@ func InitConfig(path string) (_ *RootConfiguration, err error) {
 		}
 	}
 
+	// viper's AutomaticEnv only resolves keys it already knows about, and env
+	// bindings do not merge into nested maps that originate from a config file
+	// during Unmarshal. So for every known profile, bind each default config key
+	// to its env variable (e.g. PROFILES_STAGE_CLIENT_CLIENT_ID) and, only when
+	// that env variable is actually set, promote its value into viper's explicit
+	// override layer which is deep-merged on Unmarshal. We gate on os.LookupEnv
+	// (not v.IsSet, which is also true for config-file values) so file-provided
+	// values stay in their native layer and profiles without env overrides are
+	// left untouched, still falling back to the default config in
+	// SetImplicitValues.
+	for name := range v.GetStringMap("profiles") {
+		for _, key := range defaultKeys {
+			profileKey := "profiles." + name + "." + key
+
+			if err := v.BindEnv(profileKey); err != nil {
+				return nil, err
+			}
+
+			envKey := envKeyReplacer.Replace(strings.ToUpper(profileKey))
+
+			if _, ok := os.LookupEnv(envKey); ok {
+				v.Set(profileKey, v.Get(profileKey))
+			}
+		}
+	}
+
 	if err := v.Unmarshal(&config, utils.ConfigureDecoder); err != nil {
 		return nil, err
 	}

internal/cac/config/config_test.go

@@ -34,19 +34,133 @@ func TestReadingConfiguration(t *testing.T) {
 		require.NotEmpty(t, "aaaaaaaaaaaaa", profile.Client.ClientID)
 	})
 
+	t.Run("prefers profile config over default config", func(t *testing.T) {
+		rootConf, err := config.InitConfig("./../../../examples/e2e/config.yaml")
+		require.NoError(t, err)
+
+		defaultConf, err := rootConf.ForProfile("")
+		require.NoError(t, err)
+
+		profile, err := rootConf.ForProfile("stage")
+		require.NoError(t, err)
+
+		// values defined in the profile take precedence over the default config
+		require.NotNil(t, profile.Client)
+		require.Equal(t, "https://janus.eu.authz.cloudentity.io/janus/system", profile.Client.IssuerURL.String())
+		require.NotEqual(t, defaultConf.Client.IssuerURL.String(), profile.Client.IssuerURL.String())
+
+		require.Equal(t, "fb346c287c4d4e378cbae39aa0cxxxxx", profile.Client.ClientID)
+		require.NotEqual(t, defaultConf.Client.ClientID, profile.Client.ClientID)
+
+		require.NotNil(t, profile.Storage)
+		require.Equal(t, []string{"/tmp/other"}, profile.Storage.DirPath)
+		require.NotEqual(t, defaultConf.Storage.DirPath, profile.Storage.DirPath)
+
+		// values not overridden in the profile fall back to the default config
+		require.NotNil(t, profile.Logging)
+		require.Equal(t, defaultConf.Logging.Level, profile.Logging.Level)
+		require.Equal(t, defaultConf.Logging.Format, profile.Logging.Format)
+	})
+
 	t.Run("fail on invalid path", func(t *testing.T) {
 		_, err := config.InitConfig("./invalid.json")
 		require.Error(t, err)
 		require.Equal(t, "open ./invalid.json: no such file or directory", err.Error())
 	})
 
+	// Reproduces a bug: when both the default client and a profile client are
+	// configured through environment variables, the profile silently ends up
+	// using the default client's credentials instead of its own.
+	t.Run("profile client from env does not collide with default client from env", func(t *testing.T) {
+		// default client credentials via env
+		t.Setenv("CLIENT_ISSUER_URL", "https://default.example.com/default/system")
+		t.Setenv("CLIENT_CLIENT_ID", "env-default-id")
+		t.Setenv("CLIENT_CLIENT_SECRET", "env-default-secret")
+
+		// stage profile client credentials via env
+		t.Setenv("PROFILES_STAGE_CLIENT_ISSUER_URL", "https://stage.example.com/stage/system")
+		t.Setenv("PROFILES_STAGE_CLIENT_CLIENT_ID", "env-stage-id")
+		t.Setenv("PROFILES_STAGE_CLIENT_CLIENT_SECRET", "env-stage-secret")
+
+		rootConf, err := config.InitConfig("./testdata/profile_env_conflict.yaml")
+		require.NoError(t, err)
+
+		defaultConf, err := rootConf.ForProfile("")
+		require.NoError(t, err)
+
+		stage, err := rootConf.ForProfile("stage")
+		require.NoError(t, err)
+
+		// sanity: default picks up its own env credentials
+		require.Equal(t, "env-default-id", defaultConf.Client.ClientID)
+		require.Equal(t, "env-default-secret", defaultConf.Client.ClientSecret)
+
+		// the profile must use ITS OWN env credentials, not the default's
+		require.Equal(t, "env-stage-id", stage.Client.ClientID)
+		require.Equal(t, "env-stage-secret", stage.Client.ClientSecret)
+		require.Equal(t, "https://stage.example.com/stage/system", stage.Client.IssuerURL.String())
+
+		// the two clients must not be the same backing object; assert pointer
+		// inequality directly so a regression is caught even if the IDs match
+		require.NotSame(t, defaultConf.Client, stage.Client)
+		require.NotEqual(t, defaultConf.Client.ClientID, stage.Client.ClientID)
+	})
+
+	// Reproduces the mechanism behind the conflict above: a profile that does
+	// not define its own client falls back to the default client by sharing the
+	// SAME pointer, so mutating one mutates the other.
+	t.Run("profile without client is not aliased to the default client", func(t *testing.T) {
+		t.Setenv("CLIENT_ISSUER_URL", "https://default.example.com/default/system")
+		t.Setenv("CLIENT_CLIENT_ID", "env-default-id")
+		t.Setenv("CLIENT_CLIENT_SECRET", "env-default-secret")
+
+		rootConf, err := config.InitConfig("./testdata/profile_env_conflict.yaml")
+		require.NoError(t, err)
+
+		defaultConf, err := rootConf.ForProfile("")
+		require.NoError(t, err)
+
+		stage, err := rootConf.ForProfile("stage")
+		require.NoError(t, err)
+
+		// the profile should fall back to the default values, but as an
+		// independent copy, not a shared pointer
+		require.NotSame(t, defaultConf.Client, stage.Client)
+
+		stage.Client.ClientID = "mutated-via-stage"
+		require.Equal(t, "env-default-id", defaultConf.Client.ClientID,
+			"mutating the profile client must not affect the default client")
+
+		// reference fields inside the embedded acpclient.Config must be
+		// deep-copied too, not aliased between the profile and the default
+		require.NotNil(t, defaultConf.Client.IssuerURL)
+		require.NotSame(t, defaultConf.Client.IssuerURL, stage.Client.IssuerURL,
+			"the profile client must not share the default's IssuerURL pointer")
+
+		require.NotEmpty(t, defaultConf.Client.Scopes)
+		stage.Client.Scopes[0] = "mutated-scope"
+		require.NotEqual(t, "mutated-scope", defaultConf.Client.Scopes[0],
+			"mutating the profile client scopes must not affect the default client")
+
+		// a profile without its own storage falls back to the default storage,
+		// which must also be an independent copy (its DirPath slice is not shared)
+		bare, err := rootConf.ForProfile("bare")
+		require.NoError(t, err)
+
+		require.NotSame(t, defaultConf.Storage, bare.Storage)
+		require.NotEmpty(t, defaultConf.Storage.DirPath)
+		bare.Storage.DirPath[0] = "/tmp/mutated"
+		require.NotEqual(t, "/tmp/mutated", defaultConf.Storage.DirPath[0],
+			"mutating the profile storage dir path must not affect the default storage")
+	})
+
 	t.Run("read config from env", func(t *testing.T) {
 		t.Setenv("CLIENT_ISSUER_URL", "https://postmance.eu.authz.cloudentity.io/postmance/system")
 		t.Setenv("CLIENT_CLIENT_ID", "test-cid1")
 		t.Setenv("CLIENT_CLIENT_SECRET", "test-secret")
 
-		// FIXME reading profiles from env variables is not yet supported
-		// t.Setenv("PROFILES_STAGE_CLIENT_CLIENT_SECRET", "test-secret")
+		// Reading profile values from env variables is covered by
+		// "profile client from env does not collide with default client from env".
 
 		rootConf, err := config.InitConfig("")
 		require.NoError(t, err)

internal/cac/config/testdata/profile_env_conflict.yaml

@@ -0,0 +1,16 @@
+# Default client and profile client credentials are intentionally NOT set here.
+# They are meant to be provided via environment variables (secrets should not
+# live in committed config). The profile only declares non-secret settings.
+logging:
+  level: info
+  format: text
+storage:
+  dir_path: "/tmp/default-data"
+
+profiles:
+  stage:
+    storage:
+      dir_path: "/tmp/stage-data"
+  # a profile that declares nothing of its own, so client and storage both fall
+  # back to (independent copies of) the default config
+  bare: {}

internal/cac/storage/multi.go

@@ -14,6 +14,22 @@ type MultiStorageConfiguration struct {
 	DirPath []string `json:"dir_path"`
 }
 
+// Clone returns a copy with its own backing DirPath slice so callers never
+// share (and accidentally mutate) the original's slice.
+func (c *MultiStorageConfiguration) Clone() *MultiStorageConfiguration {
+	if c == nil {
+		return nil
+	}
+
+	clone := *c
+
+	if c.DirPath != nil {
+		clone.DirPath = append([]string(nil), c.DirPath...)
+	}
+
+	return &clone
+}
+
 var DefaultMultiStorageConfig = func() *MultiStorageConfiguration {
 	return &MultiStorageConfiguration{
 		DirPath: []string{"data"},