Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure a users own tasks are the only ones returned when the users role has View/My Tasks #18311

Merged
merged 2 commits into from
Dec 21, 2018
Merged

Ensure a users own tasks are the only ones returned when the users role has View/My Tasks #18311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
485a2df
Ensure a users own tasks are the only ones returned when the users ro…
jvlcek Dec 20, 2018
cbce61d
Improve naming for only my user tasks
jvlcek Dec 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app/models/miq_product_feature.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
class MiqProductFeature < ApplicationRecord
SUPER_ADMIN_FEATURE = "everything".freeze
SUPER_ADMIN_FEATURE = "everything".freeze
REPORT_ADMIN_FEATURE = "miq_report_superadmin".freeze
REQUEST_ADMIN_FEATURE = "miq_request_approval".freeze
TENANT_ADMIN_FEATURE = "rbac_tenant".freeze
REPORT_MY_TASKS = "miq_task_my_ui".freeze
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the existing convention I think these two should be named with *_FEATURE.

Also, the REPORT prefix is misleading because these features are not reporting feature, afaik. Perhaps you can name them MY_TASKS_FEATURE and ALL_TASKS_FEATURE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thank you. Will do

REPORT_ALL_TASKS = "miq_task_all_ui".freeze
TENANT_ADMIN_FEATURE = "rbac_tenant".freeze

acts_as_tree

Expand Down
4 changes: 4 additions & 0 deletions app/models/miq_user_role.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ def tenant_admin_user?
allows?(:identifier => MiqProductFeature::TENANT_ADMIN_FEATURE)
end

def report_only_my_user_tasks?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the convention of the above methods, how about naming this one my_tasks_user?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gtanzillo Thank you for the feedback.

I named this method report_only_my_user_tasks? because it is possible that miq_task_my_ui product feature is enabled and miq_task_all_ui in which case naming this method my_tasks_user? would be somewhat inaccurate. One might expect my_tasks_user? to return true if miq_task_my_ui is set but that is not the functionality we want. We want a method to return true when only miq_task_my_ui is set and not miq_task_all_ui.

Would you be OK if I renamed this method from report_only_my_user_tasks? to only_my_user_tasks? ?

!allows?(:identifier => MiqProductFeature::REPORT_ALL_TASKS) && allows?(:identifier => MiqProductFeature::REPORT_MY_TASKS)
end

def report_admin_user?
allows?(:identifier => MiqProductFeature::REPORT_ADMIN_FEATURE)
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class User < ApplicationRecord

delegate :miq_user_role, :current_tenant, :get_filters, :has_filters?, :get_managed_filters, :get_belongsto_filters,
:to => :current_group, :allow_nil => true
delegate :super_admin_user?, :request_admin_user?, :self_service?, :limited_self_service?, :report_admin_user?,
delegate :super_admin_user?, :request_admin_user?, :self_service?, :limited_self_service?, :report_admin_user?, :report_only_my_user_tasks?,
:to => :miq_user_role, :allow_nil => true

validates_presence_of :name, :userid
Expand Down
16 changes: 16 additions & 0 deletions spec/models/miq_user_role_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@
let(:tenant_admin_role) { FactoryBot.create(:miq_user_role, :features => MiqProductFeature::TENANT_ADMIN_FEATURE) }
let(:report_admin_role) { FactoryBot.create(:miq_user_role, :features => MiqProductFeature::REPORT_ADMIN_FEATURE) }
let(:request_admin_role) { FactoryBot.create(:miq_user_role, :features => MiqProductFeature::REQUEST_ADMIN_FEATURE) }
let(:report_only_my_tasks) { FactoryBot.create(:miq_user_role, :features => MiqProductFeature::REPORT_MY_TASKS) }
let(:report_only_all_tasks) { FactoryBot.create(:miq_user_role, :features => MiqProductFeature::REPORT_ALL_TASKS) }
let(:regular_role) { FactoryBot.create(:miq_user_role) }

describe "#super_admin_user?" do
Expand All @@ -238,6 +240,20 @@
end
end

describe "#report_only_my_user_tasks?" do
it "detects access limited to only the current users tasks" do
expect(report_only_my_tasks).to be_report_only_my_user_tasks
end

it "detects access not limited to only the current users tasks" do
expect(report_only_all_tasks).not_to be_report_only_my_user_tasks
end

it "detects no access to tasks" do
expect(regular_role).not_to be_report_only_my_user_tasks
end
end

describe "#report_admin_user?" do
it "detects super admin" do
expect(super_admin_role).to be_report_admin_user
Expand Down