-
Notifications
You must be signed in to change notification settings - Fork 74
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.
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
THREESCALE-11644: Fix policy registry permission (and THREESCALE-6482) #4002
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checkbox makes much more sense now.
I tried and works well for me. Having the policies box checked, API endpoints didn't work before and now work.
However there's a small UI bug. the Dashboard shows now the allowed service, but you can't interact with it. Clicking the link does nothing, and the dropdown is empty. See the screenshot
You can also visit /apiconfig/services/2/policies/edit
and makes changes in the policy, I guess that's right and expected.
if can?(:manage, :plans) | ||
actions << { name: 'ActiveDocs', path: admin_service_api_docs_path(product) } | ||
actions << { name: 'Integration', path: admin_service_integration_path(product) } | ||
elsif can?(:manage, :policy_registry) | ||
actions << { name: 'Policies', path: edit_admin_service_policies_path(product) } | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be like:
if can?(:manage, :plans) | |
actions << { name: 'ActiveDocs', path: admin_service_api_docs_path(product) } | |
actions << { name: 'Integration', path: admin_service_integration_path(product) } | |
elsif can?(:manage, :policy_registry) | |
actions << { name: 'Policies', path: edit_admin_service_policies_path(product) } | |
end | |
if can?(:manage, :plans) | |
actions << { name: 'ActiveDocs', path: admin_service_api_docs_path(product) } | |
actions << { name: 'Integration', path: admin_service_integration_path(product) } | |
end | |
if can?(:manage, :policy_registry) | |
actions << { name: 'Policies', path: edit_admin_service_policies_path(product) } | |
end |
Can't user manage both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they can, and at first I wanted to do it as you are suggesting. But, I think that having both "Integration" and "Policies" items is redundant, because "Policies" is a sub-section of "Integration". So, if we have access to the parent section ("Integration"), we have just that one. And if we don't have access to "Integration", but only to its child "Policies", then we have "Policies" in the list.
Will this require changes to existing user permissions to make them compatible with the new approach? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By accident I discovered a side effect, not sure if it's correct or a regression:
- Uncheck the
Create, read, update and delete
box - Check the
Access & query analytics of:
box - In
master
: Product -> Integration -> Policies is not in the menu, but is accessible anyway - In this branch: Product -> Integration -> Policies is in the menu and accessible
Good catch @jlledom |
@@ -11,9 +11,6 @@ | |||
|
|||
can :manage, user | |||
|
|||
can(:manage, :policy_registry) if account.tenant? && account.provider_can_use?(:policy_registry) | |||
can(:manage, :policy_registry_ui) if account.tenant? && account.provider_can_use?(:policy_registry_ui) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any place where :policy_registry_ui
would be used...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm:
config/abilities/provider_admin.rb
: Permissions for provider admins
config/abilities/provider_member.rb
: Permissions for provider members
config/abilities/provider_any.rb
: Common permissions for provider admins and members.
Am I right?
Exactly |
9624797
to
2261d3c
Compare
What this PR does / why we need it:
After fixing member permissions per service in #3929, policy registry operations are not working as expected. This PR fixes it.
This PR changes the permission model, and now the
policy_registry
is considered a "service-level" permission, so it can be applied to a subset of serviced, or all services.This also affects the UI.
policy_registry
permission for a single service:policy_registry
permission:This PR also fixes an issue reported in https://issues.redhat.com/browse/THREESCALE-6482: when the member user only has Analytics permission, they can see "Explore all Products" link on the dashboard, and also "Products" entry in the context selector, they bring them to the Products list path, but the access is denied.
In this PR products list is shown to any member user that has access to some services with Analytics, Plans or Policy Registry permissions.
Which issue(s) this PR fixes
https://issues.redhat.com/browse/THREESCALE-11644
also fixes https://issues.redhat.com/browse/THREESCALE-6482
Verification steps
TBD
Special notes for your reviewer: