-
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-6412: Fix admin member user permissions for services (also THREESCALE-11202) #3929
base: master
Are you sure you want to change the base?
Conversation
53ec0be
to
9b051a8
Compare
80439f7
to
fb4f5f6
Compare
@@ -55,4 +55,14 @@ def setup | |||
get edit_admin_service_policies_path(@service) | |||
assert_response :service_unavailable | |||
end | |||
|
|||
test 'policies edit for members with no permissions' do |
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.
@akostadinov I applied your suggestion for memoization and added tests for the 2 pages mentioned in the JIRA description. They fail on master
- return a 200 with full details, and pass in this branch.
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.
About this quote in the PR description:
the user will be able to modify the policy chain (through the direct link), even if they don't have this specific permission set - having any type of access to the service would be enough.
That's actually not true, right? if I try to access /apiconfig/services/2/policies/edit
I this branch I get a 404
now, while I can access in master
branch. This test is precisely for that.
So if it's fixed, why not removing this?:
porta/config/abilities/provider_any.rb
Lines 14 to 15 in f291877
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.
Yeah, indeed, as accessible_services
is now returning an empty array (after fixing the permitted_for
scope), fixing this is a side effect.
As mentioned in the PR description, :policy_registry_ui
doesn't seem to be used, so probably I'll remove it. However, :policy_registry
might be used somewhere else (maybe the API?), so, I'll keep it as it is.
9f6e2d0
to
498779f
Compare
app/models/service.rb
Outdated
self.merge( | ||
account_services.merge(user.forbidden_some_services? ? where(id: user.member_permission_service_ids) : {}) | ||
merge( | ||
account_services.merge(user.selected_services_allowed? ? where(id: user.member_permission_service_ids) : {}) |
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 is the main fix (this line and the one with next none
)
The query relied on forbidden_some_services?
, which it its turn calls has_access_to_all_services?
, which checks:
!admin_sections.include?(:services)
So, it considers that if services
permission is not present, then all services are enabled.
While this is partially true, it means that a newly created member user (that is not supposed to have any permissions - i.e.everything should be prohibited) will have has_access_to_all_services? == true
. Which results in the permitted_for
scope to have the query {account_id: <num>}
, which returns all providers's services and causes the behavior reported in https://issues.redhat.com/browse/THREESCALE-6412
It is true that we determine whether all services are allowed by checking whether "services" permission is nil
(happens when permission with section :services
is not present). But I changed the behavior, and now having a nil
"services" permission (non-existing admin section services
) means "all services are allowed" ONLY IF there is at least one enabled section that has permissions per-service (:partners
, :plans
, :monitoring
)
It might break some previous functionality, so users who previously had some access will not have it anymore. But I think this is necessary, because it fixes a "security breach".
This probably was made so intentionally in the past, see https://github.com/3scale/system/commit/9c818157907c6c76d10a315fa572ff02e5ea327a
But I still think it's not right. Currently a member with no permissions cannot see the menus for the services in the UI, so it means they shouldn't be able to access the specific URLs (mentioned in the JIRA) either.
@@ -21,7 +21,9 @@ | |||
cannot %i[destroy update_role], user | |||
|
|||
# Services | |||
can %i[read show edit update], Service, user.accessible_services.where_values_hash | |||
user_accessible_services = user.accessible_services | |||
can %i[read show edit update], Service, user_accessible_services.where_values_hash unless user_accessible_services.is_a? ActiveRecord::NullRelation |
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.
user_accessible_services.is_a? ActiveRecord::NullRelation
was added to support next none
line in the Service's permitted_for
scope.
Otherwise, in case "no services are allowed", while user.accessible_services
returns an empty array, user.accessible_services.where_values_hash
is {}
, which makes cancancan authorize ALL services (because there is no query conditions).
Adding this unless
condition makes it possible to prohibit all services if permitted_for
returns none
@@ -60,11 +60,11 @@ def member_permission_ids=(roles) | |||
end | |||
|
|||
def member_permission_service_ids=(service_ids) | |||
if service_ids.present? | |||
service_ids = Array(service_ids).compact.map(&:to_i) | |||
if service_ids.is_a? Array |
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.
API and UI send these values in a "funny way", like ["[]"]
and [""]
, because of the URL-encoded params limitations.
I changed the way the service_ids
value is treated, so that it is less confusing to update the param directly with an update
call.
So member_permission_service_ids: []
sets "no services are allowed", while member_permission_service_ids: nil
sets to "all services are allowed".
While the API/UI behavior seems to be still OK.
app/models/user/permissions.rb
Outdated
return nil if admin? || !services_member_permission | ||
permitted_service_ids = services_member_permission.try(:service_ids) || [] | ||
permitted_service_ids & existing_service_ids | ||
if admin? || !services_member_permission |
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 change was for memoization, but I found out that it gives unexpected results when the permissions are changed. So I guess we can't really memoize here, because we want the values to be updated correctly.
end | ||
|
||
test '#show nonexistent application does not check permissions' do | ||
User.any_instance.expects(:has_access_to_all_services?).never |
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.
Throughout the tests, I removed stubbing has_access_to_all_services?
and opted for actually setting the proper permissions for the member. I think it is less confusing, and more reliable.
I might eventually remove this method...
all_services_allowed?
could be a good replacement for it, as it essentially returns the same - but in a correct way :) (i.e. not returning true
for members that don't have ANY permissions). However, stubbing it is not really useful, because the Service.permitted_for
scope doesn't rely on it.
That's why I actually think that rather than stub the methods, we can just set the proper state of the object.
d3de8b6
to
8b81c48
Compare
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 tested this logic locally and it works fine and forbids access to the URLs in the Jira issue 👍.
About tests, I think test/unit/user/permissions_test.rb
needs some updates, to:
- Fix failing tests after changes
- Add unit tests for the new methods
no_services_allowed?
andservice_permissions_selected?
- Review tests for
has_access_to_all_services?
, since this method was wrong before and the tests passed. Aolso to ensure the new behavior is tested correctly
@@ -55,4 +55,14 @@ def setup | |||
get edit_admin_service_policies_path(@service) | |||
assert_response :service_unavailable | |||
end | |||
|
|||
test 'policies edit for members with no permissions' do |
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.
About this quote in the PR description:
the user will be able to modify the policy chain (through the direct link), even if they don't have this specific permission set - having any type of access to the service would be enough.
That's actually not true, right? if I try to access /apiconfig/services/2/policies/edit
I this branch I get a 404
now, while I can access in master
branch. This test is precisely for that.
So if it's fixed, why not removing this?:
porta/config/abilities/provider_any.rb
Lines 14 to 15 in f291877
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) |
|
||
get admin_service_proxy_configs_path(service_id: service, environment: 'sandbox') | ||
|
||
# TODO: maybe this should be be a :forbidden |
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 agree, but, why does it return a 404? how did your changes affect routes?
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.
Yeah, right, the thing is that the first thing that the controller does is trying to find a service:
@service ||= current_user.accessible_services.find(params[:service_id]) |
And as accessible_services
scope has changed, now the service is not found in that list.
That's also the reason why all of these controllers return 404 and not 403 when trying to access - they all load accessible_services
and if they don't find the requested ID there, they just return 404.
I am not sure if it's worth fixing to be honest. And actually it also makes sense - the user doesn't need to know whether the service exists or not, if they don't have permission for it - it's as if it doesn't exist for them 😬
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 think returning 403
or 404
both are valid:
https://www.rfc-editor.org/rfc/rfc7231#section-6.5.3
An origin server that wishes to "hide" the current existence of a forbidden target resource MAY instead respond with a status code of 404 (Not Found).
8b81c48
to
c969ffb
Compare
25690b3
to
5dfa172
Compare
before_action :authorize_section, only: %i[new create] | ||
before_action :authorize_action, only: %i[new create] | ||
before_action :find_plan |
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.
After fixing the permission issue for services, the controller started responding with 404 when 403 was expected (i.e. the service is not allowed AND the required permission is missing).
So, swapping these actions results in the expected behavior, and is more in line with some other controllers.
@@ -16,15 +16,12 @@ module User::Permissions | |||
alias_attribute :allowed_service_ids, :member_permission_service_ids | |||
end | |||
|
|||
#TODO: this is repeated from bcms_hacks plugins because of some loading problem |
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 bcms ("browser CMS") thing was removed long time ago, so this comment is not relevant anymore.
admin_sections.include?(permission.to_sym).tap do |check| | ||
logger.info "~> #{username} has_permission?(#{permission}) => #{check}" |
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 don't think it's nice to pollute logs with these unnecessary logs 😬
503a5fc
to
7a4fdb5
Compare
app/models/user/permissions.rb
Outdated
if service_ids.present? | ||
service_ids = Array(service_ids).compact.map(&:to_i) | ||
if service_ids.is_a? Array | ||
service_ids = (service_ids.compact_blank - [EMPTY_VALUE]).map(&:to_i) |
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.
Somebody not very smart decided to use "[]"
as the value for setting an empty array in this PR
This resulted in array ["[]"]
here, which when converted to integers ended up being [0]
. The outcome was fine, because existing_service_ids
never contains 0. But I still think it's not cool, because theoretically maybe there might be service ID that equals 0
.
On the other hand, maybe we shouldn't treat "[]"
specifically, and discard any other character that is not integer-like, because the user can set anything like "xyz"
, and it will also be converted to 0
.
member_permission = services_member_permission || member_permissions.build(admin_section: :services) | ||
member_permission.service_ids = service_ids & existing_service_ids | ||
else | ||
elsif service_ids.blank? |
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 allows us to set member_permission_service_ids: nil
in the code and member_permission_service_ids: ""
via API and UI to allow all services.
@@ -62,10 +62,15 @@ def self.issued_by(issuer, *ids) | |||
where.has { plan_id.in( scope ) } | |||
end | |||
|
|||
# FIXME: this probably shouldn't return `all`, it's dangerous if it is accidentally |
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 don't really want to fix it in this PR as it is already too long. And it was already this way before...
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.
Maybe use scope provider_account: user.provider_account
although if we don't have these fields and I think we don't, then it may be complicated. So for now it is not worse than before at least.
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.
A few comments:
- About API: Doesn't this introduce braking changes to
PUT /admin/api/users/{id}/permissions.xml
? - Setting the permissions from UI and API works fine for me 👍
- About forbidden pages, the issue seems solved because I can't access those screens now for a user without permissions, however:
- If I check
Create, read, update and delete: the APIcast policy chain and its policies
for all services:- I can't access
/apiconfig/services/2/policies/edit
- Shouldn't I be able?
- I can't access
- If I check
Create, read, update and delete: attributes, metrics, methods, and mapping rules
orAccess & query analytics of:
- I can access
/apiconfig/services/2/policies/edit
- Is it expected? it seems wrong to me.
- I can access
- If I check
- I'd like to have a table on which permission grants access to which screens. Does that table exist?
app/models/user/permissions.rb
Outdated
ATTRIBUTES = %I[ role member_permission_ids member_permission_service_ids ] | ||
ATTRIBUTES = %I[role member_permission_ids member_permission_service_ids].freeze | ||
# The value that can be used for setting the member permission service ids to empty array | ||
EMPTY_VALUE = "[]" |
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 don't like this, but I guess it's for backwards compatibility
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.
Yeah, me neither...
I tried to explain here: #3929 (comment)
Indeed, it is for backwards compatibility, but in fact any set of characters that is not an actual integer number works the same way as this "special" character, so maybe we can treat them all the same way. WDYT?
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.
If you send any non-numerical character it will be converted to 0
, so for me the easiest way would be to just remove zeroes from the resulting service_ids
array, rather than explicitly considering an special character. Then in the docs you can tell users to send "[]"
, that's fine, but no need to be considered by backend at all. In fact, docs mentioning "[]"
are not accurate because the endpoint accepts "[]"
and any non-numerical character.
So I think it was fine before the PR, but you want 0
to be a valid service number, then it's more complicated, because receiving a non-integer character is now interpreted as allowing permissions over service 0
. We could maybe use use Integer()
for this:
Loading development environment (Rails 6.1.7.9)
[1] pry(main)> Integer(" 5 ")
=> 5
[2] pry(main)> Integer("0")
=> 0
[3] pry(main)> Integer("[]")
ArgumentError: invalid value for Integer(): "[]"
from (pry):3:in `Integer'
[4] pry(main)> Integer("24gt")
ArgumentError: invalid value for Integer(): "24gt"
from (pry):4:in `Integer'
[5] pry(main)> Integer("")
ArgumentError: invalid value for Integer(): ""
from (pry):5:in `Integer'
[6] pry(main)>
map
the given service ids, pass them through Integer()
, return nil
on exceptions, then compact the result. That would reject al non-integer characters but accept 0
.
Summarizing, I would:
- State in the docs that
"[]"
means "no services" and any non-numerical character will be removed - Don't care about
"[]"
at all in the backend - Use
Integer()
to process the given values
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.
Thanks a lot for the suggestions @jlledom!
map
the given service ids, pass them throughInteger()
, returnnil
on exceptions, then compact the result. That would reject al non-integer characters but accept0
.
I agree, and I've done it here: 06db7c6
State in the docs that
"[]"
means "no services" and any non-numerical character will be removed
I don't actually want to document []
as a special value. I think it is better to say to pass an empty value instead.
@@ -2812,7 +2812,7 @@ | |||
}, | |||
"allowed_service_ids[]": { | |||
"type": "array", | |||
"description": "IDs of the services that need to be enabled, URL-encoded array. To disable all services, set the value to '[]'. To enable all services, add parameter 'allowed_service_ids[]' with no value to the 'curl' command (can't be done in ActiveDocs)", | |||
"description": "IDs of the services that need to be enabled, URL-encoded array. To disable all services, send `allowed_service_ids[]` with an empty value, e.g. `allowed_service_ids%5B%5D=`. `<allowed_service_ids/>` or `\"allowed_service_ids\":[]` in the response indicate that no services are allowed. To enable all services, send 'allowed_service_ids' with no value to the 'curl' command, e.g. `allowed_service_ids=` (can't be done in ActiveDocs). Missing `<allowed_service_ids>` or `\"allowed_service_ids\":null` indicate that all services are allowed.", |
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.
As we talked on slack, the API Docs are very counter intuitive for this endpoint, for instance, I expect "Send empty value" to set allowed_service_ids[]
to a value that means none
. However, checking the box does nothing and the way to actually set allowed services to none
is to manually inserting any non integer value, which is even more confusing considering the input has a label which says Add integer item
.
I'm aware on how cumbersome is making any small change in SwaggerUI, so if this can't be properly fixed, at least I think we should mention in the description how to set the none
case
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 agree. I actually updated the description of this parameter, trying to make it more clear, but it is true that unfortunately "send allowed_service_ids[]
with an empty value" seems impossible through the Swagger UI 😬
It used to say "set it to []
", but I wanted to deprecate this strange way... I can probably add a note "can't be done in ActiveDocs", as I did for the use case of enabling all services.
What scenario that worked previously do you think that is broken now? This PR does introduce breaking changes, but that's related to what is seen (or not seen) to users with no explicit permissions.
I mentioned this "APIcast policy chain" permission in the PR description... To be honest, I think it is kind of disconnected from the rest of the permissions 🤷 And it's not a per-service permission. Which maybe it should... but maybe that could be done as Part 2?... (the PR already seems a bit overwhelming...)
Me too... I'll try to find something or sketch it maybe. |
I wasn't thinking on a particular scenario. I just saw the changes in
I'm sorry, I read the description the first time I checked this PR but I forgot about it when I last reviewed changes. It's true it's confusing: having a permission which apparently does nothing, and having other permissions which names are vague and misleading about what is actually permitted when you check them 🤦🏼♂️. If it's so confusing for us I can't image how will it be for clients. |
next all unless user.forbidden_some_services? | ||
permitted_services_status = user.permitted_services_status | ||
next none if permitted_services_status == :none | ||
|
||
where(service_id: user.member_permission_service_ids) | ||
next all if permitted_services_status == :all | ||
|
||
merge(permitted_services_status == :selected ? where(service_id: user.member_permission_service_ids) : {}) |
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.
Could you explain what is the actual difference in behavior compared to before. I don't really understand it and I can't properly review if I don't understand this.
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.
Well, I'll try...
From the user point of view, the issue was that a freshly created member that has NO permissions defined at all had access to some pages - specifically mapping rules, policies, metrics etc. They were not visible from the menus, but using a direct link was possible.
The main reason was in this scope, together with the cancan definition:
can %i[read show edit update], Service, user.accessible_services.where_values_hash
So, if the user had no :services
permission, it was automatically considered that it had access to all services, regardless of whether they had permissions such as partners
, plans
or monitoring
.
Currently, it is considered that the user has access to all services ONLY if in addition to not having :services
permission there is at least one of the "per-service" permissions (partners
, plans
or monitoring
).
So, a newly created member, that does not have any permissions at all will not be considered as "being able to see all services".
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.
Currently, it is considered that the user has access to all services ONLY if in addition to not having
:services
permission there is at least one of the "per-service" permissions (partners
,plans
ormonitoring
).
Do you have a reference for this?
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.
Well, it's not that straightforward, you kind of need to follow the whole chain. So, this is what the scope use to say:
self.merge(
account_services.merge(user.forbidden_some_services? ? where(id: user.member_permission_service_ids) : {})
)
user.forbidden_some_services?
call !has_access_to_all_services?
, has_access_to_all_services
calls !admin_sections.include?(:services)
.
So, if admin_sections
does NOT include :services
(which happens to a member with no permissions at all), has_access_to_all_services == true
, user.forbidden_some_services? == false
, so the scope will get resolved to:
account_services.merge({})
which basically means where(account_id: {some_id})
, which returns all provider's services.
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.
To clarify, the above is for the Service.permitted_for
scope, not for the Contract.
But when testing I realised the Contract.permitted_for
was wrong also.
What this PR does / why we need it:
Fixes an issue where a member user with NO permissions could see and update some service configuration.
Which issue(s) this PR fixes
https://issues.redhat.com/browse/THREESCALE-6412
https://issues.redhat.com/browse/THREESCALE-11202
Verification steps
tbd
Special notes for your reviewer:
There is another strange thing regarding policies specifically. There is a special permission for managing the API policy chain, but it doesn't seem to be used for this view.
In fact, it is specifically configured that any user can manage policies, if the feature is enabled in the whole account:
porta/config/abilities/provider_any.rb
Lines 14 to 15 in f291877
(also, I couldn't find references to
:policy_registry_ui
anywhere 🤔)So, I guess I will not be touching this. So, the user will be able to modify the policy chain (through the direct link), even if they don't have this specific permission set - having any type of access to the service would be enough.