-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add pure version of authorizeBiscuit #79
base: main
Are you sure you want to change the base?
Conversation
Thanks! I guess that apart from Do note that there is planned support for ECDSA, which would turn some currently pure operations into monadic ones (but it should only add a |
biscuit/src/Auth/Biscuit/Token.hs
Outdated
@@ -578,6 +581,24 @@ authorizeBiscuitWithLimits l biscuit@Biscuit{..} authorizer = | |||
(toBlockWithRevocationId <$> blocks) | |||
authorizer | |||
|
|||
authorizeBiscuitWithLimitsNoTimeout :: Limits -> Biscuit proof Verified -> Authorizer -> Either ExecutionError (AuthorizedBiscuit proof) | |||
authorizeBiscuitWithLimitsNoTimeout l biscuit@Biscuit{..} authorizer = | |||
let toBlockWithRevocationId ((_, block), sig, _, eSig) = (block, sigBytes sig, snd <$> eSig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on one hand it might be possible to extract this common code in a helper function, but OTOH it's just plumbing with no important logic in it so it's not too big an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this PR was more a proof-of-concept than a definitive alternative. I wondered if it would be better to factor out the common parts but I went for the easiest path first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The common part could be authorizeBiscuitWithRunner :: Functor f => (Limits -> BlockWithRevocationId -> [BlockWithRevocationId] -> Authorizer -> f (Either ExecutionError AuthorizationSuccess)) -> Limits -> Biscuit proof Verified -> Authorizer -> f (Either ExecutionError (AuthorizedBiscuit proof))
and it would have the exact same code as the current authorizeBiscuitWithLimits
with runAuthorizerWithLimits
replaced by the first argument.
Then authorizeBiscuitWithLimitsNoTimeout = unIdentity . authorizeBiscuitWithRunner runAuthorizerNoTimeout
…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I had in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to the PR.
@@ -578,6 +581,24 @@ authorizeBiscuitWithLimits l biscuit@Biscuit{..} authorizer = | |||
(toBlockWithRevocationId <$> blocks) | |||
authorizer | |||
|
|||
authorizeBiscuitWithLimitsNoTimeout :: Limits -> Biscuit proof Verified -> Authorizer -> Either ExecutionError (AuthorizedBiscuit proof) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a haddoc comment explaining its purpose and outlining the danger?
it might good to refer to it from the doc items of authorizeBiscuitWithLimits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know Prolog can easily have infinite loops but I don't know about Datalog or the way it's evaluated in Biscuit, so I'll write the doc warning about that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semantics for this Datalog flavour means it always converges, so it's not about infinite loops per se, but about ensuring that it does within an acceptable duration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment fixed.
BTW, our use case is that we have Polysemy actions running in Servant, and it's not yet clear to us where we'd better validate or generate biscuits in our code, but the current implementation would force us to only use functions from Having functions that only require |
Have you looked at biscuit-servant yet? My plan was to try it out with an effect system (probably effecful rather than polysemy though).
Disabling an important check to please an effect system seems a bit dangerous to me. OTOH relaxing IO to MonadRandom for key generation preserves semantics and would unlock nice use cases. It might be possible to declare the timeout check in a specific typeclass implemented by IO (the actual impl requires getCurrentTime and forkIO amongst other things) to make integrating with an effect system a bit easier |
We are indeed looking at biscuit-servant. At first glance, it would seem useful if we can use biscuit verification without needing effects like Is there a way to help you try testing an effect system with |
Biscuit's Datalog flavour always converges.
I don't use Haskell at work anymore so i might not have time to do that before a couple weeks. That being said, all the servant helpers are polymorphic on the handler monad, so this should work with effects anyway. Extracting the timeout check in a dedicated typeclass would make things easier if you want to forego timeouts (and IO) completely |
👋 I might have time to assist if you want to try and express the timeout check through a typeclass constraint, that could be discharged by IO itself, or by a dedicated effect. |
Great! How would you want to proceed? |
Once you have rebased the PR to fix conflicts, maybe we can pair-prog on it and try to come up with a typeclass that can be compatible with an effect system (ideally effectful and polysemy) |
I think I'd be available enough in a few weeks to do that, that sounds like a plan. |
A lot of biscuit's functions that require
IO
orMonadIO
could have either pure versions or versions that could run in a wider range of monads.authorizeBiscuit
may be the simplest because its only dependency onIO
is because oftimer
.This would still require documenting that it must be run in a context where a bad biscuit wouldn't make
authorizeBiscuitNoTimeout
run for an extended time.