From 415323846bfbda6e2dac9b0abc6152b172349a96 Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Thu, 28 Nov 2019 12:28:28 +0100 Subject: [PATCH] Use RESTful routing for users' API key management This commit refactors how we are using routes to use RESTful actions, moving the non-RESTful endpoint into a separate controller. This has the benefit to have a more organized codebase, but also to improve the granularity of how we can set permissions. For example, now we can have a permission set that does not allow to change or revoke users' API key but allows management of other users things. For this reason, a new ability is required and has been added in the provided UserManagement permission set. This commit also deprecates the old controller actions, printing a message in the log of users that are still using the old actions. The controller tests for the old actions have been deprecated as well, and copied over the new controller specs file but it is only checking controller related stuff now, since the real functionality is already covered by unit and feature specs elsewhere. --- api/config/routes.rb | 6 +- .../spree/admin/users/api_key_controller.rb | 29 +++++++++ .../spree/admin/users_controller.rb | 20 ++++++ .../app/views/spree/admin/users/edit.html.erb | 8 +-- .../admin/users/api_key_controller_spec.rb | 64 +++++++++++++++++++ .../spree/admin/users_controller_spec.rb | 2 + .../spree/permission_sets/user_management.rb | 1 + 7 files changed, 124 insertions(+), 6 deletions(-) create mode 100644 backend/app/controllers/spree/admin/users/api_key_controller.rb create mode 100644 backend/spec/controllers/spree/admin/users/api_key_controller_spec.rb diff --git a/api/config/routes.rb b/api/config/routes.rb index 3d3a1caf5d7..94b800c801d 100644 --- a/api/config/routes.rb +++ b/api/config/routes.rb @@ -3,9 +3,11 @@ Spree::Core::Engine.routes.draw do namespace :admin do resources :users do + resource :api_key, controller: 'users/api_key', only: [:create, :destroy] + member do - put :generate_api_key - put :clear_api_key + put :generate_api_key # Deprecated + put :clear_api_key # Deprecated end end end diff --git a/backend/app/controllers/spree/admin/users/api_key_controller.rb b/backend/app/controllers/spree/admin/users/api_key_controller.rb new file mode 100644 index 00000000000..894c2ae4705 --- /dev/null +++ b/backend/app/controllers/spree/admin/users/api_key_controller.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Spree + module Admin + module Users + class ApiKeyController < Spree::Admin::BaseController + def create + if user.generate_spree_api_key! + flash[:success] = t('spree.admin.api.key_generated') + end + redirect_to edit_admin_user_path(user) + end + + def destroy + if user.clear_spree_api_key! + flash[:success] = t('spree.admin.api.key_cleared') + end + redirect_to edit_admin_user_path(user) + end + + private + + def user + @user ||= Spree.user_class.find(params[:user_id]) + end + end + end + end +end diff --git a/backend/app/controllers/spree/admin/users_controller.rb b/backend/app/controllers/spree/admin/users_controller.rb index 72d5cb4c0d2..126d73517fb 100644 --- a/backend/app/controllers/spree/admin/users_controller.rb +++ b/backend/app/controllers/spree/admin/users_controller.rb @@ -82,6 +82,16 @@ def items end def generate_api_key + Spree::Deprecation.warn <<-WARN.strip_heredoc, caller + The route or controller action you are using is deprecated. + + Instead of: + generate_api_key_admin_user PUT /admin/users/:id/generate_api_key + + Please use: + admin_user_api_key POST /admin/users/:user_id/api_key + WARN + if @user.generate_spree_api_key! flash[:success] = t('spree.admin.api.key_generated') end @@ -89,6 +99,16 @@ def generate_api_key end def clear_api_key + Spree::Deprecation.warn <<-WARN.strip_heredoc, caller + The route or controller action you are using is deprecated. + + Instead of: + clear_api_key_admin_user PUT /admin/users/:id/clear_api_key + + Please use: + admin_user_api_key DELETE /admin/users/:user_id/api_key + WARN + if @user.clear_spree_api_key! flash[:success] = t('spree.admin.api.key_cleared') end diff --git a/backend/app/views/spree/admin/users/edit.html.erb b/backend/app/views/spree/admin/users/edit.html.erb index ea21b894238..e35e4f31b09 100644 --- a/backend/app/views/spree/admin/users/edit.html.erb +++ b/backend/app/views/spree/admin/users/edit.html.erb @@ -26,7 +26,7 @@ -<% if can?(:update, @user) %> +<% if can?(:update, @user) && can?(:manage, :api_key) %>
<%= t('.api_access') %> @@ -41,8 +41,8 @@
- <%= button_to t('.clear_key'), spree.clear_api_key_admin_user_path(@user), method: :put, data: { confirm: t('.confirm_clear_key') }, class: 'btn btn-default' %> - <%= button_to t('.regenerate_key'), spree.generate_api_key_admin_user_path(@user), method: :put, data: { confirm: t('.confirm_regenerate_key') }, class: 'btn btn-default' %> + <%= button_to t('.clear_key'), spree.admin_user_api_key_path(@user), method: :delete, data: { confirm: t('.confirm_clear_key') }, class: 'btn btn-default' %> + <%= button_to t('.regenerate_key'), spree.admin_user_api_key_path(@user), method: :post, data: { confirm: t('.confirm_regenerate_key') }, class: 'btn btn-default' %>
<% else %> @@ -50,7 +50,7 @@
<%= t('.no_key') %>
- <%= button_to t('.generate_key'), spree.generate_api_key_admin_user_path(@user), method: :put, class: 'btn btn-primary' %> + <%= button_to t('.generate_key'), spree.admin_user_api_key_path(@user), method: :post, class: 'btn btn-primary' %>
<% end %>
diff --git a/backend/spec/controllers/spree/admin/users/api_key_controller_spec.rb b/backend/spec/controllers/spree/admin/users/api_key_controller_spec.rb new file mode 100644 index 00000000000..b30eee0058c --- /dev/null +++ b/backend/spec/controllers/spree/admin/users/api_key_controller_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe Spree::Admin::Users::ApiKeyController, type: :controller do + let(:user) { create(:user) } + + describe '#create' do + context "with ability to manage users and API keys" do + stub_authorization! do |_user| + can [:manage], Spree.user_class + can [:manage], :api_key + end + + it "allows admins to create a new user's API key" do + post :create, params: { user_id: user.id } + + expect(flash[:success]).to eq I18n.t('spree.admin.api.key_generated') + expect(response).to redirect_to(spree.edit_admin_user_path(user)) + end + end + + context "without ability to manage users and API keys" do + stub_authorization! do |_user| + end + + it 'denies access' do + delete :destroy, params: { user_id: user.id } + + expect(flash[:error]).to eq I18n.t('spree.authorization_failure') + expect(response).to redirect_to '/unauthorized' + end + end + end + + describe '#destroy' do + context "with ability to manage users and API keys" do + stub_authorization! do |_user| + can [:manage], Spree.user_class + can [:manage], :api_key + end + + it "allows admins to clear an existing user's API key" do + user.generate_spree_api_key! + delete :destroy, params: { user_id: user.id } + + expect(flash[:success]).to eq I18n.t('spree.admin.api.key_cleared') + expect(response).to redirect_to(spree.edit_admin_user_path(user)) + end + end + + context "without ability to manage users and API keys" do + stub_authorization! do |_user| + end + + it 'denies access' do + delete :destroy, params: { user_id: user.id } + + expect(flash[:error]).to eq I18n.t('spree.authorization_failure') + expect(response).to redirect_to '/unauthorized' + end + end + end +end diff --git a/backend/spec/controllers/spree/admin/users_controller_spec.rb b/backend/spec/controllers/spree/admin/users_controller_spec.rb index 7a111d7efe8..415d7a935b8 100644 --- a/backend/spec/controllers/spree/admin/users_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/users_controller_spec.rb @@ -42,6 +42,7 @@ end it "allows admins to update a user's API key" do + expect(Spree::Deprecation).to receive(:warn) expect { put :generate_api_key, params: { id: user.id } }.to change { user.reload.spree_api_key } @@ -49,6 +50,7 @@ end it "allows admins to clear a user's API key" do + expect(Spree::Deprecation).to receive(:warn) user.generate_spree_api_key! expect { put :clear_api_key, params: { id: user.id } diff --git a/core/lib/spree/permission_sets/user_management.rb b/core/lib/spree/permission_sets/user_management.rb index 90a65ca0319..72457246ac6 100644 --- a/core/lib/spree/permission_sets/user_management.rb +++ b/core/lib/spree/permission_sets/user_management.rb @@ -18,6 +18,7 @@ def activate! cannot [:delete, :destroy], Spree.user_class can :manage, Spree::StoreCredit can :display, Spree::Role + can :manage, :api_key end end end