Enhance NATS provider to support multiple authentication methods

[latchmihay]

Enhance NATS provider to support multiple authentication methods including JWT, NKey, and Username/Password.

This would allow the notification-controller to support the recommended NATS 2.0+ authentication paradigms.

[latchmihay]

@cappyzawa fixed it, its pointing to main now

[cappyzawa]

Thanks for the contribution!

Please run go mod tidy to update go.mod. The github.com/nats-io/nkeys package is currently listed as an indirect dependency, but since it's now directly imported in nats.go, it should be a direct dependency.

[cappyzawa]

One suggestion: the auth decision in publish() could be moved to NewNATS. Since we already know which auth method to use at construction time, we can capture it as a closure and just call it in publish().

This keeps publish() focused on "connect and send" rather than "figure out auth and connect and send".

Something like:

type natsClient struct {
    server string
    authFn func() (nats.Option, func(), error)
}

func NewNATS(...) (*NATS, error) {
    client := &natsClient{server: server}
    
    if len(credsData) > 0 {
        client.authFn = func() (nats.Option, func(), error) {
            return nats.UserCredentialBytes(credsData), nil, nil
        }
    } else if len(nkeySeed) > 0 {
        client.authFn = func() (nats.Option, func(), error) {
            kp, err := nkeys.FromSeed(nkeySeed)
            if err != nil {
                return nil, nil, fmt.Errorf("error parsing nkey seed: %w", err)
            }
            pubKey, err := kp.PublicKey()
            if err != nil {
                kp.Wipe()
                return nil, nil, fmt.Errorf("error getting public key: %w", err)
            }
            sigCB := func(nonce []byte) ([]byte, error) {
                return kp.Sign(nonce)
            }
            return nats.Nkey(pubKey, sigCB), kp.Wipe, nil
        }
    } else if username != "" && password != "" {
        client.authFn = func() (nats.Option, func(), error) {
            return nats.UserInfo(username, password), nil, nil
        }
    }
    // ...
}

func (n *natsClient) publish(...) error {
    opts := []nats.Option{nats.Name("NATS Provider Publisher")}
    
    if n.authFn != nil {
        authOpt, cleanup, err := n.authFn()
        if err != nil {
            return err
        }
        if cleanup != nil {
            defer cleanup()
        }
        opts = append(opts, authOpt)
    }
    
    nc, err := nats.Connect(n.server, opts...)
    // ...
}

The cleanup func handles kp.Wipe() for nkey auth - it gets deferred and runs after the connection is done.

Not a blocker, just a suggestion for cleaner separation.

[cappyzawa]

The authFn refactoring looks clean! 👍

One thing I noticed: the current test checks authFn != nil, but doesn't actually call authFn() to verify it returns the correct auth type. Since we moved away from storing fields directly, it'd be nice to confirm the returned nats.Option actually sets the right authentication.

Also, the "credentials file data is stored" and "nkey seed data is stored" cases could double as priority tests by adding the other auth params - this way we can verify creds > nkey > userpass priority works correctly.

Here's a minimal diff that adds this verification:

+	"github.com/nats-io/nats.go"

+		// Priority tests: creds > nkey > userpass
 		{
-			name:            "credentials file data is stored",
+			name:            "creds takes priority over nkey and userpass",
 			subject:         "test",
 			server:          "nats",
+			username:        "user",
+			password:        "pass",
 			credsData:       []byte("credentials-content"),
+			nkeySeed:        []byte("SUAGMJH5XLGZKQQWAWKRZJIGMOU4HPFUYLXJMXOO5NLFEO2OOQJ5LPRDPM"),
 			expectedSubject: "test",
 			expectedCreds:   true,
 		},
 		{
-			name:            "nkey seed data is stored",
+			name:            "nkey takes priority over userpass",
 			subject:         "test",
 			server:          "nats",
+			username:        "user",
+			password:        "pass",
 			nkeySeed:        []byte("SUAGMJH5XLGZKQQWAWKRZJIGMOU4HPFUYLXJMXOO5NLFEO2OOQJ5LPRDPM"),
 			expectedSubject: "test",
 			expectedNkey:    true,
 		},

And add verification logic after checking client.authFn:

				// Verify authFn returns valid option and correct auth type
				if client.authFn != nil {
					opt, cleanup, err := client.authFn()
					g.Expect(err).To(BeNil(), "authFn should not return error")
					g.Expect(opt).NotTo(BeNil(), "authFn should return a valid option")
					if cleanup != nil {
						defer cleanup()
					}

					// Apply option to verify which auth method was selected
					var opts nats.Options
					g.Expect(opt(&opts)).To(Succeed())

					if tt.expectedCreds {
						g.Expect(opts.UserJWT).NotTo(BeNil(), "creds auth should set UserJWT")
					} else if tt.expectedNkey {
						g.Expect(opts.Nkey).NotTo(BeEmpty(), "nkey auth should set Nkey")
						g.Expect(opts.SignatureCB).NotTo(BeNil(), "nkey auth should set SignatureCB")
					} else if tt.expectedUsername != "" {
						g.Expect(opts.User).To(Equal(tt.expectedUsername), "userpass auth should set User")
						g.Expect(opts.Password).To(Equal(tt.expectedPassword), "userpass auth should set Password")
					}
				}

This keeps the existing test structure while adding actual auth type verification. What do you think?

[latchmihay]

This keeps the existing test structure while adding actual auth type verification. What do you think?

Thank you for the suggestions, they have been added :)

[cappyzawa]

LGTM! Thanks for implementing the test improvements - the priority tests and authFn verification look great. 🎉