Merge pull request #5689 from MetaMask/reject-cached-approval

EIP-1102: Clear approvals on rejection

4 files changed, 33 insertions, 13 deletions

diff --git a/app/scripts/contentscript.js b/app/scripts/contentscript.js
index 2327dc4ac..1a10cdb34 100644
--- a/app/scripts/contentscript.js
+++ b/app/scripts/contentscript.js
@@ -37,8 +37,10 @@ function injectScript (content) {
   try {
     const container = document.head || document.documentElement
     const scriptTag = document.createElement('script')
+    scriptTag.setAttribute('async', false)
     scriptTag.textContent = content
     container.insertBefore(scriptTag, container.children[0])
+    container.removeChild(scriptTag)
   } catch (e) {
     console.error('MetaMask script injection failed', e)
   }
diff --git a/app/scripts/controllers/provider-approval.js b/app/scripts/controllers/provider-approval.js
index f17220cb9..d3b7f6dff 100644
--- a/app/scripts/controllers/provider-approval.js
+++ b/app/scripts/controllers/provider-approval.js
@@ -54,7 +54,7 @@ class ProviderApprovalController {
   _handleProviderRequest (origin, siteTitle, siteImage, force) {
     this.store.updateState({ providerRequests: [{ origin, siteTitle, siteImage }] })
     const isUnlocked = this.keyringController.memStore.getState().isUnlocked
-    if (!force && this.isApproved(origin) && this.caching && isUnlocked) {
+    if (!force && this.approvedOrigins[origin] && this.caching && isUnlocked) {
       this.approveProviderRequest(origin)
       return
     }
@@ -67,9 +67,11 @@ class ProviderApprovalController {
    * @param {string} origin - Origin of the window
    */
   _handleIsApproved (origin) {
-    const isApproved = this.isApproved(origin) && this.caching
-    const caching = this.caching
-    this.platform && this.platform.sendMessage({ action: 'answer-is-approved', isApproved, caching }, { active: true })
+    this.platform && this.platform.sendMessage({
+      action: 'answer-is-approved',
+      isApproved: this.approvedOrigins[origin] && this.caching,
+      caching: this.caching,
+    }, { active: true })
   }
 
   /**
@@ -117,6 +119,7 @@ class ProviderApprovalController {
     this.platform && this.platform.sendMessage({ action: 'reject-provider-request' }, { active: true })
     const providerRequests = requests.filter(request => request.origin !== origin)
     this.store.updateState({ providerRequests })
+    delete this.approvedOrigins[origin]
   }
 
   /**
@@ -127,12 +130,12 @@ class ProviderApprovalController {
   }
 
   /**
-   * Determines if a given origin has been approved
+   * Determines if a given origin should have accounts exposed
    *
    * @param {string} origin - Domain origin to check for approval status
    * @returns {boolean} - True if the origin has been approved
    */
-  isApproved (origin) {
+  shouldExposeAccounts (origin) {
     const privacyMode = this.preferencesController.getFeatureFlags().privacyMode
     return !privacyMode || this.approvedOrigins[origin]
   }
diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js
index 33278db85..5ae0f608d 100644
--- a/app/scripts/metamask-controller.js
+++ b/app/scripts/metamask-controller.js
@@ -277,8 +277,8 @@ module.exports = class MetamaskController extends EventEmitter {
       getAccounts: async ({ origin }) => {
         // Expose no accounts if this origin has not been approved, preventing
         // account-requring RPC methods from completing successfully
-        const isApproved = this.providerApprovalController.isApproved(origin)
-        if (origin !== 'MetaMask' && !isApproved) { return [] }
+        const exposeAccounts = this.providerApprovalController.shouldExposeAccounts(origin)
+        if (origin !== 'MetaMask' && !exposeAccounts) { return [] }
         const isUnlocked = this.keyringController.memStore.getState().isUnlocked
         const selectedAddress = this.preferencesController.getSelectedAddress()
         // only show address if account is unlocked
diff --git a/test/e2e/beta/drizzle.spec.js b/test/e2e/beta/drizzle.spec.js
index ff4b4b74d..e6317c762 100644
--- a/test/e2e/beta/drizzle.spec.js
+++ b/test/e2e/beta/drizzle.spec.js
@@ -19,6 +19,7 @@ const {
   openNewPage,
   verboseReportOnFailure,
   waitUntilXWindowHandles,
+  switchToWindowWithTitle,
 } = require('./helpers')
 
 describe('MetaMask', function () {
@@ -266,17 +267,31 @@ describe('MetaMask', function () {
   })
 
   describe('Drizzle', () => {
-    it('should be able to detect our eth address', async () => {
+    let windowHandles
+    let extension
+    let popup
+    let dapp
+
+    it('be able to connect the account', async () => {
       await openNewPage(driver, 'http://127.0.0.1:3000/')
       await delay(regularDelayMs)
 
-      await waitUntilXWindowHandles(driver, 2)
-      const windowHandles = await driver.getAllWindowHandles()
-      const dapp = windowHandles[1]
+      await waitUntilXWindowHandles(driver, 3)
+      windowHandles = await driver.getAllWindowHandles()
+
+      extension = windowHandles[0]
+      popup = await switchToWindowWithTitle(driver, 'MetaMask Notification', windowHandles)
+      dapp = windowHandles.find(handle => handle !== extension && handle !== popup)
 
-      await driver.switchTo().window(dapp)
       await delay(regularDelayMs)
+      const approveButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Connect')]`))
+      await approveButton.click()
+    })
 
+    it('should be able to detect our eth address', async () => {
+      // Check if address exposed
+      await driver.switchTo().window(dapp)
+      await delay(regularDelayMs)
 
       const addressElement = await findElement(driver, By.css(`.pure-u-1-1 h4`))
       const addressText = await addressElement.getText()