Bearer’s expires field gets scrambled after serialize → deserialize round-trip

[silmeth]

When Bearer is deserialized it expects the expires_in field in seconds-from-now, and adds a Utc::now() to it in order to obtain a fixed non-relative UTC instant. That makes sense.

But then when it is serialized it directly serializes expires.timestamp() (Unix timestamp in secodns) as the expires_in field – which is not consistent.

This means that if you save the serialized bearer value eg. for session keeping then each time you deserialize it you add a Unix timestamp Utc::now() worth amount of seconds to the expires field, making it unusable.

There should be either type for the serialize-deserialize round-trip (a SerializableBearer type that just has expires field?) or some other way to restore it from serialization.

[Lesny]

I created PR with example fix https://github.com/kilork/openid/pull/48/files

[kyrias]

@kilork Is there anything blocking this?

I'd rather like to use this crate for our new service, but having to manually convert to and from the Bearer type to store it in a session seems like a bit of a hassle.

[kilork]

Should we try to do anything about this field actually? I am thinking about removing original field and providing the structure to use as wrapper instead.

We could have

...
struct Bearer {
  expires_in: Option<u64>,
  ...
}

...
struct BearerReceived {
  bearer: Bearer,
  expires_at: Option<DateTime<Utc>>,
}

More simple design should be better and one who consumes library can decide, how to store it better, or use our provided wrapper. What do you think?

Also we do not really use this field in library, only part responsible for token refresh. Which is most probably not used by anyone directly, as id_token has its own exp field.

[kyrias]

Personally I only care about it in that I need a reliable way to do token refreshing, but since my use-case involves ID tokens I don't necessarily need it, as you pointed out, so as long as there's a reliable way to check whether or not the token has expired then I don't really care how it's actually stored. Theoretically there might be people using OIDC without actually receiving an ID token though, but I'm not sure why anyone would.

Though BearerReceived feels a bit misleading when the only thing it contains in addition to the Bearer struct is derived data which wasn't received.

[kilork]

@kyrias this is the most difficult part always. To name the things. I wanted to name it so that it shows the "Bearer After Received" state, because the lifetime is counted from when the response is created, and when it is finally received - we can plot the expiration date. But of course, if it's misleading, it would be nice to have a better suggestion.