Merge branch 'sfang-token-read-write-permissions' into 'master'

Create new policies for read, destroy, and create tokens

See merge request gitlab-org/gitlab!56464

app/controllers/groups_controller.rb

@@ -268,7 +268,8 @@ class GroupsController < Groups::ApplicationController
       :subgroup_creation_level,
       :default_branch_protection,
       :default_branch_name,
-      :allow_mfa_for_subgroups
+      :allow_mfa_for_subgroups,
+      :resource_access_token_creation_allowed
     ]
   end
 

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_permission(:read_resource_access_tokens) }, only: [:index]
+      before_action -> { check_permission(:destroy_resource_access_tokens) }, only: [:revoke]
+      before_action -> { check_permission(:create_resource_access_tokens) }, only: [:create]
 
       feature_category :authentication_and_authorization
 
@@ -43,8 +45,8 @@ module Projects
 
       private
 
-      def check_feature_availability
-        render_404 unless project_access_token_available?(@project)
+      def check_permission(action)
+        render_404 unless can?(current_user, action, @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, :admin_resource_access_tokens, project)
-  end
-
   def build_project_breadcrumb_link(project)
     project_name = simple_sanitize(project.name)
 

app/models/group.rb

@@ -119,6 +119,7 @@ class Group < Namespace
   end
 
   delegate :default_branch_name, to: :namespace_settings
+  delegate :resource_access_token_creation_allowed, :resource_access_token_creation_allowed=, :resource_access_token_creation_allowed?, to: :namespace_settings
 
   class << self
     def sort_by_attribute(method)

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,8 +214,13 @@ class GroupPolicy < BasePolicy
   rule { developer & dependency_proxy_available }
     .enable :admin_dependency_proxy
 
-  rule { resource_access_token_available & can?(:admin_group) }.policy do
-    enable :admin_resource_access_tokens
+  rule { can?(:admin_group) & resource_access_token_feature_available }.policy do
+    enable :read_resource_access_tokens
+    enable :destroy_resource_access_tokens
+  end
+
+  rule { resource_access_token_creation_allowed & can?(:read_resource_access_tokens) }.policy do
+    enable :create_resource_access_tokens
   end
 
   rule { support_bot & has_project_with_service_desk_enabled }.policy do
@@ -242,9 +248,13 @@ class GroupPolicy < BasePolicy
     @subject
   end
 
-  def resource_access_token_available?
+  def resource_access_token_feature_available?
     true
   end
+
+  def resource_access_token_creation_allowed?
+    group.resource_access_token_creation_allowed?
+  end
 end
 
 GroupPolicy.prepend_if_ee('EE::GroupPolicy')

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,11 +633,18 @@ class ProjectPolicy < BasePolicy
 
   rule { project_bot }.enable :project_bot_access
 
-  rule { resource_access_token_available & can?(:admin_project) }.policy do
-    enable :admin_resource_access_tokens
+  rule { can?(:admin_project) & resource_access_token_feature_available }.policy do
+    enable :read_resource_access_tokens
+    enable :destroy_resource_access_tokens
   end
 
-  rule { can?(:project_bot_access) }.prevent :admin_resource_access_tokens
+  rule { can?(:read_resource_access_tokens) & resource_access_token_creation_allowed }.policy do
+    enable :create_resource_access_tokens
+  end
+
+  rule { can?(:project_bot_access) }.policy do
+    prevent :create_resource_access_tokens
+  end
 
   rule { user_defined_variables_allowed | can?(:maintainer_access) }.policy do
     enable :set_pipeline_variables
@@ -720,10 +728,18 @@ 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.resource_access_token_creation_allowed
+  end
+
   def project
     @subject
   end

app/services/resource_access_tokens/create_service.rb

@@ -39,7 +39,7 @@ module ResourceAccessTokens
     attr_reader :resource_type, :resource
 
     def has_permission_to_create?
-      %w(project group).include?(resource_type) && can?(current_user, :admin_resource_access_tokens, resource)
+      %w(project group).include?(resource_type) && can?(current_user, :create_resource_access_tokens, resource)
     end
 
     def create_user

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, :admin_project_member, @resource)
-      elsif resource.is_a?(Group)
-        can?(current_user, :admin_group_member, @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,

changelogs/unreleased/sfang-token-read-write-permissions.yml

+---
+title: Create new policies for read, destroy, and create tokens
+merge_request: 56464
+author:
+type: changed

db/migrate/20210312193532_add_resource_access_token_creation_allowed_to_namespace_settings.rb

+# frozen_string_literal: true
+class AddResourceAccessTokenCreationAllowedToNamespaceSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    with_lock_retries do
+      add_column :namespace_settings, :resource_access_token_creation_allowed, :boolean, default: true, null: false
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :namespace_settings, :resource_access_token_creation_allowed
+    end
+  end
+end

db/schema_migrations/20210312193532

+93e92e8eca0765cb8e6e08ec90ce0143d9b31d13e4d61e1b9690dbaed5a1bb63
\ No newline at end of file

db/structure.sql

@@ -14681,6 +14681,7 @@ CREATE TABLE namespace_settings (
     default_branch_name text,
     repository_read_only boolean DEFAULT false NOT NULL,
     delayed_project_removal boolean DEFAULT false NOT NULL,
+    resource_access_token_creation_allowed boolean DEFAULT true NOT NULL,
     CONSTRAINT check_0ba93c78c7 CHECK ((char_length(default_branch_name) <= 255))
 );
 

ee/app/policies/ee/group_policy.rb

@@ -391,12 +391,13 @@ 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?
-      return true unless ::Gitlab.com?
+    override :resource_access_token_feature_available?
+    def resource_access_token_feature_available?
+      value_from_super = super
 
-      ::Feature.enabled?(:resource_access_token_feature, group, default_enabled: true) &&
-        group.feature_available_non_trial?(:resource_access_token)
+      return value_from_super unless ::Gitlab.com?
+
+      value_from_super && group.feature_available_non_trial?(:resource_access_token)
     end
   end
 end

ee/app/policies/ee/project_policy.rb

@@ -423,9 +423,11 @@ 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?
-      return true unless ::Gitlab.com?
+    override :resource_access_token_feature_available?
+    def resource_access_token_feature_available?
+      value_from_super = super
+
+      return value_from_super unless ::Gitlab.com?
 
       group = project.namespace
 

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(:admin_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(:admin_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(:admin_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(:admin_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

lib/api/resource_access_tokens.rb

@@ -19,7 +19,7 @@ module API
         get ":id/access_tokens" do
           resource = find_source(source_type, params[:id])
 
-          next unauthorized! unless has_permission_to_read?(resource)
+          next unauthorized! unless current_user.can?(:read_resource_access_tokens, resource)
 
           tokens = PersonalAccessTokensFinder.new({ user: resource.bots, impersonation: false }).execute
 
@@ -85,10 +85,6 @@ module API
       def find_token(resource, token_id)
         PersonalAccessTokensFinder.new({ user: resource.bots, impersonation: false }).find_by_id(token_id)
       end
-
-      def has_permission_to_read?(resource)
-        can?(current_user, :project_bot_access, resource) || can?(current_user, :admin_resource_access_tokens, resource)
-      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/policies/project_policy_spec.rb

@@ -511,7 +511,7 @@ RSpec.describe ProjectPolicy do
         project.add_maintainer(project_bot)
       end
 
-      it { is_expected.not_to be_allowed(:admin_resource_access_tokens)}
+      it { is_expected.not_to be_allowed(:create_resource_access_tokens)}
     end
   end
 

spec/requests/api/resource_access_tokens_spec.rb

@@ -151,6 +151,23 @@ RSpec.describe API::ResourceAccessTokens do
           expect(User.exists?(project_bot.id)).to be_falsy
         end
 
+        context "when using project access token to DELETE other project access token" do
+          let_it_be(:other_project_bot) { create(:user, :project_bot) }
+          let_it_be(:other_token) { create(:personal_access_token, user: other_project_bot) }
+          let_it_be(:token_id) { other_token.id }
+
+          before do
+            project.add_maintainer(other_project_bot)
+          end
+
+          it "deletes the project access token from the project" do
+            delete_token
+
+            expect(response).to have_gitlab_http_status(:no_content)
+            expect(User.exists?(other_project_bot.id)).to be_falsy
+          end
+        end
+
         context "when attempting to delete a non-existent project access token" do
           let_it_be(:token_id) { non_existing_record_id }
 

spec/support/shared_examples/policies/resource_access_token_shared_examples.rb

@@ -5,16 +5,47 @@ RSpec.shared_examples 'Self-managed Core resource access tokens' do
     allow(::Gitlab).to receive(:com?).and_return(false)
   end
 
-  context 'with owner' do
+  context 'with owner access' do
     let(:current_user) { owner }
 
-    it { is_expected.to be_allowed(:admin_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 }
 
-    it { is_expected.not_to be_allowed(:admin_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
 
@@ -24,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(:admin_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