Feature: Use delete request with sign out (#3881)

Because: * Using a get for something destructive like signing out is a security issue. Users could be made to unintentionally sign out if they clicked on a "https://theodinproject.com/sign_out" link someone sent them. This commit: * Change sign_out path to delete instead of a get request. * Refactor nav item component to take an options hash which we can use for the mobile flag and this new method option.
TheOdinProject · Jun 26, 2023 · e8b7871 · e8b7871
1 parent 4cad719
commit e8b7871
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 22 deletions.
diff --git a/app/components/nav/item_component.html.erb b/app/components/nav/item_component.html.erb
@@ -1,8 +1,8 @@
-<% if mobile %>
-  <%= link_to path, class: "text-gray-600 hover:bg-gray-50 hover:text-gray-900 group flex items-center px-2 py-2 text-base font-medium dark:text-gray-300 dark:hover:text-gray-200 dark:hover:bg-gray-700/60 #{'bg-gray-100 text-gray-900 dark:bg-gray-700/60 dark:text-gray-3  00' if active?}", data: { test_id: }  do %>
+<% if mobile? %>
+  <%= link_to path, class: "text-gray-600 hover:bg-gray-50 hover:text-gray-900 group flex items-center px-2 py-2 text-base font-medium dark:text-gray-300 dark:hover:text-gray-200 dark:hover:bg-gray-700/60 #{'bg-gray-100 text-gray-900 dark:bg-gray-700/60 dark:text-gray-3  00' if active?}", data: { test_id:, turbo_method: http_method }  do %>
     <%= inline_svg_tag icon_path, class: 'text-gray-400 group-hover:text-gray-500 dark:text-gray-200 dark:group-hover:text-gray-200 mr-4 h-6 w-6', aria: true, title: "#{text} icon" %>
     <%= text %>
   <% end %>
 <% else %>
-  <%= link_to text, path, class: " text-sm border-transparent text-gray-500 dark:text-gray-300 hover:border-gray-300 hover:text-gray-700 inline-flex items-center px-1 pt-1  border-b-2 font-medium #{'border-gold-500 text-gray-900' if active?}", data: { test_id: } %>
+  <%= link_to text, path, class: " text-sm border-transparent text-gray-500 dark:text-gray-300 hover:border-gray-300 hover:text-gray-700 inline-flex items-center px-1 pt-1  border-b-2 font-medium #{'border-gold-500 text-gray-900' if active?}", data: { test_id:, turbo_method: http_method } %>
 <% end %>
diff --git a/app/components/nav/item_component.rb b/app/components/nav/item_component.rb
@@ -1,10 +1,10 @@
 class Nav::ItemComponent < ApplicationComponent
-  def initialize(path:, text:, test_id:, icon_path: nil, mobile: false)
+  def initialize(path:, text:, test_id:, icon_path: nil, options: {})
     @path = path
     @text = text
     @test_id = test_id
     @icon_path = icon_path
-    @mobile = mobile
+    @options = options
   end
 
   def active?
@@ -13,5 +13,13 @@ def active?
 
   private
 
-  attr_reader :path, :text, :test_id, :icon_path, :mobile
+  attr_reader :path, :text, :test_id, :icon_path, :options
+
+  def http_method
+    options.fetch(:method, :get)
+  end
+
+  def mobile?
+    options.fetch(:mobile, false)
+  end
 end
diff --git a/app/components/user/profile_dropdown_component.html.erb b/app/components/user/profile_dropdown_component.html.erb
@@ -28,7 +28,7 @@
       <%= render Theme::SwitcherComponent.new(current_theme:) %>
     <% end %>
 
-    <%= link_to sign_out_path, class: 'text-gray-700 dark:text-gray-300 group flex items-center px-3 py-2 text-sm' do %>
+    <%= button_to sign_out_path, class: 'text-gray-700 dark:text-gray-300 group flex items-center px-3 py-2 text-sm', method: :delete do %>
       <%= inline_svg_tag 'icons/sign-out.svg', class: 'mr-3 h-5 w-5 text-gray-400 group-hover:text-gray-500 dark:text-gray-300 dark:group-hover:text-gray-400', aria: true, title: 'Sign out icon' %>
       Sign out
     <% end %>

diff --git a/app/views/shared/_off_canvas_menu.html.erb b/app/views/shared/_off_canvas_menu.html.erb
@@ -53,31 +53,31 @@
         <nav aria-label="Sidebar" class="mt-3">
           <div class="px-2 space-y-1">
             <% if user_signed_in? %>
-              <%= render Nav::ItemComponent.new(path: dashboard_path, text: 'Dashboard', test_id: 'nav-dashboard', icon_path: 'icons/home.svg', mobile: true) %>
-              <%= render Nav::ItemComponent.new(path: paths_url, text: 'All Paths', test_id: 'nav-all-paths', icon_path: 'icons/map.svg', mobile: true) %>
-              <%= render Nav::ItemComponent.new(path: notifications_path, text: 'Notifications', test_id: 'nav-notifications', icon_path: 'icons/bell.svg', mobile: true) %>
-              <%= render Nav::ItemComponent.new(path: ODIN_CHAT_URL, text: 'Community', test_id: 'nav-community', icon_path: 'icons/speech-bubbles.svg', mobile: true) %>
+              <%= render Nav::ItemComponent.new(path: dashboard_path, text: 'Dashboard', test_id: 'nav-dashboard', icon_path: 'icons/home.svg', options: { mobile: true }) %>
+              <%= render Nav::ItemComponent.new(path: paths_url, text: 'All Paths', test_id: 'nav-all-paths', icon_path: 'icons/map.svg', options: { mobile: true }) %>
+              <%= render Nav::ItemComponent.new(path: notifications_path, text: 'Notifications', test_id: 'nav-notifications', icon_path: 'icons/bell.svg', options: { mobile: true }) %>
+              <%= render Nav::ItemComponent.new(path: ODIN_CHAT_URL, text: 'Community', test_id: 'nav-community', icon_path: 'icons/speech-bubbles.svg', options: { mobile: true }) %>
             <% else %>
-              <%= render Nav::ItemComponent.new(path: root_path, text: 'Home', test_id: 'nav-home', icon_path: 'icons/home.svg', mobile: true) %>
-              <%= render Nav::ItemComponent.new(path: paths_url, text: 'All Paths', test_id: 'nav-all-paths', icon_path: 'icons/map.svg', mobile: true) %>
-              <%= render Nav::ItemComponent.new(path: about_path, text: 'About', test_id: 'nav-about', icon_path: 'icons/information.svg', mobile: true) %>
-              <%= render Nav::ItemComponent.new(path: ODIN_CHAT_URL, text: 'Community', test_id: 'nav-community', icon_path: 'icons/speech-bubbles.svg', mobile: true) %>
+              <%= render Nav::ItemComponent.new(path: root_path, text: 'Home', test_id: 'nav-home', icon_path: 'icons/home.svg', options: { mobile: true }) %>
+              <%= render Nav::ItemComponent.new(path: paths_url, text: 'All Paths', test_id: 'nav-all-paths', icon_path: 'icons/map.svg', options: { mobile: true }) %>
+              <%= render Nav::ItemComponent.new(path: about_path, text: 'About', test_id: 'nav-about', icon_path: 'icons/information.svg', options: { mobile: true }) %>
+              <%= render Nav::ItemComponent.new(path: ODIN_CHAT_URL, text: 'Community', test_id: 'nav-community', icon_path: 'icons/speech-bubbles.svg', options: { mobile: true }) %>
             <% end %>
           </div>
           <hr class="border-t border-gray-200 my-4" aria-hidden="true">
           <div class="px-2 space-y-1">
             <% if user_signed_in? %>
-              <%= render Nav::ItemComponent.new(path: edit_users_profile_path, text: 'Settings', test_id: 'nav-settings', icon_path: 'icons/gear.svg', mobile: true) %>
+              <%= render Nav::ItemComponent.new(path: edit_users_profile_path, text: 'Settings', test_id: 'nav-settings', icon_path: 'icons/gear.svg', options: { mobile: true }) %>
                <%= turbo_frame_tag 'theme_switcher_mobile' do %>
                 <%= render Theme::SwitcherComponent.new(current_theme:, type: :mobile) %>
               <% end %>
-              <%= render Nav::ItemComponent.new(path: sign_out_path, text: 'Sign out', test_id: 'nav-sign-out', icon_path: 'icons/sign-out.svg', mobile: true) %>
+              <%= render Nav::ItemComponent.new(path: sign_out_path, text: 'Sign out', test_id: 'nav-sign-out', icon_path: 'icons/sign-out.svg', options: { method: :delete, mobile: true }) %>
             <% else %>
-              <%= render Nav::ItemComponent.new(path: sign_up_path, text: 'Get started', test_id: 'nav-sign-up', icon_path: 'icons/rocket.svg', mobile: true) %>
+              <%= render Nav::ItemComponent.new(path: sign_up_path, text: 'Get started', test_id: 'nav-sign-up', icon_path: 'icons/rocket.svg', options: { mobile: true }) %>
               <%= turbo_frame_tag 'theme_switcher_mobile' do %>
                 <%= render Theme::SwitcherComponent.new(current_theme:, type: :mobile) %>
               <% end %>
-              <%= render Nav::ItemComponent.new(path: sign_in_path, text: 'Sign in', test_id: 'nav-sign-in', icon_path: 'icons/sign-in.svg', mobile: true) %>
+              <%= render Nav::ItemComponent.new(path: sign_in_path, text: 'Sign in', test_id: 'nav-sign-in', icon_path: 'icons/sign-in.svg', options: { mobile: true }) %>
             <% end %>
           </div>
         </nav>

diff --git a/config/routes.rb b/config/routes.rb
@@ -32,7 +32,7 @@
 
   devise_scope :user do
     get '/sign_in' => 'users/sessions#new'
-    get '/sign_out' => 'users/sessions#destroy', method: :delete
+    delete '/sign_out' => 'users/sessions#destroy'
     get '/sign_up' => 'users/registrations#new'
   end
 

diff --git a/spec/components/nav/item_component_spec.rb b/spec/components/nav/item_component_spec.rb
@@ -8,7 +8,7 @@
         text: 'Home',
         test_id: 'nav-home',
         icon_path: 'icons/home.svg',
-        mobile: true
+        options: { mobile: true }
       )
 
       render_inline(component)
@@ -25,12 +25,28 @@
         text: 'Home',
         test_id: 'nav-home',
         icon_path: nil,
-        mobile: false
       )
 
       render_inline(component)
 
       expect(page).to have_link('Home', href: '/home')
     end
   end
+
+  context 'when nav item uses a different http request' do
+    it 'includes the different http request' do
+      component = described_class.new(
+        path: '/sign_out',
+        text: 'Sign out',
+        test_id: 'nav-sign-out',
+        icon_path: nil,
+        options: { method: :delete }
+      )
+
+      render_inline(component)
+
+      expect(page).to have_link('Sign out', href: '/sign_out')
+      expect(page.find_link('Sign out')['data-turbo-method']).to eq('delete')
+    end
+  end
 end