diff --git a/Sources/WalletConnect/Engine/SessionEngine.swift b/Sources/WalletConnect/Engine/SessionEngine.swift index f5233d034..529fc5810 100644 --- a/Sources/WalletConnect/Engine/SessionEngine.swift +++ b/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(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 + } } diff --git a/Sources/WalletConnect/Types/ReasonCode.swift b/Sources/WalletConnect/Types/ReasonCode.swift index f253fe07d..187095d47 100644 --- a/Sources/WalletConnect/Types/ReasonCode.swift +++ b/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" } diff --git a/Sources/WalletConnect/Types/Session/SessionPermissions.swift b/Sources/WalletConnect/Types/Session/SessionPermissions.swift index dc65d9ef1..6f19280b1 100644 --- a/Sources/WalletConnect/Types/Session/SessionPermissions.swift +++ b/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) } } diff --git a/Sources/WalletConnect/Types/Session/SessionSequence.swift b/Sources/WalletConnect/Types/Session/SessionSequence.swift index f09dec768..b54cf5158 100644 --- a/Sources/WalletConnect/Types/Session/SessionSequence.swift +++ b/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) } diff --git a/Sources/WalletConnect/WalletConnectClient.swift b/Sources/WalletConnect/WalletConnectClient.swift index c79d0097e..ddb239ff8 100644 --- a/Sources/WalletConnect/WalletConnectClient.swift +++ b/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. diff --git a/Sources/WalletConnect/WalletConnectError.swift b/Sources/WalletConnect/WalletConnectError.swift index a05eb5a06..67ce2bd7a 100644 --- a/Sources/WalletConnect/WalletConnectError.swift +++ b/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): diff --git a/Tests/IntegrationTests/ClientTest.swift b/Tests/IntegrationTests/ClientTest.swift index 4b76faf67..ecf9a5a83 100644 --- a/Tests/IntegrationTests/ClientTest.swift +++ b/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") diff --git a/Tests/WalletConnectTests/Helpers/Error+Extension.swift b/Tests/WalletConnectTests/Helpers/Error+Extension.swift index d21783353..25845a8cb 100644 --- a/Tests/WalletConnectTests/Helpers/Error+Extension.swift +++ b/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 + } +} diff --git a/Tests/WalletConnectTests/SessionEngineTests.swift b/Tests/WalletConnectTests/SessionEngineTests.swift index 5ac490c45..9c480ad1a 100644 --- a/Tests/WalletConnectTests/SessionEngineTests.swift +++ b/Tests/WalletConnectTests/SessionEngineTests.swift @@ -352,4 +352,118 @@ final class SessionEngineTests: XCTestCase { } // TODO: Update acknowledgement tests + + // MARK: - Upgrade call tests + + func testUpgradeSuccess() throws { + setupEngine(isController: true) + let permissions = Session.Permissions.stub() + let session = SessionSequence.stubSettled() + storageMock.setSequence(session) + try engine.upgrade(topic: session.topic, permissions: permissions) + XCTAssertTrue(relayMock.didCallRequest) + // TODO: Check permissions on stored session + } + + func testUpgradeErrorSessionNotFound() { + setupEngine(isController: true) + XCTAssertThrowsError(try engine.upgrade(topic: "", permissions: Session.Permissions.stub())) { error in + XCTAssertTrue(error.isNoSessionMatchingTopicError) + } + } + + func testUpgradeErrorSessionNotSettled() { + setupEngine(isController: true) + let session = SessionSequence.stubPreSettled() + storageMock.setSequence(session) + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub())) { error in + XCTAssertTrue(error.isSessionNotSettledError) + } + } + + func testUpgradeErrorInvalidPermissions() { + setupEngine(isController: true) + let session = SessionSequence.stubSettled() + storageMock.setSequence(session) + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub(chains: [""]))) { error in + XCTAssertTrue(error.isInvalidPermissionsError) + } + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub(methods: [""]))) { error in + XCTAssertTrue(error.isInvalidPermissionsError) + } + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub(notifications: [""]))) { error in + XCTAssertTrue(error.isInvalidPermissionsError) + } + } + + func testUpgradeErrorCalledByNonController() { + setupEngine(isController: false) + let session = SessionSequence.stubSettled() + storageMock.setSequence(session) + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub())) { error in + XCTAssertTrue(error.isUnauthorizedNonControllerCallError) + } + } + + // MARK: - Upgrade peer response tests + + func testUpgradePeerSuccess() { + setupEngine(isController: false) + var didCallbackUpgrade = false + let session = SessionSequence.stubSettled(isPeerController: true) + storageMock.setSequence(session) + engine.onSessionUpgrade = { topic, _ in + didCallbackUpgrade = true + XCTAssertEqual(topic, session.topic) + } + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic)) + XCTAssertTrue(didCallbackUpgrade) + XCTAssertTrue(relayMock.didRespondSuccess) + } + + func testUpgradePeerErrorInvalidPermissions() { + setupEngine(isController: false) + let invalidPermissions = SessionPermissions.stub(chains: [""]) + let session = SessionSequence.stubSettled(isPeerController: true) + storageMock.setSequence(session) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic, permissions: invalidPermissions)) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 1004) + } + + func testUpgradePeerErrorSessionNotFound() { + setupEngine(isController: false) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: "")) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 1301) + } + + func testUpgradePeerErrorSessionNotSettled() { + setupEngine(isController: false) + let session = SessionSequence.stubPreSettled(isPeerController: true) // Session is not fully settled + storageMock.setSequence(session) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic)) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 3004) + } + + func testUpgradePeerErrorUnauthorized() { + setupEngine(isController: false) + let session = SessionSequence.stubSettled() // Peer is not a controller + storageMock.setSequence(session) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic)) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 3004) + } + + func testUpgradePeerErrorMatchingController() { + setupEngine(isController: true) // Upgrade request received by a controller + let session = SessionSequence.stubSettled(isPeerController: true) + storageMock.setSequence(session) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic)) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 3005) + } + + // TODO: Upgrade acknowledgement tests } diff --git a/Tests/WalletConnectTests/Stub/Stubs.swift b/Tests/WalletConnectTests/Stub/Stubs.swift index d4d281ad3..11b405f85 100644 --- a/Tests/WalletConnectTests/Stub/Stubs.swift +++ b/Tests/WalletConnectTests/Stub/Stubs.swift @@ -17,12 +17,31 @@ extension Pairing { } } +extension Session.Permissions { + static func stub( + chains: Set = ["solana:4sGjMW1sUnHzSxGspuhpqLDx6wiyjNtZ"], + methods: Set = ["getGenesisHash"], + notifications: [String] = ["msg"] + ) -> Session.Permissions { + Session.Permissions( + blockchains: chains, + methods: methods, + notifications: notifications + ) + } +} + extension SessionPermissions { - static func stub(controllerKey: String = AgreementPrivateKey().publicKey.hexRepresentation) -> SessionPermissions { - SessionPermissions( - blockchain: Blockchain(chains: []), - jsonrpc: JSONRPC(methods: []), - notifications: Notifications(types: []), + static func stub( + chains: Set = ["eip155:1"], + jsonrpc: Set = ["eth_sign"], + notifications: [String] = ["a_type"], + controllerKey: String = AgreementPrivateKey().publicKey.hexRepresentation + ) -> SessionPermissions { + return SessionPermissions( + blockchain: Blockchain(chains: chains), + jsonrpc: JSONRPC(methods: jsonrpc), + notifications: Notifications(types: notifications), controller: Controller(publicKey: controllerKey) ) } @@ -45,4 +64,9 @@ extension WCRequestSubscriptionPayload { let updateMethod = WCMethod.wcSessionUpdate(SessionType.UpdateParams(accounts: accounts)).asRequest() return WCRequestSubscriptionPayload(topic: topic, wcRequest: updateMethod) } + + static func stubUpgrade(topic: String, permissions: SessionPermissions = SessionPermissions(permissions: Session.Permissions.stub())) -> WCRequestSubscriptionPayload { + let upgradeMethod = WCMethod.wcSessionUpgrade(SessionType.UpgradeParams(permissions: permissions)).asRequest() + return WCRequestSubscriptionPayload(topic: topic, wcRequest: upgradeMethod) + } }