-
Notifications
You must be signed in to change notification settings - Fork 357
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
Require cloud_controller.read access(or equivalent) to access list endpoints. #3450
Conversation
Two things to note:
|
Looking into this PR sent me down a rabbit hole about the In summary, they were first introduced by #221 for this story. It appears the main difference between the two methods is what error is returned: |
@@ -31,8 +31,7 @@ def delete?(_object) | |||
end | |||
|
|||
def index?(_object_class, _params=nil) |
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.
In general, is it necessary to fill in this logic in both index?
and index_with_token?
for all these access classes? From looking at
cloud_controller_ng/app/controllers/base/model_controller.rb
Lines 239 to 251 in 7b1848e
def validate_access(operation, obj, *) | |
if @access_context.cannot?("#{operation}_with_token".to_sym, obj) | |
obj = obj.to_s if obj.is_a? Class | |
logger.info('allowy.access-denied.insufficient-scope', op: "#{operation}_with_token", obj: obj, user: user, roles: roles) | |
raise CloudController::Errors::ApiError.new_from_details('InsufficientScope') | |
end | |
return unless @access_context.cannot?(operation, obj, *) | |
obj = obj.to_s if obj.is_a? Class | |
logger.info('allowy.access-denied.not-authorized', op: operation, obj: obj, user: user, roles: roles) | |
raise CloudController::Errors::ApiError.new_from_details('NotAuthorized') | |
end |
index_with_token?
is called first and the request will never reach index?
. Also the index_with_token?
error seems more appropriate for this case (though, we aren't very consistent with which access method we use, in general).
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.
Broadly speaking, it seems to me both safer, and moreso, consistant for us to double up on both.
It doesn't really cost anything for us to have the same thing both places, and even before the change, the same comment about how "filtering happens later" in the old code was often in both places anyways, so I just went with 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.
I definatly considered it, but it doesn't seem to matter much so I made a, I think, quick and reasonable choice and moved on. It also avoids broader re-works for a more narrow changeset.
Do you have any concerns with having both specifically? What kind of problem will that introduce?
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.
My main concern is adding code that isn't necessary to achieve the desired behavior and may never execute.
I think the "original sin" was 1d6b036, which inlined all these methods (not disparaging; it made sense at the time). These methods made sense as base class defaults, but not necessarily included in each access class.
That said, especially since this is v2 only, it's also reasonable to just continue the current pattern, messy as it is. I'm not going to block on this.
@@ -27,8 +27,7 @@ def update?(_object, _params=nil) | |||
end | |||
|
|||
def index?(_object_class, _params=nil) | |||
# This can return true because the index endpoints filter objects based on user visibilities | |||
true | |||
admin_user? || admin_read_only_user? || has_read_scope? || global_auditor? || !VCAP::CloudController::FeatureFlag.enabled?(:hide_marketplace_from_unauthenticated_users) |
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'm not super excited about including business logic here, but there is some prior art for checking feature flags in the v2 access classes: https://github.com/search?q=repo%3Acloudfoundry%2Fcloud_controller_ng+path%3A%2F%5Eapp%5C%2Faccess%5C%2F%2F+FeatureFlag&type=code
If this logic needs to go here, can/should we remove the corresponding logic in the controller?
cloud_controller_ng/app/controllers/services/services_controller.rb
Lines 28 to 39 in 7b1848e
allow_unauthenticated_access only: :enumerate | |
def enumerate | |
if SecurityContext.missing_token? | |
raise CloudController::Errors::NotAuthenticated if VCAP::CloudController::FeatureFlag.enabled?(:hide_marketplace_from_unauthenticated_users) | |
@opts.delete(:inline_relations_depth) | |
elsif SecurityContext.invalid_token? | |
raise CloudController::Errors::InvalidAuthToken | |
end | |
super | |
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.
If this logic needs to go here, can/should we remove the corresponding logic in the controller?
possibly? Do you want me to?
having this logic in the access controller
I'd rather there not be that code in the access controller at all tbh, but i would have done that by just, never anymore allowing unauthorized users to access the marketplace endpoint. Given we have what I'd, honestly consider a "compatibility flag" more then a "feature flag"(as I mentioned in my comment), I figured I should try to respect that flag unless we're thinking we're ok with dropping it.
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.
Is it ever possible for the services controller code to execute? If the InvalidAuthToken
and InvalidAuthToken
cases are already handled elsewhere, then I think we should delete the vestigial code.
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'll try and look into this and remove the redundancy.
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.
hmmm. I did "quick" checks. I'll do more tomorrow. It appears to be not possible to quickly remove.
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'm doing research, It seems like service and service_plan access effect /v2/service calls, but weirdly enough not /v2/service_plan calls? I'm not sure what to do.
Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:
Require cloud_controller.read access(or equivalent) to access list endpoints.
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
main
branchI have run all the unit tests using
bundle exec rake
I have run CF Acceptance Tests