From be4267f2374c6c9142b2877ab283356a1094e70d Mon Sep 17 00:00:00 2001
From: Kian Parvin <46668016+kian99@users.noreply.github.com>
Date: Wed, 2 Aug 2023 12:55:27 +0200
Subject: [PATCH] Fix to use postgres as the controller credential store
 (#1016)

* Fix to use postgres as the controller credential store

* Slight refactor

* Added and updated tests

* Allow jimm to continue start without a credential store

Jimm could error out if no credential store is setup but this would require several tests to change and is best left for a separate PR

* Updated test
---
 internal/jujuclient/dial.go |  5 +++++
 service.go                  | 39 +++++++++++++++++++++++------------
 service_test.go             | 41 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 71 insertions(+), 14 deletions(-)

diff --git a/internal/jujuclient/dial.go b/internal/jujuclient/dial.go
index c1dfdc835..f63e3a4b6 100644
--- a/internal/jujuclient/dial.go
+++ b/internal/jujuclient/dial.go
@@ -120,6 +120,11 @@ func (d *Dialer) Dial(ctx context.Context, ctl *dbmodel.Controller, modelTag nam
 		}
 	}
 
+	if username == "" || password == "" {
+		zapctx.Error(ctx, "empty username or password")
+		return nil, errors.E(op, errors.CodeNotFound, "missing controller username or password")
+	}
+
 	args := jujuparams.LoginRequest{
 		AuthTag:       names.NewUserTag(username).String(),
 		Credentials:   password,
diff --git a/service.go b/service.go
index 0d2b0be1d..91825c807 100644
--- a/service.go
+++ b/service.go
@@ -292,23 +292,13 @@ func NewService(ctx context.Context, p Params) (*Service, error) {
 	if err != nil {
 		return nil, errors.E(op, err)
 	}
-	vs, err := newVaultStore(ctx, p)
-	if err != nil {
-		zapctx.Error(ctx, "Vault Store error", zap.Error(err))
+
+	if err := s.setupCredentialStore(ctx, p); err != nil {
 		return nil, errors.E(op, err)
 	}
-	if vs != nil {
-		s.jimm.CredentialStore = vs
-	} else {
-		// Only enable Postgres storage for secrets if explictly enabled.
-		if _, ok := os.LookupEnv("INSECURE_SECRET_STORAGE"); ok {
-			zapctx.Warn(ctx, "using plaintext postgres for secret storage")
-			s.jimm.CredentialStore = &s.jimm.Database
-		}
-	}
 
 	s.jimm.Dialer = &jujuclient.Dialer{
-		ControllerCredentialsStore: vs,
+		ControllerCredentialsStore: s.jimm.CredentialStore,
 	}
 	if !p.DisableConnectionCache {
 		s.jimm.Dialer = jimm.CacheDialer(s.jimm.Dialer)
@@ -472,6 +462,29 @@ func newAuthenticator(ctx context.Context, db *db.Database, client *ofgaClient.O
 	}, nil
 }
 
+func (s *Service) setupCredentialStore(ctx context.Context, p Params) error {
+	const op = errors.Op("newSecretStore")
+	vs, err := newVaultStore(ctx, p)
+	if err != nil {
+		zapctx.Error(ctx, "Vault Store error", zap.Error(err))
+		return errors.E(op, err)
+	}
+	if vs != nil {
+		s.jimm.CredentialStore = vs
+		return nil
+	}
+
+	// Only enable Postgres storage for secrets if explicitly enabled.
+	if _, ok := os.LookupEnv("INSECURE_SECRET_STORAGE"); ok {
+		zapctx.Warn(ctx, "using plaintext postgres for secret storage")
+		s.jimm.CredentialStore = &s.jimm.Database
+		return nil
+	}
+	// Currently jimm will start without a credential store but
+	// functionality will be limited.
+	return nil
+}
+
 func newVaultStore(ctx context.Context, p Params) (jimmcreds.CredentialStore, error) {
 	if p.VaultSecretFile == "" {
 		return nil, nil
diff --git a/service_test.go b/service_test.go
index 30692af6b..3e36adbca 100644
--- a/service_test.go
+++ b/service_test.go
@@ -43,7 +43,7 @@ func TestDefaultService(t *testing.T) {
 
 	_, ofgaClient, cfg, err := jimmtest.SetupTestOFGAClient(c.Name())
 	c.Assert(err, qt.IsNil)
-
+	os.Setenv("INSECURE_SECRET_STORAGE", "enable")
 	svc, err := jimm.NewService(context.Background(), jimm.Params{
 		OpenFGAParams: jimm.OpenFGAParams{
 			Scheme:    cfg.ApiScheme,
@@ -62,6 +62,23 @@ func TestDefaultService(t *testing.T) {
 	c.Check(resp.StatusCode, qt.Equals, http.StatusOK)
 }
 
+func TestServiceStartsWithoutSecretStore(t *testing.T) {
+	c := qt.New(t)
+
+	_, ofgaClient, cfg, err := jimmtest.SetupTestOFGAClient(c.Name())
+	c.Assert(err, qt.IsNil)
+	_, err = jimm.NewService(context.Background(), jimm.Params{
+		OpenFGAParams: jimm.OpenFGAParams{
+			Scheme:    cfg.ApiScheme,
+			Host:      cfg.ApiHost,
+			Store:     cfg.StoreId,
+			Token:     cfg.Credentials.Config.ApiToken,
+			AuthModel: ofgaClient.AuthModelId,
+		},
+	})
+	c.Assert(err, qt.IsNil)
+}
+
 func TestAuthenticator(t *testing.T) {
 	c := qt.New(t)
 
@@ -80,6 +97,7 @@ func TestAuthenticator(t *testing.T) {
 		},
 	}
 	candid := startCandid(c, &p)
+	os.Setenv("INSECURE_SECRET_STORAGE", "enable")
 	svc, err := jimm.NewService(context.Background(), p)
 	c.Assert(err, qt.IsNil)
 
@@ -187,6 +205,27 @@ func TestVault(t *testing.T) {
 	})
 }
 
+func TestPostgresSecretStore(t *testing.T) {
+	c := qt.New(t)
+
+	_, ofgaClient, cfg, err := jimmtest.SetupTestOFGAClient(c.Name())
+	c.Assert(err, qt.IsNil)
+
+	p := jimm.Params{
+		ControllerUUID: "6acf4fd8-32d6-49ea-b4eb-dcb9d1590c11",
+		OpenFGAParams: jimm.OpenFGAParams{
+			Scheme:    cfg.ApiScheme,
+			Host:      cfg.ApiHost,
+			Store:     cfg.StoreId,
+			Token:     cfg.Credentials.Config.ApiToken,
+			AuthModel: ofgaClient.AuthModelId,
+		},
+	}
+	os.Setenv("INSECURE_SECRET_STORAGE", "enable")
+	_, err = jimm.NewService(context.Background(), p)
+	c.Assert(err, qt.IsNil)
+}
+
 func TestOpenFGA(t *testing.T) {
 	c := qt.New(t)