FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ROX-18155: pg generic store: Exists by janisz · Pull Request #6766 · stackrox/stackrox · GitHub

/ stackrox Public

ROX-18155: pg generic store: Exists - #6766

Merged
janisz merged 3 commits into
masterfrom
master-janisz/06-30-ROX-18155_pg_generic_store_Exists
Jul 6, 2023
ConversationCommits (3)ChecksFiles changed
Merged

ROX-18155: pg generic store: Exists#6766
janisz merged 3 commits into
masterfrom
master-janisz/06-30-ROX-18155_pg_generic_store_Exists

Conversation

janisz commented Jun 30, 2023

Copy link
Copy Markdown
Contributor

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

  • Investigated and inspected CI test results
  • Unit test and regression tests added
  • Evaluated and added CHANGELOG entry if required
  • Determined and documented upgrade steps
  • Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

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.

janisz requested review from a team as code owners June 30, 2023 16:18

janisz commented Jun 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

This was referenced Jun 30, 2023
Merged
Merged
This was referenced Jun 30, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged

ghost commented Jun 30, 2023 •
edited by ghost
Loading

Copy link
Copy Markdown

Images are ready for the commit at 6bddaf3.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.1.x-286-g6bddaf35c4.

janisz mentioned this pull request Jun 30, 2023
Closed
5 tasks
rhybrillou requested changes Jul 3, 2023
View reviewed changes

rhybrillou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In the case the permission checker denies access to the object, I think a behaviour change is introduced that leaks information about the user allowed scope. I'd rather stick to the old behaviour.

Comment thread pkg/search/postgres/store.go
Comment thread pkg/search/postgres/store.go Outdated
setPostgresOperationDurationTime durationTimeSetter
setAcquireDBConnDuration durationTimeSetter
permissionChecker PermissionChecker
pkGetter primaryKeyGetter[T, PT]

rhybrillou Jul 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

super-nit: field order -> move targetResource and permissionChecker to the end of the list (makes it easier to compare with NewGenericStore and NewGenericStoreWithPermissionChecker)

Comment thread pkg/search/postgres/store.go
Comment thread 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

rhybrillou Jul 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread tools/generate-helpers/pg-table-bindings/store.go.tpl Outdated
janisz requested a review from rhybrillou July 3, 2023 15:51
janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_Exists branch from c6001f4 to 6c69e38 Compare July 3, 2023 16:01

janisz commented Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

/retest

rhybrillou reviewed Jul 4, 2023
View reviewed changes
Comment thread pkg/search/postgres/store.go
Comment thread pkg/search/postgres/store.go
parametalol reviewed Jul 4, 2023
View reviewed changes
Comment thread pkg/search/postgres/store.go Outdated
parametalol reviewed Jul 4, 2023
View reviewed changes
Comment thread pkg/search/postgres/store.go
Comment on lines +94 to +98
if ok, err := s.permissionChecker.ExistsAllowed(ctx); err != nil {
return false, err
} else if !ok {
return false, nil
}

parametalol Jul 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
if ok, err := s.permissionChecker.ExistsAllowed(ctx); err != nil {
return false, err
} else if !ok {
return false, nil
}
return s.permissionChecker.ExistsAllowed(ctx)

janisz Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 July 4, 2023 13:22

openshift-ci Bot commented Jul 4, 2023

Copy link
Copy Markdown

@janisz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ocp-4-10-qa-e2e-tests 6bddaf3 link false /test ocp-4-10-qa-e2e-tests

Full PR test history. Your PR dashboard.

Details

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.

janisz commented Jul 6, 2023

Copy link
Copy Markdown
Contributor Author

@janisz started a stack merge that includes this pull request via Graphite.

janisz merged commit a3ab4c7 into master Jul 6, 2023
janisz deleted the master-janisz/06-30-ROX-18155_pg_generic_store_Exists branch July 6, 2023 09:57

janisz commented Jul 6, 2023

Copy link
Copy Markdown
Contributor Author

@janisz merged this pull request with Graphite.

This was referenced Jul 12, 2023
Merged
Merged
Merged
Merged
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

parametalol parametalol approved these changes
rhybrillou rhybrillou approved these changes

Assignees

No one assigned

Labels

area/central area/postgres

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL