Separate Role Access Restrictions for Service Templates #697
Conversation
| @@ -0,0 +1,32 @@ | |||
| class SeparateRoleAccessRestrictionsForServiceTemplates < ActiveRecord::Migration[6.0] | |||
| class MiqUserRole < ActiveRecord::Base; end | |||
There was a problem hiding this comment.
Does it make sense to have settings as a serialized column here?
So we could read / write to it and we know we are not messing up some strange yaml thing?
There was a problem hiding this comment.
Does it make sense to have settings as a serialized column here?
Thanks for the comment @kbrock, I think serialize :settings is a big improvement.
db/migrate/20230709065227_separate_role_access_restrictions_for_service_templates.rb
Outdated
Show resolved
Hide resolved
db/migrate/20230709065227_separate_role_access_restrictions_for_service_templates.rb
Outdated
Show resolved
Hide resolved
| if role.settings.include?(":vms: :user_or_group") | ||
| role.settings << " :service_templates: :user_or_group\n" | ||
| elsif role.settings.include?(":vms: :user") | ||
| role.settings << " :service_templates: :user\n" |
There was a problem hiding this comment.
Don't remember exactly what can be in settings, but I worry that this may go into the wrong place in settings.
If it is flat, then probably not a concern, but if there is any hierarchy in here, then could this will end up under another value?
There was a problem hiding this comment.
With serialize :settings I could ensure the new :service_templates key is nested under :restrictions.
jaywcarman
force-pushed
the
separate_role_access_restrictions_for_service_templates
branch
2 times, most recently
from
a7fa7f5
to
54c660a
Compare
| def up | ||
| say_with_time("Updating MiqUserRole restictions so Service Templates match existing VMs") do | ||
| MiqUserRole.where(:read_only => false).where("settings like '%vms: :user%'").find_each do |role| | ||
| role.settings[:restrictions][:service_templates] = role.settings&.dig(:restrictions, :vms) |
There was a problem hiding this comment.
Not holding up merge for this, but this is technically not needed, because we know settings exists (since we are accessing it earlier in the line).
| role.settings[:restrictions][:service_templates] = role.settings&.dig(:restrictions, :vms) | |
| role.settings[:restrictions][:service_templates] = role.settings.dig(:restrictions, :vms) |
There was a problem hiding this comment.
Might as well clean it up! 😄
|
|
||
| def up | ||
| say_with_time("Updating MiqUserRole restictions so Service Templates match existing VMs") do | ||
| MiqUserRole.where(:read_only => false).where("settings like '%vms: :user%'").find_each do |role| |
There was a problem hiding this comment.
Super-minor
| MiqUserRole.where(:read_only => false).where("settings like '%vms: :user%'").find_each do |role| | |
| MiqUserRole.where(:read_only => false).where("settings LIKE '%vms: :user%'").find_each do |role| |
db/migrate/20230709065227_separate_role_access_restrictions_for_service_templates.rb
Outdated
Show resolved
Hide resolved
jaywcarman
force-pushed
the
separate_role_access_restrictions_for_service_templates
branch
from
54c660a
to
7af3019
Compare
|
Checked commit jaywcarman@7af3019 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint |
jaywcarman
deleted the
separate_role_access_restrictions_for_service_templates
branch
Related to:
Migration to handle the separate access restrictions for service templates. Note that the migration only affects custom roles, which can easily be filtered by
:read_only = false. The 6072d2c core commit takes care of updating the read-only, built-inEvmRole-user_limited_self_serviceandEvmRole-user_self_serviceroles so that they are functionally the same before and after the separate access restrictions.