Repository: nats-io/nats-server
Pull request #5731 (2024-08-01 03:39:23)

Improve config parsing API: file vs. string
The publicly exposed API for parsing a string containing configuration and a string denoting a config file were unequal.

When parsing a config file, additional checks are automatically done in the (*Options).ProcessConfigFile method, however if parsing a string containing config then those additional checks have to be recreated in consumers of the public API.

This adds:

- conf.ParseWithChecks (pedantic mode equivalent to conf.Parse)
- (*Options).ProcessConfigString, which shares the same mechanism as (*Options).ProcesConfigFile.

<!-- Please make sure to read CONTRIBUTING.md, then delete this notice and replace it with your PR description. The below sign-off certifies that the contribution is your original work and that you license the work to the project under the Apache-2.0 license. We cannot accept contributions without it. -->

Signed-off-by: Your Name <your.email@example.com>



Patch:
diff --git a/conf/parse.go b/conf/parse.go
index bf5953c1..3e4d7a33 100644
--- a/conf/parse.go
+++ b/conf/parse.go
@@ -26,6 +26,7 @@ package conf
 // see parse_test.go for more examples.
 
 import (
+	"encoding/json"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -69,6 +70,15 @@ func Parse(data string) (map[string]any, error) {
 	return p.mapping, nil
 }
 
+// ParseWithChecks is equivalent to Parse but runs in pedantic mode.
+func ParseWithChecks(data string) (map[string]any, error) {
+	p, err := parse(data, "", true)
+	if err != nil {
+		return nil, err
+	}
+	return p.mapping, nil
+}
+
 // ParseFile is a helper to open file, etc. and parse the contents.
 func ParseFile(fp string) (map[string]any, error) {
 	data, err := os.ReadFile(fp)
@@ -105,6 +115,10 @@ type token struct {
 	sourceFile   string
 }
 
+func (t *token) MarshalJSON() ([]byte, error) {
+	return json.Marshal(t.value)
+}
+
 func (t *token) Value() any {
 	return t.value
 }
diff --git a/server/opts.go b/server/opts.go
index ab5f6c60..677686b6 100644
--- a/server/opts.go
+++ b/server/opts.go
@@ -874,6 +874,22 @@ func (o *Options) ProcessConfigFile(configFile string) error {
 	if err != nil {
 		return err
 	}
+
+	return o.processConfigFile(configFile, m)
+}
+
+// ProcessConfigString is the same as ProcessConfigFile, but expects the
+// contents of the config file to be passed in rather than the file name.
+func (o *Options) ProcessConfigString(data string) error {
+	m, err := conf.ParseWithChecks(data)
+	if err != nil {
+		return err
+	}
+
+	return o.processConfigFile(_EMPTY_, m)
+}
+
+func (o *Options) processConfigFile(configFile string, m map[string]any) error {
 	// Collect all errors and warnings and report them all together.
 	errors := make([]error, 0)
 	warnings := make([]error, 0)


Test patch:
diff --git a/conf/parse_test.go b/conf/parse_test.go
index febb6082..a434a618 100644
--- a/conf/parse_test.go
+++ b/conf/parse_test.go
@@ -1,6 +1,8 @@
 package conf
 
 import (
+	"bytes"
+	"encoding/json"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -25,6 +27,27 @@ func test(t *testing.T, data string, ex map[string]any) {
 	if !reflect.DeepEqual(m, ex) {
 		t.Fatalf("Not Equal:\nReceived: '%+v'\nExpected: '%+v'\n", m, ex)
 	}
+
+	// Also test ParseWithChecks
+	n, err := ParseWithChecks(data)
+	if err != nil {
+		t.Fatalf("Received err: %v\n", err)
+	}
+	if n == nil {
+		t.Fatal("Received nil map")
+	}
+	// Compare with the original results.
+	a, err := json.Marshal(m)
+	if err != nil {
+		t.Fatal(err)
+	}
+	b, err := json.Marshal(n)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !bytes.Equal(b, a) {
+		t.Fatalf("Not Equal:\nReceived: '%+v'\nExpected: '%+v'\n", string(a), string(b))
+	}
 }
 
 func TestSimpleTopLevel(t *testing.T) {
diff --git a/server/opts_test.go b/server/opts_test.go
index 41a36ee9..7ea6bf49 100644
--- a/server/opts_test.go
+++ b/server/opts_test.go
@@ -3319,3 +3319,87 @@ func TestAuthorizationAndAccountsMisconfigurations(t *testing.T) {
 		})
 	}
 }
+
+// TestProcessConfigString duplicates the previous test, but uses the (*Options).ProcessConfigString
+// instead of the ProcessConfigFile function.
+func TestProcessConfigString(t *testing.T) {
+	for _, test := range []struct {
+		name   string
+		config string
+		err    string
+	}{
+		{
+			"duplicate users",
+			`
+			authorization = {users = [ {user: "user1", pass: "pwd"} ] }
+			accounts { ACC { users = [ {user: "user1"} ] } }
+			`,
+			fmt.Sprintf("Duplicate user %q detected", "user1"),
+		},
+		{
+			"duplicate nkey",
+			`
+			authorization = {users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] }
+			accounts { ACC { users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] } }
+			`,
+			fmt.Sprintf("Duplicate nkey %q detected", "UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX"),
+		},
+		{
+			"auth single user and password and accounts users",
+			`
+			authorization = {user: "user1", password: "pwd"}
+			accounts = { ACC { users = [ {user: "user2", pass: "pwd"} ] } }
+			`,
+			"Can not have a single user/pass",
+		},
+		{
+			"auth single user and password and accounts nkeys",
+			`
+			authorization = {user: "user1", password: "pwd"}
+			accounts = { ACC { users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] } }
+			`,
+			"Can not have a single user/pass",
+		},
+		{
+			"auth token and accounts users",
+			`
+			authorization = {token: "my_token"}
+			accounts = { ACC { users = [ {user: "user2", pass: "pwd"} ] } }
+			`,
+			"Can not have a token",
+		},
+		{
+			"auth token and accounts nkeys",
+			`
+			authorization = {token: "my_token"}
+			accounts = { ACC { users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] } }
+			`,
+			"Can not have a token",
+		},
+		{
+			"auth callout allowed accounts",
+			`
+			accounts {
+				AUTH { users = [ {user: "auth", password: "auth"} ] }
+				FOO {}
+			}
+			authorization {
+				auth_callout {
+					issuer: "ABJHLOVMPA4CI6R5KLNGOB4GSLNIY7IOUPAJC4YFNDLQVIOBYQGUWVLA"
+					account: AUTH
+					auth_users: [ auth ]
+					allowed_accounts: [ BAR ]
+				}
+			}
+			`,
+			"auth_callout allowed account \"BAR\" not found in configured accounts",
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			opts := &Options{}
+			if err := opts.ProcessConfigString(test.config); err == nil || !strings.Contains(err.Error(), test.err) {
+				t.Fatalf("Expected error %q, got %q", test.err, err.Error())
+			}
+		})
+	}
+}