Merge pull request #2943 from nats-io/fix_2926

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

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,75 @@ func TestGetStorageSize(t *testing.T) {
 	}
 
 }
+
+func TestAuthorizationAndAccountsMisconfigurations(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 error is found while parsing authorization{} or
+	// accounts{}, but we can't control the internal parsing of those
+	// (due to lexer giving back a map and iteration over map is random)
+	// regardless of the ordering in the configuration file.
+	// The test is also repeated
+	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",
+		},
+	} {
+		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())
+			}
+		})
+	}
+}