Group member expires at update on rotate token

app/services/project_access_tokens/rotate_service.rb → app/services/resource_access_tokens/rotate_service.rb

 # frozen_string_literal: true
 
-module ProjectAccessTokens
+module ResourceAccessTokens
   class RotateService < ::PersonalAccessTokens::RotateService
     extend ::Gitlab::Utils::Override
 
     def initialize(current_user, token, resource = nil)
       @current_user = current_user
       @token = token
-      @project = resource
+      @resource = resource
     end
 
     def execute(params = {})
       super
     end
 
-    attr_reader :project
+    attr_reader :resource
 
     private
 
@@ -44,15 +44,34 @@ def update_bot_membership(target_user, expires_at)
     end
 
     def valid_access_level?
-      return true if current_user.can_admin_all_resources?
-      return false unless current_user.can?(:manage_resource_access_tokens, project)
+      return true if admin_all_resources?
+      return false unless can_manage_tokens?
 
-      token_access_level = project.team.max_member_access(token.user.id).to_i
-      current_user_access_level = project.team.max_member_access(current_user.id).to_i
+      token_access_level <= current_user_access_level
+    end
+
+    def admin_all_resources?
+      current_user.can_admin_all_resources?
+    end
+
+    def can_manage_tokens?
+      current_user.can?(:manage_resource_access_tokens, resource)
+    end
 
-      return true if token_access_level.to_i <= current_user_access_level
+    def token_access_level
+      if resource.is_a? Project
+        resource.team.max_member_access(token.user.id).to_i
+      else
+        resource.max_member_access_for_user(token.user).to_i
+      end
+    end
 
-      false
+    def current_user_access_level
+      if resource.is_a? Project
+        resource.team.max_member_access(current_user.id).to_i
+      else
+        resource.max_member_access_for_user(current_user).to_i
+      end
     end
   end
 end

lib/api/resource_access_tokens.rb

@@ -153,13 +153,8 @@ class ResourceAccessTokens < ::API::Base
           token = find_token(resource, params[:token_id]) if resource_accessible
 
           if token
-            response = if source_type == "project"
-                         ::ProjectAccessTokens::RotateService.new(current_user, token, resource)
+            response = ::ResourceAccessTokens::RotateService.new(current_user, token, resource)
                                    .execute(declared_params)
-                       else
-                         ::PersonalAccessTokens::RotateService.new(current_user, token)
-                                     .execute(declared_params)
-                       end
 
             if response.success?
               status :ok

spec/requests/api/resource_access_tokens_spec.rb

@@ -575,21 +575,14 @@
 
       context 'when service raises an error' do
         let(:error_message) { 'boom!' }
-        let(:personal_token_service) { PersonalAccessTokens::RotateService }
-        let(:project_token_service) { ProjectAccessTokens::RotateService }
+        let(:resource_token_service) { ResourceAccessTokens::RotateService }
 
         before do
           resource.add_maintainer(project_bot)
           resource.add_owner(user)
 
-          if source_type == 'project'
-            allow_next_instance_of(project_token_service) do |service|
-              allow(service).to receive(:execute).and_return(ServiceResponse.error(message: error_message))
-            end
-          else
-            allow_next_instance_of(personal_token_service) do |service|
-              allow(service).to receive(:execute).and_return(ServiceResponse.error(message: error_message))
-            end
+          allow_next_instance_of(resource_token_service) do |service|
+            allow(service).to receive(:execute).and_return(ServiceResponse.error(message: error_message))
           end
         end
 

spec/services/project_access_tokens/rotate_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-RSpec.describe ProjectAccessTokens::RotateService, feature_category: :system_access do
-  describe '#execute' do
-    let_it_be(:token, reload: true) { create(:personal_access_token) }
-    let(:current_user) { create(:user) }
-    let(:project) { create(:project, group: create(:group)) }
-    let(:error_message) { 'Not eligible to rotate token with access level higher than the user' }
-
-    subject(:response) { described_class.new(current_user, token, project).execute }
-
-    shared_examples_for 'rotates token succesfully' do
-      it "rotates user's own token", :freeze_time do
-        expect(response).to be_success
-
-        new_token = response.payload[:personal_access_token]
-
-        expect(new_token.token).not_to eq(token.token)
-        expect(new_token.expires_at).to eq(Date.today + 1.week)
-        expect(new_token.user).to eq(token.user)
-      end
-    end
-
-    context 'when user tries to rotate token with different access level' do
-      before do
-        project.add_guest(token.user)
-      end
-
-      context 'when current user is an owner' do
-        before do
-          project.add_owner(current_user)
-        end
-
-        it_behaves_like "rotates token succesfully"
-
-        context 'when creating the new token fails' do
-          let(:error_message) { 'boom!' }
-
-          before do
-            allow_next_instance_of(PersonalAccessToken) do |token|
-              allow(token).to receive_message_chain(:errors, :full_messages, :to_sentence).and_return(error_message)
-              allow(token).to receive_message_chain(:errors, :clear)
-              allow(token).to receive_message_chain(:errors, :empty?).and_return(false)
-            end
-          end
-
-          it 'returns an error' do
-            expect(response).to be_error
-            expect(response.message).to eq(error_message)
-          end
-
-          it 'reverts the changes' do
-            expect { response }.not_to change { token.reload.revoked? }.from(false)
-          end
-        end
-      end
-
-      context 'when current user is not an owner' do
-        context 'when current user is maintainer' do
-          before do
-            project.add_maintainer(current_user)
-          end
-
-          context 'when access level is not owner' do
-            it_behaves_like "rotates token succesfully"
-          end
-
-          context 'when access level is owner' do
-            before do
-              project.add_owner(token.user)
-            end
-
-            it "does not rotate token with higher priviledge" do
-              response
-
-              expect(response).to be_error
-              expect(response.message).to eq(error_message)
-            end
-          end
-        end
-
-        context 'when current user is not maintainer' do
-          before do
-            project.add_developer(current_user)
-          end
-
-          it 'does not rotate the token' do
-            response
-
-            expect(response).to be_error
-            expect(response.message).to eq(error_message)
-          end
-        end
-      end
-
-      context 'when current user is admin' do
-        let(:current_user) { create(:admin) }
-
-        context 'when admin mode enabled', :enable_admin_mode do
-          it_behaves_like "rotates token succesfully"
-        end
-
-        context 'when admin mode not enabled' do
-          it 'does not rotate the token' do
-            response
-
-            expect(response).to be_error
-            expect(response.message).to eq(error_message)
-          end
-        end
-      end
-
-      context 'when nested membership' do
-        let_it_be(:project_bot) { create(:user, :project_bot) }
-        let(:token) { create(:personal_access_token, user: project_bot) }
-        let(:top_level_group) { create(:group) }
-        let(:sub_group) { create(:group, parent: top_level_group) }
-        let(:project) { create(:project, group: sub_group) }
-
-        before do
-          project.add_maintainer(project_bot)
-        end
-
-        context 'when current user is an owner' do
-          before do
-            project.add_owner(current_user)
-          end
-
-          it_behaves_like "rotates token succesfully"
-
-          context 'when its a bot user' do
-            let_it_be(:bot_user) { create(:user, :project_bot) }
-            let_it_be(:bot_user_membership) do
-              create(:project_member, :developer, user: bot_user, project: create(:project))
-            end
-
-            let_it_be(:token, reload: true) { create(:personal_access_token, user: bot_user) }
-
-            it 'updates membership expires at' do
-              response
-
-              new_token = response.payload[:personal_access_token]
-              expect(bot_user_membership.reload.expires_at).to eq(new_token.expires_at)
-            end
-          end
-        end
-
-        context 'when current user is not an owner' do
-          context 'when current user is maintainer' do
-            before do
-              project.add_maintainer(current_user)
-            end
-
-            context 'when access level is not owner' do
-              it_behaves_like "rotates token succesfully"
-            end
-
-            context 'when access level is owner' do
-              before do
-                project.add_owner(token.user)
-              end
-
-              it "does not rotate token with higher priviledge" do
-                response
-
-                expect(response).to be_error
-                expect(response.message).to eq(error_message)
-              end
-            end
-          end
-
-          context 'when current user is not maintainer' do
-            before do
-              project.add_developer(current_user)
-            end
-
-            it 'does not rotate the token' do
-              response
-
-              expect(response).to be_error
-              expect(response.message).to eq(error_message)
-            end
-          end
-        end
-      end
-    end
-  end
-end

spec/services/resource_access_tokens/rotate_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ResourceAccessTokens::RotateService, feature_category: :system_access do
+  shared_examples_for 'rotates token successfully' do
+    it "rotates user's own token", :freeze_time do
+      expect(response).to be_success
+
+      new_token = response.payload[:personal_access_token]
+
+      expect(new_token.token).not_to eq(token.token)
+      expect(new_token.expires_at).to eq(Date.today + 1.week)
+      expect(new_token.user).to eq(token.user)
+    end
+
+    it 'updates membership expires_at' do
+      response
+
+      new_token = response.payload[:personal_access_token]
+      expect(bot_user.members.first.reload.expires_at).to eq(new_token.expires_at)
+    end
+  end
+
+  shared_examples 'token rotation access level check' do |source_type|
+    before do
+      resource.add_guest(token.user)
+    end
+
+    context 'when current user is an owner' do
+      before do
+        resource.add_owner(current_user)
+      end
+
+      it_behaves_like "rotates token successfully"
+
+      context 'when creating the new token fails' do
+        let(:error_message) { 'boom!' }
+
+        before do
+          allow_next_instance_of(PersonalAccessToken) do |token|
+            allow(token).to receive_message_chain(:errors, :full_messages, :to_sentence).and_return(error_message)
+            allow(token).to receive_message_chain(:errors, :clear)
+            allow(token).to receive_message_chain(:errors, :empty?).and_return(false)
+          end
+        end
+
+        it 'returns an error' do
+          expect(response).to be_error
+          expect(response.message).to eq(error_message)
+        end
+
+        it 'reverts the changes' do
+          expect { response }.not_to change { token.reload.revoked? }.from(false)
+        end
+      end
+    end
+
+    context 'when current user is maintainer' do
+      before do
+        resource.add_maintainer(current_user)
+      end
+
+      context 'and token user is not owner' do
+        if source_type == 'project'
+          it_behaves_like "rotates token successfully"
+        elsif source_type == 'group'
+          it 'cannot rotate the token' do
+            response
+
+            expect(response).to be_error
+            expect(response.message).to eq(error_message)
+          end
+        end
+      end
+
+      context 'and token user is owner' do
+        before do
+          resource.add_owner(token.user)
+        end
+
+        it "cannot rotate token with higher privilege" do
+          response
+
+          expect(response).to be_error
+          expect(response.message).to eq(error_message)
+        end
+      end
+    end
+
+    context 'when current user is neither owner or maintainer' do
+      before do
+        resource.add_developer(current_user)
+      end
+
+      it 'cannot rotate the token' do
+        response
+
+        expect(response).to be_error
+        expect(response.message).to eq(error_message)
+      end
+    end
+
+    context 'when current user is admin' do
+      let(:current_user) { create(:admin) }
+
+      context 'with admin mode', :enable_admin_mode do
+        it_behaves_like "rotates token successfully"
+      end
+
+      context 'without admin mode' do
+        it 'cannot rotate the token' do
+          response
+
+          expect(response).to be_error
+          expect(response.message).to eq(error_message)
+        end
+      end
+    end
+  end
+
+  describe '#execute' do
+    subject(:response) { described_class.new(current_user, token, resource).execute }
+
+    let(:current_user) { create(:user) }
+    let(:error_message) { 'Not eligible to rotate token with access level higher than the user' }
+    let(:bot_user) { create(:user, :project_bot) }
+    let(:token) { create(:personal_access_token, user: bot_user) }
+
+    context 'for project' do
+      let_it_be(:resource) { create(:project, group: create(:group)) }
+
+      it_behaves_like 'token rotation access level check', 'project'
+
+      context 'with a nested membership' do
+        let(:top_level_group) { create(:group) }
+        let(:sub_group) { create(:group, parent: top_level_group) }
+        let(:resource) { create(:project, group: sub_group) }
+
+        it_behaves_like 'token rotation access level check', 'project'
+      end
+    end
+
+    context 'for group' do
+      let(:resource) { create(:group) }
+
+      it_behaves_like 'token rotation access level check', 'group'
+
+      context 'with a nested membership' do
+        let(:top_level_group) { create(:group) }
+        let(:resource) { create(:group, parent: top_level_group) }
+
+        it_behaves_like 'token rotation access level check', 'group'
+      end
+    end
+  end
+end