brycx / pasetors

PASETOrs: PASETO tokens in pure Rust
MIT License
83 stars 8 forks source link

Maybe Claims::new() should accept a Duration parameter? #96

Closed OwenChia closed 1 year ago

OwenChia commented 1 year ago

Thanks for your hard work, it's a very good crate.

it's all works perfectly, expect when i trying to change the expiration time. for example, using Claims::new() will create a claims expired in one hours, it's works fine. but when i trying to change this, there is no easy way to do that, i had to write a helper function, like

fn helper_change_exp(claims: &mut Claims, duration: &Duration) {
  if let Some(Value::String(iat) = claims.get_claim("iat") {
    let iat = OffsetDateTime::parse(&iat, &Rfc3999).unwrap();
    let exp = (iat + *duration).format(&Rfc3999).unwrap();
    claims.expiration(&exp).unwrap();
}

or

fn helper_change_exp(claims: &mut Claims, duration: &Duration) {
  let iat = OffsetDateTime::now_utc();
  let nbf = iat;
  let mut exp = iat;
  exp += *duration;

  claims.issued_at(&iat.format(&Rfc3339).unwrap()).unwrap();
  claims.not_before(&nbf.format(&Rfc3339).unwrap()).unwrap();
  claims.expiration(&exp.format(&Rfc3339).unwrap()).unwrap();
}

if the Claims::new() can accept a Duration, it's will be more user friendly, like

pub fn new(duration: &Duration) -> Result<Self, Error> {
        let iat = OffsetDateTime:now:now_utc();
        let nbf = iat;
        let mut exp = iat;
        exp += *duration;

        let mut claims = Self {
            list_of: HashMap::new(),
        };

        claims.issued_at(&iat.format(&Rfc3339).map_err(|_| Error::InvalidClaim)?)?;
        claims.not_before(&nbf.format(&Rfc3339).map_err(|_| Error::InvalidClaim)?)?;
        claims.expiration(&exp.format(&Rfc3339).map_err(|_| Error::InvalidClaim)?)?;

        Ok(claims)
    }

then we can use Claims::new(&Duration::hours(3)), less code and easy to use. could you please considering add this? or maybe add a new new() function with another name for compatibility?

brycx commented 1 year ago

Thank you for the kind words, @OwenChia!

It's true, there's a bit of boilerplate conversion happening here, which maybe could be improved.

My first concern here is, when you mention Duration do you mean that of the time crate or that of Rust std? I'm generally against putting third-party crate types into the public API of this one, because then breaking changes in that must be synced with this one.

I see that time does implement TryFrom<core::time::Duration> for time::Duration, so I'd be open to add a function accepting the Rust std Duration type and having the TryFrom call internally, to reduce boilerplate.

Given this, I can't see an issue in adding a Claims::new_expires_in(duration: &Duration).

OwenChia commented 1 year ago

I was just using Duration as an example, without referring to any specific type. Claims::new_expires_in() looks good to me.

brycx commented 1 year ago

Released as part of version 0.6.7 just now.

CM-IV commented 1 year ago

Here's how I've been using chrono::Duration to change the expiry time for 30 minutes after creation:

image