Render create token form if can create token

Modify project policy

Add a policy to check if token creation allowed

Remove project helper

Update group policy

Add group policy specs

Update group policy and revoke service

Fix sidebar permission

Change check can destroy token name

Address MR review comments

Add comment, condense policies

Rename resource access token available

To resource access token feature available

Revert "Address MR review comments"

This reverts commit e4d52ad855b9d3f672994bb5d3224293a5aecd1a.

Revert "Rename resource access token available"

This reverts commit 402b1a1e23713c1c4d1f3aa55c822230c5c40701.

Add before checks to controller

Need to check if can read, destroy, create

app/controllers/projects/settings/access_tokens_controller.rb

@@ -6,7 +6,9 @@ module Projects
       include ProjectsHelper
 
       layout 'project_settings'
-      before_action :check_feature_availability
+      before_action :check_read, only: [:index]
+      before_action :check_destroy, only: [:revoke]
+      before_action :check_create, only: [:create]
 
       feature_category :authentication_and_authorization
 
@@ -43,8 +45,16 @@ module Projects
 
       private
 
-      def check_feature_availability
-        render_404 unless project_access_token_available?(@project)
+      def check_read
+        render_404 unless can?(current_user, :read_resource_access_tokens, @project)
+      end
+
+      def check_destroy
+        render_404 unless can?(current_user, :destroy_resource_access_tokens, @project)
+      end
+
+      def check_create
+        render_404 unless can?(current_user, :create_resource_access_tokens, @project)
       end
 
       def create_params

app/helpers/projects_helper.rb

@@ -809,10 +809,6 @@ module ProjectsHelper
       can?(current_user, :destroy_container_image, project)
   end
 
-  def project_access_token_available?(project)
-    can?(current_user, :create_resource_access_tokens, project)
-  end
-
   def build_project_breadcrumb_link(project)
     project_name = simple_sanitize(project.name)
 

app/policies/group_policy.rb

@@ -61,7 +61,8 @@ class GroupPolicy < BasePolicy
   end
 
   with_scope :subject
-  condition(:resource_access_token_available) { resource_access_token_available? }
+  condition(:resource_access_token_feature_available) { resource_access_token_feature_available? }
+  condition(:resource_access_token_creation_allowed) { resource_access_token_creation_allowed? }
 
   with_scope :subject
   condition(:has_project_with_service_desk_enabled) { @subject.has_project_with_service_desk_enabled? }
@@ -213,15 +214,12 @@ class GroupPolicy < BasePolicy
   rule { developer & dependency_proxy_available }
     .enable :admin_dependency_proxy
 
-  rule { can?(:admin_group) }.policy do
+  rule { can?(:admin_group) & resource_access_token_feature_available }.policy do
     enable :read_resource_access_tokens
-  end
-
-  rule { can?(:admin_group) }.policy do
     enable :destroy_resource_access_tokens
   end
 
-  rule { resource_access_token_available & can?(:read_resource_access_tokens) }.policy do
+  rule { resource_access_token_creation_allowed & can?(:read_resource_access_tokens) }.policy do
     enable :create_resource_access_tokens
   end
 
@@ -250,8 +248,12 @@ class GroupPolicy < BasePolicy
     @subject
   end
 
-  def resource_access_token_available?
-    group.root_ancestor.resource_access_token_creation_allowed?
+  def resource_access_token_feature_available?
+    true
+  end
+
+  def resource_access_token_creation_allowed?
+    group.resource_access_token_creation_allowed?
   end
 end
 

app/policies/project_policy.rb

@@ -108,7 +108,8 @@ class ProjectPolicy < BasePolicy
   condition(:service_desk_enabled) { @subject.service_desk_enabled? }
 
   with_scope :subject
-  condition(:resource_access_token_available) { resource_access_token_available? }
+  condition(:resource_access_token_feature_available) { resource_access_token_feature_available? }
+  condition(:resource_access_token_creation_allowed) { resource_access_token_creation_allowed? }
 
   # We aren't checking `:read_issue` or `:read_merge_request` in this case
   # because it could be possible for a user to see an issuable-iid
@@ -632,15 +633,12 @@ class ProjectPolicy < BasePolicy
 
   rule { project_bot }.enable :project_bot_access
 
-  rule { can?(:admin_project) }.policy do
+  rule { can?(:admin_project) & resource_access_token_feature_available }.policy do
     enable :read_resource_access_tokens
-  end
-
-  rule { can?(:admin_project) }.policy do
     enable :destroy_resource_access_tokens
   end
 
-  rule { resource_access_token_available & can?(:read_resource_access_tokens) }.policy do
+  rule { can?(:read_resource_access_tokens) & resource_access_token_creation_allowed }.policy do
     enable :create_resource_access_tokens
   end
 
@@ -730,12 +728,16 @@ class ProjectPolicy < BasePolicy
     end
   end
 
-  def resource_access_token_available?
+  def resource_access_token_feature_available?
+    true
+  end
+
+  def resource_access_token_creation_allowed?
     group = project.group
 
     return true unless group # always enable for projects in personal namespaces
 
-    group.root_ancestor.resource_access_token_creation_allowed?
+    group.resource_access_token_creation_allowed
   end
 
   def project

app/services/resource_access_tokens/revoke_service.rb

@@ -14,7 +14,7 @@ module ResourceAccessTokens
     end
 
     def execute
-      return error("#{current_user.name} cannot delete #{bot_user.name}") unless can_destroy_bot_member?
+      return error("#{current_user.name} cannot delete #{bot_user.name}") unless can_destroy_token?
       return error("Failed to find bot user") unless find_member
 
       access_token.revoke!
@@ -37,14 +37,8 @@ module ResourceAccessTokens
       DeleteUserWorker.perform_async(current_user.id, bot_user.id, skip_authorization: true)
     end
 
-    def can_destroy_bot_member?
-      if resource.is_a?(Project)
-        can?(current_user, :destroy_resource_access_tokens, @resource)
-      elsif resource.is_a?(Group)
-        can?(current_user, :destroy_resource_access_tokens, @resource)
-      else
-        false
-      end
+    def can_destroy_token?
+      %w(project group).include?(resource.class.name.downcase) && can?(current_user, :destroy_resource_access_tokens, resource)
     end
 
     def find_member

app/views/layouts/nav/sidebar/_project.html.haml

@@ -407,7 +407,7 @@
                 = link_to project_hooks_path(@project), title: _('Webhooks'), data: { qa_selector: 'webhooks_settings_link' } do
                   %span
                     = _('Webhooks')
-              - if project_access_token_available?(@project)
+              - if can?(current_user, :read_resource_access_tokens, @project)
                 = nav_link(controller: [:access_tokens]) do
                   = link_to project_settings_access_tokens_path(@project), title: _('Access Tokens'), data: { qa_selector: 'access_tokens_settings_link' } do
                     %span

app/views/projects/settings/access_tokens/index.html.haml

@@ -20,12 +20,13 @@
           type: type,
           new_token_value: @new_project_access_token
 
-    = render 'shared/access_tokens/form',
-        type: type,
-        path: project_settings_access_tokens_path(@project),
-        token: @project_access_token,
-        scopes: @scopes,
-        prefix: :project_access_token
+    - if current_user.can?(:create_resource_access_tokens, @project)
+      = render 'shared/access_tokens/form',
+          type: type,
+          path: project_settings_access_tokens_path(@project),
+          token: @project_access_token,
+          scopes: @scopes,
+          prefix: :project_access_token
 
     = render 'shared/access_tokens/table',
         active_tokens: @active_project_access_tokens,

ee/app/policies/ee/group_policy.rb

@@ -391,8 +391,8 @@ module EE
     end
 
     # Available in Core for self-managed but only paid, non-trial for .com to prevent abuse
-    override :resource_access_token_available?
-    def resource_access_token_available?
+    override :resource_access_token_feature_available?
+    def resource_access_token_feature_available?
       value_from_super = super
       return value_from_super unless ::Gitlab.com?
 

ee/app/policies/ee/project_policy.rb

@@ -423,17 +423,14 @@ module EE
     end
 
     # Available in Core for self-managed but only paid, non-trial for .com to prevent abuse
-    override :resource_access_token_available?
-    def resource_access_token_available?
-      value_from_super = super
+    override :resource_access_token_feature_available?
+    def resource_access_token_feature_available?
+      return true unless ::Gitlab.com?
 
-      return value_from_super unless ::Gitlab.com?
+      group = project.namespace
 
-      if project.group
-        return value_from_super && project.group.feature_available_non_trial?(:resource_access_token)
-      end
-
-      project.namespace.feature_available_non_trial?(:resource_access_token)
+      ::Feature.enabled?(:resource_access_token_feature, group, default_enabled: true) &&
+        group.feature_available_non_trial?(:resource_access_token)
     end
   end
 end

ee/spec/policies/group_policy_spec.rb

@@ -1385,7 +1385,25 @@ RSpec.describe GroupPolicy do
           group.add_owner(owner)
         end
 
-        it { is_expected.to be_allowed(:create_resource_access_tokens) }
+        context 'create resource access tokens' do
+          it { is_expected.to be_allowed(:create_resource_access_tokens) }
+
+          context 'when resource access token creation is not allowed' do
+            before do
+              group.namespace_settings.update_column(:resource_access_token_creation_allowed, false)
+            end
+
+            it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
+          end
+        end
+
+        context 'read resource access tokens' do
+          it { is_expected.to be_allowed(:read_resource_access_tokens) }
+        end
+
+        context 'destroy resource access tokens' do
+          it { is_expected.to be_allowed(:destroy_resource_access_tokens) }
+        end
       end
 
       context 'with developer' do
@@ -1395,7 +1413,17 @@ RSpec.describe GroupPolicy do
           group.add_developer(developer)
         end
 
-        it { is_expected.not_to be_allowed(:create_resource_access_tokens)}
+        context 'create resource access tokens' do
+          it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
+        end
+
+        context 'read resource access tokens' do
+          it { is_expected.not_to be_allowed(:read_resource_access_tokens) }
+        end
+
+        context 'destroy resource access tokens' do
+          it { is_expected.not_to be_allowed(:destroy_resource_access_tokens) }
+        end
       end
     end
   end

ee/spec/policies/project_policy_spec.rb

@@ -1577,24 +1577,55 @@ RSpec.describe ProjectPolicy do
         allow(::Gitlab).to receive(:com?).and_return(true)
       end
 
-      context 'with maintainer' do
+      context 'with maintainer access' do
         let(:current_user) { maintainer }
 
         before do
           project.add_maintainer(maintainer)
         end
 
-        it { is_expected.to be_allowed(:create_resource_access_tokens) }
+        context 'create resource access tokens' do
+          it { is_expected.to be_allowed(:create_resource_access_tokens) }
+
+          context 'when resource access token creation is not allowed' do
+            let(:group) { create(:group) }
+            let(:project) { create(:project, group: group) }
+
+            before do
+              group.namespace_settings.update_column(:resource_access_token_creation_allowed, false)
+            end
+
+            it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
+          end
+        end
+
+        context 'read resource access tokens' do
+          it { is_expected.to be_allowed(:read_resource_access_tokens) }
+        end
+
+        context 'destroy resource access tokens' do
+          it { is_expected.to be_allowed(:destroy_resource_access_tokens) }
+        end
       end
 
-      context 'with developer' do
+      context 'with developer access' do
         let(:current_user) { developer }
 
         before do
           project.add_developer(developer)
         end
 
-        it { is_expected.not_to be_allowed(:create_resource_access_tokens)}
+        context 'create resource access tokens' do
+          it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
+        end
+
+        context 'read resource access tokens' do
+          it { is_expected.not_to be_allowed(:read_resource_access_tokens) }
+        end
+
+        context 'destroy resource access tokens' do
+          it { is_expected.not_to be_allowed(:destroy_resource_access_tokens) }
+        end
       end
     end
   end

spec/features/projects/settings/access_tokens_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 RSpec.describe 'Project > Settings > Access Tokens', :js do
   let_it_be(:user) { create(:user) }
   let_it_be(:bot_user) { create(:user, :project_bot) }
-  let_it_be(:project) { create(:project) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, group: group) }
 
   before_all do
     project.add_maintainer(user)
@@ -57,6 +58,18 @@ RSpec.describe 'Project > Settings > Access Tokens', :js do
       expect(active_project_access_tokens).to have_text('read_api')
       expect(created_project_access_token).not_to be_empty
     end
+
+    context 'when token creation is not allowed' do
+      before do
+        group.namespace_settings.update_column(:resource_access_token_creation_allowed, false)
+      end
+
+      it 'does not show project access token creation form' do
+        visit project_settings_access_tokens_path(project)
+
+        expect(page).not_to have_selector('.new_project_access_token')
+      end
+    end
   end
 
   describe 'active tokens' do

spec/support/shared_examples/policies/resource_access_token_shared_examples.rb

@@ -5,56 +5,45 @@ RSpec.shared_examples 'Self-managed Core resource access tokens' do
     allow(::Gitlab).to receive(:com?).and_return(false)
   end
 
-  context 'create resource access tokens' do
-    context 'with owner' do
-      let(:current_user) { owner }
+  context 'with owner access' do
+    let(:current_user) { owner }
 
+    context 'create resource access tokens' do
       it { is_expected.to be_allowed(:create_resource_access_tokens) }
-    end
-
-    context 'with developer' do
-      let(:current_user) { developer }
 
-      it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
-    end
+      context 'when resource access token creation is not allowed' do
+        let(:group) { create(:group) }
+        let(:project) { create(:project, group: group) }
 
-    context 'when resource access tokens are not available' do
-      let(:current_user) { owner }
-      let(:group) { create(:group) }
-      let(:project) { create(:project, group: group) }
+        before do
+          group.namespace_settings.update_column(:resource_access_token_creation_allowed, false)
+        end
 
-      before do
-        group.namespace_settings.update_column(:resource_access_token_creation_allowed, false)
+        it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
       end
-
-      it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
     end
-  end
-
-  context 'read resource access tokens' do
-    context 'with owner' do
-      let(:current_user) { owner }
 
+    context 'read resource access tokens' do
       it { is_expected.to be_allowed(:read_resource_access_tokens) }
     end
 
-    context 'with developer' do
-      let(:current_user) { developer }
-
-      it { is_expected.not_to be_allowed(:read_resource_access_tokens) }
+    context 'destroy resource access tokens' do
+      it { is_expected.to be_allowed(:destroy_resource_access_tokens) }
     end
   end
 
-  context 'destroy resource access tokens' do
-    context 'with owner' do
-      let(:current_user) { owner }
+  context 'with developer access' do
+    let(:current_user) { developer }
 
-      it { is_expected.to be_allowed(:destroy_resource_access_tokens) }
+    context 'create resource access tokens' do
+      it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
     end
 
-    context 'with developer' do
-      let(:current_user) { developer }
+    context 'read resource access tokens' do
+      it { is_expected.not_to be_allowed(:read_resource_access_tokens) }
+    end
 
+    context 'destroy resource access tokens' do
       it { is_expected.not_to be_allowed(:destroy_resource_access_tokens) }
     end
   end
@@ -66,9 +55,19 @@ RSpec.shared_examples 'GitLab.com Core resource access tokens' do
     stub_ee_application_setting(should_check_namespace_plan: true)
   end
 
-  context 'with owner' do
+  context 'with owner access' do
     let(:current_user) { owner }
 
-    it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
+    context 'create resource access tokens' do
+      it { is_expected.not_to be_allowed(:create_resource_access_tokens) }
+    end
+
+    context 'read resource access tokens' do
+      it { is_expected.not_to be_allowed(:read_resource_access_tokens) }
+    end
+
+    context 'destroy resource access tokens' do
+      it { is_expected.not_to be_allowed(:destroy_resource_access_tokens) }
+    end
   end
 end