search

status-im / team-core

Public repository for the Core team
1 stars 1 forks source link

Migration to community WebView #4

Closed vitvly closed 4 years ago

vitvly commented 4 years ago

Migration to community WebView

Introduction

Currently we rely on react-native-webview-bridge library. It is an extension of RN's WebView that injects a special script into each webpage it loads. Its raison d'etre is allowing bidirectional communication between RN and WebView.

Current status

We maintain our own fork of react-native-webview-bridge, adding fixes for web3.js support for dapps, webview caching, permissions fixes, etc. Comprehensive list is in a comparison sheet.

webview-bridgelibrary uses sendToBridge() method for RN->WebView communication and send() method for WebView->RN communication.

In status-react codebase, we use them as

Correspondingly, on WebView side there is a WebViewBridge global variable that has a send() method and a onMessage() method.

The problem

There are some issues with it, e.g.:

Why Don't We Just...?

We could go on with maintaining our webview-bridge fork, but then we'd have to roll out our own WKWebView (UIWebView replacement) support, and would skip on community updates to WebView.

Proposal

We should migrate to community WebView. There would be some slight API changes, e.g.:

Direction webview-bridge community webview
RN -> WebView WebViewBridge.sendToBridge() injectJavaScript() method or injectedJavaScript prop
RN -> WebView onMessage onMessage
WebView -> RN WebViewBridge.send() ReactNativeWebView.postMessage()
WebView -> RN onBridgeMessage onMessage

However, currently there are unsolved issues with it that prevent using community WebView as an (almost) drop-in replacement. We will maintain our own fork until these are merged upstream.

The most important is allowing for JS injection before the document load. It is required by web3.js, and previously Status team implemented their own fixes for Android and iOS.

A proposed plan is:

Stage 1

JS injection implementation. We might borrow ideas from https://github.com/react-native-community/react-native-webview/pull/229 so that:

Stage 2

Caching, cookies persistence.

Alternate route

Or, alternatively, we can extend the API that Status offers to Dapps. There is currently a StatusAPI object and a sendAPIRequest() function in provider.js. They provide endpoints for contact code fetching and QR code scanning.

There are some security issues associated with exposing more of Status internal API. These should be solved with a number of approaches, namely (credit goes to @corpetty for elaborating on this):

Stage 3

Permissions fixes + other.

Stage 4

To be elaborated on. We can expose Status chat functionality as an API to embedded WebViews. A postMessage/onMessage mechanism could be used to provide a nice, more abstract API to dapps so that they can use what Status already has as part of status-go. This will help to avoid duplicate functionality in Status itself and nested WebViews.

rasom commented 4 years ago

dependency on UIWebView

I used WKWebView there because that was the only way to inject js before page loading on iOS. So I'm not sure why we depend on UIWebView, but probably it has been changed...

upd. it is still there https://github.com/status-im/react-native-webview-bridge/blob/master/ios/RCTWKWebView.m#L55

andremedeiros commented 4 years ago

Some concerns from the Teller swarm that we should address:

hesterbruikman commented 4 years ago

The ⬅️ on top of the UI doesn't take you back, instead it "closes" the browser /cc @hesterbruikman

This is a definite pain that results from our current browser navigation. The in-browser navigation (back, forward) is actually at the bottom. Recently we updated the back button at the top to a cross to at least better communicate that it closes the tab (https://github.com/status-im/status-react/pull/8847).

The cross doesn't actually close the browser, only the tab. Improving tab-based browsing is part of window-based navigation but has lower priority compared to Chat in the Core team. If this 'kills' Teller we need to shout louder:)

GPS access would be nice to have

@andremedeiros we might need a process around API management when it comes to sharing Profile, Location or any other information the Core app has.

vitvly commented 4 years ago

@rasom UIWebView might be a transitive dependency, I'll check, thanks for pointing this out. Update: react-native-webview-bridge depends on "react-native-webview" "^6.11.1".

yenda commented 4 years ago

@andremedeiros I don't think murmur is useful here, there is already a whisper capable node in Status, no need to run the whole stack again in the browser. Instead we could provide access to whisper api with permissions or even better the higher level chat api of Status

andremedeiros commented 4 years ago

we could provide access to whisper api with permissions

@siphiuel do you think this could work? Us exposing custom APIs to dapps that can request permissions to use?

vitvly commented 4 years ago

@andremedeiros in theory yes, postMessage/onMessage mechanism should handle this, i think we could build an abstraction on top of it. Saying "in theory" because i'd love to build an mvp of it :)

corpetty commented 4 years ago

While I love the idea of updating the webview to be more seemless with the rest of Status, I worry about permissions and how well we are sanitizing injected JS. The webview is going to be (already is) the most attacked surface on the entire app. We'll have to be damn sure we don't allow full access through this.

corpetty commented 4 years ago

GPS access would be nice to have

as of now we still request android.permission.ACCESS_FINE_LOCATION in permissions (and whatever the equivalent is in iOS). The audit asked why we do this and we haven't been able to answer, yet.

Why do we want location data?

yenda commented 4 years ago

@andremedeiros @rachelhamlin a high priority issue that could be in the scope of this one, or maybe outside because it is even more urgent is the fact that subscription to events are not supported in Status right now, and that is a pretty big deal for dapps https://github.com/status-im/status-react/issues/8577

andremedeiros commented 4 years ago

Why do we want location data?

The Teller project has (had?) a feature where it would show a seller's approximate location on a map.

@iurimatias is this still a thing?

richard-ramos commented 4 years ago

In regards to this feature: Exposing the Whisper API We had a meeting today about a requirement we have in Teller Network where users should be notified when an action happens in a trade they're involved.

Two possible solutions were suggested by @yenda:

  1. Expose the whisper api thru via a one time permission to send messages from the dapp (the permission access would be requested each time the dapp wants to send a message)
  2. Enable a functionality to watch over contract events in the Status App, that will emit a notification if an event is emitted.

Of the two solutions, the first one seemed to be easiest to implement and include along with this webview change, so I'd like this feature to be priorized above the other issues mentioned here: https://github.com/status-im/team-core/issues/4#issuecomment-546985990

andremedeiros commented 4 years ago

one time permission

I'm not sure we can support this. Reason being, I can envision scenarios where dapps will share hostnames for IPFS gateways, so it's hard to come up with a rule where the permissions are remembered and assigned to the correct dapps.

I could also be missing something and would love to be proven wrong.

andremedeiros commented 4 years ago

users should be notified when an action happens in a trade they're involved

Wouldn't this only work when the app is active and dapp is open?

vitvly commented 4 years ago

Regarding web3.eth.subscribe support - some notes after call with @yenda: 1) Our web3 provider right now is completely custom, and does not support subscriptions. In order to support subscriptions, it has to either override WebsocketProvider or IpcProvider; alternatively we could implement own logic for handling onMessage (do not confuse with WebViewBridge's onMessage) and dispatching to listeners 2) Need to implement handling of subscriptions on status-react side, so that when we get a signal from status-go, we can dispatch an event to the right listener. Also we need to persist these subscriptions.

richard-ramos commented 4 years ago

users should be notified when an action happens in a trade they're involved

Wouldn't this only work when the app is active and dapp is open?

The notification would be emited by an user actively using the dapp. i.e.: If a buyer is opening an escrow, the dapp would request permissions to that user to send a whisper message to the seller. Same case if the seller is funding an escrow, a message would be sent from seller to buyer.

guylouis commented 4 years ago

For Status (browser) to be used as a point of sales, we are implementing https://github.com/status-im/status-react/issues/9275 web3.keycard.signTypedData can be called by any dApp and this pop-ups a screen requesting a keycard (not paired to the phone) to be tapped and it signs the transaction without PIN

Should this be listed here ?

cc @gravityblast

vitvly commented 4 years ago

@yenda @guylouis I'd suggest moving things related to web3 opt-in extension (like API, web3.eth.subscribe, web3.keycard.signTypedData) to another issue. Seems that they would not affect the WebView engine itself, but rather content of the scripts we're injecting. What do you think?

guylouis commented 4 years ago

@siphiuel yes, makes sense. Thanks for the explanations this morning about this btw.

yenda commented 4 years ago

@siphiuel Yes I agree as well, it should be a separate issue. Subscribing is actually more urgent as well and should be prioritized