[CHANGED] Duplicates in authorization{} and accounts{} now detected

If accounts{} block is specified, authorization{} should not have
any user/password/token or users array defined.

The reason is that users parsed in accounts{} are associated with
their respective account but users parsed in authorization{} are
associated with the global account. If the same user name is
in both, and since internally the parsing of those 2 blocks is
completely random (even if layed out in the config in a specific
order), the outcome may be that a user is either associated with
an account or the default global account.

To minimize breaking changes, but still avoid this unexpected
outcome, the server will now detect if there are duplicate users
(or nkeys) inside authorization{} block itself, but also between
this block and accounts{}.
The check will also detect if accounts{} has any user/nkey, then
the authorization{} block should not have any user/password/token,
making this test similar to the check we had in authorization{}
block itself.

Resolves #2926

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

server/config_check_test.go

@@ -917,13 +917,13 @@ func TestConfigCheck(t *testing.T) {
 			name: "when user authorization config has both token and users",
 			config: `
 		authorization = {
-                 token = "s3cr3t"
-		  users = [
-		    {
-		      user = "foo"
-		      pass = "bar"
-		    }
-		  ]
+			token = "s3cr3t"
+			users = [
+				{
+					user = "foo"
+					pass = "bar"
+				}
+			]
 		}
 		`,
 			err:       errors.New(`Can not have a token and a users array`),
@@ -934,20 +934,63 @@ func TestConfigCheck(t *testing.T) {
 			name: "when user authorization config has both token and user",
 			config: `
 		authorization = {
-  	          user = "foo"
-		  pass = "bar"
-		  users = [
-		    {
-		      user = "foo"
-		      pass = "bar"
-		    }
-		  ]
+			user = "foo"
+			pass = "bar"
+			token = "baz"
+		}
+		`,
+			err:       errors.New(`Cannot have a user/pass and token`),
+			errorLine: 2,
+			errorPos:  3,
+		},
+		{
+			name: "when user authorization config has both user and users array",
+			config: `
+		authorization = {
+			user = "user1"
+			pass = "pwd1"
+			users = [
+				{
+					user = "user2"
+					pass = "pwd2"
+				}
+			]
 		}
 		`,
 			err:       errors.New(`Can not have a single user/pass and a users array`),
 			errorLine: 2,
 			errorPos:  3,
 		},
+		{
+			name: "when user authorization has duplicate users",
+			config: `
+		authorization = {
+			users = [
+				{user: "user1", pass: "pwd"}
+				{user: "user2", pass: "pwd"}
+				{user: "user1", pass: "pwd"}
+			]
+		}
+		`,
+			err:       fmt.Errorf(`Duplicate user %q detected`, "user1"),
+			errorLine: 2,
+			errorPos:  3,
+		},
+		{
+			name: "when user authorization has duplicate nkeys",
+			config: `
+		authorization = {
+			users = [
+				{nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX }
+				{nkey: UBAAQWTW6CG2G6ANGNKB5U2B7HRWHSGMZEZX3AQSAJOQDAUGJD46LD2E }
+				{nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX }
+			]
+		}
+		`,
+			err:       fmt.Errorf(`Duplicate nkey %q detected`, "UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX"),
+			errorLine: 2,
+			errorPos:  3,
+		},
 		{
 			name: "when user authorization config has users not as a list",
 			config: `
@@ -1600,9 +1643,9 @@ func TestConfigCheckMultipleErrors(t *testing.T) {
 		t.Errorf("Expected a %d warning, got: %d", expected, got)
 	}
 	got = len(cerr.Errors())
-	expected = 7
-	if got != 7 {
-		t.Errorf("Expected a %d errors, got: %d", expected, got)
+	// Could be 7 or 8 errors depending on internal ordering of the parsing.
+	if got != 7 && got != 8 {
+		t.Errorf("Expected 7 or 8 errors, got: %d", got)
 	}
 
 	errMsg := err.Error()
@@ -1620,7 +1663,13 @@ func TestConfigCheckMultipleErrors(t *testing.T) {
 	for _, msg := range errs {
 		found := strings.Contains(errMsg, msg)
 		if !found {
-			t.Errorf("Expected to find error %q", msg)
+			t.Fatalf("Expected to find error %q", msg)
+		}
+	}
+	if got == 8 {
+		extra := "./configs/multiple_errors.conf:54:5: Can not have a single user/pass and accounts"
+		if !strings.Contains(errMsg, extra) {
+			t.Fatalf("Expected to find error %q (%s)", extra, errMsg)
 		}
 	}
 }

server/opts.go

@@ -755,30 +755,55 @@ func (o *Options) processConfigFileLine(k string, v interface{}, errors *[]error
 		o.Username = auth.user
 		o.Password = auth.pass
 		o.Authorization = auth.token
-		if (auth.user != "" || auth.pass != "") && auth.token != "" {
+		o.AuthTimeout = auth.timeout
+		if (auth.user != _EMPTY_ || auth.pass != _EMPTY_) && auth.token != _EMPTY_ {
 			err := &configErr{tk, "Cannot have a user/pass and token"}
 			*errors = append(*errors, err)
 			return
 		}
-		o.AuthTimeout = auth.timeout
-		// Check for multiple users defined
-		if auth.users != nil {
-			if auth.user != "" {
+		// In case parseAccounts() was done first, we need to check for duplicates.
+		unames := setupUsersAndNKeysDuplicateCheckMap(o)
+		// Check for multiple users defined.
+		// Note: auth.users will be != nil as long as `users: []` is present
+		// in the authorization block, even if empty, and will also account for
+		// nkey users. We also check for users/nkeys that may have been already
+		// added in parseAccounts() (which means they will be in unames)
+		if auth.users != nil || len(unames) > 0 {
+			if auth.user != _EMPTY_ {
 				err := &configErr{tk, "Can not have a single user/pass and a users array"}
 				*errors = append(*errors, err)
 				return
 			}
-			if auth.token != "" {
+			if auth.token != _EMPTY_ {
 				err := &configErr{tk, "Can not have a token and a users array"}
 				*errors = append(*errors, err)
 				return
 			}
-			// Users may have been added from Accounts parsing, so do an append here
-			o.Users = append(o.Users, auth.users...)
+			// Now check that if we have users, there is no duplicate, including
+			// users that may have been configured in parseAccounts().
+			if len(auth.users) > 0 {
+				for _, u := range auth.users {
+					if _, ok := unames[u.Username]; ok {
+						err := &configErr{tk, fmt.Sprintf("Duplicate user %q detected", u.Username)}
+						*errors = append(*errors, err)
+						return
+					}
+					unames[u.Username] = struct{}{}
+				}
+				// Users may have been added from Accounts parsing, so do an append here
+				o.Users = append(o.Users, auth.users...)
+			}
 		}
-
 		// Check for nkeys
-		if auth.nkeys != nil {
+		if len(auth.nkeys) > 0 {
+			for _, u := range auth.nkeys {
+				if _, ok := unames[u.Nkey]; ok {
+					err := &configErr{tk, fmt.Sprintf("Duplicate nkey %q detected", u.Nkey)}
+					*errors = append(*errors, err)
+					return
+				}
+				unames[u.Nkey] = struct{}{}
+			}
 			// NKeys may have been added from Accounts parsing, so do an append here
 			o.Nkeys = append(o.Nkeys, auth.nkeys...)
 		}
@@ -1303,6 +1328,17 @@ func (o *Options) processConfigFileLine(k string, v interface{}, errors *[]error
 	}
 }
 
+func setupUsersAndNKeysDuplicateCheckMap(o *Options) map[string]struct{} {
+	unames := make(map[string]struct{}, len(o.Users)+len(o.Nkeys))
+	for _, u := range o.Users {
+		unames[u.Username] = struct{}{}
+	}
+	for _, u := range o.Nkeys {
+		unames[u.Nkey] = struct{}{}
+	}
+	return unames
+}
+
 func parseDuration(field string, tk token, v interface{}, errors *[]error, warnings *[]error) time.Duration {
 	if wd, ok := v.(string); ok {
 		if dur, err := time.ParseDuration(wd); err != nil {
@@ -2449,7 +2485,10 @@ func parseAccounts(v interface{}, opts *Options, errors *[]error, warnings *[]er
 	case map[string]interface{}:
 		// Track users across accounts, must be unique across
 		// accounts and nkeys vs users.
-		uorn := make(map[string]struct{})
+		// We also want to check for users that may have been added in
+		// parseAuthorization{} if that happened first.
+		uorn := setupUsersAndNKeysDuplicateCheckMap(opts)
+
 		for aname, mv := range vv {
 			tk, amv := unwrapValue(mv, &lt)
 
@@ -2550,6 +2589,20 @@ func parseAccounts(v interface{}, opts *Options, errors *[]error, warnings *[]er
 					}
 				}
 			}
+			// Report error if there is an authorization{} block
+			// with u/p or token and any user defined in accounts{}
+			if len(nkeyUsr) > 0 || len(users) > 0 {
+				if opts.Username != _EMPTY_ {
+					err := &configErr{usersTk, "Can not have a single user/pass and accounts"}
+					*errors = append(*errors, err)
+					continue
+				}
+				if opts.Authorization != _EMPTY_ {
+					err := &configErr{usersTk, "Can not have a token and accounts"}
+					*errors = append(*errors, err)
+					continue
+				}
+			}
 			applyDefaultPermissions(users, nkeyUsr, acc.defaultPerms)
 			for _, u := range nkeyUsr {
 				if _, ok := uorn[u.Nkey]; ok {

server/opts_test.go

@@ -3286,3 +3286,36 @@ func TestGetStorageSize(t *testing.T) {
 	}
 
 }
+
+func TestAuthorizationAndAccountsDuplicateUsers(t *testing.T) {
+	// There is a test called TestConfigCheck but we can't use it
+	// because the place where the error will be reported will depend
+	// if the duplicate is found while parsing authorization{} or
+	// accounts{}, which order is random.
+	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")},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			conf := createConfFile(t, []byte(test.config))
+			defer removeFile(t, conf)
+			if _, err := ProcessConfigFile(conf); err == nil || !strings.Contains(err.Error(), test.err) {
+				t.Fatalf("Expected error %q, got %q", test.err, err.Error())
+			}
+		})
+	}
+}