diff --git a/services/connector/api.go b/services/connector/api.go index 75d7face6..14b536add 100644 --- a/services/connector/api.go +++ b/services/connector/api.go @@ -29,11 +29,13 @@ func NewAPI(s *Service) *API { }) // Accounts query and dapp permissions - r.Register("eth_accounts", &commands.AccountsCommand{Db: s.db}) - r.Register("eth_requestAccounts", &commands.RequestAccountsCommand{ + // NOTE: Some dApps expect same behavior for both eth_accounts and eth_requestAccounts + accountsCommand := &commands.RequestAccountsCommand{ ClientHandler: c, AccountsCommand: commands.AccountsCommand{Db: s.db}, - }) + } + r.Register("eth_accounts", accountsCommand) + r.Register("eth_requestAccounts", accountsCommand) // Active chain per dapp management r.Register("eth_chainId", &commands.ChainIDCommand{ @@ -45,8 +47,11 @@ func NewAPI(s *Service) *API { NetworkManager: s.nm, }) - // Request permissions + // Permissions r.Register("wallet_requestPermissions", &commands.RequestPermissionsCommand{}) + r.Register("wallet_revokePermissions", &commands.RevokePermissionsCommand{ + Db: s.db, + }) return &API{ s: s, @@ -102,6 +107,10 @@ func (api *API) RecallDAppPermission(origin string) error { return persistence.DeleteDApp(api.s.db, origin) } +func (api *API) GetPermittedDAppsList() ([]persistence.DApp, error) { + return persistence.SelectAllDApps(api.s.db) +} + func (api *API) RequestAccountsAccepted(args commands.RequestAccountsAcceptedArgs) error { return api.c.RequestAccountsAccepted(args) } diff --git a/services/connector/commands/request_accounts.go b/services/connector/commands/request_accounts.go index 1c1c4e995..8e53e6b5a 100644 --- a/services/connector/commands/request_accounts.go +++ b/services/connector/commands/request_accounts.go @@ -38,11 +38,12 @@ func (c *RequestAccountsCommand) Execute(request RPCRequest) (interface{}, error // FIXME: this may have a security issue in case some malicious software tries to fake the origin if dApp == nil { - account, chainID, err := c.ClientHandler.RequestShareAccountForDApp(signal.ConnectorDApp{ + connectorDApp := signal.ConnectorDApp{ URL: request.URL, Name: request.Name, IconURL: request.IconURL, - }) + } + account, chainID, err := c.ClientHandler.RequestShareAccountForDApp(connectorDApp) if err != nil { return "", err } @@ -59,6 +60,7 @@ func (c *RequestAccountsCommand) Execute(request RPCRequest) (interface{}, error if err != nil { return "", err } + signal.SendConnectorDAppPermissionGranted(connectorDApp) } return FormatAccountAddressToResponse(dApp.SharedAccount), nil diff --git a/services/connector/commands/request_accounts_test.go b/services/connector/commands/request_accounts_test.go index 6e2989597..e055d45ec 100644 --- a/services/connector/commands/request_accounts_test.go +++ b/services/connector/commands/request_accounts_test.go @@ -65,6 +65,7 @@ func TestRequestAccountsAcceptedAndRequestAgain(t *testing.T) { assert.NoError(t, err) accountAddress := types.Address{0x03} + dAppPermissionGranted := false signal.SetMobileSignalHandler(signal.MobileSignalHandler(func(s []byte) { var evt EventType @@ -83,6 +84,8 @@ func TestRequestAccountsAcceptedAndRequestAgain(t *testing.T) { ChainID: walletCommon.EthereumMainnet, }) assert.NoError(t, err) + case signal.EventConnectorDAppPermissionGranted: + dAppPermissionGranted = true } })) @@ -104,6 +107,8 @@ func TestRequestAccountsAcceptedAndRequestAgain(t *testing.T) { response, err = cmd.Execute(request) assert.NoError(t, err) assert.Equal(t, expectedResponse, response) + + assert.True(t, dAppPermissionGranted) } func TestRequestAccountsRejected(t *testing.T) { diff --git a/services/connector/commands/revoke_permissions.go b/services/connector/commands/revoke_permissions.go new file mode 100644 index 000000000..997218fe6 --- /dev/null +++ b/services/connector/commands/revoke_permissions.go @@ -0,0 +1,41 @@ +package commands + +import ( + "database/sql" + + persistence "github.com/status-im/status-go/services/connector/database" + "github.com/status-im/status-go/signal" +) + +type RevokePermissionsCommand struct { + Db *sql.DB +} + +func (c *RevokePermissionsCommand) Execute(request RPCRequest) (interface{}, error) { + err := request.Validate() + if err != nil { + return "", err + } + + dApp, err := persistence.SelectDAppByUrl(c.Db, request.URL) + if err != nil { + return "", err + } + + if dApp == nil { + return "", ErrDAppIsNotPermittedByUser + } + + err = persistence.DeleteDApp(c.Db, dApp.URL) + if err != nil { + return "", err + } + + signal.SendConnectorDAppPermissionRevoked(signal.ConnectorDApp{ + URL: request.URL, + Name: request.Name, + IconURL: request.IconURL, + }) + + return nil, nil +} diff --git a/services/connector/commands/revoke_permissions_test.go b/services/connector/commands/revoke_permissions_test.go new file mode 100644 index 000000000..74fc4b142 --- /dev/null +++ b/services/connector/commands/revoke_permissions_test.go @@ -0,0 +1,75 @@ +package commands + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/status-im/status-go/eth-node/types" + persistence "github.com/status-im/status-go/services/connector/database" + "github.com/status-im/status-go/signal" +) + +func TestFailToRevokePermissionsWithMissingDAppFields(t *testing.T) { + cmd := &RequestPermissionsCommand{} + + // Missing DApp fields + request, err := ConstructRPCRequest("wallet_revokePermissions", []interface{}{}, nil) + assert.NoError(t, err) + + result, err := cmd.Execute(request) + assert.Equal(t, ErrRequestMissingDAppData, err) + assert.Empty(t, result) +} + +func TestFailToRevokePermissionsForUnpermittedDApp(t *testing.T) { + db, close := SetupTestDB(t) + defer close() + + cmd := &RevokePermissionsCommand{Db: db} + + request, err := ConstructRPCRequest("wallet_revokePermissions", []interface{}{}, &testDAppData) + assert.NoError(t, err) + + result, err := cmd.Execute(request) + assert.Equal(t, ErrDAppIsNotPermittedByUser, err) + assert.Empty(t, result) +} + +func TestRevokePermissionsSucceeded(t *testing.T) { + db, close := SetupTestDB(t) + defer close() + + cmd := &RevokePermissionsCommand{Db: db} + + sharedAccount := types.BytesToAddress(types.FromHex("0x6d0aa2a774b74bb1d36f97700315adf962c69fcg")) + dAppPermissionRevoked := false + + signal.SetMobileSignalHandler(signal.MobileSignalHandler(func(s []byte) { + var evt EventType + err := json.Unmarshal(s, &evt) + assert.NoError(t, err) + + switch evt.Type { + case signal.EventConnectorDAppPermissionRevoked: + dAppPermissionRevoked = true + } + })) + + err := PersistDAppData(db, testDAppData, sharedAccount, 0x123) + assert.NoError(t, err) + + request, err := ConstructRPCRequest("wallet_revokePermissions", []interface{}{}, &testDAppData) + assert.NoError(t, err) + + result, err := cmd.Execute(request) + assert.NoError(t, err) + assert.Empty(t, result) + + dApp, err := persistence.SelectDAppByUrl(db, testDAppData.URL) + assert.NoError(t, err) + assert.Nil(t, dApp) + + assert.True(t, dAppPermissionRevoked) +} diff --git a/services/connector/connector_flows_test.go b/services/connector/connector_flows_test.go index fb993fcee..8ddc000d0 100644 --- a/services/connector/connector_flows_test.go +++ b/services/connector/connector_flows_test.go @@ -36,22 +36,22 @@ func TestRequestAccountsSwitchChainAndSendTransactionFlow(t *testing.T) { api := NewAPI(service) - // Try to request accounts without permission - request := "{\"method\":\"eth_accounts\",\"params\":[],\"url\":\"http://testDAppURL123\",\"name\":\"testDAppName\",\"iconUrl\":\"http://testDAppIconUrl\"}" - response, err := api.CallRPC(request) - assert.Empty(t, response) - assert.Error(t, err) - assert.Equal(t, commands.ErrDAppIsNotPermittedByUser, err) - accountAddress := types.BytesToAddress(types.FromHex("0x6d0aa2a774b74bb1d36f97700315adf962c69fcg")) expectedHash := types.BytesToHash(types.FromHex("0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef")) + dAppPermissionRevoked := false + dAppPermissionGranted := false + signal.SetMobileSignalHandler(signal.MobileSignalHandler(func(s []byte) { var evt commands.EventType err := json.Unmarshal(s, &evt) assert.NoError(t, err) switch evt.Type { + case signal.EventConnectorDAppPermissionRevoked: + dAppPermissionRevoked = true + case signal.EventConnectorDAppPermissionGranted: + dAppPermissionGranted = true case signal.EventConnectorSendRequestAccounts: var ev signal.ConnectorSendRequestAccountsSignal err := json.Unmarshal(evt.Event, &ev) @@ -77,10 +77,12 @@ func TestRequestAccountsSwitchChainAndSendTransactionFlow(t *testing.T) { })) // Request accounts, now for real - request = "{\"method\": \"eth_requestAccounts\", \"params\": [], \"url\": \"http://testDAppURL123\", \"name\": \"testDAppName\", \"iconUrl\": \"http://testDAppIconUrl\" }" - response, err = api.CallRPC(request) + request := "{\"method\": \"eth_requestAccounts\", \"params\": [], \"url\": \"http://testDAppURL123\", \"name\": \"testDAppName\", \"iconUrl\": \"http://testDAppIconUrl\" }" + response, err := api.CallRPC(request) assert.NoError(t, err) assert.Equal(t, commands.FormatAccountAddressToResponse(accountAddress), response) + assert.Equal(t, true, dAppPermissionGranted) + assert.Equal(t, false, dAppPermissionRevoked) // Request to switch ethereum chain expectedChainID, err := chainutils.GetHexChainID(walletCommon.ChainID(walletCommon.EthereumMainnet).String()) @@ -107,6 +109,19 @@ func TestRequestAccountsSwitchChainAndSendTransactionFlow(t *testing.T) { response, err = api.CallRPC(request) assert.NoError(t, err) assert.Equal(t, expectedHash.Hex(), response) + + // Revoke permissions + request = "{\"method\": \"wallet_revokePermissions\", \"params\": [], \"url\": \"http://testDAppURL123\", \"name\": \"testDAppName\", \"iconUrl\": \"http://testDAppIconUrl\" }" + _, err = api.CallRPC(request) + assert.NoError(t, err) + + // Check if the account was revoked + request = fmt.Sprintf("{\"method\": \"eth_sendTransaction\", \"params\":[{\"from\":\"%s\",\"to\":\"0x0200000000000000000000000000000000000000\",\"value\":\"0x12345\",\"data\":\"0x307830\"}], \"url\": \"http://testDAppURL123\", \"name\": \"testDAppName\", \"iconUrl\": \"http://testDAppIconUrl\" }", accountAddress.Hex()) + response, err = api.CallRPC(request) + assert.Empty(t, response) + assert.Error(t, err) + assert.Equal(t, commands.ErrDAppIsNotPermittedByUser, err) + assert.Equal(t, true, dAppPermissionRevoked) } func TestForwardedRPCs(t *testing.T) { diff --git a/services/connector/database/persistence.go b/services/connector/database/persistence.go index 201a633b9..05abfdbc4 100644 --- a/services/connector/database/persistence.go +++ b/services/connector/database/persistence.go @@ -8,6 +8,7 @@ import ( const upsertDAppQuery = "INSERT INTO connector_dapps (url, name, icon_url, shared_account, chain_id) VALUES (?, ?, ?, ?, ?) ON CONFLICT(url) DO UPDATE SET name = excluded.name, icon_url = excluded.icon_url, shared_account = excluded.shared_account, chain_id = excluded.chain_id" const selectDAppByUrlQuery = "SELECT name, icon_url, shared_account, chain_id FROM connector_dapps WHERE url = ?" +const selectDAppsQuery = "SELECT url, name, icon_url, shared_account, chain_id FROM connector_dapps" const deleteDAppQuery = "DELETE FROM connector_dapps WHERE url = ?" type DApp struct { @@ -34,6 +35,25 @@ func SelectDAppByUrl(db *sql.DB, url string) (*DApp, error) { return dApp, err } +func SelectAllDApps(db *sql.DB) ([]DApp, error) { + rows, err := db.Query(selectDAppsQuery) + if err != nil { + return nil, err + } + defer rows.Close() + + var dApps []DApp + for rows.Next() { + dApp := DApp{} + err = rows.Scan(&dApp.URL, &dApp.Name, &dApp.IconURL, &dApp.SharedAccount, &dApp.ChainID) + if err != nil { + return nil, err + } + dApps = append(dApps, dApp) + } + return dApps, nil +} + func DeleteDApp(db *sql.DB, url string) error { _, err := db.Exec(deleteDAppQuery, url) return err diff --git a/services/connector/database/persistence_test.go b/services/connector/database/persistence_test.go index f6a525394..7ef1d5989 100644 --- a/services/connector/database/persistence_test.go +++ b/services/connector/database/persistence_test.go @@ -80,3 +80,16 @@ func TestInsertAndRemoveDApp(t *testing.T) { require.NoError(t, err) require.Empty(t, dAppBack) } + +func TestSelectAllDApps(t *testing.T) { + db, close := setupTestDB(t) + defer close() + + err := UpsertDApp(db, &testDApp) + require.NoError(t, err) + + dApps, err := SelectAllDApps(db) + require.NoError(t, err) + require.Len(t, dApps, 1) + require.Equal(t, testDApp, dApps[0]) +} diff --git a/signal/events_connector.go b/signal/events_connector.go index a3ea1315e..9644a61be 100644 --- a/signal/events_connector.go +++ b/signal/events_connector.go @@ -1,8 +1,10 @@ package signal const ( - EventConnectorSendRequestAccounts = "connector.sendRequestAccounts" - EventConnectorSendTransaction = "connector.sendTransaction" + EventConnectorSendRequestAccounts = "connector.sendRequestAccounts" + EventConnectorSendTransaction = "connector.sendTransaction" + EventConnectorDAppPermissionGranted = "connector.dAppPermissionGranted" + EventConnectorDAppPermissionRevoked = "connector.dAppPermissionRevoked" ) type ConnectorDApp struct { @@ -40,3 +42,11 @@ func SendConnectorSendTransaction(dApp ConnectorDApp, chainID uint64, txArgs str TxArgs: txArgs, }) } + +func SendConnectorDAppPermissionGranted(dApp ConnectorDApp) { + send(EventConnectorDAppPermissionGranted, dApp) +} + +func SendConnectorDAppPermissionRevoked(dApp ConnectorDApp) { + send(EventConnectorDAppPermissionRevoked, dApp) +}