#36 Upgrade method permissions checks

Sources/WalletConnect/Engine/SessionEngine.swift

@@ -213,25 +213,24 @@ final class SessionEngine {
         relayer.request(.wcSessionUpdate(SessionType.UpdateParams(accounts: accounts)), onTopic: topic)
     }
 
-    func upgrade(topic: String, permissions: Session.Permissions) {
+    func upgrade(topic: String, permissions: Session.Permissions) throws {
+        let permissions = SessionPermissions(permissions: permissions)
         guard var session = sequencesStore.getSequence(forTopic: topic) else {
-            logger.debug("Could not find session for topic \(topic)")
-            return
+            throw WalletConnectError.noSessionMatchingTopic(topic)
         }
-        session.upgrade(permissions)
-        guard let newPermissions = session.settled?.permissions else {
-            return
+        guard session.isSettled else {
+            throw WalletConnectError.sessionNotSettled(topic)
         }
-        relayer.request(.wcSessionUpgrade(SessionType.UpgradeParams(permissions: newPermissions)), onTopic: topic) { [unowned self] result in
-            switch result {
-            case .success(_):
-                sequencesStore.setSequence(session)
-                onSessionUpgrade?(session.topic, newPermissions)
-            case .failure(_):
-                return
-                //TODO
-            }
+        guard isController else {
+            throw WalletConnectError.unauthorizedNonControllerCall
+        }
+        guard validatePermissions(permissions) else {
+            throw WalletConnectError.invalidPermissions
         }
+        session.upgrade(permissions)
+        let newPermissions = session.settled!.permissions // We know session is settled
+        sequencesStore.setSequence(session)
+        relayer.request(.wcSessionUpgrade(SessionType.UpgradeParams(permissions: newPermissions)), onTopic: topic)
     }
 
     func notify(topic: String, params: Session.Notification, completion: ((Error?)->())?) {
@@ -270,7 +269,7 @@ final class SessionEngine {
             case .sessionUpdate(let updateParams):
                 handleSessionUpdate(payload: subscriptionPayload, updateParams: updateParams)
             case .sessionUpgrade(let upgradeParams):
-                handleSessionUpgrade(topic: topic, upgradeParams: upgradeParams, requestId: requestId)
+                handleSessionUpgrade(payload: subscriptionPayload, upgradeParams: upgradeParams)
             case .sessionDelete(let deleteParams):
                 handleSessionDelete(deleteParams, topic: topic)
             case .sessionPayload(let sessionPayloadParams):
@@ -338,40 +337,34 @@ final class SessionEngine {
             relayer.respondError(for: payload, reason: .unauthorizedMatchingController(isController: isController))
             return
         }
-
         session.settled?.state = updateParams.state
         sequencesStore.setSequence(session)
         relayer.respondSuccess(for: payload)
         onSessionUpdate?(topic, updateParams.state.accounts)
     }
 
-    private func handleSessionUpgrade(topic: String, upgradeParams: SessionType.UpgradeParams, requestId: Int64) {
-        guard var session = sequencesStore.getSequence(forTopic: topic) else {
-            logger.debug("Could not find session for topic \(topic)")
+    private func handleSessionUpgrade(payload: WCRequestSubscriptionPayload, upgradeParams: SessionType.UpgradeParams) {
+        guard validatePermissions(upgradeParams.permissions) else {
+            relayer.respondError(for: payload, reason: .invalidUpgradeRequest(context: .session))
             return
         }
-        guard session.peerIsController else {
-            let error = WalletConnectError.unauthrorized(.unauthorizedUpgradeRequest)
-            logger.error(error)
-            respond(error: error, requestId: requestId, topic: topic)
+        guard var session = sequencesStore.getSequence(forTopic: payload.topic) else {
+            relayer.respondError(for: payload, reason: .noContextWithTopic(context: .session, topic: payload.topic))
             return
         }
-        let permissions = Session.Permissions(
-            blockchains: upgradeParams.permissions.blockchain.chains,
-            methods: upgradeParams.permissions.jsonrpc.methods)
-        session.upgrade(permissions)
-        guard let newPermissions = session.settled?.permissions else {
+        guard session.peerIsController else {
+            relayer.respondError(for: payload, reason: .unauthorizedUpgradeRequest(context: .session))
             return
         }
-        let response = JSONRPCResponse<AnyCodable>(id: requestId, result: AnyCodable(true))
-        relayer.respond(topic: topic, response: JsonRpcResult.response(response)) { [unowned self] error in
-            if let error = error {
-                logger.error(error)
-            } else {
-                sequencesStore.setSequence(session)
-                onSessionUpgrade?(session.topic, newPermissions)
-            }
+        guard !isController else {
+            relayer.respondError(for: payload, reason: .unauthorizedMatchingController(isController: isController))
+            return
         }
+        session.upgrade(upgradeParams.permissions)
+        sequencesStore.setSequence(session)
+        let newPermissions = session.settled!.permissions // We know session is settled
+        relayer.respondSuccess(for: payload)
+        onSessionUpgrade?(session.topic, newPermissions)
     }
 
     private func handleSessionPing(topic: String, requestId: Int64) {
@@ -513,6 +506,8 @@ final class SessionEngine {
             handleApproveResponse(topic: response.topic, result: response.result)
         case .sessionUpdate:
             handleUpdateResponse(topic: response.topic, result: response.result)
+        case .sessionUpgrade:
+            handleUpgradeResponse(topic: response.topic, result: response.result)
         case .sessionPayload(_):
             let response = Response(topic: response.topic, chainId: response.chainId, result: response.result)
             onSessionPayloadResponse?(response)
@@ -576,4 +571,37 @@ final class SessionEngine {
             logger.error("Peer failed to update state.")
         }
     }
+
+    private func handleUpgradeResponse(topic: String, result: JsonRpcResult) {
+        guard let session = sequencesStore.getSequence(forTopic: topic), let permissions = session.settled?.permissions else {
+            return
+        }
+        switch result {
+        case .response:
+            onSessionUpgrade?(session.topic, permissions)
+        case .error:
+            logger.error("Peer failed to upgrade permissions.")
+        }
+    }
+
+    private func validatePermissions(_ permissions: SessionPermissions) -> Bool {
+        for chainId in permissions.blockchain.chains {
+            if !String.conformsToCAIP2(chainId) {
+                return false
+            }
+        }
+        for method in permissions.jsonrpc.methods {
+            if method.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
+                return false
+            }
+        }
+        if let notificationTypes = permissions.notifications?.types {
+            for notification in notificationTypes {
+                if notification.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
+                    return false
+                }
+            }
+        }
+        return true
+    }
 }

Sources/WalletConnect/Types/ReasonCode.swift

@@ -5,18 +5,27 @@ enum ReasonCode {
         case session = "session"
     }
 
+    // 0 (Generic)
     case generic(message: String)
+
+    // 1000 (Internal)
     case invalidUpdateRequest(context: Context)
+    case invalidUpgradeRequest(context: Context)
     case noContextWithTopic(context: Context, topic: String)
+
+    // 3000 (Unauthorized)
     case unauthorizedUpdateRequest(context: Context)
+    case unauthorizedUpgradeRequest(context: Context)
     case unauthorizedMatchingController(isController: Bool)
 
     var code: Int {
         switch self {
         case .generic: return 0
         case .invalidUpdateRequest: return 1003
+        case .invalidUpgradeRequest: return 1004
         case .noContextWithTopic: return 1301
         case .unauthorizedUpdateRequest: return 3003
+        case .unauthorizedUpgradeRequest: return 3004
         case .unauthorizedMatchingController: return 3005
         }
     }
@@ -27,10 +36,14 @@ enum ReasonCode {
             return message
         case .invalidUpdateRequest(let context):
             return "Invalid \(context) update request"
+        case .invalidUpgradeRequest(let context):
+            return "Invalid \(context) upgrade request"
         case .noContextWithTopic(let context, let topic):
             return "No matching \(context) with topic: \(topic)"
         case .unauthorizedUpdateRequest(let context):
             return "Unauthorized \(context) update request"
+        case .unauthorizedUpgradeRequest(let context):
+            return "Unauthorized \(context) upgrade request"
         case .unauthorizedMatchingController(let isController):
             return "Unauthorized: peer is also \(isController ? "" : "non-")controller"
         }

Sources/WalletConnect/Types/Session/SessionPermissions.swift

@@ -38,8 +38,8 @@ struct SessionPermissions: Codable, Equatable {
         self.controller = nil
     }
 
-    mutating func upgrade(with sessionPermissions: Session.Permissions) {
-        blockchain.chains.formUnion(sessionPermissions.blockchains)
-        jsonrpc.methods.formUnion(sessionPermissions.methods)
+    mutating func upgrade(with permissions: SessionPermissions) {
+        blockchain.chains.formUnion(permissions.blockchain.chains)
+        jsonrpc.methods.formUnion(permissions.jsonrpc.methods)
     }
 }

Sources/WalletConnect/Types/Session/SessionSequence.swift

@@ -73,7 +73,7 @@ struct SessionSequence: ExpirableSequence {
         return settled.permissions.jsonrpc.methods.contains(method)
     }
 
-    mutating func upgrade(_ permissions: Session.Permissions) {
+    mutating func upgrade(_ permissions: SessionPermissions) {
         settled?.permissions.upgrade(with: permissions)
     }
 

Sources/WalletConnect/WalletConnectClient.swift

@@ -168,8 +168,8 @@ public final class WalletConnectClient {
     /// - Parameters:
     ///   - topic: Topic of the session that is intended to be upgraded.
     ///   - permissions: Sets of permissions that will be combined with existing ones.
-    public func upgrade(topic: String, permissions: Session.Permissions) {
-        sessionEngine.upgrade(topic: topic, permissions: permissions)
+    public func upgrade(topic: String, permissions: Session.Permissions) throws {
+        try sessionEngine.upgrade(topic: topic, permissions: permissions)
     }
 
     /// For the proposer to send JSON-RPC requests to responding peer.

Sources/WalletConnect/WalletConnectError.swift

@@ -2,19 +2,18 @@
 
 import Foundation
 
+// TODO: Migrate protocol errors to ReasonCode enum over time. Use WalletConnectError for client errors only.
 enum WalletConnectError: Error {
 
-    // 1000 (Internal)
+    case noSessionMatchingTopic(String)
+    case sessionNotSettled(String)
+    case invalidPermissions
+    case unauthorizedNonControllerCall
+
     case `internal`(_ reason: InternalReason)
 
-    // 2000 (Timeout)
-    // 3000 (Unauthorized)
     case unauthrorized(_ reason: UnauthorizedReason)
 
-    // 4000 (EIP-1193)
-    // 5000 (CAIP-25)
-    // 9000 (Unknown)
-
     enum InternalReason: Error {
         case notApproved
         case malformedPairingURI
@@ -50,11 +49,21 @@ extension WalletConnectError: CustomStringConvertible {
             return reason.code
         case .unauthrorized(let reason):
             return reason.code
+        default:
+            return 0
         }
     }
 
     var localizedDescription: String {
         switch self {
+        case .noSessionMatchingTopic(let topic):
+            return "No session found matching topic \(topic)."
+        case .sessionNotSettled(let topic):
+            return "Session is not settled on topic \(topic)."
+        case .invalidPermissions:
+            return "Permission set is invalid."
+        case .unauthorizedNonControllerCall:
+            return "Method must be called by a controller client."
         case .internal(let reason):
             return reason.description
         case .unauthrorized(let reason):

Tests/IntegrationTests/ClientTest.swift

@@ -254,7 +254,7 @@ final class ClientTests: XCTestCase {
             self.responder.client.approve(proposal: proposal, accounts: [account])
         }
         responder.onSessionSettled = { [unowned self] sessionSettled in
-            responder.client.upgrade(topic: sessionSettled.topic, permissions: upgradePermissions)
+            try? responder.client.upgrade(topic: sessionSettled.topic, permissions: upgradePermissions)
         }
         proposer.onSessionUpgrade = { topic, permissions in
             XCTAssertTrue(permissions.blockchains.isSuperset(of: upgradePermissions.blockchains))
@@ -269,31 +269,6 @@ final class ClientTests: XCTestCase {
         waitForExpectations(timeout: defaultTimeout, handler: nil)
     }
 
-    func testSessionUpgradeFailsOnNonControllerRequest() {
-        let proposerSessionUpgradeExpectation = expectation(description: "Proposer upgrades session")
-        proposerSessionUpgradeExpectation.isInverted = true
-        let responderSessionUpgradeExpectation = expectation(description: "Responder upgrades session")
-        responderSessionUpgradeExpectation.isInverted = true
-        let account = "0x022c0c42a80bd19EA4cF0F94c4F9F96645759716"
-        let permissions = Session.Permissions.stub()
-        let upgradePermissions = Session.Permissions(blockchains: ["eip155:42"], methods: ["eth_sendTransaction"])
-        let uri = try! proposer.client.connect(sessionPermissions: permissions)!
-        try! responder.client.pair(uri: uri)
-        responder.onSessionProposal = { [unowned self] proposal in
-            self.responder.client.approve(proposal: proposal, accounts: [account])
-        }
-        proposer.onSessionSettled = { [unowned self] sessionSettled in
-            proposer.client.upgrade(topic: sessionSettled.topic, permissions: upgradePermissions)
-        }
-        proposer.onSessionUpgrade = { topic, permissions in
-            proposerSessionUpgradeExpectation.fulfill()
-        }
-        responder.onSessionUpgrade = { topic, permissions in
-            responderSessionUpgradeExpectation.fulfill()
-        }
-        waitForExpectations(timeout: 3.0, handler: nil)
-    }
-
     func testSuccessfulSessionUpdate() {
         let proposerSessionUpdateExpectation = expectation(description: "Proposer updates session on responder request")
         let responderSessionUpdateExpectation = expectation(description: "Responder updates session on proposer response")

Tests/WalletConnectTests/Helpers/Error+Extension.swift

@@ -7,3 +7,30 @@ extension NSError {
         NSError(domain: "com.walletconnect.sdk.tests.error", code: code, userInfo: nil)
     }
 }
+
+extension Error {
+
+    var wcError: WalletConnectError? {
+        self as? WalletConnectError
+    }
+
+    var isNoSessionMatchingTopicError: Bool {
+        guard case .noSessionMatchingTopic = wcError else { return false }
+        return true
+    }
+
+    var isSessionNotSettledError: Bool {
+        guard case .sessionNotSettled = wcError else { return false }
+        return true
+    }
+
+    var isInvalidPermissionsError: Bool {
+        guard case .invalidPermissions = wcError else { return false }
+        return true
+    }
+
+    var isUnauthorizedNonControllerCallError: Bool {
+        guard case .unauthorizedNonControllerCall = wcError else { return false }
+        return true
+    }
+}