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

Unsuccessful Datalog checks don't return distinguished error #152

Open
seh opened this issue Nov 6, 2024 · 5 comments
Open

Unsuccessful Datalog checks don't return distinguished error #152

seh opened this issue Nov 6, 2024 · 5 comments

Comments

@seh
Copy link
Contributor

seh commented Nov 6, 2024

If I include an attenuating check in my biscuit such as check if time($time), $time < {time}—where the {time} placeholder is replaced by an expiration horizon that winds up at or later than the time fact bound during evaluation, then the (*authorizer).Authorize method fails with a freshly constructed errors.errorString, not using or wrapping any sentinel error value or type that one could detect with the errors.Is or errors.As functions.

Callers can't detect when authorization fails due to an unsatisfied check, whereas they can detect when a policy denies authorization via the ErrPolicyDenied value.

Would you be amenable to introducing a new sentinel error value such as ErrCheckUnsatisfied, ErrUnsatisfiedCheck, or ErrCheckNotSatisfied? Using a value from errors.New would be a little bit awkward, as we'd have to wrap it in a way that the resulting message produced by the String method is still sensible. We could introduce a new type that satisfies the error interface instead.

Another question is whether we'd want to allow callers to distinguish between unsatisfied authority checks and other non-authority checks that are unsatisfied.

@seh
Copy link
Contributor Author

seh commented Nov 6, 2024

Oh, I just noticed that we collect all of these individual errors from unsuccessful checks and join them as strings, rather than combining the errors via the errors.Join function.

@divarvel
Copy link
Contributor

divarvel commented Nov 7, 2024

I have limited knowledge in the go standard libs and its idioms, so contributions on this front would be great.
I can help aligning the API with the expected use of the datalog engine and the token lifecycle, but I’m not well suited to make it idiomatic go

@seh
Copy link
Contributor Author

seh commented Nov 7, 2024

I'm willing to attempt to improve this error reporting, while retaining backward compatibility for existing callers—modulo the exact error message text that would result from these check failures.

Do you think that callers should be able to distinguish between unsatisfied checks in the authority block versus checks in following blocks?

@divarvel
Copy link
Contributor

I think that would be useful, yes.

For reference, the rust implementation exposes quite detailed information about the failed checks / policies:

https://github.com/biscuit-auth/biscuit-rust/blob/main/biscuit-auth/src/error.rs#L174

Having the check code without the block number is not enough in the general case because the authority blocks and attenuation blocks don’t see the same facts.

@seh
Copy link
Contributor Author

seh commented Nov 13, 2024

At present, we include the block number in the error message, and could continue to do so.

What I was asking was whether a caller should be able to use functions like errors.Is and errors.As to distinguish between errors arising from the authority block and following blocks by type or sentinel value, or whether it's enough to have callers recognize block zero as the authority block.

In Rust, you distinguish these with separate types: the FailedAuthorizerCheck type omits the "block_id" field that's present in the similar FailedBlockCheck type, presumably because it's implied as being block zero.

I can attempt to adapt some of these ideas into the Go library, with an eye toward maintaining backward compatibility for existing callers.

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

No branches or pull requests

2 participants