Merge pull request #2840 from nats-io/dyn_acc

Remove dynamic account behaviors.

server/accounts_test.go

@@ -1,4 +1,4 @@
-// Copyright 2018-2020 The NATS Authors
+// Copyright 2018-2022 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -322,156 +322,69 @@ func TestAccountFromOptions(t *testing.T) {
 	}
 }
 
-func TestNewAccountsFromClients(t *testing.T) {
-	opts := defaultServerOptions
-	s := New(&opts)
-	defer s.Shutdown()
-
-	c, cr, _ := newClientForServer(s)
-	defer c.close()
-	connectOp := "CONNECT {\"account\":\"foo\"}\r\n"
-	c.parseAsync(connectOp)
-	l, _ := cr.ReadString('\n')
-	if !strings.HasPrefix(l, "-ERR ") {
-		t.Fatalf("Expected an error")
-	}
+// Clients used to be able to ask that the account be forced to be new.
+// This was for dynamic sandboxes for demo environments but was never really used.
+// Make sure it always errors if set.
+func TestNewAccountAndRequireNewAlwaysError(t *testing.T) {
+	conf := createConfFile(t, []byte(`
+		listen: 127.0.0.1:-1
+		accounts: {
+			A: { users: [ {user: ua, password: pa} ] },
+			B: { users: [ {user: ub, password: pb} ] },
+		}
+	`))
+	defer removeFile(t, conf)
 
-	opts.AllowNewAccounts = true
-	s = New(&opts)
+	s, _ := RunServerWithConfig(conf)
 	defer s.Shutdown()
 
-	c, cr, _ = newClientForServer(s)
-	defer c.close()
+	// Success case
+	c, _, _ := newClientForServer(s)
+	connectOp := "CONNECT {\"user\":\"ua\", \"pass\":\"pa\"}\r\n"
 	err := c.parse([]byte(connectOp))
-	if err != nil {
-		t.Fatalf("Received an error trying to connect: %v", err)
-	}
-	c.parseAsync("PING\r\n")
-	l, err = cr.ReadString('\n')
-	if err != nil {
-		t.Fatalf("Error reading response for client from server: %v", err)
-	}
-	if !strings.HasPrefix(l, "PONG\r\n") {
-		t.Fatalf("PONG response incorrect: %q", l)
-	}
-}
-
-func TestActiveAccounts(t *testing.T) {
-	opts := defaultServerOptions
-	opts.AllowNewAccounts = true
-	opts.Cluster.Port = 22
-
-	s := New(&opts)
-	defer s.Shutdown()
-
-	if s.NumActiveAccounts() != 0 {
-		t.Fatalf("Expected no active account, got %d", s.NumActiveAccounts())
-	}
-
-	addClientWithAccount := func(accName string) *testAsyncClient {
-		t.Helper()
-		c, _, _ := newClientForServer(s)
-		connectOp := fmt.Sprintf("CONNECT {\"account\":\"%s\"}\r\n", accName)
-		err := c.parse([]byte(connectOp))
-		if err != nil {
-			t.Fatalf("Received an error trying to connect: %v", err)
-		}
-		return c
-	}
-
-	// Now add some clients.
-	cf1 := addClientWithAccount("foo")
-	defer cf1.close()
-	if s.activeAccounts != 1 {
-		t.Fatalf("Expected active accounts to be 1, got %d", s.activeAccounts)
-	}
-	// Adding in same one should not change total.
-	cf2 := addClientWithAccount("foo")
-	defer cf2.close()
-	if s.activeAccounts != 1 {
-		t.Fatalf("Expected active accounts to be 1, got %d", s.activeAccounts)
-	}
-	// Add in new one.
-	cb1 := addClientWithAccount("bar")
-	defer cb1.close()
-	if s.activeAccounts != 2 {
-		t.Fatalf("Expected active accounts to be 2, got %d", s.activeAccounts)
-	}
-
-	// Make sure the Accounts track clients.
-	foo, _ := s.LookupAccount("foo")
-	bar, _ := s.LookupAccount("bar")
-	if foo == nil || bar == nil {
-		t.Fatalf("Error looking up accounts")
-	}
-	if nc := foo.NumConnections(); nc != 2 {
-		t.Fatalf("Expected account foo to have 2 clients, got %d", nc)
-	}
-	if nc := bar.NumConnections(); nc != 1 {
-		t.Fatalf("Expected account bar to have 1 client, got %d", nc)
-	}
-
-	waitTilActiveCount := func(n int32) {
-		t.Helper()
-		checkFor(t, time.Second, 10*time.Millisecond, func() error {
-			if active := s.NumActiveAccounts(); active != n {
-				return fmt.Errorf("Number of active accounts is %d", active)
-			}
-			return nil
-		})
-	}
-
-	// Test Removal
-	cb1.closeConnection(ClientClosed)
-	waitTilActiveCount(1)
-
-	checkAccClientsCount(t, bar, 0)
-
-	// This should not change the count.
-	cf1.closeConnection(ClientClosed)
-	waitTilActiveCount(1)
-
-	checkAccClientsCount(t, foo, 1)
-
-	cf2.closeConnection(ClientClosed)
-	waitTilActiveCount(0)
-
-	checkAccClientsCount(t, foo, 0)
-}
-
-// Clients can ask that the account be forced to be new. If it exists this is an error.
-func TestNewAccountRequireNew(t *testing.T) {
-	// This has foo and bar accounts already.
-	s, _, _ := simpleAccountServer(t)
+	require_NoError(t, err)
+	c.close()
 
+	// Simple cases, any setting of account or new_account always errors.
+	// Even with proper auth.
 	c, cr, _ := newClientForServer(s)
-	defer c.close()
-	connectOp := "CONNECT {\"account\":\"foo\",\"new_account\":true}\r\n"
+	connectOp = "CONNECT {\"user\":\"ua\", \"pass\":\"pa\", \"account\":\"ANY\"}\r\n"
 	c.parseAsync(connectOp)
 	l, _ := cr.ReadString('\n')
-	if !strings.HasPrefix(l, "-ERR ") {
-		t.Fatalf("Expected an error")
+	if !strings.HasPrefix(l, "-ERR 'Authorization Violation'") {
+		t.Fatalf("Expected an error, got %q", l)
 	}
+	c.close()
 
-	// Now allow new accounts on the fly, make sure second time does not work.
-	opts := defaultServerOptions
-	opts.AllowNewAccounts = true
-	s = New(&opts)
+	// new_account with proper credentials.
+	c, cr, _ = newClientForServer(s)
+	connectOp = "CONNECT {\"user\":\"ua\", \"pass\":\"pa\", \"new_account\":true}\r\n"
+	c.parseAsync(connectOp)
+	l, _ = cr.ReadString('\n')
+	if !strings.HasPrefix(l, "-ERR 'Authorization Violation'") {
+		t.Fatalf("Expected an error, got %q", l)
+	}
+	c.close()
 
-	c, _, _ = newClientForServer(s)
-	defer c.close()
-	err := c.parse([]byte(connectOp))
-	if err != nil {
-		t.Fatalf("Received an error trying to create an account: %v", err)
+	// switch acccounts with proper credentials.
+	c, cr, _ = newClientForServer(s)
+	connectOp = "CONNECT {\"user\":\"ua\", \"pass\":\"pa\", \"account\":\"B\"}\r\n"
+	c.parseAsync(connectOp)
+	l, _ = cr.ReadString('\n')
+	if !strings.HasPrefix(l, "-ERR 'Authorization Violation'") {
+		t.Fatalf("Expected an error, got %q", l)
 	}
+	c.close()
 
+	// Even if correct account designation, still make sure we error.
 	c, cr, _ = newClientForServer(s)
-	defer c.close()
+	connectOp = "CONNECT {\"user\":\"ua\", \"pass\":\"pa\", \"account\":\"A\"}\r\n"
 	c.parseAsync(connectOp)
 	l, _ = cr.ReadString('\n')
-	if !strings.HasPrefix(l, "-ERR ") {
-		t.Fatalf("Expected an error")
+	if !strings.HasPrefix(l, "-ERR 'Authorization Violation'") {
+		t.Fatalf("Expected an error, got %q", l)
 	}
+	c.close()
 }
 
 func accountNameExists(name string, accounts []*Account) bool {

server/client.go

@@ -1,4 +1,4 @@
-// Copyright 2012-2021 The NATS Authors
+// Copyright 2012-2022 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -1822,35 +1822,15 @@ func (c *client) processConnect(arg []byte) error {
 			return ErrAuthentication
 		}
 
-		// Check for Account designation, this section should be only used when there is not a jwt.
-		if account != _EMPTY_ {
-			var acc *Account
-			var wasNew bool
-			var err error
-			if !srv.NewAccountsAllowed() {
-				acc, err = srv.LookupAccount(account)
-				if err != nil {
-					c.Errorf(err.Error())
-					c.sendErr(ErrMissingAccount.Error())
-					return err
-				} else if accountNew && acc != nil {
-					c.sendErrAndErr(ErrAccountExists.Error())
-					return ErrAccountExists
-				}
-			} else {
-				// We can create this one on the fly.
-				acc, wasNew = srv.LookupOrRegisterAccount(account)
-				if accountNew && !wasNew {
-					c.sendErrAndErr(ErrAccountExists.Error())
-					return ErrAccountExists
-				}
-			}
-			// If we are here we can register ourselves with the new account.
-			if err := c.registerWithAccount(acc); err != nil {
-				c.reportErrRegisterAccount(acc, err)
-				return ErrBadAccount
-			}
-		} else if c.acc == nil {
+		// Check for Account designation, we used to have this as an optional feature for dynamic
+		// sandbox environments. Now its considered an error.
+		if accountNew || account != _EMPTY_ {
+			c.authViolation()
+			return ErrAuthentication
+		}
+
+		// If no account designation.
+		if c.acc == nil {
 			// By default register with the global account.
 			c.registerWithAccount(srv.globalAccount())
 		}

server/events.go

@@ -343,7 +343,6 @@ RESET:
 						b, _ = json.Marshal(pm.msg)
 					}
 				}
-
 				// Setup our client. If the user wants to use a non-system account use our internal
 				// account scoped here so that we are not changing out accounts for the system client.
 				var c *client
@@ -1617,7 +1616,7 @@ func (s *Server) sendLeafNodeConnect(a *Account) {
 func (s *Server) sendLeafNodeConnectMsg(accName string) {
 	subj := fmt.Sprintf(leafNodeConnectEventSubj, accName)
 	m := accNumConnsReq{Account: accName}
-	s.sendInternalMsg(subj, "", &m.Server, &m)
+	s.sendInternalMsg(subj, _EMPTY_, &m.Server, &m)
 }
 
 // sendAccConnsUpdate is called to send out our information on the

server/jwt.go

@@ -1,4 +1,4 @@
-// Copyright 2018-2019 The NATS Authors
+// Copyright 2018-2022 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -70,9 +70,6 @@ func validateTrustedOperators(o *Options) error {
 	if len(o.TrustedOperators) == 0 {
 		return nil
 	}
-	if o.AllowNewAccounts {
-		return fmt.Errorf("operators do not allow dynamic creation of new accounts")
-	}
 	if o.AccountResolver == nil {
 		return fmt.Errorf("operators require an account resolver to be configured")
 	}

server/opts.go

@@ -204,7 +204,6 @@ type Options struct {
 	NoAuthUser            string            `json:"-"`
 	SystemAccount         string            `json:"-"`
 	NoSystemAccount       bool              `json:"-"`
-	AllowNewAccounts      bool              `json:"-"`
 	Username              string            `json:"-"`
 	Password              string            `json:"-"`
 	Authorization         string            `json:"-"`

server/route.go

@@ -1035,20 +1035,17 @@ func (c *client) processRemoteSub(argo []byte, hasOrigin bool) (err error) {
 		acc = v.(*Account)
 	}
 	if acc == nil {
-		expire := false
 		isNew := false
-		if !srv.NewAccountsAllowed() {
-			// if the option of retrieving accounts later exists, create an expired one.
-			// When a client comes along, expiration will prevent it from being used,
-			// cause a fetch and update the account to what is should be.
-			if staticResolver {
-				c.Errorf("Unknown account %q for remote subject %q", accountName, sub.subject)
-				return
-			}
-			c.Debugf("Unknown account %q for remote subject %q", accountName, sub.subject)
-			expire = true
+		// if the option of retrieving accounts later exists, create an expired one.
+		// When a client comes along, expiration will prevent it from being used,
+		// cause a fetch and update the account to what is should be.
+		if staticResolver {
+			c.Errorf("Unknown account %q for remote subject %q", accountName, sub.subject)
+			return
 		}
-		if acc, isNew = srv.LookupOrRegisterAccount(accountName); isNew && expire {
+		c.Debugf("Unknown account %q for remote subject %q", accountName, sub.subject)
+
+		if acc, isNew = srv.LookupOrRegisterAccount(accountName); isNew {
 			acc.mu.Lock()
 			acc.expired = true
 			acc.incomplete = true

server/server.go

@@ -1108,13 +1108,6 @@ func (s *Server) logPid() error {
 	return ioutil.WriteFile(s.getOpts().PidFile, []byte(pidStr), 0660)
 }
 
-// NewAccountsAllowed returns whether or not new accounts can be created on the fly.
-func (s *Server) NewAccountsAllowed() bool {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	return s.opts.AllowNewAccounts
-}
-
 // numReservedAccounts will return the number of reserved accounts configured in the server.
 // Currently this is 1, one for the global default account.
 func (s *Server) numReservedAccounts() int {