Honor child scope list permissions when recursing #1016
Conversation
Current behavior is that you need list grant on the parent scope when recursively listing, and this is sufficient. This is good for administrative workflows but can be unintuitive for resources that only exist at a project level. This change allows permissions on the child scope to also be taken into account, so that you can start a recursive list from a parent scope without direct list support on that scope but still get back child scopes for which list access is granted. This also fixes a bug where visibility of a scope for recursive listing would be based on whether the user had list resources for roles within that scope instead of for the resource type in question.
| // If it's forbidden, and it's a recursive request, and they're | ||
| // successfully authenticated but just not authorized, keep going as we | ||
| // may have authorization on downstream scopes. | ||
| if authResults.Error == handlers.ForbiddenError() && |
There was a problem hiding this comment.
is there a anyway to provide a func that combines authResults(...) and GetScopeIds(...) that could be used in all the List functions? It seems like this block is going to be copied quite a bit.
There was a problem hiding this comment.
I'll do that as a follow after this PR. I wanted to keep changes minimal (as opposed to additions) for easy reviewing.
There was a problem hiding this comment.
In this PR the changes to: Internal/common/scopeids/scope_ids.go have no unit tests.
There are quite a few missing unit tests in the packages touched by this PR. I’m not suggesting that all these missing tests be addressed in this PR, but we should note them and address them in future PRs:
- internal/auth/auth.go could use some better coverage for things like the recovery KMS code branches.
- internal/perms/options.go -> it might be nice to have unit tests for these options
- internal/servers/controller/handlers/errors.go -> no unit tests for NotFoundError(), Is(..), ApiErrorWithCode(…), ForbiddenError(…), UnauthenticatedError()
- internal/servers/controller/handlers/mask_manager.go -> no units for MaskContains(…)
- internal/servers/controller/handlers/outgoing_interceptor.go -> OutgoingInterceptor(…) missing tests for the case where *pbs.AuthenticateLoginResponse
- internal/servers/controller/handlers/authmethods/authmethods_service.go -> currently no unit tests for Service.AuthenticateLogin(…)
- internal/servers/controller/handlers/targets/target_service.go -> currently no unit tests for Service.AuthorizeSession(…)
|
@jimlambrt As discussed, updated the tests. They can be simplified more -- right now I haven't changed the logic to copy part of the group service code -- but at this point that's merely cleanup as the test just goes further than it needs to. So it's ready for another look. |
internal/servers/controller/handlers/groups/group_service_test.go
Outdated
Show resolved
Hide resolved
Current behavior is that you need list grant on the parent scope when
recursively listing, and this is sufficient. This is good for
administrative workflows but can be unintuitive for resources that only
exist at a project level. This change allows permissions on the child
scope to also be taken into account, so that you can start a recursive
list from a parent scope without direct list support on that scope but
still get back child scopes for which list access is granted.
This also fixes a bug where visibility of a scope for recursive listing
would be based on whether the user had list resources for roles within
that scope instead of for the resource type in question.