Merge pull request #23041 from opf/role-given-error

Role for "Role given to a non-admin user who creates a project" must have certain permissions

app/forms/settings/new_project_settings_form.rb

@@ -51,19 +51,42 @@ module Settings
       f.select_list(
         name: :new_project_user_role_id,
         label: I18n.t(:setting_new_project_user_role_id),
+        caption: I18n.t(:setting_new_project_user_role_id_caption),
         input_width: :medium,
-        include_blank: I18n.t(:actionview_instancetag_blank_option)
+        include_blank: false
       ) do |select|
-        ProjectRole.givable.each do |role|
-          select.option(
-            value: role.id.to_s,
-            label: role.name,
-            selected: Setting.new_project_user_role_id == role.id
-          )
-        end
+        build_new_project_user_role_options(select)
       end
 
       f.submit
     end
+
+    # Adds the role options to the new_project_user_role_id select. Roles that pass the
+    # `assignable_to_project_creator` filter are listed first; the currently configured role is
+    # always included even when it has lost required permissions (with a label suffix), so the
+    # admin can see and change the current selection.
+    def build_new_project_user_role_options(select)
+      assignable = ProjectRole.assignable_to_project_creator.to_a
+      assignable.each { |role| add_assignable_role_option(select, role) }
+
+      configured = ProjectRole.givable.find_by(id: Setting.new_project_user_role_id)
+      add_non_qualifying_role_option(select, configured) if configured && assignable.exclude?(configured)
+    end
+
+    def add_assignable_role_option(select, role)
+      select.option(
+        value: role.id.to_s,
+        label: role.name,
+        selected: Setting.new_project_user_role_id == role.id
+      )
+    end
+
+    def add_non_qualifying_role_option(select, role)
+      select.option(
+        value: role.id.to_s,
+        label: I18n.t(:label_role_missing_permissions, role: role.name),
+        selected: true
+      )
+    end
   end
 end

app/models/project_role.rb

@@ -29,6 +29,20 @@
 # ++
 
 class ProjectRole < Role
+  # Permissions a role must grant in order to be assignable as the default
+  # role for a non-admin user who creates a project. Without these, the
+  # creator cannot complete project setup (filling out the PIR, adding
+  # members, etc.).
+  PERMISSIONS_FOR_PROJECT_CREATOR = %i[
+    view_project
+
+    view_project_attributes
+    edit_project_attributes
+
+    view_members
+    manage_members
+  ].freeze
+
   has_many :custom_fields_roles,
            foreign_key: "role_id",
            dependent: :restrict_with_error,
@@ -39,6 +53,20 @@ class ProjectRole < Role
       .where(type: "ProjectRole")
   end
 
+  # Roles eligible to be granted to a non-admin user upon project creation.
+  # Restricted to givable roles that include all PERMISSIONS_FOR_PROJECT_CREATOR.
+  def self.assignable_to_project_creator
+    permissions = PERMISSIONS_FOR_PROJECT_CREATOR.map(&:to_s)
+
+    role_ids = RolePermission
+                 .where(permission: permissions)
+                 .group(:role_id)
+                 .having("COUNT(DISTINCT permission) = ?", permissions.size)
+                 .select(:role_id)
+
+    givable.where(id: role_ids)
+  end
+
   # Return the builtin 'non member' role.  If the role doesn't exist,
   # it will be created on the fly.
   def self.non_member
@@ -66,9 +94,12 @@ class ProjectRole < Role
   end
 
   def self.in_new_project
-    givable
+    assignable_to_project_creator
       .except(:order)
-      .order(Arel.sql("COALESCE(#{Setting.new_project_user_role_id.to_i} = id, false) DESC, position"))
+      .reorder(Arel.sql(
+                 "COALESCE(#{Setting.new_project_user_role_id.to_i} = #{quoted_table_name}.id, false) DESC, " \
+                 "#{quoted_table_name}.position"
+               ))
       .first
   end
 end

app/views/roles/edit.html.erb

@@ -32,16 +32,29 @@ See COPYRIGHT and LICENSE files for more details.
 <%=
   render Primer::OpenProject::PageHeader.new do |header|
     header.with_title { @role.name }
-    header.with_breadcrumbs([{ href: admin_index_path, text: t("label_administration") },
-                             { href: admin_settings_users_path, text: t(:label_user_and_permission) },
-                             { href: roles_path, text: t(:label_role_and_permissions) },
-                             @role.name])
+    header.with_breadcrumbs(
+      [{ href: admin_index_path, text: t("label_administration") },
+       { href: admin_settings_users_path, text: t(:label_user_and_permission) },
+       { href: roles_path, text: t(:label_role_and_permissions) },
+       @role.name]
+    )
   end
 %>
 
-<%= labelled_tabular_form_for @role, :url => { :action => 'update' }, :html => {:id => 'role_form'}, :as => :role do |f| %>
-  <%= hidden_field_tag :id, @role.id %>
-  <%= render partial: 'form', locals: { f: f , role: @role } %>
-  <br/>
-  <%= styled_button_tag t(:button_save), class: '-with-icon icon-checkmark' %>
+<% if @role.is_a?(ProjectRole) && Setting.new_project_user_role_id.to_i == @role.id %>
+  <%= render(Primer::Alpha::Banner.new(scheme: :warning, icon: :alert, mb: 3)) do %>
+    <p><%= t("roles.edit.default_for_new_projects_warning") %></p>
+    <ul>
+      <% ProjectRole::PERMISSIONS_FOR_PROJECT_CREATOR.each do |permission| %>
+        <li><%= t("permission_#{permission}") %></li>
+      <% end %>
+    </ul>
+  <% end %>
+<% end %>
+
+<%= labelled_tabular_form_for @role, url: { action: "update" }, html: { id: "role_form" }, as: :role do |f| %>
+  <%= hidden_field_tag :id, @role.id %>
+  <%= render partial: "form", locals: { f: f, role: @role } %>
+  <br>
+  <%= styled_button_tag t(:button_save), class: "-with-icon icon-checkmark" %>
 <% end %>

config/locales/en.yml

@@ -1165,6 +1165,11 @@ en:
       no_results_content_text: Add a news item
 
   roles:
+    edit:
+      default_for_new_projects_warning: >-
+        This role is configured as the default role given to non-admin users who create a project.
+        Do not remove the following permissions, otherwise project creators will be unable to complete
+        the setup of their newly created projects:
     permissions:
       section_check_all_label: "Assign all %{module} permissions"
       section_uncheck_all_label: "Unassign all %{module} permissions"
@@ -4383,6 +4388,7 @@ en:
   label_role_new: "New role"
   label_role_grantable: "Grantable role"
   label_role_plural: "Roles"
+  label_role_missing_permissions: "%{role} (missing required permissions)"
   label_role_search: "Assign role to new members"
   label_scm: "SCM"
   label_scroll_left: "Scroll left"
@@ -5349,6 +5355,9 @@ en:
   setting_mail_handler_body_delimiter_regex: "Truncate emails matching this regex"
   setting_mail_handler_ignore_filenames: "Ignored mail attachments"
   setting_new_project_user_role_id: "Role given to a non-admin user who creates a project"
+  setting_new_project_user_role_id_caption: >
+    Only roles that include the permissions to edit project attributes and to manage members are listed,
+    so that the creator can complete the project setup.
   setting_new_project_send_confirmation_email: "Send notification to author when creating a new project"
   setting_new_project_notification_text: "Notification text"
   setting_password_active_rules: "Password requirements"

spec/controllers/admin/settings/new_project_settings_controller_spec.rb

@@ -82,6 +82,39 @@ RSpec.describe Admin::Settings::NewProjectSettingsController do
       end
     end
 
+    describe "default role for new projects" do
+      let!(:qualifying_role) { create(:project_creator_role, name: "Project lead") }
+      let!(:non_qualifying_role) do
+        create(:project_role, name: "Reader", permissions: %i[view_work_packages])
+      end
+
+      it "lists only roles with the required permissions" do
+        get "show", params: { tab: "settings" }
+
+        expect(response.body)
+          .to have_css("select[name='settings[new_project_user_role_id]'] option",
+                       text: qualifying_role.name)
+        expect(response.body)
+          .to have_no_css("select[name='settings[new_project_user_role_id]'] option",
+                          text: non_qualifying_role.name)
+      end
+
+      context "when the configured role no longer has the required permissions" do
+        before do
+          Setting.new_project_user_role_id = non_qualifying_role.id
+        end
+
+        it "still lists the configured role, marked as missing required permissions, and selected" do
+          get "show", params: { tab: "settings" }
+
+          expect(response.body)
+            .to have_css("select[name='settings[new_project_user_role_id]'] " \
+                         "option[selected][value='#{non_qualifying_role.id}']",
+                         text: "#{non_qualifying_role.name} (missing required permissions)")
+        end
+      end
+    end
+
     describe "project creation notifications" do
       it "contains a checkbox for sending confirmation emails in the notifications tab" do
         get "show", params: { tab: "notifications" }

spec/factories/project_role_factory.rb

@@ -56,6 +56,11 @@ FactoryBot.define do
       initialize_with { ProjectRole.where(builtin: Role::BUILTIN_ANONYMOUS).first_or_initialize }
     end
 
+    factory :project_creator_role do
+      name { "Project creator" }
+      permissions { ProjectRole::PERMISSIONS_FOR_PROJECT_CREATOR }
+    end
+
     factory :existing_project_role do
       name { "Role #{Digest::MD5.hexdigest(permissions.map(&:to_s).join('/'))[0..4]}" }
       permissions { [] }

spec/features/global_roles/global_create_project_spec.rb

@@ -74,6 +74,7 @@ RSpec.describe "Global role: Global Create project",
   describe "for a user with the global permission to add projects" do
     let!(:global_role) { create(:global_role, name: "Global", permissions: %i[add_project]) }
     let!(:member_role) { create(:project_role, name: "Member", permissions: %i[view_project]) }
+    let!(:default_project_role) { create(:project_creator_role) }
 
     let!(:global_member) do
       create(:global_member,
@@ -83,6 +84,10 @@ RSpec.describe "Global role: Global Create project",
 
     current_user { user }
 
+    before do
+      allow(Setting).to receive(:new_project_user_role_id).and_return(default_project_role.id.to_s)
+    end
+
     it 'allows creating projects via the "+ Project" button' do
       projects_page.visit!
       projects_page.create_new_workspace

spec/features/portfolios/create_spec.rb

@@ -38,9 +38,13 @@ RSpec.describe "Portfolios",
            global_permissions: :add_portfolios)
   end
   # Role granted to creator on portfolio creation to be able to access the portfolio.
-  shared_let(:default_project_role) { create(:project_role) }
+  shared_let(:default_project_role) { create(:project_creator_role) }
   shared_let(:add_subproject_role) { create(:project_role, permissions: %i[add_subprojects]) }
 
+  before do
+    allow(Setting).to receive(:new_project_user_role_id).and_return(default_project_role.id.to_s)
+  end
+
   let!(:root_portfolio) do
     create(:portfolio, name: "Root portfolio", members: { user_with_permissions => add_subproject_role })
   end

spec/features/programs/create_spec.rb

@@ -38,9 +38,13 @@ RSpec.describe "Programs",
            global_permissions: :add_programs)
   end
   # Role granted to creator on program creation to be able to access the program.
-  shared_let(:default_project_role) { create(:project_role) }
+  shared_let(:default_project_role) { create(:project_creator_role) }
   shared_let(:add_subproject_role) { create(:project_role, permissions: %i[add_subprojects]) }
 
+  before do
+    allow(Setting).to receive(:new_project_user_role_id).and_return(default_project_role.id.to_s)
+  end
+
   let!(:root_portfolio) do
     create(:portfolio, name: "Root portfolio", members: { user_with_permissions => add_subproject_role })
   end

spec/features/projects/creation_wizard/wizard_from_template_flow_spec.rb

@@ -43,8 +43,8 @@ RSpec.describe "Project creation wizard from a template",
 
   # Role assigned to users in newly created projects - only wizard permissions, NO add_work_packages
   shared_let(:new_project_role) do
-    create(:project_role,
-           permissions: %i[view_work_packages view_project_attributes edit_project_attributes])
+    create(:project_creator_role,
+           permissions: ProjectRole::PERMISSIONS_FOR_PROJECT_CREATOR + %i[view_work_packages])
   end
 
   # Role for the assignee user - assigned via the user custom field role assignment

spec/features/projects/subproject_creation_spec.rb

@@ -34,6 +34,7 @@ RSpec.describe "Subproject creation", :js do
   let(:parent_field) { FormFields::SelectFormField.new :parent }
   let(:add_subproject_role) { create(:project_role, permissions: %i[edit_project add_subprojects]) }
   let(:view_project_role) { create(:project_role, permissions: %i[edit_project]) }
+  let!(:default_project_role) { create(:project_creator_role) }
   let!(:parent_project) do
     create(:project,
            name: "Foo project",
@@ -50,6 +51,7 @@ RSpec.describe "Subproject creation", :js do
   end
 
   before do
+    allow(Setting).to receive(:new_project_user_role_id).and_return(default_project_role.id.to_s)
     visit project_settings_general_path(parent_project)
   end
 

spec/features/roles/edit_default_role_warning_spec.rb

@@ -0,0 +1,76 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+require "spec_helper"
+
+RSpec.describe "Editing a project role" do
+  let!(:admin) { create(:admin) }
+  let!(:default_role) { create(:project_creator_role) }
+  let!(:other_role) { create(:project_creator_role, name: "Other creator role") }
+
+  before do
+    login_as admin
+  end
+
+  context "when the role is configured as the default role for new projects" do
+    before do
+      allow(Setting).to receive(:new_project_user_role_id).and_return(default_role.id.to_s)
+    end
+
+    it "shows a warning banner listing the required permissions" do
+      visit edit_role_path(default_role)
+
+      expect(page).to have_text("default role given to non-admin users who create a project")
+      ProjectRole::PERMISSIONS_FOR_PROJECT_CREATOR.each do |permission|
+        expect(page).to have_css("ul li", text: I18n.t("permission_#{permission}"))
+      end
+    end
+
+    it "does not show the warning when editing a different role" do
+      visit edit_role_path(other_role)
+
+      expect(page).to have_button("Save")
+      expect(page).to have_no_text("default role given to non-admin users who create a project")
+    end
+  end
+
+  context "when no default role is configured" do
+    before do
+      allow(Setting).to receive(:new_project_user_role_id).and_return("")
+    end
+
+    it "does not show the warning" do
+      visit edit_role_path(default_role)
+
+      expect(page).to have_button("Save")
+      expect(page).to have_no_text("default role given to non-admin users who create a project")
+    end
+  end
+end

spec/models/project_role_spec.rb

@@ -42,21 +42,45 @@ RSpec.describe ProjectRole do
     it { expect(described_class.givable).to contain_exactly project_role }
   end
 
-  describe ".in_new_project" do
+  describe ".assignable_to_project_creator" do
     let!(:ungivable_role) { create(:non_member) }
+    let!(:role_with_all_perms) do
+      create(:project_role, permissions: ProjectRole::PERMISSIONS_FOR_PROJECT_CREATOR + %i[view_work_packages])
+    end
+    let!(:role_missing_one_perm) do
+      create(:project_role, permissions: [ProjectRole::PERMISSIONS_FOR_PROJECT_CREATOR.first])
+    end
+    let!(:role_with_unrelated_perms) do
+      create(:project_role, permissions: %i[view_work_packages])
+    end
+
+    it "includes only givable roles that grant all required permissions" do
+      expect(described_class.assignable_to_project_creator)
+        .to contain_exactly(role_with_all_perms)
+    end
+  end
+
+  describe ".in_new_project" do
+    let(:required_permissions) { ProjectRole::PERMISSIONS_FOR_PROJECT_CREATOR }
+    let!(:ungivable_role) { create(:non_member) }
+    let!(:role_without_required_permissions) do
+      create(:project_role, permissions: %i[view_work_packages]).tap do |r|
+        r.update_column(:position, 0)
+      end
+    end
     let!(:second_role) do
-      create(:project_role).tap do |r|
+      create(:project_role, permissions: required_permissions).tap do |r|
         r.update_column(:position, 100)
       end
     end
     let!(:first_role) do
-      create(:project_role).tap do |r|
+      create(:project_role, permissions: required_permissions).tap do |r|
         r.update_column(:position, 1)
       end
     end
 
     context "without a specified role" do
-      it "returns the first role (by position)" do
+      it "returns the first qualifying role (by position)" do
         expect(described_class.in_new_project)
           .to eql first_role
       end
@@ -82,11 +106,21 @@ RSpec.describe ProjectRole do
                 .and_return("-1")
       end
 
-      it "returns the first role (by position)" do
+      it "returns the first qualifying role (by position)" do
         expect(described_class.in_new_project)
           .to eql first_role
       end
     end
+
+    context "when no role has the required permissions" do
+      before do
+        described_class.where.not(id: role_without_required_permissions.id).destroy_all
+      end
+
+      it "returns nil" do
+        expect(described_class.in_new_project).to be_nil
+      end
+    end
   end
 
   describe ".anonymous" do

spec/requests/api/v3/projects/copy/copy_resource_spec.rb

@@ -53,6 +53,8 @@ RSpec.describe "API::V3::Projects::Copy::CopyAPI", content_type: :json, with_goo
   shared_let(:work_package) { create(:work_package, project: source_project) }
   shared_let(:wiki_page) { create(:wiki_page, wiki: source_project.wiki) }
 
+  shared_let(:project_creator_role) { create(:project_creator_role) }
+
   shared_let(:current_user) do
     create(:user,
            member_with_permissions: { source_project => %i[copy_projects
@@ -67,6 +69,7 @@ RSpec.describe "API::V3::Projects::Copy::CopyAPI", content_type: :json, with_goo
   end
 
   before do
+    allow(Setting).to receive(:new_project_user_role_id).and_return(project_creator_role.id.to_s)
     login_as(current_user)
 
     post path, params.to_json

spec/services/projects/copy_service_integration_spec.rb

@@ -91,7 +91,7 @@ RSpec.describe(
                            edit_project_attributes
                            select_project_custom_fields])
   end
-  shared_let(:new_project_role) { create(:project_role, permissions: %i[]) }
+  shared_let(:new_project_role) { create(:project_creator_role) }
 
   before do
     allow(Setting)