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

feat: update definitions to support ABAC Conditions #90

Merged
merged 19 commits into from
Sep 14, 2023
Merged

Conversation

jpadilla
Copy link
Member

@jpadilla jpadilla commented Aug 21, 2023

Description

NOTE: this PR targets a new branch feat/abac

  • ListObjectsRequest now accepts context
  • StreamedListObjectsRequest now accepts context
  • WriteAuthorizationModelRequest now accepts conditions
  • CheckRequest now accepts context
  • ExpandRequest stops accepting user

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@jpadilla jpadilla marked this pull request as ready for review August 23, 2023 15:25
@jpadilla jpadilla requested a review from a team as a code owner August 23, 2023 15:25
@jpadilla jpadilla changed the base branch from main to feat/abac August 23, 2023 18:43
docs/openapiv2/apidocs.swagger.json Outdated Show resolved Hide resolved
openfga/v1/authzmodel.proto Outdated Show resolved Hide resolved
@rhamzeh rhamzeh marked this pull request as draft August 29, 2023 00:13
@rhamzeh rhamzeh marked this pull request as ready for review August 29, 2023 01:10
message ConditionParamTypeRef {
enum TypeName {
TYPE_NAME_UNSPECIFIED = 0;
TYPE_NAME_ANY = 1;
Copy link
Member

Choose a reason for hiding this comment

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

We're keeping any?

Copy link
Member

Choose a reason for hiding this comment

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

If in the future we'll introduce it, it makes sense to have it here so that we reserve the field number

* feat: adding condition for tuple key
Comment on lines 30 to 31
min_len: 1,
max_len: 50,
Copy link
Member

@miparnisari miparnisari Aug 31, 2023

Choose a reason for hiding this comment

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

Here and elsewhere in this PR: why do we need this? seems redundant. In the past, we removed these two: see #4

Whenever we have a mix of min_len/max_len and pattern validation, remove the min_len and max_len in favor of an updated pattern that includes the length.

Copy link
Member Author

Choose a reason for hiding this comment

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

thinking is that the error returned in the case that length isn't within constraints is better value length must be between 1 and 50 runes, inclusive as opposed to value does not match regex pattern. It is redundant and likely a slight performance hit, I'll remove for now.

miparnisari added a commit that referenced this pull request Sep 8, 2023
miparnisari added a commit that referenced this pull request Sep 8, 2023
@miparnisari miparnisari merged commit aa7d2ff into feat/abac Sep 14, 2023
@miparnisari miparnisari deleted the abac branch September 14, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants