Merge pull request #895 from Shopify/revert.fux.return_to.safari

Revert "Fix return_to address in Safari after enable_cookies and grated_storage_access"
Shopify · Feb 11, 2020 · 20022c0 · 20022c0
2 parents b65390d + 8e6f5fd
commit 20022c0
Show file tree

Hide file tree

Showing 6 changed files with 26 additions and 30 deletions.
diff --git a/app/assets/javascripts/shopify_app/storage_access.js b/app/assets/javascripts/shopify_app/storage_access.js
@@ -28,8 +28,8 @@
     window.parent.location.href = this.redirectData.myshopifyUrl + '/admin/apps';
   }
 
-  StorageAccessHelper.prototype.redirectToAppTargetUrl = function() {
-    window.location.href = this.redirectData.appTargetUrl;
+  StorageAccessHelper.prototype.redirectToAppHome = function() {
+    window.location.href = this.redirectData.appHomeUrl;
   }
 
   StorageAccessHelper.prototype.sameSiteNoneIncompatible = function(ua) {
@@ -68,7 +68,7 @@
       if (!document.cookie) {
         throw 'Cannot set third-party cookie.'
       }
-      this.redirectToAppTargetUrl();
+      this.redirectToAppHome();
     } catch (error) {
       console.warn('Third party cookies may be blocked.', error);
       this.redirectToAppTLD(ACCESS_DENIED_STATUS);
@@ -90,7 +90,7 @@
   StorageAccessHelper.prototype.handleHasStorageAccess = function() {
     if (sessionStorage.getItem('shopify.granted_storage_access')) {
       // If app was classified by ITP and used Storage Access API to acquire access
-      this.redirectToAppTargetUrl();
+      this.redirectToAppHome();
     } else {
       // If app has not been classified by ITP and still has storage access
       this.redirectToAppTLD(ACCESS_GRANTED_STATUS);

diff --git a/app/controllers/shopify_app/sessions_controller.rb b/app/controllers/shopify_app/sessions_controller.rb
@@ -20,12 +20,11 @@ def enable_cookies
 
       render(:enable_cookies, layout: false, locals: {
         does_not_have_storage_access_url: top_level_interaction_path(
-          shop: sanitized_shop_name,
-          return_to: params[:return_to]
+          shop: sanitized_shop_name
         ),
         has_storage_access_url: login_url_with_optional_shop(top_level: true),
-        app_target_url: params[:return_to] || granted_storage_access_path(shop: sanitized_shop_name),
-        current_shopify_domain: current_shopify_domain
+        app_home_url: granted_storage_access_path(shop: sanitized_shop_name),
+        current_shopify_domain: current_shopify_domain,
       })
     end
 
@@ -134,12 +133,11 @@ def redirect_to_request_storage_access
         layout: false,
         locals: {
           does_not_have_storage_access_url: top_level_interaction_path(
-            shop: sanitized_shop_name,
-            return_to: session[:return_to]
+            shop: sanitized_shop_name
           ),
           has_storage_access_url: login_url_with_optional_shop(top_level: true),
-          app_target_url: session[:return_to] || granted_storage_access_path(shop: sanitized_shop_name),
-          current_shopify_domain: current_shopify_domain
+          app_home_url: granted_storage_access_path(shop: sanitized_shop_name),
+          current_shopify_domain: current_shopify_domain,
         }
       )
     end

diff --git a/app/views/shopify_app/sessions/enable_cookies.html.erb b/app/views/shopify_app/sessions/enable_cookies.html.erb
@@ -32,7 +32,7 @@
           myshopifyUrl: "https://#{current_shopify_domain}",
           hasStorageAccessUrl: "#{has_storage_access_url}",
           doesNotHaveStorageAccessUrl: "#{does_not_have_storage_access_url}",
-          appTargetUrl: "#{app_target_url}"
+          appHomeUrl: "#{app_home_url}"
         },
       },
     )

diff --git a/app/views/shopify_app/sessions/request_storage_access.html.erb b/app/views/shopify_app/sessions/request_storage_access.html.erb
@@ -24,7 +24,7 @@
                       myshopifyUrl: "https://#{current_shopify_domain}",
                       hasStorageAccessUrl: "#{has_storage_access_url}",
                       doesNotHaveStorageAccessUrl: "#{does_not_have_storage_access_url}",
-                      appTargetUrl: "#{app_target_url}"
+                      appHomeUrl: "#{app_home_url}"
                   },
               },
               )

diff --git a/lib/shopify_app/controller_concerns/login_protection.rb b/lib/shopify_app/controller_concerns/login_protection.rb
@@ -100,10 +100,8 @@ def login_url_params(top_level:)
       query_params = {}
       query_params[:shop] = sanitized_params[:shop] if params[:shop].present?
 
-      return_to = session[:return_to] || params[:return_to]
-
-      if return_to.present? && return_to_param_required?
-        query_params[:return_to] = return_to
+      if session[:return_to] && return_to_param_required?
+        query_params[:return_to] = session[:return_to]
       end
 
       has_referer_shop_name = referer_sanitized_shop_name.present?

diff --git a/test/javascripts/shopify_app/storage_access_test.js b/test/javascripts/shopify_app/storage_access_test.js
@@ -43,34 +43,34 @@ suite('StorageAccessHelper', () => {
       sinon.assert.called(storageAccessHelper.setUpCookiePartitioning);
     });
 
-    test('calls redirectToAppTargetUrl instead of manageStorageAccess or setUpCookiePartitioningStub if ITPHelper.userAgentIsAffected returns true', async () => {
+    test('calls redirectToAppHome instead of manageStorageAccess or setUpCookiePartitioningStub if ITPHelper.userAgentIsAffected returns true', async () => {
       storageAccessHelperSandbox.stub(storageAccessHelper, 'sameSiteNoneIncompatible').callsFake(() => true);
       storageAccessHelperSandbox.stub(ITPHelper.prototype, 'userAgentIsAffected').callsFake(() => false);
 
       storageAccessHelperSandbox.stub(storageAccessHelper, 'manageStorageAccess').callsFake(() => true);
 
-      storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppTargetUrl');
+      storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppHome');
       storageAccessHelperSandbox.stub(storageAccessHelper, 'setUpCookiePartitioning');
 
       storageAccessHelper.execute();
 
       sinon.assert.notCalled(storageAccessHelper.manageStorageAccess);
-      sinon.assert.called(storageAccessHelper.redirectToAppTargetUrl);
+      sinon.assert.called(storageAccessHelper.redirectToAppHome);
       sinon.assert.notCalled(storageAccessHelper.setUpCookiePartitioning);
     });
 
-    test('calls manageStorageAccess instead of redirectToAppTargetUrl if ITPHelper.userAgentIsAffected returns true', async () => {
+    test('calls manageStorageAccess instead of redirectToAppHome if ITPHelper.userAgentIsAffected returns true', async () => {
       storageAccessHelperSandbox.stub(ITPHelper.prototype, 'userAgentIsAffected').callsFake(() => true);
 
       storageAccessHelperSandbox.stub(storageAccessHelper, 'manageStorageAccess').callsFake(() => true);
 
-      storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppTargetUrl');
+      storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppHome');
       storageAccessHelperSandbox.stub(storageAccessHelper, 'setUpCookiePartitioning');
 
       storageAccessHelper.execute();
 
       sinon.assert.called(storageAccessHelper.manageStorageAccess);
-      sinon.assert.notCalled(storageAccessHelper.redirectToAppTargetUrl);
+      sinon.assert.notCalled(storageAccessHelper.redirectToAppHome);
       sinon.assert.notCalled(storageAccessHelper.setUpCookiePartitioning);
     });
   });
@@ -160,34 +160,34 @@ suite('StorageAccessHelper', () => {
   });
 
   suite('handleRequestStorageAccess', () => {
-    test('calls redirectToAppTargetUrl instead of redirectToAppsIndex when document.requestStorageAccess resolves', () => {
+    test('calls redirectToAppHome instead of redirectToAppsIndex when document.requestStorageAccess resolves', () => {
       document.requestStorageAccess = () => {
         return new Promise((resolve) => {
           resolve();
         });
       };
 
-      storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppTargetUrl');
+      storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppHome');
       storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppsIndex');
 
       storageAccessHelper.handleRequestStorageAccess().then(() => {
-        sinon.assert.called(storageAccessHelper.redirectToAppTargetUrl);
+        sinon.assert.called(storageAccessHelper.redirectToAppHome);
         sinon.assert.notCalled(storageAccessHelper.redirectToAppsIndex);
       });
     });
 
-    test('calls redirectToAppsIndex with "storage_access_denied" instead of calling redirectToAppTargetUrl when document.requestStorageAccess fails', () => {
+    test('calls redirectToAppsIndex with "storage_access_denied" instead of calling redirectToAppHome when document.requestStorageAccess fails', () => {
       document.requestStorageAccess = () => {
         return new Promise((resolve, reject) => {
           reject();
         });
       };
 
-      storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppTargetUrl');
+      storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppHome');
       storageAccessHelperSandbox.stub(storageAccessHelper, 'redirectToAppsIndex');
 
       storageAccessHelper.handleRequestStorageAccess().then(() => {
-        sinon.assert.notCalled(storageAccessHelper.redirectToAppTargetUrl);
+        sinon.assert.notCalled(storageAccessHelper.redirectToAppHome);
         sinon.assert.calledWith(storageAccessHelper.redirectToAppsIndex, 'storage_access_denied');
       });
     });