Skip to content
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

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

Closed
silmeth opened this issue Sep 28, 2023 · 5 comments · Fixed by #49
Closed

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

silmeth opened this issue Sep 28, 2023 · 5 comments · Fixed by #49

Comments

@silmeth
Copy link

silmeth commented Sep 28, 2023

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.

@silmeth silmeth changed the title Bearer’s expire field gets scrambled after serialize → deserialize round-trip Bearer’s expires field gets scrambled after serialize → deserialize round-trip Sep 28, 2023
@Lesny
Copy link

Lesny commented Sep 28, 2023

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

@kyrias
Copy link

kyrias commented Feb 15, 2024

@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
Copy link
Owner

kilork commented Feb 15, 2024

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
Copy link

kyrias commented Feb 16, 2024

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
Copy link
Owner

kilork commented Feb 16, 2024

@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.

@kilork kilork linked a pull request Apr 7, 2024 that will close this issue
@kilork kilork closed this as completed Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants