From b96bcabbbdef62cd99abff14b59fc931b9037050 Mon Sep 17 00:00:00 2001 From: Kevin Date: Sun, 25 Jun 2023 15:54:17 +0100 Subject: [PATCH] Feature: Use delete request with sign out 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. --- app/components/nav/item_component.html.erb | 6 ++--- app/components/nav/item_component.rb | 14 ++++++++--- .../user/profile_dropdown_component.html.erb | 2 +- app/views/shared/_off_canvas_menu.html.erb | 24 +++++++++---------- config/routes.rb | 2 +- spec/components/nav/item_component_spec.rb | 20 ++++++++++++++-- 6 files changed, 46 insertions(+), 22 deletions(-) diff --git a/app/components/nav/item_component.html.erb b/app/components/nav/item_component.html.erb index b350db92fb..25a6d11e7f 100644 --- 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 index 429f4e7a64..97b540a912 100644 --- 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 index a05230bddb..3d9ce225a9 100644 --- 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 index 05cb4ce78d..b03c882e05 100644 --- a/app/views/shared/_off_canvas_menu.html.erb +++ b/app/views/shared/_off_canvas_menu.html.erb @@ -53,31 +53,31 @@ diff --git a/config/routes.rb b/config/routes.rb index e410f82e8e..059d5e5df4 100644 --- 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 index 61dcadae17..55bd95d43a 100644 --- 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,7 +25,6 @@ text: 'Home', test_id: 'nav-home', icon_path: nil, - mobile: false ) render_inline(component) @@ -33,4 +32,21 @@ 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