From 8924ee69486d857ab27fa40376a31dfd0cbbdf52 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 14 Jan 2022 16:58:25 +0100 Subject: [PATCH] refactor: reusable metadata validation (#729) * define and generate new metadata proto type * refactor types pkg, remove version string parsers, add PortPrefix * refactor ica entrypoint and handshake to handle connection ids in metadata * fixing broken test cases * adding controller port and metadata validation, adding new testcases * updating proto doc and removing commented out code * updating testcase names, adding metadata constructor func, updating PortPrefix and removing Delimiter * adding ErrInvalidControllerPort and ErrInvalidHostPort * refactoring metadata validation to reusable func * returning correct err type * regenerating protos after merge conflicts * adding separate validation funcs for controller and host * correcting error msg in ValidateHostMetadata * updating with review suggestions * adding additional empty address check to ACK step, adding test case * adding strings.Trimspace * adding success with empty address testcase for ValidateHostMetadata --- .../controller/keeper/handshake.go | 37 +-- .../controller/keeper/handshake_test.go | 12 + .../host/keeper/handshake.go | 26 +-- .../27-interchain-accounts/types/metadata.go | 68 ++++++ .../types/metadata_test.go | 214 ++++++++++++++++++ 5 files changed, 304 insertions(+), 53 deletions(-) create mode 100644 modules/apps/27-interchain-accounts/types/metadata_test.go diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go index e6c6f85366a..fb38401a0b2 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go @@ -8,7 +8,6 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" - connectiontypes "github.com/cosmos/ibc-go/v3/modules/core/03-connection/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" porttypes "github.com/cosmos/ibc-go/v3/modules/core/05-port/types" ) @@ -47,14 +46,10 @@ func (k Keeper) OnChanOpenInit( return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata") } - if err := k.validateConnectionParams(ctx, connectionHops, metadata.ControllerConnectionId, metadata.HostConnectionId); err != nil { + if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil { return err } - if metadata.Version != icatypes.Version { - return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version) - } - activeChannelID, found := k.GetOpenActiveChannel(ctx, portID) if found { return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "existing active channel %s for portID %s", activeChannelID, portID) @@ -84,12 +79,17 @@ func (k Keeper) OnChanOpenAck( return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata") } - if err := icatypes.ValidateAccountAddress(metadata.Address); err != nil { + channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID) + if !found { + return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID) + } + + if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, metadata); err != nil { return err } - if metadata.Version != icatypes.Version { - return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version) + if strings.TrimSpace(metadata.Address) == "" { + return sdkerrors.Wrap(icatypes.ErrInvalidAccountAddress, "interchain account address cannot be empty") } k.SetActiveChannelID(ctx, portID, channelID) @@ -107,22 +107,3 @@ func (k Keeper) OnChanCloseConfirm( return nil } - -// validateConnectionParams asserts the provided controller and host connection identifiers match that of the associated connection stored in state -func (k Keeper) validateConnectionParams(ctx sdk.Context, connectionHops []string, controllerConnectionID, hostConnectionID string) error { - connectionID := connectionHops[0] - connection, err := k.channelKeeper.GetConnection(ctx, connectionID) - if err != nil { - return err - } - - if controllerConnectionID != connectionID { - return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connectionID, controllerConnectionID) - } - - if hostConnectionID != connection.GetCounterparty().GetConnectionID() { - return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connection.GetCounterparty().GetConnectionID(), hostConnectionID) - } - - return nil -} diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go index d1d9847cde5..1b45d6166dc 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go @@ -223,6 +223,18 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() { }, false, }, + { + "empty account address", + func() { + metadata.Address = "" + + versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + + path.EndpointA.Counterparty.ChannelConfig.Version = string(versionBytes) + }, + false, + }, { "invalid counterparty version", func() { diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake.go b/modules/apps/27-interchain-accounts/host/keeper/handshake.go index 8e0501cd165..a023e42c110 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake.go @@ -8,7 +8,6 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" - connectiontypes "github.com/cosmos/ibc-go/v3/modules/core/03-connection/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v3/modules/core/24-host" ) @@ -44,14 +43,10 @@ func (k Keeper) OnChanOpenTry( return "", sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata") } - if err := k.validateConnectionParams(ctx, connectionHops, metadata.ControllerConnectionId, metadata.HostConnectionId); err != nil { + if err := icatypes.ValidateHostMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil { return "", err } - if metadata.Version != icatypes.Version { - return "", sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version) - } - // On the host chain the capability may only be claimed during the OnChanOpenTry // The capability being claimed in OpenInit is for a controller chain (the port is different) if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { @@ -93,22 +88,3 @@ func (k Keeper) OnChanCloseConfirm( return nil } - -// validateConnectionParams asserts the provided controller and host connection identifiers match that of the associated connection stored in state -func (k Keeper) validateConnectionParams(ctx sdk.Context, connectionHops []string, controllerConnectionID, hostConnectionID string) error { - connectionID := connectionHops[0] - connection, err := k.channelKeeper.GetConnection(ctx, connectionID) - if err != nil { - return err - } - - if hostConnectionID != connectionID { - return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connectionID, controllerConnectionID) - } - - if controllerConnectionID != connection.GetCounterparty().GetConnectionID() { - return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connection.GetCounterparty().GetConnectionID(), hostConnectionID) - } - - return nil -} diff --git a/modules/apps/27-interchain-accounts/types/metadata.go b/modules/apps/27-interchain-accounts/types/metadata.go index c770b74ca3a..acc1dcb940b 100644 --- a/modules/apps/27-interchain-accounts/types/metadata.go +++ b/modules/apps/27-interchain-accounts/types/metadata.go @@ -1,5 +1,12 @@ package types +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + + connectiontypes "github.com/cosmos/ibc-go/v3/modules/core/03-connection/types" +) + // NewMetadata creates and returns a new ICS27 Metadata instance func NewMetadata(version, controllerConnectionID, hostConnectionID, accAddress string) Metadata { return Metadata{ @@ -9,3 +16,64 @@ func NewMetadata(version, controllerConnectionID, hostConnectionID, accAddress s Address: accAddress, } } + +// ValidateControllerMetadata performs validation of the provided ICS27 controller metadata parameters +func ValidateControllerMetadata(ctx sdk.Context, channelKeeper ChannelKeeper, connectionHops []string, metadata Metadata) error { + connection, err := channelKeeper.GetConnection(ctx, connectionHops[0]) + if err != nil { + return err + } + + if err := validateConnectionParams(metadata, connectionHops[0], connection.GetCounterparty().GetConnectionID()); err != nil { + return err + } + + if metadata.Address != "" { + if err := ValidateAccountAddress(metadata.Address); err != nil { + return err + } + } + + if metadata.Version != Version { + return sdkerrors.Wrapf(ErrInvalidVersion, "expected %s, got %s", Version, metadata.Version) + } + + return nil +} + +// ValidateHostMetadata performs validation of the provided ICS27 host metadata parameters +func ValidateHostMetadata(ctx sdk.Context, channelKeeper ChannelKeeper, connectionHops []string, metadata Metadata) error { + connection, err := channelKeeper.GetConnection(ctx, connectionHops[0]) + if err != nil { + return err + } + + if err := validateConnectionParams(metadata, connection.GetCounterparty().GetConnectionID(), connectionHops[0]); err != nil { + return err + } + + if metadata.Address != "" { + if err := ValidateAccountAddress(metadata.Address); err != nil { + return err + } + } + + if metadata.Version != Version { + return sdkerrors.Wrapf(ErrInvalidVersion, "expected %s, got %s", Version, metadata.Version) + } + + return nil +} + +// validateConnectionParams compares the given the controller and host connection IDs to those set in the provided ICS27 Metadata +func validateConnectionParams(metadata Metadata, controllerConnectionID, hostConnectionID string) error { + if metadata.ControllerConnectionId != controllerConnectionID { + return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", controllerConnectionID, metadata.ControllerConnectionId) + } + + if metadata.HostConnectionId != hostConnectionID { + return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", hostConnectionID, metadata.HostConnectionId) + } + + return nil +} diff --git a/modules/apps/27-interchain-accounts/types/metadata_test.go b/modules/apps/27-interchain-accounts/types/metadata_test.go new file mode 100644 index 00000000000..2e569549647 --- /dev/null +++ b/modules/apps/27-interchain-accounts/types/metadata_test.go @@ -0,0 +1,214 @@ +package types_test + +import ( + "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" + ibctesting "github.com/cosmos/ibc-go/v3/testing" +) + +func (suite *TypesTestSuite) TestValidateControllerMetadata() { + + var metadata types.Metadata + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", + func() {}, + true, + }, + { + "success with empty account address", + func() { + metadata = types.Metadata{ + Version: types.Version, + ControllerConnectionId: ibctesting.FirstConnectionID, + HostConnectionId: ibctesting.FirstConnectionID, + Address: "", + } + }, + true, + }, + { + "invalid controller connection", + func() { + metadata = types.Metadata{ + Version: types.Version, + ControllerConnectionId: "connection-10", + HostConnectionId: ibctesting.FirstConnectionID, + Address: TestOwnerAddress, + } + }, + false, + }, + { + "invalid host connection", + func() { + metadata = types.Metadata{ + Version: types.Version, + ControllerConnectionId: ibctesting.FirstConnectionID, + HostConnectionId: "connection-10", + Address: TestOwnerAddress, + } + }, + false, + }, + { + "invalid address", + func() { + metadata = types.Metadata{ + Version: types.Version, + ControllerConnectionId: ibctesting.FirstConnectionID, + HostConnectionId: ibctesting.FirstConnectionID, + Address: " ", + } + }, + false, + }, + { + "invalid version", + func() { + metadata = types.Metadata{ + Version: "invalid version", + ControllerConnectionId: ibctesting.FirstConnectionID, + HostConnectionId: ibctesting.FirstConnectionID, + Address: TestOwnerAddress, + } + }, + false, + }, + } + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + metadata = types.NewMetadata(types.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, TestOwnerAddress) + + tc.malleate() // malleate mutates test data + + err := types.ValidateControllerMetadata( + suite.chainA.GetContext(), + suite.chainA.App.GetIBCKeeper().ChannelKeeper, + []string{ibctesting.FirstConnectionID}, + metadata, + ) + + if tc.expPass { + suite.Require().NoError(err, tc.name) + } else { + suite.Require().Error(err, tc.name) + } + }) + } +} + +func (suite *TypesTestSuite) TestValidateHostMetadata() { + + var metadata types.Metadata + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", + func() {}, + true, + }, + { + "success with empty account address", + func() { + metadata = types.Metadata{ + Version: types.Version, + ControllerConnectionId: ibctesting.FirstConnectionID, + HostConnectionId: ibctesting.FirstConnectionID, + Address: "", + } + }, + true, + }, + { + "invalid controller connection", + func() { + metadata = types.Metadata{ + Version: types.Version, + ControllerConnectionId: "connection-10", + HostConnectionId: ibctesting.FirstConnectionID, + Address: TestOwnerAddress, + } + }, + false, + }, + { + "invalid host connection", + func() { + metadata = types.Metadata{ + Version: types.Version, + ControllerConnectionId: ibctesting.FirstConnectionID, + HostConnectionId: "connection-10", + Address: TestOwnerAddress, + } + }, + false, + }, + { + "invalid address", + func() { + metadata = types.Metadata{ + Version: types.Version, + ControllerConnectionId: ibctesting.FirstConnectionID, + HostConnectionId: ibctesting.FirstConnectionID, + Address: " ", + } + }, + false, + }, + { + "invalid version", + func() { + metadata = types.Metadata{ + Version: "invalid version", + ControllerConnectionId: ibctesting.FirstConnectionID, + HostConnectionId: ibctesting.FirstConnectionID, + Address: TestOwnerAddress, + } + }, + false, + }, + } + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + metadata = types.NewMetadata(types.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, TestOwnerAddress) + + tc.malleate() // malleate mutates test data + + err := types.ValidateHostMetadata( + suite.chainA.GetContext(), + suite.chainA.App.GetIBCKeeper().ChannelKeeper, + []string{ibctesting.FirstConnectionID}, + metadata, + ) + + if tc.expPass { + suite.Require().NoError(err, tc.name) + } else { + suite.Require().Error(err, tc.name) + } + }) + } +}