Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EIP-1102 updates #6006

Merged
merged 2 commits into from
Jan 30, 2019
Merged

EIP-1102 updates #6006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37ce979
Update privacy notice
bitpshr Jan 9, 2019
13258ea
Respond to 1102 messages using tab ID
bitpshr Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 24 additions & 22 deletions app/scripts/controllers/provider-approval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,19 @@ class ProviderApprovalController {
})

if (platform && platform.addMessageListener) {
platform.addMessageListener(({ action = '', force, origin, siteTitle, siteImage }) => {
platform.addMessageListener(({ action = '', force, origin, siteTitle, siteImage }, { tab }) => {
switch (action) {
case 'init-provider-request':
this._handleProviderRequest(origin, siteTitle, siteImage, force)
this._handleProviderRequest(origin, siteTitle, siteImage, force, tab.id)
break
case 'init-is-approved':
this._handleIsApproved(origin)
this._handleIsApproved(origin, tab.id)
break
case 'init-is-unlocked':
this._handleIsUnlocked()
this._handleIsUnlocked(tab.id)
break
case 'init-privacy-request':
this._handlePrivacyRequest()
this._handlePrivacyRequest(tab.id)
break
}
})
Expand All @@ -53,11 +53,11 @@ class ProviderApprovalController {
* @param {string} siteTitle - The title of the document requesting full provider access
* @param {string} siteImage - The icon of the window requesting full provider access
*/
_handleProviderRequest (origin, siteTitle, siteImage, force) {
this.store.updateState({ providerRequests: [{ origin, siteTitle, siteImage }] })
_handleProviderRequest (origin, siteTitle, siteImage, force, tabID) {
this.store.updateState({ providerRequests: [{ origin, siteTitle, siteImage, tabID }] })
const isUnlocked = this.keyringController.memStore.getState().isUnlocked
if (!force && this.approvedOrigins[origin] && this.caching && isUnlocked) {
this.approveProviderRequest(origin)
this.approveProviderRequest(tabID)
return
}
this.openPopup && this.openPopup()
Expand All @@ -68,64 +68,66 @@ class ProviderApprovalController {
*
* @param {string} origin - Origin of the window
*/
_handleIsApproved (origin) {
_handleIsApproved (origin, tabID) {
this.platform && this.platform.sendMessage({
action: 'answer-is-approved',
isApproved: this.approvedOrigins[origin] && this.caching,
caching: this.caching,
}, { active: true })
}, { id: tabID })
}

/**
* Called by a tab to determine if MetaMask is currently locked or unlocked
*/
_handleIsUnlocked () {
_handleIsUnlocked (tabID) {
const isUnlocked = this.keyringController.memStore.getState().isUnlocked
this.platform && this.platform.sendMessage({ action: 'answer-is-unlocked', isUnlocked }, { active: true })
this.platform && this.platform.sendMessage({ action: 'answer-is-unlocked', isUnlocked }, { id: tabID })
}

/**
* Called to check privacy mode; if privacy mode is off, this will automatically enable the provider (legacy behavior)
*/
_handlePrivacyRequest () {
_handlePrivacyRequest (tabID) {
const privacyMode = this.preferencesController.getFeatureFlags().privacyMode
if (!privacyMode) {
this.platform && this.platform.sendMessage({
action: 'approve-legacy-provider-request',
selectedAddress: this.publicConfigStore.getState().selectedAddress,
}, { active: true })
}, { id: tabID })
this.publicConfigStore.emit('update', this.publicConfigStore.getState())
}
}

/**
* Called when a user approves access to a full Ethereum provider API
*
* @param {string} origin - Origin of the target window to approve provider access
* @param {string} tabID - ID of the target window that approved provider access
*/
approveProviderRequest (origin) {
approveProviderRequest (tabID) {
this.closePopup && this.closePopup()
const requests = this.store.getState().providerRequests
const origin = requests.find(request => request.tabID === tabID).origin
this.platform && this.platform.sendMessage({
action: 'approve-provider-request',
selectedAddress: this.publicConfigStore.getState().selectedAddress,
}, { active: true })
}, { id: tabID })
this.publicConfigStore.emit('update', this.publicConfigStore.getState())
const providerRequests = requests.filter(request => request.origin !== origin)
const providerRequests = requests.filter(request => request.tabID !== tabID)
this.store.updateState({ providerRequests })
this.approvedOrigins[origin] = true
}

/**
* Called when a tab rejects access to a full Ethereum provider API
*
* @param {string} origin - Origin of the target window to reject provider access
* @param {string} tabID - ID of the target window that rejected provider access
*/
rejectProviderRequest (origin) {
rejectProviderRequest (tabID) {
this.closePopup && this.closePopup()
const requests = this.store.getState().providerRequests
this.platform && this.platform.sendMessage({ action: 'reject-provider-request' }, { active: true })
const providerRequests = requests.filter(request => request.origin !== origin)
const origin = requests.find(request => request.tabID === tabID).origin
this.platform && this.platform.sendMessage({ action: 'reject-provider-request' }, { id: tabID })
const providerRequests = requests.filter(request => request.tabID !== tabID)
this.store.updateState({ providerRequests })
delete this.approvedOrigins[origin]
}
Expand Down
6 changes: 4 additions & 2 deletions app/scripts/platforms/extension.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@ class ExtensionPlatform {
}

sendMessage (message, query = {}) {
extension.tabs.query(query, tabs => {
const id = query.id
delete query.id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we deleting the id from the query here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Querying tabs worked oddly in Chrome and Firefox. If the id was not deleted before querying, no tabs would ever be returned. This is why this roundabout method of sending to a specific ID is used: if an id is present on a query, we cache it, delete it, run the rest of the query, then only send the resulting message to tabs matching the cached id.

extension.tabs.query({ ...query }, tabs => {
tabs.forEach(tab => {
extension.tabs.sendMessage(tab.id, message)
extension.tabs.sendMessage(id || tab.id, message)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the id is passed in and we're querying all the tabs, will this send the same message to the same tab multiple times?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be sending a message to tab id if that's passed in, else iterate all the tabs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like a query with multiple properties should hit tabs that satisfy all properties, not just some. This is why we 1) query without the ID, then 2) iterate through the results and further filter by ID. If we detected ID first then bailed, that implies an ID takes precedence over all other query properties.

Copy link
Contributor

@whymarrh whymarrh Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe I don't understand what these APIs are doing well enough. If we call platform.sendMessage(foo, 42), we'll in turn be calling extension.tabs.sendMessage(42, foo) multiple times (once for each tab returned in the query), no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whymarrh's concern made sense to me, and then I tried to word it and understood Paul's point:

The tabs.query method does not take an id parameter and filter for it, and so this basically allows our own platform.sendMessage() method to accept an id in its query, and then only send to that tab.

})
})
}
Expand Down
4 changes: 2 additions & 2 deletions notices/archive/notice_2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
MetaMask is beta software.

When you log in to MetaMask, your current account's address is visible to every new site you visit. This can be used to look up your account balances of Ether and other tokens.
When you log in to MetaMask and approve account access, your current account's address is visible to the site you're currently viewing. This can be used to look up your account balances of Ether and other tokens.

For your privacy, for now, please sign out of MetaMask when you're done using a site.
For your privacy, take caution when approving account access and sign out of MetaMask when you're done using a site.

13 changes: 7 additions & 6 deletions old-ui/app/provider-approval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { approveProviderRequest, rejectProviderRequest } from '../../ui/app/acti
import { connect } from 'react-redux'
class ProviderApproval extends Component {
render () {
const { approveProviderRequest, origin, rejectProviderRequest } = this.props
const { approveProviderRequest, origin, tabID, rejectProviderRequest } = this.props
return (
<div className="flex-column flex-grow">
<style dangerouslySetInnerHTML={{__html: `
Expand All @@ -28,7 +28,7 @@ class ProviderApproval extends Component {
<div className="section-title flex-row flex-center">
<i
className="fa fa-arrow-left fa-lg cursor-pointer"
onClick={() => { rejectProviderRequest(origin) }} />
onClick={() => { rejectProviderRequest(tabID) }} />
<h2 className="page-subtitle">Web3 API Request</h2>
</div>
<div className="provider_approval_content">
Expand All @@ -38,10 +38,10 @@ class ProviderApproval extends Component {
<div className="provider_approval_actions">
<button
className="btn-green"
onClick={() => { approveProviderRequest(origin) }}>APPROVE</button>
onClick={() => { approveProviderRequest(tabID) }}>APPROVE</button>
<button
className="cancel btn-red"
onClick={() => { rejectProviderRequest(origin) }}>REJECT</button>
onClick={() => { rejectProviderRequest(tabID) }}>REJECT</button>
</div>
</div>
)
Expand All @@ -51,13 +51,14 @@ class ProviderApproval extends Component {
ProviderApproval.propTypes = {
approveProviderRequest: PropTypes.func,
origin: PropTypes.string,
tabID: PropTypes.string,
rejectProviderRequest: PropTypes.func,
}

function mapDispatchToProps (dispatch) {
return {
approveProviderRequest: origin => dispatch(approveProviderRequest(origin)),
rejectProviderRequest: origin => dispatch(rejectProviderRequest(origin)),
approveProviderRequest: tabID => dispatch(approveProviderRequest(tabID)),
rejectProviderRequest: tabID => dispatch(rejectProviderRequest(tabID)),
}
}

Expand Down
8 changes: 4 additions & 4 deletions ui/app/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2625,15 +2625,15 @@ function setPendingTokens (pendingTokens) {
}
}

function approveProviderRequest (origin) {
function approveProviderRequest (tabID) {
return (dispatch) => {
background.approveProviderRequest(origin)
background.approveProviderRequest(tabID)
}
}

function rejectProviderRequest (origin) {
function rejectProviderRequest (tabID) {
return (dispatch) => {
background.rejectProviderRequest(origin)
background.rejectProviderRequest(tabID)
}
}

Expand Down
1 change: 1 addition & 0 deletions ui/app/components/pages/provider-approval/provider-approval.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export default class ProviderApproval extends Component {
<ProviderPageContainer
approveProviderRequest={approveProviderRequest}
origin={providerRequest.origin}
tabID={providerRequest.tabID}
rejectProviderRequest={rejectProviderRequest}
siteImage={providerRequest.siteImage}
siteTitle={providerRequest.siteTitle}
Expand Down
4 changes: 2 additions & 2 deletions ui/app/components/pages/provider-approval/provider-approval.container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import { approveProviderRequest, rejectProviderRequest } from '../../../actions'

function mapDispatchToProps (dispatch) {
return {
approveProviderRequest: origin => dispatch(approveProviderRequest(origin)),
rejectProviderRequest: origin => dispatch(rejectProviderRequest(origin)),
approveProviderRequest: tabID => dispatch(approveProviderRequest(tabID)),
rejectProviderRequest: tabID => dispatch(rejectProviderRequest(tabID)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would normally raise a concern about mixing platform-specific details (like tab ID) in with the UI, but I have a proposal for a revised approach to this I want to share with you soon (maybe tomorrow?), that I think will help clean up a lot of this tab management logic.

}
}

Expand Down
9 changes: 5 additions & 4 deletions ui/app/components/provider-page-container/provider-page-container.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,21 @@ export default class ProviderPageContainer extends PureComponent {
rejectProviderRequest: PropTypes.func.isRequired,
siteImage: PropTypes.string,
siteTitle: PropTypes.string.isRequired,
tabID: PropTypes.string.isRequired,
};

static contextTypes = {
t: PropTypes.func,
};

onCancel = () => {
const { origin, rejectProviderRequest } = this.props
rejectProviderRequest(origin)
const { tabID, rejectProviderRequest } = this.props
rejectProviderRequest(tabID)
}

onSubmit = () => {
const { approveProviderRequest, origin } = this.props
approveProviderRequest(origin)
const { approveProviderRequest, tabID } = this.props
approveProviderRequest(tabID)
}

render () {
Expand Down