#220 Permission validation for requests and events #221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llbartekll
requested changes
May 20, 2022
| // .contains(method) | ||
| // } | ||
| return true | ||
| func hasPermission(forMethod method: String, onChain chain: Blockchain) -> Bool { |
There was a problem hiding this comment.
should we have unit tests for that?
Comment on lines
75
to
80
| } else if let extensions = namespace.extensions { | ||
| for extended in extensions { | ||
| if extended.accounts.contains(where: { $0.blockchain == chain }) { | ||
| if extended.methods.contains(method) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
I am not sure it is correct, are you?
I think that if namespace.acconts has not a specific chain, extension will not have as well 🤔
lets unit test this
There was a problem hiding this comment.
Implementations fixed to follow the namespace specs
flypaper0
reviewed
May 23, 2022
| 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)") |
There was a problem hiding this comment.
What the difference between use cases of print and logger?
There was a problem hiding this comment.
logger is meant to give a better control over logging categories when running the client, and replace print. But it still needs some improvements, overall.
flypaper0
reviewed
May 23, 2022
| // } | ||
| return true | ||
| func hasPermission(forMethod method: String, onChain chain: Blockchain) -> Bool { | ||
| if let namespace = namespaces[chain.namespace] { |
There was a problem hiding this comment.
Could we reused that code with (line 88) somehow? Look's lite pretty identical. For example we could create private extension on Set<String>
There was a problem hiding this comment.
I thought that it may be possible with KeyPaths, by making a single function that receives the KeyPath (\.methods or \.events) and search using it. But I decided to keep it as it is until we're sure that there will be no more changes in here for now.
We can definitely improve it later.
llbartekll
approved these changes
May 24, 2022
flypaper0
approved these changes
May 24, 2022
llbartekll
added a commit
that referenced
this pull request
May 25, 2022
* add update methods tests * add updateEvents to ControllerSessionStateMachine * add handleUpdateEventsResponse * Add expiry date concept test case * Add inactive pairing expiry tests * Change pairing creation pattern * Add pairing extension tests * Add pairing activation test * Activate pairing created from URI * Update Tests/WalletConnectTests/Helpers/Error+Extension.swift Co-authored-by: André Vants <MisterVants@users.noreply.github.com> * build fix * refactor ControllerSessionStateMachine * add onSessionUpdateEventsRequest handling * add session update events integration test * add NonControllerSessionStateMachine unit tests * Add ControllerSessionStateMachineTests events tests * Change test case name * Fix build * Renamed PairingSequence to WCPairing * Renamed SessionSequence to WCSession * Adds Commons and Toolbox packages * update session proposal data model * update public proposal data model * update methods with protocol requirements * update account params * remove comment * Removing sequence name from storage variables names * Dropping sequence term from storage protocols signature * Renamed sequence params in storage mocks * Fixes Bad access when trying to modify subscriptions #156 * migrate update methods to state machines * rename notifications to events * move update accounts to state machines * savepoint * update client delegate * Add integration test for expiry * simplify method * Fix example wallet build * Fix example dapp build * Renamed responder VC to wallet VC * Revert "Adds Commons and Toolbox packages" This reverts commit 8b9ce2b. * Update swift.yml * Update swift.yml * refactor pairing model * refactor session model * update gitignore * savepoint * add session to pairing topic storage * Update gitignore * Update Gemfile.lock * Update fix gitignore * Workaround a double precision bug in AnyCodable tests * Fix AnyCodable double test w/ workaround * Re-enable app build actions * Disable app actions test * Re-enable test wallet build step * Test build example with xcodebuild * Fix Xcode version * Change action command place * Change xcodebuild params * Try running with destination param * Test without specifying the platform * Try using custom simulator * Fix Xcode version in path * Try adding 13.1 runtime * Test with device type param * Try listing device types * Revert version workaround, running generic build * update session delete params, change propose response body * Update swift.yml * remove update pairing metadata method from pairing engine * Fixed conflict in CI file * remove subscriber from pairing engine * fix pairing engine tests * remove subscriber from session engine * build session engine tests * remove wcsubscriber implementation * Fixes blurry QR code when running on device * rename notify to emit * remove duplicated subscriber from walletconnect relayer * add methods and events params to client's approve method * Update docs * fix build * fix build * fix crash * savepoint * Updated project structure * Renamed session proposal view & controller * Formatted some code * savepoint - namespaces check * build auth package after protocol change * savepoint * remove blockchains from proposal, refactor tests * compile test target * fix integration tests * disable automatic sample apps builds * Add empty namespace test cases * Add remaining request authorization tests * Fixed implementation to comply with test cases * Refactored request stubbing on tests * Declared namespace members as public * Fixed example wallet build * Fixed dapp build * Re-enable app build workflows * Fixed integration tests * savepoint * savepoint * Approve with proposal id * fix wallet build * remove duplicated client delegate extension method * Add SwiftUI previews extension for UIKit views * Add previews for session proposal and details * Add preview to request view * SwiftUI experiment with proposal view * Redesigned view to show namespaces data * Moved Proposal type to another file * Using async image to load icon * Refactored proposal view controller * Refactor proposal view code * savepoint * add new publish method to relayer * Adds Relayer async publish method * savepoint * fix fing integration tests * distinguish three separate wc relaying reguest methods: -request -requestNetworkAck -requestPeerResponse * Rename WalletConnectRelay to NetworkingInteractor * refactor networking interactor async method * refactor client's disconnect method to be async * turn off dispatching retry fix example apps after client interface changes * fix tests * Small tweaks to example wallet * fix wallet to not operate on mocked data * build package with starscream delete urlsession * fix relay client tests remove unused classes * add logs, integration tests wait clients connected * fix example apps * add socket connection status * add diddisconnect delegate method * fix unit tsts * Fix integration tests issue * fix delete session issue when no internet connection * Fix tests * Update project structure * Removed optional from event chainId * Removed optional from request chainId * Fixed integration test case * remove unused code * savepoint * add pair method engine actor * update client's connect to async * Added validation checks for empty namespaces * Fixed build * savepoint * fix client's connect method, uncomment integration tests * restructure project * add Concurrency to emit method * savepoint * fix dapp build * fix deeplinking * remove commented code * clean up * Add split namespaces * Changed proposal schema with required namespaces * Change client API to use namespace dict for approval * Remove update accounts method * Updated wc methods signatures * Fixed test build * Removed update accounts commented code * Changed update call to new specs * Disable CI app build * rename WalletConnectClient to AuthClient * Rename Relayer to Relay Client * rename auth and relay packages * Fix package issues after renaming * Add validation test cases * Implemented namespace validation * Removed previous namespace code * Removed previous namespace code * Added peer validation matching test cases * Implemented namespace peer validation * Added approver side namespace checks * Added proposer side namespace checks on approval * Removed leftover unnecessary check * remove code examples * add Concurrency to extend and update functions * fix tests, add XCTAssertNoThrowAsync * Moved namespace validation when proposal is received * Remove unnecessary todo * Merge callbacks on proposal response * map proposed namespaces to session namespaces * fix wallet build * remove session state validating and namespace type * fix dapp build * reenable app builds * fix swift.yml * fix tests * Setting proposal to nil after using it on settlement * tvOS support * #219 auth wrapper (#222) * Add Auth singleton * savepoint * Add combine publishers * savepoint * change build number * add socket connection status publisher * fix sample apps builds * savepoiint * savepoint * add Auth to sample dapp * remove client delegate * fix dapp with Auth consumption * subscribe main scheduler * add all delegate methods to Auth * Add missing Auth publishers * restructure project * Add Auth to the sample wallet * add connect and disconnect methods to Auth * fix tests * adjust style * remove logger from Auth * fix build * #220 Permission validation for requests and events (#221) * 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 * remove getAcknowledgedSessions (#228) * remove getAcknowledgedSessions * rename method * fix Auth connect method (#229) * remove getAcknowledgedSessions * rename method * fix connect() bug * Update namespaces (#230) * remove getAcknowledgedSessions * rename method * fix connect() bug * update namespaces on controller request * turn off logger * [Wallet] Edit session namespaces (#231) * Edit session namespaces UI * Update namespace error handling * Pending requests * Show SessionRequest Screen * Reload SessionDetails on request respond * Unused import removed * Hot fix dapp personal sign (#234) use session account for personal_sign request * #226 Rename Auth package to Sign (#232) * Renamed package and base directories * Renamed imports to WalletConnectSign * Renamed client and delegate * Renamed Auth type to Sign * Fixed a log typo * Removed auth reference from build scheme Co-authored-by: André Vants * rename update expiry to extend on Sign publishers (#237) rename update expiry to extend Co-authored-by: André Vants <andrevants@icloud.com> Co-authored-by: André Vants <MisterVants@users.noreply.github.com> Co-authored-by: Krypto Pank <krypto.pank@gmail.com> Co-authored-by: Artur Guseinov <arthur.guseinov@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #220
Re-enables permission checks when sending / receiving session requests and emitting events.