rpcperms: allow multiple read-only middleware

itest/lnd_rpc_middleware_interceptor_test.go

@@ -78,6 +78,19 @@ func testRPCMiddlewareInterceptor(ht *lntest.HarnessTest) {
 		)
 	})
 
+	// Test that multiple read-only middlewares can be registered at the
+	// same time and that both receive intercept messages.
+	//
+	// NOTE: we restart the node here to make sure the old interceptor is
+	// removed from registration.
+	ht.RestartNode(alice)
+	ht.EnsureConnected(alice, bob)
+	ht.Run("multiple read-only middlewares", func(tt *testing.T) {
+		multipleReadOnlyMiddlewareTest(
+			tt, alice, bob, readonlyMac,
+		)
+	})
+
 	// We've manually disconnected Bob from Alice in the previous test, make
 	// sure they're connected again.
 	//
@@ -207,6 +220,61 @@ func middlewareRegistrationRestrictionTests(t *testing.T,
 	}
 }
 
+// multipleReadOnlyMiddlewareTest verifies that multiple read-only middlewares
+// can be registered simultaneously and that both receive intercept messages for
+// the same RPC call.
+func multipleReadOnlyMiddlewareTest(t *testing.T, node,
+	peer *node.HarnessNode, userMac *macaroon.Macaroon) {
+
+	t.Helper()
+
+	ctxb := t.Context()
+	ctxc, cancel := context.WithTimeout(ctxb, defaultTimeout)
+	defer cancel()
+
+	// Register two read-only middlewares with different names.
+	reg1 := registerMiddleware(
+		t, node, &lnrpc.MiddlewareRegistration{
+			MiddlewareName: "itest-readonly-one",
+			ReadOnlyMode:   true,
+		}, true,
+	)
+	defer reg1.cancel()
+
+	reg2 := registerMiddleware(
+		t, node, &lnrpc.MiddlewareRegistration{
+			MiddlewareName: "itest-readonly-two",
+			ReadOnlyMode:   true,
+		}, true,
+	)
+	defer reg2.cancel()
+
+	// Create a client connection to simulate a user request.
+	cleanup, client := macaroonClient(t, node, userMac)
+	defer cleanup()
+
+	// Send a simple RPC request listing all channels to trigger the rpc
+	// interceptors. We need to invoke the intercept logic in a goroutine
+	// because we'd block the execution of the main task otherwise.
+	req := &lnrpc.ListChannelsRequest{ActiveOnly: true}
+	go reg1.interceptUnary(
+		"/lnrpc.Lightning/ListChannels", req, nil, true, false,
+		nil,
+	)
+	go reg2.interceptUnary(
+		"/lnrpc.Lightning/ListChannels", req, nil, true, false,
+		nil,
+	)
+
+	// Do the actual call now and wait for both interceptors to process.
+	resp, err := client.ListChannels(ctxc, req)
+	require.NoError(t, err)
+
+	// Verify both middlewares received the response intercept message.
+	assertInterceptedType(t, resp, <-reg1.responsesChan)
+	assertInterceptedType(t, resp, <-reg2.responsesChan)
+}
+
 // middlewareInterceptionTest tests that unary and streaming requests can be
 // intercepted. It also makes sure that depending on the mode (read-only or
 // custom macaroon caveat) a middleware only gets access to the requests it

rpcperms/interceptor.go

@@ -443,9 +443,9 @@ func (r *InterceptorChain) Permissions() map[string][]bakery.Op {
 
 // RegisterMiddleware registers a new middleware that will handle request/
 // response interception for all RPC messages that are initiated with a custom
-// macaroon caveat. The name of the custom caveat a middleware is handling is
-// also its unique identifier. Only one middleware can be registered for each
-// custom caveat.
+// macaroon caveat. Only one read/write middleware can be registered for each
+// custom caveat name. Multiple read-only middlewares are permitted since they
+// cannot modify responses.
 func (r *InterceptorChain) RegisterMiddleware(mw *MiddlewareHandler) error {
 	r.Lock()
 	defer r.Unlock()
@@ -457,12 +457,15 @@ func (r *InterceptorChain) RegisterMiddleware(mw *MiddlewareHandler) error {
 			"registered", mw.middlewareName)
 	}
 
-	// For now, we only want one middleware per custom caveat name. If we
-	// allowed multiple middlewares handling the same caveat there would be
-	// a need for extra call chaining logic, and they could overwrite each
-	// other's responses.
+	// We only want one read/write middleware per custom caveat name since
+	// multiple could overwrite each other's responses. Read-only
+	// middlewares are exempt because they cannot modify responses.
 	for _, middleware := range r.registeredMiddleware {
 		if middleware.customCaveatName == mw.customCaveatName {
+			if middleware.readOnly && mw.readOnly {
+				continue
+			}
+
 			return fmt.Errorf("a middleware is already registered "+
 				"for the custom caveat name '%s': %v",
 				mw.customCaveatName, middleware.middlewareName)