From 35c827e9d16096a4791379471c46b205776d6761 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Vants?= Date: Tue, 24 May 2022 10:29:03 -0300 Subject: [PATCH] #220 Permission validation for requests and events (#221) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add permission checks for requests * Add permission checks for event emissions * Add tests for basic permission validation * Fixed permission validation with test cases * Improve session permission tests Co-authored-by: André Vants --- .../WalletConnectAuth/Auth/AuthClient.swift | 2 +- .../Engine/Common/SessionEngine.swift | 31 ++-- .../Types/Session/WCSession.swift | 73 ++++----- .../WalletConnectError.swift | 3 + Tests/WalletConnectTests/WCSessionTests.swift | 139 ++++++++++++++++++ 5 files changed, 198 insertions(+), 50 deletions(-) create mode 100644 Tests/WalletConnectTests/WCSessionTests.swift diff --git a/Sources/WalletConnectAuth/Auth/AuthClient.swift b/Sources/WalletConnectAuth/Auth/AuthClient.swift index 3490d2ea1..4ac751d85 100644 --- a/Sources/WalletConnectAuth/Auth/AuthClient.swift +++ b/Sources/WalletConnectAuth/Auth/AuthClient.swift @@ -190,7 +190,7 @@ public final class AuthClient { /// - Parameters: /// - params: Parameters defining request and related session public func request(params: Request) async throws { - try await sessionEngine.request(params: params) + try await sessionEngine.request(params) } /// For the responder to respond on pending peer's session JSON-RPC Request diff --git a/Sources/WalletConnectAuth/Engine/Common/SessionEngine.swift b/Sources/WalletConnectAuth/Engine/Common/SessionEngine.swift index 36741fee9..5cd7d97a9 100644 --- a/Sources/WalletConnectAuth/Engine/Common/SessionEngine.swift +++ b/Sources/WalletConnectAuth/Engine/Common/SessionEngine.swift @@ -108,15 +108,18 @@ final class SessionEngine { } } - func request(params: Request) async throws { - print("will request on session topic: \(params.topic)") - guard sessionStore.hasSession(forTopic: params.topic) else { - logger.debug("Could not find session for topic \(params.topic)") - return + func request(_ request: Request) async throws { + print("will request on session topic: \(request.topic)") + guard let session = sessionStore.getSession(forTopic: request.topic), session.acknowledged else { + logger.debug("Could not find session for topic \(request.topic)") + return // TODO: Marked to review on developer facing error cases + } + guard session.hasPermission(forMethod: request.method, onChain: request.chainId) else { + throw WalletConnectError.invalidPermissions } - let request = SessionType.RequestParams.Request(method: params.method, params: params.params) - let sessionRequestParams = SessionType.RequestParams(request: request, chainId: params.chainId) - try await networkingInteractor.request(.wcSessionRequest(sessionRequestParams), onTopic: params.topic) + let chainRequest = SessionType.RequestParams.Request(method: request.method, params: request.params) + let sessionRequestParams = SessionType.RequestParams(request: chainRequest, chainId: request.chainId) + try await networkingInteractor.request(.wcSessionRequest(sessionRequestParams), onTopic: request.topic) } func respondSessionRequest(topic: String, response: JsonRpcResult) { @@ -138,10 +141,10 @@ final class SessionEngine { logger.debug("Could not find session for topic \(topic)") return } - let params = SessionType.EventParams(event: event, chainId: chainId) - guard session.hasNamespace(for: chainId, event: event.name) else { + guard session.hasPermission(forEvent: event.name, onChain: chainId) else { throw WalletConnectError.invalidEvent } + let params = SessionType.EventParams(event: event, chainId: chainId) try await networkingInteractor.request(.wcSessionEvent(params), onTopic: topic) } @@ -235,7 +238,7 @@ final class SessionEngine { networkingInteractor.respondError(for: payload, reason: .unauthorizedTargetChain(chain.absoluteString)) return } - guard session.hasNamespace(for: chain, method: request.method) else { + guard session.hasPermission(forMethod: request.method, onChain: chain) else { networkingInteractor.respondError(for: payload, reason: .unauthorizedMethod(request.method)) return } @@ -253,8 +256,10 @@ final class SessionEngine { networkingInteractor.respondError(for: payload, reason: .noContextWithTopic(context: .session, topic: payload.topic)) return } - guard session.peerIsController, - session.hasNamespace(for: eventParams.chainId, event: event.name) else { + guard + session.peerIsController, + session.hasPermission(forEvent: event.name, onChain: eventParams.chainId) + else { networkingInteractor.respondError(for: payload, reason: .unauthorizedEvent(event.name)) return } diff --git a/Sources/WalletConnectAuth/Types/Session/WCSession.swift b/Sources/WalletConnectAuth/Types/Session/WCSession.swift index f2c528178..05f6e3803 100644 --- a/Sources/WalletConnectAuth/Types/Session/WCSession.swift +++ b/Sources/WalletConnectAuth/Types/Session/WCSession.swift @@ -62,47 +62,48 @@ struct WCSession: ExpirableSequence { return controller.publicKey == peerParticipant.publicKey } - // FIXME func hasNamespace(for chain: Blockchain) -> Bool { - // TODO -// namespaces.contains{$0.chains.contains(chain)} - return true + return namespaces[chain.namespace] != nil } - // TODO: Remove optional for chain param, it's required now / protocol change - func hasNamespace(for chain: Blockchain?, method: String) -> Bool { - // TODO -// if let chain = chain { -// let namespacesIncludingChain = namespaces.filter{$0.chains.contains(chain)} -// let methods = namespacesIncludingChain.flatMap{$0.methods} -// return methods.contains(method) -// } else { -// return namespaces -// .filter { $0.chains.isEmpty } -// .flatMap { $0.methods } -// .contains(method) -// } - return true + func hasPermission(forMethod method: String, onChain chain: Blockchain) -> Bool { + if let namespace = namespaces[chain.namespace] { + if namespace.accounts.contains(where: { $0.blockchain == chain }) { + if namespace.methods.contains(method) { + return true + } + if let extensions = namespace.extensions { + for extended in extensions { + if extended.accounts.contains(where: { $0.blockchain == chain }) { + if extended.methods.contains(method) { + return true + } + } + } + } + } + } + return false } - func hasNamespace(for chain: Blockchain?, event: String) -> Bool { - // TODO -// if let chain = chain { -// if let namespace = namespaces.first(where: {$0.chains.contains(chain)}), -// namespace.events.contains(event) { -// return true -// } else { -// return false -// } -// } else { -// if let namespace = namespaces.first(where: {$0.chains.isEmpty}), -// namespace.events.contains(event) { -// return true -// } else { -// return false -// } -// } - return true + func hasPermission(forEvent event: String, onChain chain: Blockchain) -> Bool { + if let namespace = namespaces[chain.namespace] { + if namespace.accounts.contains(where: { $0.blockchain == chain }) { + if namespace.events.contains(event) { + return true + } + if let extensions = namespace.extensions { + for extended in extensions { + if extended.accounts.contains(where: { $0.blockchain == chain }) { + if extended.events.contains(event) { + return true + } + } + } + } + } + } + return false } mutating func updateNamespaces(_ namespaces: [String: SessionNamespace]) { diff --git a/Sources/WalletConnectAuth/WalletConnectError.swift b/Sources/WalletConnectAuth/WalletConnectError.swift index 9e35fd760..35e488a55 100644 --- a/Sources/WalletConnectAuth/WalletConnectError.swift +++ b/Sources/WalletConnectAuth/WalletConnectError.swift @@ -15,6 +15,7 @@ enum WalletConnectError: Error { case pairingAlreadyExist case topicGenerationFailed case invalidNamespaceMatch // TODO: Refactor into actual cases + case invalidPermissions // TODO: Same case `internal`(_ reason: InternalReason) @@ -58,6 +59,8 @@ extension WalletConnectError { return "Pairing already exist" case .invalidNamespaceMatch: return "Invalid namespace approval." + case .invalidPermissions: + return "Invalid permissions for call." case .internal(_): // TODO: Remove internal case return "" } diff --git a/Tests/WalletConnectTests/WCSessionTests.swift b/Tests/WalletConnectTests/WCSessionTests.swift new file mode 100644 index 000000000..27598cd39 --- /dev/null +++ b/Tests/WalletConnectTests/WCSessionTests.swift @@ -0,0 +1,139 @@ +import XCTest +@testable import WalletConnectAuth + +final class WCSessionTests: XCTestCase { + + let ethAccount = Account("eip155:1:0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb")! + let polyAccount = Account("eip155:137:0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb")! + let cosmosAccount = Account("cosmos:cosmoshub-4:cosmos1t2uflqwqe0fsj0shcfkrvpukewcw40yjj6hdc0")! + + func testHasPermissionForMethod() { + let namespace = [ + "eip155": SessionNamespace( + accounts: [ethAccount], + methods: ["method"], + events: [], + extensions: nil) + ] + var session = WCSession.stub() + session.updateNamespaces(namespace) + XCTAssertTrue(session.hasPermission(forMethod: "method", onChain: ethAccount.blockchain)) + } + + func testHasPermissionForMethodInExtension() { + let namespace = [ + "eip155": SessionNamespace( + accounts: [ethAccount], + methods: [], + events: [], + extensions: [ + SessionNamespace.Extension( + accounts: [ethAccount], + methods: ["method"], + events: [])]) + ] + var session = WCSession.stub() + session.updateNamespaces(namespace) + XCTAssertTrue(session.hasPermission(forMethod: "method", onChain: ethAccount.blockchain)) + } + + func testDenyPermissionForMethodInOtherChain() { + let namespace = [ + "eip155": SessionNamespace( + accounts: [ethAccount], + methods: [], + events: [], + extensions: nil), + "cosmos": SessionNamespace( + accounts: [cosmosAccount], + methods: ["method"], + events: [], + extensions: nil), + ] + var session = WCSession.stub() + session.updateNamespaces(namespace) + XCTAssertFalse(session.hasPermission(forMethod: "method", onChain: ethAccount.blockchain)) + } + + func testDenyPermissionForMethodInOtherChainExtension() { + let namespace = [ + "eip155": SessionNamespace( + accounts: [ethAccount, polyAccount], + methods: [], + events: [], + extensions: [ + SessionNamespace.Extension( + accounts: [polyAccount], + methods: ["method"], + events: [])]) + ] + var session = WCSession.stub() + session.updateNamespaces(namespace) + XCTAssertFalse(session.hasPermission(forMethod: "method", onChain: ethAccount.blockchain)) + } + + func testHasPermissionForEvent() { + let namespace = [ + "eip155": SessionNamespace( + accounts: [ethAccount], + methods: [], + events: ["event"], + extensions: nil) + ] + var session = WCSession.stub() + session.updateNamespaces(namespace) + XCTAssertTrue(session.hasPermission(forEvent: "event", onChain: ethAccount.blockchain)) + } + + func testHasPermissionForEventInExtension() { + let namespace = [ + "eip155": SessionNamespace( + accounts: [ethAccount], + methods: [], + events: [], + extensions: [ + SessionNamespace.Extension( + accounts: [ethAccount], + methods: [], + events: ["event"])]) + ] + var session = WCSession.stub() + session.updateNamespaces(namespace) + XCTAssertTrue(session.hasPermission(forEvent: "event", onChain: ethAccount.blockchain)) + } + + func testDenyPermissionForEventInOtherChain() { + let namespace = [ + "eip155": SessionNamespace( + accounts: [ethAccount], + methods: [], + events: [], + extensions: nil), + "cosmos": SessionNamespace( + accounts: [cosmosAccount], + methods: [], + events: ["event"], + extensions: nil), + ] + var session = WCSession.stub() + session.updateNamespaces(namespace) + XCTAssertFalse(session.hasPermission(forEvent: "event", onChain: ethAccount.blockchain)) + } + + func testDenyPermissionForEventInOtherChainExtension() { + let namespace = [ + "eip155": SessionNamespace( + accounts: [ethAccount, polyAccount], + methods: [], + events: [], + extensions: [ + SessionNamespace.Extension( + accounts: [polyAccount], + methods: [], + events: ["event"])]) + ] + var session = WCSession.stub() + session.updateNamespaces(namespace) + XCTAssertFalse(session.hasPermission(forEvent: "event", onChain: ethAccount.blockchain)) + } +}