From 6e42ac9283e809bf087c24367423631a77e54275 Mon Sep 17 00:00:00 2001
From: Alberto Vena <kennyadsl@gmail.com>
Date: Thu, 12 Nov 2020 17:37:18 +0100
Subject: [PATCH] Fix permissions to see admin menu items

Right now a user without roles like logged users or guests can
see the admin menu partially.

This is because it actually has permission to show zones and
shipping methods and the current code is checking against the
:show ability, so it seems to be legit.

We use :admin for the rest of the menu items checks though, and
this is also what we use in the controller to determine if
we can access that page, see: https://github.com/solidusio/solidus/blob/3c8ffcc34f9248b286a9d4ca94d1f9a3197ac7b2/backend/app/controllers/spree/admin/base_controller.rb#L29

Test setup lines have been added to be sure the links are populated
correctly in the menu.
---
 .../admin/shared/_settings_sub_menu.html.erb  | 10 ++++-----
 backend/lib/spree/backend_configuration.rb    | 22 +++++++++----------
 backend/spec/features/admin/homepage_spec.rb  |  8 ++++---
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/backend/app/views/spree/admin/shared/_settings_sub_menu.html.erb b/backend/app/views/spree/admin/shared/_settings_sub_menu.html.erb
index 5b62e5bc8bc..b7180af6265 100644
--- a/backend/app/views/spree/admin/shared/_settings_sub_menu.html.erb
+++ b/backend/app/views/spree/admin/shared/_settings_sub_menu.html.erb
@@ -3,24 +3,24 @@
     <%= tab :stores, label: :stores, url: spree.admin_stores_path %>
   <% end %>
 
-  <% if can?(:show, Spree::PaymentMethod) %>
+  <% if can?(:admin, Spree::PaymentMethod) %>
     <%= tab :payments, url: spree.admin_payment_methods_path %>
   <% end %>
 
-  <% if can?(:show, Spree::TaxCategory) || can?(:show, Spree::TaxRate) %>
+  <% if can?(:admin, Spree::TaxCategory) || can?(:admin, Spree::TaxRate) %>
     <%= tab :taxes, url: spree.admin_tax_categories_path, match_path: %r(tax_categories|tax_rates) %>
   <% end %>
 
-  <% if can?(:show, Spree::RefundReason) || can?(:show, Spree::ReimbursementType) ||
+  <% if can?(:admin, Spree::RefundReason) || can?(:admin, Spree::ReimbursementType) ||
     can?(:show, Spree::ReturnReason) || can?(:show, Spree::AdjustmentReason) %>
     <%= tab :checkout, url: spree.admin_refund_reasons_path, match_path: %r(refund_reasons|reimbursement_types|return_reasons|adjustment_reasons|store_credit_reasons) %>
   <% end %>
 
-  <% if can?(:show, Spree::ShippingMethod) || can?(:show, Spree::ShippingCategory) || can?(:show, Spree::StockLocation) %>
+  <% if can?(:admin, Spree::ShippingMethod) || can?(:admin, Spree::ShippingCategory) || can?(:admin, Spree::StockLocation) %>
     <%= tab :shipping, url: spree.admin_shipping_methods_path, match_path: %r(shipping_methods|shipping_categories|stock_locations) %>
   <% end %>
 
-  <% if can?(:show, Spree::Zone) %>
+  <% if can?(:admin, Spree::Zone) %>
     <%= tab :zones, url: spree.admin_zones_path %>
   <% end %>
 </ul>
diff --git a/backend/lib/spree/backend_configuration.rb b/backend/lib/spree/backend_configuration.rb
index ef57d259c3d..d5319065ec9 100644
--- a/backend/lib/spree/backend_configuration.rb
+++ b/backend/lib/spree/backend_configuration.rb
@@ -133,17 +133,17 @@ def menu_items
           'wrench',
           condition: -> {
             can?(:admin, Spree::Store) ||
-            can?(:show, Spree::AdjustmentReason) ||
-            can?(:show, Spree::PaymentMethod) ||
-            can?(:show, Spree::RefundReason) ||
-            can?(:show, Spree::ReimbursementType) ||
-            can?(:show, Spree::ShippingCategory) ||
-            can?(:show, Spree::ShippingMethod) ||
-            can?(:show, Spree::StockLocation) ||
-            can?(:show, Spree::TaxCategory) ||
-            can?(:show, Spree::TaxRate) ||
-            can?(:show, Spree::ReturnReason) ||
-            can?(:show, Spree::Zone)
+            can?(:admin, Spree::AdjustmentReason) ||
+            can?(:admin, Spree::PaymentMethod) ||
+            can?(:admin, Spree::RefundReason) ||
+            can?(:admin, Spree::ReimbursementType) ||
+            can?(:admin, Spree::ShippingCategory) ||
+            can?(:admin, Spree::ShippingMethod) ||
+            can?(:admin, Spree::StockLocation) ||
+            can?(:admin, Spree::TaxCategory) ||
+            can?(:admin, Spree::TaxRate) ||
+            can?(:admin, Spree::ReturnReason) ||
+            can?(:admin, Spree::Zone)
           },
           label: :settings,
           partial: 'spree/admin/shared/settings_sub_menu',
diff --git a/backend/spec/features/admin/homepage_spec.rb b/backend/spec/features/admin/homepage_spec.rb
index 372390aa6dc..114d1c80940 100644
--- a/backend/spec/features/admin/homepage_spec.rb
+++ b/backend/spec/features/admin/homepage_spec.rb
@@ -73,8 +73,8 @@
     custom_authorization! do |_user|
       can [:admin, :home], :dashboards
       can [:admin, :edit, :index, :show], Spree::Order
-      cannot [:show], Spree::StockLocation
-      cannot [:show], Spree::Zone
+      cannot [:admin], Spree::StockLocation
+      can [:admin], Spree::Zone
     end
 
     it 'should only display tabs fakedispatch has access to' do
@@ -82,7 +82,9 @@
       expect(page).to have_link('Orders')
       expect(page).not_to have_link('Products')
       expect(page).not_to have_link('Promotions')
-      expect(page).not_to have_link('Settings')
+      expect(page).to have_link('Settings')
+      expect(page).not_to have_link('Stock Locations', visible: false)
+      expect(page).to have_link('Zones', visible: false)
     end
   end
 end