instructure / paseto

A paseto implementation in rust.
MIT License
151 stars 14 forks source link

[FR] Also support the time crate #31

Closed Weasy666 closed 3 years ago

Weasy666 commented 3 years ago

Is your feature request related to a problem? Please describe. As of now, the builder accepts only chrono types for set_issued_at, and so on, but in my application i only use the time crate on don't want to pull in chrono as a dependency just for the builder.

Describe the solution you'd like A feature flag to choose between chrono and time would be really great.

Describe alternatives you've considered An alternative would be to pass a UNIX timestamp, but that is not really feasible, as it would need a conversion to ISO8601 time, which i guess would end up in using chrono for the conversion. Something else would be to accept a String, but then all type safety and guaranties that a correctly ISO8601 was passed in would be gone. So i think this is not an option.

So...would be open to such a feature flag? Then i could open a PR.

Mythra commented 3 years ago

Hey @Weasy666, this seems more than fine to me to have an option to use time over chrono. Though I'd prefer if we keep chrono as the default if that's okay for you?

Weasy666 commented 3 years ago

Sure, otherwise it would be breaking change for current users. 😀

Weasy666 commented 3 years ago

Oh...i still have a question, about how you want me to do this. As far as i know, cargo features need to be additive and as such the crate needs to be able to be build both features activated. So...i would maybe replicate the two or three functions, like this

#[cfg(feature = "easy_tokens_chrono")]
pub fn set_issued_at(&'a mut self, issued_at: Option<DateTime<Utc>>) -> &'a mut Self

#[cfg(feature = "easy_tokens_time")]
pub fn set_issued_at(&'a mut self, issued_at: Option<OffsetDateTime>) -> &'a mut Self

#[cfg(all(feature = "easy_tokens_chrono", feature = "easy_tokens_time"))]
pub fn set_issued_at_chrono(&'a mut self, issued_at: Option<DateTime<Utc>>) -> &'a mut Self

#[cfg(all(feature = "easy_tokens_chrono", feature = "easy_tokens_time"))]
pub fn set_issued_at_time(&'a mut self, issued_at: Option<OffsetDateTime>) -> &'a mut Self

and hope that this works 😅. Would this be ok for you?

Mythra commented 3 years ago

Those functions look fine to me! Although we probably want the first cfg's to be: #[cfg(all(feature = "easy_tokens_<time>", not(feature = "easy_tokens_<other_time>")))] (I believe that's the right syntax but it's been a bit).

Weasy666 commented 3 years ago

Hm....i have bit of problem building master branch with cargo build --no-default-features --features easy_tokens.

error[E0392]: parameter `'a` is never used
  --> src\tokens\mod.rs:22:26
   |
22 | pub enum PasetoPublicKey<'a> {
   |                          ^^ unused parameter
   |
   = help: consider removing `'a`, referring to it in a field, or using a marker such as `std::marker::PhantomData`
Mythra commented 3 years ago

You need to also specify at least one version of the algorithm to use (either V2, or V1). You can do that by adding a comma:

cargo build --no-default-features --features easy_tokens,v2

For example

Weasy666 commented 3 years ago

Ah....ok, wasn't obvious to me that i need to use both.

Mythra commented 3 years ago

Yeah, we can open up an issue to document that better. But you need a token version supported as otherwise what tokens would easy_tokens build? It could just error at runtime, but we personally think it's better to know at build time that this thing would never work.

Mythra commented 3 years ago

fixed by #32 thanks again! 🥳