diff --git a/itest/lnd_rpc_middleware_interceptor_test.go b/itest/lnd_rpc_middleware_interceptor_test.go index 5b16da015f9..fbee49a5393 100644 --- a/itest/lnd_rpc_middleware_interceptor_test.go +++ b/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 diff --git a/rpcperms/interceptor.go b/rpcperms/interceptor.go index 9bbef0414da..70584bfd46f 100644 --- a/rpcperms/interceptor.go +++ b/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)