ROX-18155: pg generic store: Exists #6766
Conversation
|
Images are ready for the commit at 6bddaf3. To use with deploy scripts, first |
pkg/search/postgres/store.go
Outdated
| setPostgresOperationDurationTime durationTimeSetter | ||
| setAcquireDBConnDuration durationTimeSetter | ||
| permissionChecker PermissionChecker | ||
| pkGetter primaryKeyGetter[T, PT] |
There was a problem hiding this comment.
super-nit: field order -> move targetResource and permissionChecker to the end of the list (makes it easier to compare with NewGenericStore and NewGenericStoreWithPermissionChecker)
pkg/search/postgres/store.go
Outdated
| if ok, err := s.permissionChecker.ExistsAllowed(ctx); err != nil { | ||
| return false, err | ||
| } else if !ok { | ||
| return false, sac.ErrResourceAccessDenied |
There was a problem hiding this comment.
This changes the behaviour compared to the generated code.
I'd stick to the previous behaviour that returns false, nil, as if the item did not exist, rather than an error that already tells that the item exists. I think from a security point of view, the new behaviour is a data leak.
janisz
force-pushed
the
master-janisz/06-30-ROX-18155_pg_generic_store_Exists
branch
from
c6001f4
to
6c69e38
Compare
|
/retest |
| if ok, err := s.permissionChecker.ExistsAllowed(ctx); err != nil { | ||
| return false, err | ||
| } else if !ok { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
| if ok, err := s.permissionChecker.ExistsAllowed(ctx); err != nil { | |
| return false, err | |
| } else if !ok { | |
| return false, nil | |
| } | |
| return s.permissionChecker.ExistsAllowed(ctx) |
There was a problem hiding this comment.
we cannot do that as this does not handle true correctly.
janisz
force-pushed
the
master-janisz/06-30-ROX-18155_pg_generic_store_Exists
branch
from
21509c8
to
6bddaf3
Compare
|
@janisz: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Description
A detailed explanation of the changes in your PR.
Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.
Checklist
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why
you did not do so. Valid reasons include, for example, "CI is sufficient",
"No testable changes". Feel free to attach JSON snippets, curl commands,
screenshots.
In addition to reviewing your code, reviewers must also review your testing
instructions and make sure they are sufficient.