Skip to content
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

Policy: GetAttributesByValueFqns RPC request validation should happen in protovalidate rules #1651

Closed
ryanulit opened this issue Oct 15, 2024 · 1 comment · Fixed by #1657

Comments

@ryanulit
Copy link
Contributor

ryanulit commented Oct 15, 2024

Background

Originally brought up in a separate PR: https://github.com/opentdf/platform/pull/1633/files/e34689983f9be1b52af49254f5ccfa7eb8baf5f9#r1801590942

The request validation checks at the beginning of the RPC method should be moved to protovalidate rules. Additionally, see comments below for other changes.

Acceptance Criteria

  • remove r.GetWithValue() nil check and any existing protovalidate rules requiring it in the request, then add an optional comment on the request proto field
  • move fqns array length check to protovalidate with a min and max items range of 1-250 (sensible default for now)
  • unit test coverage for new rules
@ryanulit ryanulit changed the title feat(policy): move request validation in policy GetAttributesByValueFqns RPRC to protovalidate rules feat(policy): move request validation in policy GetAttributesByValueFqns RPC to protovalidate rules Oct 15, 2024
@ryanulit ryanulit changed the title feat(policy): move request validation in policy GetAttributesByValueFqns RPC to protovalidate rules feat(policy): policy GetAttributesByValueFqns RPC request validation should happen in protovalidate rules Oct 15, 2024
@ryanulit ryanulit changed the title feat(policy): policy GetAttributesByValueFqns RPC request validation should happen in protovalidate rules Policy: GetAttributesByValueFqns RPC request validation should happen in protovalidate rules Oct 15, 2024
@ryanulit
Copy link
Contributor Author

ryanulit commented Oct 16, 2024

After further discussion with @jakedoublev and @jrschumacher, we've decided the FQN /value/ format validation is no longer necessary and can be removed since the request will return Not Found if any of the FQNs are invalid. Additionally, r.GetWithValue() is not even used anywhere in the GetAttributesByValueFqns DB layer function except for required request validation, so this will be made optional now but remain on the request proto for future use. The description will be updated accordingly.

github-merge-queue bot pushed a commit that referenced this issue Nov 4, 2024
…on to protovalidate (#1657)

fix #1651

---------

Co-authored-by: Jake Van Vorhis <[email protected]>
Co-authored-by: jakedoublev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant