From 8a61bbc2d482e1bd5c0c6240dde59c9850ac417b Mon Sep 17 00:00:00 2001
From: Nikita Kotikov <25552941+onidoru@users.noreply.github.com>
Date: Fri, 16 Feb 2024 22:21:19 +0200
Subject: [PATCH] fix: Update error handling, add more tests

Signed-off-by: onidoru <25552941+onidoru@users.noreply.github.com>
Signed-off-by: Nikita Kotikov <25552941+onidoru@users.noreply.github.com>
---
 pkg/cli/server/root.go      |  5 ++-
 pkg/cli/server/root_test.go | 80 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/pkg/cli/server/root.go b/pkg/cli/server/root.go
index 022af65d..e61a8962 100644
--- a/pkg/cli/server/root.go
+++ b/pkg/cli/server/root.go
@@ -2,6 +2,7 @@ package server
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net"
 	"net/http"
@@ -856,7 +857,7 @@ func readLDAPCredentials(ldapConfigPath string) (config.LDAPCredentials, error)
 	if err := viperInstance.ReadInConfig(); err != nil {
 		log.Error().Err(err).Msg("failed to read configuration")
 
-		return config.LDAPCredentials{}, err
+		return config.LDAPCredentials{}, errors.Join(zerr.ErrBadConfig, err)
 	}
 
 	var ldapCredentials config.LDAPCredentials
@@ -865,7 +866,7 @@ func readLDAPCredentials(ldapConfigPath string) (config.LDAPCredentials, error)
 	if err := viperInstance.UnmarshalExact(&ldapCredentials, metadataConfig(metaData)); err != nil {
 		log.Error().Err(err).Msg("failed to unmarshal ldap credentials config")
 
-		return config.LDAPCredentials{}, err
+		return config.LDAPCredentials{}, errors.Join(zerr.ErrBadConfig, err)
 	}
 
 	if len(metaData.Keys) == 0 {
diff --git a/pkg/cli/server/root_test.go b/pkg/cli/server/root_test.go
index 73bab520..5375598f 100644
--- a/pkg/cli/server/root_test.go
+++ b/pkg/cli/server/root_test.go
@@ -1275,7 +1275,7 @@ storage:
 		So(err, ShouldBeNil)
 	})
 
-	Convey("Test verify bad ldap config", t, func(c C) {
+	Convey("Test verify bad ldap config: key is missing", t, func(c C) {
 		tmpFile, err := os.CreateTemp("", "zot-test*.json")
 		So(err, ShouldBeNil)
 		defer os.Remove(tmpFile.Name())
@@ -1313,6 +1313,84 @@ storage:
 		So(err, ShouldNotBeNil)
 		So(err.Error(), ShouldContainSubstring, "invalid server config")
 	})
+
+	Convey("Test verify bad ldap config: unused key", t, func(c C) {
+		tmpFile, err := os.CreateTemp("", "zot-test*.json")
+		So(err, ShouldBeNil)
+		defer os.Remove(tmpFile.Name())
+
+		tmpCredsFile, err := os.CreateTemp("", "zot-cred*.json")
+		So(err, ShouldBeNil)
+		defer os.Remove(tmpCredsFile.Name())
+
+		// `bindDN` key is missing
+		content := []byte(`{
+    		"bindDN":"cn=ldap-searcher,ou=Users,dc=example,dc=org",
+    		"bindPassword":"ldap-searcher-password",
+			"extraKey": "extraValue"
+		}`)
+
+		_, err = tmpCredsFile.Write(content)
+		So(err, ShouldBeNil)
+		err = tmpCredsFile.Close()
+		So(err, ShouldBeNil)
+
+		content = []byte(fmt.Sprintf(`{ "distSpecVersion": "1.1.0-dev", 
+			"storage": { "rootDirectory": "/tmp/zot" }, "http": { "address": "127.0.0.1", "port": "8080", 
+			"auth": { "ldap": { "credentialsFile": "%v", "address": "ldap.example.org", "port": 389, 
+			"startTLS": false, "baseDN": "ou=Users,dc=example,dc=org", 
+			"userAttribute": "uid", "userGroupAttribute": "memberOf", "skipVerify": true, "subtreeSearch": true }, 
+			"failDelay": 5 } }, "log": { "level": "debug" } }`,
+			tmpCredsFile.Name()),
+		)
+
+		_, err = tmpFile.Write(content)
+		So(err, ShouldBeNil)
+		err = tmpFile.Close()
+		So(err, ShouldBeNil)
+
+		os.Args = []string{"cli_test", "verify", tmpFile.Name()}
+		err = cli.NewServerRootCmd().Execute()
+		So(err, ShouldNotBeNil)
+		So(err.Error(), ShouldContainSubstring, "invalid server config")
+	})
+
+	Convey("Test verify bad ldap config: no keys set", t, func(c C) {
+		tmpFile, err := os.CreateTemp("", "zot-test*.json")
+		So(err, ShouldBeNil)
+		defer os.Remove(tmpFile.Name())
+
+		tmpCredsFile, err := os.CreateTemp("", "zot-cred*.json")
+		So(err, ShouldBeNil)
+		defer os.Remove(tmpCredsFile.Name())
+
+		// `bindDN` key is missing
+		content := []byte(``)
+
+		_, err = tmpCredsFile.Write(content)
+		So(err, ShouldBeNil)
+		err = tmpCredsFile.Close()
+		So(err, ShouldBeNil)
+
+		content = []byte(fmt.Sprintf(`{ "distSpecVersion": "1.1.0-dev", 
+			"storage": { "rootDirectory": "/tmp/zot" }, "http": { "address": "127.0.0.1", "port": "8080", 
+			"auth": { "ldap": { "credentialsFile": "%v", "address": "ldap.example.org", "port": 389, 
+			"startTLS": false, "baseDN": "ou=Users,dc=example,dc=org", 
+			"userAttribute": "uid", "userGroupAttribute": "memberOf", "skipVerify": true, "subtreeSearch": true }, 
+			"failDelay": 5 } }, "log": { "level": "debug" } }`,
+			tmpCredsFile.Name()),
+		)
+
+		_, err = tmpFile.Write(content)
+		So(err, ShouldBeNil)
+		err = tmpFile.Close()
+		So(err, ShouldBeNil)
+
+		os.Args = []string{"cli_test", "verify", tmpFile.Name()}
+		err = cli.NewServerRootCmd().Execute()
+		So(err, ShouldNotBeNil)
+		So(err.Error(), ShouldContainSubstring, "invalid server config")
+	})
 }
 
 func TestApiKeyConfig(t *testing.T) {