Merge branch '208412-fj-improve-group-feature-logic' into 'master'

Add more functionality to GroupFeature

See merge request gitlab-org/gitlab!82091

app/models/concerns/featurable.rb

@@ -50,7 +50,7 @@ module Featurable
     end
 
     def available_features
-      @available_features
+      @available_features || []
     end
 
     def access_level_attribute(feature)
@@ -74,6 +74,12 @@ module Featurable
       STRING_OPTIONS.key(level)
     end
 
+    def required_minimum_access_level(feature)
+      ensure_feature!(feature)
+
+      Gitlab::Access::GUEST
+    end
+
     def ensure_feature!(feature)
       feature = feature.model_name.plural if feature.respond_to?(:model_name)
       feature = feature.to_sym
@@ -91,8 +97,8 @@ module Featurable
     public_send(self.class.access_level_attribute(feature)) # rubocop:disable GitlabSecurity/PublicSend
   end
 
-  def feature_available?(feature, user)
-    get_permission(user, feature)
+  def feature_available?(feature, user = nil)
+    has_permission?(user, feature)
   end
 
   def string_access_level(feature)
@@ -115,4 +121,30 @@ module Featurable
   def feature_validation_exclusion
     []
   end
+
+  def has_permission?(user, feature)
+    case access_level(feature)
+    when DISABLED
+      false
+    when PRIVATE
+      member?(user, feature)
+    when ENABLED
+      true
+    when PUBLIC
+      true
+    else
+      true
+    end
+  end
+
+  def member?(user, feature)
+    return false unless user
+    return true if user.can_read_all_resources?
+
+    resource_member?(user, feature)
+  end
+
+  def resource_member?(user, feature)
+    raise NotImplementedError
+  end
 end

app/models/group.rb

@@ -106,6 +106,7 @@ class Group < Namespace
   has_one :crm_settings, class_name: 'Group::CrmSettings', inverse_of: :group
 
   accepts_nested_attributes_for :variables, allow_destroy: true
+  accepts_nested_attributes_for :group_feature, update_only: true
 
   validate :visibility_level_allowed_by_projects
   validate :visibility_level_allowed_by_sub_groups
@@ -835,6 +836,17 @@ class Group < Namespace
     end
   end
 
+  # Check for enabled features, similar to `Project#feature_available?`
+  # NOTE: We still want to keep this after removing `Namespace#feature_available?`.
+  override :feature_available?
+  def feature_available?(feature, user = nil)
+    if ::Groups::FeatureSetting.available_features.include?(feature)
+      group_feature.feature_available?(feature, user) # rubocop:disable Gitlab/FeatureAvailableUsage
+    else
+      super
+    end
+  end
+
   private
 
   def max_member_access(user_ids)

app/models/groups/feature_setting.rb

@@ -2,11 +2,23 @@
 
 module Groups
   class FeatureSetting < ApplicationRecord
+    include Featurable
+    extend ::Gitlab::Utils::Override
+
     self.primary_key = :group_id
     self.table_name = 'group_features'
 
     belongs_to :group
 
     validates :group, presence: true
+
+    private
+
+    override :resource_member?
+    def resource_member?(user, feature)
+      group.member?(user, ::Groups::FeatureSetting.required_minimum_access_level(feature))
+    end
   end
 end
+
+::Groups::FeatureSetting.prepend_mod_with('Groups::FeatureSetting')

app/models/namespace.rb

@@ -373,7 +373,7 @@ class Namespace < ApplicationRecord
   end
 
   # Deprecated, use #licensed_feature_available? instead. Remove once Namespace#feature_available? isn't used anymore.
-  def feature_available?(feature)
+  def feature_available?(feature, _user = nil)
     licensed_feature_available?(feature)
   end
 

app/models/project_feature.rb

@@ -3,6 +3,7 @@
 class ProjectFeature < ApplicationRecord
   include Featurable
   extend Gitlab::ConfigHelper
+  extend ::Gitlab::Utils::Override
 
   # When updating this array, make sure to update rubocop/cop/gitlab/feature_available_usage.rb as well.
   FEATURES = %i[
@@ -155,31 +156,14 @@ class ProjectFeature < ApplicationRecord
     %i(merge_requests_access_level builds_access_level).each(&validator)
   end
 
-  def get_permission(user, feature)
-    case access_level(feature)
-    when DISABLED
-      false
-    when PRIVATE
-      team_access?(user, feature)
-    when ENABLED
-      true
-    when PUBLIC
-      true
-    else
-      true
-    end
+  def feature_validation_exclusion
+    %i(pages)
   end
 
-  def team_access?(user, feature)
-    return unless user
-    return true if user.can_read_all_resources?
-
+  override :resource_member?
+  def resource_member?(user, feature)
     project.team.member?(user, ProjectFeature.required_minimum_access_level(feature))
   end
-
-  def feature_validation_exclusion
-    %i(pages)
-  end
 end
 
 ProjectFeature.prepend_mod_with('ProjectFeature')

ee/app/models/ee/groups/feature_setting.rb

+# frozen_string_literal: true
+
+module EE
+  module Groups
+    module FeatureSetting
+      extend ActiveSupport::Concern
+
+      EE_FEATURES = %i(wiki).freeze
+
+      prepended do
+        set_available_features(EE_FEATURES)
+
+        default_value_for :wiki_access_level, value: Featurable::ENABLED, allows_nil: false
+      end
+    end
+  end
+end

ee/app/policies/ee/group_policy.rb

@@ -357,7 +357,9 @@ module EE
       end
 
       desc "Group has wiki disabled"
-      condition(:wiki_disabled, score: 32) { !@subject.feature_available?(:group_wikis) }
+      condition(:wiki_disabled, score: 32) do
+        !@subject.licensed_feature_available?(:group_wikis) || !@subject.feature_available?(:wiki, @user)
+      end
 
       rule { wiki_disabled }.policy do
         prevent(*create_read_update_admin_destroy(:wiki))

ee/spec/models/ee/groups/feature_setting_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::FeatureSetting do
+  # rubocop:disable Gitlab/FeatureAvailableUsage
+  describe '#feature_available?' do
+    let_it_be_with_reload(:other_user) { create(:user) }
+    let_it_be(:user) { create(:user) }
+
+    let_it_be_with_reload(:group) do
+      create(:group) do |g|
+        g.add_guest(user)
+      end
+    end
+
+    let(:features) { %w(wiki) }
+
+    context 'when features are disabled' do
+      it 'returns false' do
+        update_all_group_features(group, features, Featurable::DISABLED)
+
+        features.each do |feature|
+          expect(group.feature_available?(feature.to_sym, user)).to eq(false), "#{feature} failed"
+        end
+      end
+    end
+
+    context 'when features are enabled only for group members' do
+      it 'returns false when user is not a group member' do
+        update_all_group_features(group, features, Featurable::PRIVATE)
+
+        features.each do |feature|
+          expect(group.feature_available?(feature.to_sym, other_user)).to eq(false), "#{feature} failed"
+        end
+      end
+
+      it 'returns true when user is a group member' do
+        update_all_group_features(group, features, Featurable::PRIVATE)
+
+        features.each do |feature|
+          expect(group.feature_available?(feature.to_sym, user)).to eq(true)
+        end
+      end
+
+      context 'when admin mode is enabled', :enable_admin_mode do
+        it 'returns true if user is an admin' do
+          other_user.update_attribute(:admin, true)
+
+          update_all_group_features(group, features, Featurable::PRIVATE)
+
+          features.each do |feature|
+            expect(group.feature_available?(feature.to_sym, other_user)).to eq(true)
+          end
+        end
+      end
+
+      context 'when admin mode is disabled' do
+        it 'returns false when user is an admin' do
+          other_user.update_attribute(:admin, true)
+
+          update_all_group_features(group, features, Featurable::PRIVATE)
+
+          features.each do |feature|
+            expect(group.feature_available?(feature.to_sym, other_user)).to eq(false), "#{feature} failed"
+          end
+        end
+      end
+    end
+
+    context 'when feature is enabled for everyone' do
+      it 'returns true' do
+        expect(group.feature_available?(:wiki)).to eq(true)
+      end
+    end
+
+    context 'when feature has any other value' do
+      it 'returns true' do
+        group.group_feature.update_attribute(:wiki_access_level, 200)
+
+        expect(group.feature_available?(:wiki)).to eq(true)
+      end
+    end
+
+    def update_all_group_features(group, features, value)
+      group_feature_attributes = features.to_h { |f| ["#{f}_access_level", value] }
+      group.group_feature.update!(group_feature_attributes)
+    end
+  end
+  # rubocop:enable Gitlab/FeatureAvailableUsage
+end

ee/spec/policies/group_policy_spec.rb

@@ -1512,17 +1512,8 @@ RSpec.describe GroupPolicy do
         enable_namespace_license_check!
       end
 
-      # We don't have feature toggles on groups yet, so we currently simulate
-      # this by stubbing the license check instead.
       def set_access_level(access_level)
-        case access_level
-        when ProjectFeature::ENABLED
-          stub_licensed_features(group_wikis: true)
-        when ProjectFeature::DISABLED
-          stub_licensed_features(group_wikis: false)
-        when ProjectFeature::PRIVATE
-          skip('Access level private is not supported yet for group wikis, see https://gitlab.com/gitlab-org/gitlab/-/issues/208412')
-        end
+        container.group_feature.update_attribute(:wiki_access_level, access_level)
       end
 
       context 'when the feature is not licensed on this group' do

spec/models/concerns/featurable_spec.rb

@@ -3,171 +3,101 @@
 require 'spec_helper'
 
 RSpec.describe Featurable do
-  let_it_be(:user) { create(:user) }
+  let!(:klass) do
+    Class.new(ApplicationRecord) do
+      include Featurable
 
-  let(:project) { create(:project) }
-  let(:feature_class) { subject.class }
-  let(:features) { feature_class::FEATURES }
+      self.table_name = 'project_features'
 
-  subject { project.project_feature }
+      set_available_features %i(feature1 feature2 feature3)
 
-  describe '.quoted_access_level_column' do
-    it 'returns the table name and quoted column name for a feature' do
-      expected = '"project_features"."issues_access_level"'
-
-      expect(feature_class.quoted_access_level_column(:issues)).to eq(expected)
-    end
-  end
+      def feature1_access_level
+        Featurable::DISABLED
+      end
 
-  describe '.access_level_attribute' do
-    it { expect(feature_class.access_level_attribute(:wiki)).to eq :wiki_access_level }
+      def feature2_access_level
+        Featurable::ENABLED
+      end
 
-    it 'raises error for unspecified feature' do
-      expect { feature_class.access_level_attribute(:unknown) }
-        .to raise_error(ArgumentError, /invalid feature: unknown/)
+      def feature3_access_level
+        Featurable::PRIVATE
+      end
     end
   end
 
-  describe '.set_available_features' do
-    let!(:klass) do
-      Class.new(ApplicationRecord) do
-        include Featurable
+  subject { klass.new }
 
-        self.table_name = 'project_features'
-
-        set_available_features %i(feature1 feature2)
+  describe '.set_available_features' do
+    it { expect(klass.available_features).to match_array [:feature1, :feature2, :feature3] }
+  end
 
-        def feature1_access_level
-          Featurable::DISABLED
-        end
+  describe '#*_enabled?' do
+    it { expect(subject.feature1_enabled?).to be_falsey }
+    it { expect(subject.feature2_enabled?).to be_truthy }
+  end
 
-        def feature2_access_level
-          Featurable::ENABLED
-        end
-      end
+  describe '.quoted_access_level_column' do
+    it 'returns the table name and quoted column name for a feature' do
+      expect(klass.quoted_access_level_column(:feature1)).to eq('"project_features"."feature1_access_level"')
     end
-
-    let!(:instance) { klass.new }
-
-    it { expect(klass.available_features).to eq [:feature1, :feature2] }
-    it { expect(instance.feature1_enabled?).to be_falsey }
-    it { expect(instance.feature2_enabled?).to be_truthy }
   end
 
-  describe '.available_features' do
-    it { expect(feature_class.available_features).to include(*features) }
+  describe '.access_level_attribute' do
+    it { expect(klass.access_level_attribute(:feature1)).to eq :feature1_access_level }
+
+    it 'raises error for unspecified feature' do
+      expect { klass.access_level_attribute(:unknown) }
+        .to raise_error(ArgumentError, /invalid feature: unknown/)
+    end
   end
 
   describe '#access_level' do
     it 'returns access level' do
-      expect(subject.access_level(:wiki)).to eq(subject.wiki_access_level)
+      expect(subject.access_level(:feature1)).to eq(subject.feature1_access_level)
     end
   end
 
   describe '#feature_available?' do
-    let(:features) { %w(issues wiki builds merge_requests snippets repository pages metrics_dashboard) }
-
     context 'when features are disabled' do
-      it "returns false" do
-        update_all_project_features(project, features, ProjectFeature::DISABLED)
-
-        features.each do |feature|
-          expect(project.feature_available?(feature.to_sym, user)).to eq(false), "#{feature} failed"
-        end
+      it 'returns false' do
+        expect(subject.feature_available?(:feature1)).to eq(false)
       end
     end
 
     context 'when features are enabled only for team members' do
-      it "returns false when user is not a team member" do
-        update_all_project_features(project, features, ProjectFeature::PRIVATE)
+      let_it_be(:user) { create(:user) }
 
-        features.each do |feature|
-          expect(project.feature_available?(feature.to_sym, user)).to eq(false), "#{feature} failed"
-        end
+      before do
+        expect(subject).to receive(:member?).and_call_original
       end
 
-      it "returns true when user is a team member" do
-        project.add_developer(user)
-
-        update_all_project_features(project, features, ProjectFeature::PRIVATE)
-
-        features.each do |feature|
-          expect(project.feature_available?(feature.to_sym, user)).to eq(true), "#{feature} failed"
+      context 'when user is not present' do
+        it 'returns false' do
+          expect(subject.feature_available?(:feature3)).to eq(false)
         end
       end
 
-      it "returns true when user is a member of project group" do
-        group = create(:group)
-        project = create(:project, namespace: group)
-        group.add_developer(user)
+      context 'when user can read all resources' do
+        it 'returns true' do
+          allow(user).to receive(:can_read_all_resources?).and_return(true)
 
-        update_all_project_features(project, features, ProjectFeature::PRIVATE)
-
-        features.each do |feature|
-          expect(project.feature_available?(feature.to_sym, user)).to eq(true), "#{feature} failed"
+          expect(subject.feature_available?(:feature3, user)).to eq(true)
         end
       end
 
-      context 'when admin mode is enabled', :enable_admin_mode do
-        it "returns true if user is an admin" do
-          user.update_attribute(:admin, true)
-
-          update_all_project_features(project, features, ProjectFeature::PRIVATE)
+      context 'when user cannot read all resources' do
+        it 'raises NotImplementedError exception' do
+          expect(subject).to receive(:resource_member?).and_call_original
 
-          features.each do |feature|
-            expect(project.feature_available?(feature.to_sym, user)).to eq(true), "#{feature} failed"
-          end
-        end
-      end
-
-      context 'when admin mode is disabled' do
-        it "returns false when user is an admin" do
-          user.update_attribute(:admin, true)
-
-          update_all_project_features(project, features, ProjectFeature::PRIVATE)
-
-          features.each do |feature|
-            expect(project.feature_available?(feature.to_sym, user)).to eq(false), "#{feature} failed"
-          end
+          expect { subject.feature_available?(:feature3, user) }.to raise_error(NotImplementedError)
         end
       end
     end
 
     context 'when feature is enabled for everyone' do
-      it "returns true" do
-        expect(project.feature_available?(:issues, user)).to eq(true)
+      it 'returns true' do
+        expect(subject.feature_available?(:feature2)).to eq(true)
       end
     end
   end
-
-  describe '#*_enabled?' do
-    let(:features) { %w(wiki builds merge_requests) }
-
-    it "returns false when feature is disabled" do
-      update_all_project_features(project, features, ProjectFeature::DISABLED)
-
-      features.each do |feature|
-        expect(project.public_send("#{feature}_enabled?")).to eq(false), "#{feature} failed"
-      end
-    end
-
-    it "returns true when feature is enabled only for team members" do
-      update_all_project_features(project, features, ProjectFeature::PRIVATE)
-
-      features.each do |feature|
-        expect(project.public_send("#{feature}_enabled?")).to eq(true), "#{feature} failed"
-      end
-    end
-
-    it "returns true when feature is enabled for everyone" do
-      features.each do |feature|
-        expect(project.public_send("#{feature}_enabled?")).to eq(true), "#{feature} failed"
-      end
-    end
-  end
-
-  def update_all_project_features(project, features, value)
-    project_feature_attributes = features.to_h { |f| ["#{f}_access_level", value] }
-    project.project_feature.update!(project_feature_attributes)
-  end
 end

spec/models/groups/feature_setting_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::FeatureSetting do
+  describe 'associations' do
+    it { is_expected.to belong_to(:group) }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:group) }
+  end
+end

spec/models/project_feature_spec.rb

@@ -5,8 +5,8 @@ require 'spec_helper'
 RSpec.describe ProjectFeature do
   using RSpec::Parameterized::TableSyntax
 
-  let(:project) { create(:project) }
-  let(:user) { create(:user) }
+  let_it_be_with_reload(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
 
   it { is_expected.to belong_to(:project) }
 
@@ -242,4 +242,95 @@ RSpec.describe ProjectFeature do
       end
     end
   end
+
+  # rubocop:disable Gitlab/FeatureAvailableUsage
+  describe '#feature_available?' do
+    let(:features) { ProjectFeature::FEATURES }
+
+    context 'when features are disabled' do
+      it 'returns false' do
+        update_all_project_features(project, features, ProjectFeature::DISABLED)
+
+        features.each do |feature|
+          expect(project.feature_available?(feature.to_sym, user)).to eq(false), "#{feature} failed"
+        end
+      end
+    end
+
+    context 'when features are enabled only for team members' do
+      it 'returns false when user is not a team member' do
+        update_all_project_features(project, features, ProjectFeature::PRIVATE)
+
+        features.each do |feature|
+          expect(project.feature_available?(feature.to_sym, user)).to eq(false), "#{feature} failed"
+        end
+      end
+
+      it 'returns true when user is a team member' do
+        project.add_developer(user)
+
+        update_all_project_features(project, features, ProjectFeature::PRIVATE)
+
+        features.each do |feature|
+          expect(project.feature_available?(feature.to_sym, user)).to eq(true)
+        end
+      end
+
+      it 'returns true when user is a member of project group' do
+        group = create(:group)
+        project = create(:project, namespace: group)
+        group.add_developer(user)
+
+        update_all_project_features(project, features, ProjectFeature::PRIVATE)
+
+        features.each do |feature|
+          expect(project.feature_available?(feature.to_sym, user)).to eq(true)
+        end
+      end
+
+      context 'when admin mode is enabled', :enable_admin_mode do
+        it 'returns true if user is an admin' do
+          user.update_attribute(:admin, true)
+
+          update_all_project_features(project, features, ProjectFeature::PRIVATE)
+
+          features.each do |feature|
+            expect(project.feature_available?(feature.to_sym, user)).to eq(true)
+          end
+        end
+      end
+
+      context 'when admin mode is disabled' do
+        it 'returns false when user is an admin' do
+          user.update_attribute(:admin, true)
+
+          update_all_project_features(project, features, ProjectFeature::PRIVATE)
+
+          features.each do |feature|
+            expect(project.feature_available?(feature.to_sym, user)).to eq(false), "#{feature} failed"
+          end
+        end
+      end
+    end
+
+    context 'when feature is enabled for everyone' do
+      it 'returns true' do
+        expect(project.feature_available?(:issues, user)).to eq(true)
+      end
+    end
+
+    context 'when feature has any other value' do
+      it 'returns true' do
+        project.project_feature.update_attribute(:issues_access_level, 200)
+
+        expect(project.feature_available?(:issues)).to eq(true)
+      end
+    end
+
+    def update_all_project_features(project, features, value)
+      project_feature_attributes = features.to_h { |f| ["#{f}_access_level", value] }
+      project.project_feature.update!(project_feature_attributes)
+    end
+  end
+  # rubocop:enable Gitlab/FeatureAvailableUsage
 end

spec/support/shared_examples/policies/wiki_policies_shared_examples.rb

@@ -107,10 +107,4 @@ RSpec.shared_examples 'model with wiki policies' do
       expect_disallowed(*disallowed_permissions)
     end
   end
-
-  # TODO: Remove this helper once we implement group features
-  # https://gitlab.com/gitlab-org/gitlab/-/issues/208412
-  def set_access_level(access_level)
-    raise NotImplementedError
-  end
 end