From dee6663119459affa02f651d8ad1071f7756d03d Mon Sep 17 00:00:00 2001 From: nirajkumar999 <1751nk@gmail.com> Date: Sun, 29 Sep 2024 22:22:42 +0530 Subject: [PATCH 1/3] Extended return condition to render error if Users are associated with role. A.> I have tried to modify return condition for error status :method_not_allowed by adding additional OR condition which checks if any User is associated with the role because if we don't manually do it here we get a Internal Server Error from postgres stating foreign key constraint violation which is correct at its place. B.> But at UI we get "The action cant be completed" in the toast message. This don't align with our requirements. So i have manually added the OR condition check. C.> This way I eliminate Internal Server Error stuff which is a good practice. Also the toast shows the proper message as already stated in useDeleteRole hook. The same message gets displayed when we try to delete the pre defined roles. D.> If we look carefully at UI we dont have option to delete pre defined roles. So the return condition for error status :method_not_allowed will never be true. On adding the OR condition to this we ensure proper invoking in this case. --- app/controllers/api/v1/admin/roles_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/v1/admin/roles_controller.rb b/app/controllers/api/v1/admin/roles_controller.rb index 4c4d1f43ed..e941177859 100644 --- a/app/controllers/api/v1/admin/roles_controller.rb +++ b/app/controllers/api/v1/admin/roles_controller.rb @@ -66,7 +66,7 @@ def update # Deletes a role def destroy undeletable_roles = %w[User Administrator Guest] - return render_error errors: @role.errors.to_a, status: :method_not_allowed if undeletable_roles.include?(@role.name) + return render_error errors: @role.errors.to_a, status: :method_not_allowed if undeletable_roles.include?(@role.name) || User.find_by(role_id: @role.id) @role.destroy! From f340750260b59c01a91ccbc1ebbb32adeebb4a6b Mon Sep 17 00:00:00 2001 From: nirajkumar999 <1751nk@gmail.com> Date: Sun, 29 Sep 2024 22:46:14 +0530 Subject: [PATCH 2/3] Modified update and destroy action route for roles_controller. Added further error handler to useDeleteRole.jsx A.> In update action we chain the name update to SiteSetting DefaultRole. This ensures consistency.action we chain the name update to SiteSetting DefaultRole. This ensures consistency. B.> We avoid the delete action for the role that is assigned as DefaultRole in SiteSetting. If we don't do this we don't have any side effects as this case is already handled by setting fallback to User role, in case the DefaultRole is not present. But I think this should be handled properly by giving a correct error message. We return status: :forbidden in that case and the same is handled in the useDeleteRole hook. C.> What I observe is that if the update and delete action is not aligned with SiteSetting Default Role then the dropdown appearing for the setting in the UI is left empty without any valid selection. This isn't consistent. --- app/assets/locales/en.json | 3 ++- app/controllers/api/v1/admin/roles_controller.rb | 11 +++++++++++ .../hooks/mutations/admin/roles/useDeleteRole.jsx | 2 ++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/assets/locales/en.json b/app/assets/locales/en.json index 665d511926..553b17d250 100644 --- a/app/assets/locales/en.json +++ b/app/assets/locales/en.json @@ -452,7 +452,8 @@ "signin_required": "You must be signed in to access this page.", "malware_detected": "Malware Detected! The file you uploaded may contain malware. Please check your file and try again.", "roles": { - "role_assigned": "This role can't be deleted because it is assigned to at least one user." + "role_assigned": "This role can't be deleted because it is assigned to at least one user.", + "role_set_as_default": "This role can't be deleted because it is set as Default Role in Site Setting." }, "users": { "signup_error": "You can't be authenticated. Please contact your administrator.", diff --git a/app/controllers/api/v1/admin/roles_controller.rb b/app/controllers/api/v1/admin/roles_controller.rb index e941177859..7e08bd1a70 100644 --- a/app/controllers/api/v1/admin/roles_controller.rb +++ b/app/controllers/api/v1/admin/roles_controller.rb @@ -57,8 +57,15 @@ def create # POST /api/v1/:id/roles.json # Updates a role def update + old_role_name = @role.name + return render_error errors: @role.errors.to_a, status: :bad_request unless @role.update role_params + # update the 'DefaultRole' site setting value, if it used to be the current role. + default_role_site_setting = SiteSetting.joins(:setting).find_by(provider: current_provider, setting: { name: 'DefaultRole'}) + + default_role_site_setting.update(value: @role.name) if default_role_site_setting.value == old_role_name + render_data status: :ok end @@ -68,6 +75,10 @@ def destroy undeletable_roles = %w[User Administrator Guest] return render_error errors: @role.errors.to_a, status: :method_not_allowed if undeletable_roles.include?(@role.name) || User.find_by(role_id: @role.id) + # prevent role from being deleted if its the default role in site setting + default_role_site_setting = SiteSetting.joins(:setting).find_by(provider: current_provider, setting: { name: 'DefaultRole'}) + + return render_error status: :forbidden if default_role_site_setting.value == @role.name @role.destroy! render_data status: :ok diff --git a/app/javascript/hooks/mutations/admin/roles/useDeleteRole.jsx b/app/javascript/hooks/mutations/admin/roles/useDeleteRole.jsx index 471b5ba8ec..dac7a6db2e 100644 --- a/app/javascript/hooks/mutations/admin/roles/useDeleteRole.jsx +++ b/app/javascript/hooks/mutations/admin/roles/useDeleteRole.jsx @@ -31,6 +31,8 @@ export default function useDeleteRole({ role, onSettled }) { onError: (error) => { if (error.response?.status === 405) { toast.error(t('toast.error.roles.role_assigned')); + } else if (error.response?.status === 403) { + toast.error(t('toast.error.roles.role_set_as_default')); } else { toast.error(t('toast.error.problem_completing_action')); } From 2880ed1150fa673459f9d4031e213224c26f335a Mon Sep 17 00:00:00 2001 From: nirajkumar999 <1751nk@gmail.com> Date: Sun, 29 Sep 2024 23:02:40 +0530 Subject: [PATCH 3/3] passed rubocop + eslint test --- app/controllers/api/v1/admin/roles_controller.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/v1/admin/roles_controller.rb b/app/controllers/api/v1/admin/roles_controller.rb index 7e08bd1a70..2d2fdf893b 100644 --- a/app/controllers/api/v1/admin/roles_controller.rb +++ b/app/controllers/api/v1/admin/roles_controller.rb @@ -62,7 +62,7 @@ def update return render_error errors: @role.errors.to_a, status: :bad_request unless @role.update role_params # update the 'DefaultRole' site setting value, if it used to be the current role. - default_role_site_setting = SiteSetting.joins(:setting).find_by(provider: current_provider, setting: { name: 'DefaultRole'}) + default_role_site_setting = SiteSetting.joins(:setting).find_by(provider: current_provider, setting: { name: 'DefaultRole' }) default_role_site_setting.update(value: @role.name) if default_role_site_setting.value == old_role_name @@ -73,12 +73,16 @@ def update # Deletes a role def destroy undeletable_roles = %w[User Administrator Guest] - return render_error errors: @role.errors.to_a, status: :method_not_allowed if undeletable_roles.include?(@role.name) || User.find_by(role_id: @role.id) + if undeletable_roles.include?(@role.name) || User.find_by(role_id: @role.id) + return render_error errors: @role.errors.to_a, + status: :method_not_allowed + end # prevent role from being deleted if its the default role in site setting - default_role_site_setting = SiteSetting.joins(:setting).find_by(provider: current_provider, setting: { name: 'DefaultRole'}) + default_role_site_setting = SiteSetting.joins(:setting).find_by(provider: current_provider, setting: { name: 'DefaultRole' }) return render_error status: :forbidden if default_role_site_setting.value == @role.name + @role.destroy! render_data status: :ok