Merge branch '514909-nwittstruck-admin-token-api-delete-oauth-application-secrets' into 'master'

Admin Token API: Reset OAuth Application Secrets

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179035



Merged-by: Vitali Tatarintev <vtatarintev@gitlab.com>
Approved-by: Amr Taha <ataha@gitlab.com>
Approved-by: Vitali Tatarintev <vtatarintev@gitlab.com>
Reviewed-by: Isaac Durham <idurham@gitlab.com>
Reviewed-by: Amr Taha <ataha@gitlab.com>
Co-authored-by: Nicholas Wittstruck <1494491-nwittstruck@users.noreply.gitlab.com>

app/services/authz/applications/reset_secret_service.rb

+# frozen_string_literal: true
+
+module Authz
+  module Applications
+    class ResetSecretService
+      attr_reader :application, :current_user
+
+      def initialize(application:, current_user:)
+        @application = application
+        @current_user = current_user
+      end
+
+      def execute
+        return error(message: "#{current_user.name} cannot reset secret") unless can_reset_secret?(current_user)
+
+        application.renew_secret
+
+        return ServiceResponse.success if application.save
+
+        error(message: "Couldn't save application")
+      end
+
+      private
+
+      def error(message:)
+        ServiceResponse.error(message: message)
+      end
+
+      def can_reset_secret?(current_user)
+        current_user.can_admin_all_resources?
+      end
+    end
+  end
+end

doc/api/admin/token.md

@@ -108,6 +108,7 @@ Example response:
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/170421) in GitLab 17.7 [with a flag](../../administration/feature_flags.md) named `api_admin_token_revoke`. Disabled by default.
 > - [Cluster agent tokens added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/178211) in GitLab 17.9.
 > - [Runner authentication tokens added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179066) in GitLab 17.9.
+> - [OAuth application secrets added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179035) in GitLab 17.9.
 
 FLAG:
 The availability of this feature is controlled by a feature flag.
@@ -116,15 +117,16 @@ This feature is available for testing, but not ready for production use.
 
 Revokes or resets a given token based on the token type. This endpoint supports the following token types:
 
-| Token type                                                                                  | Supported action   |
-|---------------------------------------------------------------------------------------------|--------------------|
-| [Personal access tokens](../../user/profile/personal_access_tokens.md)                      | Revoke             |
+| Token type                                                                                   | Supported action   |
+|----------------------------------------------------------------------------------------------|--------------------|
+| [Personal access tokens](../../user/profile/personal_access_tokens.md)                       | Revoke             |
 | [Project access tokens](../../security/tokens/_index.md#project-access-tokens)               | Revoke             |
 | [Group access tokens](../../security/tokens/_index.md#group-access-tokens)                   | Revoke             |
-| [Deploy tokens](../../user/project/deploy_tokens/index.md)                                  | Revoke             |
+| [Deploy tokens](../../user/project/deploy_tokens/index.md)                                   | Revoke             |
 | [Cluster agent tokens](../../security/tokens/_index.md#gitlab-cluster-agent-tokens)          | Revoke             |
 | [Feed tokens](../../security/tokens/_index.md#feed-token)                                    | Reset              |
 | [Runner authentication tokens](../../security/tokens/_index.md#runner-authentication-tokens) | Reset              |
+| [OAuth application secrets](../../integration/oauth_provider.md)                             | Reset              |
 
 ```plaintext
 DELETE /api/v4/admin/token

lib/authn/tokens/oauth_application_secret.rb

@@ -22,10 +22,13 @@ def present_with
         ::API::Entities::Application
       end
 
-      def revoke!(_current_user)
+      def revoke!(current_user)
         raise ::Authn::AgnosticTokenIdentifier::NotFoundError, 'Not Found' if revocable.blank?
 
-        raise ::Authn::AgnosticTokenIdentifier::UnsupportedTokenError, 'Revocation not supported for this token type'
+        Authz::Applications::ResetSecretService.new(
+          application: revocable,
+          current_user: current_user
+        ).execute
       end
     end
   end

spec/lib/authn/tokens/oauth_application_secret_spec.rb

@@ -4,6 +4,7 @@
 
 RSpec.describe Authn::Tokens::OauthApplicationSecret, feature_category: :system_access do
   let_it_be(:user) { create(:user) }
+  let_it_be(:admin) { create(:admin) }
 
   let(:oauth_application_secret) { create(:oauth_application) }
 
@@ -15,12 +16,31 @@
 
     it_behaves_like 'finding the valid revocable'
 
-    describe '#revoke!' do
-      it 'does not support revocation yet' do
-        expect do
-          token.revoke!(user)
-        end.to raise_error(::Authn::AgnosticTokenIdentifier::UnsupportedTokenError,
-          'Revocation not supported for this token type')
+    describe '#revoke!', :enable_admin_mode do
+      subject(:revoke) { described_class.new(plaintext, :api_admin_token).revoke!(current_user) }
+
+      context 'as admin' do
+        let(:current_user) { admin }
+
+        it 'successfully revokes the token' do
+          expect { revoke }.to change { oauth_application_secret.reload.secret }
+        end
+
+        it 'does support revocation' do
+          expect { revoke }.not_to raise_error
+        end
+      end
+
+      context 'as a user' do
+        let(:current_user) { user }
+
+        it 'does not reset the token' do
+          expect { revoke }.not_to change { oauth_application_secret.reload.secret }
+        end
+
+        it 'returns an error' do
+          expect(revoke.error?).to be_truthy
+        end
       end
     end
   end

spec/requests/api/admin/token_spec.rb

@@ -186,6 +186,16 @@
         end
       end
 
+      context 'when the token is an oauth application token' do
+        let(:plaintext) { oauth_application.plaintext_secret }
+
+        it 'resets the token' do
+          expect { delete_token }.to change { oauth_application.reload.secret }
+
+          expect(response).to have_gitlab_http_status(:no_content)
+        end
+      end
+
       context 'when the revocation feature is disabled' do
         before do
           stub_feature_flags(api_admin_token_revoke: false)

spec/services/authz/applications/reset_secret_service_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe ::Authz::Applications::ResetSecretService, :aggregate_failures, feature_category: :system_access do
+  let(:application) { create(:oauth_application) }
+
+  describe '#execute' do
+    subject(:service) { described_class.new(application: application, current_user: current_user) }
+
+    context 'as a user' do
+      let_it_be(:current_user) { create(:user) }
+
+      it 'does not change the secret' do
+        expect { service.execute }.not_to change { application.reload.secret }
+      end
+
+      it 'returns an error response' do
+        response = service.execute
+        expect(response.error?).to be_truthy
+        expect(response.message).to include('cannot reset secret')
+      end
+    end
+
+    context 'as an admin', :enable_admin_mode do
+      let_it_be(:current_user) { create(:admin) }
+
+      it 'returns a successful ServiceResponse' do
+        response = service.execute
+        expect(response).to be_kind_of(ServiceResponse)
+        expect(response.success?).to be_truthy
+      end
+
+      it 'changes the secret' do
+        expect { service.execute }.to change { application.reload.secret }
+      end
+
+      context 'when saving fails' do
+        before do
+          allow(application).to receive(:save).and_return(false)
+        end
+
+        it 'does not change the secret' do
+          expect { service.execute }.not_to change { application.reload.secret }
+        end
+      end
+    end
+  end
+end