Skip to content

Ensure all resources are returned for relation check when caveats are specified - #2027

Merged
josephschorr merged 1 commit into
authzed:mainfrom
josephschorr:caveat-check-fix
Aug 15, 2024
Merged

Ensure all resources are returned for relation check when caveats are specified#2027
josephschorr merged 1 commit into
authzed:mainfrom
josephschorr:caveat-check-fix

Conversation

@josephschorr

Copy link
Copy Markdown
Member

Fixes #2026

@josephschorr
josephschorr requested a review from a team August 15, 2024 22:10
@github-actions github-actions Bot added area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) area/dispatch Affects dispatching of requests labels Aug 15, 2024
tstirrat15
tstirrat15 previously approved these changes Aug 15, 2024

@tstirrat15 tstirrat15 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.

LGTM, see comment

Comment thread internal/graph/check.go Outdated

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.

Understanding check:

The issue is that we were short-circuiting when we found a single relation which matched the check but whose caveat was false and calling it false.

This fix says that when we have a caveat, we must find all relations that match the check so that we can evaluate the caveats on all of them and then OR them together so that we can decide whether the overall check was true.

The more efficient fix would be to continue evaluating relationships that match the check until we find one that also matches the caveat or we exhaust the set.

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.

Ah I'm reading slack and realizing that my "more efficient fix" isn't how it would work for the caching reasons described.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct. We cannot just eval here because we build the full caveat expression and then evaluate outside of the dispatcher.

ecordell
ecordell previously approved these changes Aug 15, 2024

@ecordell ecordell 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.

LGTM

@josephschorr
josephschorr added this pull request to the merge queue Aug 15, 2024
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 15, 2024
@josephschorr
josephschorr added this pull request to the merge queue Aug 15, 2024
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 15, 2024

@ecordell ecordell 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.

it would be nice if there were a helper that could assert that two cel expressions are equivalent, instead of providing two options in the test

@josephschorr
josephschorr added this pull request to the merge queue Aug 15, 2024
Merged via the queue into authzed:main with commit d4ef8e1 Aug 15, 2024
@josephschorr
josephschorr deleted the caveat-check-fix branch August 15, 2024 22:59
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area/dispatch Affects dispatching of requests area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permission check ignores further relationships when a relationship with caveat does not match

3 participants