biscuit-auth / biscuit-go

Apache License 2.0
75 stars 22 forks source link

Unsuccessful Datalog checks don't return distinguished error #152

Open seh opened 2 weeks ago

seh commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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.