Merge branch 'mw/pat-revoke-service' into 'master'

Add PersonalAccessToken::RevokeService

See merge request gitlab-org/gitlab!38501

app/controllers/profiles/personal_access_tokens_controller.rb

@@ -20,12 +20,8 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
 
   def revoke
     @personal_access_token = finder.find(params[:id])
-
-    if @personal_access_token.revoke!
-      flash[:notice] = _("Revoked personal access token %{personal_access_token_name}!") % { personal_access_token_name: @personal_access_token.name }
-    else
-      flash[:alert] = _("Could not revoke personal access token %{personal_access_token_name}.") % { personal_access_token_name: @personal_access_token.name }
-    end
+    service = PersonalAccessTokens::RevokeService.new(current_user, token: @personal_access_token).execute
+    service.success? ? flash[:notice] = service.message : flash[:alert] = service.message
 
     redirect_to profile_personal_access_tokens_path
   end

app/policies/personal_access_token_policy.rb

@@ -3,7 +3,8 @@
 class PersonalAccessTokenPolicy < BasePolicy
   condition(:is_owner) { user && subject.user_id == user.id }
 
-  rule { is_owner | admin }.policy do
+  rule { is_owner | admin & ~blocked }.policy do
     enable :read_token
+    enable :revoke_token
   end
 end

app/services/personal_access_tokens/revoke_service.rb

+# frozen_string_literal: true
+
+module PersonalAccessTokens
+  class RevokeService
+    attr_reader :token, :current_user
+
+    def initialize(current_user = nil, params = { token: nil })
+      @current_user = current_user
+      @token = params[:token]
+    end
+
+    def execute
+      return ServiceResponse.error(message: 'Not permitted to revoke') unless revocation_permitted?
+
+      if token.revoke!
+        ServiceResponse.success(message: success_message)
+      else
+        ServiceResponse.error(message: error_message)
+      end
+    end
+
+    private
+
+    def error_message
+      _("Could not revoke personal access token %{personal_access_token_name}.") % { personal_access_token_name: token.name }
+    end
+
+    def success_message
+      _("Revoked personal access token %{personal_access_token_name}!") % { personal_access_token_name: token.name }
+    end
+
+    def revocation_permitted?
+      Ability.allowed?(current_user, :revoke_token, token)
+    end
+  end
+end

spec/features/profiles/personal_access_tokens_spec.rb

@@ -100,14 +100,11 @@ RSpec.describe 'Profile > Personal Access Tokens', :js do
     context "when revocation fails" do
       it "displays an error message" do
         visit profile_personal_access_tokens_path
-        allow_any_instance_of(PersonalAccessToken).to receive(:update!).and_return(false)
-
-        errors = ActiveModel::Errors.new(PersonalAccessToken.new).tap { |e| e.add(:name, "cannot be nil") }
-        allow_any_instance_of(PersonalAccessToken).to receive(:errors).and_return(errors)
+        allow_any_instance_of(PersonalAccessTokens::RevokeService).to receive(:revocation_permitted?).and_return(false)
 
         accept_confirm { click_on "Revoke" }
         expect(active_personal_access_tokens).to have_text(personal_access_token.name)
-        expect(page).to have_content("Could not revoke")
+        expect(page).to have_content("Not permitted to revoke")
       end
     end
   end

spec/policies/personal_access_token_policy_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe PersonalAccessTokenPolicy do
   end
 
   with_them do
-    context 'determine if a token is readable by a user' do
+    context 'determine if a token is readable or revocable by a user' do
       let(:user) { build_stubbed(user_type) }
       let(:token_owner) { owned_by_same_user ? user : build(:user) }
       let(:token) { build(:personal_access_token, user: token_owner) }
@@ -26,6 +26,17 @@ RSpec.describe PersonalAccessTokenPolicy do
       end
 
       it { is_expected.to(expected_permitted? ? be_allowed(:read_token) : be_disallowed(:read_token)) }
+      it { is_expected.to(expected_permitted? ? be_allowed(:revoke_token) : be_disallowed(:revoke_token)) }
     end
   end
+
+  context 'current_user is a blocked administrator', :enable_admin_mode do
+    subject { described_class.new(current_user, token) }
+
+    let(:current_user) { create(:user, :admin, :blocked) }
+    let(:token) { create(:personal_access_token) }
+
+    it { is_expected.to be_disallowed(:revoke_token) }
+    it { is_expected.to be_disallowed(:read_token) }
+  end
 end

spec/services/personal_access_tokens/revoke_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe PersonalAccessTokens::RevokeService do
+  shared_examples_for 'a successfully revoked token' do
+    it { expect(subject.success?).to be true }
+    it { expect(service.token.revoked?).to be true }
+  end
+
+  shared_examples_for 'an unsuccessfully revoked token' do
+    it { expect(subject.success?).to be false }
+    it { expect(service.token.revoked?).to be false }
+  end
+
+  describe '#execute' do
+    subject { service.execute }
+
+    let(:service) { described_class.new(current_user, token: token) }
+
+    context 'when current_user is an administrator' do
+      let_it_be(:current_user) { create(:admin) }
+      let_it_be(:token) { create(:personal_access_token) }
+
+      it_behaves_like 'a successfully revoked token'
+    end
+
+    context 'when current_user is not an administrator' do
+      let_it_be(:current_user) { create(:user) }
+
+      context 'token belongs to a different user' do
+        let_it_be(:token) { create(:personal_access_token) }
+
+        it_behaves_like 'an unsuccessfully revoked token'
+      end
+
+      context 'token belongs to current_user' do
+        let_it_be(:token) { create(:personal_access_token, user: current_user) }
+
+        it_behaves_like 'a successfully revoked token'
+      end
+    end
+  end
+end