From 49194d1a45dc37e9d4addb7d8ee2a970d8bd85de Mon Sep 17 00:00:00 2001 From: Oleksii Kriuchykhin Date: Thu, 18 Feb 2021 09:54:53 +0100 Subject: [PATCH 1/2] Prepare initial user roles in the database [SCI-5435] --- app/models/concerns/user/project_roles.rb | 60 -------------- app/models/user.rb | 1 - app/models/user_role.rb | 78 ++++++++++++++++++- app/permissions/experiment.rb | 7 +- app/permissions/my_module.rb | 21 +++-- app/permissions/project.rb | 4 +- .../extends/permission_extends.rb | 6 +- config/locales/en.yml | 8 ++ ...14508_create_user_roles_and_assignments.rb | 1 + db/structure.sql | 1 + 10 files changed, 104 insertions(+), 83 deletions(-) delete mode 100644 app/models/concerns/user/project_roles.rb diff --git a/app/models/concerns/user/project_roles.rb b/app/models/concerns/user/project_roles.rb deleted file mode 100644 index c20d2da55..000000000 --- a/app/models/concerns/user/project_roles.rb +++ /dev/null @@ -1,60 +0,0 @@ -require 'aspector' - -module User::ProjectRoles - extend ActiveSupport::Concern - - aspector do - # Check if user is member of project - around %i( - is_member_of_project? - is_owner_of_project? - is_user_of_project? - is_user_or_higher_of_project? - is_technician_of_project? - is_technician_or_higher_of_project? - is_viewer_of_project? - ) do |proxy, *args, &block| - if args[0] - @user_project = user_projects.where(project: args[0]).take - @user_project ? proxy.call(*args, &block) : false - else - false - end - end - end - - def is_member_of_project?(project) - # This is already checked by aspector, so just return true - true - end - - def is_creator_of_project?(project) - project.created_by == self - end - - def is_owner_of_project?(project) - @user_project.owner? - end - - def is_user_of_project?(project) - @user_project.normal_user? - end - - def is_user_or_higher_of_project?(project) - @user_project.normal_user? or @user_project.owner? - end - - def is_technician_of_project?(project) - @user_project.technician? - end - - def is_technician_or_higher_of_project?(project) - @user_project.technician? or - @user_project.normal_user? or - @user_project.owner? - end - - def is_viewer_of_project?(project) - @user_project.viewer? - end -end diff --git a/app/models/user.rb b/app/models/user.rb index 5bc6add06..9646853ac 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -5,7 +5,6 @@ class User < ApplicationRecord include SettingsModel include VariablesModel include User::TeamRoles - include User::ProjectRoles include TeamBySubjectModel include InputSanitizeHelper include ActiveStorageConcerns diff --git a/app/models/user_role.rb b/app/models/user_role.rb index b7b5efe4e..c9a121be5 100644 --- a/app/models/user_role.rb +++ b/app/models/user_role.rb @@ -1,14 +1,88 @@ # frozen_string_literal: true class UserRole < ApplicationRecord + before_update :prevent_update, if: :predefined? + validates :name, presence: true, length: { minimum: Constants::NAME_MIN_LENGTH, maximum: Constants::NAME_MAX_LENGTH }, uniqueness: { case_sensitive: false } validates :permissions, presence: true, length: { minimum: 1 } + validates :created_by, presence: true, unless: :predefined? + validates :last_modified_by, presence: true, unless: :predefined? - belongs_to :created_by, foreign_key: 'created_by_id', class_name: 'User' - belongs_to :last_modified_by, foreign_key: 'last_modified_by_id', class_name: 'User' + belongs_to :created_by, foreign_key: 'created_by_id', class_name: 'User', optional: true + belongs_to :last_modified_by, foreign_key: 'last_modified_by_id', class_name: 'User', optional: true has_many :user_assignments, dependent: :destroy + + def self.owner_role + new( + name: I18n.t('user_roles.predefined.owner'), + permissions: ProjectPermissions.constants + ExperimentPermissions.constants + MyModulePermissions.constants, + predefined: true + ) + end + + def self.normal_user_role + new( + name: I18n.t('user_roles.predefined.normal_user'), + permissions: + [ + ProjectPermissions::READ, + ProjectPermissions::CREATE_EXPERIMENTS, + ProjectPermissions::CREATE_COMMENTS, + ExperimentPermissions::READ, + ExperimentPermissions::MANAGE, + ExperimentPermissions::ARCHIVE, + ExperimentPermissions::RESTORE, + ExperimentPermissions::CLONE, + ExperimentPermissions::CREATE_TASKS, + MyModulePermissions::READ, + MyModulePermissions::CREATE_COMMENTS, + MyModulePermissions::ASSIGN_REPOSITORY_ROWS, + MyModulePermissions::CHANGE_FLOW_STATUS, + MyModulePermissions::CREATE_REPOSITORY_SNAPSHOT, + MyModulePermissions::MANAGE_REPOSITORY_SNAPSHOT + ], + predefined: true + ) + end + + def self.technician_role + new( + name: I18n.t('user_roles.predefined.technician'), + permissions: + [ + ProjectPermissions::READ, + ProjectPermissions::CREATE_COMMENTS, + ExperimentPermissions::READ, + MyModulePermissions::READ, + MyModulePermissions::CREATE_COMMENTS, + MyModulePermissions::ASSIGN_REPOSITORY_ROWS, + MyModulePermissions::CHANGE_FLOW_STATUS, + MyModulePermissions::CREATE_REPOSITORY_SNAPSHOT, + MyModulePermissions::MANAGE_REPOSITORY_SNAPSHOT + ], + predefined: true + ) + end + + def self.viewer_role + new( + name: I18n.t('user_roles.predefined.viewer'), + permissions: + [ + ProjectPermissions::READ, + ExperimentPermissions::READ, + MyModulePermissions::READ + ] + ) + end + + private + + def prevent_update + raise ActiveRecord::RecordInvalid, I18n.t('user_roles.predefined.unchangable_error_message') + end end diff --git a/app/permissions/experiment.rb b/app/permissions/experiment.rb index a68eb54b1..b539d9d2c 100644 --- a/app/permissions/experiment.rb +++ b/app/permissions/experiment.rb @@ -59,7 +59,7 @@ Canaid::Permissions.register_for(Experiment) do # experiment: move can :move_experiment do |user, experiment| - can_clone_experiment?(user, experiment) + experiment.permission_granted?(user, ExperimentPermissions::MOVE) end end @@ -125,9 +125,6 @@ Canaid::Permissions.register_for(Comment) do # step: update/delete comment can :manage_comment_in_module do |user, comment| my_module = ::PermissionsUtil.get_comment_module(comment) - project = my_module.experiment.project - # Same check as in `can_manage_comment_in_project?` - project.present? && - (user.is_owner_of_project?(project) || comment.user == user) + comment.user == user || my_module.permission_granted?(user, MyModulePermissions::MANAGE_COMMENTS) end end diff --git a/app/permissions/my_module.rb b/app/permissions/my_module.rb index 3419be5ef..380421ea3 100644 --- a/app/permissions/my_module.rb +++ b/app/permissions/my_module.rb @@ -23,57 +23,56 @@ Canaid::Permissions.register_for(MyModule) do # module: update # result: create, update can :manage_module do |user, my_module| - can_manage_experiment?(user, my_module.experiment) + my_module.permission_granted?(user, MyModulePermissions::MANAGE) end # module: archive can :archive_module do |user, my_module| - can_manage_experiment?(user, my_module.experiment) + my_module.permission_granted?(user, MyModulePermissions::ARCHIVE) end # NOTE: Must not be dependent on canaid parmision for which we check if it's # active # module: restore can :restore_module do |user, my_module| - user.is_user_or_higher_of_project?(my_module.experiment.project) && - my_module.archived? + my_module.archived? && my_module.permission_granted?(user, MyModulePermissions::RESTORE) end # module: move can :move_module do |user, my_module| - can_manage_experiment?(user, my_module.experiment) + my_module.permission_granted?(user, MyModulePermissions::MOVE) end # module: assign/reassign/unassign users can :manage_users_in_module do |user, my_module| - user.is_owner_of_project?(my_module.experiment.project) + my_module.permission_granted?(user, MyModulePermissions::MANAGE_USERS) end # module: assign/unassign repository record # NOTE: Use 'module_page? &&' before calling this permission! can :assign_repository_rows_to_module do |user, my_module| - user.is_technician_or_higher_of_project?(my_module.experiment.project) + my_module.permission_granted?(user, MyModulePermissions::ASSIGN_REPOSITORY_ROWS) end # module: change_flow_status can :change_my_module_flow_status do |user, my_module| - user.is_technician_or_higher_of_project?(my_module.experiment.project) + my_module.permission_granted?(user, MyModulePermissions::CHANGE_FLOW_STATUS) end # module: create comment # result: create comment # step: create comment can :create_comments_in_module do |user, my_module| - can_create_comments_in_project?(user, my_module.experiment.project) + my_module.permission_granted?(user, MyModulePermissions::CREATE_COMMENTS) end # module: create a snapshot of repository item can :create_my_module_repository_snapshot do |user, my_module| - user.is_technician_or_higher_of_project?(my_module.experiment.project) + my_module.permission_granted?(user, MyModulePermissions::CREATE_REPOSITORY_SNAPSHOT) end # module: make a repository snapshot selected can :manage_my_module_repository_snapshots do |user, my_module| - user.is_technician_or_higher_of_project?(my_module.experiment.project) + my_module.permission_granted?(user, MyModulePermissions::MANAGE_REPOSITORY_SNAPSHOT) end end diff --git a/app/permissions/project.rb b/app/permissions/project.rb index a42bdf89f..b31e8c858 100644 --- a/app/permissions/project.rb +++ b/app/permissions/project.rb @@ -76,7 +76,7 @@ Canaid::Permissions.register_for(Project) do # project: create/update/delete tag # module: assign/reassign/unassign tag can :manage_tags do |user, project| - user.is_user_or_higher_of_project?(project) + project.permission_granted?(user, ProjectPermissions::MANAGE_TAGS) end end @@ -92,6 +92,6 @@ Canaid::Permissions.register_for(ProjectComment) do # project: update/delete comment can :manage_comment_in_project do |user, project_comment| project_comment.project.present? && (project_comment.user == user || - project.permission_granted?(user, ProjectPermissions::EDIT_COMMENTS)) + project.permission_granted?(user, ProjectPermissions::MANAGE_COMMENTS)) end end diff --git a/config/initializers/extends/permission_extends.rb b/config/initializers/extends/permission_extends.rb index 687adc167..1fc307646 100644 --- a/config/initializers/extends/permission_extends.rb +++ b/config/initializers/extends/permission_extends.rb @@ -10,8 +10,7 @@ module PermissionExtends RESTORE CREATE_EXPERIMENTS CREATE_COMMENTS - EDIT_COMMENTS - DELETE_COMMENTS + MANAGE_COMMENTS MANAGE_TAGS ).each { |permission| const_set(permission, permission.underscore) } end @@ -24,11 +23,13 @@ module PermissionExtends RESTORE CLONE MOVE + CREATE_TASKS ).each { |permission| const_set(permission, permission.underscore) } end module MyModulePermissions %w( + READ MANAGE ARCHIVE RESTORE @@ -37,6 +38,7 @@ module PermissionExtends ASSIGN_REPOSITORY_ROWS CHANGE_FLOW_STATUS CREATE_COMMENTS + MANAGE_COMMENTS CREATE_REPOSITORY_SNAPSHOT MANAGE_REPOSITORY_SNAPSHOT ).each { |permission| const_set(permission, permission.underscore) } diff --git a/config/locales/en.yml b/config/locales/en.yml index a41543113..d9885af9c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1828,6 +1828,14 @@ en: destroy_uo_confirm: "Remove" leave_flash: "Successfuly left team %{team}." + user_roles: + predefined: + owner: "Owner" + normal_user: "User" + technician: "Technician" + viewer: "Viewer" + unchangable_error_message: "Predefined roles can not be changed!" + protocols: protocols_io_import: title_too_long: "... Text is too long so we had to cut it off." diff --git a/db/migrate/20210202214508_create_user_roles_and_assignments.rb b/db/migrate/20210202214508_create_user_roles_and_assignments.rb index df5a75b53..2f9dbd5e3 100644 --- a/db/migrate/20210202214508_create_user_roles_and_assignments.rb +++ b/db/migrate/20210202214508_create_user_roles_and_assignments.rb @@ -4,6 +4,7 @@ class CreateUserRolesAndAssignments < ActiveRecord::Migration[6.1] def change create_table :user_roles do |t| t.string :name + t.boolean :predefined, default: false t.string :permissions, array: true, default: [] t.references :created_by, foreign_key: { to_table: :users }, null: false t.references :last_modified_by, foreign_key: { to_table: :users }, null: false diff --git a/db/structure.sql b/db/structure.sql index a824cb8d7..2c104a470 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2609,6 +2609,7 @@ ALTER SEQUENCE public.user_projects_id_seq OWNED BY public.user_projects.id; CREATE TABLE public.user_roles ( id bigint NOT NULL, name character varying, + predefined boolean DEFAULT false, permissions character varying[] DEFAULT '{}'::character varying[], created_by_id bigint NOT NULL, last_modified_by_id bigint NOT NULL, From d8cffe53c7e3170c7d3d7aa083e2a1f07ceec3dd Mon Sep 17 00:00:00 2001 From: Oleksii Kriuchykhin Date: Mon, 1 Mar 2021 14:28:21 +0100 Subject: [PATCH 2/2] Add migration for new user assignments to projects [SCI-5478] --- app/models/user_assignment.rb | 2 +- app/models/user_role.rb | 7 +- .../extends/permission_extends.rb | 8 +-- ...14508_create_user_roles_and_assignments.rb | 6 +- ...0210222123823_migrate_to_new_user_roles.rb | 71 +++++++++++++++++++ db/structure.sql | 9 +-- 6 files changed, 89 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20210222123823_migrate_to_new_user_roles.rb diff --git a/app/models/user_assignment.rb b/app/models/user_assignment.rb index b4a1f2fe9..bd0c728b5 100644 --- a/app/models/user_assignment.rb +++ b/app/models/user_assignment.rb @@ -4,5 +4,5 @@ class UserAssignment < ApplicationRecord belongs_to :assignable, polymorphic: true belongs_to :user_role belongs_to :user - belongs_to :assigned_by, foreign_key: 'assigned_by_id', class_name: 'User' + belongs_to :assigned_by, foreign_key: 'assigned_by_id', class_name: 'User', optional: true end diff --git a/app/models/user_role.rb b/app/models/user_role.rb index c9a121be5..5c9827e50 100644 --- a/app/models/user_role.rb +++ b/app/models/user_role.rb @@ -19,7 +19,9 @@ class UserRole < ApplicationRecord def self.owner_role new( name: I18n.t('user_roles.predefined.owner'), - permissions: ProjectPermissions.constants + ExperimentPermissions.constants + MyModulePermissions.constants, + permissions: ProjectPermissions.constants.map { |const| ProjectPermissions.const_get(const) } + + ExperimentPermissions.constants.map { |const| ExperimentPermissions.const_get(const) } + + MyModulePermissions.constants.map { |const| MyModulePermissions.const_get(const) }, predefined: true ) end @@ -76,7 +78,8 @@ class UserRole < ApplicationRecord ProjectPermissions::READ, ExperimentPermissions::READ, MyModulePermissions::READ - ] + ], + predefined: true ) end diff --git a/config/initializers/extends/permission_extends.rb b/config/initializers/extends/permission_extends.rb index 1fc307646..d5418cae6 100644 --- a/config/initializers/extends/permission_extends.rb +++ b/config/initializers/extends/permission_extends.rb @@ -12,7 +12,7 @@ module PermissionExtends CREATE_COMMENTS MANAGE_COMMENTS MANAGE_TAGS - ).each { |permission| const_set(permission, permission.underscore) } + ).each { |permission| const_set(permission, "project_#{permission.underscore}") } end module ExperimentPermissions @@ -24,7 +24,7 @@ module PermissionExtends CLONE MOVE CREATE_TASKS - ).each { |permission| const_set(permission, permission.underscore) } + ).each { |permission| const_set(permission, "experiment_#{permission.underscore}") } end module MyModulePermissions @@ -41,7 +41,7 @@ module PermissionExtends MANAGE_COMMENTS CREATE_REPOSITORY_SNAPSHOT MANAGE_REPOSITORY_SNAPSHOT - ).each { |permission| const_set(permission, permission.underscore) } + ).each { |permission| const_set(permission, "task_#{permission.underscore}") } end module RepositoryPermissions @@ -59,6 +59,6 @@ module PermissionExtends CREATE_COLUMN UPDATE_COLUMN DELETE_COLUMN - ).each { |permission| const_set(permission, permission.underscore) } + ).each { |permission| const_set(permission, "inventory_#{permission.underscore}") } end end diff --git a/db/migrate/20210202214508_create_user_roles_and_assignments.rb b/db/migrate/20210202214508_create_user_roles_and_assignments.rb index 2f9dbd5e3..3d6dc65d4 100644 --- a/db/migrate/20210202214508_create_user_roles_and_assignments.rb +++ b/db/migrate/20210202214508_create_user_roles_and_assignments.rb @@ -6,8 +6,8 @@ class CreateUserRolesAndAssignments < ActiveRecord::Migration[6.1] t.string :name t.boolean :predefined, default: false t.string :permissions, array: true, default: [] - t.references :created_by, foreign_key: { to_table: :users }, null: false - t.references :last_modified_by, foreign_key: { to_table: :users }, null: false + t.references :created_by, foreign_key: { to_table: :users }, null: true + t.references :last_modified_by, foreign_key: { to_table: :users }, null: true t.timestamps end @@ -16,7 +16,7 @@ class CreateUserRolesAndAssignments < ActiveRecord::Migration[6.1] t.references :assignable, polymorphic: true, null: false t.references :user, foreign_key: true, null: false t.references :user_role, foreign_key: true, null: false - t.references :assigned_by, foreign_key: { to_table: :users }, null: false + t.references :assigned_by, foreign_key: { to_table: :users }, null: true t.timestamps end diff --git a/db/migrate/20210222123823_migrate_to_new_user_roles.rb b/db/migrate/20210222123823_migrate_to_new_user_roles.rb new file mode 100644 index 000000000..b7fccc161 --- /dev/null +++ b/db/migrate/20210222123823_migrate_to_new_user_roles.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +class MigrateToNewUserRoles < ActiveRecord::Migration[6.1] + def change + reversible do |dir| + dir.up do + owner_role = UserRole.owner_role + owner_role.save! + normal_user_role = UserRole.normal_user_role + normal_user_role.save! + technician_role = UserRole.technician_role + technician_role.save! + viewer_role = UserRole.viewer_role + viewer_role.save! + + create_user_assignments(UserProject.owner, owner_role) + create_user_assignments(UserProject.normal_user, normal_user_role) + create_user_assignments(UserProject.technician, technician_role) + create_user_assignments(UserProject.viewer, viewer_role) + + UserProject.delete_all + end + dir.down do + project_assignments = UserAssignment.joins(:user_role).where(assignable_type: 'Project') + create_user_project( + project_assignments.where(user_role: { name: I18n.t('user_roles.predefined.owner'), predefined: true }), + 'owner' + ) + create_user_project( + project_assignments.where(user_role: { name: I18n.t('user_roles.predefined.normal_user'), predefined: true }), + 'normal_user' + ) + create_user_project( + project_assignments.where(user_role: { name: I18n.t('user_roles.predefined.technician'), predefined: true }), + 'technician' + ) + create_user_project( + project_assignments.where(user_role: { name: I18n.t('user_roles.predefined.viewer'), predefined: true }), + 'viewer' + ) + + UserAssignment.joins(:user_role).where(user_role: { predefined: true }).delete_all + UserRole.where(predefined: true).delete_all + end + end + end + + private + + def create_user_assignments(user_projects, user_role) + user_projects.includes(:user, :project).find_in_batches(batch_size: 100) do |user_project_batch| + user_assignments = [] + user_project_batch.each do |user_project| + user_assignments << UserAssignment.new(user: user_project.user, + assignable: user_project.project, + user_role: user_role) + end + UserAssignment.import(user_assignments) + end + end + + def create_user_project(user_roles, role) + user_roles.includes(:user, :assignable).find_in_batches(batch_size: 100) do |user_role_batch| + user_projects = [] + user_role_batch.each do |user_role| + user_projects << UserProject.new(user: user_role.user, project: user_role.assignable, role: role) + end + UserProject.import(user_projects) + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 2c104a470..e8a29c461 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2440,7 +2440,7 @@ CREATE TABLE public.user_assignments ( assignable_id bigint NOT NULL, user_id bigint NOT NULL, user_role_id bigint NOT NULL, - assigned_by_id bigint NOT NULL, + assigned_by_id bigint, created_at timestamp(6) without time zone NOT NULL, updated_at timestamp(6) without time zone NOT NULL ); @@ -2611,8 +2611,8 @@ CREATE TABLE public.user_roles ( name character varying, predefined boolean DEFAULT false, permissions character varying[] DEFAULT '{}'::character varying[], - created_by_id bigint NOT NULL, - last_modified_by_id bigint NOT NULL, + created_by_id bigint, + last_modified_by_id bigint, created_at timestamp(6) without time zone NOT NULL, updated_at timestamp(6) without time zone NOT NULL ); @@ -7336,6 +7336,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20201209165626'), ('20210128105457'), ('20210128105458'), -('20210202214508'); +('20210202214508'), +('20210222123823');