Merge branch...

Merge branch '17176-add-application-and-audit-event-logs-for-create-revoke-pat-services' into 'master'

Add app and audit event logs to PAT create and revoke services

See merge request gitlab-org/gitlab!45976

app/controllers/profiles/personal_access_tokens_controller.rb

@@ -9,9 +9,13 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
   end
 
   def create
-    @personal_access_token = finder.build(personal_access_token_params)
+    result = ::PersonalAccessTokens::CreateService.new(
+      current_user: current_user, target_user: current_user, params: personal_access_token_params
+    ).execute
 
-    if @personal_access_token.save
+    @personal_access_token = result.payload[:personal_access_token]
+
+    if result.success?
       PersonalAccessToken.redis_store!(current_user.id, @personal_access_token.token)
       redirect_to profile_personal_access_tokens_path, notice: _("Your new personal access token has been created.")
     else

app/policies/user_policy.rb

@@ -27,6 +27,7 @@ class UserPolicy < BasePolicy
   rule { default }.enable :read_user_profile
   rule { (private_profile | blocked_user) & ~(user_is_self | admin) }.prevent :read_user_profile
   rule { user_is_self | admin }.enable :disable_two_factor
+  rule { (user_is_self | admin) & ~blocked }.enable :create_user_personal_access_token
 end
 
 UserPolicy.prepend_if_ee('EE::UserPolicy')

app/services/personal_access_tokens/create_service.rb

@@ -2,23 +2,30 @@
 
 module PersonalAccessTokens
   class CreateService < BaseService
-    def initialize(current_user, params = {})
+    def initialize(current_user:, target_user:, params: {})
       @current_user = current_user
+      @target_user = target_user
       @params = params.dup
+      @ip_address = @params.delete(:ip_address)
     end
 
     def execute
-      personal_access_token = current_user.personal_access_tokens.create(params.slice(*allowed_params))
+      return ServiceResponse.error(message: 'Not permitted to create') unless creation_permitted?
 
-      if personal_access_token.persisted?
-        ServiceResponse.success(payload: { personal_access_token: personal_access_token })
+      token = target_user.personal_access_tokens.create(params.slice(*allowed_params))
+
+      if token.persisted?
+        log_event(token)
+        ServiceResponse.success(payload: { personal_access_token: token })
       else
-        ServiceResponse.error(message: personal_access_token.errors.full_messages.to_sentence)
+        ServiceResponse.error(message: token.errors.full_messages.to_sentence, payload: { personal_access_token: token })
       end
     end
 
     private
 
+    attr_reader :target_user, :ip_address
+
     def allowed_params
       [
         :name,
@@ -27,5 +34,15 @@ module PersonalAccessTokens
         :expires_at
       ]
     end
+
+    def creation_permitted?
+      Ability.allowed?(current_user, :create_user_personal_access_token, target_user)
+    end
+
+    def log_event(token)
+      log_info("PAT CREATION: created_by: '#{current_user.username}', created_for: '#{token.user.username}', token_id: '#{token.id}'")
+    end
   end
 end
+
+PersonalAccessTokens::CreateService.prepend_if_ee('EE::PersonalAccessTokens::CreateService')

app/services/personal_access_tokens/revoke_service.rb

@@ -4,16 +4,17 @@ module PersonalAccessTokens
   class RevokeService
     attr_reader :token, :current_user, :group
 
-    def initialize(current_user = nil, params = { token: nil, group: nil })
+    def initialize(current_user = nil, token: nil, group: nil )
       @current_user = current_user
-      @token = params[:token]
-      @group = params[:group]
+      @token = token
+      @group = group
     end
 
     def execute
       return ServiceResponse.error(message: 'Not permitted to revoke') unless revocation_permitted?
 
       if token.revoke!
+        log_event
         ServiceResponse.success(message: success_message)
       else
         ServiceResponse.error(message: error_message)
@@ -33,6 +34,10 @@ module PersonalAccessTokens
     def revocation_permitted?
       Ability.allowed?(current_user, :revoke_token, token)
     end
+
+    def log_event
+      Gitlab::AppLogger.info("PAT REVOCATION: revoked_by: '#{current_user.username}', revoked_for: '#{token.user.username}', token_id: '#{token.id}'")
+    end
   end
 end
 

app/services/resource_access_tokens/create_service.rb

@@ -83,7 +83,9 @@ module ResourceAccessTokens
     end
 
     def create_personal_access_token(user)
-      PersonalAccessTokens::CreateService.new(user, personal_access_token_params).execute
+      PersonalAccessTokens::CreateService.new(
+        current_user: user, target_user: user, params: personal_access_token_params
+      ).execute
     end
 
     def personal_access_token_params

doc/administration/audit_events.md

@@ -127,6 +127,8 @@ recorded:
 - User was blocked via Admin Area ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/251) in GitLab 12.8)
 - User was blocked via API ([introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25872) in GitLab 12.9)
 - Failed second-factor authentication attempt ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/16826) in GitLab 13.5)
+- A user's personal access token was successfully created or revoked ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276921) in GitLab 13.6)
+- A failed attempt to create or revoke a user's personal access token ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276921) in GitLab 13.6)
 
 It's possible to filter particular actions by choosing an audit data type from
 the filter dropdown box. You can further filter by specific group, project, or user

ee/app/services/ee/personal_access_tokens/create_service.rb

+# frozen_string_literal: true
+
+module EE
+  module PersonalAccessTokens
+    module CreateService
+      def execute
+        super.tap do |response|
+          log_audit_event(response.payload[:personal_access_token], response)
+        end
+      end
+
+      private
+
+      def log_audit_event(token, response)
+        audit_event_service(token, response).for_user(full_path: target_user.username, entity_id: target_user.id).security_event
+      end
+
+      def audit_event_service(token, response)
+        message = if response.success?
+                    "Created personal access token with id #{token.id}"
+                  else
+                    "Attempted to create personal access token but failed with message: #{response.message}"
+                  end
+
+        ::AuditEventService.new(
+          current_user,
+          target_user,
+          action: :custom,
+          custom_message: message,
+          ip_address: ip_address
+        )
+      end
+    end
+  end
+end

ee/app/services/ee/personal_access_tokens/revoke_service.rb

@@ -6,9 +6,16 @@ module EE
       include ::Gitlab::Allowable
       extend ::Gitlab::Utils::Override
 
+      def execute
+        super.tap do |response|
+          log_audit_event(token, response)
+        end
+      end
+
       private
 
       override :revocation_permitted?
+
       def revocation_permitted?
         super || managed_user_revocation_allowed?
       end
@@ -20,6 +27,25 @@ module EE
           token.user.managing_group == group &&
           can?(current_user, :admin_group_credentials_inventory, group)
       end
+
+      def log_audit_event(token, response)
+        audit_event_service(token, response).for_user(full_path: token.user.username, entity_id: token.user.id).security_event
+      end
+
+      def audit_event_service(token, response)
+        message = if response.success?
+                    "Revoked personal access token with id #{token.id}"
+                  else
+                    "Attempted to revoke personal access token with id #{token.id} but failed with message: #{response.message}"
+                  end
+
+        ::AuditEventService.new(
+          current_user,
+          token.user,
+          action: :custom,
+          custom_message: message
+        )
+      end
     end
   end
 end

ee/changelogs/unreleased/17176-add-application-and-audit-event-logs-for-create-revoke-pat-services.yml

+---
+title: Add audit logs for PAT create and revoke services
+merge_request: 45976
+author:
+type: added

ee/spec/features/admin/admin_audit_logs_spec.rb

@@ -112,6 +112,73 @@ RSpec.describe 'Admin::AuditLogs', :js do
       it_behaves_like 'audit events date filter'
     end
 
+    describe 'personal access token events' do
+      shared_examples 'personal access token audit event' do
+        it 'show personal access token event details' do
+          visit admin_audit_logs_path
+
+          expect(page).to have_content(message)
+        end
+      end
+
+      context 'create personal access token' do
+        let(:personal_access_token_params) { { name: 'Test token', impersonation: false, scopes: [:api], expires_at: Date.today + 1.month } }
+        let(:personal_access_token) do
+          PersonalAccessTokens::CreateService.new(
+            current_user: admin, target_user: user, params: personal_access_token_params
+          ).execute.payload[:personal_access_token]
+        end
+
+        context 'when creation succeeds' do
+          before do
+            personal_access_token
+          end
+
+          let(:message) { "Created personal access token with id #{personal_access_token.id}" }
+
+          it_behaves_like 'personal access token audit event'
+        end
+
+        context 'when creation fails' do
+          before do
+            allow_any_instance_of(ServiceResponse).to receive(:success?).and_return(false)
+            allow_any_instance_of(ServiceResponse).to receive(:message).and_return('error')
+            personal_access_token
+          end
+
+          let(:message) { "Attempted to create personal access token but failed with message: error" }
+
+          it_behaves_like 'personal access token audit event'
+        end
+      end
+
+      context 'revoke personal access token' do
+        let(:personal_access_token) { create(:personal_access_token, user: user) }
+
+        context 'when revocation succeeds' do
+          before do
+            PersonalAccessTokens::RevokeService.new(admin, token: personal_access_token).execute
+          end
+
+          let(:message) { "Revoked personal access token with id #{personal_access_token.id}" }
+
+          it_behaves_like 'personal access token audit event'
+        end
+
+        context 'when revocation fails' do
+          let(:message) { "Attempted to revoke personal access token with id #{personal_access_token.id} but failed with message: error" }
+
+          before do
+            allow_any_instance_of(ServiceResponse).to receive(:success?).and_return(false)
+            allow_any_instance_of(ServiceResponse).to receive(:message).and_return('error')
+            PersonalAccessTokens::RevokeService.new(admin, token: personal_access_token).execute
+          end
+
+          it_behaves_like 'personal access token audit event'
+        end
+      end
+    end
+
     describe 'impersonated events' do
       it 'show impersonation details' do
         visit admin_user_path(user)

ee/spec/services/personal_access_tokens/create_service_audit_log_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe PersonalAccessTokens::CreateService do
+  describe '#execute' do
+    subject { service.execute }
+
+    let(:user) { create(:user) }
+    let(:params) { { name: 'Test token', impersonation: true, scopes: [:api], expires_at: Date.today + 1.month } }
+    let(:service) { described_class.new(current_user: user, target_user: user, params: params) }
+
+    it 'creates audit logs' do
+      expect(::AuditEventService)
+        .to receive(:new)
+        .with(user, user, action: :custom, custom_message: /Created personal access token with id \d+/, ip_address: nil)
+        .and_call_original
+
+      subject
+    end
+  end
+end

ee/spec/services/personal_access_tokens/revoke_service_audit_log_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe PersonalAccessTokens::RevokeService do
+  describe '#execute' do
+    subject { service.execute }
+
+    let(:user) { create(:user) }
+    let(:token) { create(:personal_access_token, user: user) }
+    let(:service) { described_class.new(user, token: token) }
+
+    it 'creates audit logs' do
+      expect(::AuditEventService)
+        .to receive(:new)
+        .with(user, user, action: :custom, custom_message: "Revoked personal access token with id #{token.id}")
+        .and_call_original
+
+      subject
+    end
+  end
+end

lib/api/internal/base.rb

@@ -245,7 +245,7 @@ module API
           end
 
           result = ::PersonalAccessTokens::CreateService.new(
-            user, name: params[:name], scopes: params[:scopes], expires_at: expires_at
+            current_user: user, target_user: user, params: { name: params[:name], scopes: params[:scopes], expires_at: expires_at }
           ).execute
 
           unless result.status == :success

lib/api/personal_access_tokens.rb

@@ -51,7 +51,7 @@ module API
       delete ':id' do
         service = ::PersonalAccessTokens::RevokeService.new(
           current_user,
-          { token: find_token(params[:id]) }
+          token: find_token(params[:id])
         ).execute
 
         service.success? ? no_content! : bad_request!(nil)

spec/features/profiles/personal_access_tokens_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe 'Profile > Personal Access Tokens', :js do
   let(:user) { create(:user) }
+  let(:pat_create_service) { double('PersonalAccessTokens::CreateService', execute: ServiceResponse.error(message: 'error', payload: { personal_access_token: PersonalAccessToken.new })) }
 
   def active_personal_access_tokens
     find(".table.active-tokens")
@@ -18,7 +19,7 @@ RSpec.describe 'Profile > Personal Access Tokens', :js do
   end
 
   def disallow_personal_access_token_saves!
-    allow_any_instance_of(PersonalAccessToken).to receive(:save).and_return(false)
+    allow(PersonalAccessTokens::CreateService).to receive(:new).and_return(pat_create_service)
 
     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)
@@ -100,7 +101,10 @@ 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(PersonalAccessTokens::RevokeService).to receive(:revocation_permitted?).and_return(false)
+
+        allow_next_instance_of(PersonalAccessTokens::RevokeService) do |instance|
+          allow(instance).to receive(:revocation_permitted?).and_return(false)
+        end
 
         accept_confirm { click_on "Revoke" }
         expect(active_personal_access_tokens).to have_text(personal_access_token.name)

spec/policies/user_policy_spec.rb

@@ -40,6 +40,46 @@ RSpec.describe UserPolicy do
     end
   end
 
+  describe "creating a different user's Personal Access Tokens" do
+    context 'when current_user is admin' do
+      let(:current_user) { create(:user, :admin) }
+
+      context 'when admin mode is enabled and current_user is not blocked', :enable_admin_mode do
+        it { is_expected.to be_allowed(:create_user_personal_access_token) }
+      end
+
+      context 'when admin mode is enabled and current_user is blocked', :enable_admin_mode do
+        let(:current_user) { create(:admin, :blocked) }
+
+        it { is_expected.not_to be_allowed(:create_user_personal_access_token) }
+      end
+
+      context 'when admin mode is disabled' do
+        it { is_expected.not_to be_allowed(:create_user_personal_access_token) }
+      end
+    end
+
+    context 'when current_user is not an admin' do
+      context 'creating their own personal access tokens' do
+        subject { described_class.new(current_user, current_user) }
+
+        context 'when current_user is not blocked' do
+          it { is_expected.to be_allowed(:create_user_personal_access_token) }
+        end
+
+        context 'when current_user is blocked' do
+          let(:current_user) { create(:user, :blocked) }
+
+          it { is_expected.not_to be_allowed(:create_user_personal_access_token) }
+        end
+      end
+
+      context "creating a different user's personal access tokens" do
+        it { is_expected.not_to be_allowed(:create_user_personal_access_token) }
+      end
+    end
+  end
+
   shared_examples 'changing a user' do |ability|
     context "when a regular user tries to destroy another regular user" do
       it { is_expected.not_to be_allowed(ability) }

spec/services/personal_access_tokens/create_service_spec.rb

@@ -3,21 +3,53 @@
 require 'spec_helper'
 
 RSpec.describe PersonalAccessTokens::CreateService do
+  shared_examples_for 'a successfully created token' do
+    it 'creates personal access token record' do
+      expect(subject.success?).to be true
+      expect(token.name).to eq(params[:name])
+      expect(token.impersonation).to eq(params[:impersonation])
+      expect(token.scopes).to eq(params[:scopes])
+      expect(token.expires_at).to eq(params[:expires_at])
+      expect(token.user).to eq(user)
+    end
+
+    it 'logs the event' do
+      expect(Gitlab::AppLogger).to receive(:info).with(/PAT CREATION: created_by: '#{current_user.username}', created_for: '#{user.username}', token_id: '\d+'/)
+
+      subject
+    end
+  end
+
+  shared_examples_for 'an unsuccessfully created token' do
+    it { expect(subject.success?).to be false }
+    it { expect(subject.message).to eq('Not permitted to create') }
+    it { expect(token).to be_nil }
+  end
+
   describe '#execute' do
-    context 'with valid params' do
-      it 'creates personal access token record' do
-        user = create(:user)
-        params = { name: 'Test token', impersonation: true, scopes: [:api], expires_at: Date.today + 1.month }
-
-        response = described_class.new(user, params).execute
-        personal_access_token = response.payload[:personal_access_token]
-
-        expect(response.success?).to be true
-        expect(personal_access_token.name).to eq(params[:name])
-        expect(personal_access_token.impersonation).to eq(params[:impersonation])
-        expect(personal_access_token.scopes).to eq(params[:scopes])
-        expect(personal_access_token.expires_at).to eq(params[:expires_at])
-        expect(personal_access_token.user).to eq(user)
+    subject { service.execute }
+
+    let(:current_user) { create(:user) }
+    let(:user) { create(:user) }
+    let(:params) { { name: 'Test token', impersonation: false, scopes: [:api], expires_at: Date.today + 1.month } }
+    let(:service) { described_class.new(current_user: current_user, target_user: user, params: params) }
+    let(:token) { subject.payload[:personal_access_token] }
+
+    context 'when current_user is an administrator' do
+      let(:current_user) { create(:admin) }
+
+      it_behaves_like 'a successfully created token'
+    end
+
+    context 'when current_user is not an administrator' do
+      context 'target_user is not the same as current_user' do
+        it_behaves_like 'an unsuccessfully created token'
+      end
+
+      context 'target_user is same as current_user' do
+        let(:current_user) { user }
+
+        it_behaves_like 'a successfully created token'
       end
     end
   end

spec/services/personal_access_tokens/revoke_service_spec.rb

@@ -6,6 +6,11 @@ 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 }
+    it 'logs the event' do
+      expect(Gitlab::AppLogger).to receive(:info).with(/PAT REVOCATION: revoked_by: '#{current_user.username}', revoked_for: '#{token.user.username}', token_id: '\d+'/)
+
+      subject
+    end
   end
 
   shared_examples_for 'an unsuccessfully revoked token' do