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

treat cycles as falsy outcome in intersection and exclusion CheckFuncReducers #1372

Merged
merged 11 commits into from
Feb 16, 2024

Conversation

jon-whit
Copy link
Member

@jon-whit jon-whit commented Feb 16, 2024

Description

The changes in this PR adjust the intersection and exclusion CheckFuncReducers to treat a loopback cycle effectively as {allowed: false}. The idea being that if you could have resolved something in the resolution path then you would have, otherwise you can return {allowed: false} explicitly. ErrCycleDetected is not an error that should be considered a critical error like transient errors and/or ErrResolutionDepthExceeded. It should be considered the same as a falsy result because it signals that a resolution branch was exhausted without any explicitly truthy or falsy outcome. Treating this error in this specific way within the intersection and exclusion reducers will result in more correct and fault-tolerant Check resolution.

I've also added some tests for the intersection and exclusion reducers so that this behavior is more apparent in the future as well. The tests I added also include behavior around context cancellation, so that will help us avoid accidentally introducing regression in cancellation of in-progress subproblem resolution.

References

Related to #1371

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

@jon-whit jon-whit requested a review from a team as a code owner February 16, 2024 06:25
Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

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

Agree in principal to these changes.

internal/graph/check.go Outdated Show resolved Hide resolved
internal/graph/check.go Outdated Show resolved Hide resolved
assets/tests/consolidated_1_1_tests.yaml Outdated Show resolved Hide resolved
Copy link
Member

@rhamzeh rhamzeh left a comment

Choose a reason for hiding this comment

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

I would caution against ignoring errors in Check. We did the same mistake in ListRelations in the SDK and been wanting to revert since then.

You are assuming all checks are of the form:

can user:anne read document 1?

Which implies if we cannot say that Anne can read, it implies that Anne cannot.

But this is not the only possibility. For example, a check may be on a relation with negative connotations, for example:

is user:anne blocked from document 1?

Now imagine someone calling FGA from a different context (e.g. OPA), so FGA is functioning as an information point instead of a decision point. In OPA the policy might be:

Anne can access the document, if it is tagged "marketing" and "published" and not blocked in FGA

By us returning false, we are indicating that we know 100% that anne is not blocked, so OPA will assume that and the user will be granted access.

indeterimite answers (such as errors) should be ignored ONLY if another path 100% guarantees a determinate answer, for example (true or error is always true, and false AND error is always false), because no matter what the value of error is, we know the final outcome.

For the full cases where error can be ignored:

true OR true = true
true OR false = true
true OR error = true # error can be ignored
false OR true = true
false OR false = false
false OR error = error # error CANNOT be ignored
error OR error = error # error CANNOT be ignored
true AND true = true
true AND false = false
true AND error = error # error CANNOT be ignored
false AND true = false
false AND false = false
false AND error = false # error can be ignored
error AND error = error # error cannot be ignored
true BUT NOT true = false
true BUT NOT false = true
true BUT NOT error = error # error CANNOT be ignored
false BUT NOT true = false
false BUT NOT false = false
false BUT NOT error = false # error can be ignored
error BUT NOT error = error # error CANNOT be ignored

@willvedd
Copy link
Contributor

@rhamzeh This is not ignoring all errors but effectively considering ErrCycleDetected as allowed=false. In fact, I'd argue that it's not even an error at all during check evaluations because the graph does indeed exhaust all possible nodes along the cyclical branch. I'd even go one step further and say that cycles are determinate especially because their behavior is consistent and repeatable. This is in contrast to other errors that are intermittent and do not exhausted the nodes along a resolution branch.

jon-whit and others added 2 commits February 16, 2024 12:26
Co-authored-by: Maria Ines Parnisari <maria.inesparnisari@okta.com>
willvedd
willvedd previously approved these changes Feb 16, 2024
Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

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

Great! Agree with the decisions here.

@jon-whit jon-whit merged commit 1843929 into main Feb 16, 2024
13 checks passed
@jon-whit jon-whit deleted the adjust-and-test-intersection-and-exclusion-reducers branch February 16, 2024 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants