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

@@ -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

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

backend/app/controllers/spree/admin/users_controller.rb

@@ -82,13 +82,33 @@ 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
         redirect_to edit_admin_user_path(@user)
       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

backend/app/views/spree/admin/users/edit.html.erb

@@ -26,7 +26,7 @@
   </div>
 </fieldset>
 
-<% if can?(:update, @user) %>
+<% if can?(:update, @user) && can?(:manage, :api_key) %>
   <fieldset data-hook="admin_user_api_key" id="admin_user_edit_api_key">
     <legend><%= t('.api_access') %></legend>
 
@@ -41,16 +41,16 @@
 
       </div>
       <div class="filter-actions actions">
-        <%= 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' %>
       </div>
 
     <% else %>
 
       <div class="no-objects-found"><%= t('.no_key') %></div>
 
       <div class="filter-actions actions">
-        <%= 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' %>
       </div>
     <% end %>
   </fieldset>

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

backend/spec/controllers/spree/admin/users_controller_spec.rb

@@ -42,13 +42,15 @@
       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 }
         expect(response).to redirect_to(spree.edit_admin_user_path(user))
       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 }

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